Re: [PATCH v2 1/3] tmpfs: zero post-eof uptodate folios on swapout

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2025/11/19 22:08, Brian Foster wrote:
> On Wed, Nov 19, 2025 at 11:53:41AM +0800, Baolin Wang wrote:
>>
>>
>> On 2025/11/18 22:39, Brian Foster wrote:
>>> On Tue, Nov 18, 2025 at 10:33:44AM +0800, Baolin Wang wrote:
>>>>
>>>>
>>>> On 2025/11/13 00:25, Brian Foster wrote:
>>>>> As a first step to facilitate efficient post-eof zeroing in tmpfs,
>>>>> zero post-eof uptodate folios at swap out time. This ensures that
>>>>> post-eof ranges are zeroed "on disk" (i.e. analogous to traditional
>>>>> pagecache writeback) and facilitates zeroing on file size changes by
>>>>> allowing it to not have to swap in.
>>>>>
>>>>> Note that shmem_writeout() already zeroes !uptodate folios so this
>>>>> introduces some duplicate logic. We'll clean this up in the next
>>>>> patch.
>>>>>
>>>>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>>>>> ---
>>>>>     mm/shmem.c | 19 +++++++++++++++++--
>>>>>     1 file changed, 17 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>>> index 0a25ee095b86..5fb3c911894f 100644
>>>>> --- a/mm/shmem.c
>>>>> +++ b/mm/shmem.c
>>>>> @@ -1577,6 +1577,8 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
>>>>>     	struct inode *inode = mapping->host;
>>>>>     	struct shmem_inode_info *info = SHMEM_I(inode);
>>>>>     	struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
>>>>> +	loff_t i_size = i_size_read(inode);
>>>>> +	pgoff_t end_index = DIV_ROUND_UP(i_size, PAGE_SIZE);
>>>>>     	pgoff_t index;
>>>>>     	int nr_pages;
>>>>>     	bool split = false;
>>>>> @@ -1596,8 +1598,7 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
>>>>>     	 * (unless fallocate has been used to preallocate beyond EOF).
>>>>>     	 */
>>>>>     	if (folio_test_large(folio)) {
>>>>> -		index = shmem_fallocend(inode,
>>>>> -			DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE));
>>>>> +		index = shmem_fallocend(inode, end_index);
>>>>>     		if ((index > folio->index && index < folio_next_index(folio)) ||
>>>>>     		    !IS_ENABLED(CONFIG_THP_SWAP))
>>>>>     			split = true;
>>>>> @@ -1647,6 +1648,20 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
>>>>>     		folio_mark_uptodate(folio);
>>>>>     	}
>>>>> +	/*
>>>>> +	 * Ranges beyond EOF must be zeroed at writeout time. This mirrors
>>>>> +	 * traditional writeback behavior and facilitates zeroing on file size
>>>>> +	 * changes without having to swap back in.
>>>>> +	 */
>>>>> +	if (folio_next_index(folio) >= end_index) {
>>>>> +		size_t from = offset_in_folio(folio, i_size);
>>>>> +
>>>>> +		if (index >= end_index) {
>>>>> +			folio_zero_segment(folio, 0, folio_size(folio));
>>>>> +		} else if (from)
>>>>> +			folio_zero_segment(folio, from, folio_size(folio));
>>>>> +	}
>>>>
>>>> As I mentioned before[1], if a large folio is beyond EOF, it will be split
>>>> in shmem_writeout(), and those small folios beyond EOF will be dropped and
>>>> freed in __folio_split(). Of course, there's another special case as Hugh
>>>> mentioned: when there's a 'fallocend' beyond i_size (e.g., fallocate()), it
>>>> will keep the pages allocated beyond EOF after the split. However, your
>>>> 'end_index' here does not consider 'fallocend,' so it seems to me that this
>>>> portion of the code doesn't actually take effect.
>>>>
>>>
>>> Hi Boalin,
>>
>> s/Boalin/Baolin :)
>>
> 
> Sorry, Baolin! ;)
> 
>>>
>>> So I get that split post-eof folios can fall off depending on fallocate
>>> status. I'm not sure what you mean by considering fallocend, however.
>>> ISTM that fallocend contributes to the boundary where we decide to split
>>> and/or preserve, but i_size is what is relevant for zeroing. It's not
>>> clear to me if you're suggesting the logic is potentially spurious, or
>>> this might not actually be zeroing correctly due to falloc interactions.
>>> Can you clarify the concern please? Thanks.
>>
>> Sorry for not being clear enough (for my quick response yesterday). After
>> thinking more, I want to divide this into 3 cases to clearly explain the
>> logic here:
>>
> 
> No worries. Thanks for breaking it down. Much easier to discuss this
> way.
> 
>> 1. Without fallocate(), if a large folio is beyond EOF (i.e. i_size), it
>> will be split in shmem_writeout(), and those small folios beyond EOF will be
>> dropped and freed in __folio_split(). So, your changes should also have no
>> impact, because after the split, ‘folio_next_index(folio)’ is definitely <=
>> ‘end_index’. So the logic is correct.
>>
> 
> Ack, but we still want to zero any post-eof portion of a small folio
> straddling i_size...

Yes.

