On Thu, 6 Aug 2015 16:23:36 -0400
bfields@xxxxxxxxxxxx (J. Bruce Fields) wrote:
> We have two grace periods, one for NLM and one for NFSv4+, and don't
> permit normal locking until both are over.
>
> As far as I can tell nfsdcltrack doesn't understand statd/NSM state; it
> only concerns itself with NFSv4+. It can end the NFSv4 grace period
> early, but that still leaves the server returning GRACE errors until NSM
> exits its grace period.
>
> The server has never had any smarts to end the NSM grace period early,
> even though there are common cases (e.g., no clients at all) where it
> easily could. We could fix that.
>
Erm...but it does have those smarts. statd should end the grace period
early if there were no clients to be notified when it starts. That's
done by writing to /proc/fs/lockd/nlm_end_grace. Those patches were
merged upstream in the kernel and nfs-utils around the same time as the
other early grace lifting patches.
> But one quick thing we can do to limit the impact of the NLM grace
> period is just to note that NLM locks have never conflicted with NFSv4
> opens, so NFSv4 opens don't really need to be waiting.
>
> Am I missing anything?
>
> --b.
>
> commit d2ab7fe05cd0
> Author: J. Bruce Fields <bfields@xxxxxxxxxx>
> Date: Thu Aug 6 12:47:02 2015 -0400
>
> lockd: NLM grace period shouldn't block NFSv4 opens
>
> NLM locks don't conflict with NFSv4 share reservations, so we're not
> going to learn anything new by watiting for them.
>
> They do conflict with NFSv4 locks and with delegations.
>
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 530914b5c455..d678bcc3cbcb 100644
> --- a/fs/lockd/svc.c
> +++ b/fs/lockd/svc.c
> @@ -591,6 +591,7 @@ static int lockd_init_net(struct net *net)
>
> INIT_DELAYED_WORK(&ln->grace_period_end, grace_ender);
> INIT_LIST_HEAD(&ln->lockd_manager.list);
> + ln->lockd_manager.block_opens = false;
> spin_lock_init(&ln->nsm_clnt_lock);
> return 0;
> }
> diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c
> index ae6e58ea4de5..fd8c9a5bcac4 100644
> --- a/fs/nfs_common/grace.c
> +++ b/fs/nfs_common/grace.c
> @@ -63,14 +63,33 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
> * lock reclaims.
> */
> int
> -locks_in_grace(struct net *net)
> +__state_in_grace(struct net *net, bool open)
> {
> struct list_head *grace_list = net_generic(net, grace_net_id);
> + struct lock_manager *lm;
>
> - return !list_empty(grace_list);
> + if (!open)
> + return !list_empty(grace_list);
> +
> + list_for_each_entry(lm, grace_list, list) {
> + if (lm->block_opens)
> + return true;
> + }
> + return false;
> +}
> +
> +int locks_in_grace(struct net *net)
> +{
> + return __state_in_grace(net, 0);
nit: 0 -> false (since the arg is a bool)
> }
> EXPORT_SYMBOL_GPL(locks_in_grace);
>
> +int opens_in_grace(struct net *net)
> +{
> + return __state_in_grace(net, 1);
> +}
> +EXPORT_SYMBOL_GPL(opens_in_grace);
> +
> static int __net_init
> grace_init_net(struct net *net)
> {
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index e779d7db24b0..b9681ee0ed19 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -415,10 +415,10 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> /* Openowner is now set, so sequence id will get bumped. Now we need
> * these checks before we do any creates: */
> status = nfserr_grace;
> - if (locks_in_grace(net) && open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
> + if (opens_in_grace(net) && open->op_claim_type != NFS4_OPEN_CLAIM_PREVIOUS)
> goto out;
> status = nfserr_no_grace;
> - if (!locks_in_grace(net) && open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS)
> + if (!opens_in_grace(net) && open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS)
> goto out;
>
> switch (open->op_claim_type) {
> @@ -827,7 +827,7 @@ nfsd4_remove(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> {
> __be32 status;
>
> - if (locks_in_grace(SVC_NET(rqstp)))
> + if (opens_in_grace(SVC_NET(rqstp)))
> return nfserr_grace;
> status = nfsd_unlink(rqstp, &cstate->current_fh, 0,
> remove->rm_name, remove->rm_namelen);
> @@ -846,7 +846,7 @@ nfsd4_rename(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>
> if (!cstate->save_fh.fh_dentry)
> return status;
> - if (locks_in_grace(SVC_NET(rqstp)) &&
> + if (opens_in_grace(SVC_NET(rqstp)) &&
> !(cstate->save_fh.fh_export->ex_flags & NFSEXP_NOSUBTREECHECK))
> return nfserr_grace;
> status = nfsd_rename(rqstp, &cstate->save_fh, rename->rn_sname,
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c94859122e6f..97fe7c8c6a8e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4052,7 +4052,8 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
> case NFS4_OPEN_CLAIM_FH:
> /*
> * Let's not give out any delegations till everyone's
> - * had the chance to reclaim theirs....
> + * had the chance to reclaim theirs, *and* until
> + * NLM locks have all been reclaimed:
> */
> if (locks_in_grace(clp->net))
> goto out_no_deleg;
> @@ -4426,7 +4427,7 @@ check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid,
> {
> if (ONE_STATEID(stateid) && (flags & RD_STATE))
> return nfs_ok;
> - else if (locks_in_grace(net)) {
> + else if (opens_in_grace(net)) {
> /* Answer in remaining cases depends on existence of
> * conflicting state; so we must wait out the grace period. */
> return nfserr_grace;
> @@ -4445,7 +4446,7 @@ check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid,
> static inline int
> grace_disallows_io(struct net *net, struct inode *inode)
> {
> - return locks_in_grace(net) && mandatory_lock(inode);
> + return opens_in_grace(net) && mandatory_lock(inode);
> }
>
> /* Returns true iff a is later than b: */
> @@ -6564,6 +6565,7 @@ nfs4_state_start_net(struct net *net)
> return ret;
> nn->boot_time = get_seconds();
> nn->grace_ended = false;
> + nn->nfsd4_manager.block_opens = true;
> locks_start_grace(net, &nn->nfsd4_manager);
> nfsd4_client_tracking_init(net);
> printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index cc008c338f5a..9a9d314f7b27 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -942,12 +942,18 @@ struct lock_manager_operations {
>
> struct lock_manager {
> struct list_head list;
> + /*
> + * NFSv4 and up also want opens blocked during the grace period;
> + * NLM doesn't care:
> + */
> + bool block_opens;
> };
>
> struct net;
> void locks_start_grace(struct net *, struct lock_manager *);
> void locks_end_grace(struct lock_manager *);
> int locks_in_grace(struct net *);
> +int opens_in_grace(struct net *);
>
> /* that will die - we need it for nfs_lock_info */
> #include <linux/nfs_fs_i.h>
Looks reasonable to me. I can't think of any reason for NLM to block v4
opens.
Acked-by: Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
