Re: [PATCH v1 9/9] exfat: support multi-cluster for exfat_get_cluster

[Chi Zhiling <chizhiling@163.com>]

On 1/4/26 15:56, Yuezhang.Mo@sony.com wrote:
>> On 12/30/25 17:06, Yuezhang.Mo@sony.com wrote:
>>>> +     /*
>>>> +      * Return on cache hit to keep the code simple.
>>>> +      */
>>>> +     if (fclus == cluster) {
>>>> +             *count = cid.fcluster + cid.nr_contig - fclus + 1;
>>>>                 return 0;
>>>
>>> If 'cid.fcluster + cid.nr_contig - fclus + 1 < *count', how about continuing to collect clusters?
>>> The following clusters may be continuous.
>>
>> I'm glad you noticed this detail. It is necessary to explain this and
>> update it in the code comments.
>>
>> The main reason why I didn't continue the collection was that the
>> subsequent clusters might also exist in the cache. This requires us to
>> search the cache again to confirm this, and this action might introduce
>> additional performance overhead.
>>
>> I think we can continue to collect, but we need to check the cache
>> before doing so.
>>
> 
> So we also need to check the cache in the following, right?

Uh, I don't think it's necessary in here, because these clusters won't 
exist in the cache.

In the cache_lru, all exfat_cache start from non-continuous clusters. 
This is because exfat_get_cluster adds consecutive clusters to the cache 
from left to right, which means that the left side of all caches is 
non-continuous.

For instance, if a file contains two extents,  [0,30] and [31,60], then 
exfat_cache must start at either 0 or 31, right?

When we have found a cache is [31, 45], then there won't be [41, 60] in 
the cache_lru.

So when we already get some head clusters of a continuous extent, the 
tail cluster will definitely not be present in the cache.


---

Here are some modifications regarding this patch (which may be reflected 
in the V2 version). Do you have any thoughts or suggestions on this?


diff --git a/fs/exfat/cache.c b/fs/exfat/cache.c
index 1ec531859944..8ff416beea3c 100644
--- a/fs/exfat/cache.c
+++ b/fs/exfat/cache.c
@@ -80,6 +80,10 @@ static inline void exfat_cache_update_lru(struct 
inode *inode,
                 list_move(&cache->cache_list, &ei->cache_lru);
  }

+/*
+ * Return fcluster of the cache which behind fclus, or
+ * EXFAT_EOF_CLUSTER if no cache in there.
+ */
  static bool exfat_cache_lookup(struct inode *inode,
                 unsigned int fclus, struct exfat_cache_id *cid,
                 unsigned int *cached_fclus, unsigned int *cached_dclus)
@@ -87,6 +91,7 @@ static bool exfat_cache_lookup(struct inode *inode,
         struct exfat_inode_info *ei = EXFAT_I(inode);
         static struct exfat_cache nohit = { .fcluster = 0, };
         struct exfat_cache *hit = &nohit, *p;
+       unsigned int next = EXFAT_EOF_CLUSTER;
         unsigned int offset;

         spin_lock(&ei->cache_lru_lock);
@@ -98,8 +103,9 @@ static bool exfat_cache_lookup(struct inode *inode,
                                 offset = hit->nr_contig;
                         } else {
                                 offset = fclus - hit->fcluster;
-                               break;
                         }
+               } else if (p->fcluster > fclus && p->fcluster < next) {
+                       next = p->fcluster;
                 }
         }
         if (hit != &nohit) {
@@ -114,7 +120,7 @@ static bool exfat_cache_lookup(struct inode *inode,
         }
         spin_unlock(&ei->cache_lru_lock);

-       return hit != &nohit;
+       return next;
  }

  static struct exfat_cache *exfat_cache_merge(struct inode *inode,
@@ -243,7 +249,7 @@ int exfat_get_cluster(struct inode *inode, unsigned 
int cluster,
         struct exfat_inode_info *ei = EXFAT_I(inode);
         struct buffer_head *bh = NULL;
         struct exfat_cache_id cid;
-       unsigned int content, fclus;
+       unsigned int content, fclus, next;
         unsigned int end = cluster + *count - 1;

         if (ei->start_clu == EXFAT_FREE_CLUSTER) {
@@ -272,14 +278,15 @@ int exfat_get_cluster(struct inode *inode, 
unsigned int cluster,
                 return 0;

         cache_init(&cid, fclus, *dclus);
-       exfat_cache_lookup(inode, cluster, &cid, &fclus, dclus);
+       next = exfat_cache_lookup(inode, cluster, &cid, &fclus, dclus);

-       /*
-        * Return on cache hit to keep the code simple.
-        */
         if (fclus == cluster) {
-               *count = cid.fcluster + cid.nr_contig - fclus + 1;
-               return 0;
+               /* The cache includes all cluster requested */
+               if (cid.fcluster + cid.nr_contig >= end)
+                       return 0;
+               /* No cache hole behind this cache */
+               if (next == cid.fcluster + cid.nr_contig + 1)
+                       return 0;
         }

         /*


Thanks,

> 
> ```
>          /*
>           * Collect the remaining clusters of this contiguous extent.
>           */
>          if (*dclus != EXFAT_EOF_CLUSTER) {
>                  unsigned int clu = *dclus;
> 
>                  /*
>                   * Now the cid cache contains the first cluster requested,
>                   * Advance the fclus to the last cluster of contiguous
>                   * extent, then update the count and cid cache accordingly.
>                   */
>                  while (fclus < end) {
>                          if (exfat_ent_get(sb, clu, &content, &bh))
>                                  goto err;
>                          if (++clu != content) {
>                                  /* TODO: read ahead if content valid */
>                                  break;
>                          }
>                          fclus++;
>                  }
>                  cid.nr_contig = fclus - cid.fcluster;
>                  *count = fclus - cluster + 1;
> ```
> 
>>>>
>>>> +             while (fclus < end) {
>>>> +                     if (exfat_ent_get(sb, clu, &content, &bh))
>>>> +                             goto err;
>>>> +                     if (++clu != content) {
>>>> +                             /* TODO: read ahead if content valid */
>>>> +                             break;
>>>
>>> The next cluster index has been read and will definitely be used.
>>> How about add it to the cache?
>>
>> Good idea!
>> will add it in v2,