[PATCH v3 0/4] ext4: fix a data corruption problem

[PATCH v3 0/4] ext4: fix a data corruption problem

[zhangyi (F)]

Hi:

This patch set is the third version which want to fix a data corruption,
have been testd by xfstests.

 - The first patch fix the data corruption problem we captured. The root
   case is that we forgot to clear dirty flag when revorking a buffer
   belongs to older transaction.
 - The second and third patches are cleanup patches which remove
   clean_bdev_aliases() related calls and move the logic to
   jbd2_journal_forget() when forgetting metadata (suggested by Jan Kara).
 - The last patch change the return value of ext4_ext_convert_to_initialized().

Please review.

Thanks,
Yi.

----------

Changes since v2:
 - Change the commit log and comments in the first patch.
 - Add the three cleanup patches to remove all clean_bdev_aliases() calls
   and change the return value of ext4_ext_convert_to_initialized().


zhangyi (F) (4):
  jbd2: make sure dirty flag is cleared while revorking a buffer which
    belongs to older transaction
  jbd2: discard dirty data when forgetting an un-journalled buffer
  ext4: cleanup clean_bdev_aliases() calls
  ext4: convert ext4_split_extent() to return requested length

 fs/ext4/extents.c     | 26 +++++++---------------
 fs/ext4/inode.c       |  7 ------
 fs/ext4/page-io.c     |  4 +---
 fs/jbd2/transaction.c | 60 +++++++++++++++++++++++++++++++++++++++++++--------
 4 files changed, 60 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] jbd2: make sure dirty flag is cleared while revorking a buffer which belongs to older transaction

[zhangyi (F)]

Now, we capture a data corruption problem on ext4 while we're truncating
an extent index block. Imaging that if we are revoking a buffer which
has been journaled by the committing transaction, the buffer's jbddirty
flag will not be cleared in jbd2_journal_forget(), so the commit code
will set the buffer dirty flag again after refile the buffer.

fsx                               kjournald2
                                  jbd2_journal_commit_transaction
jbd2_journal_revoke                commit phase 1~5...
 jbd2_journal_forget
   belongs to older transaction    commit phase 6
   jbddirty not clear               __jbd2_journal_refile_buffer
                                     __jbd2_journal_unfile_buffer
                                      test_clear_buffer_jbddirty
                                       mark_buffer_dirty

Finally, if the freed extent index block was allocated again as data
block by some other files, it may corrupt the file data after writing
cached pages later, such as during unmount time. (In general,
clean_bdev_aliases() related helpers should be invoked after
re-allocation to prevent the above corruption, but unfortunately we
missed it when zeroout the head of extra extent blocks in
ext4_ext_handle_unwritten_extents()).

This patch mark buffer as freed and set j_next_transaction to the new
transaction when it already belongs to the committing transaction in
jbd2_journal_forget(), so that commit code knows it should clear dirty
bits when it is done with the buffer.

This problem can be reproduced by xfstests generic/455 easily with
seeds (3246 3247 3248 3249).

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
Cc: stable@vger.kernel.org
---
 fs/jbd2/transaction.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index f07f006..0c0cbda 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -1609,14 +1609,21 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh)
 		/* However, if the buffer is still owned by a prior
 		 * (committing) transaction, we can't drop it yet... */
 		JBUFFER_TRACE(jh, "belongs to older transaction");
-		/* ... but we CAN drop it from the new transaction if we
-		 * have also modified it since the original commit. */
+		/* ... but we CAN drop it from the new transaction through
+		 * marking the buffer as freed and set j_next_transaction to
+		 * the new transaction, so that not only the commit code
+		 * knows it should clear dirty bits when it is done with the
+		 * buffer, but also we can avoid this buffer be checkpointed
+		 * without writing out before the new transaction complete. */
 
