lkml.org 
[lkml]   [2021]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] exfat: speed up iterate/lookup by fixing start point of traversing cluster chain
From
Date
Hi,
On 3/19/21 6:53 PM, Sungjong Seo wrote:
>> When directory iterate and lookup is called, there's a buggy rewinding of
>> start point for traversing cluster chain to the parent directory entry's
>> first cluster. This caused repeated cluster chain traversing from the
>> first entry of the parent directory that would show worse performance if
>> huge amounts of files exist under the parent directory.
>> Fix not to rewind, make continue from currently referenced cluster and dir
>> entry.
>>
>> Tested with 50,000 files under single directory / 256GB sdcard, with
>> command "time ls -l > /dev/null",
>> Before : 0m08.69s real 0m00.27s user 0m05.91s system
>> After : 0m07.01s real 0m00.25s user 0m04.34s system
>>
>> Signed-off-by: Hyeongseok Kim <hyeongseok@gmail.com>
>> ---
>> fs/exfat/dir.c | 39 ++++++++++++++++++++++++++++++++-------
>> fs/exfat/exfat_fs.h | 2 +-
>> fs/exfat/namei.c | 6 ++++--
>> 3 files changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
>> e1d5536de948..63f08987a8fe 100644
>> --- a/fs/exfat/dir.c
>> +++ b/fs/exfat/dir.c
>> @@ -147,7 +147,7 @@ static int exfat_readdir(struct inode *inode, loff_t
>> *cpos, struct exfat_dir_ent
> [snip]
>> + * @de: If p_uniname is found, filled with optimized dir/entry
>> + * for traversing cluster chain. Basically,
>> + * (p_dir.dir+return entry) and (de.dir.dir+de.entry) are
>> + * pointing the same physical directory entry, but if
>> + * caller needs to start to traverse cluster chain,
>> + * it's better option to choose the information in de.
>> + * Caller could only trust .dir and .entry field.
> exfat-fs has exfat_hint structure for keeping clusters and entries as hints.
> Of course, the caller, exfat_find(), should adjust exfat_chain with
> hint value just before calling exfat_get_dentry_set() as follows.
>
> /* adjust cdir to the optimized value */
> cdir.dir = hint_opt.clu;
> if (cdir.flag & ALLOC_NO_FAT_CHAIN) {
> cdir.size -= dentry / sbi->dentries_per_clu;
> dentry = hint_opt.eidx;
>
> What do you think about using it?
Agreed.
What I want to change here is very clear and any way to achieve the goal
would be good.
>> + * @return:
>> + * >= 0: file directory entry position where the name exists
>> + * -ENOENT: entry with the name does not exist
>> + * -EIO: I/O error
>> */
> [snip]
>> @@ -1070,11 +1081,14 @@ int exfat_find_dir_entry(struct super_block *sb,
>> struct exfat_inode_info *ei,
>> }
>>
>> if (clu.flags == ALLOC_NO_FAT_CHAIN) {
>> - if (--clu.size > 0)
>> + if (--clu.size > 0) {
>> + exfat_chain_dup(&de->dir, &clu);
> If you want to make a backup of the clu, it seems more appropriate to move
> exfat_chain_dup() right above the "if".
Yes, but we would not need this backup any more.
>
>> clu.dir++;
>> + }
>> else
>> clu.dir = EXFAT_EOF_CLUSTER;
>> } else {
>> + exfat_chain_dup(&de->dir, &clu);
>> if (exfat_get_next_cluster(sb, &clu.dir))
>> return -EIO;
>> }
>> @@ -1101,6 +1115,17 @@ int exfat_find_dir_entry(struct super_block *sb,
>> struct exfat_inode_info *ei,
>> return -ENOENT;
>>
>> found:
>> + /* set as dentry in cluster */
>> + de->entry = (dentry - num_ext) & (dentries_per_clu - 1);
>> + /*
>> + * if dentry_set spans to the next_cluster,
>> + * e.g. (de->entry + num_ext + 1 > dentries_per_clu)
>> + * current de->dir is correct which have previous cluster info,
>> + * but if it doesn't span as below, "clu" is correct, so update.
>> + */
>> + if (de->entry + num_ext + 1 <= dentries_per_clu)
>> + exfat_chain_dup(&de->dir, &clu);
>> +
> Let it be simple.
> 1. Keep an old value in the stack variable, when it found a FILE or DIR
> entry.
> 2. And just copy that here.
>
> There are more assignments, but I think its impact might be negligible.
> Thanks.
>
>
OK. I thought this routine is more straightforward to understand what is
being done here.
But I don't have any strong opinion about this, so I'll change in that way.
BTW, I think we can do this without local variable.

\
 
 \ /
  Last update: 2021-03-22 01:14    [W:0.059 / U:0.440 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site