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

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

[fdmanana]

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>
---
 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);
 }
 
-- 
2.43.0

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

[Qu Wenruo]

在 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);
>   }
>

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

[Filipe Manana]

On Fri, May 17, 2024 at 12:17 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 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?

Yep, however it may be harmless in that case I think, but still things
like KCSAN are likely to detect and report a data race there.
Just sent a patchset to cover that as well:
https://lore.kernel.org/linux-btrfs/cover.1715951291.git.fdmanana@suse.com/

Thanks.

>
> 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);
> >   }
> >