RE: [PATCH v2 3/6] exfat: use exfat_fat_walk helper to simplify fat entry walking

[Sungjong Seo]

Hi, Chi Zhiling,
> Replace the custom exfat_walk_fat_chain() function and open-coded FAT 
> chain walking logic with the exfat_fat_walk() helper across 
> exfat_find_location, __exfat_get_dentry_set, and exfat_map_cluster.
> 
> Signed-off-by: Chi Zhiling <chizhiling@xxxxxxxxxx>
> ---
>  fs/exfat/dir.c   | 39 +++------------------------------------
>  fs/exfat/inode.c | 11 ++---------
>  2 files changed, 5 insertions(+), 45 deletions(-)
> 
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 7619410d668e..cfc6f16a5fb2 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -562,38 +562,6 @@ int exfat_put_dentry_set(struct 
> exfat_entry_set_cache *es, int sync)
>  	return err;
>  }
> 
> -static int exfat_walk_fat_chain(struct super_block *sb,
> -		struct exfat_chain *p_dir, unsigned int byte_offset,
> -		unsigned int *clu)
> -{
> -	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> -	unsigned int clu_offset;
> -	unsigned int cur_clu;
> -
> -	clu_offset = EXFAT_B_TO_CLU(byte_offset, sbi);
> -	cur_clu = p_dir->dir;
> -
> -	if (p_dir->flags == ALLOC_NO_FAT_CHAIN) {
> -		cur_clu += clu_offset;
> -	} else {
> -		while (clu_offset > 0) {
> -			if (exfat_get_next_cluster(sb, &cur_clu))
> -				return -EIO;
> -			if (cur_clu == EXFAT_EOF_CLUSTER) {

The intentional exfat_fs_error() call for chain damage conditions is lost.
Instead of removing the error handling for this condition, it had better be
moved to exfat_find_location().

Other than this, the rest of the patchset looks excellent.
Thanks.

> -				exfat_fs_error(sb,
> -					"invalid dentry access beyond EOF
> (clu : %u, eidx : %d)",
> -					p_dir->dir,
> -					EXFAT_B_TO_DEN(byte_offset));
We lost 
> -				return -EIO;
> -			}
> -			clu_offset--;
> -		}
> -	}
> -
> -	*clu = cur_clu;
> -	return 0;
> -}
> -
>  static int exfat_find_location(struct super_block *sb, struct 
> exfat_chain *p_dir,
>  			       int entry, sector_t *sector, int *offset)  {
@@ -
> 603,7 +571,8 @@ static int exfat_find_location(struct super_block *sb, 
> struct exfat_chain *p_dir
> 
>  	off = EXFAT_DEN_TO_B(entry);
> 
> -	ret = exfat_walk_fat_chain(sb, p_dir, off, &clu);
> +	clu = p_dir->dir;
> +	ret = exfat_fat_walk(sb, &clu, EXFAT_B_TO_CLU(off, sbi), 
> +p_dir->flags);
>  	if (ret)
>  		return ret;
> 
> @@ -792,9 +761,7 @@ static int __exfat_get_dentry_set(struct 
> exfat_entry_set_cache *es,
>  		if (exfat_is_last_sector_in_cluster(sbi, sec)) {
>  			unsigned int clu = exfat_sector_to_cluster(sbi,
sec);
> 
> -			if (p_dir->flags == ALLOC_NO_FAT_CHAIN)
> -				clu++;
> -			else if (exfat_get_next_cluster(sb, &clu))
> +			if (exfat_fat_walk(sb, &clu, 1, p_dir->flags))
>  				goto put_es;
>  			sec = exfat_cluster_to_sector(sbi, clu);
>  		} else {
> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c index
> beb9ea7cca9f..817d9a135bb6 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -225,15 +225,8 @@ static int exfat_map_cluster(struct inode *inode, 
> unsigned int clu_offset,
>  		 * *clu = (the first cluster of the allocated chain) =>
>  		 * (the last cluster of ...)
>  		 */
> -		if (ei->flags == ALLOC_NO_FAT_CHAIN) {
> -			*clu += num_to_be_allocated - 1;
> -		} else {
> -			while (num_to_be_allocated > 1) {
> -				if (exfat_get_next_cluster(sb, clu))
> -					return -EIO;
> -				num_to_be_allocated--;
> -			}
> -		}
> +		if (exfat_fat_walk(sb, clu, num_to_be_allocated - 1, ei-
> >flags))
> +			return -EIO;
>  		*count = 1;
>  	}
> 
> --
> 2.43.0