| Message ID | fc21b3d5ddf062b746bc55425672969f897d685d.1681801005.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: fix bitops api misuse | expand |
On Tue, Apr 18, 2023 at 05:45:24PM +0900, Naohiro Aota wrote: > From: Naohiro Aota <naohiro.aota@wdc.com> > > find_next_bit and find_next_zero_bit take @size as the second parameter and > @offset as the third parameter. They are specified opposite in > btrfs_ensure_empty_zones(). Thanks to the later loop, it never failed to > detect the empty zones. Fix them and (maybe) return the result a bit > faster. > > Fixes: 1cd6121f2a38 ("btrfs: zoned: implement zoned chunk allocator") > CC: stable@vger.kernel.org # 5.15+ > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > --- > fs/btrfs/zoned.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index 2b160fda7301..55bde1336d81 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -1171,12 +1171,12 @@ int btrfs_ensure_empty_zones(struct btrfs_device *device, u64 start, u64 size) > return -ERANGE; > > /* All the zones are conventional */ > - if (find_next_bit(zinfo->seq_zones, begin, end) == end) > + if (find_next_bit(zinfo->seq_zones, end, begin) == end) End is defined as "end = (start + size) >> shift", and the 2nd parameter of find_next_bit is supposed to be 'size'. Shouldn't it be "size >> shift"?
On Wed, Apr 19, 2023 at 01:24:56AM +0200, David Sterba wrote: > On Tue, Apr 18, 2023 at 05:45:24PM +0900, Naohiro Aota wrote: > > From: Naohiro Aota <naohiro.aota@wdc.com> > > > > find_next_bit and find_next_zero_bit take @size as the second parameter and > > @offset as the third parameter. They are specified opposite in > > btrfs_ensure_empty_zones(). Thanks to the later loop, it never failed to > > detect the empty zones. Fix them and (maybe) return the result a bit > > faster. > > > > Fixes: 1cd6121f2a38 ("btrfs: zoned: implement zoned chunk allocator") > > CC: stable@vger.kernel.org # 5.15+ > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > --- > > fs/btrfs/zoned.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > > index 2b160fda7301..55bde1336d81 100644 > > --- a/fs/btrfs/zoned.c > > +++ b/fs/btrfs/zoned.c > > @@ -1171,12 +1171,12 @@ int btrfs_ensure_empty_zones(struct btrfs_device *device, u64 start, u64 size) > > return -ERANGE; > > > > /* All the zones are conventional */ > > - if (find_next_bit(zinfo->seq_zones, begin, end) == end) > > + if (find_next_bit(zinfo->seq_zones, end, begin) == end) > > End is defined as "end = (start + size) >> shift", and the 2nd parameter > of find_next_bit is supposed to be 'size'. Shouldn't it be "size >> > shift"? Not so. The argument "size" represents the size of the allocation range, which is to be confirmed as empty. OTOH, find_next_bit()'s "size" (the 2nd parameter) represents the size of an entire bitmap, not the number of bits to be tested.
On Thu, Apr 20, 2023 at 12:58:14PM +0900, Naohiro Aota wrote: > On Wed, Apr 19, 2023 at 01:24:56AM +0200, David Sterba wrote: > > On Tue, Apr 18, 2023 at 05:45:24PM +0900, Naohiro Aota wrote: > > > From: Naohiro Aota <naohiro.aota@wdc.com> > > > > > > find_next_bit and find_next_zero_bit take @size as the second parameter and > > > @offset as the third parameter. They are specified opposite in > > > btrfs_ensure_empty_zones(). Thanks to the later loop, it never failed to > > > detect the empty zones. Fix them and (maybe) return the result a bit > > > faster. > > > > > > Fixes: 1cd6121f2a38 ("btrfs: zoned: implement zoned chunk allocator") > > > CC: stable@vger.kernel.org # 5.15+ > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > > --- > > > fs/btrfs/zoned.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > > > index 2b160fda7301..55bde1336d81 100644 > > > --- a/fs/btrfs/zoned.c > > > +++ b/fs/btrfs/zoned.c > > > @@ -1171,12 +1171,12 @@ int btrfs_ensure_empty_zones(struct btrfs_device *device, u64 start, u64 size) > > > return -ERANGE; > > > > > > /* All the zones are conventional */ > > > - if (find_next_bit(zinfo->seq_zones, begin, end) == end) > > > + if (find_next_bit(zinfo->seq_zones, end, begin) == end) > > > > End is defined as "end = (start + size) >> shift", and the 2nd parameter > > of find_next_bit is supposed to be 'size'. Shouldn't it be "size >> > > shift"? > > Not so. The argument "size" represents the size of the allocation range, > which is to be confirmed as empty. OTOH, find_next_bit()'s "size" (the 2nd > parameter) represents the size of an entire bitmap, not the number of bits to > be tested. BTW, I found the same logic is implemented in subpage.c as bitmap_test_range_all_{set,zero}. So, it might worth moving them to somewhere (misc.h?) and use them here.
On Thu, Apr 20, 2023 at 01:14:30PM +0900, Naohiro Aota wrote: > On Thu, Apr 20, 2023 at 12:58:14PM +0900, Naohiro Aota wrote: > > On Wed, Apr 19, 2023 at 01:24:56AM +0200, David Sterba wrote: > > > On Tue, Apr 18, 2023 at 05:45:24PM +0900, Naohiro Aota wrote: > > > > From: Naohiro Aota <naohiro.aota@wdc.com> > > > > > > > > find_next_bit and find_next_zero_bit take @size as the second parameter and > > > > @offset as the third parameter. They are specified opposite in > > > > btrfs_ensure_empty_zones(). Thanks to the later loop, it never failed to > > > > detect the empty zones. Fix them and (maybe) return the result a bit > > > > faster. > > > > > > > > Fixes: 1cd6121f2a38 ("btrfs: zoned: implement zoned chunk allocator") > > > > CC: stable@vger.kernel.org # 5.15+ > > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > > > --- > > > > fs/btrfs/zoned.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > > > > index 2b160fda7301..55bde1336d81 100644 > > > > --- a/fs/btrfs/zoned.c > > > > +++ b/fs/btrfs/zoned.c > > > > @@ -1171,12 +1171,12 @@ int btrfs_ensure_empty_zones(struct btrfs_device *device, u64 start, u64 size) > > > > return -ERANGE; > > > > > > > > /* All the zones are conventional */ > > > > - if (find_next_bit(zinfo->seq_zones, begin, end) == end) > > > > + if (find_next_bit(zinfo->seq_zones, end, begin) == end) > > > > > > End is defined as "end = (start + size) >> shift", and the 2nd parameter > > > of find_next_bit is supposed to be 'size'. Shouldn't it be "size >> > > > shift"? > > > > Not so. The argument "size" represents the size of the allocation range, > > which is to be confirmed as empty. OTOH, find_next_bit()'s "size" (the 2nd > > parameter) represents the size of an entire bitmap, not the number of bits to > > be tested. > > BTW, I found the same logic is implemented in subpage.c as > bitmap_test_range_all_{set,zero}. So, it might worth moving them to > somewhere (misc.h?) and use them here. Good idea, please send it as a separate patch. Thanks.
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index 2b160fda7301..55bde1336d81 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -1171,12 +1171,12 @@ int btrfs_ensure_empty_zones(struct btrfs_device *device, u64 start, u64 size) return -ERANGE; /* All the zones are conventional */ - if (find_next_bit(zinfo->seq_zones, begin, end) == end) + if (find_next_bit(zinfo->seq_zones, end, begin) == end) return 0; /* All the zones are sequential and empty */ - if (find_next_zero_bit(zinfo->seq_zones, begin, end) == end && - find_next_zero_bit(zinfo->empty_zones, begin, end) == end) + if (find_next_zero_bit(zinfo->seq_zones, end, begin) == end && + find_next_zero_bit(zinfo->empty_zones, end, begin) == end) return 0; for (pos = start; pos < start + size; pos += zinfo->zone_size) {