* break leases on nfsd rename, setattr, etc. @ 2011-01-12 17:36 J. Bruce Fields 2011-01-12 17:36 ` [PATCH 1/2] nfsd4: break lease on nfsd setattr J. Bruce Fields 2011-01-12 17:36 ` [PATCH 2/2] nfsd: break lease on unlink, link, and rename J. Bruce Fields 0 siblings, 2 replies; 7+ messages in thread From: J. Bruce Fields @ 2011-01-12 17:36 UTC (permalink / raw) To: linux-nfs NFSv4 read delegations should be broken on anything that modifies file metadata, or modifies (by rename, link, or unlink) any of the links that point to the file. It's going to take a while longer to finish vfs patches that solve the problem completely; for now I wonder if an (incomplete) nfsd-only fix would be worthwhile? --b. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] nfsd4: break lease on nfsd setattr 2011-01-12 17:36 break leases on nfsd rename, setattr, etc J. Bruce Fields @ 2011-01-12 17:36 ` J. Bruce Fields 2011-01-12 17:36 ` [PATCH 2/2] nfsd: break lease on unlink, link, and rename J. Bruce Fields 1 sibling, 0 replies; 7+ messages in thread From: J. Bruce Fields @ 2011-01-12 17:36 UTC (permalink / raw) To: linux-nfs; +Cc: J. Bruce Fields Leases (delegations) should really be broken on any metadata change, not just on size change. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/vfs.c | 12 ++++-------- 1 files changed, 4 insertions(+), 8 deletions(-) diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index f8919f5..839ed88 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -374,14 +374,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, goto out; } - /* - * If we are changing the size of the file, then - * we need to break all leases. - */ - host_err = break_lease(inode, O_WRONLY | O_NONBLOCK); - if (host_err) /* ENOMEM or EWOULDBLOCK */ - goto out_nfserr; - host_err = get_write_access(inode); if (host_err) goto out_nfserr; @@ -422,7 +414,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, err = nfserr_notsync; if (!check_guard || guardtime == inode->i_ctime.tv_sec) { + host_err = break_lease(inode, O_WRONLY | O_NONBLOCK); + if (host_err) + goto out_nfserr; fh_lock(fhp); + host_err = notify_change(dentry, iap); err = nfserrno(host_err); fh_unlock(fhp); -- 1.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] nfsd: break lease on unlink, link, and rename 2011-01-12 17:36 break leases on nfsd rename, setattr, etc J. Bruce Fields 2011-01-12 17:36 ` [PATCH 1/2] nfsd4: break lease on nfsd setattr J. Bruce Fields @ 2011-01-12 17:36 ` J. Bruce Fields 2011-01-14 8:21 ` Mi Jinlong 1 sibling, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2011-01-12 17:36 UTC (permalink / raw) To: linux-nfs; +Cc: J. Bruce Fields Any change to any of the links pointing to an entry should also break delegations. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/vfs.c | 27 +++++++++++++++++++++++---- 1 files changed, 23 insertions(+), 4 deletions(-) diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 839ed88..f31321a 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -272,6 +272,13 @@ out: return err; } +static int nfsd_break_lease(struct inode *inode) +{ + if (!S_ISREG(inode->i_mode)) + return 0; + return break_lease(inode, O_WRONLY | O_NONBLOCK); +} + /* * Commit metadata changes to stable storage. */ @@ -414,7 +421,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, err = nfserr_notsync; if (!check_guard || guardtime == inode->i_ctime.tv_sec) { - host_err = break_lease(inode, O_WRONLY | O_NONBLOCK); + host_err = nfsd_break_lease(inode); if (host_err) goto out_nfserr; fh_lock(fhp); @@ -1641,6 +1648,12 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, err = nfserrno(host_err); goto out_dput; } + err = nfserr_noent; + if (!dold->d_inode) + goto out_drop_write; + host_err = nfsd_break_lease(dold->d_inode); + if (host_err) + goto out_drop_write; host_err = vfs_link(dold, dirp, dnew); if (!host_err) { err = nfserrno(commit_metadata(ffhp)); @@ -1652,6 +1665,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, else err = nfserrno(host_err); } +out_drop_write: mnt_drop_write(tfhp->fh_export->ex_path.mnt); out_dput: dput(dnew); @@ -1733,15 +1747,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, if (host_err) goto out_dput_new; + host_err = nfsd_break_lease(odentry->d_inode); + if (host_err) + goto out_drop_write; host_err = vfs_rename(fdir, odentry, tdir, ndentry); if (!host_err) { host_err = commit_metadata(tfhp); if (!host_err) host_err = commit_metadata(ffhp); } - +out_drop_write: mnt_drop_write(ffhp->fh_export->ex_path.mnt); - out_dput_new: dput(ndentry); out_dput_old: @@ -1804,11 +1820,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, if (host_err) goto out_nfserr; + host_err = nfsd_break_lease(rdentry->d_inode); + if (host_err) + goto out_put; if (type != S_IFDIR) host_err = vfs_unlink(dirp, rdentry); else host_err = vfs_rmdir(dirp, rdentry); - +out_put: dput(rdentry); if (!host_err) -- 1.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] nfsd: break lease on unlink, link, and rename 2011-01-12 17:36 ` [PATCH 2/2] nfsd: break lease on unlink, link, and rename J. Bruce Fields @ 2011-01-14 8:21 ` Mi Jinlong 2011-01-14 19:33 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: Mi Jinlong @ 2011-01-14 8:21 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs J. Bruce Fields: > Any change to any of the links pointing to an entry should also break > delegations. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/nfsd/vfs.c | 27 +++++++++++++++++++++++---- > 1 files changed, 23 insertions(+), 4 deletions(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 839ed88..f31321a 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -272,6 +272,13 @@ out: > return err; > } > > +static int nfsd_break_lease(struct inode *inode) > +{ > + if (!S_ISREG(inode->i_mode)) > + return 0; > + return break_lease(inode, O_WRONLY | O_NONBLOCK); Hi Bruce, break_lease will return -EWOULDBLOCK here if break success, not 0. at __break_lease: 1193 */ 1194 int __break_lease(struct inode *inode, unsigned int mode) 1195 { ... ... 1253 if (i_have_this_lease || (mode & O_NONBLOCK)) { 1254 error = -EWOULDBLOCK; 1255 goto out; 1256 } ... ... 1283 out: 1284 unlock_flocks(); 1285 if (!IS_ERR(new_fl)) 1286 locks_free_lock(new_fl); 1287 return error; 1288 } If we just return the error -EWOULDBLOCK (which mapped to -EAGAIN) at nfsd_break_lease(), the NFS request will be drop for -EAGAIN is mapped to nfserr_dropit at nfserrno(). As that, client will hang up about 60s until a retry success. Maybe we should have a check here as: - return break_lease(inode, O_WRONLY | O_NONBLOCK); + int status = break_lease(inode, O_WRONLY | O_NONBLOCK); + return -EWOULDBLOCK == status ? 0 : status; -- ---- thanks Mi Jinlong ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] nfsd: break lease on unlink, link, and rename 2011-01-14 8:21 ` Mi Jinlong @ 2011-01-14 19:33 ` J. Bruce Fields 2011-01-17 2:09 ` Mi Jinlong 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2011-01-14 19:33 UTC (permalink / raw) To: Mi Jinlong; +Cc: J. Bruce Fields, linux-nfs On Fri, Jan 14, 2011 at 04:21:53PM +0800, Mi Jinlong wrote: > > > J. Bruce Fields: > > Any change to any of the links pointing to an entry should also break > > delegations. > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > fs/nfsd/vfs.c | 27 +++++++++++++++++++++++---- > > 1 files changed, 23 insertions(+), 4 deletions(-) > > > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index 839ed88..f31321a 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -272,6 +272,13 @@ out: > > return err; > > } > > > > +static int nfsd_break_lease(struct inode *inode) > > +{ > > + if (!S_ISREG(inode->i_mode)) > > + return 0; > > + return break_lease(inode, O_WRONLY | O_NONBLOCK); > > Hi Bruce, > > break_lease will return -EWOULDBLOCK here if break success, not 0. > at __break_lease: > > 1193 */ > 1194 int __break_lease(struct inode *inode, unsigned int mode) > 1195 { > ... ... > 1253 if (i_have_this_lease || (mode & O_NONBLOCK)) { > 1254 error = -EWOULDBLOCK; > 1255 goto out; > 1256 } > ... ... > 1283 out: > 1284 unlock_flocks(); > 1285 if (!IS_ERR(new_fl)) > 1286 locks_free_lock(new_fl); > 1287 return error; > 1288 } > > If we just return the error -EWOULDBLOCK (which mapped to -EAGAIN) at > nfsd_break_lease(), the NFS request will be drop for -EAGAIN is mapped > to nfserr_dropit at nfserrno(). See my for-2.6.38-incoming branch (especially 062304a815fe and surrounding). I was getting tired of needing special handling for EAGAIN and EWOULDBLOCK, so I removed that mapping and am setting a flag on the rqstp instead to indicate the need to drop a request. Does that look reasonable to you? --b. > As that, client will hang up about 60s > until a retry success. > > Maybe we should have a check here as: > > - return break_lease(inode, O_WRONLY | O_NONBLOCK); > + int status = break_lease(inode, O_WRONLY | O_NONBLOCK); > + return -EWOULDBLOCK == status ? 0 : status; > > -- > ---- > thanks > Mi Jinlong > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] nfsd: break lease on unlink, link, and rename 2011-01-14 19:33 ` J. Bruce Fields @ 2011-01-17 2:09 ` Mi Jinlong 2011-01-17 15:20 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: Mi Jinlong @ 2011-01-17 2:09 UTC (permalink / raw) To: J. Bruce Fields; +Cc: J. Bruce Fields, linux-nfs J. Bruce Fields : > On Fri, Jan 14, 2011 at 04:21:53PM +0800, Mi Jinlong wrote: >> >> J. Bruce Fields: >>> Any change to any of the links pointing to an entry should also break >>> delegations. >>> >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> >>> --- >>> fs/nfsd/vfs.c | 27 +++++++++++++++++++++++---- >>> 1 files changed, 23 insertions(+), 4 deletions(-) >>> >>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >>> index 839ed88..f31321a 100644 >>> --- a/fs/nfsd/vfs.c >>> +++ b/fs/nfsd/vfs.c >>> @@ -272,6 +272,13 @@ out: >>> return err; >>> } >>> >>> +static int nfsd_break_lease(struct inode *inode) >>> +{ >>> + if (!S_ISREG(inode->i_mode)) >>> + return 0; >>> + return break_lease(inode, O_WRONLY | O_NONBLOCK); >> Hi Bruce, >> >> break_lease will return -EWOULDBLOCK here if break success, not 0. >> at __break_lease: >> >> 1193 */ >> 1194 int __break_lease(struct inode *inode, unsigned int mode) >> 1195 { >> ... ... >> 1253 if (i_have_this_lease || (mode & O_NONBLOCK)) { >> 1254 error = -EWOULDBLOCK; >> 1255 goto out; >> 1256 } >> ... ... >> 1283 out: >> 1284 unlock_flocks(); >> 1285 if (!IS_ERR(new_fl)) >> 1286 locks_free_lock(new_fl); >> 1287 return error; >> 1288 } >> >> If we just return the error -EWOULDBLOCK (which mapped to -EAGAIN) at >> nfsd_break_lease(), the NFS request will be drop for -EAGAIN is mapped >> to nfserr_dropit at nfserrno(). > > See my for-2.6.38-incoming branch (especially 062304a815fe and > surrounding). I was getting tired of needing special handling for > EAGAIN and EWOULDBLOCK, so I removed that mapping and am setting a flag > on the rqstp instead to indicate the need to drop a request. > > Does that look reasonable to you? I'm sorry for missing those patch you have post before, I just test the two patch at v2.6.37. ^_^ Those patch is great. According to the discussing before, after the two patch for breaking delegation, a race as you said " One problem is that there's a race: nothing I can see stops anyone from getting another lease after may_delete() but before the delete happens." also exist here. We can't make sure that a new delegation don't be granted between nfsd_break_lease() and unlink file complete or rename complete. --- thanks Mi Jinlong ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] nfsd: break lease on unlink, link, and rename 2011-01-17 2:09 ` Mi Jinlong @ 2011-01-17 15:20 ` J. Bruce Fields 0 siblings, 0 replies; 7+ messages in thread From: J. Bruce Fields @ 2011-01-17 15:20 UTC (permalink / raw) To: Mi Jinlong; +Cc: J. Bruce Fields, linux-nfs On Mon, Jan 17, 2011 at 10:09:00AM +0800, Mi Jinlong wrote: > > > J. Bruce Fields : > > On Fri, Jan 14, 2011 at 04:21:53PM +0800, Mi Jinlong wrote: > >> > >> J. Bruce Fields: > >>> Any change to any of the links pointing to an entry should also break > >>> delegations. > >>> > >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> > >>> --- > >>> fs/nfsd/vfs.c | 27 +++++++++++++++++++++++---- > >>> 1 files changed, 23 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > >>> index 839ed88..f31321a 100644 > >>> --- a/fs/nfsd/vfs.c > >>> +++ b/fs/nfsd/vfs.c > >>> @@ -272,6 +272,13 @@ out: > >>> return err; > >>> } > >>> > >>> +static int nfsd_break_lease(struct inode *inode) > >>> +{ > >>> + if (!S_ISREG(inode->i_mode)) > >>> + return 0; > >>> + return break_lease(inode, O_WRONLY | O_NONBLOCK); > >> Hi Bruce, > >> > >> break_lease will return -EWOULDBLOCK here if break success, not 0. > >> at __break_lease: > >> > >> 1193 */ > >> 1194 int __break_lease(struct inode *inode, unsigned int mode) > >> 1195 { > >> ... ... > >> 1253 if (i_have_this_lease || (mode & O_NONBLOCK)) { > >> 1254 error = -EWOULDBLOCK; > >> 1255 goto out; > >> 1256 } > >> ... ... > >> 1283 out: > >> 1284 unlock_flocks(); > >> 1285 if (!IS_ERR(new_fl)) > >> 1286 locks_free_lock(new_fl); > >> 1287 return error; > >> 1288 } > >> > >> If we just return the error -EWOULDBLOCK (which mapped to -EAGAIN) at > >> nfsd_break_lease(), the NFS request will be drop for -EAGAIN is mapped > >> to nfserr_dropit at nfserrno(). > > > > See my for-2.6.38-incoming branch (especially 062304a815fe and > > surrounding). I was getting tired of needing special handling for > > EAGAIN and EWOULDBLOCK, so I removed that mapping and am setting a flag > > on the rqstp instead to indicate the need to drop a request. > > > > Does that look reasonable to you? > > I'm sorry for missing those patch you have post before, > I just test the two patch at v2.6.37. ^_^ > > Those patch is great. > > According to the discussing before, after the two patch for breaking delegation, > a race as you said " > One problem is that there's a race: nothing I can see stops anyone from > getting another lease after may_delete() but before the delete happens." > also exist here. We can't make sure that a new delegation don't be granted > between nfsd_break_lease() and unlink file complete or rename complete. That's correct. There are also at least a couple more preexisting races in the lease code that can allow conflicting leases and opens. Also, this patch doesn't address the problem of unlinks and renames not done by nfs clients. --b. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-01-17 15:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-12 17:36 break leases on nfsd rename, setattr, etc J. Bruce Fields 2011-01-12 17:36 ` [PATCH 1/2] nfsd4: break lease on nfsd setattr J. Bruce Fields 2011-01-12 17:36 ` [PATCH 2/2] nfsd: break lease on unlink, link, and rename J. Bruce Fields 2011-01-14 8:21 ` Mi Jinlong 2011-01-14 19:33 ` J. Bruce Fields 2011-01-17 2:09 ` Mi Jinlong 2011-01-17 15:20 ` J. Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.