-		if (jh->b_next_transaction) {
-			J_ASSERT(jh->b_next_transaction == transaction);
+		set_buffer_freed(bh);
+
+		if (!jh->b_next_transaction) {
 			spin_lock(&journal->j_list_lock);
-			jh->b_next_transaction = NULL;
+			jh->b_next_transaction = transaction;
 			spin_unlock(&journal->j_list_lock);
+		} else {
+			J_ASSERT(jh->b_next_transaction == transaction);
 
 			/*
 			 * only drop a reference if this transaction modified
-- 
2.7.4

Re: [PATCH v3 1/4] jbd2: make sure dirty flag is cleared while revorking a buffer which belongs to older transaction

[Jan Kara]

On Fri 25-01-19 20:30:53, zhangyi (F) wrote:
> Now, we capture a data corruption problem on ext4 while we're truncating
> an extent index block. Imaging that if we are revoking a buffer which
> has been journaled by the committing transaction, the buffer's jbddirty
> flag will not be cleared in jbd2_journal_forget(), so the commit code
> will set the buffer dirty flag again after refile the buffer.
> 
> fsx                               kjournald2
>                                   jbd2_journal_commit_transaction
> jbd2_journal_revoke                commit phase 1~5...
>  jbd2_journal_forget
>    belongs to older transaction    commit phase 6
>    jbddirty not clear               __jbd2_journal_refile_buffer
>                                      __jbd2_journal_unfile_buffer
>                                       test_clear_buffer_jbddirty
>                                        mark_buffer_dirty
> 
> Finally, if the freed extent index block was allocated again as data
> block by some other files, it may corrupt the file data after writing
> cached pages later, such as during unmount time. (In general,
> clean_bdev_aliases() related helpers should be invoked after
> re-allocation to prevent the above corruption, but unfortunately we
> missed it when zeroout the head of extra extent blocks in
> ext4_ext_handle_unwritten_extents()).
> 
> This patch mark buffer as freed and set j_next_transaction to the new
> transaction when it already belongs to the committing transaction in
> jbd2_journal_forget(), so that commit code knows it should clear dirty
> bits when it is done with the buffer.
> 
> This problem can be reproduced by xfstests generic/455 easily with
> seeds (3246 3247 3248 3249).
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> Cc: stable@vger.kernel.org

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

Just one comment below to make the comment more readable:

> @@ -1609,14 +1609,21 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh)
>  		/* However, if the buffer is still owned by a prior
>  		 * (committing) transaction, we can't drop it yet... */
>  		JBUFFER_TRACE(jh, "belongs to older transaction");
> -		/* ... but we CAN drop it from the new transaction if we
> -		 * have also modified it since the original commit. */
> +		/* ... but we CAN drop it from the new transaction through
> +		 * marking the buffer as freed and set j_next_transaction to
> +		 * the new transaction, so that not only the commit code
> +		 * knows it should clear dirty bits when it is done with the
> +		 * buffer, but also we can avoid this buffer be checkpointed
> +		 * without writing out before the new transaction complete. */

.... but also the buffer can be checkpointed only after the new transaction
commits.

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

[PATCH v3 2/4] jbd2: discard dirty data when forgetting an un-journalled buffer

[zhangyi (F)]

We do not unmap and clear dirty flag when forgetting a buffer without
journal or does not belongs to any transaction, so the invalid dirty
data may still be written to the disk later. It's fine if the
corresponding block is never used before the next mount, and it's also
fine that we invoke clean_bdev_aliases() related functions to unmap
the block device mapping when re-allocating such freed block as data
block. But this logic is somewhat fragile and risky that may lead to
data corruption if we forget to clean bdev aliases. So, It's better to
discard dirty data during forget time.

We have been already handled all the cases of forgetting journalled
buffer, this patch deal with the remaining two cases.

- buffer is not journalled yet,
- buffer is journalled but doesn't belongs to any transaction.

We invoke __bforget() instead of __brelese() when forgetting an
un-journalled buffer in jbd2_journal_forget(). After this patch we can
remove all clean_bdev_aliases() related calls in ext4.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/jbd2/transaction.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 0c0cbda..8825d45 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -1597,9 +1597,7 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh)
 			__jbd2_journal_unfile_buffer(jh);
 			if (!buffer_jbd(bh)) {
 				spin_unlock(&journal->j_list_lock);
-				jbd_unlock_bh_state(bh);
-				__bforget(bh);
-				goto drop;
+				goto not_jbd;
 			}
 		}
 		spin_unlock(&journal->j_list_lock);
@@ -1632,9 +1630,41 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh)
 			if (was_modified)
 				drop_reserve = 1;
 		}
