RE: [PATCH v1 1/2] exfat: simplify empty entry hint

[Sungjong 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@xxxxxxxxxx>
> > Sent: Monday, October 31, 2022 2:31 PM
> > To: Sungjong Seo <sj1557.seo@xxxxxxxxxxx>; Mo, Yuezhang
> > <Yuezhang.Mo@xxxxxxxx>
> > Cc: linux-fsdevel <linux-fsdevel@xxxxxxxxxxxxxxx>; linux-kernel
> > <linux-kernel@xxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxx>:
> > > 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@xxxxxxxx>
> > >> Reviewed-by: Andy Wu <Andy.Wu@xxxxxxxx>
> > >> Reviewed-by: Aoyama Wataru <wataru.aoyama@xxxxxxxx>
> > >> ---
> > >>  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
> > >
> > >