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

[Qu Wenruo <wqu@suse.com>]

在 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