Re: [PATCH v1 3/9] exfat: reuse cache to improve exfat_get_cluster

[Chi Zhiling <chizhiling@163.com>]

On 1/4/26 15:56, Yuezhang.Mo@sony.com wrote:
>> On 12/30/25 17:05, Yuezhang.Mo@sony.com wrote:
>>>> -             if (exfat_ent_get(sb, *dclus, &content, NULL))
>>>> -                     return -EIO;
>>>> +             if (exfat_ent_get(sb, *dclus, &content, &bh))
>>>> +                     goto err;
>>>
>>> As you commented,  the buffer_head needs release if no error return.
>>> Here, an error was returned, buffer_head had been released.
>>
>>
>> I mean, it seems like a duplicate release in there, but in fact it's not.
>>
>> When exfat_ent_get return an error, *bh is released and set to NULL. So
>> the second brelse() call in exfat_get_cluster() does nothing.
>>
>> ~~~
>> int exfat_ent_get(struct super_block *sb, unsigned int loc,
>>                   unsigned int *content, struct buffer_head **last)
>> {
>>
>>         ...
>>
>> err:
>>           if (last) {
>>                  brelse(*last);
>>
>>                  /* Avoid double release */
>>                  *last = NULL;
>>           }
>>           return -EIO;
>> }
>> ~~~
>>
>> The reason using "goto err" is that I want to handle all errors in the
>> same way. Although this does seem a bit strange and confusing with the
>> comment in exfat_ent_get.
> 
> I don't think it's necessary to handle all errors in the same way.
> 
> - Only the following error handling requires calling brelse.
>      /* prevent the infinite loop of cluster chain */
>      if (fclus > limit) {
>              exfat_fs_error(sb,
>                      "detected the cluster chain loop (i_pos %u)",
>                      fclus);
>              goto err;
>      }
> - This makes confused with the comment in exfat_ent_get.
> - Unnecessary code modifications are avoided.

Okay, V2 is ready:

diff --git a/fs/exfat/cache.c b/fs/exfat/cache.c
index 61af3fa05ab7..a5e6858e5a20 100644
--- a/fs/exfat/cache.c
+++ b/fs/exfat/cache.c
@@ -241,6 +241,7 @@ int exfat_get_cluster(struct inode *inode, unsigned 
int cluster,
         struct exfat_sb_info *sbi = EXFAT_SB(sb);
         unsigned int limit = sbi->num_clusters;
         struct exfat_inode_info *ei = EXFAT_I(inode);
+       struct buffer_head *bh = NULL;
         struct exfat_cache_id cid;
         unsigned int content;

@@ -284,10 +285,10 @@ int exfat_get_cluster(struct inode *inode, 
unsigned int cluster,
                         exfat_fs_error(sb,
                                 "detected the cluster chain loop (i_pos 
%u)",
                                 (*fclus));
-                       return -EIO;
+                       goto err;
                 }

-               if (exfat_ent_get(sb, *dclus, &content, NULL))
+               if (exfat_ent_get(sb, *dclus, &content, &bh))
                         return -EIO;

                 *last_dclus = *dclus;
@@ -299,7 +300,7 @@ int exfat_get_cluster(struct inode *inode, unsigned 
int cluster,
                                 exfat_fs_error(sb,
                                        "invalid cluster chain (i_pos 
%u, last_clus 0x%08x is EOF)",
                                        *fclus, (*last_dclus));
-                               return -EIO;
+                               goto err; // will remove in patch 6/9
                         }

                         break;
@@ -309,6 +310,10 @@ int exfat_get_cluster(struct inode *inode, unsigned 
int cluster,
                         cache_init(&cid, *fclus, *dclus);
         }

+       brelse(bh);
         exfat_cache_add(inode, &cid);
         return 0;
+err:
+       brelse(bh);
+       return -EIO;
  }


Thanks,

> 
>>>
>>>>
>>>>                 *last_dclus = *dclus;
>>>>                 *dclus = content;
>>>> @@ -299,7 +300,7 @@ int exfat_get_cluster(struct inode *inode, unsigned int cluster,
>>>>                                 exfat_fs_error(sb,
>>>>                                        "invalid cluster chain (i_pos %u, last_clus 0x%08x is EOF)",
>>>>                                        *fclus, (*last_dclus));
>>>> -                             return -EIO;
>>>> +                             goto err;
>>>>                         }
>>>>
>>>>                         break;
>>>> @@ -309,6 +310,10 @@ int exfat_get_cluster(struct inode *inode, unsigned int cluster,
>>>>                         cache_init(&cid, *fclus, *dclus);
>>>>         }
>>>>
>>>> +     brelse(bh);
>>>>         exfat_cache_add(inode, &cid);
>>>>         return 0;
>>>> +err:
>>>> +     brelse(bh);
>>>> +     return -EIO;
>>>>    }
>>>
>>>
> 
>