Received-SPF: pass (google.com: domain of linux-nfs+bounces-3064-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240429151632.212571-6-cel@kernel.org> <20240429151632.212571-10-cel@kernel.org>
In-Reply-To: <20240429151632.212571-10-cel@kernel.org>
From: Olga Kornievskaia <aglo@umich.edu>
Date: Mon, 29 Apr 2024 11:35:20 -0400
Message-ID: <CAN-5tyH2e7uR_GN3vYp201h2U83gra=U8a+57Ughk1BfpRGZaQ@mail.gmail.com>
Subject: Re: [RFC PATCH 4/4] NFS: Implement NFSv4.2's OFFLOAD_STATUS operation
To: cel@kernel.org
Cc: Olga Kornievskaia <kolga@netapp.com>, Dai Ngo <dai.ngo@oracle.com>, linux-nfs@vger.kernel.org, 
	Chuck Lever <chuck.lever@oracle.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 29, 2024 at 11:22=E2=80=AFAM <cel@kernel.org> wrote:
>
> From: Chuck Lever <chuck.lever@oracle.com>
>
> We've found that there are cases where a transport disconnection
> results in the loss of callback RPCs. NFS servers typically do not
> retransmit callback operations after a disconnect.
>
> This can be a problem for the Linux NFS client's implementation of
> asynchronous COPY, which waits indefinitely for a CB_OFFLOAD
> callback. If a transport disconnect occurs while an async COPY is
> running, there's a good chance the client will never get the
> matching CB_OFFLOAD.
>
> Fix this by implementing the OFFLOAD_STATUS operation so that the
> Linux NFS client can probe the NFS server if it doesn't see a
> CB_OFFLOAD in a reasonable amount of time.
>
> This patch implements a simplistic check. As future work, the client
> might also be able to detect whether there is no forward progress on
> the request asynchronous COPY operation, and CANCEL it.

I think this patch series needs a bit more nuances
(1) if we know that server doesn't support offload_status we might as
well wait uninterrupted perhaps? but I can see how as you mentioned we
might want to measure no forward progress and cancel the copy and
fallback to read/write.
(2) we can't really go back to the "wait" after failing a
offload_status as the cb_offload callback might have already arrived
and I think we need to walk the pending_cb_callbacks to make sure we
haven't received it before waiting again (otherwise, we'd wait
forever).
(3) then also there is the case where we woke up and sent the
offload_status and got a 'copy finished' reply but we also got the
cb_callback reply as well and the copy things need to be cleaned up
now.