+	} else {
+		/*
+		 * Finally, if the buffer is not belongs to any
+		 * transaction, we can just drop it now if it has no
+		 * checkpoint.
+		 */
+		spin_lock(&journal->j_list_lock);
+		if (!jh->b_cp_transaction) {
+			JBUFFER_TRACE(jh, "belongs to none transaction");
+			spin_unlock(&journal->j_list_lock);
+			goto not_jbd;
+		}
+
+		/*
+		 * Otherwise, if the buffer has been written to disk,
+		 * it is safe to remove the checkpoint and drop it.
+		 */
+		if (!buffer_dirty(bh)) {
+			__jbd2_journal_remove_checkpoint(jh);
+			spin_unlock(&journal->j_list_lock);
+			goto not_jbd;
+		}
+
+		/*
+		 * The buffer is still not written to disk, we should
+		 * attach this buffer to current transaction to prevent
+		 * missing writing back when doing checkpoint before
+		 * the current transaction complete submittion.
+		 */
+		__jbd2_journal_temp_unlink_buffer(jh);
+		clear_buffer_dirty(bh);
+		__jbd2_journal_file_buffer(jh, transaction, BJ_Forget);
+		spin_unlock(&journal->j_list_lock);
 	}
 
-not_jbd:
 	jbd_unlock_bh_state(bh);
 	__brelse(bh);
 drop:
@@ -1643,6 +1673,11 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh)
 		handle->h_buffer_credits++;
 	}
 	return err;
+
+not_jbd:
+	jbd_unlock_bh_state(bh);
+	__bforget(bh);
+	goto drop;
 }
 
 /**
-- 
2.7.4

Re: [PATCH v3 2/4] jbd2: discard dirty data when forgetting an un-journalled buffer

[Jan Kara]

On Fri 25-01-19 20:30:54, zhangyi (F) wrote:
> We do not unmap and clear dirty flag when forgetting a buffer without
> journal or does not belongs to any transaction, so the invalid dirty
> data may still be written to the disk later. It's fine if the
> corresponding block is never used before the next mount, and it's also
> fine that we invoke clean_bdev_aliases() related functions to unmap
> the block device mapping when re-allocating such freed block as data
> block. But this logic is somewhat fragile and risky that may lead to
> data corruption if we forget to clean bdev aliases. So, It's better to
> discard dirty data during forget time.
> 
> We have been already handled all the cases of forgetting journalled
> buffer, this patch deal with the remaining two cases.
> 
> - buffer is not journalled yet,
> - buffer is journalled but doesn't belongs to any transaction.
> 
> We invoke __bforget() instead of __brelese() when forgetting an
> un-journalled buffer in jbd2_journal_forget(). After this patch we can
> remove all clean_bdev_aliases() related calls in ext4.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Thanks for the patch! Just one small comment below:

> +		/*
> +		 * The buffer is still not written to disk, we should
> +		 * attach this buffer to current transaction to prevent
> +		 * missing writing back when doing checkpoint before
> +		 * the current transaction complete submittion.
> +		 */
> +		__jbd2_journal_temp_unlink_buffer(jh);

Calling __jbd2_journal_temp_unlink_buffer() is not needed when you know the
buffer does not belong to any transaction. Otherwise the patch looks good
to me so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

after fixing this.

								Honza

