btrfs: avoid load/store tearing races when checking if an inode was logged

At inode_logged() we do a couple lockless checks for ->logged_trans, and
these are generally safe except the second one in case we get a load or
store tearing due to a concurrent call updating ->logged_trans (either at
btrfs_log_inode() or later at inode_logged()).

In the first case it's safe to compare to the current transaction ID since
once ->logged_trans is set the current transaction, we never set it to a
lower value.

In the second case, where we check if it's greater than zero, we are prone
to load/store tearing races, since we can have a concurrent task updating
to the current transaction ID with store tearing for example, instead of
updating with a single 64 bits write, to update with two 32 bits writes or
four 16 bits writes. In that case the reading side at inode_logged() could
see a positive value that does not match the current transaction and then
return a false negative.

Fix this by doing the second check while holding the inode's spinlock, add
some comments about it too. Also add the data_race() annotation to the
first check to avoid any reports from KCSAN (or similar tools) and comment
about it.

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 2a3d9e996e729b9503d68174bf677df582ef5df2..7d5d90845ca985c940730d76fa7d718915e1216d 100644 (file)
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3382,15 +3382,32 @@ static int inode_logged(const struct btrfs_trans_handle *trans,
        struct btrfs_key key;
        int ret;
 
-       if (inode->logged_trans == trans->transid)
+       /*
+        * Quick lockless call, since once ->logged_trans is set to the current
+        * transaction, we never set it to a lower value anywhere else.
+        */
+       if (data_race(inode->logged_trans) == trans->transid)
                return 1;
 
        /*
-        * If logged_trans is not 0, then we know the inode logged was not logged
-        * in this transaction, so we can return false right away.
+        * If logged_trans is not 0 and not trans->transid, then we know the
+        * inode was not logged in this transaction, so we can return false
+        * right away. We take the lock to avoid a race caused by load/store
+        * tearing with a concurrent btrfs_log_inode() call or a concurrent task
+        * in this function further below - an update to trans->transid can be
+        * teared into two 32 bits updates for example, in which case we could
+        * see a positive value that is not trans->transid and assume the inode
+        * was not logged when it was.
         */
-       if (inode->logged_trans > 0)
+       spin_lock(&inode->lock);
+       if (inode->logged_trans == trans->transid) {
+               spin_unlock(&inode->lock);
+               return 1;
+       } else if (inode->logged_trans > 0) {
+               spin_unlock(&inode->lock);
                return 0;
+       }
+       spin_unlock(&inode->lock);
 
        /*
         * If no log tree was created for this root in this transaction, then