[PATCH] fs: Fix data race in btrfs_drop_extents

[PATCH] fs: Fix data race in btrfs_drop_extents

[Hao-ran Zheng]

A data race occurs when the function `insert_ordered_extent_file_extent()`
and the function `btrfs_inode_safe_disk_i_size_write()` are executed
concurrently. The function `insert_ordered_extent_file_extent()` is not
locked when reading inode->disk_i_size, causing
`btrfs_inode_safe_disk_i_size_write()`to cause data competition when
writing inode->disk_i_size, thus affecting the value of `modify_tree`,
leading to some unexpected results such as disk data being overwritten.
The specific call stack that appears during testing is as follows:

============DATA_RACE============
 btrfs_drop_extents+0x89a/0xa060 [btrfs]
 insert_reserved_file_extent+0xb54/0x2960 [btrfs]
 insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
 btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
 btrfs_finish_ordered_io+0x37/0x60 [btrfs]
 finish_ordered_fn+0x3e/0x50 [btrfs]
 btrfs_work_helper+0x9c9/0x27a0 [btrfs]
 process_scheduled_works+0x716/0xf10
 worker_thread+0xb6a/0x1190
 kthread+0x292/0x330
 ret_from_fork+0x4d/0x80
 ret_from_fork_asm+0x1a/0x30
============OTHER_INFO============
 btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
 btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
 btrfs_finish_ordered_io+0x37/0x60 [btrfs]
 finish_ordered_fn+0x3e/0x50 [btrfs]
 btrfs_work_helper+0x9c9/0x27a0 [btrfs]
 process_scheduled_works+0x716/0xf10
 worker_thread+0xb6a/0x1190
 kthread+0x292/0x330
 ret_from_fork+0x4d/0x80
 ret_from_fork_asm+0x1a/0x30
=================================

To address this issue, it is recommended to add locks when reading
inode->disk_i_size and setting the value of modify_tree to prevent
data inconsistency.

Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
---
 fs/btrfs/file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 4fb521d91b06..189708e6e91a 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -242,8 +242,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 	if (args->drop_cache)
 		btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
 
+	spin_lock(&inode->lock);
 	if (args->start >= inode->disk_i_size && !args->replace_extent)
 		modify_tree = 0;
