btrfs: fix race between logging inode and checking if it was logged before

There's a race between checking if an inode was logged before and logging
an inode that can cause us to mark an inode as not logged just after it
was logged by a concurrent task:

1) We have inode X which was not logged before neither in the current
   transaction not in past transaction since the inode was loaded into
   memory, so it's ->logged_trans value is 0;

2) We are at transaction N;

3) Task A calls inode_logged() against inode X, sees that ->logged_trans
   is 0 and there is a log tree and so it proceeds to search in the log
   tree for an inode item for inode X. It doesn't see any, but before
   it sets ->logged_trans to N - 1...

3) Task B calls btrfs_log_inode() against inode X, logs the inode and
   sets ->logged_trans to N;

4) Task A now sets ->logged_trans to N - 1;

5) At this point anyone calling inode_logged() gets 0 (inode not logged)
   since ->logged_trans is greater than 0 and less than N, but our inode
   was really logged. As a consequence operations like rename, unlink and
   link that happen afterwards in the current transaction end up not
   updating the log when they should.

Fix this by ensuring inode_logged() only updates ->logged_trans in case
the inode item is not found in the log tree if after tacking the inode's
lock (spinlock struct btrfs_inode::lock) the ->logged_trans value is still
zero, since the inode lock is what protects setting ->logged_trans at
btrfs_log_inode().

Fixes: 0f8ce49821de ("btrfs: avoid inode logging during rename and link when possible")
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 69e11557fd13d80c5be7dafe3376a98d1301aa1b..839dad70129005e3ee6545585820f13cfb930d58 100644 (file)
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3340,6 +3340,31 @@ int btrfs_free_log_root_tree(struct btrfs_trans_handle *trans,
        return 0;
 }
 
+static bool mark_inode_as_not_logged(const struct btrfs_trans_handle *trans,
+                                    struct btrfs_inode *inode)
+{
+       bool ret = false;
+
+       /*
+        * Do this only if ->logged_trans is still 0 to prevent races with
+        * concurrent logging as we may see the inode not logged when
+        * inode_logged() is called but it gets logged after inode_logged() did
+        * not find it in the log tree and we end up setting ->logged_trans to a
+        * value less than trans->transid after the concurrent logging task has
+        * set it to trans->transid. As a consequence, subsequent rename, unlink
+        * and link operations may end up not logging new names and removing old
+        * names from the log.
+        */
+       spin_lock(&inode->lock);
+       if (inode->logged_trans == 0)
+               inode->logged_trans = trans->transid - 1;
+       else if (inode->logged_trans == trans->transid)
+               ret = true;
+       spin_unlock(&inode->lock);
+
+       return ret;
+}
+
 /*
  * Check if an inode was logged in the current transaction. This correctly deals
  * with the case where the inode was logged but has a logged_trans of 0, which
@@ -3374,10 +3399,8 @@ static int inode_logged(const struct btrfs_trans_handle *trans,
         * transaction's ID, to avoid the search below in a future call in case
         * a log tree gets created after this.
         */
-       if (!test_bit(BTRFS_ROOT_HAS_LOG_TREE, &inode->root->state)) {
-               inode->logged_trans = trans->transid - 1;
-               return 0;
-       }
+       if (!test_bit(BTRFS_ROOT_HAS_LOG_TREE, &inode->root->state))
+               return mark_inode_as_not_logged(trans, inode);
 
        /*
         * We have a log tree and the inode's logged_trans is 0. We can't tell
@@ -3431,8 +3454,7 @@ static int inode_logged(const struct btrfs_trans_handle *trans,
                 * Set logged_trans to a value greater than 0 and less then the
                 * current transaction to avoid doing the search in future calls.
                 */
-               inode->logged_trans = trans->transid - 1;
-               return 0;
+               return mark_inode_as_not_logged(trans, inode);
        }
 
        /*
@@ -3440,7 +3462,9 @@ static int inode_logged(const struct btrfs_trans_handle *trans,
         * the current transacion's ID, to avoid future tree searches as long as
         * the inode is not evicted again.
         */
+       spin_lock(&inode->lock);
        inode->logged_trans = trans->transid;
+       spin_unlock(&inode->lock);
 
        /*
         * If it's a directory, then we must set last_dir_index_offset to the