>> 2. With fallocate(), If a large folio is beyond EOF (i.e. i_size) but
>> smaller than 'fallocend', the folio will not be split. So, we should zero
>> the post-EOF part. Because 'index' (now representing 'fallocend') is greater
>> than 'end_index', you are zeroing the entire large folio, which does not
>> seem correct to me.
>>
> 
> Unless CONFIG_THP_SWAP is disabled (no idea how likely that is, seems
> like an arch thing), in which case it seems we always split (and then
> the split path will still use fallocend to determine whether to toss or
> preserve).
> 
> But otherwise yes, partial zeroing should be the same for the large
> folio across EOF case, it just happens to be a larger folio size...
> 
>> if (index >= end_index) {
>> 	folio_zero_segment(folio, 0, folio_size(folio));
>> } else if ...
>>
>> I think you should only zero the range from 'from' to 'folio_size(folio)' of
>> this large folio in this case. Right?
>>
> 
> However index is folio->index here, not fallocend. index is reassigned a
> bit further down the function just after the block try_split: lands in:
> 
>          index = folio->index;
>          nr_pages = folio_nr_pages(folio);
> 
> This wasn't introduced by this patch, FWIW, but I suppose we could make
> the fallocend block use a local for clarity.

Thanks for correcting me. I mistakenly assumed that the 'index' 
represents 'fallocend' from your patch:

+		index = shmem_fallocend(inode, end_index);

> So given that, the logic is effectively if the folio starts at or beyond
> the first page size index beyond EOF, zero the whole thing. That seems
> pretty straightforward to me, so I'm not clear on why we'd need to
> consider whether the folio is large or not at this point.

Yes, you are right. After you corrected me, I understand that index 
refers to folio->index.

>> 3. With fallocate(), If a large folio is beyond EOF (i.e. i_size) and also
>> beyond 'fallocend', the large folio will be split to small folios. If we
>> continue attempting to write out these small folios beyond EOF, we need to
>> zero the entire mall folios at this point. So, the logic looks correct
>> (because 'index' > 'end_index').
>>
> 
> Ack..
> 
>> Based on the above analysis, I believe the logic should be:
>>
>> if (folio_next_index(folio) >= end_index) {
>> 	size_t from = offset_in_folio(folio, i_size);
>>
>> 	if (!folio_test_large(folio) && index >= end_index)
>> 		folio_zero_segment(folio, 0, folio_size(folio));
>> 	else if (from)
>> 		folio_zero_segment(folio, from, folio_size(folio));
>> }
>>
>> The logic here is a bit complex, please correct me if I misunderstood you.
>>
> 
> Hmm.. so I'm not really sure about the large folio check. Have you
> reviewed the next patch, by chance? It occurs to me that I probably
> split these two up wrongly. I probably should have split off the
> existing !uptodate zeroing into a separate hunk in patch 1 (i.e. as a
> non functional change, refactoring patch) and then introduce the
> functional change in patch 2. I'll try that for v3.
> 
> But in the meantime, this is the logic after patch 2:
> 
> 	/*
> 	 * Ranges beyond EOF must be zeroed at writeout time. This mirrors
> 	 * traditional writeback behavior and facilitates zeroing on file size
> 	 * changes without having to swap back in.
> 	 */
> 	if (!folio_test_uptodate(folio) ||
> 	    folio_next_index(folio) >= end_index) {
> 		size_t from = offset_in_folio(folio, i_size);
> 
> 		if (!folio_test_uptodate(folio) || index >= end_index) {
> 			folio_zero_segment(folio, 0, folio_size(folio));
> 			flush_dcache_folio(folio);
> 			folio_mark_uptodate(folio);
> 		} else if (from)
> 			folio_zero_segment(folio, from, folio_size(folio));
> 	}
> 
> So factoring out the preexisting uptodate logic, this looks mostly
> equivalent to what you posted above with the exception of the large
> folio check. I could be wrong, but it kind of sounds like that is maybe
> due to confusion over the index value.. hm?

Right. My example is wrong due to confusion over the index value. I 
think you are right.

> 
> In any event, I'm trying to make this logic as simple and clear as
> possible. The idea here is basically that any folio being written out
> that is either !uptodate or at least partially beyond EOF needs zeroing.
> 
> The split logic earlier in the function simply dictates what folios make
> it to this point (to be written out vs. tossed) or not, and otherwise we
> don't really have to care about state wrt zeroing post-eof folio ranges
> (particularly if any of that splitting logic happens to change in the
> future). Let me know if I'm missing something. Thanks again.

Thanks for your work and explanation. Now I think your patch is correct, 
and I will continue to review the following patches. Feel free to add:

Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

One nit: using 'fallocend' instead of 'index' can avoid confusion:)

diff --git a/mm/shmem.c b/mm/shmem.c
index 371af9e322d5..7f7bdb7944cc 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1586,8 +1586,8 @@ int shmem_writeout(struct folio *folio, struct 
swap_iocb **plug,
          * (unless fallocate has been used to preallocate beyond EOF).
          */
         if (folio_test_large(folio)) {
-               index = shmem_fallocend(inode, end_index);
-               if ((index > folio->index && index < 
folio_next_index(folio)) ||
+               pgoff_t fallocend = shmem_fallocend(inode, end_index);
+               if ((fallocend > folio->index && fallocend < 
folio_next_index(folio)) ||
                     !IS_ENABLED(CONFIG_THP_SWAP))
                         split = true;
         }