Re: [PATCH] Btrfs: fix error handling in btrfs_truncate()

[Omar Sandoval <osandov@osandov.com>]

On Fri, May 18, 2018 at 02:43:02PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Jun Wu at Facebook reported that an internal service was seeing a return
> value of 1 from ftruncate() on Btrfs when compression is enabled. This
> is coming from the NEED_TRUNCATE_BLOCK return value from
> btrfs_truncate_inode_items().
> 
> btrfs_truncate() uses two variables for error handling, ret and err (if
> this sounds familiar, it's because btrfs_truncate_inode_items() does
> something similar). When btrfs_truncate_inode_items() returns non-zero,
> we set err to the return value, but we don't reset it to zero in the
> successful NEED_TRUNCATE_BLOCK case. We only have err because we don't
> want to mask an error if we call btrfs_update_inode() and
> btrfs_end_transaction(), so let's make that its own scoped return
> variable and use ret everywhere else.

To expand on this, this is bad because userspace that checks for a
non-zero return value will think the truncate failed even though it
succeeded, and we also end up not creating an inotify event for the
truncate.

> Fixes: ddfae63cc8e0 ("btrfs: move btrfs_truncate_block out of trans handle")
> Reported-by: Jun Wu <quark@fb.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> This is based on Linus' master rather than my orphan ENOSPC fixes
> because I think we want to get this in for v4.17 and stable, and rebase
> my fixes on top of this.
> 
>  fs/btrfs/inode.c | 34 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d241285a0d2a..d4a47ae36ed8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -9031,8 +9031,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>  	struct btrfs_block_rsv *rsv;
> -	int ret = 0;
> -	int err = 0;
> +	int ret;
>  	struct btrfs_trans_handle *trans;
>  	u64 mask = fs_info->sectorsize - 1;
>  	u64 min_size = btrfs_calc_trunc_metadata_size(fs_info, 1);
> @@ -9092,7 +9091,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
>  	 */
>  	trans = btrfs_start_transaction(root, 2);
>  	if (IS_ERR(trans)) {
> -		err = PTR_ERR(trans);
> +		ret = PTR_ERR(trans);
>  		goto out;
>  	}
>  
> @@ -9116,23 +9115,19 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
>  						 inode->i_size,
>  						 BTRFS_EXTENT_DATA_KEY);
>  		trans->block_rsv = &fs_info->trans_block_rsv;
> -		if (ret != -ENOSPC && ret != -EAGAIN) {
> -			err = ret;
> +		if (ret != -ENOSPC && ret != -EAGAIN)
>  			break;
> -		}
>  
>  		ret = btrfs_update_inode(trans, root, inode);
> -		if (ret) {
> -			err = ret;
> +		if (ret)
>  			break;
> -		}
>  
>  		btrfs_end_transaction(trans);
>  		btrfs_btree_balance_dirty(fs_info);
>  
>  		trans = btrfs_start_transaction(root, 2);
>  		if (IS_ERR(trans)) {
> -			ret = err = PTR_ERR(trans);
> +			ret = PTR_ERR(trans);
>  			trans = NULL;
>  			break;
>  		}
> @@ -9168,26 +9163,25 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
>  	if (ret == 0 && inode->i_nlink > 0) {
>  		trans->block_rsv = root->orphan_block_rsv;
>  		ret = btrfs_orphan_del(trans, BTRFS_I(inode));
> -		if (ret)
> -			err = ret;
>  	}
>  
>  	if (trans) {
> +		int ret2;
> +
>  		trans->block_rsv = &fs_info->trans_block_rsv;
> -		ret = btrfs_update_inode(trans, root, inode);
> -		if (ret && !err)
> -			err = ret;
> +		ret2 = btrfs_update_inode(trans, root, inode);
> +		if (ret2 && !ret)
> +			ret = ret2;
>  
> -		ret = btrfs_end_transaction(trans);
> +		ret2 = btrfs_end_transaction(trans);
> +		if (ret2 && !ret)
> +			ret = ret2;
>  		btrfs_btree_balance_dirty(fs_info);
>  	}
>  out:
>  	btrfs_free_block_rsv(fs_info, rsv);
>  
> -	if (ret && !err)
> -		err = ret;
> -
> -	return err;
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.17.0
>