On Fri, Aug 25 2017, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
>
> We currently revoke read delegations on any write open or any operation
> that modifies file data or metadata (including rename, link, and
> unlink). But if the delegation in question is the only read delegation
> and is held by the client performing the operation, that's not really
> necessary.
>
> It's not always possible to prevent this in the NFSv4.0 case, because
> there's not always a way to determine which client an NFSv4.0 delegation
> came from. (In theory we could try to guess this from the transport
> layer, e.g., by assuming all traffic on a given TCP connection comes
> from the same client. But that's not really correct.)
>
> In the NFSv4.1 case the session layer always tells us the client.
>
> This patch should remove such self-conflicts in all cases where we can
> reliably determine the client from the compound.
I don't see any mention of the new DELEG_NO_WAIT, either here or where
the value is defined. That means I have to figure it out for myself?
>
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> ---
> Documentation/filesystems/Locking | 2 ++
> fs/locks.c | 7 ++++++-
> fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++++++++++++
> fs/nfsd/vfs.c | 26 ++++++++++++++++++++-----
> include/linux/fs.h | 27 ++++++++++++++++----------
> 5 files changed, 86 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
> index fe25787ff6d4..8876a32df5ff 100644
> --- a/Documentation/filesystems/Locking
> +++ b/Documentation/filesystems/Locking
> @@ -366,6 +366,7 @@ prototypes:
> int (*lm_grant)(struct file_lock *, struct file_lock *, int);
> void (*lm_break)(struct file_lock *); /* break_lease callback */
> int (*lm_change)(struct file_lock **, int);
> + bool (*lm_breaker_owns_lease)(void *, struct file_lock *);
>
> locking rules:
>
> @@ -376,6 +377,7 @@ lm_notify: yes yes no
> lm_grant: no no no
> lm_break: yes no no
> lm_change yes no no
> +lm_breaker_owns_lease: no no no
>
> [1]: ->lm_compare_owner and ->lm_owner_key are generally called with
> *an* inode->i_lock held. It may not be the i_lock of the inode
> diff --git a/fs/locks.c b/fs/locks.c
> index afefeb4ad6de..a3de5b96c81c 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1404,6 +1404,9 @@ static void time_out_leases(struct inode *inode, struct list_head *dispose)
>
> static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker)
> {
> + if (lease->fl_lmops->lm_breaker_owns_lease && breaker->fl_owner &&
> + lease->fl_lmops->lm_breaker_owns_lease(breaker->fl_owner, lease))
> + return false;
> if ((breaker->fl_flags & FL_LAYOUT) != (lease->fl_flags & FL_LAYOUT))
> return false;
> if ((breaker->fl_flags & FL_DELEG) && (lease->fl_flags & FL_LEASE))
> @@ -1429,6 +1432,7 @@ any_leases_conflict(struct inode *inode, struct file_lock *breaker)
> /**
> * __break_lease - revoke all outstanding leases on file
> * @inode: the inode of the file to return
> + * @who: if an nfs client is breaking, which client it is
> * @mode: O_RDONLY: break only write leases; O_WRONLY or O_RDWR:
> * break all leases
> * @type: FL_LEASE: break leases and delegations; FL_DELEG: break
> @@ -1439,7 +1443,7 @@ any_leases_conflict(struct inode *inode, struct file_lock *breaker)
> * a call to open() or truncate(). This function can sleep unless you
> * specified %O_NONBLOCK to your open().
> */
> -int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
> +int __break_lease(struct inode *inode, void *who, unsigned int mode, unsigned int type)
> {
> int error = 0;
> struct file_lock_context *ctx;
> @@ -1452,6 +1456,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
> if (IS_ERR(new_fl))
> return PTR_ERR(new_fl);
> new_fl->fl_flags = type;
> + new_fl->fl_owner = who;
When I saw this, I thought: "Shouldn't 'who' be 'fl_owner_t' rather that
'void*'".
Then I saw
/* legacy typedef, should eventually be removed */
typedef void *fl_owner_t;
Maybe you could do the world a favor and remove fl_owner_t in a
preliminary patch :-)
And it is kind-a weird that the "who" you pass to break_lease() is
different from the owner passed to vfs_setlease().
>
> /* typically we will check that ctx is non-NULL before calling */
> ctx = smp_load_acquire(&inode->i_flctx);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 0c04f81aa63b..fb15efcc4e08 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3825,6 +3825,45 @@ nfsd_break_deleg_cb(struct file_lock *fl)
> return ret;
> }
>
> +static struct nfs4_client *nfsd4_client_from_rqst(struct svc_rqst *rqst)
> +{
> + struct nfsd4_compoundres *resp;
> +
> + /*
> + * In case it's possible we could be called from NLM or ACL
> + * code?:
> + */
> + if (rqst->rq_prog != NFS_PROGRAM)
> + return NULL;
> + if (rqst->rq_vers != 4)
> + return NULL;
> + resp = rqst->rq_resp;
> + return resp->cstate.clp;
> +}
> +
> +static bool nfsd_breaker_owns_lease(void *who, struct file_lock *fl)
> +{
> + struct svc_rqst *rqst = who;
> + struct nfs4_client *cl;
> + struct nfs4_delegation *dl;
> + struct nfs4_file *fi = (struct nfs4_file *)fl->fl_owner;
> + bool ret = true;
> +
> + cl = nfsd4_client_from_rqst(rqst);
> + if (!cl)
> + return false;
> +
> + spin_lock(&fi->fi_lock);
> + list_for_each_entry(dl, &fi->fi_delegations, dl_perfile) {
> + if (dl->dl_stid.sc_client != cl) {
> + ret = false;
> + break;
> + }
> + }
> + spin_unlock(&fi->fi_lock);
> + return ret;
> +}
You haven't provided any documentation telling me what the
"lm_breaker_owns_lease" callback is meant to do.
So I look at this one piece of sample code -- it seems to compute:
not (an other client owns lease)
rather than
(this client owns lease)
which is what I would have expected.
Given that any_leases_conflict() already loops over all leases, does
nfsd_breaker_owns_lease() need to loop as well?
Or does nfsd only take a single lease to cover all the delegations to
all the clients?
... hmmm, yes that does seem to be how nfsd works.
Would this all turn out to be easier if nfsd took a separate lease for
each client? What would be the cost of that?
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