> +		clear_buffer_dirty(bh);
> +		__jbd2_journal_file_buffer(jh, transaction, BJ_Forget);
> +		spin_unlock(&journal->j_list_lock);
>  	}
>  
> -not_jbd:
>  	jbd_unlock_bh_state(bh);
>  	__brelse(bh);
>  drop:
> @@ -1643,6 +1673,11 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh)
>  		handle->h_buffer_credits++;
>  	}
>  	return err;
> +
> +not_jbd:
> +	jbd_unlock_bh_state(bh);
> +	__bforget(bh);
> +	goto drop;
>  }
>  
>  /**
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 3/4] ext4: cleanup clean_bdev_aliases() calls

[zhangyi (F)]

Now, we have already handle all cases of forgetting buffer in
jbd2_journal_forget(), the buffer should not be mapped to blockdevice
when reallocating it. So this patch remove all clean_bdev_aliases() and
clean_bdev_bh_alias() calls which were invoked by ext4 explicitly.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/ext4/extents.c | 12 +-----------
 fs/ext4/inode.c   |  7 -------
 fs/ext4/page-io.c |  4 +---
 3 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index a054f51..ffb72d8 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4068,18 +4068,8 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 	} else
 		allocated = ret;
 	map->m_flags |= EXT4_MAP_NEW;
-	/*
-	 * if we allocated more blocks than requested
-	 * we need to make sure we unmap the extra block
-	 * allocated. The actual needed block will get
-	 * unmapped later when we find the buffer_head marked
-	 * new.
-	 */
-	if (allocated > map->m_len) {
-		clean_bdev_aliases(inode->i_sb->s_bdev, newblock + map->m_len,
-				   allocated - map->m_len);
+	if (allocated > map->m_len)
 		allocated = map->m_len;
-	}
 	map->m_len = allocated;
 
 map_out:
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e7adf87..3068c83 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -678,8 +678,6 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 		if (flags & EXT4_GET_BLOCKS_ZERO &&
 		    map->m_flags & EXT4_MAP_MAPPED &&
 		    map->m_flags & EXT4_MAP_NEW) {
-			clean_bdev_aliases(inode->i_sb->s_bdev, map->m_pblk,
-					   map->m_len);
 			ret = ext4_issue_zeroout(inode, map->m_lblk,
 						 map->m_pblk, map->m_len);
 			if (ret) {
@@ -1194,7 +1192,6 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
 			if (err)
 				break;
 			if (buffer_new(bh)) {
-				clean_bdev_bh_alias(bh);
 				if (PageUptodate(page)) {
 					clear_buffer_new(bh);
 					set_buffer_uptodate(bh);
@@ -2490,10 +2487,6 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
 	}
 
 	BUG_ON(map->m_len == 0);
-	if (map->m_flags & EXT4_MAP_NEW) {
-		clean_bdev_aliases(inode->i_sb->s_bdev, map->m_pblk,
-				   map->m_len);
-	}
 	return 0;
 }
 
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 2aa62d5..1559946 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -467,10 +467,8 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 				ext4_io_submit(io);
 			continue;
 		}
-		if (buffer_new(bh)) {
+		if (buffer_new(bh))
 			clear_buffer_new(bh);
-			clean_bdev_bh_alias(bh);
-		}
 		set_buffer_async_write(bh);
 		nr_to_submit++;
 	} while ((bh = bh->b_this_page) != head);
-- 
2.7.4

Re: [PATCH v3 3/4] ext4: cleanup clean_bdev_aliases() calls

[Jan Kara]

On Fri 25-01-19 20:30:55, zhangyi (F) wrote:
> Now, we have already handle all cases of forgetting buffer in
> jbd2_journal_forget(), the buffer should not be mapped to blockdevice
> when reallocating it. So this patch remove all clean_bdev_aliases() and
> clean_bdev_bh_alias() calls which were invoked by ext4 explicitly.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c | 12 +-----------
>  fs/ext4/inode.c   |  7 -------
>  fs/ext4/page-io.c |  4 +---
>  3 files changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index a054f51..ffb72d8 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4068,18 +4068,8 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
>  	} else
>  		allocated = ret;
>  	map->m_flags |= EXT4_MAP_NEW;
> -	/*
> -	 * if we allocated more blocks than requested
> -	 * we need to make sure we unmap the extra block
> -	 * allocated. The actual needed block will get
> -	 * unmapped later when we find the buffer_head marked
> -	 * new.
> -	 */
> -	if (allocated > map->m_len) {
> -		clean_bdev_aliases(inode->i_sb->s_bdev, newblock + map->m_len,
> -				   allocated - map->m_len);
> +	if (allocated > map->m_len)
>  		allocated = map->m_len;
> -	}
>  	map->m_len = allocated;
>  
>  map_out:
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index e7adf87..3068c83 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -678,8 +678,6 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  		if (flags & EXT4_GET_BLOCKS_ZERO &&
>  		    map->m_flags & EXT4_MAP_MAPPED &&
>  		    map->m_flags & EXT4_MAP_NEW) {
> -			clean_bdev_aliases(inode->i_sb->s_bdev, map->m_pblk,
> -					   map->m_len);
>  			ret = ext4_issue_zeroout(inode, map->m_lblk,
>  						 map->m_pblk, map->m_len);
>  			if (ret) {
> @@ -1194,7 +1192,6 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
>  			if (err)
>  				break;
>  			if (buffer_new(bh)) {
> -				clean_bdev_bh_alias(bh);
>  				if (PageUptodate(page)) {
>  					clear_buffer_new(bh);
>  					set_buffer_uptodate(bh);
> @@ -2490,10 +2487,6 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd)
>  	}
>  
>  	BUG_ON(map->m_len == 0);
> -	if (map->m_flags & EXT4_MAP_NEW) {
> -		clean_bdev_aliases(inode->i_sb->s_bdev, map->m_pblk,
> -				   map->m_len);
> -	}
>  	return 0;
>  }
>  
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 2aa62d5..1559946 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -467,10 +467,8 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
>  				ext4_io_submit(io);
>  			continue;
>  		}
> -		if (buffer_new(bh)) {
> +		if (buffer_new(bh))
>  			clear_buffer_new(bh);
> -			clean_bdev_bh_alias(bh);
> -		}
>  		set_buffer_async_write(bh);
>  		nr_to_submit++;
>  	} while ((bh = bh->b_this_page) != head);
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 4/4] ext4: convert ext4_split_extent() to return requested length

