Re: [PATCH 1/1] NFSv4.1 mark qualified async operations as MOVEABLE tasks

From: Trond Myklebust <trondmy@hammerspace.com>
To: "anna.schumaker@netapp.com" <anna.schumaker@netapp.com>,
	"olga.kornievskaia@gmail.com" <olga.kornievskaia@gmail.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/1] NFSv4.1 mark qualified async operations as MOVEABLE tasks
Date: Mon, 16 May 2022 18:16:50 +0000	[thread overview]
Message-ID: <317febc7f8a3264c78b34bc6e022f2b373b6aaa4.camel@hammerspace.com> (raw)
In-Reply-To: <CAN-5tyFJeLWi5UYYSvaYspwOn3VhGMQkKx-QQkXSX1ZR+guD8g@mail.gmail.com>

On Mon, 2022-05-16 at 12:34 -0400, Olga Kornievskaia wrote:
> Hi Trond and Anna,
> 
> Any update on taking this patch? Thank you.

Can we please rewrite this to not use clp->cl_minorversion? The problem
with assigning functionality to the value of the minor version field is
that you have no guarantees that future minor version updates will
support the same functionality.

I'd therefore much prefer to see a capability assigned to the 'RPC task
is movable' functionality, and that any tests take their cue from the
value of that flag (just set that flag in the 'init_caps' field in
nfs_v4_1_minor_ops and nfs_v4_1_minor_ops). Please also change the
existing tests in the code to use the new flag.

The second suggestion is that we move these tests themselves to the
functions that set up the RPC call. IOW: nfs_initiate_pgio(),
nfs_initiate_commit(), nfs_do_call_unlink(), nfs_async_rename(), etc.
They don't need to be in the rpc_call_prepare() callback, since there
is no condition that might change the outcome of the test once the RPC
call has been set up.

> 
> On Wed, Apr 13, 2022 at 9:22 AM Olga Kornievskaia
> <olga.kornievskaia@gmail.com> wrote:
> > 
> > From: Olga Kornievskaia <kolga@netapp.com>
> > 
> > Mark async operations such as RENAME, REMOVE, COMMIT MOVEABLE
> > for the nfsv4.1+ sessions.
> > 
> > Fixes: 85e39feead948 ("NFSv4.1 identify and mark RPC tasks that can
> > move between transports")
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/nfs4proc.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 16106f805ffa..f593bad996af 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -4780,7 +4780,11 @@ static void nfs4_proc_unlink_setup(struct
> > rpc_message *msg,
> > 
> >  static void nfs4_proc_unlink_rpc_prepare(struct rpc_task *task,
> > struct nfs_unlinkdata *data)
> >  {
> > -       nfs4_setup_sequence(NFS_SB(data->dentry->d_sb)->nfs_client,
> > +       struct nfs_client *clp = NFS_SB(data->dentry->d_sb)-
> > >nfs_client;
> > +
> > +       if (clp->cl_minorversion)
> > +               task->tk_flags |= RPC_TASK_MOVEABLE;
> > +       nfs4_setup_sequence(clp,
> >                         &data->args.seq_args,
> >                         &data->res.seq_res,
> >                         task);
> > @@ -4823,7 +4827,11 @@ static void nfs4_proc_rename_setup(struct
> > rpc_message *msg,
> > 
> >  static void nfs4_proc_rename_rpc_prepare(struct rpc_task *task,
> > struct nfs_renamedata *data)
> >  {
> > -       nfs4_setup_sequence(NFS_SERVER(data->old_dir)->nfs_client,
> > +       struct nfs_client *clp = NFS_SERVER(data->old_dir)-
> > >nfs_client;
> > +
> > +       if (clp->cl_minorversion)
> > +               task->tk_flags |= RPC_TASK_MOVEABLE;
> > +       nfs4_setup_sequence(clp,
> >                         &data->args.seq_args,
> >                         &data->res.seq_res,
> >                         task);
> > @@ -5457,7 +5465,11 @@ static void nfs4_proc_read_setup(struct
> > nfs_pgio_header *hdr,
> >  static int nfs4_proc_pgio_rpc_prepare(struct rpc_task *task,
> >                                       struct nfs_pgio_header *hdr)
> >  {
> > -       if (nfs4_setup_sequence(NFS_SERVER(hdr->inode)->nfs_client,
> > +       struct nfs_client *clp = NFS_SERVER(hdr->inode)-
> > >nfs_client;
> > +
> > +       if (clp->cl_minorversion)
> > +               task->tk_flags |= RPC_TASK_MOVEABLE;
> > +       if (nfs4_setup_sequence(clp,
> >                         &hdr->args.seq_args,
> >                         &hdr->res.seq_res,
> >                         task))
> > @@ -5595,7 +5607,11 @@ static void nfs4_proc_write_setup(struct
> > nfs_pgio_header *hdr,
> > 
> >  static void nfs4_proc_commit_rpc_prepare(struct rpc_task *task,
> > struct nfs_commit_data *data)
> >  {
> > -       nfs4_setup_sequence(NFS_SERVER(data->inode)->nfs_client,
> > +       struct nfs_client *clp = NFS_SERVER(data->inode)-
> > >nfs_client;
> > +
> > +       if (clp->cl_minorversion)
> > +               task->tk_flags |= RPC_TASK_MOVEABLE;
> > +       nfs4_setup_sequence(clp,
> >                         &data->args.seq_args,
> >                         &data->res.seq_res,
> >                         task);
> > --
> > 2.27.0
> > 

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