MIME-Version: 1.0
References: <20230220134308.1193219-1-dwysocha@redhat.com> <CAPt2mGPJxPWfFGtEacBw-AN5nMZfP_pvL6=wEM+QbrPf1brAFg@mail.gmail.com>
 <CALF+zOmcROD6tzSCixYQN-+hw8bpboTrF-Ff-hEZOMDAPe7BOA@mail.gmail.com> <CAPt2mGMCzGdqgrF6e7hNAzJiz55wP3PkPCJvaZkNtcVW9Ydnvg@mail.gmail.com>
In-Reply-To: <CAPt2mGMCzGdqgrF6e7hNAzJiz55wP3PkPCJvaZkNtcVW9Ydnvg@mail.gmail.com>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Thu, 9 Mar 2023 18:50:58 -0500
Message-ID: <CALF+zO=ZH3vt17653OC=x-ED4HbCKrdJ5QYxC6GmiQa=NW_VWA@mail.gmail.com>
Subject: Re: [PATCH v11 0/5] Convert NFS with fscache to the netfs API
To:     Daire Byrne <daire@dneg.com>
Cc:     Anna Schumaker <anna.schumaker@netapp.com>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        David Howells <dhowells@redhat.com>,
        Benjamin Maynard <benmaynard@google.com>,
        Daire Byrne <daire.byrne@gmail.com>, linux-nfs@vger.kernel.org,
        linux-cachefs@redhat.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

I posted a patch which should fix the problem.  It applied cleanly
to 6.3-rc1 as well as this series.

I think netfs should not be counting in the unlock path
because the PID won't be correct.  Fixing netfs should
be a separate patch though, orthogonal to this v11 series.

On Thu, Feb 23, 2023 at 10:47=E2=80=AFAM Daire Byrne <daire@dneg.com> wrote=
:
>
> Oh, I didn't realise that was included.
>
> I did a quick test doing a dd off a NFS mount but I did not see the
> read_bytes increment - rchar yes, read_bytes no (as before). This was
> an NFSv3 mount.
>
> Daire
>
> On Wed, 22 Feb 2023 at 15:57, David Wysochanski <dwysocha@redhat.com> wro=
te:
> >
> > Thanks Daire!  Did this also resolve the issue with
> > /proc/PID/io/read_bytes you reported (link below)
> > since netfs should increment that count now?
> >
> > https://lore.kernel.org/linux-nfs/CAPt2mGNEYUk5u8V4abe=3D5MM5msZqmvzCVr=
tCP4Qw1n=3DgCHCnww@mail.gmail.com/
> >
> >
> >
> > On Wed, Feb 22, 2023 at 7:51 AM Daire Byrne <daire@dneg.com> wrote:
> > >
> > > Dave,
> > >
> > > Thanks for this! I have been testing it with our production (render
> > > farm) loads for the last couple of days and have not run into any
> > > issues so far. It seems to be performing on par with your previous
> > > version of the patchset (based on v6.0).
> > >
> > > I am also running with both the "known issues" dhowells patches [8] &
> > > [9] mentioned in your email (as I was with your previous version).
> > >
> > > Tested-by: Daire Byrne <daire@dneg.com>
> > >
> > >
> > >
> > > On Mon, 20 Feb 2023 at 13:44, Dave Wysochanski <dwysocha@redhat.com> =
wrote:
> > > >
> > > > Trond, this v11 patchset addresses your latest feedback on patch #2=
,
> > > > and I did a little bit of cleanup to patch 3 (see Changes notes bel=
ow).
> > > > I'm not sure of further changes to patch #3 without a more in-depth
> > > > review with specifics, if you feel the current approach is unaccept=
able [1].
> > > >
> > > > Ben and Daire, if you could test this set and provide you feedback
> > > > and a Tested-By: that would be appreciated.  This set addresses
> > > > the existing NFS + fscache performance concerns seen by a few
> > > > users [5], which is due to utilization use of the deprecated
> > > > single-page function, fscache_fallback_read_page().  However,
> > > > until "known issue #1" below is also resolved, even with these
> > > > patches, performance of NFS+fscache will still be a problem
> > > > in some scenarios.
> > > >
> > > > This patchset converts NFS with fscache buffered read IO paths to
> > > > use the netfs API with a non-invasive approach.  The existing NFS p=
gio
> > > > layer does not need extensive changes, and is the best way so far I=
've
> > > > found to address Trond's previous concerns about modifying the IO
> > > > path [2] as well as only enabling netfs when fscache is configured
> > > > and enabled [3].  I have not attempted performance comparisions to
> > > > address Chuck Lever's concern [4] because we are not converting the
> > > > non-fscache enabled NFS IO paths to netfs.
> > > >
> > > > The patchset is based on Trond's latest 'testing' branch which incl=
udes
> > > > his folio patchset, and is based on 6.2-rc5.  It has been pushed to
> > > > github at:
> > > > https://github.com/DaveWysochanskiRH/kernel/commits/nfs-fscache-net=
fs
> > > > https://github.com/DaveWysochanskiRH/kernel/commit/6424e4f139652b75=
52eff26eb5da1f2282d35616
> > > >
> > > > Changes since v10 [6]
> > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > > PATCH6: Dropped
> > > > PATCH1: Rename nfs_pageio_add_page to nfs_read_add_folio
> > > > PATCH2: Use anonymous union to add struct netfs_inode to nfs_inode =
(Trond) [7]
> > > > PATCH3: Change nfs_netfs_readpage_release() to nfs_netfs_folio_unlo=
ck()
> > > >
> > > > Testing
> > > > =3D=3D=3D=3D=3D=3D=3D
> > > > I did a full round of testing on this because it was rebased on top=
 of
