Re: [PATCH v9 03/13] exfat: add inode operations

[Christoph Hellwig <hch@lst.de>]

On Thu, Jan 02, 2020 at 04:20:26PM +0800, Namjae Jeon wrote:
> +#include "exfat_fs.h"
> +
> +/* 2-level option flag */
> +enum {
> +	BMAP_NOT_CREATE,
> +	BMAP_ADD_CLUSTER,
> +};

I looked at how this flag is used and found the get_block code a little
confusing.  Let me know what you think of the following untested patch to
streamline that area:


diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index c2b04537cb24..ccf9700c6a55 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -17,12 +17,6 @@
 #include "exfat_raw.h"
 #include "exfat_fs.h"
 
-/* 2-level option flag */
-enum {
-	BMAP_NOT_CREATE,
-	BMAP_ADD_CLUSTER,
-};
-
 static int __exfat_write_inode(struct inode *inode, int sync)
 {
 	int ret = -EIO;
@@ -298,109 +292,91 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
 	return 0;
 }
 
-static int exfat_bmap(struct inode *inode, sector_t sector, sector_t *phys,
-		unsigned long *mapped_blocks, int *create)
+static int exfat_map_new_buffer(struct exfat_inode_info *ei,
+		struct buffer_head *bh, loff_t pos)
 {
-	struct super_block *sb = inode->i_sb;
-	struct exfat_sb_info *sbi = EXFAT_SB(sb);
-	sector_t last_block;
-	unsigned int cluster, clu_offset, sec_offset;
-	int err = 0;
-
-	*phys = 0;
-	*mapped_blocks = 0;
-
-	last_block = EXFAT_B_TO_BLK_ROUND_UP(i_size_read(inode), sb);
-	if (sector >= last_block && *create == BMAP_NOT_CREATE)
-		return 0;
-
-	/* Is this block already allocated? */
-	clu_offset = sector >> sbi->sect_per_clus_bits;  /* cluster offset */
-
-	err = exfat_map_cluster(inode, clu_offset, &cluster,
-		*create & BMAP_ADD_CLUSTER);
-	if (err) {
-		if (err != -ENOSPC)
-			return -EIO;
-		return err;
-	}
-
-	if (cluster != EXFAT_EOF_CLUSTER) {
-		/* sector offset in cluster */
-		sec_offset = sector & (sbi->sect_per_clus - 1);
-
-		*phys = exfat_cluster_to_sector(sbi, cluster) + sec_offset;
-		*mapped_blocks = sbi->sect_per_clus - sec_offset;
-	}
+	if (buffer_delay(bh) && pos > ei->i_size_aligned)
+		return -EIO;
+	set_buffer_new(bh);
 
-	if (sector < last_block)
-		*create = BMAP_NOT_CREATE;
+	/*
+	 * Adjust i_size_aligned if i_size_ondisk is bigger than it.
+	 * (i.e. non-DA)
+	 */
+	if (ei->i_size_ondisk > ei->i_size_aligned)
+		ei->i_size_aligned = ei->i_size_ondisk;
 	return 0;
 }
 
 static int exfat_get_block(struct inode *inode, sector_t iblock,
 		struct buffer_head *bh_result, int create)
 {
+	struct exfat_inode_info *ei = EXFAT_I(inode);
 	struct super_block *sb = inode->i_sb;
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
 	unsigned long max_blocks = bh_result->b_size >> inode->i_blkbits;
 	int err = 0;
-	unsigned long mapped_blocks;
-	sector_t phys;
+	unsigned long mapped_blocks = 0;
+	unsigned int cluster, sec_offset;
+	sector_t last_block;
+	sector_t phys = 0;
 	loff_t pos;
-	int bmap_create = create ? BMAP_ADD_CLUSTER : BMAP_NOT_CREATE;
+	
+	mutex_lock(&sbi->s_lock);
+	last_block = EXFAT_B_TO_BLK_ROUND_UP(i_size_read(inode), sb);
+	if (iblock >= last_block && !create)
+		goto done;
 
-	mutex_lock(&EXFAT_SB(sb)->s_lock);
-	err = exfat_bmap(inode, iblock, &phys, &mapped_blocks, &bmap_create);
+	/* Is this block already allocated? */
+	err = exfat_map_cluster(inode, iblock >> sbi->sect_per_clus_bits,
+				&cluster, create);
 	if (err) {
-		if (err != -ENOSPC)
-			exfat_fs_error_ratelimit(sb,
-				"failed to bmap (inode : %p iblock : %llu, err : %d)",
-				inode, (unsigned long long)iblock, err);
+		if (err == -ENOSPC)
+			goto unlock_ret;
+
+		exfat_fs_error_ratelimit(sb,
+			"failed to bmap (inode : %p iblock : %llu, err : %d)",
+			inode, (unsigned long long)iblock, err);
 		goto unlock_ret;
 	}
 
-	if (phys) {
-		max_blocks = min(mapped_blocks, max_blocks);
-
-		/* Treat newly added block / cluster */
-		if (bmap_create || buffer_delay(bh_result)) {
-			/* Update i_size_ondisk */
-			pos = EXFAT_BLK_TO_B((iblock + 1), sb);
-			if (EXFAT_I(inode)->i_size_ondisk < pos)
-				EXFAT_I(inode)->i_size_ondisk = pos;
-
-			if (bmap_create) {
-				if (buffer_delay(bh_result) &&
-				    pos > EXFAT_I(inode)->i_size_aligned) {
-					exfat_fs_error(sb,
-						"requested for bmap out of range(pos : (%llu) > i_size_aligned(%llu)\n",
-						pos,
-						EXFAT_I(inode)->i_size_aligned);
-					err = -EIO;
-					goto unlock_ret;
-				}
-				set_buffer_new(bh_result);
-
-				/*
-				 * adjust i_size_aligned if i_size_ondisk is
-				 * bigger than it. (i.e. non-DA)
-				 */
-				if (EXFAT_I(inode)->i_size_ondisk >
-				    EXFAT_I(inode)->i_size_aligned) {
-					EXFAT_I(inode)->i_size_aligned =
-						EXFAT_I(inode)->i_size_ondisk;
-				}
-			}
+	if (cluster == EXFAT_EOF_CLUSTER)
+		goto done;
+
+	/* sector offset in cluster */
+	sec_offset = iblock & (sbi->sect_per_clus - 1);
+
+	phys = exfat_cluster_to_sector(sbi, cluster) + sec_offset;
+	mapped_blocks = sbi->sect_per_clus - sec_offset;
+	max_blocks = min(mapped_blocks, max_blocks);
 
-			if (buffer_delay(bh_result))
-				clear_buffer_delay(bh_result);
+	/* Treat newly added block / cluster */
+	if (iblock < last_block)
+		create = 0;
+
+	if (create || buffer_delay(bh_result)) {
+		pos = EXFAT_BLK_TO_B((iblock + 1), sb);
+		if (ei->i_size_ondisk < pos)
+			ei->i_size_ondisk = pos;
+	}
+
+	if (create) {
+		err = exfat_map_new_buffer(ei, bh_result, pos);
+		if (err) {
+			exfat_fs_error(sb,
+				"requested for bmap out of range(pos : (%llu) > i_size_aligned(%llu)\n",
+				pos, ei->i_size_aligned);
+			goto unlock_ret;
 		}
-		map_bh(bh_result, sb, phys);
 	}
 
+	if (buffer_delay(bh_result))
+		clear_buffer_delay(bh_result);
+	map_bh(bh_result, sb, phys);
+done:
 	bh_result->b_size = EXFAT_BLK_TO_B(max_blocks, sb);
 unlock_ret:
-	mutex_unlock(&EXFAT_SB(sb)->s_lock);
+	mutex_unlock(&sbi->s_lock);
 	return err;
 }