+	spin_unlock(&inode->lock);
 
 	update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
 	while (1) {
-- 
2.34.1

Re: [PATCH] fs: Fix data race in btrfs_drop_extents

[Filipe Manana]

On Sun, Dec 1, 2024 at 11:26 AM Hao-ran Zheng <zhenghaoran154@gmail.com> wrote:
>
> A data race occurs when the function `insert_ordered_extent_file_extent()`
> and the function `btrfs_inode_safe_disk_i_size_write()` are executed
> concurrently. The function `insert_ordered_extent_file_extent()` is not
> locked when reading inode->disk_i_size, causing
> `btrfs_inode_safe_disk_i_size_write()`to cause data competition when
> writing inode->disk_i_size, thus affecting the value of `modify_tree`,
> leading to some unexpected results such as disk data being overwritten.

How can that cause "disk data being overwritten"?
And the results are not unexpected at all.

The value of modify_tree is irrelevant from a correctness point of view.
It's used for an optimization to avoid taking write locks on the btree
in case we're doing a write at or beyond eof.

If we end up taking a write lock when it's not needed, everything's
fine - we just may unnecessarily block concurrent readers that need to
access the same btree path (leaf and parent node).

If we don't take a write lock and we need it, we will later figure
that out and switch to a write lock.

> The specific call stack that appears during testing is as follows:
>
> ============DATA_RACE============
>  btrfs_drop_extents+0x89a/0xa060 [btrfs]
>  insert_reserved_file_extent+0xb54/0x2960 [btrfs]
>  insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
>  btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
>  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
>  finish_ordered_fn+0x3e/0x50 [btrfs]
>  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
>  process_scheduled_works+0x716/0xf10
>  worker_thread+0xb6a/0x1190
>  kthread+0x292/0x330
>  ret_from_fork+0x4d/0x80
>  ret_from_fork_asm+0x1a/0x30
> ============OTHER_INFO============
>  btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
>  btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
>  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
>  finish_ordered_fn+0x3e/0x50 [btrfs]
>  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
>  process_scheduled_works+0x716/0xf10
>  worker_thread+0xb6a/0x1190
>  kthread+0x292/0x330
>  ret_from_fork+0x4d/0x80
>  ret_from_fork_asm+0x1a/0x30
> =================================
>
> To address this issue, it is recommended to add locks when reading
> inode->disk_i_size and setting the value of modify_tree to prevent
> data inconsistency.

Can also use data_race() here, as it's a harmless race.

Also, please use a proper subject like for example:

btrfs: fix data race when accessing the inode's disk_i_size at
btrfs_drop_extents()

Also please update the changelog with a proper analysis - saying it's
a harmless race and why.

Thanks.

>
> Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
> ---
>  fs/btrfs/file.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 4fb521d91b06..189708e6e91a 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -242,8 +242,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>         if (args->drop_cache)
>                 btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
>
> +       spin_lock(&inode->lock);
>         if (args->start >= inode->disk_i_size && !args->replace_extent)
>                 modify_tree = 0;
> +       spin_unlock(&inode->lock);
>
>         update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
>         while (1) {
> --
> 2.34.1
>
>

Re: [PATCH] fs: Fix data race in btrfs_drop_extents

[haoran zheng]

I read the relevant code again and found that modify_tree is also used for
judgment at line 331, but the judgment here may lead to the release of the
path. Will this cause other problems such as unexpected path release?
331: if (recow || !modify_tree) {
332:     modify_tree = -1;
333:     btrfs_release_path(path);
334:     continue;
335: }

On Mon, Dec 2, 2024 at 11:13 AM haoran zheng <zhenghaoran154@gmail.com> wrote:
>
> Thanks for the explanation. I will fix the description and resubmit the patch.
>
> On Mon, Dec 2, 2024 at 1:39 AM Filipe Manana <fdmanana@kernel.org> wrote:
>>
>> On Sun, Dec 1, 2024 at 11:26 AM Hao-ran Zheng <zhenghaoran154@gmail.com> wrote:
>> >
>> > A data race occurs when the function `insert_ordered_extent_file_extent()`
>> > and the function `btrfs_inode_safe_disk_i_size_write()` are executed
>> > concurrently. The function `insert_ordered_extent_file_extent()` is not
>> > locked when reading inode->disk_i_size, causing
>> > `btrfs_inode_safe_disk_i_size_write()`to cause data competition when
>> > writing inode->disk_i_size, thus affecting the value of `modify_tree`,
>> > leading to some unexpected results such as disk data being overwritten.
>>
>> How can that cause "disk data being overwritten"?
>> And the results are not unexpected at all.
>>
>> The value of modify_tree is irrelevant from a correctness point of view.
>> It's used for an optimization to avoid taking write locks on the btree
>> in case we're doing a write at or beyond eof.
>>
>> If we end up taking a write lock when it's not needed, everything's
>> fine - we just may unnecessarily block concurrent readers that need to
>> access the same btree path (leaf and parent node).
>>
>> If we don't take a write lock and we need it, we will later figure
>> that out and switch to a write lock.
>>
>> > The specific call stack that appears during testing is as follows:
>> >
>> > ============DATA_RACE============
>> >  btrfs_drop_extents+0x89a/0xa060 [btrfs]
>> >  insert_reserved_file_extent+0xb54/0x2960 [btrfs]
>> >  insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
>> >  btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
>> >  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
>> >  finish_ordered_fn+0x3e/0x50 [btrfs]
>> >  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
>> >  process_scheduled_works+0x716/0xf10
>> >  worker_thread+0xb6a/0x1190
>> >  kthread+0x292/0x330
>> >  ret_from_fork+0x4d/0x80
>> >  ret_from_fork_asm+0x1a/0x30
>> > ============OTHER_INFO============
>> >  btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
>> >  btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
>> >  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
>> >  finish_ordered_fn+0x3e/0x50 [btrfs]
>> >  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
>> >  process_scheduled_works+0x716/0xf10
>> >  worker_thread+0xb6a/0x1190
>> >  kthread+0x292/0x330
>> >  ret_from_fork+0x4d/0x80
>> >  ret_from_fork_asm+0x1a/0x30
>> > =================================
>> >
>> > To address this issue, it is recommended to add locks when reading
>> > inode->disk_i_size and setting the value of modify_tree to prevent
>> > data inconsistency.
>>
>> Can also use data_race() here, as it's a harmless race.
>>
>> Also, please use a proper subject like for example:
>>
>> btrfs: fix data race when accessing the inode's disk_i_size at
>> btrfs_drop_extents()
>>
>> Also please update the changelog with a proper analysis - saying it's
>> a harmless race and why.
>>
>> Thanks.
>>
>> >
>> > Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
>> > ---
>> >  fs/btrfs/file.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> > index 4fb521d91b06..189708e6e91a 100644
>> > --- a/fs/btrfs/file.c
>> > +++ b/fs/btrfs/file.c
>> > @@ -242,8 +242,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>> >         if (args->drop_cache)
>> >                 btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
>> >
>> > +       spin_lock(&inode->lock);
>> >         if (args->start >= inode->disk_i_size && !args->replace_extent)
>> >                 modify_tree = 0;
>> > +       spin_unlock(&inode->lock);
>> >
>> >         update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
>> >         while (1) {
>> > --
>> > 2.34.1
>> >
>> >

Re: [PATCH] fs: Fix data race in btrfs_drop_extents

[Filipe Manana]

On Mon, Dec 2, 2024 at 3:36 AM haoran zheng <zhenghaoran154@gmail.com> wrote:
>
> I read the relevant code again and found that modify_tree is also used for
> judgment at line 331, but the judgment here may lead to the release of the
> path. Will this cause other problems such as unexpected path release?
> 331: if (recow || !modify_tree) {
> 332:     modify_tree = -1;
> 333:     btrfs_release_path(path);
> 334:     continue;
> 335: }

This is what I told you before - this is triggered when we have taken
a read lock (modify_tree == 0) but it turns out we need a write lock
(in order to drop extent items from the inode's root).
In that case we release the path and reacquire it again with write
locks, that's the meaning of setting modify_tree to -1 and doing the
continue.

Btw, before you do extraordinary claims in a change log such as
"unexpected results" and "disk data being overwritten", make sure you
actually have a good understanding, not just of this
function (which you clearly don't), as well as btrfs' metadata, the
btree data structure and its operations and the write path in general
(which you clearly don't have as well).

Extraordinary claims demand a good explanation.

This time you found a harmless race (with KCSAN), but not long ago
you've sent out another patch that was even more puzzling:

https://lore.kernel.org/linux-btrfs/20241101035133.925251-1-zhenghaoran@buaa.edu.cn/

You claimed a race on code that is run sequentially for a data
structure that is not shared amongst threads, so there's no way on
earth a race condition could happen there.
This lack of explanation demonstrated a total lack of understanding of
the btrfs logging code, and even worse the patch didn't even compile -
you didn't even test compilation, let alone exercise the code.

That sort of behaviour made me think you were a bot or one of those
trolls that often show up (like the infamous Nick Krause some years
ago).
Avoid that sort of behaviour please, otherwise no one will take you seriously.

Thanks.

>
> On Mon, Dec 2, 2024 at 11:13 AM haoran zheng <zhenghaoran154@gmail.com> wrote:
> >
> > Thanks for the explanation. I will fix the description and resubmit the patch.
> >
> > On Mon, Dec 2, 2024 at 1:39 AM Filipe Manana <fdmanana@kernel.org> wrote:
> >>
> >> On Sun, Dec 1, 2024 at 11:26 AM Hao-ran Zheng <zhenghaoran154@gmail.com> wrote:
> >> >
> >> > A data race occurs when the function `insert_ordered_extent_file_extent()`
> >> > and the function `btrfs_inode_safe_disk_i_size_write()` are executed
> >> > concurrently. The function `insert_ordered_extent_file_extent()` is not
> >> > locked when reading inode->disk_i_size, causing
> >> > `btrfs_inode_safe_disk_i_size_write()`to cause data competition when
> >> > writing inode->disk_i_size, thus affecting the value of `modify_tree`,
> >> > leading to some unexpected results such as disk data being overwritten.
> >>
> >> How can that cause "disk data being overwritten"?
> >> And the results are not unexpected at all.
> >>
> >> The value of modify_tree is irrelevant from a correctness point of view.
> >> It's used for an optimization to avoid taking write locks on the btree
> >> in case we're doing a write at or beyond eof.
> >>
> >> If we end up taking a write lock when it's not needed, everything's
> >> fine - we just may unnecessarily block concurrent readers that need to
> >> access the same btree path (leaf and parent node).
> >>
> >> If we don't take a write lock and we need it, we will later figure
> >> that out and switch to a write lock.
> >>
> >> > The specific call stack that appears during testing is as follows:
> >> >
> >> > ============DATA_RACE============
> >> >  btrfs_drop_extents+0x89a/0xa060 [btrfs]
> >> >  insert_reserved_file_extent+0xb54/0x2960 [btrfs]
> >> >  insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
> >> >  btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
> >> >  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
> >> >  finish_ordered_fn+0x3e/0x50 [btrfs]
> >> >  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
> >> >  process_scheduled_works+0x716/0xf10
> >> >  worker_thread+0xb6a/0x1190
> >> >  kthread+0x292/0x330
> >> >  ret_from_fork+0x4d/0x80
> >> >  ret_from_fork_asm+0x1a/0x30
> >> > ============OTHER_INFO============
> >> >  btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
> >> >  btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
> >> >  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
> >> >  finish_ordered_fn+0x3e/0x50 [btrfs]
> >> >  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
> >> >  process_scheduled_works+0x716/0xf10
> >> >  worker_thread+0xb6a/0x1190
> >> >  kthread+0x292/0x330
> >> >  ret_from_fork+0x4d/0x80
> >> >  ret_from_fork_asm+0x1a/0x30
> >> > =================================
> >> >
> >> > To address this issue, it is recommended to add locks when reading
> >> > inode->disk_i_size and setting the value of modify_tree to prevent
> >> > data inconsistency.
> >>
> >> Can also use data_race() here, as it's a harmless race.
> >>
> >> Also, please use a proper subject like for example:
> >>
> >> btrfs: fix data race when accessing the inode's disk_i_size at
> >> btrfs_drop_extents()
> >>
> >> Also please update the changelog with a proper analysis - saying it's
> >> a harmless race and why.
> >>
> >> Thanks.
> >>
> >> >
> >> > Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
> >> > ---
> >> >  fs/btrfs/file.c | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> >
> >> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> >> > index 4fb521d91b06..189708e6e91a 100644
> >> > --- a/fs/btrfs/file.c
> >> > +++ b/fs/btrfs/file.c
> >> > @@ -242,8 +242,10 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
> >> >         if (args->drop_cache)
> >> >                 btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
> >> >
> >> > +       spin_lock(&inode->lock);
> >> >         if (args->start >= inode->disk_i_size && !args->replace_extent)
> >> >                 modify_tree = 0;
> >> > +       spin_unlock(&inode->lock);
> >> >
> >> >         update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
> >> >         while (1) {
> >> > --
> >> > 2.34.1
> >> >
> >> >

[PATCH v2] btrfs: fix data race when accessing the inode's disk_i_size at btrfs_drop_extents()

[Hao-ran Zheng]

A data race occurs when the function `insert_ordered_extent_file_extent()`
and the function `btrfs_inode_safe_disk_i_size_write()` are executed
concurrently. The function `insert_ordered_extent_file_extent()` is not
locked when reading inode->disk_i_size, causing
`btrfs_inode_safe_disk_i_size_write()` to cause data competition when
writing inode->disk_i_size, thus affecting the value of `modify_tree`.

Since the impact of data race is limited, it is recommended to use the
`data_race` mark judgment.

The specific call stack that appears during testing is as follows:
============DATA_RACE============
 btrfs_drop_extents+0x89a/0xa060 [btrfs]
 insert_reserved_file_extent+0xb54/0x2960 [btrfs]
 insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
 btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
 btrfs_finish_ordered_io+0x37/0x60 [btrfs]
 finish_ordered_fn+0x3e/0x50 [btrfs]
 btrfs_work_helper+0x9c9/0x27a0 [btrfs]
 process_scheduled_works+0x716/0xf10
 worker_thread+0xb6a/0x1190
 kthread+0x292/0x330
 ret_from_fork+0x4d/0x80
 ret_from_fork_asm+0x1a/0x30
============OTHER_INFO============
 btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
 btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
 btrfs_finish_ordered_io+0x37/0x60 [btrfs]
 finish_ordered_fn+0x3e/0x50 [btrfs]
 btrfs_work_helper+0x9c9/0x27a0 [btrfs]
 process_scheduled_works+0x716/0xf10
 worker_thread+0xb6a/0x1190
 kthread+0x292/0x330
 ret_from_fork+0x4d/0x80
 ret_from_fork_asm+0x1a/0x30
=================================

Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
---
V1->V2: Modify patch description and format
---
 fs/btrfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 4fb521d91b06..f9631713f67d 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -242,7 +242,7 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 	if (args->drop_cache)
 		btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
 
-	if (args->start >= inode->disk_i_size && !args->replace_extent)
+	if (data_race(args->start >= inode->disk_i_size && !args->replace_extent))
 		modify_tree = 0;
 
 	update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
-- 
2.34.1

Re: [PATCH v2] btrfs: fix data race when accessing the inode's disk_i_size at btrfs_drop_extents()

[Filipe Manana]

On Mon, Dec 2, 2024 at 1:35 PM Hao-ran Zheng <zhenghaoran154@gmail.com> wrote:
>
> A data race occurs when the function `insert_ordered_extent_file_extent()`
> and the function `btrfs_inode_safe_disk_i_size_write()` are executed
> concurrently. The function `insert_ordered_extent_file_extent()` is not
> locked when reading inode->disk_i_size, causing
> `btrfs_inode_safe_disk_i_size_write()` to cause data competition when
> writing inode->disk_i_size, thus affecting the value of `modify_tree`.
>
> Since the impact of data race is limited, it is recommended to use the
> `data_race` mark judgment.

This should explain why it's a harmless race.
Also it's better to explicitly say harmless race rather than say it
has limited impact, because the latter gives the idea of rare problems
when we don't have any.

>
> The specific call stack that appears during testing is as follows:
> ============DATA_RACE============
>  btrfs_drop_extents+0x89a/0xa060 [btrfs]
>  insert_reserved_file_extent+0xb54/0x2960 [btrfs]
>  insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
>  btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
>  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
>  finish_ordered_fn+0x3e/0x50 [btrfs]
>  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
>  process_scheduled_works+0x716/0xf10
>  worker_thread+0xb6a/0x1190
>  kthread+0x292/0x330
>  ret_from_fork+0x4d/0x80
>  ret_from_fork_asm+0x1a/0x30
> ============OTHER_INFO============
>  btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
>  btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
>  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
>  finish_ordered_fn+0x3e/0x50 [btrfs]
>  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
>  process_scheduled_works+0x716/0xf10
>  worker_thread+0xb6a/0x1190
>  kthread+0x292/0x330
>  ret_from_fork+0x4d/0x80
>  ret_from_fork_asm+0x1a/0x30
> =================================
>
> Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
> ---
> V1->V2: Modify patch description and format
> ---
>  fs/btrfs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 4fb521d91b06..f9631713f67d 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -242,7 +242,7 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>         if (args->drop_cache)
>                 btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
>
> -       if (args->start >= inode->disk_i_size && !args->replace_extent)
> +       if (data_race(args->start >= inode->disk_i_size && !args->replace_extent))

Don't surround the whole condition with data_race().
Just put it around the disk_i_size check:

if (data_race(args->start >= inode->disk_i_size) && !args->replace_extent))

This makes it more clear it's about disk_i_size and nothing else.

Thanks.

>                 modify_tree = 0;
>
>         update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
> --
> 2.34.1
>
>

Re: [PATCH v2] btrfs: fix data race when accessing the inode's disk_i_size at btrfs_drop_extents()

[haoran zheng]

Thanks for the advice, I will explain in detail why it's a harmless race.
And submit patch v3.

On Mon, Dec 2, 2024 at 10:40 PM Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Mon, Dec 2, 2024 at 1:35 PM Hao-ran Zheng <zhenghaoran154@gmail.com> wrote:
> >
> > A data race occurs when the function `insert_ordered_extent_file_extent()`
> > and the function `btrfs_inode_safe_disk_i_size_write()` are executed
> > concurrently. The function `insert_ordered_extent_file_extent()` is not
> > locked when reading inode->disk_i_size, causing
> > `btrfs_inode_safe_disk_i_size_write()` to cause data competition when
> > writing inode->disk_i_size, thus affecting the value of `modify_tree`.
> >
> > Since the impact of data race is limited, it is recommended to use the
> > `data_race` mark judgment.
>
> This should explain why it's a harmless race.
> Also it's better to explicitly say harmless race rather than say it
> has limited impact, because the latter gives the idea of rare problems
> when we don't have any.
>
> >
> > The specific call stack that appears during testing is as follows:
> > ============DATA_RACE============
> >  btrfs_drop_extents+0x89a/0xa060 [btrfs]
> >  insert_reserved_file_extent+0xb54/0x2960 [btrfs]
> >  insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
> >  btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
> >  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
> >  finish_ordered_fn+0x3e/0x50 [btrfs]
> >  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
> >  process_scheduled_works+0x716/0xf10
> >  worker_thread+0xb6a/0x1190
> >  kthread+0x292/0x330
> >  ret_from_fork+0x4d/0x80
> >  ret_from_fork_asm+0x1a/0x30
> > ============OTHER_INFO============
> >  btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
> >  btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
> >  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
> >  finish_ordered_fn+0x3e/0x50 [btrfs]
> >  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
> >  process_scheduled_works+0x716/0xf10
> >  worker_thread+0xb6a/0x1190
> >  kthread+0x292/0x330
> >  ret_from_fork+0x4d/0x80
> >  ret_from_fork_asm+0x1a/0x30
> > =================================
> >
> > Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
> > ---
> > V1->V2: Modify patch description and format
> > ---
> >  fs/btrfs/file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 4fb521d91b06..f9631713f67d 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -242,7 +242,7 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
> >         if (args->drop_cache)
> >                 btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
> >
> > -       if (args->start >= inode->disk_i_size && !args->replace_extent)
> > +       if (data_race(args->start >= inode->disk_i_size && !args->replace_extent))
>
> Don't surround the whole condition with data_race().
> Just put it around the disk_i_size check:
>
> if (data_race(args->start >= inode->disk_i_size) && !args->replace_extent))
>
> This makes it more clear it's about disk_i_size and nothing else.
>
> Thanks.
>
> >                 modify_tree = 0;
> >
> >         update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
> > --
> > 2.34.1
> >
> >

[PATCH v3] btrfs: fix data race when accessing the inode's disk_i_size at btrfs_drop_extents()

[Hao-ran Zheng]

A data race occurs when the function `insert_ordered_extent_file_extent()`
and the function `btrfs_inode_safe_disk_i_size_write()` are executed
concurrently. The function `insert_ordered_extent_file_extent()` is not
locked when reading inode->disk_i_size, causing
`btrfs_inode_safe_disk_i_size_write()` to cause data competition when
writing inode->disk_i_size, thus affecting the value of `modify_tree`.

The specific call stack that appears during testing is as follows:
============DATA_RACE============
 btrfs_drop_extents+0x89a/0xa060 [btrfs]
 insert_reserved_file_extent+0xb54/0x2960 [btrfs]
 insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
 btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
 btrfs_finish_ordered_io+0x37/0x60 [btrfs]
 finish_ordered_fn+0x3e/0x50 [btrfs]
 btrfs_work_helper+0x9c9/0x27a0 [btrfs]
 process_scheduled_works+0x716/0xf10
 worker_thread+0xb6a/0x1190
 kthread+0x292/0x330
 ret_from_fork+0x4d/0x80
 ret_from_fork_asm+0x1a/0x30
============OTHER_INFO============
 btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
 btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
 btrfs_finish_ordered_io+0x37/0x60 [btrfs]
 finish_ordered_fn+0x3e/0x50 [btrfs]
 btrfs_work_helper+0x9c9/0x27a0 [btrfs]
 process_scheduled_works+0x716/0xf10
 worker_thread+0xb6a/0x1190
 kthread+0x292/0x330
 ret_from_fork+0x4d/0x80
 ret_from_fork_asm+0x1a/0x30
=================================

The main purpose of the modify_tree variable is to optimize the
acquisition of read-write locks at or after the end of file (EOF).
When the value of modify_tree is modified due to concurrency, resulting
in unnecessary acquisition of write locks, the correctness of the
system will not be affected. When the system requires a write lock but
does not acquire the write lock because the value of modify_tree is
incorrect, the path will be subsequently released and the lock will
be reacquired to ensure the correctness of the system.

Since this data race does not affect the correctness of the function,
it is a harmless data race, and it is recommended to use `data_race`
to mark it.

Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
---
V2->V3: Added details on why this is a harmless data race
V1->V2: Modified patch description and format
---
 fs/btrfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 4fb521d91b06..559c177456e6 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -242,7 +242,7 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 	if (args->drop_cache)
 		btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
 
-	if (args->start >= inode->disk_i_size && !args->replace_extent)
+	if (data_race(args->start >= inode->disk_i_size) && !args->replace_extent)
 		modify_tree = 0;
 
 	update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
-- 
2.34.1

Re: [PATCH v3] btrfs: fix data race when accessing the inode's disk_i_size at btrfs_drop_extents()

[Filipe Manana]

On Tue, Dec 3, 2024 at 7:59 AM Hao-ran Zheng <zhenghaoran154@gmail.com> wrote:
>
> A data race occurs when the function `insert_ordered_extent_file_extent()`
> and the function `btrfs_inode_safe_disk_i_size_write()` are executed
> concurrently. The function `insert_ordered_extent_file_extent()` is not
> locked when reading inode->disk_i_size, causing
> `btrfs_inode_safe_disk_i_size_write()` to cause data competition when
> writing inode->disk_i_size, thus affecting the value of `modify_tree`.
>
> The specific call stack that appears during testing is as follows:
> ============DATA_RACE============
>  btrfs_drop_extents+0x89a/0xa060 [btrfs]
>  insert_reserved_file_extent+0xb54/0x2960 [btrfs]
>  insert_ordered_extent_file_extent+0xff5/0x1760 [btrfs]
>  btrfs_finish_one_ordered+0x1b85/0x36a0 [btrfs]
>  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
>  finish_ordered_fn+0x3e/0x50 [btrfs]
>  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
>  process_scheduled_works+0x716/0xf10
>  worker_thread+0xb6a/0x1190
>  kthread+0x292/0x330
>  ret_from_fork+0x4d/0x80
>  ret_from_fork_asm+0x1a/0x30
> ============OTHER_INFO============
>  btrfs_inode_safe_disk_i_size_write+0x4ec/0x600 [btrfs]
>  btrfs_finish_one_ordered+0x24c7/0x36a0 [btrfs]
>  btrfs_finish_ordered_io+0x37/0x60 [btrfs]
>  finish_ordered_fn+0x3e/0x50 [btrfs]
>  btrfs_work_helper+0x9c9/0x27a0 [btrfs]
>  process_scheduled_works+0x716/0xf10
>  worker_thread+0xb6a/0x1190
>  kthread+0x292/0x330
>  ret_from_fork+0x4d/0x80
>  ret_from_fork_asm+0x1a/0x30
> =================================
>
> The main purpose of the modify_tree variable is to optimize the
> acquisition of read-write locks at or after the end of file (EOF).
> When the value of modify_tree is modified due to concurrency, resulting
> in unnecessary acquisition of write locks, the correctness of the
> system will not be affected. When the system requires a write lock but
> does not acquire the write lock because the value of modify_tree is
> incorrect, the path will be subsequently released and the lock will
> be reacquired to ensure the correctness of the system.

Looks good, I'll slightly rephrase this on things like "modify_tree is
modified due to concurrency" because what gets modified concurrently
is inode->disk_i_size.
I'll push it to the github btrfs for-next branch soon with this
paragraph instead:

"The main purpose of the check of the inode's disk_i_size is to avoid
taking write locks on a btree path when we have a write at or beyond
eof, since in these cases we don't expect to find extent items in the
root to drop. However if we end up taking write locks due to a data
race on disk_i_size, everything is still correct, we only add extra
lock contention on the tree in case there's concurrency from other tasks.
If the race causes us to not take write locks when we actually need them,
then everything is functionally correct as well, since if we find out we
have extent items to drop and we took read locks (modify_tree set to 0),
we release the path and retry again with write locks."

Thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

>
> Since this data race does not affect the correctness of the function,
> it is a harmless data race, and it is recommended to use `data_race`
> to mark it.
>
> Signed-off-by: Hao-ran Zheng <zhenghaoran154@gmail.com>
> ---
> V2->V3: Added details on why this is a harmless data race
> V1->V2: Modified patch description and format
> ---
>  fs/btrfs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 4fb521d91b06..559c177456e6 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -242,7 +242,7 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>         if (args->drop_cache)
>                 btrfs_drop_extent_map_range(inode, args->start, args->end - 1, false);
>
> -       if (args->start >= inode->disk_i_size && !args->replace_extent)
> +       if (data_race(args->start >= inode->disk_i_size) && !args->replace_extent)
>                 modify_tree = 0;
>
>         update_refs = (btrfs_root_id(root) != BTRFS_TREE_LOG_OBJECTID);
> --
> 2.34.1
>
>