> > > > Trond's testing branch which included his folio series.
> > > > All of my unit tests pass except the one with the known issue #1 be=
low.
> > > > Multiple runs of xfstests generic tests (applicable to NFS) were ru=
n with
> > > > with various servers, both with and without fscache enabled, and
> > > > compared to baseline (Trond's testing branch).  No new failures wer=
e
> > > > observed with these patches, and in some xfstest instances, this
> > > > patchset improves the results (some tests that were failing now pas=
s).
> > > > - hammerspace(pNFS flexfiles): NFS4.1, NFS4.2
> > > > - NetApp(pNFS filelayout): NFS4.1, NFS4.0, NFS3
> > > > - RHEL9: NFS4.2, NFS4.1, NFS4.0, NFS3
> > > >
> > > > Known issues
> > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > > 1. Unit test setting rsize < readahead does not properly read from
> > > > fscache but re-reads data from the NFS server
> > > > * This will be fixed with another dhowells patch [8]:
> > > >   "[PATCH v6 2/2] mm, netfs, fscache: Stop read optimisation when f=
olio removed from pagecache"
> > > > * Daire Byrne verified the patch fixes his issue as well
> > > >
> > > > 2. "Cache volume key already in use" after xfstest runs involving m=
ultiple mounts
> > > > * Simple reproducer requires just two mounts as follows:
> > > >  mount -overs=3D4.1,fsc,nosharecache -o context=3Dsystem_u:object_r=
:root_t:s0  nfs-server:/exp1 /mnt1
> > > >  mount -overs=3D4.1,fsc,nosharecache -o context=3Dsystem_u:object_r=
:root_t:s0  nfs-server:/exp2 /mnt2
> > > > * This should be fixed with dhowells patch [9]:
> > > >   "[PATCH v5] vfs, security: Fix automount superblock LSM init prob=
lem, preventing NFS sb sharing"
> > > >
> > > >
> > > > References
> > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > > [1] https://lore.kernel.org/linux-nfs/0676ecb2bb708e6fc29dbbe6b4455=
1d6a0d021dc.camel@kernel.org/
> > > > [2] https://lore.kernel.org/linux-nfs/9cfd5bc3cfc6abc2d3316b0387222=
e708d67f595.camel@hammerspace.com/
> > > > [3] https://lore.kernel.org/linux-nfs/da9200f1bded9b8b078a7aef227fd=
6b92eb028fb.camel@hammerspace.com/
> > > > [4] https://lore.kernel.org/linux-nfs/0A640C47-5F51-47E8-864D-E0E98=
0F8B310@oracle.com/
> > > > [5] https://lore.kernel.org/linux-nfs/CA+QRt4tPqH87NVkoETLjxieGjZ_f=
7XxRj+xS3NVxcJ+b8AAKQg@mail.gmail.com/
> > > > [6] https://lore.kernel.org/linux-nfs/20221103161637.1725471-1-dwys=
ocha@redhat.com/
> > > > [7] https://lore.kernel.org/linux-nfs/4d60636f62df4f5c200666ed2d1a5=
f2414c18e1f.camel@kernel.org/
> > > > [8] https://lore.kernel.org/linux-nfs/20230216150701.3654894-1-dhow=
ells@redhat.com/T/#mf3807fa68fb6d495b87dde0d76b5237833a0cc81
> > > > [9] https://lore.kernel.org/linux-kernel/217595.1662033775@warthog.=
procyon.org.uk/
> > > >
> > > > Dave Wysochanski (5):
> > > >   NFS: Rename readpage_async_filler to nfs_read_add_folio
> > > >   NFS: Configure support for netfs when NFS fscache is configured
> > > >   NFS: Convert buffered read paths to use netfs when fscache is ena=
bled
> > > >   NFS: Remove all NFSIOS_FSCACHE counters due to conversion to netf=
s API
> > > >   NFS: Remove fscache specific trace points and NFS_INO_FSCACHE bit
> > > >
> > > >  fs/nfs/Kconfig             |   1 +
> > > >  fs/nfs/fscache.c           | 242 ++++++++++++++++++++++-----------=
----
> > > >  fs/nfs/fscache.h           | 131 ++++++++++++++------
> > > >  fs/nfs/inode.c             |   2 +
> > > >  fs/nfs/internal.h          |   9 ++
> > > >  fs/nfs/iostat.h            |  17 ---
> > > >  fs/nfs/nfstrace.h          |  91 --------------
> > > >  fs/nfs/pagelist.c          |   4 +
> > > >  fs/nfs/read.c              | 105 ++++++++--------
> > > >  fs/nfs/super.c             |  11 --
> > > >  include/linux/nfs_fs.h     |  25 ++--
> > > >  include/linux/nfs_iostat.h |  12 --
> > > >  include/linux/nfs_page.h   |   3 +
> > > >  include/linux/nfs_xdr.h    |   3 +
> > > >  14 files changed, 317 insertions(+), 339 deletions(-)
> > > >
> > > > --
> > > > 2.31.1
> > > >
> > >
> >
>