Re: [PATCH 2/3] btrfs: use folio_contains() for EOF detection

[David Sterba <dsterba@suse.cz>]

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.