| Message ID | r3pglf6fh5hqqfd6iwt5eklmalm4idnumjxo5v23buu7zfjdfm@ixnvwldw5yjg (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: do not clear EXTENT_LOCKED in try_release_extent_state() | expand |
On Fri, Jun 9, 2023 at 2:20 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > clear_bits unsets EXTENT_LOCKED in the else branch of checking > EXTENT_LOCKED. It doesn't. Because of the negation operator: u32 clear_bits = ~(EXTENT_LOCKED | EXTENT_NODATASUM | ... > At this point, it is not possible that EXTENT_LOCKED bits > are set, so do not clear EXTENT_LOCKED. And it doesn't clear them because of ~ > > Besides, The comment above try_release_extent_state():__clear_extent_bit() > also says that locked bit should not be cleared. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > --- > fs/btrfs/extent_io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index a91d5ad27984..e5bec73b5991 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -2357,7 +2357,7 @@ static int try_release_extent_state(struct extent_io_tree *tree, > if (test_range_bit(tree, start, end, EXTENT_LOCKED, 0, NULL)) { > ret = 0; > } else { > - u32 clear_bits = ~(EXTENT_LOCKED | EXTENT_NODATASUM | > + u32 clear_bits = ~(EXTENT_NODATASUM | > EXTENT_DELALLOC_NEW | EXTENT_CTLBITS); So because of the ~, this is actually introducing a bug: 1) test_range_bit() returns false because the range is locked 2) some other task locks the range just after the test_range_bit() call 3) we get to the else branch, clear_bits has EXTENT_LOCKED, because of the ~, and we unlock the range when we shouldn't.... Thanks. > > /* > -- > 2.40.1
On Fri, Jun 9, 2023 at 2:25 PM Filipe Manana <fdmanana@kernel.org> wrote: > > On Fri, Jun 9, 2023 at 2:20 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > > > clear_bits unsets EXTENT_LOCKED in the else branch of checking > > EXTENT_LOCKED. > > It doesn't. Because of the negation operator: > > u32 clear_bits = ~(EXTENT_LOCKED | EXTENT_NODATASUM | ... > > > At this point, it is not possible that EXTENT_LOCKED bits > > are set, so do not clear EXTENT_LOCKED. > > And it doesn't clear them because of ~ > > > > > Besides, The comment above try_release_extent_state():__clear_extent_bit() > > also says that locked bit should not be cleared. > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > --- > > fs/btrfs/extent_io.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > index a91d5ad27984..e5bec73b5991 100644 > > --- a/fs/btrfs/extent_io.c > > +++ b/fs/btrfs/extent_io.c > > @@ -2357,7 +2357,7 @@ static int try_release_extent_state(struct extent_io_tree *tree, > > if (test_range_bit(tree, start, end, EXTENT_LOCKED, 0, NULL)) { > > ret = 0; > > } else { > > - u32 clear_bits = ~(EXTENT_LOCKED | EXTENT_NODATASUM | > > + u32 clear_bits = ~(EXTENT_NODATASUM | > > EXTENT_DELALLOC_NEW | EXTENT_CTLBITS); > > So because of the ~, this is actually introducing a bug: > > 1) test_range_bit() returns false because the range is locked not locked, of course > 2) some other task locks the range just after the test_range_bit() call > 3) we get to the else branch, clear_bits has EXTENT_LOCKED, because of > the ~, and we unlock the range when we shouldn't.... > > Thanks. > > > > > /* > > -- > > 2.40.1
On 14:25 09/06, Filipe Manana wrote: > On Fri, Jun 9, 2023 at 2:20 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > > > clear_bits unsets EXTENT_LOCKED in the else branch of checking > > EXTENT_LOCKED. > > It doesn't. Because of the negation operator: > > u32 clear_bits = ~(EXTENT_LOCKED | EXTENT_NODATASUM | ... > > > At this point, it is not possible that EXTENT_LOCKED bits > > are set, so do not clear EXTENT_LOCKED. > > And it doesn't clear them because of ~ Yes, apparently I have to re-learn to read code. This patch is incorrect.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index a91d5ad27984..e5bec73b5991 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2357,7 +2357,7 @@ static int try_release_extent_state(struct extent_io_tree *tree, if (test_range_bit(tree, start, end, EXTENT_LOCKED, 0, NULL)) { ret = 0; } else { - u32 clear_bits = ~(EXTENT_LOCKED | EXTENT_NODATASUM | + u32 clear_bits = ~(EXTENT_NODATASUM | EXTENT_DELALLOC_NEW | EXTENT_CTLBITS); /*
clear_bits unsets EXTENT_LOCKED in the else branch of checking EXTENT_LOCKED. At this point, it is not possible that EXTENT_LOCKED bits are set, so do not clear EXTENT_LOCKED. Besides, The comment above try_release_extent_state():__clear_extent_bit() also says that locked bit should not be cleared. Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> --- fs/btrfs/extent_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)