Hello Heinrich,

On Fri, Sep 17, 2021 at 5:36 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 9/16/21 4:09 PM, Oleksandr Suvorov wrote:

...

From: Ricardo Salveti ricardo@foundries.io

This patch optimizes the commit mentioned below by avoiding running a set of commands which useless in the case when

%s/which/which are/

...

size < mydata->sect_size and idx would be 0.

Thank you for reviewing the FAT coding.

...

Fixes: 5b3ddb17ba ("fs/fat/fat.c: Do not perform zero block reads if there are no blocks left")

Signed-off-by: Ricardo Salveti ricardo@foundries.io Co-developed-by: Oleksandr Suvorov oleksandr.suvorov@foundries.io Signed-off-by: Oleksandr Suvorov oleksandr.suvorov@foundries.io

---

fs/fat/fat.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 7021138b987..4a509755442 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -275,14 +275,10 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size) buffer += mydata->sect_size; size -= mydata->sect_size; }

• } else {
  

• } else if (size >= mydata->sect_size) {
          __u32 idx;
  

The variable name idx is a misnomer. Can we call it sect_count?

Ok, sure.

...

•         idx = size / mydata->sect_size;
  

•         if (idx == 0)
  

•                 ret = 0;
  

size could be 0. In this case we still have to set ret = 0. Both for the DMA aligned and the unaligned case. You could initialize ret = 0 where it is defined.

What for? There is no chance to use uninitialized "ret" in that function.

...

•         else
  

•                 ret = disk_read(startsect, idx, buffer);
  

•         ret = disk_read(startsect, idx, buffer);
          if (ret != idx) {
                  debug("Error reading data (got %d)\n", ret);
                  return -1;
  
  

291 idx *= mydata->sect_size; 292 buffer += idx; 293 size -= idx;

These lines should be adjusted, too: size is defined as unsigned long. We should not use the u32 variable idx but a separate variable of type unsigned long to hold the number of bytes read.

The original variant uses the same variable for a sectors counter and for the number of bytes read. I changed that part to use 2 different variables to make the code more readable. As for now, the maximum size of a cluster is 128 sectors (64Kb). This is the largest value for the parameter "size". And accordingly for the variables sect_count and bytes_read. IMHO even u16 would be enough for both these vars :) But the function disk_read() waits for __u32 as a 2nd parameter, so it would be more natural for sect_count to be __u32. What do you think?

Thanks for your detailed review!

I'm preparing the 2nd version of the patchset.

Best regards

Heinrich