* RE: [PATCH v1 1/2] exfat: simplify empty entry hint
[not found] ` <PUZPR04MB6316EBE97C82DFBEFE3CCDAF812B9@PUZPR04MB6316.apcprd04.prod.outlook.com>
@ 2022-10-31 5:16 ` Sungjong Seo
2022-10-31 6:30 ` Namjae Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Sungjong Seo @ 2022-10-31 5:16 UTC (permalink / raw)
To: 'Namjae Jeon'
Cc: 'linux-fsdevel', 'linux-kernel', sj1557.seo
Hello, Yuezhang Mo,
> This commit adds exfat_hint_empty_entry() to reduce code complexity and
> make code more readable.
>
> 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 | 56 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 7b648b6662f0..a569f285f4fd 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -934,6 +934,24 @@ struct exfat_entry_set_cache
> *exfat_get_dentry_set(struct super_block *sb,
> return NULL;
> }
>
> +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)
> +{
> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE ||
> + ei->hint_femp.count < num_entries ||
It seems like a good approach.
BTW, ei->hint_femp.count was already reset at the beginning of
exfat_find_dir_entry(). So condition-check above could be removed.
Is there any scenario I'm missing?
> + ei->hint_femp.eidx > dentry) {
> + if (candi_empty->count == 0) {
> + candi_empty->cur = *clu;
> + candi_empty->eidx = dentry;
> + }
> +
> + candi_empty->count++;
> + if (candi_empty->count == num_entries)
> + ei->hint_femp = *candi_empty;
> + }
> +}
> +
> enum {
> DIRENT_STEP_FILE,
> DIRENT_STEP_STRM,
> @@ -958,7 +976,7 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei, {
> int i, rewind = 0, dentry = 0, end_eidx = 0, num_ext = 0, len;
> int order, step, name_len = 0;
> - int dentries_per_clu, num_empty = 0;
> + int dentries_per_clu;
> unsigned int entry_type;
> unsigned short *uniname = NULL;
> struct exfat_chain clu;
> @@ -976,7 +994,15 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei,
> end_eidx = dentry;
> }
>
> - candi_empty.eidx = EXFAT_HINT_NONE;
> + if (ei->hint_femp.eidx != EXFAT_HINT_NONE &&
> + ei->hint_femp.count < num_entries)
> + ei->hint_femp.eidx = EXFAT_HINT_NONE;
> +
> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE)
> + ei->hint_femp.count = 0;
> +
> + candi_empty = ei->hint_femp;
> +
It would be nice to make the code block above a static inline function as well.
> rewind:
> order = 0;
> step = DIRENT_STEP_FILE;
[snip]
> --
> 2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/2] exfat: simplify empty entry hint
2022-10-31 5:16 ` [PATCH v1 1/2] exfat: simplify empty entry hint Sungjong Seo
@ 2022-10-31 6:30 ` Namjae Jeon
2022-10-31 11:04 ` Yuezhang.Mo
0 siblings, 1 reply; 4+ messages in thread
From: Namjae Jeon @ 2022-10-31 6:30 UTC (permalink / raw)
To: Sungjong Seo, Yuezhang Mo; +Cc: linux-fsdevel, linux-kernel
Add missing Cc: Yuezhang Mo.
2022-10-31 14:16 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>:
> Hello, Yuezhang Mo,
>
>> This commit adds exfat_hint_empty_entry() to reduce code complexity and
>> make code more readable.
>>
>> 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 | 56 ++++++++++++++++++++++++++++----------------------
>> 1 file changed, 32 insertions(+), 24 deletions(-)
>>
>> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
>> 7b648b6662f0..a569f285f4fd 100644
>> --- a/fs/exfat/dir.c
>> +++ b/fs/exfat/dir.c
>> @@ -934,6 +934,24 @@ struct exfat_entry_set_cache
>> *exfat_get_dentry_set(struct super_block *sb,
>> return NULL;
>> }
>>
>> +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)
>> +{
>> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE ||
>> + ei->hint_femp.count < num_entries ||
>
> It seems like a good approach.
> BTW, ei->hint_femp.count was already reset at the beginning of
> exfat_find_dir_entry(). So condition-check above could be removed.
> Is there any scenario I'm missing?
>
>> + ei->hint_femp.eidx > dentry) {
>> + if (candi_empty->count == 0) {
>> + candi_empty->cur = *clu;
>> + candi_empty->eidx = dentry;
>> + }
>> +
>> + candi_empty->count++;
>> + if (candi_empty->count == num_entries)
>> + ei->hint_femp = *candi_empty;
>> + }
>> +}
>> +
>> enum {
>> DIRENT_STEP_FILE,
>> DIRENT_STEP_STRM,
>> @@ -958,7 +976,7 @@ int exfat_find_dir_entry(struct super_block *sb,
>> struct exfat_inode_info *ei, {
>> int i, rewind = 0, dentry = 0, end_eidx = 0, num_ext = 0, len;
>> int order, step, name_len = 0;
>> - int dentries_per_clu, num_empty = 0;
>> + int dentries_per_clu;
>> unsigned int entry_type;
>> unsigned short *uniname = NULL;
>> struct exfat_chain clu;
>> @@ -976,7 +994,15 @@ int exfat_find_dir_entry(struct super_block *sb,
>> struct exfat_inode_info *ei,
>> end_eidx = dentry;
>> }
>>
>> - candi_empty.eidx = EXFAT_HINT_NONE;
>> + if (ei->hint_femp.eidx != EXFAT_HINT_NONE &&
>> + ei->hint_femp.count < num_entries)
>> + ei->hint_femp.eidx = EXFAT_HINT_NONE;
>> +
>> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE)
>> + ei->hint_femp.count = 0;
>> +
>> + candi_empty = ei->hint_femp;
>> +
>
> It would be nice to make the code block above a static inline function as
> well.
>
>> rewind:
>> order = 0;
>> step = DIRENT_STEP_FILE;
> [snip]
>> --
>> 2.25.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v1 1/2] exfat: simplify empty entry hint
2022-10-31 6:30 ` Namjae Jeon
@ 2022-10-31 11:04 ` Yuezhang.Mo
2022-11-01 6:26 ` Sungjong Seo
0 siblings, 1 reply; 4+ messages in thread
From: Yuezhang.Mo @ 2022-10-31 11:04 UTC (permalink / raw)
To: Namjae Jeon, Sungjong Seo
Cc: linux-fsdevel, linux-kernel, Andy.Wu, Wataru.Aoyama
> > BTW, ei->hint_femp.count was already reset at the beginning of
> > exfat_find_dir_entry(). So condition-check above could be removed.
> > Is there any scenario I'm missing?
If the search does not start from the first entry and there are not enough empty entries.
This condition will be true when rewinding.
> >> - candi_empty.eidx = EXFAT_HINT_NONE;
> >> + if (ei->hint_femp.eidx != EXFAT_HINT_NONE &&
> >> + ei->hint_femp.count < num_entries)
> >> + ei->hint_femp.eidx = EXFAT_HINT_NONE;
> >> +
> >> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE)
> >> + ei->hint_femp.count = 0;
> >> +
> >> + candi_empty = ei->hint_femp;
> >> +
> >
> > It would be nice to make the code block above a static inline function
> > as well.
Since the code is called once only in exfat_find_dir_entry(), I didn't make a function for the code.
How about make function exfat_reset_empty_hint_if_not_enough() for this code?
The function name is a bit long☹, do you have a better idea?
Or maybe, we can add exfat_reset_empty_hint() and unconditionally reset ei->hint_femp in it.
> -----Original Message-----
> From: Namjae Jeon <linkinjeon@kernel.org>
> Sent: Monday, October 31, 2022 2:31 PM
> To: Sungjong Seo <sj1557.seo@samsung.com>; Mo, Yuezhang
> <Yuezhang.Mo@sony.com>
> Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>; linux-kernel
> <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v1 1/2] exfat: simplify empty entry hint
>
> Add missing Cc: Yuezhang Mo.
>
> 2022-10-31 14:16 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>:
> > Hello, Yuezhang Mo,
> >
> >> This commit adds exfat_hint_empty_entry() to reduce code complexity
> >> and make code more readable.
> >>
> >> 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 | 56
> >> ++++++++++++++++++++++++++++----------------------
> >> 1 file changed, 32 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> >> 7b648b6662f0..a569f285f4fd 100644
> >> --- a/fs/exfat/dir.c
> >> +++ b/fs/exfat/dir.c
> >> @@ -934,6 +934,24 @@ struct exfat_entry_set_cache
> >> *exfat_get_dentry_set(struct super_block *sb,
> >> return NULL;
> >> }
> >>
> >> +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)
> >> +{
> >> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE ||
> >> + ei->hint_femp.count < num_entries ||
> >
> > It seems like a good approach.
> > BTW, ei->hint_femp.count was already reset at the beginning of
> > exfat_find_dir_entry(). So condition-check above could be removed.
> > Is there any scenario I'm missing?
> >
> >> + ei->hint_femp.eidx > dentry) {
> >> + if (candi_empty->count == 0) {
> >> + candi_empty->cur = *clu;
> >> + candi_empty->eidx = dentry;
> >> + }
> >> +
> >> + candi_empty->count++;
> >> + if (candi_empty->count == num_entries)
> >> + ei->hint_femp = *candi_empty;
> >> + }
> >> +}
> >> +
> >> enum {
> >> DIRENT_STEP_FILE,
> >> DIRENT_STEP_STRM,
> >> @@ -958,7 +976,7 @@ int exfat_find_dir_entry(struct super_block *sb,
> >> struct exfat_inode_info *ei, {
> >> int i, rewind = 0, dentry = 0, end_eidx = 0, num_ext = 0, len;
> >> int order, step, name_len = 0;
> >> - int dentries_per_clu, num_empty = 0;
> >> + int dentries_per_clu;
> >> unsigned int entry_type;
> >> unsigned short *uniname = NULL;
> >> struct exfat_chain clu;
> >> @@ -976,7 +994,15 @@ int exfat_find_dir_entry(struct super_block *sb,
> >> struct exfat_inode_info *ei,
> >> end_eidx = dentry;
> >> }
> >>
> >> - candi_empty.eidx = EXFAT_HINT_NONE;
> >> + if (ei->hint_femp.eidx != EXFAT_HINT_NONE &&
> >> + ei->hint_femp.count < num_entries)
> >> + ei->hint_femp.eidx = EXFAT_HINT_NONE;
> >> +
> >> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE)
> >> + ei->hint_femp.count = 0;
> >> +
> >> + candi_empty = ei->hint_femp;
> >> +
> >
> > It would be nice to make the code block above a static inline function
> > as well.
> >
> >> rewind:
> >> order = 0;
> >> step = DIRENT_STEP_FILE;
> > [snip]
> >> --
> >> 2.25.1
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v1 1/2] exfat: simplify empty entry hint
2022-10-31 11:04 ` Yuezhang.Mo
@ 2022-11-01 6:26 ` Sungjong Seo
0 siblings, 0 replies; 4+ messages in thread
From: Sungjong Seo @ 2022-11-01 6:26 UTC (permalink / raw)
To: 'Namjae Jeon'
Cc: 'linux-fsdevel', 'linux-kernel', sj1557.seo
Hi, Yuezhang,
I am sorry that I cannot reply directly to you due to environmental restrictions.
> > > BTW, ei->hint_femp.count was already reset at the beginning of
> > > exfat_find_dir_entry(). So condition-check above could be removed.
> > > Is there any scenario I'm missing?
>
> If the search does not start from the first entry and there are not enough
> empty entries.
> This condition will be true when rewinding.
I didn't get what you said. do you mean "ei->hint_femp.eidx > dentry"?
Even so, it could be true, if the search does not start from the first entry
and there are "enough" empty entries.
Anyway, what I'm saying is "ei->hint_femp.count < num_entries", and hint_femp
Seems to be reset as EXFAT_HINT_NONE with 0 by below code.
+ if (ei->hint_femp.eidx != EXFAT_HINT_NONE &&
+ ei->hint_femp.count < num_entries)
+ ei->hint_femp.eidx = EXFAT_HINT_NONE;
+
+ if (ei->hint_femp.eidx == EXFAT_HINT_NONE)
+ ei->hint_femp.count = 0;
After then, ei->hint_femp can be updated only if there are enough free entries.
>
> > >> - candi_empty.eidx = EXFAT_HINT_NONE;
> > >> + if (ei->hint_femp.eidx != EXFAT_HINT_NONE &&
> > >> + ei->hint_femp.count < num_entries)
> > >> + ei->hint_femp.eidx = EXFAT_HINT_NONE;
> > >> +
> > >> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE)
> > >> + ei->hint_femp.count = 0;
> > >> +
> > >> + candi_empty = ei->hint_femp;
> > >> +
> > >
> > > It would be nice to make the code block above a static inline function
> > > as well.
>
> Since the code is called once only in exfat_find_dir_entry(), I didn't
> make a function for the code.
>
> How about make function exfat_reset_empty_hint_if_not_enough() for this
> code?
> The function name is a bit long☹, do you have a better idea?
>
> Or maybe, we can add exfat_reset_empty_hint() and unconditionally reset
> ei->hint_femp in it.
It's always difficult for me as well :).
What do you think of exfat_test_reset_empty_hint() or
exfat_cond_reset_empty_hint()?
> > -----Original Message-----
> > From: Namjae Jeon <linkinjeon@kernel.org>
> > Sent: Monday, October 31, 2022 2:31 PM
> > To: Sungjong Seo <sj1557.seo@samsung.com>; Mo, Yuezhang
> > <Yuezhang.Mo@sony.com>
> > Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>; linux-kernel
> > <linux-kernel@vger.kernel.org>
> > Subject: Re: [PATCH v1 1/2] exfat: simplify empty entry hint
> >
> > Add missing Cc: Yuezhang Mo.
> >
> > 2022-10-31 14:16 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>:
> > > Hello, Yuezhang Mo,
> > >
> > >> This commit adds exfat_hint_empty_entry() to reduce code complexity
> > >> and make code more readable.
> > >>
> > >> 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 | 56
> > >> ++++++++++++++++++++++++++++----------------------
> > >> 1 file changed, 32 insertions(+), 24 deletions(-)
> > >>
> > >> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > >> 7b648b6662f0..a569f285f4fd 100644
> > >> --- a/fs/exfat/dir.c
> > >> +++ b/fs/exfat/dir.c
> > >> @@ -934,6 +934,24 @@ struct exfat_entry_set_cache
> > >> *exfat_get_dentry_set(struct super_block *sb,
> > >> return NULL;
> > >> }
> > >>
> > >> +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)
> > >> +{
> > >> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE ||
> > >> + ei->hint_femp.count < num_entries ||
> > >
> > > It seems like a good approach.
> > > BTW, ei->hint_femp.count was already reset at the beginning of
> > > exfat_find_dir_entry(). So condition-check above could be removed.
> > > Is there any scenario I'm missing?
> > >
> > >> + ei->hint_femp.eidx > dentry) {
> > >> + if (candi_empty->count == 0) {
> > >> + candi_empty->cur = *clu;
> > >> + candi_empty->eidx = dentry;
> > >> + }
> > >> +
> > >> + candi_empty->count++;
> > >> + if (candi_empty->count == num_entries)
> > >> + ei->hint_femp = *candi_empty;
> > >> + }
> > >> +}
> > >> +
> > >> enum {
> > >> DIRENT_STEP_FILE,
> > >> DIRENT_STEP_STRM,
> > >> @@ -958,7 +976,7 @@ int exfat_find_dir_entry(struct super_block *sb,
> > >> struct exfat_inode_info *ei, {
> > >> int i, rewind = 0, dentry = 0, end_eidx = 0, num_ext = 0, len;
> > >> int order, step, name_len = 0;
> > >> - int dentries_per_clu, num_empty = 0;
> > >> + int dentries_per_clu;
> > >> unsigned int entry_type;
> > >> unsigned short *uniname = NULL;
> > >> struct exfat_chain clu;
> > >> @@ -976,7 +994,15 @@ int exfat_find_dir_entry(struct super_block *sb,
> > >> struct exfat_inode_info *ei,
> > >> end_eidx = dentry;
> > >> }
> > >>
> > >> - candi_empty.eidx = EXFAT_HINT_NONE;
> > >> + if (ei->hint_femp.eidx != EXFAT_HINT_NONE &&
> > >> + ei->hint_femp.count < num_entries)
> > >> + ei->hint_femp.eidx = EXFAT_HINT_NONE;
> > >> +
> > >> + if (ei->hint_femp.eidx == EXFAT_HINT_NONE)
> > >> + ei->hint_femp.count = 0;
> > >> +
> > >> + candi_empty = ei->hint_femp;
> > >> +
> > >
> > > It would be nice to make the code block above a static inline function
> > > as well.
> > >
> > >> rewind:
> > >> order = 0;
> > >> step = DIRENT_STEP_FILE;
> > > [snip]
> > >> --
> > >> 2.25.1
> > >
> > >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-01 6:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20221019072850epcas1p459b27e0d44eb0cc36ec09e9a734dcf60@epcas1p4.samsung.com>
[not found] ` <PUZPR04MB6316EBE97C82DFBEFE3CCDAF812B9@PUZPR04MB6316.apcprd04.prod.outlook.com>
2022-10-31 5:16 ` [PATCH v1 1/2] exfat: simplify empty entry hint Sungjong Seo
2022-10-31 6:30 ` Namjae Jeon
2022-10-31 11:04 ` Yuezhang.Mo
2022-11-01 6:26 ` Sungjong Seo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.