Btrfs: fix xattr loss after power failure

Commit Message

Filipe Manana May 11, 2018, 3:42 p.m. UTC

From: Filipe Manana <[e-mail]>

If a file has xattrs, we fsync it, to ensure we clear the flags
BTRFS_INODE_NEEDS_FULL_SYNC and BTRFS_INODE_COPY_EVERYTHING from its
inode, the current transaction commits and then we fsync it (without
either of those bits being set in its inode), we end up not logging
all its xattrs. This results in deleting all xattrs when replying the
log after a power failure.

Trivial reproducer

  $ mkfs.btrfs -f /dev/sdb
  $ mount /dev/sdb /mnt

  $ touch /mnt/foobar
  $ setfattr -n user.xa -v qwerty /mnt/foobar
  $ xfs_io -c "fsync" /mnt/foobar

  $ sync

  $ xfs_io -c "pwrite -S 0xab 0 64K" /mnt/foobar
  $ xfs_io -c "fsync" /mnt/foobar
  <power failure>

  $ mount /dev/sdb /mnt
  $ getfattr --absolute-names --dump /mnt/foobar
  <empty output>
  $

So fix this by making sure all xattrs are logged if we log a file's inode
item and neither the flags BTRFS_INODE_NEEDS_FULL_SYNC nor
BTRFS_INODE_COPY_EVERYTHING were set in the inode.

Fixes: 36283bf777d9 ("Btrfs: fix fsync xattr loss in the fast fsync path")
Cc: <[e-mail]>
Signed-off-by: Filipe Manana <[e-mail]>
---
 fs/btrfs/tree-log.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

David Sterba May 14, 2018, 1:51 p.m. UTC | #1

On Fri, May 11, 2018 at 04:42:42PM +0100, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
> 
> If a file has xattrs, we fsync it, to ensure we clear the flags
> BTRFS_INODE_NEEDS_FULL_SYNC and BTRFS_INODE_COPY_EVERYTHING from its
> inode, the current transaction commits and then we fsync it (without
> either of those bits being set in its inode), we end up not logging
> all its xattrs. This results in deleting all xattrs when replying the
> log after a power failure.
> 
> Trivial reproducer
> 
>   $ mkfs.btrfs -f /dev/sdb
>   $ mount /dev/sdb /mnt
> 
>   $ touch /mnt/foobar
>   $ setfattr -n user.xa -v qwerty /mnt/foobar
>   $ xfs_io -c "fsync" /mnt/foobar
> 
>   $ sync
> 
>   $ xfs_io -c "pwrite -S 0xab 0 64K" /mnt/foobar
>   $ xfs_io -c "fsync" /mnt/foobar
>   <power failure>
> 
>   $ mount /dev/sdb /mnt
>   $ getfattr --absolute-names --dump /mnt/foobar
>   <empty output>
>   $
> 
> So fix this by making sure all xattrs are logged if we log a file's inode
> item and neither the flags BTRFS_INODE_NEEDS_FULL_SYNC nor
> BTRFS_INODE_COPY_EVERYTHING were set in the inode.
> 
> Fixes: 36283bf777d9 ("Btrfs: fix fsync xattr loss in the fast fsync path")
> Cc: <[e-mail]>
> Signed-off-by: Filipe Manana <[e-mail]>

Added to next, thanks, and I'll probably add that to 4.17-rc too.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 25b888df00c9..d656de8bec52 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4916,6 +4916,7 @@  static int btrfs_log_inode(struct btrfs_trans_handle *trans,
 	struct extent_map_tree *em_tree = &inode->extent_tree;
 	u64 logged_isize = 0;
 	bool need_log_inode_item = true;
+	bool xattrs_logged = false;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -5217,6 +5218,7 @@  static int btrfs_log_inode(struct btrfs_trans_handle *trans,
 	err = btrfs_log_all_xattrs(trans, root, inode, path, dst_path);
 	if (err)
 		goto out_unlock;
+	xattrs_logged = true;
 	if (max_key.type >= BTRFS_EXTENT_DATA_KEY && !fast_search) {
 		btrfs_release_path(path);
 		btrfs_release_path(dst_path);
@@ -5229,6 +5231,11 @@  static int btrfs_log_inode(struct btrfs_trans_handle *trans,
 	btrfs_release_path(dst_path);
 	if (need_log_inode_item) {
 		err = log_inode_item(trans, log, dst_path, inode);
+		if (!err && !xattrs_logged) {
+			err = btrfs_log_all_xattrs(trans, root, inode, path,
+						   dst_path);
+			btrfs_release_path(path);
+		}
 		if (err)
 			goto out_unlock;
 	}