[v2] Btrfs: fix mount failure after fsync due to hard link recreation

Commit Message

Filipe Manana July 20, 2018, 9:59 a.m. UTC

From: Filipe Manana <[e-mail]>

If we end up with logging an inode reference item which has the same name
but different index from the one we have persisted, we end up failing when
replaying the log with an errno value of -EEXIST. The error comes from
btrfs_add_link(), which is called from add_inode_ref(), when we are
replaying an inode reference item.

Example scenario where this happens:

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

  $ touch /mnt/foo
  $ ln /mnt/foo /mnt/bar

  $ sync

  # Rename the first hard link (foo) to a new name and rename the second
  # hard link (bar) to the old name of the first hard link (foo).
  $ mv /mnt/foo /mnt/qwerty
  $ mv /mnt/bar /mnt/foo

  # Create a new file, in the same parent directory, with the old name of
  # the second hard link (bar) and fsync this new file.
  # We do this instead of calling fsync on foo/qwerty because if we did
  # that the fsync resulted in a full transaction commit, not triggering
  # the problem.
  $ touch /mnt/bar
  $ xfs_io -c "fsync" /mnt/bar

  <power fail>

  $ mount /dev/sdb /mnt
  mount: mount /dev/sdb on /mnt failed: File exists

So fix this by checking if a conflicting inode reference exists (same
name, same parent but different index), removing it (and the associated
dir index entries from the parent inode) if it exists, before attempting
to add the new reference.

A test case for fstests follows soon.

Signed-off-by: Filipe Manana <[e-mail]>
---

V2: added missing error check from btrfs_unlink_inode()

 fs/btrfs/tree-log.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

Comments

David Sterba Aug. 2, 2018, 3:06 p.m. UTC | #1

On Fri, Jul 20, 2018 at 10:59:06AM +0100, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
> 
> If we end up with logging an inode reference item which has the same name
> but different index from the one we have persisted, we end up failing when
> replaying the log with an errno value of -EEXIST. The error comes from
> btrfs_add_link(), which is called from add_inode_ref(), when we are
> replaying an inode reference item.
> 
> Example scenario where this happens:
> 
>   $ mkfs.btrfs -f /dev/sdb
>   $ mount /dev/sdb /mnt
> 
>   $ touch /mnt/foo
>   $ ln /mnt/foo /mnt/bar
> 
>   $ sync
> 
>   # Rename the first hard link (foo) to a new name and rename the second
>   # hard link (bar) to the old name of the first hard link (foo).
>   $ mv /mnt/foo /mnt/qwerty
>   $ mv /mnt/bar /mnt/foo
> 
>   # Create a new file, in the same parent directory, with the old name of
>   # the second hard link (bar) and fsync this new file.
>   # We do this instead of calling fsync on foo/qwerty because if we did
>   # that the fsync resulted in a full transaction commit, not triggering
>   # the problem.
>   $ touch /mnt/bar
>   $ xfs_io -c "fsync" /mnt/bar
> 
>   <power fail>
> 
>   $ mount /dev/sdb /mnt
>   mount: mount /dev/sdb on /mnt failed: File exists
> 
> So fix this by checking if a conflicting inode reference exists (same
> name, same parent but different index), removing it (and the associated
> dir index entries from the parent inode) if it exists, before attempting
> to add the new reference.
> 
> A test case for fstests follows soon.
> 
> Signed-off-by: Filipe Manana <[e-mail]>

The patch has been in misc-next for more than a weak but I probably did
not let you know, so here goes. Thanks.
--
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 f8220ec02036..5f3c33466c18 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1291,6 +1291,49 @@  static int unlink_old_inode_refs(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+static int btrfs_inode_ref_exists(struct inode *inode,
+				  struct inode *dir,
+				  const u8 ref_type,
+				  const char *name,
+				  const int namelen)
+{
+	struct btrfs_key key;
+	struct btrfs_path *path;
+	const u64 parent_id = btrfs_ino(BTRFS_I(dir));
+	int ret;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	key.objectid = btrfs_ino(BTRFS_I(inode));
+	key.type = ref_type;
+	if (key.type == BTRFS_INODE_REF_KEY)
+		key.offset = parent_id;
+	else
+		key.offset = btrfs_extref_hash(parent_id, name, namelen);
+
+	ret = btrfs_search_slot(NULL, BTRFS_I(inode)->root, &key, path, 0, 0);
+	if (ret < 0)
+		goto out;
+	if (ret > 0) {
+		ret = 0;
+		goto out;
+	}
+	if (key.type == BTRFS_INODE_EXTREF_KEY)
+		ret = btrfs_find_name_in_ext_backref(path->nodes[0],
+						     path->slots[0],
+						     parent_id, name,
+						     namelen, NULL);
+	else
+		ret = btrfs_find_name_in_backref(path->nodes[0], path->slots[0],
+						 name, namelen, NULL);
+
+out:
+	btrfs_free_path(path);
+	return ret;
+}
+
 /*
  * replay one inode back reference item found in the log tree.
  * eb, slot and key refer to the buffer and key found in the log tree.
@@ -1400,6 +1443,32 @@  static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
 				}
 			}
 
+			/*
+			 * If a reference item already exists for this inode
+			 * with the same parent and name, but different index,
+			 * drop it and the corresponding directory index entries
+			 * from the parent before adding the new reference item
+			 * and dir index entries, otherwise we would fail with
+			 * -EEXIST returned from btrfs_add_link() below.
+			 */
+			ret = btrfs_inode_ref_exists(inode, dir, key->type,
+						     name, namelen);
+			if (ret > 0) {
+				ret = btrfs_unlink_inode(trans, root,
+							 BTRFS_I(dir),
+							 BTRFS_I(inode),
+							 name, namelen);
+				/*
+				 * If we dropped the link count to 0, bump it so
+				 * that later the iput() on the inode will not
+				 * free it. We will fixup the link count later.
+				 */
+				if (!ret && inode->i_nlink == 0)
+					inc_nlink(inode);
+			}
+			if (ret < 0)
+				goto out;
+
 			/* insert our name */
 			ret = btrfs_add_link(trans, BTRFS_I(dir),
 					BTRFS_I(inode),