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.

The same type of race happens in case our inode is a directory when we
update the inode's ->last_dir_index_offset field at inode_logged() to
(u64)-1, and in that case such a race could make directory logging skip
logging new entries at process_dir_items_leaf(), since any new dir entry
has an index less than (u64).

Fix this by ensuring inode_logged() is always called while holding the
inode's log_mutex.

Fixes: 0f8ce49 ("btrfs: avoid inode logging during rename and link when possible")
Reviewed-by: Boris Burkov <[email protected]>
Signed-off-by: Filipe Manana <[email protected]>
Signed-off-by: David Sterba <[email protected]>

Author: fdmanana

fs/btrfs/tree-log.c

@@ -3485,14 +3485,27 @@ int btrfs_free_log_root_tree(struct btrfs_trans_handle *trans,
  * Returns 1 if the inode was logged before in the transaction, 0 if it was not,
  * and < 0 on error.
  */
-static int inode_logged(const struct btrfs_trans_handle *trans,
-			struct btrfs_inode *inode,
-			struct btrfs_path *path_in)
+static int inode_logged_locked(const struct btrfs_trans_handle *trans,
+			       struct btrfs_inode *inode,
+			       struct btrfs_path *path_in)
 {
 	struct btrfs_path *path = path_in;
 	struct btrfs_key key;
 	int ret;
 
+	/*
+	 * The log_mutex must be taken to prevent races with concurrent logging
+	 * as we may see the inode not logged when we are called but it gets
+	 * logged right after we did not find it in the log tree and we end up
+	 * setting inode->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.
+	 * The same type of race also happens if out inode is a directory when
+	 * we update inode->last_dir_index_offset below.
+	 */
+	lockdep_assert_held(&inode->log_mutex);
+
 	if (inode->logged_trans == trans->transid)
 		return 1;
 
@@ -3594,6 +3607,19 @@ static int inode_logged(const struct btrfs_trans_handle *trans,
 	return 1;
 }
 
+static inline int inode_logged(const struct btrfs_trans_handle *trans,
+			       struct btrfs_inode *inode,
+			       struct btrfs_path *path)
+{
+	int ret;
+
+	mutex_lock(&inode->log_mutex);
+	ret = inode_logged_locked(trans, inode, path);
+	mutex_unlock(&inode->log_mutex);
+
+	return ret;
+}
+
 /*
  * Delete a directory entry from the log if it exists.
  *
@@ -6678,7 +6704,7 @@ static int btrfs_log_inode(struct btrfs_trans_handle *trans,
 	 * inode_logged(), because after that we have the need to figure out if
 	 * the inode was previously logged in this transaction.
 	 */
-	ret = inode_logged(trans, inode, path);
+	ret = inode_logged_locked(trans, inode, path);
 	if (ret < 0)
 		goto out_unlock;
 	ctx->logged_before = (ret == 1);