[PATCH 0/2] Small cleanup of ext[34] get_blocks function

[PATCH 0/2] Small cleanup of ext[34] get_blocks function

[Jan Kara]

  Hi,

  two patches below cleanup get_blocks() functions of ext3 and ext4 - they
remove parameter extend_disksize which was in fact unused. Ted, would you
merge the ext4 patch and Andrew, would you merge the ext3 patch? Thanks.

								Honza

[PATCH 1/2] ext3: Get rid of extenddisksize parameter of ext3_get_blocks_handle()

[Jan Kara]

Get rid of extenddisksize parameter of ext3_get_blocks_handle(). This seems to
be a relict from some old days and setting disksize in this function does not
make much sence. Currently it was set only by ext3_getblk().  Since the
parameter has some effect only if create == 1, it is easy to check that the
three callers which end up calling ext3_getblk() with create == 1 (ext3_append,
ext3_quota_write, ext3_mkdir) do the right thing and set disksize themselves.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext3/dir.c           |    3 +--
 fs/ext3/inode.c         |   13 +++----------
 include/linux/ext3_fs.h |    2 +-
 3 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/ext3/dir.c b/fs/ext3/dir.c
index 3d724a9..373fa90 100644
--- a/fs/ext3/dir.c
+++ b/fs/ext3/dir.c
@@ -130,8 +130,7 @@ static int ext3_readdir(struct file * filp,
 		struct buffer_head *bh = NULL;
 
 		map_bh.b_state = 0;
-		err = ext3_get_blocks_handle(NULL, inode, blk, 1,
-						&map_bh, 0, 0);
+		err = ext3_get_blocks_handle(NULL, inode, blk, 1, &map_bh, 0);
 		if (err > 0) {
 			pgoff_t index = map_bh.b_blocknr >>
 					(PAGE_CACHE_SHIFT - inode->i_blkbits);
diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index fcfa243..60f0feb 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -788,7 +788,7 @@ err_out:
 int ext3_get_blocks_handle(handle_t *handle, struct inode *inode,
 		sector_t iblock, unsigned long maxblocks,
 		struct buffer_head *bh_result,
-		int create, int extend_disksize)
+		int create)
 {
 	int err = -EIO;
 	int offsets[4];
@@ -911,13 +911,6 @@ int ext3_get_blocks_handle(handle_t *handle, struct inode *inode,
 	if (!err)
 		err = ext3_splice_branch(handle, inode, iblock,
 					partial, indirect_blks, count);
-	/*
-	 * i_disksize growing is protected by truncate_mutex.  Don't forget to
-	 * protect it if you're about to implement concurrent
-	 * ext3_get_block() -bzzz
-	*/
-	if (!err && extend_disksize && inode->i_size > ei->i_disksize)
-		ei->i_disksize = inode->i_size;
 	mutex_unlock(&ei->truncate_mutex);
 	if (err)
 		goto cleanup;
@@ -972,7 +965,7 @@ static int ext3_get_block(struct inode *inode, sector_t iblock,
 	}
 
 	ret = ext3_get_blocks_handle(handle, inode, iblock,
-					max_blocks, bh_result, create, 0);
+					max_blocks, bh_result, create);
 	if (ret > 0) {
 		bh_result->b_size = (ret << inode->i_blkbits);
 		ret = 0;
@@ -1005,7 +998,7 @@ struct buffer_head *ext3_getblk(handle_t *handle, struct inode *inode,
 	dummy.b_blocknr = -1000;
 	buffer_trace_init(&dummy.b_history);
 	err = ext3_get_blocks_handle(handle, inode, block, 1,
-					&dummy, create, 1);
+					&dummy, create);
 	/*
 	 * ext3_get_blocks_handle() returns number of blocks
 	 * mapped. 0 in case of a HOLE.
diff --git a/include/linux/ext3_fs.h b/include/linux/ext3_fs.h
index 634a5e5..7499b36 100644
--- a/include/linux/ext3_fs.h
+++ b/include/linux/ext3_fs.h
@@ -874,7 +874,7 @@ struct buffer_head * ext3_getblk (handle_t *, struct inode *, long, int, int *);
 struct buffer_head * ext3_bread (handle_t *, struct inode *, int, int, int *);
 int ext3_get_blocks_handle(handle_t *handle, struct inode *inode,
 	sector_t iblock, unsigned long maxblocks, struct buffer_head *bh_result,
-	int create, int extend_disksize);
+	int create);
 
 extern struct inode *ext3_iget(struct super_block *, unsigned long);
 extern int  ext3_write_inode (struct inode *, int);
-- 
1.6.0.2

[PATCH 2/2] ext4: Get rid of extend_disksize parameter of ext4_get_blocks_handle()

[Jan Kara]

Get rid of extenddisksize parameter of ext4_get_blocks_handle(). This seems to
be a relict from some old days and setting disksize in this function does not
make much sence. Currently it was set only by ext4_getblk().  Since the
parameter has some effect only if create == 1, it is easy to check that the
three callers which end up calling ext4_getblk() with create == 1 (ext4_append,
ext4_quota_write, ext4_mkdir) do the right thing and set disksize themselves.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/dir.c     |    3 +--
 fs/ext4/ext4.h    |    6 ++----
 fs/ext4/extents.c |   16 +++-------------
 fs/ext4/inode.c   |   43 ++++++++++++-------------------------------
 4 files changed, 18 insertions(+), 50 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index b647899..c8e0623 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -131,8 +131,7 @@ static int ext4_readdir(struct file *filp,
 		struct buffer_head *bh = NULL;
 
 		map_bh.b_state = 0;
-		err = ext4_get_blocks_wrap(NULL, inode, blk, 1, &map_bh,
-						0, 0, 0);
+		err = ext4_get_blocks_wrap(NULL, inode, blk, 1, &map_bh, 0, 0);
 		if (err > 0) {
 			pgoff_t index = map_bh.b_blocknr >>
 					(PAGE_CACHE_SHIFT - inode->i_blkbits);
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index d0f15ef..2b02ae0 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1338,8 +1338,7 @@ extern int ext4_ext_index_trans_blocks(struct inode *inode, int nrblocks,
 				       int chunk);
 extern int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 			       ext4_lblk_t iblock, unsigned int max_blocks,
-			       struct buffer_head *bh_result,
-			       int create, int extend_disksize);
+			       struct buffer_head *bh_result, int create);
 extern void ext4_ext_truncate(struct inode *);
 extern void ext4_ext_init(struct super_block *);
 extern void ext4_ext_release(struct super_block *);
@@ -1347,8 +1346,7 @@ extern long ext4_fallocate(struct inode *inode, int mode, loff_t offset,
 			  loff_t len);
 extern int ext4_get_blocks_wrap(handle_t *handle, struct inode *inode,
 			sector_t block, unsigned int max_blocks,
-			struct buffer_head *bh, int create,
-			int extend_disksize, int flag);
+			struct buffer_head *bh, int create, int flag);
 extern int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			__u64 start, __u64 len);
 
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e403321..59e9327 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2775,9 +2775,8 @@ fix_extent_len:
  * return < 0, error case.
  */
 int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
-			ext4_lblk_t iblock,
-			unsigned int max_blocks, struct buffer_head *bh_result,
-			int create, int extend_disksize)
+			ext4_lblk_t iblock, unsigned int max_blocks,
+			struct buffer_head *bh_result, int create)
 {
 	struct ext4_ext_path *path = NULL;
 	struct ext4_extent_header *eh;
@@ -2786,7 +2785,6 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 	int err = 0, depth, ret, cache_type;
 	unsigned int allocated = 0;
 	struct ext4_allocation_request ar;
-	loff_t disksize;
 
 	__clear_bit(BH_New, &bh_result->b_state);
 	ext_debug("blocks %u/%u requested for inode %u\n",
@@ -2974,14 +2972,6 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 	newblock = ext_pblock(&newex);
 	allocated = ext4_ext_get_actual_len(&newex);
 outnew:
-	if (extend_disksize) {
-		disksize = ((loff_t) iblock + ar.len) << inode->i_blkbits;
-		if (disksize > i_size_read(inode))
-			disksize = i_size_read(inode);
-		if (disksize > EXT4_I(inode)->i_disksize)
-			EXT4_I(inode)->i_disksize = disksize;
-	}
-
 	set_buffer_new(bh_result);
 
 	/* Cache only when it is _not_ an uninitialized extent */
@@ -3143,7 +3133,7 @@ retry:
 		}
 		ret = ext4_get_blocks_wrap(handle, inode, block,
 					  max_blocks, &map_bh,
-					  EXT4_CREATE_UNINITIALIZED_EXT, 0, 0);
+					  EXT4_CREATE_UNINITIALIZED_EXT, 0);
 		if (ret <= 0) {
 #ifdef EXT4FS_DEBUG
 			WARN_ON(ret <= 0);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c6bd6ce..7711968 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -916,8 +916,7 @@ err_out:
  */
 static int ext4_get_blocks_handle(handle_t *handle, struct inode *inode,
 				  ext4_lblk_t iblock, unsigned int maxblocks,
-				  struct buffer_head *bh_result,
-				  int create, int extend_disksize)
+				  struct buffer_head *bh_result, int create)
 {
 	int err = -EIO;
 	ext4_lblk_t offsets[4];
@@ -927,11 +926,8 @@ static int ext4_get_blocks_handle(handle_t *handle, struct inode *inode,
 	int indirect_blks;
 	int blocks_to_boundary = 0;
 	int depth;
-	struct ext4_inode_info *ei = EXT4_I(inode);
 	int count = 0;
 	ext4_fsblk_t first_block = 0;
-	loff_t disksize;
-
 
 	J_ASSERT(!(EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL));
 	J_ASSERT(handle != NULL || create == 0);
@@ -997,18 +993,6 @@ static int ext4_get_blocks_handle(handle_t *handle, struct inode *inode,
 	if (!err)
 		err = ext4_splice_branch(handle, inode, iblock,
 					partial, indirect_blks, count);
-	/*
-	 * i_disksize growing is protected by i_data_sem.  Don't forget to
-	 * protect it if you're about to implement concurrent
-	 * ext4_get_block() -bzzz
-	*/
-	if (!err && extend_disksize) {
-		disksize = ((loff_t) iblock + count) << inode->i_blkbits;
-		if (disksize > i_size_read(inode))
-			disksize = i_size_read(inode);
-		if (disksize > ei->i_disksize)
-			ei->i_disksize = disksize;
-	}
 	if (err)
 		goto cleanup;
 
@@ -1144,7 +1128,7 @@ static void ext4_da_update_reserve_space(struct inode *inode, int used)
  */
 int ext4_get_blocks_wrap(handle_t *handle, struct inode *inode, sector_t block,
 			unsigned int max_blocks, struct buffer_head *bh,
-			int create, int extend_disksize, int flag)
+			int create, int flag)
 {
 	int retval;
 
@@ -1157,10 +1141,10 @@ int ext4_get_blocks_wrap(handle_t *handle, struct inode *inode, sector_t block,
 	down_read((&EXT4_I(inode)->i_data_sem));
 	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) {
 		retval =  ext4_ext_get_blocks(handle, inode, block, max_blocks,
-				bh, 0, 0);
+				bh, 0);
 	} else {
 		retval = ext4_get_blocks_handle(handle,
-				inode, block, max_blocks, bh, 0, 0);
+				inode, block, max_blocks, bh, 0);
 	}
 	up_read((&EXT4_I(inode)->i_data_sem));
 
@@ -1200,10 +1184,10 @@ int ext4_get_blocks_wrap(handle_t *handle, struct inode *inode, sector_t block,
 	 */
 	if (EXT4_I(inode)->i_flags & EXT4_EXTENTS_FL) {
 		retval =  ext4_ext_get_blocks(handle, inode, block, max_blocks,
-				bh, create, extend_disksize);
+				bh, create);
 	} else {
 		retval = ext4_get_blocks_handle(handle, inode, block,
-				max_blocks, bh, create, extend_disksize);
+				max_blocks, bh, create);
 
 		if (retval > 0 && buffer_new(bh)) {
 			/*
@@ -1256,7 +1240,7 @@ int ext4_get_block(struct inode *inode, sector_t iblock,
 	}
 
 	ret = ext4_get_blocks_wrap(handle, inode, iblock,
-					max_blocks, bh_result, create, 0, 0);
+					max_blocks, bh_result, create, 0);
 	if (ret > 0) {
 		bh_result->b_size = (ret << inode->i_blkbits);
 		ret = 0;
@@ -1281,8 +1265,7 @@ struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
 	dummy.b_state = 0;
 	dummy.b_blocknr = -1000;
 	buffer_trace_init(&dummy.b_history);
-	err = ext4_get_blocks_wrap(handle, inode, block, 1,
-					&dummy, create, 1, 0);
+	err = ext4_get_blocks_wrap(handle, inode, block, 1, &dummy, create, 0);
 	/*
 	 * ext4_get_blocks_handle() returns number of blocks
 	 * mapped. 0 in case of a HOLE.
@@ -1989,7 +1972,7 @@ static int ext4_da_get_block_write(struct inode *inode, sector_t iblock,
 	handle = ext4_journal_current_handle();
 	BUG_ON(!handle);
 	ret = ext4_get_blocks_wrap(handle, inode, iblock, max_blocks,
-				   bh_result, create, 0, EXT4_DELALLOC_RSVED);
+				   bh_result, create, EXT4_DELALLOC_RSVED);
 	if (ret <= 0)
 		return ret;
 
@@ -2007,9 +1990,7 @@ static int ext4_da_get_block_write(struct inode *inode, sector_t iblock,
 	}
 
 	/*
-	 * Update on-disk size along with block allocation we don't
-	 * use 'extend_disksize' as size may change within already
-	 * allocated block -bzzz
+	 * Update on-disk size along with block allocation.
 	 */
 	disksize = ((loff_t) iblock + ret) << inode->i_blkbits;
 	if (disksize > i_size_read(inode))
@@ -2306,7 +2287,7 @@ static int ext4_da_get_block_prep(struct inode *inode, sector_t iblock,
 	 * preallocated blocks are unmapped but should treated
 	 * the same as allocated blocks.
 	 */
-	ret = ext4_get_blocks_wrap(NULL, inode, iblock, 1,  bh_result, 0, 0, 0);
+	ret = ext4_get_blocks_wrap(NULL, inode, iblock, 1,  bh_result, 0, 0);
 	if ((ret == 0) && !buffer_delay(bh_result)) {
 		/* the block isn't (pre)allocated yet, let's reserve space */
 		/*
@@ -2349,7 +2330,7 @@ static int ext4_normal_get_block_write(struct inode *inode, sector_t iblock,
 	 * so call get_block_wrap with create = 0
 	 */
 	ret = ext4_get_blocks_wrap(NULL, inode, iblock, max_blocks,
-				   bh_result, 0, 0, 0);
+				   bh_result, 0, 0);
 	if (ret > 0) {
 		bh_result->b_size = (ret << inode->i_blkbits);
 		ret = 0;
-- 
1.6.0.2

Re: [PATCH 2/2] ext4: Get rid of extend_disksize parameter of ext4_get_blocks_handle()

[Theodore Tso]

On Wed, May 20, 2009 at 07:25:34PM +0200, Jan Kara wrote:
> Get rid of extenddisksize parameter of ext4_get_blocks_handle(). This seems to
> be a relict from some old days and setting disksize in this function does not
> make much sence. Currently it was set only by ext4_getblk().  Since the

s/sence/sense/

> parameter has some effect only if create == 1, it is easy to check that the
> three callers which end up calling ext4_getblk() with create == 1 (ext4_append,
> ext4_quota_write, ext4_mkdir) do the right thing and set disksize themselves.

So this patch doesn't apply any more since I've done my own set of
cleanups to the ext4 code base, so extend_disksize is no longer a
separate parameter, but a bit flag (and ext4_get_blocks_wrap has been
renamed to a more sensible ext4_get_blocks).

More to the point, this removess the code which sets the disksize --
which I agree is funny that it is done in ext4_ext4_get_blocks() and
ext4_ind_get_blocks() --- but I don't see anything in your patch which
actually sets disksize in ext4_append(), ext4_quota_write(), or
ext4_mkdir().  Do none of these actually need disksize to be set?  If
so, the commit message should explain that.

If not, perhaps it would be better if the update of the disksize field
be done in ext4_getblk()?

(And the same question here applies to the ext3 patch; it seems to
remove the code which manages the i_disksize without replacing it
anywhere that I can see.)

						- Ted

Re: [PATCH 2/2] ext4: Get rid of extend_disksize parameter of ext4_get_blocks_handle()

[Jan Kara]

  Hi,

On Fri 22-05-09 00:57:34, Theodore Tso wrote:
> On Wed, May 20, 2009 at 07:25:34PM +0200, Jan Kara wrote:
> > Get rid of extenddisksize parameter of ext4_get_blocks_handle(). This seems to
> > be a relict from some old days and setting disksize in this function does not
> > make much sence. Currently it was set only by ext4_getblk().  Since the
> 
> s/sence/sense/
  Thanks.

> > parameter has some effect only if create == 1, it is easy to check that the
> > three callers which end up calling ext4_getblk() with create == 1 (ext4_append,
> > ext4_quota_write, ext4_mkdir) do the right thing and set disksize themselves.
> 
> So this patch doesn't apply any more since I've done my own set of
> cleanups to the ext4 code base, so extend_disksize is no longer a
> separate parameter, but a bit flag (and ext4_get_blocks_wrap has been
> renamed to a more sensible ext4_get_blocks).
> 
> More to the point, this removess the code which sets the disksize --
> which I agree is funny that it is done in ext4_ext4_get_blocks() and
> ext4_ind_get_blocks() --- but I don't see anything in your patch which
> actually sets disksize in ext4_append(), ext4_quota_write(), or
> ext4_mkdir().  Do none of these actually need disksize to be set?  If
> so, the commit message should explain that.
  All these functions already set i_disksize themselves. E.g. ext4_append()
does
bh = ext4_bread(handle, inode, *block, 1, err);
if (bh) {
         inode->i_size += inode->i_sb->s_blocksize;
         EXT4_I(inode)->i_disksize = inode->i_size;
         *err = ext4_journal_get_write_access(handle, bh);
...
  BTW: I've tried to mention this in the changelog but obviously I've
failed doing it clearly enough ;).

> If not, perhaps it would be better if the update of the disksize field
> be done in ext4_getblk()?
  I'd keep ext4_getblk() and ext4_bread() just being lowlevel functions
mapping / allocating a block. The caller is responsible for updating i_size
and i_disksize if it needs to be changed.
  I'll rediff the ext4 patch against your tree and resend it.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR