Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 30 Jul 2023 09:50:44 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Chuck Lever <chuck.lever@oracle.com>
cc:     NeilBrown <neilb@suse.de>, Chuck Lever <cel@kernel.org>,
        David Howells <dhowells@redhat.com>,
        Jeff Layton <jlayton@kernel.org>,
        Hugh Dickins <hughd@google.com>, Jens Axboe <axboe@kernel.dk>,
        Matthew Wilcox <willy@infradead.org>,
        linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH] nfsd: Fix reading via splice
In-Reply-To: <ZMaB1BwBfNko1ZoE@tissot.1015granger.net>
Message-ID: <3082a8da-4a13-de28-ed50-8aa2e7a59afd@google.com>
References: <169054754615.3783.11682801287165281930.stgit@klimt.1015granger.net> <169058849828.32308.14965537137761913794@noble.neil.brown.name> <ZMaB1BwBfNko1ZoE@tissot.1015granger.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.

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.

Hugh