Commit ef07b74e authored by Filipe Manana's avatar Filipe Manana Committed by David Sterba
Browse files

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: 0f8ce498 ("btrfs: avoid inode logging during rename and link when possible")
Reviewed-by: default avatarBoris Burkov <boris@bur.io>
Signed-off-by: default avatarFilipe Manana <fdmanana@suse.com>
Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
parent 5bb00879

fs/btrfs/tree-log.c

+30 −6
Original line number Diff line number Diff line
@@ -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