| Message ID | 6a71b4597a65114b646032648129558fe6bef38d.1743731232.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: fsstress hang fix for large data folios | expand |
On Fri, Apr 4, 2025 at 2:48 AM Qu Wenruo <wqu@suse.com> wrote: > > Currently we use the following pattern to detect if the folio contains > the end of a file: > > if (folio->index == end_index) > folio_zero_range(); > > But that only works if the folio is page sized. > > For the following case, it will not work and leave the range beyond EOF > uninitialized: > > The page size is 4K, and the fs block size is also 4K. > > 16K 20K 24K > | | | | > | > EOF at 22K > > And we have a large folio sized 8K at file offset 16K. > > In that case, the old "folio->index == end_index" will not work, thus > we the range [22K, 24K) will not be zeroed out. thus we the range -> thus the range > > Fix the following call sites which use the above pattern: > > - add_ra_bio_pages() > > - extent_writepage() > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Filipe Manana <fdmanana@suse.com> Otherwise it looks good, thanks. > --- > fs/btrfs/compression.c | 2 +- > fs/btrfs/extent_io.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > index cb954f9bc332..7aa63681f92a 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -523,7 +523,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, > free_extent_map(em); > unlock_extent(tree, cur, page_end, NULL); > > - if (folio->index == end_index) { > + if (folio_contains(folio, end_index)) { > size_t zero_offset = offset_in_folio(folio, isize); > > if (zero_offset) { > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 013268f70621..f0d51f6ed951 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -221,7 +221,7 @@ static void __process_folios_contig(struct address_space *mapping, > } > > static noinline void unlock_delalloc_folio(const struct inode *inode, > - const struct folio *locked_folio, > + struct folio *locked_folio, > u64 start, u64 end) > { > ASSERT(locked_folio); > @@ -231,7 +231,7 @@ static noinline void unlock_delalloc_folio(const struct inode *inode, > } > > static noinline int lock_delalloc_folios(struct inode *inode, > - const struct folio *locked_folio, > + struct folio *locked_folio, > u64 start, u64 end) > { > struct btrfs_fs_info *fs_info = inode_to_fs_info(inode); > @@ -1711,7 +1711,7 @@ static int extent_writepage(struct folio *folio, struct btrfs_bio_ctrl *bio_ctrl > return 0; > } > > - if (folio->index == end_index) > + if (folio_contains(folio, end_index)) > folio_zero_range(folio, pg_offset, folio_size(folio) - pg_offset); > > /* > -- > 2.49.0 > >
On Fri, Apr 04, 2025 at 12:17:41PM +1030, Qu Wenruo wrote: > Currently we use the following pattern to detect if the folio contains > the end of a file: > > if (folio->index == end_index) > folio_zero_range(); > > But that only works if the folio is page sized. > > For the following case, it will not work and leave the range beyond EOF > uninitialized: > > The page size is 4K, and the fs block size is also 4K. > > 16K 20K 24K > | | | | > | > EOF at 22K > > And we have a large folio sized 8K at file offset 16K. > > In that case, the old "folio->index == end_index" will not work, thus > we the range [22K, 24K) will not be zeroed out. > > Fix the following call sites which use the above pattern: > > - add_ra_bio_pages() > > - extent_writepage() > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/compression.c | 2 +- > fs/btrfs/extent_io.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > index cb954f9bc332..7aa63681f92a 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -523,7 +523,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, > free_extent_map(em); > unlock_extent(tree, cur, page_end, NULL); > > - if (folio->index == end_index) { > + if (folio_contains(folio, end_index)) { > size_t zero_offset = offset_in_folio(folio, isize); > > if (zero_offset) { > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 013268f70621..f0d51f6ed951 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -221,7 +221,7 @@ static void __process_folios_contig(struct address_space *mapping, > } > > static noinline void unlock_delalloc_folio(const struct inode *inode, > - const struct folio *locked_folio, > + struct folio *locked_folio, I'm not happy to see removing const from the parameters as it's quite tedious to find them. Here it's not necessary as it's still not changing the folio, only required because folio API is not const-clean, folio_contains() in particular.
在 2025/4/8 04:09, David Sterba 写道: > On Fri, Apr 04, 2025 at 12:17:41PM +1030, Qu Wenruo wrote: >> Currently we use the following pattern to detect if the folio contains >> the end of a file: >> >> if (folio->index == end_index) >> folio_zero_range(); >> >> But that only works if the folio is page sized. >> >> For the following case, it will not work and leave the range beyond EOF >> uninitialized: >> >> The page size is 4K, and the fs block size is also 4K. >> >> 16K 20K 24K >> | | | | >> | >> EOF at 22K >> >> And we have a large folio sized 8K at file offset 16K. >> >> In that case, the old "folio->index == end_index" will not work, thus >> we the range [22K, 24K) will not be zeroed out. >> >> Fix the following call sites which use the above pattern: >> >> - add_ra_bio_pages() >> >> - extent_writepage() >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/compression.c | 2 +- >> fs/btrfs/extent_io.c | 6 +++--- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c >> index cb954f9bc332..7aa63681f92a 100644 >> --- a/fs/btrfs/compression.c >> +++ b/fs/btrfs/compression.c >> @@ -523,7 +523,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, >> free_extent_map(em); >> unlock_extent(tree, cur, page_end, NULL); >> >> - if (folio->index == end_index) { >> + if (folio_contains(folio, end_index)) { >> size_t zero_offset = offset_in_folio(folio, isize); >> >> if (zero_offset) { >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 013268f70621..f0d51f6ed951 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -221,7 +221,7 @@ static void __process_folios_contig(struct address_space *mapping, >> } >> >> static noinline void unlock_delalloc_folio(const struct inode *inode, >> - const struct folio *locked_folio, >> + struct folio *locked_folio, > > I'm not happy to see removing const from the parameters as it's quite > tedious to find them. Here it's not necessary as it's still not changing > the folio, only required because folio API is not const-clean, > folio_contains() in particular. > Yes, I'm not happy with that either, and I'm planning to constify the parameters for those helpers in a dedicated series. Thanks, Qu
On Tue, Apr 08, 2025 at 07:28:58AM +0930, Qu Wenruo wrote: > 在 2025/4/8 04:09, David Sterba 写道: > > On Fri, Apr 04, 2025 at 12:17:41PM +1030, Qu Wenruo wrote: > >> Currently we use the following pattern to detect if the folio contains > >> the end of a file: > >> > >> if (folio->index == end_index) > >> folio_zero_range(); > >> > >> But that only works if the folio is page sized. > >> > >> For the following case, it will not work and leave the range beyond EOF > >> uninitialized: > >> > >> The page size is 4K, and the fs block size is also 4K. > >> > >> 16K 20K 24K > >> | | | | > >> | > >> EOF at 22K > >> > >> And we have a large folio sized 8K at file offset 16K. > >> > >> In that case, the old "folio->index == end_index" will not work, thus > >> we the range [22K, 24K) will not be zeroed out. > >> > >> Fix the following call sites which use the above pattern: > >> > >> - add_ra_bio_pages() > >> > >> - extent_writepage() > >> > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> fs/btrfs/compression.c | 2 +- > >> fs/btrfs/extent_io.c | 6 +++--- > >> 2 files changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > >> index cb954f9bc332..7aa63681f92a 100644 > >> --- a/fs/btrfs/compression.c > >> +++ b/fs/btrfs/compression.c > >> @@ -523,7 +523,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, > >> free_extent_map(em); > >> unlock_extent(tree, cur, page_end, NULL); > >> > >> - if (folio->index == end_index) { > >> + if (folio_contains(folio, end_index)) { > >> size_t zero_offset = offset_in_folio(folio, isize); > >> > >> if (zero_offset) { > >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > >> index 013268f70621..f0d51f6ed951 100644 > >> --- a/fs/btrfs/extent_io.c > >> +++ b/fs/btrfs/extent_io.c > >> @@ -221,7 +221,7 @@ static void __process_folios_contig(struct address_space *mapping, > >> } > >> > >> static noinline void unlock_delalloc_folio(const struct inode *inode, > >> - const struct folio *locked_folio, > >> + struct folio *locked_folio, > > > > I'm not happy to see removing const from the parameters as it's quite > > tedious to find them. Here it's not necessary as it's still not changing > > the folio, only required because folio API is not const-clean, > > folio_contains() in particular. > > > > Yes, I'm not happy with that either, and I'm planning to constify the > parameters for those helpers in a dedicated series. Thanks, but don't let it distract you from the more important folio changes. I noticed there are missing consts in the page API too, like page_offset() or the folio/page boundary like folio_pos() or folio_pgoff(). This is can wait, my comment was more like a note to self to have a look later.
在 2025/4/9 08:42, David Sterba 写道: > On Tue, Apr 08, 2025 at 07:28:58AM +0930, Qu Wenruo wrote: >> 在 2025/4/8 04:09, David Sterba 写道: >>> On Fri, Apr 04, 2025 at 12:17:41PM +1030, Qu Wenruo wrote: >>>> Currently we use the following pattern to detect if the folio contains >>>> the end of a file: >>>> >>>> if (folio->index == end_index) >>>> folio_zero_range(); >>>> >>>> But that only works if the folio is page sized. >>>> >>>> For the following case, it will not work and leave the range beyond EOF >>>> uninitialized: >>>> >>>> The page size is 4K, and the fs block size is also 4K. >>>> >>>> 16K 20K 24K >>>> | | | | >>>> | >>>> EOF at 22K >>>> >>>> And we have a large folio sized 8K at file offset 16K. >>>> >>>> In that case, the old "folio->index == end_index" will not work, thus >>>> we the range [22K, 24K) will not be zeroed out. >>>> >>>> Fix the following call sites which use the above pattern: >>>> >>>> - add_ra_bio_pages() >>>> >>>> - extent_writepage() >>>> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> --- >>>> fs/btrfs/compression.c | 2 +- >>>> fs/btrfs/extent_io.c | 6 +++--- >>>> 2 files changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c >>>> index cb954f9bc332..7aa63681f92a 100644 >>>> --- a/fs/btrfs/compression.c >>>> +++ b/fs/btrfs/compression.c >>>> @@ -523,7 +523,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, >>>> free_extent_map(em); >>>> unlock_extent(tree, cur, page_end, NULL); >>>> >>>> - if (folio->index == end_index) { >>>> + if (folio_contains(folio, end_index)) { >>>> size_t zero_offset = offset_in_folio(folio, isize); >>>> >>>> if (zero_offset) { >>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>>> index 013268f70621..f0d51f6ed951 100644 >>>> --- a/fs/btrfs/extent_io.c >>>> +++ b/fs/btrfs/extent_io.c >>>> @@ -221,7 +221,7 @@ static void __process_folios_contig(struct address_space *mapping, >>>> } >>>> >>>> static noinline void unlock_delalloc_folio(const struct inode *inode, >>>> - const struct folio *locked_folio, >>>> + struct folio *locked_folio, >>> >>> I'm not happy to see removing const from the parameters as it's quite >>> tedious to find them. Here it's not necessary as it's still not changing >>> the folio, only required because folio API is not const-clean, >>> folio_contains() in particular. >>> >> >> Yes, I'm not happy with that either, and I'm planning to constify the >> parameters for those helpers in a dedicated series. > > Thanks, but don't let it distract you from the more important folio > changes. I noticed there are missing consts in the page API too, like > page_offset() or the folio/page boundary like folio_pos() or > folio_pgoff(). This is can wait, my comment was more like a note to self > to have a look later. No worry, as the large folios work is done (at least on x86_64, no new regression for the whole fstests run). Now I only need to wait for the remaining 4 patches before the final enablement. Sure there are still something left for large folios, e.g. data reloc inode is still using page sized folios, but that is not urgent either. So the constification of the MM interfaces can be a good low hanging fruit here. Thanks, Qu
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index cb954f9bc332..7aa63681f92a 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -523,7 +523,7 @@ static noinline int add_ra_bio_pages(struct inode *inode, free_extent_map(em); unlock_extent(tree, cur, page_end, NULL); - if (folio->index == end_index) { + if (folio_contains(folio, end_index)) { size_t zero_offset = offset_in_folio(folio, isize); if (zero_offset) { diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 013268f70621..f0d51f6ed951 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -221,7 +221,7 @@ static void __process_folios_contig(struct address_space *mapping, } static noinline void unlock_delalloc_folio(const struct inode *inode, - const struct folio *locked_folio, + struct folio *locked_folio, u64 start, u64 end) { ASSERT(locked_folio); @@ -231,7 +231,7 @@ static noinline void unlock_delalloc_folio(const struct inode *inode, } static noinline int lock_delalloc_folios(struct inode *inode, - const struct folio *locked_folio, + struct folio *locked_folio, u64 start, u64 end) { struct btrfs_fs_info *fs_info = inode_to_fs_info(inode); @@ -1711,7 +1711,7 @@ static int extent_writepage(struct folio *folio, struct btrfs_bio_ctrl *bio_ctrl return 0; } - if (folio->index == end_index) + if (folio_contains(folio, end_index)) folio_zero_range(folio, pg_offset, folio_size(folio) - pg_offset); /*
Currently we use the following pattern to detect if the folio contains the end of a file: if (folio->index == end_index) folio_zero_range(); But that only works if the folio is page sized. For the following case, it will not work and leave the range beyond EOF uninitialized: The page size is 4K, and the fs block size is also 4K. 16K 20K 24K | | | | | EOF at 22K And we have a large folio sized 8K at file offset 16K. In that case, the old "folio->index == end_index" will not work, thus we the range [22K, 24K) will not be zeroed out. Fix the following call sites which use the above pattern: - add_ra_bio_pages() - extent_writepage() Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/compression.c | 2 +- fs/btrfs/extent_io.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)