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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <PUZPR04MB631665D55F4F0B290D0D543581379@PUZPR04MB6316.apcprd04.prod.outlook.com>
Date:   Mon, 31 Oct 2022 11:04:34 +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>,
        "Andy.Wu@...y.com" <Andy.Wu@...y.com>,
        "Wataru.Aoyama@...y.com" <Wataru.Aoyama@...y.com>
Subject: RE: [PATCH v1 1/2] exfat: simplify empty entry hint

> > 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@...nel.org>
> Sent: Monday, October 31, 2022 2:31 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 1/2] exfat: simplify empty entry hint
> 
> Add missing Cc: Yuezhang Mo.
> 
> 2022-10-31 14:16 GMT+09:00, Sungjong Seo <sj1557.seo@...sung.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@...y.com>
> >> Reviewed-by: Andy Wu <Andy.Wu@...y.com>
> >> Reviewed-by: Aoyama Wataru <wataru.aoyama@...y.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
> >
> >