[zhangyi (F)]

After we remove clean_bdev_aliases() calls which used to unmap extra
blocks in ext4_ext_handle_unwritten_extents(), return extra initialized
region in ext4_ext_convert_to_initialized() is no longer needed, so
in order to simplify logic, this patch convert to return the requested
size instead.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/ext4/extents.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ffb72d8..ffe9671 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3456,9 +3456,8 @@ static int ext4_split_extent(handle_t *handle,
  *    of the logical span [map->m_lblk, map->m_lblk + map->m_len).
  *
  * Post-conditions on success:
- *  - the returned value is the number of blocks beyond map->l_lblk
- *    that are allocated and initialized.
- *    It is guaranteed to be >= map->m_len.
+ *  - The returned value is the minimum number of requested blocks or
+ *    initialized blocks. It is guaranteed to be <= map->m_len.
  */
 static int ext4_ext_convert_to_initialized(handle_t *handle,
 					   struct inode *inode,
@@ -3700,7 +3699,6 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
 
 			split_map.m_len += split_map.m_lblk - ee_block;
 			split_map.m_lblk = ee_block;
-			allocated = map->m_len;
 		}
 	}
 
@@ -3709,6 +3707,9 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
 	if (err > 0)
 		err = 0;
 out:
+	if (allocated > map->m_len)
+		allocated = map->m_len;
+
 	/* If we have gotten a failure, don't zero out status tree */
 	if (!err) {
 		err = ext4_zeroout_es(inode, &zero_ex1);
@@ -4065,11 +4066,10 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 	if (ret <= 0) {
 		err = ret;
 		goto out2;
-	} else
-		allocated = ret;
+	}
+
+	allocated = ret;
 	map->m_flags |= EXT4_MAP_NEW;
-	if (allocated > map->m_len)
-		allocated = map->m_len;
 	map->m_len = allocated;
 
 map_out:
-- 
2.7.4

Re: [PATCH v3 4/4] ext4: convert ext4_split_extent() to return requested length

[Jan Kara]

On Fri 25-01-19 20:30:56, zhangyi (F) wrote:
> After we remove clean_bdev_aliases() calls which used to unmap extra
> blocks in ext4_ext_handle_unwritten_extents(), return extra initialized
> region in ext4_ext_convert_to_initialized() is no longer needed, so
> in order to simplify logic, this patch convert to return the requested
> size instead.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index ffb72d8..ffe9671 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3456,9 +3456,8 @@ static int ext4_split_extent(handle_t *handle,
>   *    of the logical span [map->m_lblk, map->m_lblk + map->m_len).
>   *
>   * Post-conditions on success:
> - *  - the returned value is the number of blocks beyond map->l_lblk
> - *    that are allocated and initialized.
> - *    It is guaranteed to be >= map->m_len.
> + *  - The returned value is the minimum number of requested blocks or
> + *    initialized blocks. It is guaranteed to be <= map->m_len.
>   */
>  static int ext4_ext_convert_to_initialized(handle_t *handle,
>  					   struct inode *inode,
> @@ -3700,7 +3699,6 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  
>  			split_map.m_len += split_map.m_lblk - ee_block;
>  			split_map.m_lblk = ee_block;
> -			allocated = map->m_len;
>  		}
>  	}
>  
> @@ -3709,6 +3707,9 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  	if (err > 0)
>  		err = 0;
>  out:
> +	if (allocated > map->m_len)
> +		allocated = map->m_len;
> +
>  	/* If we have gotten a failure, don't zero out status tree */
>  	if (!err) {
>  		err = ext4_zeroout_es(inode, &zero_ex1);
> @@ -4065,11 +4066,10 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
>  	if (ret <= 0) {
>  		err = ret;
>  		goto out2;
> -	} else
> -		allocated = ret;
> +	}
> +
> +	allocated = ret;
>  	map->m_flags |= EXT4_MAP_NEW;
> -	if (allocated > map->m_len)
> -		allocated = map->m_len;
>  	map->m_len = allocated;
>  
>  map_out:
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR