Received-SPF: pass (google.com: domain of linux-nfs+bounces-3065-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Apr 2024 12:12:37 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Olga Kornievskaia <aglo@umich.edu>
Cc: cel@kernel.org, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <dai.ngo@oracle.com>, linux-nfs@vger.kernel.org
Subject: Re: [RFC PATCH 4/4] NFS: Implement NFSv4.2's OFFLOAD_STATUS operation
Message-ID: <Zi/G9dtc9VVCnEZA@tissot.1015granger.net>
References: <20240429151632.212571-6-cel@kernel.org>
 <20240429151632.212571-10-cel@kernel.org>
 <CAN-5tyH2e7uR_GN3vYp201h2U83gra=U8a+57Ughk1BfpRGZaQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAN-5tyH2e7uR_GN3vYp201h2U83gra=U8a+57Ughk1BfpRGZaQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?b1dCbGNOdTVXSnFONXZtOXZaVTluc1BwUk1iYmZ4d3hOTzNJN1l0QlBIazlJ?=
 =?utf-8?B?cWd0YWh6Q051QUIyY0pFUGZNZmprNGxsNldLVEJZc1dmS2RkVG9NWFdFdjhI?=
 =?utf-8?B?OE5BNVpqdVJlZE9uUytiMEc0VDZDUkVlcm0wZmhrN0FuZ0JDYlpqT1dTcGZJ?=
 =?utf-8?B?cWEwY0JLNlpaUC9haVRwbDVIem41V0VNeE9VcWVwOWc2RTJFbTV2TGhPaEpG?=
 =?utf-8?B?aUJ2TG1xNnlDazFpbjNocTdyb3dOODRBbHpodkpVbVRqWVMzK3k4VHRla0FP?=
 =?utf-8?B?YUJ5eHpNS2hYUlVnbjdpNXVrSzEyd2VWclE2NGlJazV6MGw2S3JhWjgxb0t3?=
 =?utf-8?B?VC9yWWpaK1MrMWlPd2UxNkRIdHNDYW1jd3VCSVBCbUZHc1BtNlFWekhqWEFs?=
 =?utf-8?B?ZktLNVRVWW5HNHZybU5iNXB6Tkk4Y0tqQlBUUUMyY1ZTU2I1N05IYzh0c09R?=
 =?utf-8?B?QnlobHVQU3FHYkxnQ2FzdkR3VTlMdjRPQ0YyWldWdFpkUXAxZU56YTIvejZs?=
 =?utf-8?B?UGhMd1U5c2hUZjBQbWFScU5Gb3hSTTdSQzZqRTNsRnVaWUtndkxMLzdaa1Vz?=
 =?utf-8?B?VStESmdkZXpWaHdUbGxQY0FqRUx5WXg5Z25EdVllSy8wSHZtM1B2Vk5JRlVF?=
 =?utf-8?B?eG1tRHZra216aHBJZFYraXJKQUhZRkxVdE80VFJBQmVyd2ZDMjRsSXZvT3k3?=
 =?utf-8?B?RUQwRm5Fd0NtaVJxcXFKTjFPeUdYdndLNUdYOXF4M0NHc2cwL1NJMTFVTFBm?=
 =?utf-8?B?ZERpclFkSWwvdVpEUWxaODBvbTdXdExYTDRSdGpCL3p4czlKd3RWcVUxSVZF?=
 =?utf-8?B?bEhXOHdxMmx0U2dlUS9PUTgrQlp5VG1ibGo1MG1sWHJhYk0vaG1xZC8xclJW?=
 =?utf-8?B?Q0RCN2lBZ1dwcGd3VXFvZ3FpZHV0aDIvc0o5SkoxMW1GQzJWWlpQM1VmVXpO?=
 =?utf-8?B?blV6OFZBVElmWC96Z2ZISEFpM09VT0FCOTV6Y0JpSXN6ZCtHd1lVaVYrVEJQ?=
 =?utf-8?B?V2lEWUdrSk5OU2NJYmtPenU5UlQwb3QyNHc4amNFOG1nNmRIRDRSV0FISFRC?=
 =?utf-8?B?OHB0MVNxZEtwdTlSM1JmRzBpRms0bWZzZi8yVnFZdHR0NnEyVGljOUltSlBG?=
 =?utf-8?B?SkZKY0FCMkFKSCsyYmMxUlBQNFhPSWN1TUMrcnh4bDhoS1lLWEt6cmIydkJQ?=
 =?utf-8?B?QzUwSCtXV0NsMXFObHlSRUVqek5acjNoZVh1R0h2TWR4OFZQVTJqTm1ZVk9u?=
 =?utf-8?B?RTNHREkzckNMREs5Q1hoa1ZMSmxDNkJNRU12UEJwanU1ZEF5TUlOWGNCazRo?=
 =?utf-8?B?eWNuWk9FVVhKNmVodFQ5SE9oUkU5RHBVSWpqeENpK1o1a3kxN0U3TmwwOFhY?=
 =?utf-8?B?WFhnWW9UQmMzM0JEWVBIY0IvcG5pckRwSlR0WGdxUFJiNTRsQ3NQb01wbXJk?=
 =?utf-8?B?OEpFdzN3a1VINWxTVERCVlp5ZVJIK0xJdEdoMUZLdmxRM3NjdEplTTUwd1NU?=
 =?utf-8?B?em5kUHczbUdLMXFxZm5zV1F3QVhsNHJpWTBMK3gyZEpkOGNKMkF2VWdNZUJq?=
 =?utf-8?B?UUEzYU4yQVFMUzlObit6dmRMODR6aURvL2RpdjEyNEJRQlM0UmVLQ2JFaXlS?=
 =?utf-8?B?SjdSWXd5VXlndkp5Tlh3R2lNMHNNTm44Rk9FeUlEakg5Q29OVjVNaHFDOTlN?=
 =?utf-8?B?MWVlayt2VmpwSkNCRkxuWkFCNkxLWGR5WGFvN1lZbFFTb3B6NzNSOTNDZDNk?=
 =?utf-8?B?eXl2MmZvcHhZakNFczlRUGZvSVdBYVJDemdSRDYvSUxPdDB5bHBqTThvVEZo?=
 =?utf-8?B?Mld2dGRtd3lHTXREQzc1eGhDcFdMN2F2VFlvQ1RoL1N5RGtZODlrU1lzcDlK?=
 =?utf-8?B?Sk1pYmZ4TlB5dEo5aFFyckZmK1h5U05VTG82NEhSaFRhaXBaa1kyVmVtNmFM?=
 =?utf-8?B?aE5CMENkczAxSG5JS0lPZzNGamFwK2crWXhEdkdINThlMTN3WXQvNWE3RHJH?=
 =?utf-8?B?K0Z2ZkZwZzh2SnhLUTBEN0dSYWJmT0JncVNNa284S05RYU5oenAxZ1BoVUhv?=
 =?utf-8?B?OUpkSk82bjk3blgwUXhMN0NMWmRjaTZJZXY0cUkyQkNzVk90WU02MFhXeFpt?=
 =?utf-8?B?cmx1VTFOWEFRQks4WGZiQXIxbUFkdzA1b284K3FidFlHenhxV1hFWEowTmtS?=
 =?utf-8?B?ZkE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	sypq4vCz3Ssyj3yp4td/mICO71t5IpNyFz2239ykFy/siSRvefMR4cDWmxF5/Tk5BEIciU3UibGsJxwPmiuj8kUD0+QaC0GbMX9DdBvZk1V+ul32jao7hrslYQo12wOzm2lMT7LCAiQABG/Xwmyoi5XwwVGkrna2agHFIEWrGhAdstUKFeSckpvsJrWDgnWUNv1zyZvMGbH1INZW4cmZgsUHWdHcKFLXqHywWUPt+QhWAmFq/0hdxeYL7QlkXFKEayweWDq0OJ/RJf2sMXriSUnmF3qK2unmCF8wAM01PPQ08Jy8skSS3VBhjNMoO5rRRtqXR49f5kTLsNs1QEPulVPWc2/9U/3sNDUjlhdJndcK6w91lpr3iTL4iczUQHe+C0rtTF5y4CIY7p3d417ZvNluL1rXGSl0c0eDjfoGBqzDtne7dc/SWs3I6BCTqB3pAAmQ0OnF6ARrnBiuVpCUGpwsexBRNXxrDbQ4VOKMmnjP01BSn80ZC+qcwqyYHB0k3uweWoKAdOlzi+60WWOwYT9aOu857j9xPz7ruM3aNEz/LVKWeS2g2zRwGJdmKMoxJ9+98lZmEzaQRktMwqHhuEwSE+vAZFoCgkGzlSNBJEs=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 690253b4-5d06-4ef1-1175-08dc686731ba
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 16:12:40.6496
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Fr5NE31A55KbuYTExpJ/vmmMNLr+6hVdROiqA557PA1pOxBDQ25gu3K7vARVkObMCMnRSowHYgnLtvXjQepXyg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB5809
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-04-29_14,2024-04-29_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 bulkscore=0
 suspectscore=0 malwarescore=0 spamscore=0 adultscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2404290103
X-Proofpoint-ORIG-GUID: QX9b8UNOh0sPVV0QaH1J-PxvWFljYLjF
X-Proofpoint-GUID: QX9b8UNOh0sPVV0QaH1J-PxvWFljYLjF

On Mon, Apr 29, 2024 at 11:35:20AM -0400, Olga Kornievskaia wrote:
> On Mon, Apr 29, 2024 at 11:22 AM <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.

The client doesn't know whether the server supports OFFLOAD_STATUS
until the first OFFLOAD_STATUS is sent. So at least /one/ of those
waits will be of the timeout variety, no matter what.

We can mitigate the cost of waking up periodically by making
NFS42_COPY_TIMEOUT longer than 7 seconds. That's just a number I
pulled out of the air.


> (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).

IIUC the wait_for_completion() family of APIs does not sleep at all
if the completion has already been marked "done". So in that case
wfc() should just return a positive number and execution drops into
the "copy complete" code.


> (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.

The callback reply should just wake the completion, even though
there might no longer be a waiter, and then free the call data. That
seems OK.

I think what I missed is that the call data needs to be cleaned
up when we are sure the CB_OFFLOAD was lost, otherwise it leaks.
Will need to think about that. How is that done if the client
detects a server restart?

Also nfs42_proc_offload_status() needs to distinguish between "still
running," "all done," "bad stateid," and "no answer".


> > Suggested-by: Olga Kornievskaia <kolga@netapp.com>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218735
> > 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 *stateid);
> >
> >  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 *res,
> >                 spin_unlock(&src_server->nfs_client->cl_lock);
> >         }
> >
> > -       status = wait_for_completion_interruptible(&copy->completion);
> > +wait:
> > +       status = wait_for_completion_interruptible_timeout(&copy->completion,
> > +                                                          NFS42_COPY_TIMEOUT);
> >         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 == -ERESTARTSYS) {
> > -               goto out_cancel;
> > -       } else if (copy->flags || copy->error == NFS4ERR_PARTNER_NO_AUTH) {
> > -               status = -EAGAIN;
> > -               *restart = true;
> > +       switch (status) {
> > +       case 0:
> > +               status = nfs42_proc_offload_status(src, src_stateid);
> > +               if (status && status != -EOPNOTSUPP)
> > +                       goto wait;
> > +               break;
> > +       case -ERESTARTSYS:
> >                 goto out_cancel;
> > +       default:
> > +               if (copy->flags || copy->error == NFS4ERR_PARTNER_NO_AUTH) {
> > +                       status = -EAGAIN;
> > +                       *restart = true;
> > +                       goto out_cancel;
> > +               }
> >         }
> >  out:
> >         res->write_res.count = 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 *calldata)
> > +{
> > +       struct nfs42_offload_data *data = 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 *calldata)
> > +{
> > +       struct nfs42_offload_data *data = 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) == -EAGAIN)
> > +               rpc_restart_call_prepare(task);
> > +}
> > +
> > +static const struct rpc_call_ops nfs42_offload_status_ops = {
> > +       .rpc_call_prepare = nfs42_offload_status_prepare,
> > +       .rpc_call_done = nfs42_offload_status_done,
> > +       .rpc_release = nfs42_free_offload_data,
> > +};
> > +
> > +static int nfs42_proc_offload_status(struct file *file, nfs4_stateid *stateid)
> > +{
> > +       struct nfs_open_context *ctx = nfs_file_open_context(file);
> > +       struct nfs_server *server = NFS_SERVER(file_inode(file));
> > +       struct nfs42_offload_data *data = NULL;
> > +       struct rpc_task *task;
> > +       struct rpc_message msg = {
> > +               .rpc_proc       = &nfs4_procedures[NFSPROC4_CLNT_OFFLOAD_STATUS],
> > +               .rpc_cred       = ctx->cred,
> > +       };
> > +       struct rpc_task_setup task_setup_data = {
> > +               .rpc_client     = server->client,
> > +               .rpc_message    = &msg,
> > +               .callback_ops   = &nfs42_offload_status_ops,
> > +               .workqueue      = nfsiod_workqueue,
> > +               .flags          = RPC_TASK_ASYNC | RPC_TASK_SOFTCONN,
> > +       };
> > +       int status;
> > +
> > +       if (!(server->caps & NFS_CAP_OFFLOAD_STATUS))
> > +               return -EOPNOTSUPP;
> > +
> > +       data = kzalloc(sizeof(struct nfs42_offload_data), GFP_KERNEL);
> > +       if (data == NULL)
> > +               return -ENOMEM;
> > +
> > +       data->seq_server = server;
> > +       data->args.osa_src_fh = NFS_FH(file_inode(file));
> > +       memcpy(&data->args.osa_stateid, stateid,
> > +               sizeof(data->args.osa_stateid));
> > +       msg.rpc_argp = &data->args;
> > +       msg.rpc_resp = &data->res;
> > +       task_setup_data.callback_data = data;
> > +       nfs4_init_sequence(&data->args.osa_seq_args, &data->res.osr_seq_res,
> > +                          1, 0);
> > +       task = rpc_run_task(&task_setup_data);
> > +       if (IS_ERR(task)) {
> > +               nfs42_free_offload_data(data);
> > +               return PTR_ERR(task);
> > +       }
> > +       status = rpc_wait_for_completion_task(task);
> > +       if (status == -ENOTSUPP)
> > +               server->caps &= ~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
> >
> >

-- 
Chuck Lever