Re: [PATCH] Btrfs: fix fsync data loss after append write

[Liu Bo <bo.li.liu@oracle.com>]

On Wed, Jun 17, 2015 at 12:49:23PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we do an append write to a file (which increases its inode's i_size)
> that does not have the flag BTRFS_INODE_NEEDS_FULL_SYNC set in its inode,
> and the previous transaction added a new hard link to the file, which sets
> the flag BTRFS_INODE_COPY_EVERYTHING in the file's inode, and then fsync
> the file, the inode's new i_size isn't logged. This has the consequence
> that after the fsync log is replayed, the file size remains what it was
> before the append write operation, which means users/applications will
> not be able to read the data that was successsfully fsync'ed before.
> 
> This happens because neither the inode item nor the delayed inode get
> their i_size updated when the append write is made - doing so would
> require starting a transaction in the buffered write path, something that
> we do not do intentionally for performance reasons.
> 
> Fix this by making sure that when the flag BTRFS_INODE_COPY_EVERYTHING is
> set the inode is logged with its current i_size (log the in-memory inode
> into the log tree).

Looks good to me.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo
> 
> This issue is not a recent regression and is easy to reproduce with the
> following test case for fstests:
> 
>   seq=`basename $0`
>   seqres=$RESULT_DIR/$seq
>   echo "QA output created by $seq"
> 
>   here=`pwd`
>   tmp=/tmp/$$
>   status=1	# failure is the default!
> 
>   _cleanup()
>   {
>           _cleanup_flakey
>           rm -f $tmp.*
>   }
>   trap "_cleanup; exit \$status" 0 1 2 3 15
> 
>   # get standard environment, filters and checks
>   . ./common/rc
>   . ./common/filter
>   . ./common/dmflakey
> 
>   # real QA test starts here
>   _supported_fs generic
>   _supported_os Linux
>   _need_to_be_root
>   _require_scratch
>   _require_dm_flakey
>   _require_metadata_journaling $SCRATCH_DEV
> 
>   _crash_and_mount()
>   {
>           # Simulate a crash/power loss.
>           _load_flakey_table $FLAKEY_DROP_WRITES
>           _unmount_flakey
>           # Allow writes again and mount. This makes the fs replay its fsync log.
>           _load_flakey_table $FLAKEY_ALLOW_WRITES
>           _mount_flakey
>   }
> 
>   rm -f $seqres.full
> 
>   _scratch_mkfs >> $seqres.full 2>&1
>   _init_flakey
>   _mount_flakey
> 
>   # Create the test file with some initial data and then fsync it.
>   # The fsync here is only needed to trigger the issue in btrfs, as it causes the
>   # the flag BTRFS_INODE_NEEDS_FULL_SYNC to be removed from the btrfs inode.
>   $XFS_IO_PROG -f -c "pwrite -S 0xaa 0 32k" \
>                   -c "fsync" \
>                   $SCRATCH_MNT/foo | _filter_xfs_io
>   sync
> 
>   # Add a hard link to our file.
>   # On btrfs this sets the flag BTRFS_INODE_COPY_EVERYTHING on the btrfs inode,
>   # which is a necessary condition to trigger the issue.
>   ln $SCRATCH_MNT/foo $SCRATCH_MNT/bar
> 
>   # Sync the filesystem to force a commit of the current btrfs transaction, this
>   # is a necessary condition to trigger the bug on btrfs.
>   sync
> 
>   # Now append more data to our file, increasing its size, and fsync the file.
>   # In btrfs because the inode flag BTRFS_INODE_COPY_EVERYTHING was set and the
>   # write path did not update the inode item in the btree nor the delayed inode
>   # item (in memory struture) in the current transaction (created by the fsync
>   # handler), the fsync did not record the inode's new i_size in the fsync
>   # log/journal. This made the data unavailable after the fsync log/journal is
>   # replayed.
>   $XFS_IO_PROG -c "pwrite -S 0xbb 32K 32K" \
>                -c "fsync" \
>                $SCRATCH_MNT/foo | _filter_xfs_io
> 
>   echo "File content after fsync and before crash:"
>   od -t x1 $SCRATCH_MNT/foo
> 
>   _crash_and_mount
> 
>   echo "File content after crash and log replay:"
>   od -t x1 $SCRATCH_MNT/foo
> 
>   status=0
>   exit
> 
> The expected file output before and after the crash/power failure expects the
> appended data to be available, which is:
> 
>   0000000 aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa
>   *
>   0100000 bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb
>   *
>   0200000
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/tree-log.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index d049683..4920fce 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -4161,6 +4161,7 @@ static int btrfs_log_inode(struct btrfs_trans_handle *trans,
>  	u64 ino = btrfs_ino(inode);
>  	struct extent_map_tree *em_tree = &BTRFS_I(inode)->extent_tree;
>  	u64 logged_isize = 0;
> +	bool need_log_inode_item = true;
>  
>  	path = btrfs_alloc_path();
>  	if (!path)
> @@ -4269,11 +4270,6 @@ static int btrfs_log_inode(struct btrfs_trans_handle *trans,
>  		} else {
>  			if (inode_only == LOG_INODE_ALL)
>  				fast_search = true;
> -			ret = log_inode_item(trans, log, dst_path, inode);
> -			if (ret) {
> -				err = ret;
> -				goto out_unlock;
> -			}
>  			goto log_extents;
>  		}
>  
> @@ -4296,6 +4292,9 @@ again:
>  		if (min_key.type > max_key.type)
>  			break;
>  
> +		if (min_key.type == BTRFS_INODE_ITEM_KEY)
> +			need_log_inode_item = false;
> +
>  		src = path->nodes[0];
>  		if (ins_nr && ins_start_slot + ins_nr == path->slots[0]) {
>  			ins_nr++;
> @@ -4366,6 +4365,11 @@ next_slot:
>  log_extents:
>  	btrfs_release_path(path);
>  	btrfs_release_path(dst_path);
> +	if (need_log_inode_item) {
> +		err = log_inode_item(trans, log, dst_path, inode);
> +		if (err)
> +			goto out_unlock;
> +	}
>  	if (fast_search) {
>  		/*
>  		 * Some ordered extents started by fsync might have completed
> -- 
> 2.1.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html