[PATCH 0/7] ext4 code simplification and clean ups

[PATCH 0/7] ext4 code simplification and clean ups

[Theodore Ts'o]

I posted an earlier version of the first couple of patches while we were
stablizing ext4 during the last merge window.  I've since done some
further cleanup and rebased these patches v3.9-rc4.

I've also added Zheng's recent patch to clean up ext4_write_end()'s.

Finally, I've added some debugging annotations to make it easier to find
bugs when doing future development work.

Theodore Ts'o (6):
  ext4: collapse handling of data=ordered and data=writeback codepaths
  ext4: fold ext4_alloc_blocks() in ext4_alloc_branch()
  ext4: refactor punch hole code
  ext4: refactor truncate code
  ext4: add mutex_is_locked() assertion to ext4_truncate()
  ext4: add might_sleep() annotations

Zheng Liu (1):
  ext4: fold ext4_generic_write_end() into ext4_write_end()

 fs/ext4/ext4.h              |  12 +-
 fs/ext4/ext4_jbd2.c         |   6 +
 fs/ext4/extents.c           | 245 +----------------------
 fs/ext4/indirect.c          | 473 +++++---------------------------------------
 fs/ext4/inode.c             | 433 ++++++++++++++++++++++++++++------------
 fs/ext4/mballoc.c           |   4 +
 include/trace/events/ext4.h |  10 +-
 7 files changed, 376 insertions(+), 807 deletions(-)

-- 
1.7.12.rc0.22.gcdd159b

[PATCH 1/7] ext4: collapse handling of data=ordered and data=writeback codepaths

[Theodore Ts'o]

The only difference between how we handle data=ordered and
data=writeback is a single call to ext4_jbd2_file_inode().  Eliminate
code duplication by factoring out redundant the code paths.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/ext4.h              |   1 +
 fs/ext4/inode.c             | 125 ++++++++++----------------------------------
 include/trace/events/ext4.h |  10 +---
 3 files changed, 31 insertions(+), 105 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 3b83cd6..f91e11b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1374,6 +1374,7 @@ enum {
 	EXT4_STATE_DIOREAD_LOCK,	/* Disable support for dio read
 					   nolocking */
 	EXT4_STATE_MAY_INLINE_DATA,	/* may have in-inode data */
+	EXT4_STATE_ORDERED_MODE,	/* data=ordered mode */
 };
 
 #define EXT4_INODE_BIT_FNS(name, field, offset)				\
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b3a5213..4ee6927 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1145,77 +1145,36 @@ static int ext4_generic_write_end(struct file *file,
  * ext4 never places buffers on inode->i_mapping->private_list.  metadata
  * buffers are managed internally.
  */
-static int ext4_ordered_write_end(struct file *file,
-				  struct address_space *mapping,
-				  loff_t pos, unsigned len, unsigned copied,
-				  struct page *page, void *fsdata)
+static int ext4_write_end(struct file *file,
+			  struct address_space *mapping,
+			  loff_t pos, unsigned len, unsigned copied,
+			  struct page *page, void *fsdata)
 {
 	handle_t *handle = ext4_journal_current_handle();
 	struct inode *inode = mapping->host;
 	int ret = 0, ret2;
 
-	trace_ext4_ordered_write_end(inode, pos, len, copied);
-	ret = ext4_jbd2_file_inode(handle, inode);
-
-	if (ret == 0) {
-		ret2 = ext4_generic_write_end(file, mapping, pos, len, copied,
-							page, fsdata);
-		copied = ret2;
-		if (pos + len > inode->i_size && ext4_can_truncate(inode))
-			/* if we have allocated more blocks and copied
-			 * less. We will have blocks allocated outside
-			 * inode->i_size. So truncate them
-			 */
-			ext4_orphan_add(handle, inode);
-		if (ret2 < 0)
-			ret = ret2;
-	} else {
-		unlock_page(page);
-		page_cache_release(page);
-	}
-
-	ret2 = ext4_journal_stop(handle);
-	if (!ret)
-		ret = ret2;
-
-	if (pos + len > inode->i_size) {
-		ext4_truncate_failed_write(inode);
-		/*
-		 * If truncate failed early the inode might still be
-		 * on the orphan list; we need to make sure the inode
-		 * is removed from the orphan list in that case.
-		 */
-		if (inode->i_nlink)
-			ext4_orphan_del(NULL, inode);
+	trace_ext4_write_end(inode, pos, len, copied);
+	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
+		ret = ext4_jbd2_file_inode(handle, inode);
+		if (ret) {
+			unlock_page(page);
+			page_cache_release(page);
+			goto errout;
+		}
 	}
 
-
-	return ret ? ret : copied;
-}
-
-static int ext4_writeback_write_end(struct file *file,
-				    struct address_space *mapping,
-				    loff_t pos, unsigned len, unsigned copied,
-				    struct page *page, void *fsdata)
-{
-	handle_t *handle = ext4_journal_current_handle();
-	struct inode *inode = mapping->host;
-	int ret = 0, ret2;
-
-	trace_ext4_writeback_write_end(inode, pos, len, copied);
-	ret2 = ext4_generic_write_end(file, mapping, pos, len, copied,
-							page, fsdata);
-	copied = ret2;
+	copied = ext4_generic_write_end(file, mapping, pos, len, copied,
+					page, fsdata);
+	if (copied < 0)
+		ret = copied;
 	if (pos + len > inode->i_size && ext4_can_truncate(inode))
 		/* if we have allocated more blocks and copied
 		 * less. We will have blocks allocated outside
 		 * inode->i_size. So truncate them
 		 */
 		ext4_orphan_add(handle, inode);
-
-	if (ret2 < 0)
-		ret = ret2;
-
+errout:
 	ret2 = ext4_journal_stop(handle);
 	if (!ret)
 		ret = ret2;
@@ -2818,18 +2777,9 @@ static int ext4_da_write_end(struct file *file,
 	unsigned long start, end;
 	int write_mode = (int)(unsigned long)fsdata;
 
-	if (write_mode == FALL_BACK_TO_NONDELALLOC) {
-		switch (ext4_inode_journal_mode(inode)) {
-		case EXT4_INODE_ORDERED_DATA_MODE:
-			return ext4_ordered_write_end(file, mapping, pos,
-					len, copied, page, fsdata);
-		case EXT4_INODE_WRITEBACK_DATA_MODE:
-			return ext4_writeback_write_end(file, mapping, pos,
-					len, copied, page, fsdata);
-		default:
-			BUG();
-		}
-	}
+	if (write_mode == FALL_BACK_TO_NONDELALLOC)
+		return ext4_write_end(file, mapping, pos,
+				      len, copied, page, fsdata);
 
 	trace_ext4_da_write_end(inode, pos, len, copied);
 	start = pos & (PAGE_CACHE_SIZE - 1);
@@ -3334,27 +3284,12 @@ static int ext4_journalled_set_page_dirty(struct page *page)
 	return __set_page_dirty_nobuffers(page);
 }
 
-static const struct address_space_operations ext4_ordered_aops = {
+static const struct address_space_operations ext4_aops = {
 	.readpage		= ext4_readpage,
 	.readpages		= ext4_readpages,
 	.writepage		= ext4_writepage,
 	.write_begin		= ext4_write_begin,
-	.write_end		= ext4_ordered_write_end,
-	.bmap			= ext4_bmap,
-	.invalidatepage		= ext4_invalidatepage,
-	.releasepage		= ext4_releasepage,
-	.direct_IO		= ext4_direct_IO,
-	.migratepage		= buffer_migrate_page,
-	.is_partially_uptodate  = block_is_partially_uptodate,
-	.error_remove_page	= generic_error_remove_page,
-};
-
-static const struct address_space_operations ext4_writeback_aops = {
-	.readpage		= ext4_readpage,
-	.readpages		= ext4_readpages,
-	.writepage		= ext4_writepage,
-	.write_begin		= ext4_write_begin,
-	.write_end		= ext4_writeback_write_end,
+	.write_end		= ext4_write_end,
 	.bmap			= ext4_bmap,
 	.invalidatepage		= ext4_invalidatepage,
 	.releasepage		= ext4_releasepage,
@@ -3399,23 +3334,21 @@ void ext4_set_aops(struct inode *inode)
 {
 	switch (ext4_inode_journal_mode(inode)) {
 	case EXT4_INODE_ORDERED_DATA_MODE:
-		if (test_opt(inode->i_sb, DELALLOC))
-			inode->i_mapping->a_ops = &ext4_da_aops;
-		else
-			inode->i_mapping->a_ops = &ext4_ordered_aops;
+		ext4_set_inode_state(inode, EXT4_STATE_ORDERED_MODE);
 		break;
 	case EXT4_INODE_WRITEBACK_DATA_MODE:
-		if (test_opt(inode->i_sb, DELALLOC))
-			inode->i_mapping->a_ops = &ext4_da_aops;
-		else
-			inode->i_mapping->a_ops = &ext4_writeback_aops;
+		ext4_clear_inode_state(inode, EXT4_STATE_ORDERED_MODE);
 		break;
 	case EXT4_INODE_JOURNAL_DATA_MODE:
 		inode->i_mapping->a_ops = &ext4_journalled_aops;
-		break;
+		return;
 	default:
 		BUG();
 	}
+	if (test_opt(inode->i_sb, DELALLOC))
+		inode->i_mapping->a_ops = &ext4_da_aops;
+	else
+		inode->i_mapping->a_ops = &ext4_aops;
 }
 
 
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index 4ee4710..58459b7 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -257,15 +257,7 @@ DECLARE_EVENT_CLASS(ext4__write_end,
 		  __entry->pos, __entry->len, __entry->copied)
 );
 
-DEFINE_EVENT(ext4__write_end, ext4_ordered_write_end,
-
-	TP_PROTO(struct inode *inode, loff_t pos, unsigned int len,
-		 unsigned int copied),
-
-	TP_ARGS(inode, pos, len, copied)
-);

Re: [PATCH 1/7] ext4: collapse handling of data=ordered and data=writeback codepaths

[Lukáš Czerner]

On Sun, 24 Mar 2013, Theodore Ts'o wrote:

> Date: Sun, 24 Mar 2013 20:06:48 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Subject: [PATCH 1/7] ext4: collapse handling of data=ordered and
>     data=writeback codepaths
> 
> The only difference between how we handle data=ordered and
> data=writeback is a single call to ext4_jbd2_file_inode().  Eliminate
> code duplication by factoring out redundant the code paths.

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

Thanks!
-Lukas

> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/ext4.h              |   1 +
>  fs/ext4/inode.c             | 125 ++++++++++----------------------------------
>  include/trace/events/ext4.h |  10 +---
>  3 files changed, 31 insertions(+), 105 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 3b83cd6..f91e11b 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1374,6 +1374,7 @@ enum {
>  	EXT4_STATE_DIOREAD_LOCK,	/* Disable support for dio read
>  					   nolocking */
>  	EXT4_STATE_MAY_INLINE_DATA,	/* may have in-inode data */
> +	EXT4_STATE_ORDERED_MODE,	/* data=ordered mode */
>  };
>  
>  #define EXT4_INODE_BIT_FNS(name, field, offset)				\
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b3a5213..4ee6927 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1145,77 +1145,36 @@ static int ext4_generic_write_end(struct file *file,
>   * ext4 never places buffers on inode->i_mapping->private_list.  metadata
>   * buffers are managed internally.
>   */
> -static int ext4_ordered_write_end(struct file *file,
> -				  struct address_space *mapping,
> -				  loff_t pos, unsigned len, unsigned copied,
> -				  struct page *page, void *fsdata)
> +static int ext4_write_end(struct file *file,
> +			  struct address_space *mapping,
> +			  loff_t pos, unsigned len, unsigned copied,
> +			  struct page *page, void *fsdata)
>  {
>  	handle_t *handle = ext4_journal_current_handle();
>  	struct inode *inode = mapping->host;
>  	int ret = 0, ret2;
>  
> -	trace_ext4_ordered_write_end(inode, pos, len, copied);
> -	ret = ext4_jbd2_file_inode(handle, inode);
> -
> -	if (ret == 0) {
> -		ret2 = ext4_generic_write_end(file, mapping, pos, len, copied,
> -							page, fsdata);
> -		copied = ret2;
> -		if (pos + len > inode->i_size && ext4_can_truncate(inode))
> -			/* if we have allocated more blocks and copied
> -			 * less. We will have blocks allocated outside
> -			 * inode->i_size. So truncate them
> -			 */
> -			ext4_orphan_add(handle, inode);
> -		if (ret2 < 0)
> -			ret = ret2;
> -	} else {
> -		unlock_page(page);
> -		page_cache_release(page);
> -	}
> -
> -	ret2 = ext4_journal_stop(handle);
> -	if (!ret)
> -		ret = ret2;
> -
> -	if (pos + len > inode->i_size) {
> -		ext4_truncate_failed_write(inode);
> -		/*
> -		 * If truncate failed early the inode might still be
> -		 * on the orphan list; we need to make sure the inode
> -		 * is removed from the orphan list in that case.
> -		 */
> -		if (inode->i_nlink)
> -			ext4_orphan_del(NULL, inode);
> +	trace_ext4_write_end(inode, pos, len, copied);
> +	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> +		ret = ext4_jbd2_file_inode(handle, inode);
> +		if (ret) {
> +			unlock_page(page);
> +			page_cache_release(page);
> +			goto errout;
> +		}
>  	}
>  
> -
> -	return ret ? ret : copied;
> -}
> -
> -static int ext4_writeback_write_end(struct file *file,
> -				    struct address_space *mapping,
> -				    loff_t pos, unsigned len, unsigned copied,
> -				    struct page *page, void *fsdata)
> -{
> -	handle_t *handle = ext4_journal_current_handle();
> -	struct inode *inode = mapping->host;
> -	int ret = 0, ret2;
> -
> -	trace_ext4_writeback_write_end(inode, pos, len, copied);
> -	ret2 = ext4_generic_write_end(file, mapping, pos, len, copied,
> -							page, fsdata);
> -	copied = ret2;
> +	copied = ext4_generic_write_end(file, mapping, pos, len, copied,
> +					page, fsdata);
> +	if (copied < 0)
> +		ret = copied;
>  	if (pos + len > inode->i_size && ext4_can_truncate(inode))
>  		/* if we have allocated more blocks and copied
>  		 * less. We will have blocks allocated outside
>  		 * inode->i_size. So truncate them
>  		 */
>  		ext4_orphan_add(handle, inode);
> -
> -	if (ret2 < 0)
> -		ret = ret2;
> -
> +errout:
>  	ret2 = ext4_journal_stop(handle);
>  	if (!ret)
>  		ret = ret2;
> @@ -2818,18 +2777,9 @@ static int ext4_da_write_end(struct file *file,
>  	unsigned long start, end;
>  	int write_mode = (int)(unsigned long)fsdata;
>  
> -	if (write_mode == FALL_BACK_TO_NONDELALLOC) {
> -		switch (ext4_inode_journal_mode(inode)) {
> -		case EXT4_INODE_ORDERED_DATA_MODE:
> -			return ext4_ordered_write_end(file, mapping, pos,
> -					len, copied, page, fsdata);
> -		case EXT4_INODE_WRITEBACK_DATA_MODE:
> -			return ext4_writeback_write_end(file, mapping, pos,
> -					len, copied, page, fsdata);
> -		default:
> -			BUG();
> -		}
> -	}
> +	if (write_mode == FALL_BACK_TO_NONDELALLOC)
> +		return ext4_write_end(file, mapping, pos,
> +				      len, copied, page, fsdata);
>  
>  	trace_ext4_da_write_end(inode, pos, len, copied);
>  	start = pos & (PAGE_CACHE_SIZE - 1);
> @@ -3334,27 +3284,12 @@ static int ext4_journalled_set_page_dirty(struct page *page)
>  	return __set_page_dirty_nobuffers(page);
>  }
>  
> -static const struct address_space_operations ext4_ordered_aops = {
> +static const struct address_space_operations ext4_aops = {
>  	.readpage		= ext4_readpage,
>  	.readpages		= ext4_readpages,
>  	.writepage		= ext4_writepage,
>  	.write_begin		= ext4_write_begin,
> -	.write_end		= ext4_ordered_write_end,
> -	.bmap			= ext4_bmap,
> -	.invalidatepage		= ext4_invalidatepage,
> -	.releasepage		= ext4_releasepage,
> -	.direct_IO		= ext4_direct_IO,
> -	.migratepage		= buffer_migrate_page,
> -	.is_partially_uptodate  = block_is_partially_uptodate,
> -	.error_remove_page	= generic_error_remove_page,
> -};
> -
> -static const struct address_space_operations ext4_writeback_aops = {
> -	.readpage		= ext4_readpage,
> -	.readpages		= ext4_readpages,
> -	.writepage		= ext4_writepage,
> -	.write_begin		= ext4_write_begin,
> -	.write_end		= ext4_writeback_write_end,
> +	.write_end		= ext4_write_end,
>  	.bmap			= ext4_bmap,
>  	.invalidatepage		= ext4_invalidatepage,
>  	.releasepage		= ext4_releasepage,
> @@ -3399,23 +3334,21 @@ void ext4_set_aops(struct inode *inode)
>  {
>  	switch (ext4_inode_journal_mode(inode)) {
>  	case EXT4_INODE_ORDERED_DATA_MODE:
> -		if (test_opt(inode->i_sb, DELALLOC))
> -			inode->i_mapping->a_ops = &ext4_da_aops;
> -		else
> -			inode->i_mapping->a_ops = &ext4_ordered_aops;
> +		ext4_set_inode_state(inode, EXT4_STATE_ORDERED_MODE);
>  		break;
>  	case EXT4_INODE_WRITEBACK_DATA_MODE:
> -		if (test_opt(inode->i_sb, DELALLOC))
> -			inode->i_mapping->a_ops = &ext4_da_aops;
> -		else
> -			inode->i_mapping->a_ops = &ext4_writeback_aops;
> +		ext4_clear_inode_state(inode, EXT4_STATE_ORDERED_MODE);
>  		break;
>  	case EXT4_INODE_JOURNAL_DATA_MODE:
>  		inode->i_mapping->a_ops = &ext4_journalled_aops;
> -		break;
> +		return;
>  	default:
>  		BUG();
>  	}
> +	if (test_opt(inode->i_sb, DELALLOC))
> +		inode->i_mapping->a_ops = &ext4_da_aops;
> +	else
> +		inode->i_mapping->a_ops = &ext4_aops;
>  }
>  
>  
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index 4ee4710..58459b7 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -257,15 +257,7 @@ DECLARE_EVENT_CLASS(ext4__write_end,
>  		  __entry->pos, __entry->len, __entry->copied)
>  );
>  
> -DEFINE_EVENT(ext4__write_end, ext4_ordered_write_end,
> -
> -	TP_PROTO(struct inode *inode, loff_t pos, unsigned int len,
> -		 unsigned int copied),
> -
> -	TP_ARGS(inode, pos, len, copied)
> -);
> -
> -DEFINE_EVENT(ext4__write_end, ext4_writeback_write_end,
> +DEFINE_EVENT(ext4__write_end, ext4_write_end,
>  
>  	TP_PROTO(struct inode *inode, loff_t pos, unsigned int len,
>  		 unsigned int copied),
> 

[PATCH 2/7] ext4: fold ext4_generic_write_end() into ext4_write_end()

[Theodore Ts'o]

From: Zheng Liu <wenqing.lz@taobao.com>

After collpasing the handling of data ordered and data writeback
codepath, ext4_generic_write_end() has only one caller,
ext4_write_end().  So we fold it into ext4_write_end().

Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/inode.c | 67 +++++++++++++++++++++++----------------------------------
 1 file changed, 27 insertions(+), 40 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4ee6927..a4ffb47 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1087,14 +1087,32 @@ static int write_end_fn(handle_t *handle, struct buffer_head *bh)
 	return ext4_handle_dirty_metadata(handle, NULL, bh);
 }
 
-static int ext4_generic_write_end(struct file *file,
-				  struct address_space *mapping,
-				  loff_t pos, unsigned len, unsigned copied,
-				  struct page *page, void *fsdata)
+/*
+ * We need to pick up the new inode size which generic_commit_write gave us
+ * `file' can be NULL - eg, when called from page_symlink().
+ *
+ * ext4 never places buffers on inode->i_mapping->private_list.  metadata
+ * buffers are managed internally.
+ */
+static int ext4_write_end(struct file *file,
+			  struct address_space *mapping,
+			  loff_t pos, unsigned len, unsigned copied,
+			  struct page *page, void *fsdata)
 {
-	int i_size_changed = 0;
-	struct inode *inode = mapping->host;
 	handle_t *handle = ext4_journal_current_handle();
+	struct inode *inode = mapping->host;
+	int ret = 0, ret2;
+	int i_size_changed = 0;
+
+	trace_ext4_write_end(inode, pos, len, copied);
+	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
+		ret = ext4_jbd2_file_inode(handle, inode);
+		if (ret) {
+			unlock_page(page);
+			page_cache_release(page);
+			goto errout;
+		}
+	}
 
 	if (ext4_has_inline_data(inode))
 		copied = ext4_write_inline_data_end(inode, pos, len,
@@ -1105,7 +1123,7 @@ static int ext4_generic_write_end(struct file *file,
 
 	/*
 	 * No need to use i_size_read() here, the i_size
-	 * cannot change under us because we hold i_mutex.
+	 * cannot change under us because we hole i_mutex.
 	 *
 	 * But it's important to update i_size while still holding page lock:
 	 * page writeout could otherwise come in and zero beyond i_size.
@@ -1115,10 +1133,10 @@ static int ext4_generic_write_end(struct file *file,
 		i_size_changed = 1;
 	}
 
-	if (pos + copied >  EXT4_I(inode)->i_disksize) {
+	if (pos + copied > EXT4_I(inode)->i_disksize) {
 		/* We need to mark inode dirty even if
 		 * new_i_size is less that inode->i_size
-		 * bu greater than i_disksize.(hint delalloc)
+		 * but greater than i_disksize. (hint delalloc)
 		 */
 		ext4_update_i_disksize(inode, (pos + copied));
 		i_size_changed = 1;
@@ -1135,37 +1153,6 @@ static int ext4_generic_write_end(struct file *file,
 	if (i_size_changed)
 		ext4_mark_inode_dirty(handle, inode);
 
-	return copied;
-}
-
-/*
- * We need to pick up the new inode size which generic_commit_write gave us
- * `file' can be NULL - eg, when called from page_symlink().
- *
- * ext4 never places buffers on inode->i_mapping->private_list.  metadata
- * buffers are managed internally.
- */
-static int ext4_write_end(struct file *file,
-			  struct address_space *mapping,
-			  loff_t pos, unsigned len, unsigned copied,
-			  struct page *page, void *fsdata)
-{
-	handle_t *handle = ext4_journal_current_handle();
-	struct inode *inode = mapping->host;
-	int ret = 0, ret2;
-
-	trace_ext4_write_end(inode, pos, len, copied);
-	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
-		ret = ext4_jbd2_file_inode(handle, inode);
-		if (ret) {
-			unlock_page(page);
-			page_cache_release(page);
-			goto errout;
-		}
-	}

Re: [PATCH 2/7] ext4: fold ext4_generic_write_end() into ext4_write_end()

[Lukáš Czerner]

On Sun, 24 Mar 2013, Theodore Ts'o wrote:

> Date: Sun, 24 Mar 2013 20:06:49 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Cc: Zheng Liu <wenqing.lz@taobao.com>, Theodore Ts'o <tytso@mit.edu>
> Subject: [PATCH 2/7] ext4: fold ext4_generic_write_end() into ext4_write_end()
> 
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> After collpasing the handling of data ordered and data writeback
> codepath, ext4_generic_write_end() has only one caller,
> ext4_write_end().  So we fold it into ext4_write_end().

Looks good.

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

Thanks!
-Lukas

> 
> Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/inode.c | 67 +++++++++++++++++++++++----------------------------------
>  1 file changed, 27 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 4ee6927..a4ffb47 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1087,14 +1087,32 @@ static int write_end_fn(handle_t *handle, struct buffer_head *bh)
>  	return ext4_handle_dirty_metadata(handle, NULL, bh);
>  }
>  
> -static int ext4_generic_write_end(struct file *file,
> -				  struct address_space *mapping,
> -				  loff_t pos, unsigned len, unsigned copied,
> -				  struct page *page, void *fsdata)
> +/*
> + * We need to pick up the new inode size which generic_commit_write gave us
> + * `file' can be NULL - eg, when called from page_symlink().
> + *
> + * ext4 never places buffers on inode->i_mapping->private_list.  metadata
> + * buffers are managed internally.
> + */
> +static int ext4_write_end(struct file *file,
> +			  struct address_space *mapping,
> +			  loff_t pos, unsigned len, unsigned copied,
> +			  struct page *page, void *fsdata)
>  {
> -	int i_size_changed = 0;
> -	struct inode *inode = mapping->host;
>  	handle_t *handle = ext4_journal_current_handle();
> +	struct inode *inode = mapping->host;
> +	int ret = 0, ret2;
> +	int i_size_changed = 0;
> +
> +	trace_ext4_write_end(inode, pos, len, copied);
> +	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> +		ret = ext4_jbd2_file_inode(handle, inode);
> +		if (ret) {
> +			unlock_page(page);
> +			page_cache_release(page);
> +			goto errout;
> +		}
> +	}
>  
>  	if (ext4_has_inline_data(inode))
>  		copied = ext4_write_inline_data_end(inode, pos, len,
> @@ -1105,7 +1123,7 @@ static int ext4_generic_write_end(struct file *file,
>  
>  	/*
>  	 * No need to use i_size_read() here, the i_size
> -	 * cannot change under us because we hold i_mutex.
> +	 * cannot change under us because we hole i_mutex.
>  	 *
>  	 * But it's important to update i_size while still holding page lock:
>  	 * page writeout could otherwise come in and zero beyond i_size.
> @@ -1115,10 +1133,10 @@ static int ext4_generic_write_end(struct file *file,
>  		i_size_changed = 1;
>  	}
>  
> -	if (pos + copied >  EXT4_I(inode)->i_disksize) {
> +	if (pos + copied > EXT4_I(inode)->i_disksize) {
>  		/* We need to mark inode dirty even if
>  		 * new_i_size is less that inode->i_size
> -		 * bu greater than i_disksize.(hint delalloc)
> +		 * but greater than i_disksize. (hint delalloc)
>  		 */
>  		ext4_update_i_disksize(inode, (pos + copied));
>  		i_size_changed = 1;
> @@ -1135,37 +1153,6 @@ static int ext4_generic_write_end(struct file *file,
>  	if (i_size_changed)
>  		ext4_mark_inode_dirty(handle, inode);
>  
> -	return copied;
> -}
> -
> -/*
> - * We need to pick up the new inode size which generic_commit_write gave us
> - * `file' can be NULL - eg, when called from page_symlink().
> - *
> - * ext4 never places buffers on inode->i_mapping->private_list.  metadata
> - * buffers are managed internally.
> - */
> -static int ext4_write_end(struct file *file,
> -			  struct address_space *mapping,
> -			  loff_t pos, unsigned len, unsigned copied,
> -			  struct page *page, void *fsdata)
> -{
> -	handle_t *handle = ext4_journal_current_handle();
> -	struct inode *inode = mapping->host;
> -	int ret = 0, ret2;
> -
> -	trace_ext4_write_end(inode, pos, len, copied);
> -	if (ext4_test_inode_state(inode, EXT4_STATE_ORDERED_MODE)) {
> -		ret = ext4_jbd2_file_inode(handle, inode);
> -		if (ret) {
> -			unlock_page(page);
> -			page_cache_release(page);
> -			goto errout;
> -		}
> -	}
> -
> -	copied = ext4_generic_write_end(file, mapping, pos, len, copied,
> -					page, fsdata);
>  	if (copied < 0)
>  		ret = copied;
>  	if (pos + len > inode->i_size && ext4_can_truncate(inode))
> 

Re: [PATCH 2/7] ext4: fold ext4_generic_write_end() into ext4_write_end()

[Jan Kara]

On Sun 24-03-13 20:06:49, Ted Tso wrote:
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> After collpasing the handling of data ordered and data writeback
> codepath, ext4_generic_write_end() has only one caller,
> ext4_write_end().  So we fold it into ext4_write_end().
> 
> Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/inode.c | 67 +++++++++++++++++++++++----------------------------------
>  1 file changed, 27 insertions(+), 40 deletions(-)
> 
...
> @@ -1105,7 +1123,7 @@ static int ext4_generic_write_end(struct file *file,
>  
>  	/*
>  	 * No need to use i_size_read() here, the i_size
> -	 * cannot change under us because we hold i_mutex.
> +	 * cannot change under us because we hole i_mutex.
  Typo added here. Otherwise the patch is fine. You can add
Reviewed-by: Jan Kara <jack@suse.cz>

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

[PATCH 3/7] ext4: fold ext4_alloc_blocks() in ext4_alloc_branch()

[Theodore Ts'o]

The older code was far more complicated than it needed to be because
of how we spliced in the ext4's new multiblock allocator into ext3's
indirect block code.  By folding ext4_alloc_blocks() into
ext4_alloc_branch(), we make the code far more understable, shave off
over 130 lines of code and half a kilobyte of compiled object code.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/indirect.c | 227 +++++++++++------------------------------------------
 1 file changed, 46 insertions(+), 181 deletions(-)

diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index b505a14..b780c4a 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -292,131 +292,6 @@ static int ext4_blks_to_allocate(Indirect *branch, int k, unsigned int blks,
 }
 
 /**
- *	ext4_alloc_blocks: multiple allocate blocks needed for a branch
- *	@handle: handle for this transaction
- *	@inode: inode which needs allocated blocks
- *	@iblock: the logical block to start allocated at
- *	@goal: preferred physical block of allocation
- *	@indirect_blks: the number of blocks need to allocate for indirect
- *			blocks
- *	@blks: number of desired blocks
- *	@new_blocks: on return it will store the new block numbers for
- *	the indirect blocks(if needed) and the first direct block,
- *	@err: on return it will store the error code
- *
- *	This function will return the number of blocks allocated as
- *	requested by the passed-in parameters.
- */
-static int ext4_alloc_blocks(handle_t *handle, struct inode *inode,
-			     ext4_lblk_t iblock, ext4_fsblk_t goal,
-			     int indirect_blks, int blks,
-			     ext4_fsblk_t new_blocks[4], int *err)
-{
-	struct ext4_allocation_request ar;
-	int target, i;
-	unsigned long count = 0, blk_allocated = 0;
-	int index = 0;
-	ext4_fsblk_t current_block = 0;
-	int ret = 0;
-
-	/*
-	 * Here we try to allocate the requested multiple blocks at once,
-	 * on a best-effort basis.
-	 * To build a branch, we should allocate blocks for
-	 * the indirect blocks(if not allocated yet), and at least
-	 * the first direct block of this branch.  That's the
-	 * minimum number of blocks need to allocate(required)
-	 */
-	/* first we try to allocate the indirect blocks */
-	target = indirect_blks;
-	while (target > 0) {
-		count = target;
-		/* allocating blocks for indirect blocks and direct blocks */
-		current_block = ext4_new_meta_blocks(handle, inode, goal,
-						     0, &count, err);
-		if (*err)
-			goto failed_out;
-
-		if (unlikely(current_block + count > EXT4_MAX_BLOCK_FILE_PHYS)) {
-			EXT4_ERROR_INODE(inode,
-					 "current_block %llu + count %lu > %d!",
-					 current_block, count,
-					 EXT4_MAX_BLOCK_FILE_PHYS);
-			*err = -EIO;
-			goto failed_out;
-		}
-
-		target -= count;
-		/* allocate blocks for indirect blocks */
-		while (index < indirect_blks && count) {
-			new_blocks[index++] = current_block++;
-			count--;
-		}
-		if (count > 0) {
-			/*
-			 * save the new block number
-			 * for the first direct block
-			 */
-			new_blocks[index] = current_block;
-			WARN(1, KERN_INFO "%s returned more blocks than "
-						"requested\n", __func__);
-			break;
-		}
-	}
-
-	target = blks - count ;
-	blk_allocated = count;
-	if (!target)
-		goto allocated;
-	/* Now allocate data blocks */
-	memset(&ar, 0, sizeof(ar));
-	ar.inode = inode;
-	ar.goal = goal;
-	ar.len = target;
-	ar.logical = iblock;
-	if (S_ISREG(inode->i_mode))
-		/* enable in-core preallocation only for regular files */
-		ar.flags = EXT4_MB_HINT_DATA;
-
-	current_block = ext4_mb_new_blocks(handle, &ar, err);
-	if (unlikely(current_block + ar.len > EXT4_MAX_BLOCK_FILE_PHYS)) {
-		EXT4_ERROR_INODE(inode,
-				 "current_block %llu + ar.len %d > %d!",
-				 current_block, ar.len,
-				 EXT4_MAX_BLOCK_FILE_PHYS);
-		*err = -EIO;
-		goto failed_out;
-	}
-
-	if (*err && (target == blks)) {
-		/*
-		 * if the allocation failed and we didn't allocate
-		 * any blocks before
-		 */
-		goto failed_out;
-	}
-	if (!*err) {
-		if (target == blks) {
-			/*
-			 * save the new block number
-			 * for the first direct block
-			 */
-			new_blocks[index] = current_block;
-		}
-		blk_allocated += ar.len;
-	}
-allocated:
-	/* total number of blocks allocated for direct blocks */
-	ret = blk_allocated;
-	*err = 0;
-	return ret;
-failed_out:
-	for (i = 0; i < index; i++)
-		ext4_free_blocks(handle, inode, NULL, new_blocks[i], 1, 0);
-	return ret;
-}
-
-/**
  *	ext4_alloc_branch - allocate and set up a chain of blocks.
  *	@handle: handle for this transaction
  *	@inode: owner
@@ -448,60 +323,59 @@ static int ext4_alloc_branch(handle_t *handle, struct inode *inode,
 			     int *blks, ext4_fsblk_t goal,
 			     ext4_lblk_t *offsets, Indirect *branch)
 {
-	int blocksize = inode->i_sb->s_blocksize;
-	int i, n = 0;
-	int err = 0;
-	struct buffer_head *bh;
-	int num;
-	ext4_fsblk_t new_blocks[4];
-	ext4_fsblk_t current_block;
+	struct ext4_allocation_request	ar;
+	struct buffer_head *		bh;
+	ext4_fsblk_t			b, new_blocks[4];
+	__le32				*p;
+	int				i, j, err, len = 1;
 
-	num = ext4_alloc_blocks(handle, inode, iblock, goal, indirect_blks,
-				*blks, new_blocks, &err);
-	if (err)
-		return err;
-
-	branch[0].key = cpu_to_le32(new_blocks[0]);
 	/*
-	 * metadata blocks and data blocks are allocated.
+	 * Set up for the direct block allocation
 	 */
-	for (n = 1; n <= indirect_blks;  n++) {
-		/*
-		 * Get buffer_head for parent block, zero it out
-		 * and set the pointer to new one, then send
-		 * parent to disk.
-		 */
-		bh = sb_getblk(inode->i_sb, new_blocks[n-1]);
+	memset(&ar, 0, sizeof(ar));
+	ar.inode = inode;
+	ar.len = *blks;
+	ar.logical = iblock;
+	if (S_ISREG(inode->i_mode))
+		ar.flags = EXT4_MB_HINT_DATA;
+
+	for (i = 0; i <= indirect_blks; i++) {
+		if (i == indirect_blks) {
+			ar.goal = goal;
+			new_blocks[i] = ext4_mb_new_blocks(handle, &ar, &err);
+		} else
+			goal = new_blocks[i] = ext4_new_meta_blocks(handle, inode,
+							goal, 0, NULL, &err);
+		if (err) {
+			i--;
+			goto failed;
+		}
+		branch[i].key = cpu_to_le32(new_blocks[i]);
+		if (i == 0)
+			continue;
+
+		bh = branch[i].bh = sb_getblk(inode->i_sb, new_blocks[i-1]);
 		if (unlikely(!bh)) {
 			err = -ENOMEM;
 			goto failed;
 		}
-
-		branch[n].bh = bh;
 		lock_buffer(bh);
 		BUFFER_TRACE(bh, "call get_create_access");
 		err = ext4_journal_get_create_access(handle, bh);
 		if (err) {
-			/* Don't brelse(bh) here; it's done in
-			 * ext4_journal_forget() below */
 			unlock_buffer(bh);
 			goto failed;
 		}
 
-		memset(bh->b_data, 0, blocksize);
-		branch[n].p = (__le32 *) bh->b_data + offsets[n];
-		branch[n].key = cpu_to_le32(new_blocks[n]);
-		*branch[n].p = branch[n].key;
-		if (n == indirect_blks) {
-			current_block = new_blocks[n];
-			/*
-			 * End of chain, update the last new metablock of
-			 * the chain to point to the new allocated
-			 * data blocks numbers
-			 */
-			for (i = 1; i < num; i++)
-				*(branch[n].p + i) = cpu_to_le32(++current_block);
-		}
+		memset(bh->b_data, 0, bh->b_size);
+		p = branch[i].p = (__le32 *) bh->b_data + offsets[i];
+		b = new_blocks[i];
+
+		if (i == indirect_blks)
+			len = ar.len;
+		for (j = 0; j < len; j++)
+			*p++ = cpu_to_le32(b++);
+
 		BUFFER_TRACE(bh, "marking uptodate");
 		set_buffer_uptodate(bh);
 		unlock_buffer(bh);
@@ -511,25 +385,16 @@ static int ext4_alloc_branch(handle_t *handle, struct inode *inode,
 		if (err)
 			goto failed;
 	}
-	*blks = num;
-	return err;
+	*blks = ar.len;
+	return 0;
 failed:
-	/* Allocation failed, free what we already allocated */
-	ext4_free_blocks(handle, inode, NULL, new_blocks[0], 1, 0);
-	for (i = 1; i <= n ; i++) {
-		/*
-		 * branch[i].bh is newly allocated, so there is no
-		 * need to revoke the block, which is why we don't
-		 * need to set EXT4_FREE_BLOCKS_METADATA.
-		 */
-		ext4_free_blocks(handle, inode, NULL, new_blocks[i], 1,
-				 EXT4_FREE_BLOCKS_FORGET);
+	for (; i >= 0; i--) {
+		if (i != indirect_blks && branch[i].bh)
+			ext4_forget(handle, 1, inode, branch[i].bh,
+				    branch[i].bh->b_blocknr);
+		ext4_free_blocks(handle, inode, NULL, new_blocks[i],
+				 (i == indirect_blks) ? ar.len : 1, 0);
 	}
-	for (i = n+1; i < indirect_blks; i++)
-		ext4_free_blocks(handle, inode, NULL, new_blocks[i], 1, 0);
-
-	ext4_free_blocks(handle, inode, NULL, new_blocks[i], num, 0);

Re: [PATCH 3/7] ext4: fold ext4_alloc_blocks() in ext4_alloc_branch()

[Jan Kara]

On Sun 24-03-13 20:06:50, Ted Tso wrote:
> The older code was far more complicated than it needed to be because
> of how we spliced in the ext4's new multiblock allocator into ext3's
> indirect block code.  By folding ext4_alloc_blocks() into
> ext4_alloc_branch(), we make the code far more understable, shave off
> over 130 lines of code and half a kilobyte of compiled object code.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
  The patch looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/indirect.c | 227 +++++++++++------------------------------------------
>  1 file changed, 46 insertions(+), 181 deletions(-)
> 
> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index b505a14..b780c4a 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -292,131 +292,6 @@ static int ext4_blks_to_allocate(Indirect *branch, int k, unsigned int blks,
>  }
>  
>  /**
> - *	ext4_alloc_blocks: multiple allocate blocks needed for a branch
> - *	@handle: handle for this transaction
> - *	@inode: inode which needs allocated blocks
> - *	@iblock: the logical block to start allocated at
> - *	@goal: preferred physical block of allocation
> - *	@indirect_blks: the number of blocks need to allocate for indirect
> - *			blocks
> - *	@blks: number of desired blocks
> - *	@new_blocks: on return it will store the new block numbers for
> - *	the indirect blocks(if needed) and the first direct block,
> - *	@err: on return it will store the error code
> - *
> - *	This function will return the number of blocks allocated as
> - *	requested by the passed-in parameters.
> - */
> -static int ext4_alloc_blocks(handle_t *handle, struct inode *inode,
> -			     ext4_lblk_t iblock, ext4_fsblk_t goal,
> -			     int indirect_blks, int blks,
> -			     ext4_fsblk_t new_blocks[4], int *err)
> -{
> -	struct ext4_allocation_request ar;
> -	int target, i;
> -	unsigned long count = 0, blk_allocated = 0;
> -	int index = 0;
> -	ext4_fsblk_t current_block = 0;
> -	int ret = 0;
> -
> -	/*
> -	 * Here we try to allocate the requested multiple blocks at once,
> -	 * on a best-effort basis.
> -	 * To build a branch, we should allocate blocks for
> -	 * the indirect blocks(if not allocated yet), and at least
> -	 * the first direct block of this branch.  That's the
> -	 * minimum number of blocks need to allocate(required)
> -	 */
> -	/* first we try to allocate the indirect blocks */
> -	target = indirect_blks;
> -	while (target > 0) {
> -		count = target;
> -		/* allocating blocks for indirect blocks and direct blocks */
> -		current_block = ext4_new_meta_blocks(handle, inode, goal,
> -						     0, &count, err);
> -		if (*err)
> -			goto failed_out;
> -
> -		if (unlikely(current_block + count > EXT4_MAX_BLOCK_FILE_PHYS)) {
> -			EXT4_ERROR_INODE(inode,
> -					 "current_block %llu + count %lu > %d!",
> -					 current_block, count,
> -					 EXT4_MAX_BLOCK_FILE_PHYS);
> -			*err = -EIO;
> -			goto failed_out;
> -		}
> -
> -		target -= count;
> -		/* allocate blocks for indirect blocks */
> -		while (index < indirect_blks && count) {
> -			new_blocks[index++] = current_block++;
> -			count--;
> -		}
> -		if (count > 0) {
> -			/*
> -			 * save the new block number
> -			 * for the first direct block
> -			 */
> -			new_blocks[index] = current_block;
> -			WARN(1, KERN_INFO "%s returned more blocks than "
> -						"requested\n", __func__);
> -			break;
> -		}
> -	}
> -
> -	target = blks - count ;
> -	blk_allocated = count;
> -	if (!target)
> -		goto allocated;
> -	/* Now allocate data blocks */
> -	memset(&ar, 0, sizeof(ar));
> -	ar.inode = inode;
> -	ar.goal = goal;
> -	ar.len = target;
> -	ar.logical = iblock;
> -	if (S_ISREG(inode->i_mode))
> -		/* enable in-core preallocation only for regular files */
> -		ar.flags = EXT4_MB_HINT_DATA;
> -
> -	current_block = ext4_mb_new_blocks(handle, &ar, err);
> -	if (unlikely(current_block + ar.len > EXT4_MAX_BLOCK_FILE_PHYS)) {
> -		EXT4_ERROR_INODE(inode,
> -				 "current_block %llu + ar.len %d > %d!",
> -				 current_block, ar.len,
> -				 EXT4_MAX_BLOCK_FILE_PHYS);
> -		*err = -EIO;
> -		goto failed_out;
> -	}
> -
> -	if (*err && (target == blks)) {
> -		/*
> -		 * if the allocation failed and we didn't allocate
> -		 * any blocks before
> -		 */
> -		goto failed_out;
> -	}
> -	if (!*err) {
> -		if (target == blks) {
> -			/*
> -			 * save the new block number
> -			 * for the first direct block
> -			 */
> -			new_blocks[index] = current_block;
> -		}
> -		blk_allocated += ar.len;
> -	}
> -allocated:
> -	/* total number of blocks allocated for direct blocks */
> -	ret = blk_allocated;
> -	*err = 0;
> -	return ret;
> -failed_out:
> -	for (i = 0; i < index; i++)
> -		ext4_free_blocks(handle, inode, NULL, new_blocks[i], 1, 0);
> -	return ret;
> -}
> -
> -/**
>   *	ext4_alloc_branch - allocate and set up a chain of blocks.
>   *	@handle: handle for this transaction
>   *	@inode: owner
> @@ -448,60 +323,59 @@ static int ext4_alloc_branch(handle_t *handle, struct inode *inode,
>  			     int *blks, ext4_fsblk_t goal,
>  			     ext4_lblk_t *offsets, Indirect *branch)
>  {
> -	int blocksize = inode->i_sb->s_blocksize;
> -	int i, n = 0;
> -	int err = 0;
> -	struct buffer_head *bh;
> -	int num;
> -	ext4_fsblk_t new_blocks[4];
> -	ext4_fsblk_t current_block;
> +	struct ext4_allocation_request	ar;
> +	struct buffer_head *		bh;
> +	ext4_fsblk_t			b, new_blocks[4];
> +	__le32				*p;
> +	int				i, j, err, len = 1;
>  
> -	num = ext4_alloc_blocks(handle, inode, iblock, goal, indirect_blks,
> -				*blks, new_blocks, &err);
> -	if (err)
> -		return err;
> -
> -	branch[0].key = cpu_to_le32(new_blocks[0]);
>  	/*
> -	 * metadata blocks and data blocks are allocated.
> +	 * Set up for the direct block allocation
>  	 */
> -	for (n = 1; n <= indirect_blks;  n++) {
> -		/*
> -		 * Get buffer_head for parent block, zero it out
> -		 * and set the pointer to new one, then send
> -		 * parent to disk.
> -		 */
> -		bh = sb_getblk(inode->i_sb, new_blocks[n-1]);
> +	memset(&ar, 0, sizeof(ar));
> +	ar.inode = inode;
> +	ar.len = *blks;
> +	ar.logical = iblock;
> +	if (S_ISREG(inode->i_mode))
> +		ar.flags = EXT4_MB_HINT_DATA;
> +
> +	for (i = 0; i <= indirect_blks; i++) {
> +		if (i == indirect_blks) {
> +			ar.goal = goal;
> +			new_blocks[i] = ext4_mb_new_blocks(handle, &ar, &err);
> +		} else
> +			goal = new_blocks[i] = ext4_new_meta_blocks(handle, inode,
> +							goal, 0, NULL, &err);
> +		if (err) {
> +			i--;
> +			goto failed;
> +		}
> +		branch[i].key = cpu_to_le32(new_blocks[i]);
> +		if (i == 0)
> +			continue;
> +
> +		bh = branch[i].bh = sb_getblk(inode->i_sb, new_blocks[i-1]);
>  		if (unlikely(!bh)) {
>  			err = -ENOMEM;
>  			goto failed;
>  		}
> -
> -		branch[n].bh = bh;
>  		lock_buffer(bh);
>  		BUFFER_TRACE(bh, "call get_create_access");
>  		err = ext4_journal_get_create_access(handle, bh);
>  		if (err) {
> -			/* Don't brelse(bh) here; it's done in
> -			 * ext4_journal_forget() below */
>  			unlock_buffer(bh);
>  			goto failed;
>  		}
>  
> -		memset(bh->b_data, 0, blocksize);
> -		branch[n].p = (__le32 *) bh->b_data + offsets[n];
> -		branch[n].key = cpu_to_le32(new_blocks[n]);
> -		*branch[n].p = branch[n].key;
> -		if (n == indirect_blks) {
> -			current_block = new_blocks[n];
> -			/*
> -			 * End of chain, update the last new metablock of
> -			 * the chain to point to the new allocated
> -			 * data blocks numbers
> -			 */
> -			for (i = 1; i < num; i++)
> -				*(branch[n].p + i) = cpu_to_le32(++current_block);
> -		}
> +		memset(bh->b_data, 0, bh->b_size);
> +		p = branch[i].p = (__le32 *) bh->b_data + offsets[i];
> +		b = new_blocks[i];
> +
> +		if (i == indirect_blks)
> +			len = ar.len;
> +		for (j = 0; j < len; j++)
> +			*p++ = cpu_to_le32(b++);
> +
>  		BUFFER_TRACE(bh, "marking uptodate");
>  		set_buffer_uptodate(bh);
>  		unlock_buffer(bh);
> @@ -511,25 +385,16 @@ static int ext4_alloc_branch(handle_t *handle, struct inode *inode,
>  		if (err)
>  			goto failed;
>  	}
> -	*blks = num;
> -	return err;
> +	*blks = ar.len;
> +	return 0;
>  failed:
> -	/* Allocation failed, free what we already allocated */
> -	ext4_free_blocks(handle, inode, NULL, new_blocks[0], 1, 0);
> -	for (i = 1; i <= n ; i++) {
> -		/*
> -		 * branch[i].bh is newly allocated, so there is no
> -		 * need to revoke the block, which is why we don't
> -		 * need to set EXT4_FREE_BLOCKS_METADATA.
> -		 */
> -		ext4_free_blocks(handle, inode, NULL, new_blocks[i], 1,
> -				 EXT4_FREE_BLOCKS_FORGET);
> +	for (; i >= 0; i--) {
> +		if (i != indirect_blks && branch[i].bh)
> +			ext4_forget(handle, 1, inode, branch[i].bh,
> +				    branch[i].bh->b_blocknr);
> +		ext4_free_blocks(handle, inode, NULL, new_blocks[i],
> +				 (i == indirect_blks) ? ar.len : 1, 0);
>  	}
> -	for (i = n+1; i < indirect_blks; i++)
> -		ext4_free_blocks(handle, inode, NULL, new_blocks[i], 1, 0);
> -
> -	ext4_free_blocks(handle, inode, NULL, new_blocks[i], num, 0);
> -
>  	return err;
>  }
>  
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 4/7] ext4: refactor punch hole code

[Theodore Ts'o]

Move common code in ext4_ind_punch_hole() and ext4_ext_punch_hole()
into ext4_punch_hole().  This saves over 150 lines of code.

This also fixes a potential bug when the punch_hole() code is racing
against indirect-to-extents or extents-to-indirect migation.  We are
currently using i_mutex to protect against changes to the inode flag;
specifically, the append-only, immutable, and extents inode flags.  So
we need to take i_mutex before deciding whether to use the
extents-specific or indirect-specific punch_hole code.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/ext4.h     |   7 +-
 fs/ext4/extents.c  | 185 +----------------------------------------------------
 fs/ext4/indirect.c | 158 +--------------------------------------------
 fs/ext4/inode.c    | 181 +++++++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 184 insertions(+), 347 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index f91e11b..0649253 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2110,7 +2110,8 @@ extern ssize_t ext4_ind_direct_IO(int rw, struct kiocb *iocb,
 extern int ext4_ind_calc_metadata_amount(struct inode *inode, sector_t lblock);
 extern int ext4_ind_trans_blocks(struct inode *inode, int nrblocks, int chunk);
 extern void ext4_ind_truncate(struct inode *inode);
-extern int ext4_ind_punch_hole(struct file *file, loff_t offset, loff_t length);
+extern int ext4_free_hole_blocks(handle_t *handle, struct inode *inode,
+				 ext4_lblk_t first, ext4_lblk_t stop);
 
 /* ioctl.c */
 extern long ext4_ioctl(struct file *, unsigned int, unsigned long);
@@ -2575,8 +2576,8 @@ extern int ext4_ext_index_trans_blocks(struct inode *inode, int nrblocks,
 extern int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			       struct ext4_map_blocks *map, int flags);
 extern void ext4_ext_truncate(struct inode *);
-extern int ext4_ext_punch_hole(struct file *file, loff_t offset,
-				loff_t length);
+extern int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
+				 ext4_lblk_t end);
 extern void ext4_ext_init(struct super_block *);
 extern void ext4_ext_release(struct super_block *);
 extern long ext4_fallocate(struct file *file, int mode, loff_t offset,
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 56efcaa..8f6b3de 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2599,8 +2599,8 @@ ext4_ext_more_to_rm(struct ext4_ext_path *path)
 	return 1;
 }
 
-static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
-				 ext4_lblk_t end)
+int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
+			  ext4_lblk_t end)
 {
 	struct super_block *sb = inode->i_sb;
 	int depth = ext_depth(inode);
@@ -4620,187 +4620,6 @@ static int ext4_xattr_fiemap(struct inode *inode,
 	return (error < 0 ? error : 0);
 }
 
-/*
- * ext4_ext_punch_hole
- *
- * Punches a hole of "length" bytes in a file starting
- * at byte "offset"
- *
- * @inode:  The inode of the file to punch a hole in
- * @offset: The starting byte offset of the hole
- * @length: The length of the hole
- *
- * Returns the number of blocks removed or negative on err
- */
-int ext4_ext_punch_hole(struct file *file, loff_t offset, loff_t length)
-{
-	struct inode *inode = file_inode(file);
-	struct super_block *sb = inode->i_sb;
-	ext4_lblk_t first_block, stop_block;
-	struct address_space *mapping = inode->i_mapping;
-	handle_t *handle;
-	loff_t first_page, last_page, page_len;
-	loff_t first_page_offset, last_page_offset;
-	int credits, err = 0;
-
-	/*
-	 * Write out all dirty pages to avoid race conditions
-	 * Then release them.
-	 */
-	if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
-		err = filemap_write_and_wait_range(mapping,
-			offset, offset + length - 1);
-
-		if (err)
-			return err;
-	}
-
-	mutex_lock(&inode->i_mutex);
-	/* It's not possible punch hole on append only file */
-	if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
-		err = -EPERM;
-		goto out_mutex;
-	}
-	if (IS_SWAPFILE(inode)) {
-		err = -ETXTBSY;
-		goto out_mutex;
-	}
-
-	/* No need to punch hole beyond i_size */
-	if (offset >= inode->i_size)
-		goto out_mutex;
-
-	/*
-	 * If the hole extends beyond i_size, set the hole
-	 * to end after the page that contains i_size
-	 */
-	if (offset + length > inode->i_size) {
-		length = inode->i_size +
-		   PAGE_CACHE_SIZE - (inode->i_size & (PAGE_CACHE_SIZE - 1)) -
-		   offset;
-	}
-
-	first_page = (offset + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
-	last_page = (offset + length) >> PAGE_CACHE_SHIFT;
-
-	first_page_offset = first_page << PAGE_CACHE_SHIFT;
-	last_page_offset = last_page << PAGE_CACHE_SHIFT;
-
-	/* Now release the pages */
-	if (last_page_offset > first_page_offset) {
-		truncate_pagecache_range(inode, first_page_offset,
-					 last_page_offset - 1);
-	}
-
-	/* Wait all existing dio workers, newcomers will block on i_mutex */
-	ext4_inode_block_unlocked_dio(inode);
-	err = ext4_flush_unwritten_io(inode);
-	if (err)
-		goto out_dio;
-	inode_dio_wait(inode);
-
-	credits = ext4_writepage_trans_blocks(inode);
-	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
-	if (IS_ERR(handle)) {
-		err = PTR_ERR(handle);
-		goto out_dio;
-	}
-
-
-	/*
-	 * Now we need to zero out the non-page-aligned data in the
-	 * pages at the start and tail of the hole, and unmap the buffer
-	 * heads for the block aligned regions of the page that were
-	 * completely zeroed.
-	 */
-	if (first_page > last_page) {
-		/*
-		 * If the file space being truncated is contained within a page
-		 * just zero out and unmap the middle of that page
-		 */
-		err = ext4_discard_partial_page_buffers(handle,
-			mapping, offset, length, 0);
-
-		if (err)
-			goto out;
-	} else {
-		/*
-		 * zero out and unmap the partial page that contains
-		 * the start of the hole
-		 */
-		page_len  = first_page_offset - offset;
-		if (page_len > 0) {
-			err = ext4_discard_partial_page_buffers(handle, mapping,
-						   offset, page_len, 0);
-			if (err)
-				goto out;
-		}
-
-		/*
-		 * zero out and unmap the partial page that contains
-		 * the end of the hole
-		 */
-		page_len = offset + length - last_page_offset;
-		if (page_len > 0) {
-			err = ext4_discard_partial_page_buffers(handle, mapping,
-					last_page_offset, page_len, 0);
-			if (err)
-				goto out;
-		}
-	}
-
-	/*
-	 * If i_size is contained in the last page, we need to
-	 * unmap and zero the partial page after i_size
-	 */
-	if (inode->i_size >> PAGE_CACHE_SHIFT == last_page &&
-	   inode->i_size % PAGE_CACHE_SIZE != 0) {
-
-		page_len = PAGE_CACHE_SIZE -
-			(inode->i_size & (PAGE_CACHE_SIZE - 1));
-
-		if (page_len > 0) {
-			err = ext4_discard_partial_page_buffers(handle,
-			  mapping, inode->i_size, page_len, 0);
-
-			if (err)
-				goto out;
-		}
-	}
-
-	first_block = (offset + sb->s_blocksize - 1) >>
-		EXT4_BLOCK_SIZE_BITS(sb);
-	stop_block = (offset + length) >> EXT4_BLOCK_SIZE_BITS(sb);
-
-	/* If there are no blocks to remove, return now */
-	if (first_block >= stop_block)
-		goto out;
-
-	down_write(&EXT4_I(inode)->i_data_sem);
-	ext4_discard_preallocations(inode);
-
-	err = ext4_es_remove_extent(inode, first_block,
-				    stop_block - first_block);
-	err = ext4_ext_remove_space(inode, first_block, stop_block - 1);
-
-	ext4_discard_preallocations(inode);
-
-	if (IS_SYNC(inode))
-		ext4_handle_sync(handle);
-
-	up_write(&EXT4_I(inode)->i_data_sem);
-
-out:
-	inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
-	ext4_mark_inode_dirty(handle, inode);
-	ext4_journal_stop(handle);
-out_dio:
-	ext4_inode_resume_unlocked_dio(inode);
-out_mutex:
-	mutex_unlock(&inode->i_mutex);
-	return err;
-}
-
 int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		__u64 start, __u64 len)
 {
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index b780c4a..926028c 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -1434,8 +1434,8 @@ err:
 	return ret;
 }
 
-static int ext4_free_hole_blocks(handle_t *handle, struct inode *inode,
-				 ext4_lblk_t first, ext4_lblk_t stop)
+int ext4_free_hole_blocks(handle_t *handle, struct inode *inode,
+			  ext4_lblk_t first, ext4_lblk_t stop)
 {
 	int addr_per_block = EXT4_ADDR_PER_BLOCK(inode->i_sb);
 	int level, ret = 0;
@@ -1469,157 +1469,3 @@ err:
 	return ret;
 }
 
-int ext4_ind_punch_hole(struct file *file, loff_t offset, loff_t length)
-{
-	struct inode *inode = file_inode(file);
-	struct super_block *sb = inode->i_sb;
-	ext4_lblk_t first_block, stop_block;
-	struct address_space *mapping = inode->i_mapping;
-	handle_t *handle = NULL;
-	loff_t first_page, last_page, page_len;
-	loff_t first_page_offset, last_page_offset;
-	int err = 0;
-
-	/*
-	 * Write out all dirty pages to avoid race conditions
-	 * Then release them.
-	 */
-	if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
-		err = filemap_write_and_wait_range(mapping,
-			offset, offset + length - 1);
-		if (err)
-			return err;
-	}
-
-	mutex_lock(&inode->i_mutex);
-	/* It's not possible punch hole on append only file */
-	if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
-		err = -EPERM;
-		goto out_mutex;
-	}
-	if (IS_SWAPFILE(inode)) {
-		err = -ETXTBSY;
-		goto out_mutex;
-	}
-
-	/* No need to punch hole beyond i_size */
-	if (offset >= inode->i_size)
-		goto out_mutex;
-
-	/*
-	 * If the hole extents beyond i_size, set the hole
-	 * to end after the page that contains i_size
-	 */
-	if (offset + length > inode->i_size) {
-		length = inode->i_size +
-		    PAGE_CACHE_SIZE - (inode->i_size & (PAGE_CACHE_SIZE - 1)) -
-		    offset;
-	}
-
-	first_page = (offset + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
-	last_page = (offset + length) >> PAGE_CACHE_SHIFT;
-
-	first_page_offset = first_page << PAGE_CACHE_SHIFT;
-	last_page_offset = last_page << PAGE_CACHE_SHIFT;
-
-	/* Now release the pages */
-	if (last_page_offset > first_page_offset) {
-		truncate_pagecache_range(inode, first_page_offset,
-					 last_page_offset - 1);
-	}
-
-	/* Wait all existing dio works, newcomers will block on i_mutex */
-	inode_dio_wait(inode);
-
-	handle = start_transaction(inode);
-	if (IS_ERR(handle))
-		goto out_mutex;
-
-	/*
-	 * Now we need to zero out the non-page-aligned data in the
-	 * pages at the start and tail of the hole, and unmap the buffer
-	 * heads for the block aligned regions of the page that were
-	 * completely zerod.
-	 */
-	if (first_page > last_page) {
-		/*
-		 * If the file space being truncated is contained within a page
-		 * just zero out and unmap the middle of that page
-		 */
-		err = ext4_discard_partial_page_buffers(handle,
-			mapping, offset, length, 0);
-		if (err)
-			goto out;
-	} else {
-		/*
-		 * Zero out and unmap the paritial page that contains
-		 * the start of the hole
-		 */
-		page_len = first_page_offset - offset;
-		if (page_len > 0) {
-			err = ext4_discard_partial_page_buffers(handle, mapping,
-							offset, page_len, 0);
-			if (err)
-				goto out;
-		}
-
-		/*
-		 * Zero out and unmap the partial page that contains
-		 * the end of the hole
-		 */
-		page_len = offset + length - last_page_offset;
-		if (page_len > 0) {
-			err = ext4_discard_partial_page_buffers(handle, mapping,
-						last_page_offset, page_len, 0);
-			if (err)
-				goto out;
-		}
-	}
-
-	/*
-	 * If i_size contained in the last page, we need to
-	 * unmap and zero the paritial page after i_size
-	 */
-	if (inode->i_size >> PAGE_CACHE_SHIFT == last_page &&
-	    inode->i_size % PAGE_CACHE_SIZE != 0) {
-		page_len = PAGE_CACHE_SIZE -
-			(inode->i_size & (PAGE_CACHE_SIZE - 1));
-		if (page_len > 0) {
-			err = ext4_discard_partial_page_buffers(handle,
-				mapping, inode->i_size, page_len, 0);
-			if (err)
-				goto out;
-		}
-	}
-
-	first_block = (offset + sb->s_blocksize - 1) >>
-		EXT4_BLOCK_SIZE_BITS(sb);
-	stop_block = (offset + length) >> EXT4_BLOCK_SIZE_BITS(sb);
-
-	if (first_block >= stop_block)
-		goto out;
-
-	down_write(&EXT4_I(inode)->i_data_sem);
-	ext4_discard_preallocations(inode);
-
-	err = ext4_es_remove_extent(inode, first_block,
-				    stop_block - first_block);
-	err = ext4_free_hole_blocks(handle, inode, first_block, stop_block);
-
-	ext4_discard_preallocations(inode);
-
-	if (IS_SYNC(inode))
-		ext4_handle_sync(handle);
-
-	up_write(&EXT4_I(inode)->i_data_sem);
-
-out:
-	inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
-	ext4_mark_inode_dirty(handle, inode);
-	ext4_journal_stop(handle);
-
-out_mutex:
-	mutex_unlock(&inode->i_mutex);
-
-	return err;
-}
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index a4ffb47..11cf505 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3566,20 +3566,191 @@ int ext4_can_truncate(struct inode *inode)
 int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 {
 	struct inode *inode = file_inode(file);
+	struct super_block *sb = inode->i_sb;
+	ext4_lblk_t first_block, stop_block;
+	struct address_space *mapping = inode->i_mapping;
+	loff_t first_page, last_page, page_len;
+	loff_t first_page_offset, last_page_offset;
+	handle_t *handle;
+	unsigned int credits;
+	int ret = 0;
+
 	if (!S_ISREG(inode->i_mode))
 		return -EOPNOTSUPP;
 
-	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
-		return ext4_ind_punch_hole(file, offset, length);
-
-	if (EXT4_SB(inode->i_sb)->s_cluster_ratio > 1) {
+	if (EXT4_SB(sb)->s_cluster_ratio > 1) {
 		/* TODO: Add support for bigalloc file systems */
 		return -EOPNOTSUPP;
 	}
 
 	trace_ext4_punch_hole(inode, offset, length);
 
-	return ext4_ext_punch_hole(file, offset, length);
+	/*
+	 * Write out all dirty pages to avoid race conditions
+	 * Then release them.
+	 */
+	if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
+		ret = filemap_write_and_wait_range(mapping, offset,
+						   offset + length - 1);
+		if (ret)
+			return ret;
+	}
+
+	mutex_lock(&inode->i_mutex);
+	/* It's not possible punch hole on append only file */
+	if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) {
+		ret = -EPERM;
+		goto out_mutex;
+	}
+	if (IS_SWAPFILE(inode)) {
+		ret = -ETXTBSY;
+		goto out_mutex;
+	}
+
+	/* No need to punch hole beyond i_size */
+	if (offset >= inode->i_size)
+		goto out_mutex;
+
+	/*
+	 * If the hole extends beyond i_size, set the hole
+	 * to end after the page that contains i_size
+	 */
+	if (offset + length > inode->i_size) {
+		length = inode->i_size +
+		   PAGE_CACHE_SIZE - (inode->i_size & (PAGE_CACHE_SIZE - 1)) -
+		   offset;
+	}
+
+	first_page = (offset + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
+	last_page = (offset + length) >> PAGE_CACHE_SHIFT;
+
+	first_page_offset = first_page << PAGE_CACHE_SHIFT;
+	last_page_offset = last_page << PAGE_CACHE_SHIFT;
+
+	/* Now release the pages */
+	if (last_page_offset > first_page_offset) {
+		truncate_pagecache_range(inode, first_page_offset,
+					 last_page_offset - 1);
+	}
+
+	/* Wait all existing dio workers, newcomers will block on i_mutex */
+	ext4_inode_block_unlocked_dio(inode);
+	ret = ext4_flush_unwritten_io(inode);
+	if (ret)
+		goto out_dio;
+	inode_dio_wait(inode);
+
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
+		credits = ext4_writepage_trans_blocks(inode);
+	else
+		credits = ext4_blocks_for_truncate(inode);
+	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE,
+				    ext4_blocks_for_truncate(inode));
+	if (IS_ERR(handle)) {
+		ret = PTR_ERR(handle);
+		ext4_std_error(sb, ret);
+		goto out_dio;
+	}
+
+	/*
+	 * Now we need to zero out the non-page-aligned data in the
+	 * pages at the start and tail of the hole, and unmap the
+	 * buffer heads for the block aligned regions of the page that
+	 * were completely zeroed.
+	 */
+	if (first_page > last_page) {
+		/*
+		 * If the file space being truncated is contained
+		 * within a page just zero out and unmap the middle of
+		 * that page
+		 */
+		ret = ext4_discard_partial_page_buffers(handle,
+			mapping, offset, length, 0);
+
+		if (ret)
+			goto out_stop;
+	} else {
+		/*
+		 * zero out and unmap the partial page that contains
+		 * the start of the hole
+		 */
+		page_len = first_page_offset - offset;
+		if (page_len > 0) {
+			ret = ext4_discard_partial_page_buffers(handle, mapping,
+						offset, page_len, 0);
+			if (ret)
+				goto out_stop;
+		}
+
+		/*
+		 * zero out and unmap the partial page that contains
+		 * the end of the hole
+		 */
+		page_len = offset + length - last_page_offset;
+		if (page_len > 0) {
+			ret = ext4_discard_partial_page_buffers(handle, mapping,
+					last_page_offset, page_len, 0);
+			if (ret)
+				goto out_stop;
+		}
+	}
+
+	/*
+	 * If i_size is contained in the last page, we need to
+	 * unmap and zero the partial page after i_size
+	 */
+	if (inode->i_size >> PAGE_CACHE_SHIFT == last_page &&
+	   inode->i_size % PAGE_CACHE_SIZE != 0) {
+		page_len = PAGE_CACHE_SIZE -
+			(inode->i_size & (PAGE_CACHE_SIZE - 1));
+
+		if (page_len > 0) {
+			ret = ext4_discard_partial_page_buffers(handle,
+					mapping, inode->i_size, page_len, 0);
+
+			if (ret)
+				goto out_stop;
+		}
+	}
+
+	first_block = (offset + sb->s_blocksize - 1) >>
+		EXT4_BLOCK_SIZE_BITS(sb);
+	stop_block = (offset + length) >> EXT4_BLOCK_SIZE_BITS(sb);
+
+	/* If there are no blocks to remove, return now */
+	if (first_block >= stop_block)
+		goto out_stop;
+
+	down_write(&EXT4_I(inode)->i_data_sem);
+	ext4_discard_preallocations(inode);
+
+	ret = ext4_es_remove_extent(inode, first_block,
+				    stop_block - first_block);
+	if (ret) {
+		up_write(&EXT4_I(inode)->i_data_sem);
+		goto out_stop;
+	}
+
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
+		ret = ext4_ext_remove_space(inode, first_block,
+					    stop_block - 1);
+	else
+		ret = ext4_free_hole_blocks(handle, inode, first_block,
+					    stop_block);
+
+	ext4_discard_preallocations(inode);
+	if (IS_SYNC(inode))
+		ext4_handle_sync(handle);
+	up_write(&EXT4_I(inode)->i_data_sem);
+	inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
+	ext4_mark_inode_dirty(handle, inode);
+out_stop:
+	ext4_journal_stop(handle);
+out_dio:
+	ext4_inode_resume_unlocked_dio(inode);
+out_mutex:
+	mutex_unlock(&inode->i_mutex);
+	return ret;
 }
 
 /*
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH 4/7] ext4: refactor punch hole code

[Theodore Ts'o]

On Tue, Mar 26, 2013 at 01:54:19PM +0100, Lukáš Czerner wrote:
> > +	/* Wait all existing dio workers, newcomers will block on i_mutex */
> > +	ext4_inode_block_unlocked_dio(inode);
> 
> This was not present in the indirect punch hole code, does it means
> that there was a bug ? If so maybe it's worth mentioning in the
> description? 

Yes, I'm pretty sure it was a bug.  The
ext4_inode_block_unlocked_dio() call was added in the extents code
path by commit 02d262dffcf4c.  The problem is that i_mutex will not
block DIO readers in dioread_nolock mode.  One of the problems with
not having done the code refactorization earlier was that a bug fixed
in one code path doesn't necessarily get fixed in another.

I'll add a comment to this effect in the commit description.

> > +	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> > +		credits = ext4_writepage_trans_blocks(inode);
> > +	else
> > +		credits = ext4_blocks_for_truncate(inode);
> > +	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE,
> > +				    ext4_blocks_for_truncate(inode));
> 
> Hmm, shouldn't we be using 'credits' instead of
> ext4_blocks_for_truncate(inode) here ?

Yes, good catch!

Thanks,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] ext4: refactor punch hole code

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1882 bytes --]

On Tue, 26 Mar 2013, Theodore Ts'o wrote:

> Date: Tue, 26 Mar 2013 22:38:16 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Subject: Re: [PATCH 4/7] ext4: refactor punch hole code
> 
> On Tue, Mar 26, 2013 at 01:54:19PM +0100, Lukáš Czerner wrote:
> > > +	/* Wait all existing dio workers, newcomers will block on i_mutex */
> > > +	ext4_inode_block_unlocked_dio(inode);
> > 
> > This was not present in the indirect punch hole code, does it means
> > that there was a bug ? If so maybe it's worth mentioning in the
> > description? 
> 
> Yes, I'm pretty sure it was a bug.  The
> ext4_inode_block_unlocked_dio() call was added in the extents code
> path by commit 02d262dffcf4c.  The problem is that i_mutex will not
> block DIO readers in dioread_nolock mode.  One of the problems with
> not having done the code refactorization earlier was that a bug fixed
> in one code path doesn't necessarily get fixed in another.
> 
> I'll add a comment to this effect in the commit description.

Great, thanks.

> 
> > > +	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> > > +		credits = ext4_writepage_trans_blocks(inode);
> > > +	else
> > > +		credits = ext4_blocks_for_truncate(inode);
> > > +	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE,
> > > +				    ext4_blocks_for_truncate(inode));
> > 
> > Hmm, shouldn't we be using 'credits' instead of
> > ext4_blocks_for_truncate(inode) here ?
> 
> Yes, good catch!
> 
> Thanks,
> 

I am not sure why I do not see my mail on the ext4 list.. But
anyway, you've missed my last comment bellow this one.

-Lukas

> 						- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 5/7] ext4: refactor truncate code

[Theodore Ts'o]

Move common code in ext4_ind_truncate() and ext4_ext_truncate() into
ext4_truncate().  This saves over 60 lines of code.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/ext4.h     |  4 +--
 fs/ext4/extents.c  | 60 +------------------------------------
 fs/ext4/indirect.c | 88 +++---------------------------------------------------
 fs/ext4/inode.c    | 72 ++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 77 insertions(+), 147 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0649253..d05ba38 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2109,7 +2109,7 @@ extern ssize_t ext4_ind_direct_IO(int rw, struct kiocb *iocb,
 				unsigned long nr_segs);
 extern int ext4_ind_calc_metadata_amount(struct inode *inode, sector_t lblock);
 extern int ext4_ind_trans_blocks(struct inode *inode, int nrblocks, int chunk);
-extern void ext4_ind_truncate(struct inode *inode);
+extern void ext4_ind_truncate(handle_t *, struct inode *inode);
 extern int ext4_free_hole_blocks(handle_t *handle, struct inode *inode,
 				 ext4_lblk_t first, ext4_lblk_t stop);
 
@@ -2575,7 +2575,7 @@ extern int ext4_ext_index_trans_blocks(struct inode *inode, int nrblocks,
 				       int chunk);
 extern int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			       struct ext4_map_blocks *map, int flags);
-extern void ext4_ext_truncate(struct inode *);
+extern void ext4_ext_truncate(handle_t *, struct inode *);
 extern int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
 				 ext4_lblk_t end);
 extern void ext4_ext_init(struct super_block *);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 8f6b3de..57cb4b7 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4254,48 +4254,13 @@ out3:
 	return err ? err : allocated;
 }
 
-void ext4_ext_truncate(struct inode *inode)
+void ext4_ext_truncate(handle_t *handle, struct inode *inode)
 {
-	struct address_space *mapping = inode->i_mapping;
 	struct super_block *sb = inode->i_sb;
 	ext4_lblk_t last_block;
-	handle_t *handle;
-	loff_t page_len;
 	int err = 0;
 
 	/*
-	 * finish any pending end_io work so we won't run the risk of
-	 * converting any truncated blocks to initialized later
-	 */
-	ext4_flush_unwritten_io(inode);
-
-	/*
-	 * probably first extent we're gonna free will be last in block
-	 */
-	err = ext4_writepage_trans_blocks(inode);
-	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, err);
-	if (IS_ERR(handle))
-		return;
-
-	if (inode->i_size % PAGE_CACHE_SIZE != 0) {
-		page_len = PAGE_CACHE_SIZE -
-			(inode->i_size & (PAGE_CACHE_SIZE - 1));
-
-		err = ext4_discard_partial_page_buffers(handle,
-			mapping, inode->i_size, page_len, 0);
-
-		if (err)
-			goto out_stop;
-	}
-
-	if (ext4_orphan_add(handle, inode))
-		goto out_stop;
-
-	down_write(&EXT4_I(inode)->i_data_sem);
-
-	ext4_discard_preallocations(inode);
-
-	/*
 	 * TODO: optimization is possible here.
 	 * Probably we need not scan at all,
 	 * because page truncation is enough.
@@ -4310,29 +4275,6 @@ void ext4_ext_truncate(struct inode *inode)
 	err = ext4_es_remove_extent(inode, last_block,
 				    EXT_MAX_BLOCKS - last_block);
 	err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
-
-	/* In a multi-transaction truncate, we only make the final
-	 * transaction synchronous.
-	 */
-	if (IS_SYNC(inode))
-		ext4_handle_sync(handle);
-
-	up_write(&EXT4_I(inode)->i_data_sem);
-
-out_stop:
-	/*
-	 * If this was a simple ftruncate() and the file will remain alive,
-	 * then we need to clear up the orphan record which we created above.
-	 * However, if this was a real unlink then we were called by
-	 * ext4_delete_inode(), and we allow that function to clean up the
-	 * orphan info for us.
-	 */
-	if (inode->i_nlink)
-		ext4_orphan_del(handle, inode);
-
-	inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
-	ext4_mark_inode_dirty(handle, inode);
-	ext4_journal_stop(handle);
 }
 
 static void ext4_falloc_update_inode(struct inode *inode,
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 926028c..80a798b 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -806,26 +806,9 @@ int ext4_ind_trans_blocks(struct inode *inode, int nrblocks, int chunk)
  * be able to restart the transaction at a conventient checkpoint to make
  * sure we don't overflow the journal.
  *
- * start_transaction gets us a new handle for a truncate transaction,
- * and extend_transaction tries to extend the existing one a bit.  If
+ * Try to extend this transaction for the purposes of truncation.  If
  * extend fails, we need to propagate the failure up and restart the
  * transaction in the top-level truncate loop. --sct
- */
-static handle_t *start_transaction(struct inode *inode)
-{
-	handle_t *result;
-
-	result = ext4_journal_start(inode, EXT4_HT_TRUNCATE,
-				    ext4_blocks_for_truncate(inode));
-	if (!IS_ERR(result))
-		return result;
-
-	ext4_std_error(inode->i_sb, PTR_ERR(result));
-	return result;
-}
-
-/*
- * Try to extend this transaction for the purposes of truncation.
  *
  * Returns 0 if we managed to create more room.  If we can't create more
  * room, and the transaction must be restarted we return 1.
@@ -1218,68 +1201,30 @@ static void ext4_free_branches(handle_t *handle, struct inode *inode,
 	}
 }
 
-void ext4_ind_truncate(struct inode *inode)
+void ext4_ind_truncate(handle_t *handle, struct inode *inode)
 {
-	handle_t *handle;
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	__le32 *i_data = ei->i_data;
 	int addr_per_block = EXT4_ADDR_PER_BLOCK(inode->i_sb);
-	struct address_space *mapping = inode->i_mapping;
 	ext4_lblk_t offsets[4];
 	Indirect chain[4];
 	Indirect *partial;
 	__le32 nr = 0;
 	int n = 0;
 	ext4_lblk_t last_block, max_block;
-	loff_t page_len;
 	unsigned blocksize = inode->i_sb->s_blocksize;
-	int err;
-
-	handle = start_transaction(inode);
-	if (IS_ERR(handle))
-		return;		/* AKPM: return what? */
 
 	last_block = (inode->i_size + blocksize-1)
 					>> EXT4_BLOCK_SIZE_BITS(inode->i_sb);
 	max_block = (EXT4_SB(inode->i_sb)->s_bitmap_maxbytes + blocksize-1)
 					>> EXT4_BLOCK_SIZE_BITS(inode->i_sb);
 
-	if (inode->i_size % PAGE_CACHE_SIZE != 0) {
-		page_len = PAGE_CACHE_SIZE -
-			(inode->i_size & (PAGE_CACHE_SIZE - 1));
-
-		err = ext4_discard_partial_page_buffers(handle,
-			mapping, inode->i_size, page_len, 0);
-
-		if (err)
-			goto out_stop;
-	}
-
 	if (last_block != max_block) {
 		n = ext4_block_to_path(inode, last_block, offsets, NULL);
 		if (n == 0)
-			goto out_stop;	/* error */
+			return;
 	}
 
-	/*
-	 * OK.  This truncate is going to happen.  We add the inode to the
-	 * orphan list, so that if this truncate spans multiple transactions,
-	 * and we crash, we will resume the truncate when the filesystem
-	 * recovers.  It also marks the inode dirty, to catch the new size.
-	 *
-	 * Implication: the file must always be in a sane, consistent
-	 * truncatable state while each transaction commits.
-	 */
-	if (ext4_orphan_add(handle, inode))
-		goto out_stop;
-
-	/*
-	 * From here we block out all ext4_get_block() callers who want to
-	 * modify the block allocation tree.
-	 */
-	down_write(&ei->i_data_sem);
-
-	ext4_discard_preallocations(inode);
 	ext4_es_remove_extent(inode, last_block, EXT_MAX_BLOCKS - last_block);
 
 	/*
@@ -1296,7 +1241,7 @@ void ext4_ind_truncate(struct inode *inode)
 		 * It is unnecessary to free any data blocks if last_block is
 		 * equal to the indirect block limit.
 		 */
-		goto out_unlock;
+		return;
 	} else if (n == 1) {		/* direct blocks */
 		ext4_free_data(handle, inode, NULL, i_data+offsets[0],
 			       i_data + EXT4_NDIR_BLOCKS);
@@ -1356,31 +1301,6 @@ do_indirects:
 	case EXT4_TIND_BLOCK:
 		;
 	}
-
-out_unlock:
-	up_write(&ei->i_data_sem);
-	inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
-	ext4_mark_inode_dirty(handle, inode);
-
-	/*
-	 * In a multi-transaction truncate, we only make the final transaction
-	 * synchronous
-	 */
-	if (IS_SYNC(inode))
-		ext4_handle_sync(handle);
-out_stop:
-	/*
-	 * If this was a simple ftruncate(), and the file will remain alive
-	 * then we need to clear up the orphan record which we created above.
-	 * However, if this was a real unlink then we were called by
-	 * ext4_delete_inode(), and we allow that function to clean up the
-	 * orphan info for us.
-	 */
-	if (inode->i_nlink)
-		ext4_orphan_del(handle, inode);
-
-	ext4_journal_stop(handle);
-	trace_ext4_truncate_exit(inode);
 }
 
 static int free_hole_blocks(handle_t *handle, struct inode *inode,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 11cf505..ab20015 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3783,6 +3783,12 @@ out_mutex:
  */
 void ext4_truncate(struct inode *inode)
 {
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	unsigned int credits;
+	handle_t *handle;
+	struct address_space *mapping = inode->i_mapping;
+	loff_t page_len;
+
 	trace_ext4_truncate_enter(inode);
 
 	if (!ext4_can_truncate(inode))
@@ -3801,10 +3807,72 @@ void ext4_truncate(struct inode *inode)
 			return;
 	}
 
+	/*
+	 * finish any pending end_io work so we won't run the risk of
+	 * converting any truncated blocks to initialized later
+	 */
+	ext4_flush_unwritten_io(inode);
+
 	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
-		ext4_ext_truncate(inode);
+		credits = ext4_writepage_trans_blocks(inode);
 	else
-		ext4_ind_truncate(inode);
+		credits = ext4_blocks_for_truncate(inode);
+
+	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
+	if (IS_ERR(handle)) {
+		ext4_std_error(inode->i_sb, PTR_ERR(handle));
+		return;
+	}
+
+	if (inode->i_size % PAGE_CACHE_SIZE != 0) {
+		page_len = PAGE_CACHE_SIZE -
+			(inode->i_size & (PAGE_CACHE_SIZE - 1));
+
+		if (ext4_discard_partial_page_buffers(handle,
+				mapping, inode->i_size, page_len, 0))
+			goto out_stop;
+	}
+
+	/*
+	 * We add the inode to the orphan list, so that if this
+	 * truncate spans multiple transactions, and we crash, we will
+	 * resume the truncate when the filesystem recovers.  It also
+	 * marks the inode dirty, to catch the new size.
+	 *
+	 * Implication: the file must always be in a sane, consistent
+	 * truncatable state while each transaction commits.
+	 */
+	if (ext4_orphan_add(handle, inode))
+		goto out_stop;
+
+	down_write(&EXT4_I(inode)->i_data_sem);
+
+	ext4_discard_preallocations(inode);
+
+	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
+		ext4_ext_truncate(handle, inode);
+	else
+		ext4_ind_truncate(handle, inode);
+
+	up_write(&ei->i_data_sem);
+
+	if (IS_SYNC(inode))
+		ext4_handle_sync(handle);
+
+out_stop:
+	/*
+	 * If this was a simple ftruncate() and the file will remain alive,
+	 * then we need to clear up the orphan record which we created above.
+	 * However, if this was a real unlink then we were called by
+	 * ext4_delete_inode(), and we allow that function to clean up the
+	 * orphan info for us.
+	 */
+	if (inode->i_nlink)
+		ext4_orphan_del(handle, inode);
+
+	inode->i_mtime = inode->i_ctime = ext4_current_time(inode);
+	ext4_mark_inode_dirty(handle, inode);
+	ext4_journal_stop(handle);
 
 	trace_ext4_truncate_exit(inode);
 }
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH 5/7] ext4: refactor truncate code

[Theodore Ts'o]

On Wed, Mar 27, 2013 at 11:53:42AM +0100, Lukáš Czerner wrote:
> > +	up_write(&ei->i_data_sem);
> 
> In ext_truncate we used to unlock it after the ext4_handle_sync(),
> however in ind_truncate we used to unlock it before the
> ext4_handle_sync(). Which one is correct ? I guess it does not have
> to be done under the i_data_sem, so maybe we can move it outside the
> semaphore in the punch_hole code as well ?

Yes, we can move this outside of the semaphore protected code.  Given
that ext4_handle_sync() an inline function which sets a single memory
location, my guess is that it didn't make a huge amount of difference,
but it's better to keep the critical section as small as possible.
I'll make that change.


Hmm.... one thing I'm not sure about, now that I'm looking at this
code.  We call ext4_discard_preallocations() twice; once before we
remove the extent, and once afterwards.  I'm not sure why we're doing
that.  It doesn't look to me like ext4_free_blocks() ever releases
blocks back to the preallocation space.  Am I missing something, or
could we eliminate one of the calls to ext4_discard_preallocations()?

      	 	       	      	       - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] ext4: refactor truncate code

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2384 bytes --]

On Wed, 27 Mar 2013, Theodore Ts'o wrote:

> Date: Wed, 27 Mar 2013 08:36:51 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Subject: Re: [PATCH 5/7] ext4: refactor truncate code
> 
> On Wed, Mar 27, 2013 at 11:53:42AM +0100, Lukáš Czerner wrote:
> > > +	up_write(&ei->i_data_sem);
> > 
> > In ext_truncate we used to unlock it after the ext4_handle_sync(),
> > however in ind_truncate we used to unlock it before the
> > ext4_handle_sync(). Which one is correct ? I guess it does not have
> > to be done under the i_data_sem, so maybe we can move it outside the
> > semaphore in the punch_hole code as well ?
> 
> Yes, we can move this outside of the semaphore protected code.  Given
> that ext4_handle_sync() an inline function which sets a single memory
> location, my guess is that it didn't make a huge amount of difference,
> but it's better to keep the critical section as small as possible.
> I'll make that change.
> 
> 
> Hmm.... one thing I'm not sure about, now that I'm looking at this
> code.  We call ext4_discard_preallocations() twice; once before we
> remove the extent, and once afterwards.  I'm not sure why we're doing
> that.  It doesn't look to me like ext4_free_blocks() ever releases
> blocks back to the preallocation space.  Am I missing something, or
> could we eliminate one of the calls to ext4_discard_preallocations()?

I was wondering about that as well. There is a possibility of
allocation occurring in ext4_ext_remove_space() however it is only
metadata allocation and as such there should be no preallocation so
it seems to me that the second ext4_discard_preallocations() is
unnecessary. Note that it has been there from the introduction of
punch hole.

However let's take it one step further. What about the first call of
ext4_discard_preallocations() is this entirely necessary for a punch
hole ? I am wondering whether we can optimize things by dropping the
preallocation only within the hole we're punching, or possibly only
when we punch a hole at the end, or past the end of the file.

-Lukas

> 
>       	 	       	      	       - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 6/7] ext4: add mutex_is_locked() assertion to ext4_truncate()

[Theodore Ts'o]

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/inode.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ab20015..eb9a5a9 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -258,8 +258,21 @@ void ext4_evict_inode(struct inode *inode)
 			     "couldn't mark inode dirty (err %d)", err);
 		goto stop_handle;
 	}
-	if (inode->i_blocks)
+	if (inode->i_blocks) {
+		/*
+		 * Since we are evicting the inode, it shouldn't be
+		 * locked.  We've added a warning which triggers if
+		 * the mutex is not locked, so take the lock even
+		 * though it's not strictly necessary.  However,
+		 * taking the lock using a simple mutex_lock() will
+		 * trigger a (false positive) lockdep warning, so take
+		 * it using a trylock.
+		 */
+		int locked = mutex_trylock(&inode->i_mutex);
 		ext4_truncate(inode);
+		if (likely(locked))
+			mutex_unlock(&inode->i_mutex);
+	}
 
 	/*
 	 * ext4_ext_truncate() doesn't reserve any slop when it
@@ -3789,6 +3802,7 @@ void ext4_truncate(struct inode *inode)
 	struct address_space *mapping = inode->i_mapping;
 	loff_t page_len;
 
+	WARN_ON_ONCE(!mutex_is_locked(&inode->i_mutex));
 	trace_ext4_truncate_enter(inode);
 
 	if (!ext4_can_truncate(inode))
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH 6/7] ext4: add mutex_is_locked() assertion to ext4_truncate()

[Lukáš Czerner]

On Sun, 24 Mar 2013, Theodore Ts'o wrote:

> Date: Sun, 24 Mar 2013 20:06:53 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Subject: [PATCH 6/7] ext4: add mutex_is_locked() assertion to ext4_truncate()
> 

Hi Ted,

I have to admit I do not necessarily understand the reason for this.
Have you seen any specific problem with mutex not being locked in
the truncation path ? I understand that it should be locked, but
there are lot of places where thing should be locked and we do not
usually check them, especially since we need special hook to "fool"
the check. Also there are not a lot of places we call truncate from.

I am not necessarily against the check, defensive programming is
proven to be very useful, but I would like to know why this one ?
Also, having mutex not locked in the truncation path is a bug so I
am not sure why we only do WARN_ON_ONCE() which can be easily missed
? Can we do WARN_ON(), or WARN_ON_ONCE() + ext4_warning() ?

Thanks!
-Lukas

> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/inode.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ab20015..eb9a5a9 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -258,8 +258,21 @@ void ext4_evict_inode(struct inode *inode)
>  			     "couldn't mark inode dirty (err %d)", err);
>  		goto stop_handle;
>  	}
> -	if (inode->i_blocks)
> +	if (inode->i_blocks) {
> +		/*
> +		 * Since we are evicting the inode, it shouldn't be
> +		 * locked.  We've added a warning which triggers if
> +		 * the mutex is not locked, so take the lock even
> +		 * though it's not strictly necessary.  However,
> +		 * taking the lock using a simple mutex_lock() will
> +		 * trigger a (false positive) lockdep warning, so take
> +		 * it using a trylock.
> +		 */
> +		int locked = mutex_trylock(&inode->i_mutex);
>  		ext4_truncate(inode);
> +		if (likely(locked))
> +			mutex_unlock(&inode->i_mutex);
> +	}
>  
>  	/*
>  	 * ext4_ext_truncate() doesn't reserve any slop when it
> @@ -3789,6 +3802,7 @@ void ext4_truncate(struct inode *inode)
>  	struct address_space *mapping = inode->i_mapping;
>  	loff_t page_len;
>  
> +	WARN_ON_ONCE(!mutex_is_locked(&inode->i_mutex));
>  	trace_ext4_truncate_enter(inode);
>  
>  	if (!ext4_can_truncate(inode))
> 

Re: [PATCH 6/7] ext4: add mutex_is_locked() assertion to ext4_truncate()

[Theodore Ts'o]

On Tue, Mar 26, 2013 at 10:31:29AM +0100, Lukáš Czerner wrote:
> 
> I have to admit I do not necessarily understand the reason for this.
> Have you seen any specific problem with mutex not being locked in
> the truncation path?

This was really me being paranoid more than anything else and wanting
to add more defensive programming.  I was originally just going to add
a comment, but decided it was better to put the assertion into the
code.

> Also, having mutex not locked in the truncation path is a bug so I
> am not sure why we only do WARN_ON_ONCE() which can be easily missed
> ? Can we do WARN_ON(), or WARN_ON_ONCE() + ext4_warning() ?

Agreed, WARN_ON() is probably more appropriate here.

							- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/7] ext4: add might_sleep() annotations

[Theodore Ts'o]

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/ext4_jbd2.c | 6 ++++++
 fs/ext4/mballoc.c   | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 7058975..0e1dc9e 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -43,6 +43,8 @@ handle_t *__ext4_journal_start_sb(struct super_block *sb, unsigned int line,
 {
 	journal_t *journal;
 
+	might_sleep();
+
 	trace_ext4_journal_start(sb, nblocks, _RET_IP_);
 	if (sb->s_flags & MS_RDONLY)
 		return ERR_PTR(-EROFS);
@@ -113,6 +115,8 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line,
 {
 	int err = 0;
 
+	might_sleep();
+
 	if (ext4_handle_valid(handle)) {
 		err = jbd2_journal_get_write_access(handle, bh);
 		if (err)
@@ -209,6 +213,8 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
 {
 	int err = 0;
 
+	might_sleep();
+
 	if (ext4_handle_valid(handle)) {
 		err = jbd2_journal_dirty_metadata(handle, bh);
 		if (err) {
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index ee6614b..36c82a3 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1011,6 +1011,7 @@ int ext4_mb_init_group(struct super_block *sb, ext4_group_t group)
 	struct page *page;
 	int ret = 0;
 
+	might_sleep();
 	mb_debug(1, "init group %u\n", group);
 	this_grp = ext4_get_group_info(sb, group);
 	/*
@@ -1082,6 +1083,7 @@ ext4_mb_load_buddy(struct super_block *sb, ext4_group_t group,
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct inode *inode = sbi->s_buddy_cache;
 
+	might_sleep();
 	mb_debug(1, "load group %u\n", group);
 
 	blocks_per_page = PAGE_CACHE_SIZE / sb->s_blocksize;
@@ -4217,6 +4219,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
 	unsigned int inquota = 0;
 	unsigned int reserv_clstrs = 0;
 
+	might_sleep();
 	sb = ar->inode->i_sb;
 	sbi = EXT4_SB(sb);
 
@@ -4470,6 +4473,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
 	int err = 0;
 	int ret;
 
+	might_sleep();
 	if (bh) {
 		if (block)
 			BUG_ON(block != bh->b_blocknr);
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH 7/7] ext4: add might_sleep() annotations

[Lukáš Czerner]

On Sun, 24 Mar 2013, Theodore Ts'o wrote:

> Date: Sun, 24 Mar 2013 20:06:54 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Subject: [PATCH 7/7] ext4: add might_sleep() annotations

Looks good and useful.

Thanks!
Signed-off-by: Lukas Czerner <lczerner@redhat.com>


> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/ext4_jbd2.c | 6 ++++++
>  fs/ext4/mballoc.c   | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index 7058975..0e1dc9e 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -43,6 +43,8 @@ handle_t *__ext4_journal_start_sb(struct super_block *sb, unsigned int line,
>  {
>  	journal_t *journal;
>  
> +	might_sleep();
> +
>  	trace_ext4_journal_start(sb, nblocks, _RET_IP_);
>  	if (sb->s_flags & MS_RDONLY)
>  		return ERR_PTR(-EROFS);
> @@ -113,6 +115,8 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line,
>  {
>  	int err = 0;
>  
> +	might_sleep();
> +
>  	if (ext4_handle_valid(handle)) {
>  		err = jbd2_journal_get_write_access(handle, bh);
>  		if (err)
> @@ -209,6 +213,8 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
>  {
>  	int err = 0;
>  
> +	might_sleep();
> +
>  	if (ext4_handle_valid(handle)) {
>  		err = jbd2_journal_dirty_metadata(handle, bh);
>  		if (err) {
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index ee6614b..36c82a3 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -1011,6 +1011,7 @@ int ext4_mb_init_group(struct super_block *sb, ext4_group_t group)
>  	struct page *page;
>  	int ret = 0;
>  
> +	might_sleep();
>  	mb_debug(1, "init group %u\n", group);
>  	this_grp = ext4_get_group_info(sb, group);
>  	/*
> @@ -1082,6 +1083,7 @@ ext4_mb_load_buddy(struct super_block *sb, ext4_group_t group,
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	struct inode *inode = sbi->s_buddy_cache;
>  
> +	might_sleep();
>  	mb_debug(1, "load group %u\n", group);
>  
>  	blocks_per_page = PAGE_CACHE_SIZE / sb->s_blocksize;
> @@ -4217,6 +4219,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
>  	unsigned int inquota = 0;
>  	unsigned int reserv_clstrs = 0;
>  
> +	might_sleep();
>  	sb = ar->inode->i_sb;
>  	sbi = EXT4_SB(sb);
>  
> @@ -4470,6 +4473,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
>  	int err = 0;
>  	int ret;
>  
> +	might_sleep();
>  	if (bh) {
>  		if (block)
>  			BUG_ON(block != bh->b_blocknr);
> 

Re: [PATCH 7/7] ext4: add might_sleep() annotations

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3314 bytes --]

On Tue, 26 Mar 2013, Lukáš Czerner wrote:

> Date: Tue, 26 Mar 2013 10:48:00 +0100 (CET)
> From: Lukáš Czerner <lczerner@redhat.com>
> To: Theodore Ts'o <tytso@mit.edu>
> Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>
> Subject: Re: [PATCH 7/7] ext4: add might_sleep() annotations
> 
> On Sun, 24 Mar 2013, Theodore Ts'o wrote:
> 
> > Date: Sun, 24 Mar 2013 20:06:54 -0400
> > From: Theodore Ts'o <tytso@mit.edu>
> > To: Ext4 Developers List <linux-ext4@vger.kernel.org>
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Subject: [PATCH 7/7] ext4: add might_sleep() annotations
> 
> Looks good and useful.
> 
> Thanks!
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

oops, wrong tag sorry :) I meant to say

Reviewed-by: Lukas Czerner <lczerner@redhat.com>

> 
> 
> > 
> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> > ---
> >  fs/ext4/ext4_jbd2.c | 6 ++++++
> >  fs/ext4/mballoc.c   | 4 ++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> > index 7058975..0e1dc9e 100644
> > --- a/fs/ext4/ext4_jbd2.c
> > +++ b/fs/ext4/ext4_jbd2.c
> > @@ -43,6 +43,8 @@ handle_t *__ext4_journal_start_sb(struct super_block *sb, unsigned int line,
> >  {
> >  	journal_t *journal;
> >  
> > +	might_sleep();
> > +
> >  	trace_ext4_journal_start(sb, nblocks, _RET_IP_);
> >  	if (sb->s_flags & MS_RDONLY)
> >  		return ERR_PTR(-EROFS);
> > @@ -113,6 +115,8 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line,
> >  {
> >  	int err = 0;
> >  
> > +	might_sleep();
> > +
> >  	if (ext4_handle_valid(handle)) {
> >  		err = jbd2_journal_get_write_access(handle, bh);
> >  		if (err)
> > @@ -209,6 +213,8 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
> >  {
> >  	int err = 0;
> >  
> > +	might_sleep();
> > +
> >  	if (ext4_handle_valid(handle)) {
> >  		err = jbd2_journal_dirty_metadata(handle, bh);
> >  		if (err) {
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index ee6614b..36c82a3 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -1011,6 +1011,7 @@ int ext4_mb_init_group(struct super_block *sb, ext4_group_t group)
> >  	struct page *page;
> >  	int ret = 0;
> >  
> > +	might_sleep();
> >  	mb_debug(1, "init group %u\n", group);
> >  	this_grp = ext4_get_group_info(sb, group);
> >  	/*
> > @@ -1082,6 +1083,7 @@ ext4_mb_load_buddy(struct super_block *sb, ext4_group_t group,
> >  	struct ext4_sb_info *sbi = EXT4_SB(sb);
> >  	struct inode *inode = sbi->s_buddy_cache;
> >  
> > +	might_sleep();
> >  	mb_debug(1, "load group %u\n", group);
> >  
> >  	blocks_per_page = PAGE_CACHE_SIZE / sb->s_blocksize;
> > @@ -4217,6 +4219,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
> >  	unsigned int inquota = 0;
> >  	unsigned int reserv_clstrs = 0;
> >  
> > +	might_sleep();
> >  	sb = ar->inode->i_sb;
> >  	sbi = EXT4_SB(sb);
> >  
> > @@ -4470,6 +4473,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
> >  	int err = 0;
> >  	int ret;
> >  
> > +	might_sleep();
> >  	if (bh) {
> >  		if (block)
> >  			BUG_ON(block != bh->b_blocknr);
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>