2022-09-08 22:24:07

by Chuck Lever III

Subject: [PATCH v4 6/8] NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY

nfsd_setattr() can kick off a CB_RECALL (via
notify_change() -> break_lease()) if a delegation is present. Before
returning NFS4ERR_DELAY, give the client holding that delegation a
chance to return it and then retry the nfsd_setattr() again, once.

Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=354
Tested-by: Igor Mammedov <[email protected]>
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/vfs.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 02f31d8c727a..03a826ccc165 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -394,6 +394,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
int host_err;
bool get_write_count;
bool size_change = (iap->ia_valid & ATTR_SIZE);
+ int retries;

if (iap->ia_valid & ATTR_SIZE) {
accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
@@ -455,7 +456,13 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
}

inode_lock(inode);
- host_err = __nfsd_setattr(dentry, iap);
+ for (retries = 1;;) {
+ host_err = __nfsd_setattr(dentry, iap);
+ if (host_err != -EAGAIN || !retries--)
+ break;
+ if (!nfsd_wait_for_delegreturn(rqstp, inode))
+ break;
+ }
if (attr->na_seclabel && attr->na_seclabel->len)
attr->na_labelerr = security_inode_setsecctx(dentry,
attr->na_seclabel->data, attr->na_seclabel->len);

2022-09-12 16:26:06

by Jeff Layton

Subject: Re: [PATCH v4 6/8] NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY

On Thu, 2022-09-08 at 18:14 -0400, Chuck Lever wrote:
> nfsd_setattr() can kick off a CB_RECALL (via
> notify_change() -> break_lease()) if a delegation is present. Before
> returning NFS4ERR_DELAY, give the client holding that delegation a
> chance to return it and then retry the nfsd_setattr() again, once.
>
> Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=354
> Tested-by: Igor Mammedov <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/vfs.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 02f31d8c727a..03a826ccc165 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -394,6 +394,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> int host_err;
> bool get_write_count;
> bool size_change = (iap->ia_valid & ATTR_SIZE);
> + int retries;
>
> if (iap->ia_valid & ATTR_SIZE) {
> accmode |= NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE;
> @@ -455,7 +456,13 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> }
>
> inode_lock(inode);
> - host_err = __nfsd_setattr(dentry, iap);
> + for (retries = 1;;) {
> + host_err = __nfsd_setattr(dentry, iap);
> + if (host_err != -EAGAIN || !retries--)
> + break;

Can __nfsd_setattr return -EAGAIN for entirely different reasons? I
try_break_deleg will, but could an underlying filesystem's ->setattr
operation return -EAGAIN for an unrelated reason?

Then again, you're only retrying once, so maybe it's no big deal.

> + if (!nfsd_wait_for_delegreturn(rqstp, inode))
> + break;
> + }
> if (attr->na_seclabel && attr->na_seclabel->len)
> attr->na_labelerr = security_inode_setsecctx(dentry,
> attr->na_seclabel->data, attr->na_seclabel->len);
>
>

--
Jeff Layton <[email protected]>