Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <105f33d12c2f2d74f4102721a596bc321baf777f.camel@kernel.org>
Subject: Re: [PATCH 3/8] NFSD: always drop directory lock in nfsd_unlink()
From:   Jeff Layton <jlayton@kernel.org>
To:     NeilBrown <neilb@suse.de>, Chuck Lever <chuck.lever@oracle.com>
Cc:     linux-nfs@vger.kernel.org
Date:   Wed, 06 Jul 2022 09:30:17 -0400
In-Reply-To: <165708109257.1940.5635801592688577227.stgit@noble.brown>
References: <165708033167.1940.3364591321728458949.stgit@noble.brown>
         <165708109257.1940.5635801592688577227.stgit@noble.brown>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote:
> Some error paths in nfsd_unlink() allow it to exit without unlocking the
> directory.  This is not a problem in practice as the directory will be
> locked with an fh_put(), but it is untidy and potentially confusing.
>=20
> This allows us to remove all the fh_unlock() calls that are immediately
> after nfsd_unlink() calls.
>=20
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs3proc.c |    2 --
>  fs/nfsd/nfs4proc.c |    4 +---
>  fs/nfsd/vfs.c      |    7 +++++--
>  3 files changed, 6 insertions(+), 7 deletions(-)
>=20
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 38255365ef71..ad7941001106 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -478,7 +478,6 @@ nfsd3_proc_remove(struct svc_rqst *rqstp)
>  	fh_copy(&resp->fh, &argp->fh);
>  	resp->status =3D nfsd_unlink(rqstp, &resp->fh, -S_IFDIR,
>  				   argp->name, argp->len);
> -	fh_unlock(&resp->fh);
>  	return rpc_success;
>  }
> =20
> @@ -499,7 +498,6 @@ nfsd3_proc_rmdir(struct svc_rqst *rqstp)
>  	fh_copy(&resp->fh, &argp->fh);
>  	resp->status =3D nfsd_unlink(rqstp, &resp->fh, S_IFDIR,
>  				   argp->name, argp->len);
> -	fh_unlock(&resp->fh);
>  	return rpc_success;
>  }
> =20
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 3279daab909d..4737019738ab 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1052,10 +1052,8 @@ nfsd4_remove(struct svc_rqst *rqstp, struct nfsd4_=
compound_state *cstate,
>  		return nfserr_grace;
>  	status =3D nfsd_unlink(rqstp, &cstate->current_fh, 0,
>  			     remove->rm_name, remove->rm_namelen);
> -	if (!status) {
> -		fh_unlock(&cstate->current_fh);
> +	if (!status)
>  		set_change_info(&remove->rm_cinfo, &cstate->current_fh);
> -	}
>  	return status;
>  }
> =20
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 1e7ca39e8a49..3f4579f5775c 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1762,12 +1762,12 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh=
 *fhp, int type,
>  	rdentry =3D lookup_one_len(fname, dentry, flen);
>  	host_err =3D PTR_ERR(rdentry);
>  	if (IS_ERR(rdentry))
> -		goto out_drop_write;
> +		goto out_unlock;
> =20
>  	if (d_really_is_negative(rdentry)) {
>  		dput(rdentry);
>  		host_err =3D -ENOENT;
> -		goto out_drop_write;
> +		goto out_unlock;
>  	}
>  	rinode =3D d_inode(rdentry);
>  	ihold(rinode);
> @@ -1805,6 +1805,9 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *=
fhp, int type,
>  	}
>  out:
>  	return err;
> +out_unlock:
> +	fh_unlock(fhp);
> +	goto out_drop_write;
>  }
> =20
>  /*
>=20
>=20

Reviewed-by: Jeff Layton <jlayton@kernel.org>