Re: [PATCH 0/3] btrfs: avoid data races when accessing an inode's delayed_node

[David Sterba <dsterba@suse.cz>]

On Fri, May 17, 2024 at 02:13:23PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We do have some data races when accessing an inode's delayed_node, namely
> we use READ_ONCE() in a couple places while there's no pairing WRITE_ONCE()
> anywhere, and in one place (btrfs_dirty_inode()) we neither user READ_ONCE()
> nor take the lock that protects the delayed_node. So fix these and add
> helpers to access and update an inode's delayed_node.
> 
> Filipe Manana (3):
>   btrfs: always set an inode's delayed_inode with WRITE_ONCE()
>   btrfs: use READ_ONCE() when accessing delayed_node at btrfs_dirty_node()
>   btrfs: add and use helpers to get and set an inode's delayed_node

The READ_ONCE for delayed nodes has been there historically but I don't
think it's needed everywhere. The legitimate case is in
btrfs_get_delayed_node() where first use is without lock and then again
recheck under the lock so we do want to read fresh value. This is to
prevent compiler optimization to coalesce the reads.

Writing to delayed node under lock also does not need WRITE_ONCE.

IOW, I would rather remove use of the _ONCE helpers and not add more as
it is not the pattern where it's supposed to be used. You say it's to
prevent load tearing but for a pointer type this does not happen and is
an assumption of the hardware.