Re: [PATCH v2 2/3] tmpfs: combine !uptodate and post-eof zeroing logic at swapout

[Brian Foster <bfoster@redhat.com>]

On Thu, Nov 20, 2025 at 10:56:41AM +0800, Baolin Wang wrote:
> 
> 
> On 2025/11/13 00:25, Brian Foster wrote:
> > shmem_writeout() zeroes folios that are !uptodate (before marking
> > them uptodate) or that extend beyond EOF to preserve data integrity
> > according to POSIX. This is handled in a couple different blocks.
> > Fold the !uptodate zeroing into the post-eof block so we zero from
> > one place.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >   mm/shmem.c | 40 +++++++++++++++++++---------------------
> >   1 file changed, 19 insertions(+), 21 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 5fb3c911894f..7925ced8a05d 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1627,25 +1627,20 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
> >   	 * good idea to continue anyway, once we're pushing into swap.  So
> >   	 * reactivate the folio, and let shmem_fallocate() quit when too many.
> >   	 */
> > -	if (!folio_test_uptodate(folio)) {
> > -		if (inode->i_private) {
> > -			struct shmem_falloc *shmem_falloc;
> > -			spin_lock(&inode->i_lock);
> > -			shmem_falloc = inode->i_private;
> > -			if (shmem_falloc &&
> > -			    !shmem_falloc->waitq &&
> > -			    index >= shmem_falloc->start &&
> > -			    index < shmem_falloc->next)
> > -				shmem_falloc->nr_unswapped += nr_pages;
> > -			else
> > -				shmem_falloc = NULL;
> > -			spin_unlock(&inode->i_lock);
> > -			if (shmem_falloc)
> > -				goto redirty;
> > -		}
> > -		folio_zero_range(folio, 0, folio_size(folio));
> > -		flush_dcache_folio(folio);
> > -		folio_mark_uptodate(folio);
> > +	if (!folio_test_uptodate(folio) && inode->i_private) {
> > +		struct shmem_falloc *shmem_falloc;
> > +		spin_lock(&inode->i_lock);
> > +		shmem_falloc = inode->i_private;
> > +		if (shmem_falloc &&
> > +		    !shmem_falloc->waitq &&
> > +		    index >= shmem_falloc->start &&
> > +		    index < shmem_falloc->next)
> > +			shmem_falloc->nr_unswapped += nr_pages;
> > +		else
> > +			shmem_falloc = NULL;
> > +		spin_unlock(&inode->i_lock);
> > +		if (shmem_falloc)
> > +			goto redirty;
> >   	}
> >   	/*
> > @@ -1653,11 +1648,14 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
> >   	 * traditional writeback behavior and facilitates zeroing on file size
> >   	 * changes without having to swap back in.
> >   	 */
> > -	if (folio_next_index(folio) >= end_index) {
> > +	if (!folio_test_uptodate(folio) ||
> > +	    folio_next_index(folio) >= end_index) {
> >   		size_t from = offset_in_folio(folio, i_size);
> > -		if (index >= end_index) {
> > +		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));
> >   	}
> 
> Overall, this looks correct to me. However, I have some concerns about this
> cleanup, as it involves handling different logic for !uptodate folios of the
> fallocate() and EOF zeroing. I'm not sure if combining them together makes
> the code more readable, since, as discussed in patch 1, there are multiple
> scenarios to consider for EOF zeroing. Let's see how Hugh views this
> cleanup.
> 

Yeah, fair. FWIW I played around with reordering this a bit yesterday
and made another cleanup so the end result now looks like this:

	/*
	 * 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));
		else if (from)
			folio_zero_segment(folio, from, folio_size(folio));

		flush_dcache_folio(folio);
		folio_mark_uptodate(folio);
	}

I find it a little cleaner personally, but that is still clearly
condensing some of the logic between the two cases, so certainly
debatable.

I'm going to try to spin around a v3 quicker than anticipated to fix up
the wonky ordering thing, since I think the way I did it here adds
unnecessary confusion. Hopefully I can get to that before Hugh digs into
this one..

Brian