Re: [PATCH] btrfs: always set an inode's delayed_inode with WRITE_ONCE()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/5/17 00:58, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
>
> Currently we have a couple places using READ_ONCE() to access an inode's
> delayed_inode without taking the lock that protects the xarray for delayed
> inodes, while all the other places access it while holding the lock.
>
> However we never update the delayed_inode pointer of an inode with
> WRITE_ONCE(), making the use of READ_ONCE() pointless since it should
> always be paired with a WRITE_ONCE() in order to protect against issues
> such as write tearing for example.
>
> So change all the places that update struct btrfs_inode::delayed_inode to
> use WRITE_ONCE() instead of simple assignments.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

A quick search shows that we still have one call site not using READ_ONCE():

Inside btrfs_dirty_inode() of inode.c we have

	btrfs_end_transaction(trans);
	if (inode->delayed_inode)
		btrfs_balance_delayed_items(fs_info);

I guess we should also use READ_ONCE() for it?

Thanks,
Qu

> ---
>   fs/btrfs/delayed-inode.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 483c141dc488..6df7e44d9d31 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -106,7 +106,7 @@ static struct btrfs_delayed_node *btrfs_get_delayed_node(
>   		 */
>   		if (refcount_inc_not_zero(&node->refs)) {
>   			refcount_inc(&node->refs);
> -			btrfs_inode->delayed_node = node;
> +			WRITE_ONCE(btrfs_inode->delayed_node, node);
>   		} else {
>   			node = NULL;
>   		}
> @@ -161,7 +161,7 @@ static struct btrfs_delayed_node *btrfs_get_or_create_delayed_node(
>   	ASSERT(xa_err(ptr) != -EINVAL);
>   	ASSERT(xa_err(ptr) != -ENOMEM);
>   	ASSERT(ptr == NULL);
> -	btrfs_inode->delayed_node = node;
> +	WRITE_ONCE(btrfs_inode->delayed_node, node);
>   	xa_unlock(&root->delayed_nodes);
>
>   	return node;
> @@ -1312,7 +1312,7 @@ void btrfs_remove_delayed_node(struct btrfs_inode *inode)
>   	if (!delayed_node)
>   		return;
>
> -	inode->delayed_node = NULL;
> +	WRITE_ONCE(inode->delayed_node, NULL);
>   	btrfs_release_delayed_node(delayed_node);
>   }
>