Skip to contentSkip to conversation

Patch "Btrfs: fix fsync data loss after append write" has been added to the 4.1-stable tree

PATCH [email protected] 1 participant 1 message Jul 31, 2015

Subsystems (2)
lore
Share & tools
lore

[email protected]

author maintainer Applied
#0 lore raw

This is a note to let you know that I've just added the patch titled

Btrfs: fix fsync data loss after append write

to the 4.1-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
btrfs-fix-fsync-data-loss-after-append-write.patch
and it can be found in the queue-4.1 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From e4545de5b035c7debb73d260c78377dbb69cbfb5 Mon Sep 17 00:00:00 2001

From: Filipe Manana <[email protected]>
Date: Wed, 17 Jun 2015 12:49:23 +0100
Subject: Btrfs: fix fsync data loss after append write

From: Filipe Manana <[email protected]>

commit e4545de5b035c7debb73d260c78377dbb69cbfb5 upstream.

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).

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

---
 fs/btrfs/tree-log.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
fs/btrfs/tree-log.c +9 -5
@@ -4161,6 +4161,7 @@ static int btrfs_log_inode(struct btrfs_
 	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_
 		} 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

Patches currently in stable-queue which might be from [email protected] are

queue-4.1/btrfs-fix-file-corruption-after-cloning-inline-extents.patch queue-4.1/btrfs-fix-race-between-caching-kthread-and-returning-inode-to-inode-cache.patch queue-4.1/btrfs-fix-list-transaction-pending_ordered-corruption.patch queue-4.1/btrfs-fix-memory-leak-in-the-extent_same-ioctl.patch queue-4.1/btrfs-use-kmem_cache_free-when-freeing-entry-in-inode-cache.patch queue-4.1/btrfs-fix-fsync-data-loss-after-append-write.patch