Re: [PATCH] nfsd: Fix reading via splice

[Chuck Lever <chuck.lever@oracle.com>]

On Sun, Jul 30, 2023 at 09:50:44AM -0700, Hugh Dickins wrote:
> On Sun, 30 Jul 2023, Chuck Lever wrote:
> > On Sat, Jul 29, 2023 at 09:54:58AM +1000, NeilBrown wrote:
> > > On Fri, 28 Jul 2023, Chuck Lever wrote:
> > > > From: David Howells <dhowells@redhat.com>
> ...
> > - This fix is destined for 6.5-rc, which limits the amount of
> >   clean up and optimization we should be doing
> > 
> > I'd like to apply David's fix as-is, unless it's truly broken or
> > someone has a better quick solution.
> 
> I certainly have no objection to you doing so; and think that you
> and David will have a much better appreciation of the risks than me.
> 
> But I ought to mention that this two-ZERO_PAGEs-in-a-row behaviour
> was problematic for splice() in the past - see the comments on
> ZERO_PAGE(0) and its alternative block in shmem_file_read_iter().
> 1bdec44b1eee ("tmpfs: fix regressions from wider use of ZERO_PAGE"):
> ah, that came from a report by you too, xfstests on nfsd.

Yes, I thought we had visited this ZERO_PAGE approach before, but
couldn't put my finger on exactly when or where.


> In principle there's a very simple (but inferior) solution at the
> shmem end: for shmem_file_splice_read() to use SGP_CACHE (used when
> faulting in a hole) instead of SGP_READ in its call to shmem_get_folio().
> (And delete all of shmem's splice_zeropage_into_pipe() code.)
> 
> I say "in principle" because all David's testing has been with the
> SGP_READ there, and perhaps there's some gotcha I'm overlooking which
> would turn up when switching over to SGP_CACHE.  And I say "inferior"
> because that way entails allocating and zeroing pages for holes (which
> page reclaim will then free later on if they remain clean).
> 
> My vote would be for putting David's nfsd patch in for now, but
> keeping an open mind as to whether the shmem end has to change,
> if there might be further problems elsewhere than nfsd.

I'm open to that.

-- 
Chuck Lever