Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <014c01d8ecf0$6e74bc80$4b5e3580$@samsung.com>
References: <CGME20221019072854epcas1p2a2b272458803045b4dfa95b17fb4f547@epcas1p2.samsung.com>
 <PUZPR04MB631604A0BBD29713D3F8DAB0812B9@PUZPR04MB6316.apcprd04.prod.outlook.com>
 <014c01d8ecf0$6e74bc80$4b5e3580$@samsung.com>
From:   Namjae Jeon <linkinjeon@kernel.org>
Date:   Mon, 31 Oct 2022 15:31:53 +0900
Message-ID: <CAKYAXd9omiOTAaAWSnzE5jCQFDL8Nkok_wm_OAYwxVpgcCxykg@mail.gmail.com>
Subject: Re: [PATCH v1 2/2] exfat: hint the empty entry which at the end of
 cluster chain
To:     Sungjong Seo <sj1557.seo@samsung.com>,
        Yuezhang Mo <Yuezhang.Mo@sony.com>
Cc:     linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Add missing Cc: Yuezhang Mo.

2022-10-31 15:17 GMT+09:00, 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
>
>