[RFC PATCH] vfat:avoid unnecessary check

[RFC PATCH] vfat:avoid unnecessary check

[zhoumin]

Remove redundant and unreachable name check code in dir.c.

Remove flags check in fat_update_time since fat does not support
inode version.

Optimize fat_truncate_time to return a meaningful value, allowing
the removal of redundant inode checks in fat_update_time. This
ensures non-root inodes are validated only once.

Signed-off-by: zhoumin <teczm@foxmail.com>
---
 fs/fat/dir.c  | 12 ++++--------
 fs/fat/misc.c | 11 ++++-------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index acbec5bdd521..4f1be9939292 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -337,11 +337,11 @@ static int fat_parse_long(struct inode *dir, loff_t *pos,
 		if (ds->alias_checksum != alias_checksum)
 			goto parse_long;
 	}
-	if ((*de)->name[0] == DELETED_FLAG)
+	if (IS_FREE((*de)->name))
 		return PARSE_INVALID;
 	if ((*de)->attr == ATTR_EXT)
 		goto parse_long;
-	if (IS_FREE((*de)->name) || ((*de)->attr & ATTR_VOLUME))
+	if ((*de)->attr & ATTR_VOLUME)
 		return PARSE_INVALID;
 	if (fat_checksum((*de)->name) != alias_checksum)
 		*nr_slots = 0;
@@ -491,12 +491,10 @@ int fat_search_long(struct inode *inode, const unsigned char *name,
 			goto end_of_dir;
 parse_record:
 		nr_slots = 0;
-		if (de->name[0] == DELETED_FLAG)
+		if (IS_FREE(de->name))
 			continue;
 		if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))
 			continue;
-		if (de->attr != ATTR_EXT && IS_FREE(de->name))
-			continue;
 		if (de->attr == ATTR_EXT) {
 			int status = fat_parse_long(inode, &cpos, &bh, &de,
 						    &unicode, &nr_slots);
@@ -608,12 +606,10 @@ static int __fat_readdir(struct inode *inode, struct file *file,
 	 * need to parse long filename.
 	 */
 	if (isvfat && !short_only) {
-		if (de->name[0] == DELETED_FLAG)
+		if (IS_FREE(de->name))
 			goto record_end;
 		if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))
 			goto record_end;
-		if (de->attr != ATTR_EXT && IS_FREE(de->name))
-			goto record_end;
 	} else {
 		if ((de->attr & ATTR_VOLUME) || IS_FREE(de->name))
 			goto record_end;
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index c7a2d27120ba..41f6362a0428 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -335,7 +335,7 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
 		inode_set_mtime_to_ts(inode,
 				      inode_set_ctime_to_ts(inode, fat_truncate_mtime(sbi, now)));
 
-	return 0;
+	return 1;
 }
 EXPORT_SYMBOL_GPL(fat_truncate_time);
 
