| Message ID | 20170901161809.GA22140@parsley.fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 01 2017, J. Bruce Fields wrote: >> >> nfsd would need to find that delegation, prevent further delegations >> being handed out, and check that there aren't already conflicting >> delegations. If there are conflicts, recall them. Once there are no >> conflicting delegations, make the vfs_ request. > > The way that we currently serialize setting, unsetting, and breaking > delegations is by locks on the delegated inodes which aren't taken till > deeper in the vfs code. Do we? I can see nfs4_set_delegation adding a new delegation for a new client without entering the vfs at all if there is already a lease held. If there isn't a lease already, vfs_setlease() is called, which doesn't its own internal locking of course. Much the same applies to unsetting delegations. Breaking delegations involves nfsd_break_deleg_cb() which has a comment that it is called with i_lock held.... that seems to be used to be sure that it is safe to a reference to the delegation state id. Is that the only dependency on the vfs locking, or did I miss something? > > I guess you're suggesting adding a second mechanism to prevent > delegations being given out on the inode. We could add an atomic > counter taken by each nfsd breaker while it's in progress. Hrm. Something like that. We would also need to be able to look up an nfs4_file by inode (why *are* they hashed by file handle??) and add some wait queue somewhere so the breaker could wait for a delegation to be returned. My big-picture point is that any complexity created by NFSD's choice to merge delegations to multiple clients into a single vfs-level delegation should be handled by NFSD, and not imposed on the VFS. It certainly makes sense for the VFS to understand that certain operations are being performed by an fl_owner_t, and to allow delegations to that owner to remain. It doesn't make as much sense for the VFS to understand that there is a finer granularity of ownership than the one that it already supports. Thanks, NeilBrown
On Mon, Sep 04, 2017 at 02:52:43PM +1000, NeilBrown wrote: > On Fri, Sep 01 2017, J. Bruce Fields wrote: > > >> > >> nfsd would need to find that delegation, prevent further delegations > >> being handed out, and check that there aren't already conflicting > >> delegations. If there are conflicts, recall them. Once there are no > >> conflicting delegations, make the vfs_ request. > > > > The way that we currently serialize setting, unsetting, and breaking > > delegations is by locks on the delegated inodes which aren't taken till > > deeper in the vfs code. > > Do we? > I can see nfs4_set_delegation adding a new delegation for a new client > without entering the vfs at all if there is already a lease held. By "delegations", I meant locks of type FL_DELEG. But even then I was wrong, apologies. There is an inode_trylock in generic_add_lease that will prevent any new delegations from being given while the inode's locked. > If there isn't a lease already, vfs_setlease() is called, which doesn't > its own internal locking of course. Much the same applies to unsetting > delegations. > Breaking delegations involves nfsd_break_deleg_cb() which has a comment > that it is called with i_lock held.... that seems to be used to > be sure that it is safe to a reference to the delegation state id. > Is that the only dependency on the vfs locking, or did I miss something? > > > > > I guess you're suggesting adding a second mechanism to prevent > > delegations being given out on the inode. We could add an atomic > > counter taken by each nfsd breaker while it's in progress. Hrm. > > Something like that. > We would also need to be able to look up an nfs4_file by inode (why > *are* they hashed by file handle??) Grepping the logs.... That was ca9432178378 "nfsd: Use the filehandle to look up the struct nfs4_file instead of inode" which doesn't give a full justification. Later commits suggest it might be about keeping nfsv4 state in many-to-one filehandle->inode cases (spec requirement, I believe) and preventing the nfs4_file from pinning the inode (not seeing immediately why that was an issue). Anyway, I can't think of a reason why hashing the filehandle's a problem. > and add some wait queue somewhere > so the breaker could wait for a delegation to be returned. In the nfsd case we're just returning to the client immediately, so that's not really necessary, though maybe it could be useful. > My big-picture point is that any complexity created by NFSD's choice to > merge delegations to multiple clients into a single vfs-level delegation > should be handled by NFSD, and not imposed on the VFS. > It certainly makes sense for the VFS to understand that certain > operations are being performed by an fl_owner_t, and to allow > delegations to that owner to remain. It doesn't make as much sense for > the VFS to understand that there is a finer granularity of ownership > than the one that it already supports. Fair enough, I'll think about that. --b.
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index fb15efcc4e08..b50a7492f47f 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3825,7 +3825,7 @@ nfsd_break_deleg_cb(struct file_lock *fl) return ret; } -static struct nfs4_client *nfsd4_client_from_rqst(struct svc_rqst *rqst) +void *nfsd4_client_from_rqst(struct svc_rqst *rqst) { struct nfsd4_compoundres *resp; @@ -3843,19 +3843,14 @@ static struct nfs4_client *nfsd4_client_from_rqst(struct svc_rqst *rqst) 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) { + if (dl->dl_stid.sc_client != who) { ret = false; break; } diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h index b9c538ab7a59..f7819ce6c817 100644 --- a/fs/nfsd/nfsd.h +++ b/fs/nfsd/nfsd.h @@ -125,6 +125,7 @@ void nfs4_reset_lease(time_t leasetime); int nfs4_reset_recoverydir(char *recdir); char * nfs4_recoverydir(void); bool nfsd4_spo_must_allow(struct svc_rqst *rqstp); +void *nfsd4_client_from_rqst(struct svc_rqst *rqst); #else static inline int nfsd4_init_slabs(void) { return 0; } static inline void nfsd4_free_slabs(void) { } @@ -139,6 +140,7 @@ static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp) { return false; } +void *nfsd4_client_from_rqst(struct svc_rqst *rqst) { return NULL; } #endif /* diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index fe62bc744143..fc9b9ad1d444 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -395,8 +395,8 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, bool get_write_count; bool size_change = (iap->ia_valid & ATTR_SIZE); struct deleg_break_ctl deleg_break_ctl = { - .delegated_inode = DELEG_NO_WAIT, - .who = rqstp + .delegated_inode = DELEG_NO_WAIT, + .who = nfsd4_client_from_rqst(rqstp), }; if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE)) @@ -1559,7 +1559,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, int host_err; struct deleg_break_ctl deleg_break_ctl = { .delegated_inode = DELEG_NO_WAIT, - .who = rqstp + .who = nfsd4_client_from_rqst(rqstp), }; err = fh_verify(rqstp, ffhp, S_IFDIR, NFSD_MAY_CREATE); @@ -1636,7 +1636,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, int host_err; struct deleg_break_ctl deleg_break_ctl = { .delegated_inode = DELEG_NO_WAIT, - .who = rqstp + .who = nfsd4_client_from_rqst(rqstp), }; err = fh_verify(rqstp, ffhp, S_IFDIR, NFSD_MAY_REMOVE); @@ -1736,7 +1736,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, int host_err; struct deleg_break_ctl deleg_break_ctl = { .delegated_inode = DELEG_NO_WAIT, - .who = rqstp + .who = nfsd4_client_from_rqst(rqstp), }; err = nfserr_acces;