Re: [RFC PATCH] rados_cluster: add a "design" manpage

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Jun 14, 2018 at 06:01:27AM -0400, Jeff Layton wrote:
> I know this is a "best effort" sort of thing, but should this be done
> with atomic loads and stores (READ_ONCE/WRITE_ONCE)?

We're just setting it to 1 when a reclaim comes in, and then once a
second checking the result and clearing it.  So yes that's a slightly
sloppy way of checking whether something happens once a second.

I *think* the worst that could happen is something like:

	read 0 from somebody_reclaimed
					reclaim writes 1 to somebody_reclaimed
	write 0 to somebody_reclaimed

and then a client that's reclaiming only very close to the 1-second mark
could get overlooked.

I'm assuming in any reasonable situation that a client can manage at
least 10-100 reclaims a second, and that the race window is probably
several orders of magnitude less than a second (even after taking into
account weird memory ordering).

Would READ/WRITE_ONCE do it right?  I'm not sure it's worth it.

--b.


> 
> >  
> >  	dprintk("NFSD: nfsd4_open filename %.*s op_openowner %p\n",
> >  		(int)open->op_fname.len, open->op_fname.data,
> > @@ -424,6 +425,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  			if (status)
> >  				goto out;
> >  			open->op_openowner->oo_flags |= NFS4_OO_CONFIRMED;
> > +			reclaim = true;
> >  		case NFS4_OPEN_CLAIM_FH:
> >  		case NFS4_OPEN_CLAIM_DELEG_CUR_FH:
> >  			status = do_open_fhandle(rqstp, cstate, open);
> > @@ -452,6 +454,8 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	WARN(status && open->op_created,
> >  	     "nfsd4_process_open2 failed to open newly-created file! status=%u\n",
> >  	     be32_to_cpu(status));
> > +	if (reclaim && !status)
> > +		nn->somebody_reclaimed = true;
> >  out:
> >  	if (resfh && resfh != &cstate->current_fh) {
> >  		fh_dup2(&cstate->current_fh, resfh);
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 59ae65d3eec3..ffe816fe6e89 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4689,6 +4689,28 @@ nfsd4_end_grace(struct nfsd_net *nn)
> >  	 */
> >  }
> >  
> > +/*
> > + * If we've waited a lease period but there are still clients trying to
> > + * reclaim, wait a little longer to give them a chance to finish.
> > + */
> > +static bool clients_still_reclaiming(struct nfsd_net *nn)
> > +{
> > +	unsigned long now = get_seconds();
> > +	unsigned long double_grace_period_end = nn->boot_time +
> > +						2 * nn->nfsd4_lease;
> > +
> > +	if (!nn->somebody_reclaimed)
> > +		return false;
> > +	nn->somebody_reclaimed = false;
> > +	/*
> > +	 * If we've given them *two* lease times to reclaim, and they're
> > +	 * still not done, give up:
> > +	 */
> > +	if (time_after(now, double_grace_period_end))
> > +		return false;
> > +	return true;
> > +}
> > +
> >  static time_t
> >  nfs4_laundromat(struct nfsd_net *nn)
> >  {
> > @@ -4702,6 +4724,11 @@ nfs4_laundromat(struct nfsd_net *nn)
> >  	time_t t, new_timeo = nn->nfsd4_lease;
> >  
> >  	dprintk("NFSD: laundromat service - starting\n");
> > +
> > +	if (clients_still_reclaiming(nn)) {
> > +		new_timeo = 0;
> > +		goto out;
> > +	}
> >  	nfsd4_end_grace(nn);
> >  	INIT_LIST_HEAD(&reaplist);
> >  	spin_lock(&nn->client_lock);
> > @@ -4799,7 +4826,7 @@ nfs4_laundromat(struct nfsd_net *nn)
> >  		posix_unblock_lock(&nbl->nbl_lock);
> >  		free_blocked_lock(nbl);
> >  	}
> > -
> > +out:
> >  	new_timeo = max_t(time_t, new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
> >  	return new_timeo;
> >  }
> > @@ -6049,6 +6076,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	case 0: /* success! */
> >  		nfs4_inc_and_copy_stateid(&lock->lk_resp_stateid, &lock_stp->st_stid);
> >  		status = 0;
> > +		if (lock->lk_reclaim)
> > +			nn->somebody_reclaimed = true;
> >  		break;
> >  	case FILE_LOCK_DEFERRED:
> >  		nbl = NULL;
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index d107b4426f7e..5f22476cf371 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1239,6 +1239,7 @@ static __net_init int nfsd_init_net(struct net *net)
> >  		goto out_idmap_error;
> >  	nn->nfsd4_lease = 90;	/* default lease time */
> >  	nn->nfsd4_grace = 90;
> > +	nn->somebody_reclaimed = false;
> >  	nn->clverifier_counter = prandom_u32();
> >  	nn->clientid_counter = prandom_u32();
> >  
> 
> -- 
> Jeff Layton <jlayton@kernel.org>