| Message ID | a4dfaf8ca94754560f4d9196b04ba763256124ce.1715873248.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: always set an inode's delayed_inode with WRITE_ONCE() | expand |
在 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); > } >
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); > > } > >
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); }