>
> Suggested-by: Olga Kornievskaia <kolga@netapp.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=3D218735
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/nfs42proc.c        | 100 +++++++++++++++++++++++++++++++++++---
>  fs/nfs/nfs4trace.h        |   1 +
>  include/linux/nfs_fs_sb.h |   1 +
>  3 files changed, 96 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 7656d7c103fa..224fb3b8696a 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -21,6 +21,7 @@
>
>  #define NFSDBG_FACILITY NFSDBG_PROC
>  static int nfs42_do_offload_cancel_async(struct file *dst, nfs4_stateid =
*std);
> +static int nfs42_proc_offload_status(struct file *file, nfs4_stateid *st=
ateid);
>
>  static void nfs42_set_netaddr(struct file *filep, struct nfs42_netaddr *=
naddr)
>  {
> @@ -173,6 +174,9 @@ int nfs42_proc_deallocate(struct file *filep, loff_t =
offset, loff_t len)
>         return err;
>  }
>
> +/* Wait this long before checking progress on a COPY operation */
> +#define NFS42_COPY_TIMEOUT     (7 * HZ)
> +
>  static int handle_async_copy(struct nfs42_copy_res *res,
>                              struct nfs_server *dst_server,
>                              struct nfs_server *src_server,
> @@ -222,7 +226,9 @@ static int handle_async_copy(struct nfs42_copy_res *r=
es,
>                 spin_unlock(&src_server->nfs_client->cl_lock);
>         }
>
> -       status =3D wait_for_completion_interruptible(&copy->completion);
> +wait:
> +       status =3D wait_for_completion_interruptible_timeout(&copy->compl=
etion,
> +                                                          NFS42_COPY_TIM=
EOUT);
>         spin_lock(&dst_server->nfs_client->cl_lock);
>         list_del_init(&copy->copies);
>         spin_unlock(&dst_server->nfs_client->cl_lock);
> @@ -231,12 +237,20 @@ static int handle_async_copy(struct nfs42_copy_res =
*res,
>                 list_del_init(&copy->src_copies);
>                 spin_unlock(&src_server->nfs_client->cl_lock);
>         }
> -       if (status =3D=3D -ERESTARTSYS) {
> -               goto out_cancel;
> -       } else if (copy->flags || copy->error =3D=3D NFS4ERR_PARTNER_NO_A=
UTH) {
> -               status =3D -EAGAIN;
> -               *restart =3D true;
> +       switch (status) {
> +       case 0:
> +               status =3D nfs42_proc_offload_status(src, src_stateid);
> +               if (status && status !=3D -EOPNOTSUPP)
> +                       goto wait;
> +               break;
> +       case -ERESTARTSYS:
>                 goto out_cancel;
> +       default:
> +               if (copy->flags || copy->error =3D=3D NFS4ERR_PARTNER_NO_=
AUTH) {
> +                       status =3D -EAGAIN;
> +                       *restart =3D true;
> +                       goto out_cancel;
> +               }
>         }
>  out:
>         res->write_res.count =3D copy->count;
> @@ -582,6 +596,80 @@ static int nfs42_do_offload_cancel_async(struct file=
 *dst,
>         return status;
>  }
>
> +static void nfs42_offload_status_prepare(struct rpc_task *task, void *ca=
lldata)
> +{
> +       struct nfs42_offload_data *data =3D calldata;
> +
> +       nfs4_setup_sequence(data->seq_server->nfs_client,
> +                               &data->args.osa_seq_args,
> +                               &data->res.osr_seq_res, task);
> +}
> +
> +static void nfs42_offload_status_done(struct rpc_task *task, void *calld=
ata)
> +{
> +       struct nfs42_offload_data *data =3D calldata;
> +
> +       trace_nfs4_offload_status(&data->args, task->tk_status);
> +       nfs41_sequence_done(task, &data->res.osr_seq_res);
> +       if (task->tk_status &&
> +               nfs4_async_handle_error(task, data->seq_server, NULL,
> +                       NULL) =3D=3D -EAGAIN)
> +               rpc_restart_call_prepare(task);
> +}
> +
> +static const struct rpc_call_ops nfs42_offload_status_ops =3D {
> +       .rpc_call_prepare =3D nfs42_offload_status_prepare,
> +       .rpc_call_done =3D nfs42_offload_status_done,
> +       .rpc_release =3D nfs42_free_offload_data,
> +};
> +
> +static int nfs42_proc_offload_status(struct file *file, nfs4_stateid *st=
ateid)
> +{
> +       struct nfs_open_context *ctx =3D nfs_file_open_context(file);
> +       struct nfs_server *server =3D NFS_SERVER(file_inode(file));
> +       struct nfs42_offload_data *data =3D NULL;
> +       struct rpc_task *task;
> +       struct rpc_message msg =3D {
> +               .rpc_proc       =3D &nfs4_procedures[NFSPROC4_CLNT_OFFLOA=
D_STATUS],
> +               .rpc_cred       =3D ctx->cred,
> +       };
> +       struct rpc_task_setup task_setup_data =3D {
> +               .rpc_client     =3D server->client,
> +               .rpc_message    =3D &msg,
> +               .callback_ops   =3D &nfs42_offload_status_ops,
> +               .workqueue      =3D nfsiod_workqueue,
> +               .flags          =3D RPC_TASK_ASYNC | RPC_TASK_SOFTCONN,
> +       };
> +       int status;
> +
> +       if (!(server->caps & NFS_CAP_OFFLOAD_STATUS))
> +               return -EOPNOTSUPP;
> +
> +       data =3D kzalloc(sizeof(struct nfs42_offload_data), GFP_KERNEL);
> +       if (data =3D=3D NULL)
> +               return -ENOMEM;
> +
> +       data->seq_server =3D server;
> +       data->args.osa_src_fh =3D NFS_FH(file_inode(file));
> +       memcpy(&data->args.osa_stateid, stateid,
> +               sizeof(data->args.osa_stateid));
> +       msg.rpc_argp =3D &data->args;
> +       msg.rpc_resp =3D &data->res;
> +       task_setup_data.callback_data =3D data;
> +       nfs4_init_sequence(&data->args.osa_seq_args, &data->res.osr_seq_r=
es,
> +                          1, 0);
> +       task =3D rpc_run_task(&task_setup_data);
> +       if (IS_ERR(task)) {
> +               nfs42_free_offload_data(data);
> +               return PTR_ERR(task);
> +       }
> +       status =3D rpc_wait_for_completion_task(task);
> +       if (status =3D=3D -ENOTSUPP)
> +               server->caps &=3D ~NFS_CAP_OFFLOAD_STATUS;
> +       rpc_put_task(task);
> +       return status;
> +}
> +
>  static int _nfs42_proc_copy_notify(struct file *src, struct file *dst,
>                                    struct nfs42_copy_notify_args *args,
>                                    struct nfs42_copy_notify_res *res)
> diff --git a/fs/nfs/nfs4trace.h b/fs/nfs/nfs4trace.h
> index 8f32dbf9c91d..9bcc525c71d1 100644
> --- a/fs/nfs/nfs4trace.h
> +++ b/fs/nfs/nfs4trace.h
> @@ -2564,6 +2564,7 @@ DECLARE_EVENT_CLASS(nfs4_offload_class,
>                         ), \
>                         TP_ARGS(args, error))
>  DEFINE_NFS4_OFFLOAD_EVENT(nfs4_offload_cancel);
> +DEFINE_NFS4_OFFLOAD_EVENT(nfs4_offload_status);
>
>  DECLARE_EVENT_CLASS(nfs4_xattr_event,
>                 TP_PROTO(
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index 92de074e63b9..0937e73c4767 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -278,6 +278,7 @@ struct nfs_server {
>  #define NFS_CAP_LGOPEN         (1U << 5)
>  #define NFS_CAP_CASE_INSENSITIVE       (1U << 6)
>  #define NFS_CAP_CASE_PRESERVING        (1U << 7)
> +#define NFS_CAP_OFFLOAD_STATUS (1U << 8)
>  #define NFS_CAP_POSIX_LOCK     (1U << 14)
>  #define NFS_CAP_UIDGID_NOMAP   (1U << 15)
>  #define NFS_CAP_STATEID_NFSV41 (1U << 16)
> --
> 2.44.0
>
>