@@ -343,18 +343,15 @@ int fat_update_time(struct inode *inode, int flags)
 {
 	int dirty_flags = 0;
 
-	if (inode->i_ino == MSDOS_ROOT_INO)
-		return 0;
-
-	if (flags & (S_ATIME | S_CTIME | S_MTIME)) {
-		fat_truncate_time(inode, NULL, flags);
+	if (fat_truncate_time(inode, NULL, flags)) {
 		if (inode->i_sb->s_flags & SB_LAZYTIME)
 			dirty_flags |= I_DIRTY_TIME;
 		else
 			dirty_flags |= I_DIRTY_SYNC;
+
+		__mark_inode_dirty(inode, dirty_flags);
 	}
 
-	__mark_inode_dirty(inode, dirty_flags);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fat_update_time);
-- 
2.43.0

Re: [RFC PATCH] vfat:avoid unnecessary check

[OGAWA Hirofumi]

zhoumin <teczm@foxmail.com> writes:

> Remove redundant and unreachable name check code in dir.c.

Looks like you changed the logic, but no explanation.

> Remove flags check in fat_update_time since fat does not support
> inode version.
>
> Optimize fat_truncate_time to return a meaningful value, allowing
> the removal of redundant inode checks in fat_update_time. This
> ensures non-root inodes are validated only once.

Also changed the logic, you removed the check of flags.

The change may not have the issue, however please change and explain
more carefully.

Thanks.

> Signed-off-by: zhoumin <teczm@foxmail.com>
> ---
>  fs/fat/dir.c  | 12 ++++--------
>  fs/fat/misc.c | 11 ++++-------
>  2 files changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index acbec5bdd521..4f1be9939292 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -337,11 +337,11 @@ static int fat_parse_long(struct inode *dir, loff_t *pos,
>  		if (ds->alias_checksum != alias_checksum)
>  			goto parse_long;
>  	}
> -	if ((*de)->name[0] == DELETED_FLAG)
> +	if (IS_FREE((*de)->name))
>  		return PARSE_INVALID;
>  	if ((*de)->attr == ATTR_EXT)
>  		goto parse_long;
> -	if (IS_FREE((*de)->name) || ((*de)->attr & ATTR_VOLUME))
> +	if ((*de)->attr & ATTR_VOLUME)
>  		return PARSE_INVALID;
>  	if (fat_checksum((*de)->name) != alias_checksum)
>  		*nr_slots = 0;
> @@ -491,12 +491,10 @@ int fat_search_long(struct inode *inode, const unsigned char *name,
>  			goto end_of_dir;
>  parse_record:
>  		nr_slots = 0;
> -		if (de->name[0] == DELETED_FLAG)
> +		if (IS_FREE(de->name))
>  			continue;
>  		if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))
>  			continue;
> -		if (de->attr != ATTR_EXT && IS_FREE(de->name))
> -			continue;
>  		if (de->attr == ATTR_EXT) {
>  			int status = fat_parse_long(inode, &cpos, &bh, &de,
>  						    &unicode, &nr_slots);
> @@ -608,12 +606,10 @@ static int __fat_readdir(struct inode *inode, struct file *file,
>  	 * need to parse long filename.
>  	 */
>  	if (isvfat && !short_only) {
> -		if (de->name[0] == DELETED_FLAG)
> +		if (IS_FREE(de->name))
>  			goto record_end;
>  		if (de->attr != ATTR_EXT && (de->attr & ATTR_VOLUME))
>  			goto record_end;
> -		if (de->attr != ATTR_EXT && IS_FREE(de->name))
> -			goto record_end;
>  	} else {
>  		if ((de->attr & ATTR_VOLUME) || IS_FREE(de->name))
>  			goto record_end;
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index c7a2d27120ba..41f6362a0428 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -335,7 +335,7 @@ int fat_truncate_time(struct inode *inode, struct timespec64 *now, int flags)
>  		inode_set_mtime_to_ts(inode,
>  				      inode_set_ctime_to_ts(inode, fat_truncate_mtime(sbi, now)));
>  
> -	return 0;
> +	return 1;
>  }
>  EXPORT_SYMBOL_GPL(fat_truncate_time);
>  
> @@ -343,18 +343,15 @@ int fat_update_time(struct inode *inode, int flags)
>  {
>  	int dirty_flags = 0;
>  
> -	if (inode->i_ino == MSDOS_ROOT_INO)
> -		return 0;
> -
> -	if (flags & (S_ATIME | S_CTIME | S_MTIME)) {
> -		fat_truncate_time(inode, NULL, flags);
> +	if (fat_truncate_time(inode, NULL, flags)) {
>  		if (inode->i_sb->s_flags & SB_LAZYTIME)
>  			dirty_flags |= I_DIRTY_TIME;
>  		else
>  			dirty_flags |= I_DIRTY_SYNC;
> +
> +		__mark_inode_dirty(inode, dirty_flags);
>  	}
>  
> -	__mark_inode_dirty(inode, dirty_flags);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(fat_update_time);

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [RFC PATCH] vfat:avoid unnecessary check

[zhoumin]

Hi Hirofumi

>> Remove redundant and unreachable name check code in dir.c.

> Looks like you changed the logic, but no explanation.

1. In fat_parse_long:
If (*de)->name[0] equals DELETED_FLAG, the function returns immediately.
Consequently, the subsequent IS_FREE check can never evaluate to true.
Therefore, retaining only the ATTR_VOLUME verification should be sufficient.

2. In fat_search_long:
If (*de)->name[0] equals DELETED_FLAG, the loop skips to the next iteration.
This makes the subsequent checks for IS_FREE and ATTR_EXT unreachable.These
checks should therefore be removed.

3. In __fat_readdir:
The same reasoning as in fat_search_long applies here.



>> Remove flags check in fat_update_time since fat does not support
>> inode version.
>>
>> Optimize fat_truncate_time to return a meaningful value, allowing
>> the removal of redundant inode checks in fat_update_time. This
>> ensures non-root inodes are validated only once.

> Also changed the logic, you removed the check of flags.

Changing the return value of fat_truncate_time and removing the ino check in 
fat_update_time is a minor optimization, as mentioned in my previous patch email.

The reason for removing the flags check is that the enum file_time_flags has
only four values. Since vfat does not support SB_I_VERSION, higher-level
functions such as inode_needs_update_time or inode_update_timestamps will never
set flags with S_VERSION. Thus, checking the flags is unnecessary.

Note that __mark_inode_dirty will not be called only for the root inode. This
logic remains consistent with the previous version.


Thanks,

zhoumin

Re: [RFC PATCH] vfat:avoid unnecessary check

[OGAWA Hirofumi]

zhoumin <teczm@foxmail.com> writes:

>>> Remove redundant and unreachable name check code in dir.c.
>
>> Looks like you changed the logic, but no explanation.
>
> 1. In fat_parse_long:
> If (*de)->name[0] equals DELETED_FLAG, the function returns immediately.
> Consequently, the subsequent IS_FREE check can never evaluate to true.
> Therefore, retaining only the ATTR_VOLUME verification should be sufficient.
>
> 2. In fat_search_long:
> If (*de)->name[0] equals DELETED_FLAG, the loop skips to the next iteration.
> This makes the subsequent checks for IS_FREE and ATTR_EXT unreachable.These
> checks should therefore be removed.
>
> 3. In __fat_readdir:
> The same reasoning as in fat_search_long applies here.

Hm, IS_FREE() checks 0 and DELETED_FLAG, isn't it?

>>> Remove flags check in fat_update_time since fat does not support
>>> inode version.
>>>
>>> Optimize fat_truncate_time to return a meaningful value, allowing
>>> the removal of redundant inode checks in fat_update_time. This
>>> ensures non-root inodes are validated only once.
>
>> Also changed the logic, you removed the check of flags.
>
> Changing the return value of fat_truncate_time and removing the ino check in 
> fat_update_time is a minor optimization, as mentioned in my previous patch email.
>
> The reason for removing the flags check is that the enum file_time_flags has
> only four values. Since vfat does not support SB_I_VERSION, higher-level
> functions such as inode_needs_update_time or inode_update_timestamps will never
> set flags with S_VERSION. Thus, checking the flags is unnecessary.
>
> Note that __mark_inode_dirty will not be called only for the root inode. This
> logic remains consistent with the previous version.

OK, thanks. I got the reason.

However I would prefer to keep the current code, for readability and
future changes, and explicitly check those time flags if there is no
measurable improvement.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [RFC PATCH] vfat:avoid unnecessary check

[zhoumin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 630 bytes --]

Hi Hirofumi

> Hm, IS_FREE() checks 0 and DELETED_FLAG, isn't it?

Yes, you're absolutely right. That was my mistake—sorry about that. Please
disregard that part.

> OK, thanks. I got the reason.

> However I would prefer to keep the current code, for readability and
> future changes, and explicitly check those time flags if there is no
> measurable improvement.

I understand your preference. That said, since fat_truncate_time already checks
the flags and handles them appropriately, I still feel the double-check might be
redundant. That said, I respect your decision and am okay with keeping it as is.


Thanks,

zhoumin