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;
Message-ID: <870684b35a45b94c426554a62b63f80f421dbb08.camel@kernel.org>
Subject: Re: [PATCH v9 3/5] NFS: Convert buffered read paths to use netfs
 when fscache is enabled
From:   Trond Myklebust <trondmy@kernel.org>
To:     Dave Wysochanski <dwysocha@redhat.com>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        David Howells <dhowells@redhat.com>
Cc:     linux-nfs@vger.kernel.org, linux-cachefs@redhat.com,
        Benjamin Maynard <benmaynard@google.com>,
        Daire Byrne <daire.byrne@gmail.com>
Date:   Thu, 27 Oct 2022 15:16:03 -0400
In-Reply-To: <20221017105212.77588-4-dwysocha@redhat.com>
References: <20221017105212.77588-1-dwysocha@redhat.com>
         <20221017105212.77588-4-dwysocha@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-10-17 at 06:52 -0400, Dave Wysochanski wrote:
> Convert the NFS buffered read code paths to corresponding netfs APIs,
> but only when fscache is configured and enabled.
> 
> The netfs API defines struct netfs_request_ops which must be filled
> in by the network filesystem.  For NFS, we only need to define 5 of
> the functions, the main one being the issue_read() function.
> The issue_read() function is called by the netfs layer when a read
> cannot be fulfilled locally, and must be sent to the server (either
> the cache is not active, or it is active but the data is not
> available).
> Once the read from the server is complete, netfs requires a call to
> netfs_subreq_terminated() which conveys either how many bytes were
> read
> successfully, or an error.  Note that issue_read() is called with a
> structure, netfs_io_subrequest, which defines the IO requested, and
> contains a start and a length (both in bytes), and assumes the
> underlying
> netfs will return a either an error on the whole region, or the
> number
> of bytes successfully read.
> 
> The NFS IO path is page based and the main APIs are the pgio APIs
> defined
> in pagelist.c.  For the pgio APIs, there is no way for the caller to
> know how many RPCs will be sent and how the pages will be broken up
> into underlying RPCs, each of which will have their own completion
> and
> return code.  In contrast, netfs is subrequest based, a single
> subrequest may contain multiple pages, and a single subrequest is
> initiated with issue_read() and terminated with
> netfs_subreq_terminated().
> Thus, to utilze the netfs APIs, NFS needs some way to accommodate
> the netfs API requirement on the single response to the whole
> subrequest, while also minimizing disruptive changes to the NFS
> pgio layer.
> 
> The approach taken with this patch is to allocate a small structure
> for each nfs_netfs_issue_read() call, store the final error and
> number
> of bytes successfully transferred in the structure, and update these
> values
> as each RPC completes.  The refcount on the structure is used as a
> marker
> for the last RPC completion, is incremented in
> nfs_netfs_read_initiate(),
> and decremented inside nfs_netfs_read_completion(), when a
> nfs_pgio_header
> contains a valid pointer to the data.  On the final put (which
> signals
> the final outstanding RPC is complete) in
> nfs_netfs_read_completion(),
> call netfs_subreq_terminated() with either the final error value (if
> one or more READs complete with an error) or the number of bytes
> successfully transferred (if all RPCs complete successfully).  Note
> that when all RPCs complete successfully, the number of bytes
> transferred
> is capped to the length of the subrequest.  Capping the transferred
> length
> to the subrequest length prevents "Subreq overread" warnings from
> netfs.
> This is due to the "aligned_len" in nfs_pageio_add_page(), and the
> corner case where NFS requests a full page at the end of the file,
> even when i_size reflects only a partial page (NFS overread).
> 
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>


This is not doing what I asked for, which was to separate out the
fscache functionality, so that we can call that if and when it is
available.

Instead, it is just wrapping the NFS requests inside netfs requests. As
it stands, that means it is just duplicating information, and adding
unnecessary overhead to the standard I/O path (extra allocations, extra
indirect calls, and extra bloat to the inode).

My expectation is that the standard I/O path should have minimal
overhead, and should certainly not increase the overhead that we
already have. Will this be addressed in future iterations of these
patches?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com