On Mon, 2024-06-24 at 11:41 -0400, Olga Kornievskaia wrote:
> On Mon, Jun 24, 2024 at 11:17 AM Trond Myklebust
> <trondmy@xxxxxxxxxxxxxxx> wrote:
> >
> > Hi Olga,
> >
> > On Mon, 2024-06-24 at 09:28 -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@xxxxxxxxxx>
> > >
> > > Previously in order to mark the communication with the DS server,
> > > we tried to use NFS_CS_DS in cl_flags. However, this flag would
> > > only be saved for the DS server and in case where DS equals MDS,
> > > the client would not find a matching nfs_client in
> > > nfs_match_client
> > > that represents the MDS (but is also a DS).
> > >
> > > Instead, don't rely on the NFS_CS_DS but instead use NFS_CS_PNFS.
> > >
> > > Fixes: 379e4adfddd6 ("NFSv4.1: fixup use
> > > EXCHGID4_FLAG_USE_PNFS_DS
> > > for DS server")
> > > Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
> > > ---
> > > fs/nfs/nfs4client.c | 6 ++----
> > > fs/nfs/nfs4proc.c | 2 +-
> > > 2 files changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> > > index 11e3a285594c..ac80f87cb9d9 100644
> > > --- a/fs/nfs/nfs4client.c
> > > +++ b/fs/nfs/nfs4client.c
> > > @@ -231,9 +231,8 @@ struct nfs_client *nfs4_alloc_client(const
> > > struct
> > > nfs_client_initdata *cl_init)
> > > __set_bit(NFS_CS_INFINITE_SLOTS, &clp->cl_flags);
> > > __set_bit(NFS_CS_DISCRTRY, &clp->cl_flags);
> > > __set_bit(NFS_CS_NO_RETRANS_TIMEOUT, &clp->cl_flags);
> > > -
> > > - if (test_bit(NFS_CS_DS, &cl_init->init_flags))
> > > - __set_bit(NFS_CS_DS, &clp->cl_flags);
> > > + if (test_bit(NFS_CS_PNFS, &cl_init->init_flags))
> > > + __set_bit(NFS_CS_PNFS, &clp->cl_flags);
> >
> > Won't this change cause the match in nfs_get_client() to fail?
>
> At which stage? The problem was that nfs_match_client explicitly
> looks
> for NFS_CS_DS for matching.
>
> /* Match request for a dedicated DS */
> if (test_bit(NFS_CS_DS, &data->init_flags) !=
> test_bit(NFS_CS_DS, &clp->cl_flags))
> continue;
>
> We have pnfs flow creating client where NFS_CS_DS was set in
> init_flags and yet the stored nfs_client didn't because we dont mark
> the MDS exchange_id with DS flags.
>
> In my testing the fixed way appropriately finds the MDS's nfs_client
> for when MDS=DS and for when it's not there isn't one to begin with
> but we still only mark USE_PNFS_DS on the pnfs path and not 4.1
> mount.
>
>
AFAICS, we now match any NFSv4.1 client with the right IP address,
whether or not that client told us that it is PNFS enabled in the reply
to EXCHANGE_ID.
That appears to break the entire premise of your initial commit
51d674a5e488 ("NFSv4.1: use EXCHGID4_FLAG_USE_PNFS_DS for DS server").
So question:
Why are we not just setting (EXCHGID4_FLAG_USE_NON_PNFS |
EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_PNFS_DS) for exchange_id
(assuming that pNFS is compiled in), and letting the server return the
combination that it supports?
What is the value of leaving out flags, and then adding them in later?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
