2022-01-13 00:06:16

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH RFC v9 2/2] nfsd: Initial implementation of NFSv4 Courteous Server

On Mon, Jan 10, 2022 at 10:50:53AM -0800, Dai Ngo wrote:
> @@ -4912,7 +4987,128 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
> return nfsd_setattr(rqstp, fh, &iattr, 0, (time64_t)0);
> }
>
> -static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
> +static bool
> +__nfs4_check_access_deny_bmap(struct nfs4_ol_stateid *stp, u32 access,
> + bool share_access)
> +{
> + if (share_access) {
> + if (!stp->st_deny_bmap)
> + return false;
> +
> + if ((stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_BOTH)) ||
> + (access & NFS4_SHARE_ACCESS_READ &&
> + stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_READ)) ||
> + (access & NFS4_SHARE_ACCESS_WRITE &&
> + stp->st_deny_bmap & (1 << NFS4_SHARE_DENY_WRITE))) {
> + return true;
> + }
> + return false;
> + }
> + if ((access & NFS4_SHARE_DENY_BOTH) ||
> + (access & NFS4_SHARE_DENY_READ &&
> + stp->st_access_bmap & (1 << NFS4_SHARE_ACCESS_READ)) ||
> + (access & NFS4_SHARE_DENY_WRITE &&
> + stp->st_access_bmap & (1 << NFS4_SHARE_ACCESS_WRITE))) {
> + return true;
> + }
> + return false;
> +}
> +
> +/*
> + * Check all files belong to the specified client to determine if there is
> + * any conflict with the specified access_mode/deny_mode of the file 'fp.
> + *
> + * If share_access is true then 'access' is the access mode. Check if
> + * this access mode conflicts with current deny mode of the file.
> + *
> + * If share_access is false then 'access' the deny mode. Check if
> + * this deny mode conflicts with current access mode of the file.
> + */
> +static bool
> +nfs4_check_access_deny_bmap(struct nfs4_client *clp, struct nfs4_file *fp,
> + struct nfs4_ol_stateid *st, u32 access, bool share_access)
> +{
> + int i;
> + struct nfs4_openowner *oo;
> + struct nfs4_stateowner *so, *tmp;
> + struct nfs4_ol_stateid *stp, *stmp;
> +
> + spin_lock(&clp->cl_lock);
> + for (i = 0; i < OWNER_HASH_SIZE; i++) {
> + list_for_each_entry_safe(so, tmp, &clp->cl_ownerstr_hashtbl[i],
> + so_strhash) {
> + if (!so->so_is_open_owner)
> + continue;
> + oo = openowner(so);
> + list_for_each_entry_safe(stp, stmp,
> + &oo->oo_owner.so_stateids, st_perstateowner) {
> + if (stp == st || stp->st_stid.sc_file != fp)
> + continue;
> + if (__nfs4_check_access_deny_bmap(stp, access,
> + share_access)) {
> + spin_unlock(&clp->cl_lock);
> + return true;
> + }
> + }
> + }
> + }
> + spin_unlock(&clp->cl_lock);
> + return false;
> +}
> +
> +/*
> + * This function is called to check whether nfserr_share_denied should
> + * be returning to client.
> + *
> + * access: is op_share_access if share_access is true.
> + * Check if access mode, op_share_access, would conflict with
> + * the current deny mode of the file 'fp'.
> + * access: is op_share_deny if share_access is true.
> + * Check if the deny mode, op_share_deny, would conflict with
> + * current access of the file 'fp'.
> + * stp: skip checking this entry.
> + *
> + * Function returns:
> + * true - access/deny mode conflict with courtesy client(s).
> + * Caller to return nfserr_jukebox while client(s) being expired.
> + * false - access/deny mode conflict with non-courtesy client.
> + * Caller to return nfserr_share_denied to client.
> + */
> +static bool
> +nfs4_conflict_courtesy_clients(struct svc_rqst *rqstp, struct nfs4_file *fp,
> + struct nfs4_ol_stateid *stp, u32 access, bool share_access)
> +{
> + struct nfs4_client *cl;
> + bool conflict = false;
> + int async_cnt = 0;
> + struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> +
> + spin_lock(&nn->client_lock);
> + list_for_each_entry(cl, &nn->client_lru, cl_lru) {

This means we're manually searching through all the state of every
client each time we find a share conflict.

Well, maybe I'm OK with that. Share conflicts are not the normal case.
(I'm not sure anyone actually uses them.) So I guess I don't care if
that case is slow.

It's kind of a lot of code, though, I wish there were a way to simplify.

--b.

> + if (!nfs4_check_access_deny_bmap(cl, fp, stp, access, share_access))
> + continue;
> + spin_lock(&cl->cl_cs_lock);
> + if (test_bit(NFSD4_COURTESY_CLIENT, &cl->cl_flags)) {
> + set_bit(NFSD4_DESTROY_COURTESY_CLIENT, &cl->cl_flags);
> + async_cnt++;
> + spin_unlock(&cl->cl_cs_lock);
> + continue;
> + }
> + /* conflict with non-courtesy client */
> + spin_unlock(&cl->cl_cs_lock);
> + conflict = false;
> + break;
> + }
> + spin_unlock(&nn->client_lock);
> + if (async_cnt) {
> + mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
> + conflict = true;
> + }
> + return conflict;
> +}
> +
> +static __be32
> +nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
> struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp,
> struct nfsd4_open *open)
> {
> @@ -4931,6 +5127,11 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
> status = nfs4_file_check_deny(fp, open->op_share_deny);
> if (status != nfs_ok) {
> spin_unlock(&fp->fi_lock);
> + if (status != nfserr_share_denied)
> + goto out;
> + if (nfs4_conflict_courtesy_clients(rqstp, fp,
> + stp, open->op_share_deny, false))
> + status = nfserr_jukebox;
> goto out;
> }
>
> @@ -4938,6 +5139,11 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
> status = nfs4_file_get_access(fp, open->op_share_access);
> if (status != nfs_ok) {
> spin_unlock(&fp->fi_lock);
> + if (status != nfserr_share_denied)
> + goto out;
> + if (nfs4_conflict_courtesy_clients(rqstp, fp,
> + stp, open->op_share_access, true))
> + status = nfserr_jukebox;
> goto out;
> }
>