| Message ID | 20250121054054.4008049-2-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] btrfs: Fix some folio-related comments | expand |
在 2025/1/21 16:10, Matthew Wilcox (Oracle) 写道: > It is meaningless to shift a byte count by folio_shift(). The folio index > is in units of PAGE_SIZE, not folio_size(). We can use folio_contains() > to make this work for arbitrary-order folios, so remove the assertion > that the folios are of order 0. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/btrfs/extent_io.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 289ecb8ce217..c9b0ee841501 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -523,8 +523,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) > u64 end; > u32 len; > > - /* For now only order 0 folios are supported for data. */ > - ASSERT(folio_order(folio) == 0); I'd prefer to keep this ASSERT(), as all the btrfs_folio_*() helpers can only handle page sized folio for now. > btrfs_debug(fs_info, > "%s: bi_sector=%llu, err=%d, mirror=%u", > __func__, bio->bi_iter.bi_sector, bio->bi_status, > @@ -552,7 +550,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) > > if (likely(uptodate)) { > loff_t i_size = i_size_read(inode); > - pgoff_t end_index = i_size >> folio_shift(folio); > > /* > * Zero out the remaining part if this range straddles > @@ -563,7 +560,8 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) > * > * NOTE: i_size is exclusive while end is inclusive. > */ > - if (folio_index(folio) == end_index && i_size <= end) { > + if (folio_contains(folio, i_size >> PAGE_SHIFT) && Although folio_contains() is already a pretty good improvement, can we have a bytenr/i_sized based solution for fs usages? Thanks, Qu > + i_size <= end) { > u32 zero_start = max(offset_in_folio(folio, i_size), > offset_in_folio(folio, start)); > u32 zero_len = offset_in_folio(folio, end) + 1 - > @@ -956,7 +954,7 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, > return ret; > } > > - if (folio->index == last_byte >> folio_shift(folio)) { > + if (folio_contains(folio, last_byte >> PAGE_SHIFT)) { > size_t zero_offset = offset_in_folio(folio, last_byte); > > if (zero_offset) {
On Tue, Jan 21, 2025 at 05:51:40PM +1030, Qu Wenruo wrote: > > > 在 2025/1/21 16:10, Matthew Wilcox (Oracle) 写道: > > It is meaningless to shift a byte count by folio_shift(). The folio index > > is in units of PAGE_SIZE, not folio_size(). We can use folio_contains() > > to make this work for arbitrary-order folios, so remove the assertion > > that the folios are of order 0. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > --- > > fs/btrfs/extent_io.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > index 289ecb8ce217..c9b0ee841501 100644 > > --- a/fs/btrfs/extent_io.c > > +++ b/fs/btrfs/extent_io.c > > @@ -523,8 +523,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) > > u64 end; > > u32 len; > > > > - /* For now only order 0 folios are supported for data. */ > > - ASSERT(folio_order(folio) == 0); > > I'd prefer to keep this ASSERT(), as all the btrfs_folio_*() helpers can > only handle page sized folio for now. > > > btrfs_debug(fs_info, > > "%s: bi_sector=%llu, err=%d, mirror=%u", > > __func__, bio->bi_iter.bi_sector, bio->bi_status, > > @@ -552,7 +550,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) > > > > if (likely(uptodate)) { > > loff_t i_size = i_size_read(inode); > > - pgoff_t end_index = i_size >> folio_shift(folio); > > > > /* > > * Zero out the remaining part if this range straddles > > @@ -563,7 +560,8 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) > > * > > * NOTE: i_size is exclusive while end is inclusive. > > */ > > - if (folio_index(folio) == end_index && i_size <= end) { > > + if (folio_contains(folio, i_size >> PAGE_SHIFT) && > > Although folio_contains() is already a pretty good improvement, can we > have a bytenr/i_sized based solution for fs usages? Good point, something like folio_contains_offset() that does the correct shifts.
On Tue, Jan 21, 2025 at 05:10:11PM +0100, David Sterba wrote: > On Tue, Jan 21, 2025 at 05:51:40PM +1030, Qu Wenruo wrote: > > 在 2025/1/21 16:10, Matthew Wilcox (Oracle) 写道: > > > - /* For now only order 0 folios are supported for data. */ > > > - ASSERT(folio_order(folio) == 0); > > > > I'd prefer to keep this ASSERT(), as all the btrfs_folio_*() helpers can > > only handle page sized folio for now. I'd rather have the assertions in the places that actually don't work. And the better assertion is !folio_test_large(folio) as it only has to test one bit rather than testing a bit and then also testing a field. > > > - if (folio_index(folio) == end_index && i_size <= end) { > > > + if (folio_contains(folio, i_size >> PAGE_SHIFT) && > > > > Although folio_contains() is already a pretty good improvement, can we > > have a bytenr/i_sized based solution for fs usages? > > Good point, something like folio_contains_offset() that does the correct > shifts. I'd call it folio_contains_pos(), but I'm not sure there's a lot of users. We've got a lot of filesystems converted now, and btrfs is the first one to want something like that. I've had a look through iomap and some other filesystems, and I can't see anywhere else that would use folio_contains_pos().
On Tue, Jan 21, 2025 at 05:59:53PM +0000, Matthew Wilcox wrote: > > > > + if (folio_contains(folio, i_size >> PAGE_SHIFT) && > > > > > > Although folio_contains() is already a pretty good improvement, can we > > > have a bytenr/i_sized based solution for fs usages? > > > > Good point, something like folio_contains_offset() that does the correct > > shifts. > > I'd call it folio_contains_pos(), but I'm not sure there's a lot of > users. We've got a lot of filesystems converted now, and btrfs is the > first one to want something like that. I've had a look through iomap > and some other filesystems, and I can't see anywhere else that would use > folio_contains_pos(). Ok then, we can live with that, but please keep it in mind for future folio API updates. Thanks.
在 2025/1/23 01:54, David Sterba 写道: > On Tue, Jan 21, 2025 at 05:59:53PM +0000, Matthew Wilcox wrote: >>>>> + if (folio_contains(folio, i_size >> PAGE_SHIFT) && >>>> >>>> Although folio_contains() is already a pretty good improvement, can we >>>> have a bytenr/i_sized based solution for fs usages? >>> >>> Good point, something like folio_contains_offset() that does the correct >>> shifts. >> >> I'd call it folio_contains_pos(), but I'm not sure there's a lot of >> users. We've got a lot of filesystems converted now, and btrfs is the >> first one to want something like that. I've had a look through iomap >> and some other filesystems, and I can't see anywhere else that would use >> folio_contains_pos(). > > Ok then, we can live with that, but please keep it in mind for future > folio API updates. Thanks. > Then the whole series looks good to me. Reviewed-by: Qu Wenruo <wqu@suse.com> And since the whole series is reviewed, should I merge this series into for-next now? Thanks, Qu
On Fri, Jan 24, 2025 at 04:41:46PM +1030, Qu Wenruo wrote: > > > 在 2025/1/23 01:54, David Sterba 写道: > > On Tue, Jan 21, 2025 at 05:59:53PM +0000, Matthew Wilcox wrote: > >>>>> + if (folio_contains(folio, i_size >> PAGE_SHIFT) && > >>>> > >>>> Although folio_contains() is already a pretty good improvement, can we > >>>> have a bytenr/i_sized based solution for fs usages? > >>> > >>> Good point, something like folio_contains_offset() that does the correct > >>> shifts. > >> > >> I'd call it folio_contains_pos(), but I'm not sure there's a lot of > >> users. We've got a lot of filesystems converted now, and btrfs is the > >> first one to want something like that. I've had a look through iomap > >> and some other filesystems, and I can't see anywhere else that would use > >> folio_contains_pos(). > > > > Ok then, we can live with that, but please keep it in mind for future > > folio API updates. Thanks. > > > Then the whole series looks good to me. > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > And since the whole series is reviewed, should I merge this series into > for-next now? Yes please.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 289ecb8ce217..c9b0ee841501 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -523,8 +523,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) u64 end; u32 len; - /* For now only order 0 folios are supported for data. */ - ASSERT(folio_order(folio) == 0); btrfs_debug(fs_info, "%s: bi_sector=%llu, err=%d, mirror=%u", __func__, bio->bi_iter.bi_sector, bio->bi_status, @@ -552,7 +550,6 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) if (likely(uptodate)) { loff_t i_size = i_size_read(inode); - pgoff_t end_index = i_size >> folio_shift(folio); /* * Zero out the remaining part if this range straddles @@ -563,7 +560,8 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) * * NOTE: i_size is exclusive while end is inclusive. */ - if (folio_index(folio) == end_index && i_size <= end) { + if (folio_contains(folio, i_size >> PAGE_SHIFT) && + i_size <= end) { u32 zero_start = max(offset_in_folio(folio, i_size), offset_in_folio(folio, start)); u32 zero_len = offset_in_folio(folio, end) + 1 - @@ -956,7 +954,7 @@ static int btrfs_do_readpage(struct folio *folio, struct extent_map **em_cached, return ret; } - if (folio->index == last_byte >> folio_shift(folio)) { + if (folio_contains(folio, last_byte >> PAGE_SHIFT)) { size_t zero_offset = offset_in_folio(folio, last_byte); if (zero_offset) {
It is meaningless to shift a byte count by folio_shift(). The folio index is in units of PAGE_SIZE, not folio_size(). We can use folio_contains() to make this work for arbitrary-order folios, so remove the assertion that the folios are of order 0. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/btrfs/extent_io.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)