| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 31 Oct 2022 11:04:47 +0000
From: "Yuezhang.Mo@...y.com" <Yuezhang.Mo@...y.com>
To: Namjae Jeon <linkinjeon@...nel.org>,
Sungjong Seo <sj1557.seo@...sung.com>
CC: linux-fsdevel <linux-fsdevel@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1 2/2] exfat: hint the empty entry which at the end of
cluster chain
> > 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?
We plan to add a new helper exfat_get_empty_dentry_set(), this helper is called before
setting the entry type, it caches and then checks for empty entries(Check-on-write is safer
than checking when looking for empty directory entries).
for (i = 0; i < es->num_entries; i++) {
ep = exfat_get_dentry_cached(es, i);
type = exfat_get_entry_type(ep);
if (type == TYPE_UNUSED)
unused_hit = true;
else if (type == TYPE_DELETED) {
if (unused_hit)
goto error;
} else
goto error;
}
This code is not ready, we are testing and internal reviewing.
> -----Original Message-----
> From: Namjae Jeon <linkinjeon@...nel.org>
> Sent: Monday, October 31, 2022 2:32 PM
> To: Sungjong Seo <sj1557.seo@...sung.com>; Mo, Yuezhang
> <Yuezhang.Mo@...y.com>
> Cc: linux-fsdevel <linux-fsdevel@...r.kernel.org>; linux-kernel
> <linux-kernel@...r.kernel.org>
> Subject: Re: [PATCH v1 2/2] exfat: hint the empty entry which at the end of
> cluster chain
>
> Add missing Cc: Yuezhang Mo.
>
> 2022-10-31 15:17 GMT+09:00, Sungjong Seo <sj1557.seo@...sung.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@...y.com>
> >> Reviewed-by: Andy Wu <Andy.Wu@...y.com>
> >> Reviewed-by: Aoyama Wataru <wataru.aoyama@...y.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
> >
> >
Powered by blists - more mailing lists