diff mbox series

[1/1] NFSv4.1 another fix for EXCHGID4_FLAG_USE_PNFS_DS for DS server

Message ID 20240624132827.71808-1-olga.kornievskaia@gmail.com (mailing list archive)
State New
Headers show
Series [1/1] NFSv4.1 another fix for EXCHGID4_FLAG_USE_PNFS_DS for DS server | expand

Commit Message

Olga Kornievskaia June 24, 2024, 1:28 p.m. UTC 
From: Olga Kornievskaia <kolga@netapp.com>

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@netapp.com>
---
 fs/nfs/nfs4client.c | 6 ++----
 fs/nfs/nfs4proc.c   | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

Comments

Trond Myklebust June 24, 2024, 3:16 p.m. UTC | #1 
Hi Olga,

On Mon, 2024-06-24 at 09:28 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> 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@netapp.com>
> ---
>  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?
Olga Kornievskaia June 24, 2024, 3:41 p.m. UTC | #2 
On Mon, Jun 24, 2024 at 11:17 AM Trond Myklebust
<trondmy@hammerspace.com> wrote:
>
> Hi Olga,
>
> On Mon, 2024-06-24 at 09:28 -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > 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@netapp.com>
> > ---
> >  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.

>
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>
Trond Myklebust June 24, 2024, 6:46 p.m. UTC | #3 
On Mon, 2024-06-24 at 11:41 -0400, Olga Kornievskaia wrote:
> On Mon, Jun 24, 2024 at 11:17 AM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > Hi Olga,
> > 
> > On Mon, 2024-06-24 at 09:28 -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > > 
> > > 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@netapp.com>
> > > ---
> > >  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?
diff mbox series

Patch

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);
 	/*
 	 * Set up the connection to the server before we add add to the
 	 * global list.
@@ -1011,7 +1010,6 @@  struct nfs_client *nfs4_set_ds_client(struct nfs_server *mds_srv,
 	if (mds_srv->flags & NFS_MOUNT_NORESVPORT)
 		__set_bit(NFS_CS_NORESVPORT, &cl_init.init_flags);
 
-	__set_bit(NFS_CS_DS, &cl_init.init_flags);
 	__set_bit(NFS_CS_PNFS, &cl_init.init_flags);
 	cl_init.max_connect = NFS_MAX_TRANSPORTS;
 	/*
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 23819a756508..1afba4c1c352 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8820,7 +8820,7 @@  nfs4_run_exchange_id(struct nfs_client *clp, const struct cred *cred,
 #ifdef CONFIG_NFS_V4_1_MIGRATION
 	calldata->args.flags |= EXCHGID4_FLAG_SUPP_MOVED_MIGR;
 #endif
-	if (test_bit(NFS_CS_DS, &clp->cl_flags))
+	if (test_bit(NFS_CS_PNFS, &clp->cl_flags))
 		calldata->args.flags |= EXCHGID4_FLAG_USE_PNFS_DS;
 	msg.rpc_argp = &calldata->args;
 	msg.rpc_resp = &calldata->res;