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

[Chi Zhiling]

On 1/4/26 15:56, Yuezhang.Mo@xxxxxxxx wrote:
> > On 12/30/25 17:05, Yuezhang.Mo@xxxxxxxx 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;
> > > >    }