RE: [PATCH v1 2/2] exfat: hint the empty entry which at the end of cluster chain

["Sungjong Seo" <sj1557.seo@samsung.com>]

Hi, Yuezhang Mo,

> After traversing all directory entries, hint the empty directory
> entry no matter whether or not there are enough empty directory
> entries.
> 
> After this commit, hint the empty directory entries like this:
> 
> 1. Hint the deleted directory entries if enough;
> 2. Hint the deleted and unused directory entries which at the
>    end of the cluster chain no matter whether enough or not(Add
>    by this commit);
> 3. If no any empty directory entries, hint the empty directory
>    entries in the new cluster(Add by this commit).
> 
> This avoids repeated traversal of directory entries, reduces CPU
> usage, and improves the performance of creating files and
> directories(especially on low-performance CPUs).
> 
> Test create 5000 files in a class 4 SD card on imx6q-sabrelite
> with:
> 
> for ((i=0;i<5;i++)); do
>    sync
>    time (for ((j=1;j<=1000;j++)); do touch file$((i*1000+j)); done)
> done
> 
> The more files, the more performance improvements.
> 
>             Before   After    Improvement
>    1~1000   25.360s  22.168s  14.40%
> 1001~2000   38.242s  28.72ss  33.15%
> 2001~3000   49.134s  35.037s  40.23%
> 3001~4000   62.042s  41.624s  49.05%
> 4001~5000   73.629s  46.772s  57.42%
> 
> Signed-off-by: Yuezhang Mo <Yuezhang.Mo@sony.com>
> Reviewed-by: Andy Wu <Andy.Wu@sony.com>
> Reviewed-by: Aoyama Wataru <wataru.aoyama@sony.com>
> ---
>  fs/exfat/dir.c   | 26 ++++++++++++++++++++++----
>  fs/exfat/namei.c | 22 ++++++++++++++--------
>  2 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
> index a569f285f4fd..7600f3521246 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -936,18 +936,25 @@ struct exfat_entry_set_cache
> *exfat_get_dentry_set(struct super_block *sb,
> 
>  static inline void exfat_hint_empty_entry(struct exfat_inode_info *ei,
>  		struct exfat_hint_femp *candi_empty, struct exfat_chain *clu,
> -		int dentry, int num_entries)
> +		int dentry, int num_entries, int entry_type)
>  {
>  	if (ei->hint_femp.eidx == EXFAT_HINT_NONE ||
>  	    ei->hint_femp.count < num_entries ||
>  	    ei->hint_femp.eidx > dentry) {
> +		int total_entries = EXFAT_B_TO_DEN(i_size_read(&ei-
> >vfs_inode));
> +
>  		if (candi_empty->count == 0) {
>  			candi_empty->cur = *clu;
>  			candi_empty->eidx = dentry;
>  		}
> 
> -		candi_empty->count++;
> -		if (candi_empty->count == num_entries)
> +		if (entry_type == TYPE_UNUSED)
> +			candi_empty->count += total_entries - dentry;

This seems like a very good approach. Perhaps the key fix that improved
performance seems to be the handling of cases where empty space was not
found and ended with TYPE_UNUSED.

However, there are concerns about trusting and using the number of free
entries after TYPE_UNUSED calculated based on directory size. This is
because, unlike exFAT Spec., in the real world, unexpected TYPE_UNUSED
entries may exist. :( 
That's why exfat_search_empty_slot() checks if there is any valid entry
after TYPE_UNUSED. In my experience, they can be caused by a wrong FS driver
and H/W defects, and the probability of occurrence is not low.

Therefore, when the lookup ends with TYPE_UNUSED, if there are no empty
entries found yet, it would be better to set the last empty entry to
hint_femp.eidx and set hint_femp.count to 0.
If so, even if the lookup ends with TYPE_UNUSED, exfat_search_empty_slot()
can start searching from the position of the last empty entry and check
whether there are actually empty entries as many as the required
num_entries as now.

what do you think?

> +		else
> +			candi_empty->count++;
> +
> +		if (candi_empty->count == num_entries ||
> +		    candi_empty->count + candi_empty->eidx == total_entries)
>  			ei->hint_femp = *candi_empty;
>  	}
>  }
[snip]
> --
> 2.25.1