nfsd: untangle code in nfsd4_deleg_getattr_conflict()

The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.

With this patch we:
 - properly handle non-nfsd leases.  We must not assume flc_owner is a
    delegation unless fl_lmops == &nfsd_lease_mng_ops
 - move the main code out of the for loop
 - have a single exit which calls nfs4_put_stid()
   (and other exits which don't need to call that)

[ jlayton: refactored on top of Neil's other patch: nfsd: fix
	   nfsd4_deleg_getattr_conflict in presence of third party lease ]

Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
Signed-off-by: NeilBrown <neilb@suse.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a49aa75bc0b797aaf96840e8e5470dbeb565fa12..df69dc6af467b47aff6bfd53c658b0b0ff1bf9c7 100644 (file)
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8834,6 +8834,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
        __be32 status;
        struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
        struct file_lock_context *ctx;
+       struct nfs4_delegation *dp = NULL;
        struct file_lease *fl;
        struct iattr attrs;
        struct nfs4_cb_fattr *ncf;
@@ -8843,84 +8844,76 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
        ctx = locks_inode_context(inode);
        if (!ctx)
                return 0;
+
+#define NON_NFSD_LEASE ((void *)1)
+
        spin_lock(&ctx->flc_lock);
        for_each_file_lock(fl, &ctx->flc_lease) {
-               unsigned char type = fl->c.flc_type;
-
                if (fl->c.flc_flags == FL_LAYOUT)
                        continue;
-               if (fl->fl_lmops != &nfsd_lease_mng_ops) {
-                       /*
-                        * non-nfs lease, if it's a lease with F_RDLCK then
-                        * we are done; there isn't any write delegation
-                        * on this inode
-                        */
-                       if (type == F_RDLCK)
-                               break;
-
-                       nfsd_stats_wdeleg_getattr_inc(nn);
-                       spin_unlock(&ctx->flc_lock);
-
-                       status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+               if (fl->c.flc_type == F_WRLCK) {
+                       if (fl->fl_lmops == &nfsd_lease_mng_ops)
+                               dp = fl->c.flc_owner;
+                       else
+                               dp = NON_NFSD_LEASE;
+               }
+               break;
+       }
+       if (dp == NULL || dp == NON_NFSD_LEASE ||
+           dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
+               spin_unlock(&ctx->flc_lock);
+               if (dp == NON_NFSD_LEASE) {
+                       status = nfserrno(nfsd_open_break_lease(inode,
+                                                               NFSD_MAY_READ));
                        if (status != nfserr_jukebox ||
                            !nfsd_wait_for_delegreturn(rqstp, inode))
                                return status;
-                       return 0;
                }
-               if (type == F_WRLCK) {
-                       struct nfs4_delegation *dp = fl->c.flc_owner;
+               return 0;
+       }
 
-                       if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
-                               spin_unlock(&ctx->flc_lock);
-                               return 0;
-                       }
-                       nfsd_stats_wdeleg_getattr_inc(nn);
-                       dp = fl->c.flc_owner;
-                       refcount_inc(&dp->dl_stid.sc_count);
-                       ncf = &dp->dl_cb_fattr;
-                       nfs4_cb_getattr(&dp->dl_cb_fattr);
-                       spin_unlock(&ctx->flc_lock);
-                       wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
-                                       TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
-                       if (ncf->ncf_cb_status) {
-                               /* Recall delegation only if client didn't respond */
-                               status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
-                               if (status != nfserr_jukebox ||
-                                               !nfsd_wait_for_delegreturn(rqstp, inode)) {
-                                       nfs4_put_stid(&dp->dl_stid);
-                                       return status;
-                               }
-                       }
-                       if (!ncf->ncf_file_modified &&
-                                       (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
-                                       ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
-                               ncf->ncf_file_modified = true;
-                       if (ncf->ncf_file_modified) {
-                               int err;
-
-                               /*
-                                * Per section 10.4.3 of RFC 8881, the server would
-                                * not update the file's metadata with the client's
-                                * modified size
-                                */
-                               attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
-                               attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
-                               inode_lock(inode);
-                               err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
-                               inode_unlock(inode);
-                               if (err) {
-                                       nfs4_put_stid(&dp->dl_stid);
-                                       return nfserrno(err);
-                               }
-                               ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
-                               *size = ncf->ncf_cur_fsize;
-                               *modified = true;
-                       }
-                       nfs4_put_stid(&dp->dl_stid);
-                       return 0;
+       nfsd_stats_wdeleg_getattr_inc(nn);
+       refcount_inc(&dp->dl_stid.sc_count);
+       ncf = &dp->dl_cb_fattr;
+       nfs4_cb_getattr(&dp->dl_cb_fattr);
+       spin_unlock(&ctx->flc_lock);
+
+       wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
+                           TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
+       if (ncf->ncf_cb_status) {
+               /* Recall delegation only if client didn't respond */
+               status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+               if (status != nfserr_jukebox ||
+                   !nfsd_wait_for_delegreturn(rqstp, inode))
+                       goto out_status;
+       }
+       if (!ncf->ncf_file_modified &&
+           (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
+            ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
+               ncf->ncf_file_modified = true;
+       if (ncf->ncf_file_modified) {
+               int err;
+
+               /*
+                * Per section 10.4.3 of RFC 8881, the server would
+                * not update the file's metadata with the client's
+                * modified size
+                */
+               attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
+               attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
+               inode_lock(inode);
+               err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
+               inode_unlock(inode);
+               if (err) {
+                       status = nfserrno(err);
+                       goto out_status;
                }
-               break;
+               ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
+               *size = ncf->ncf_cur_fsize;
+               *modified = true;
        }
-       spin_unlock(&ctx->flc_lock);
-       return 0;
+       status = 0;
+out_status:
+       nfs4_put_stid(&dp->dl_stid);
+       return status;
 }