| Message ID | 20140304190442.GE12805@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 04, 2014 at 02:04:42PM -0500, J. Bruce Fields wrote: > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 6d7be3f..eea5ad1 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -404,6 +404,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, > umode_t ftype = 0; > __be32 err; > int host_err; > + bool get_write_count; > int size_change = 0; > > if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE)) > @@ -411,10 +412,18 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, > if (iap->ia_valid & ATTR_SIZE) > ftype = S_IFREG; > > + /* Callers that do fh_verify should do the fh_want_write: */ > + get_write_count = !fhp->fh_dentry; Eww, this is nasty. Given that there are only 6 callers of nfsd_setattr in total, and only half of these might cause size changes I'd rather deal with this properly, e.g. by taking both the fh_verify into the callers. -- 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
On Mon, Mar 10, 2014 at 06:34:51AM -0700, Christoph Hellwig wrote: > On Tue, Mar 04, 2014 at 02:04:42PM -0500, J. Bruce Fields wrote: > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index 6d7be3f..eea5ad1 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -404,6 +404,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, > > umode_t ftype = 0; > > __be32 err; > > int host_err; > > + bool get_write_count; > > int size_change = 0; > > > > if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE)) > > @@ -411,10 +412,18 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, > > if (iap->ia_valid & ATTR_SIZE) > > ftype = S_IFREG; > > > > + /* Callers that do fh_verify should do the fh_want_write: */ > > + get_write_count = !fhp->fh_dentry; > > Eww, this is nasty. Given that there are only 6 callers of nfsd_setattr > in total, and only half of these might cause size changes I'd rather > deal with this properly, e.g. by taking both the fh_verify into the > callers. Maybe so. (Size is irrelevant, though, right? Won't any setattr need an elevated write count?) --b. -- 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
On Mon, Mar 10, 2014 at 03:57:09PM -0400, J. Bruce Fields wrote: > (Size is irrelevant, though, right? Won't any setattr need an elevated > write count?) Indeed. Not sure why I was thinking of truncate as a special case here. -- 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
On Mon, Mar 10, 2014 at 03:57:09PM -0400, J. Bruce Fields wrote: > On Mon, Mar 10, 2014 at 06:34:51AM -0700, Christoph Hellwig wrote: > > On Tue, Mar 04, 2014 at 02:04:42PM -0500, J. Bruce Fields wrote: > > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > > index 6d7be3f..eea5ad1 100644 > > > --- a/fs/nfsd/vfs.c > > > +++ b/fs/nfsd/vfs.c > > > @@ -404,6 +404,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, > > > umode_t ftype = 0; > > > __be32 err; > > > int host_err; > > > + bool get_write_count; > > > int size_change = 0; > > > > > > if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE)) > > > @@ -411,10 +412,18 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, > > > if (iap->ia_valid & ATTR_SIZE) > > > ftype = S_IFREG; > > > > > > + /* Callers that do fh_verify should do the fh_want_write: */ > > > + get_write_count = !fhp->fh_dentry; > > > > Eww, this is nasty. Given that there are only 6 callers of nfsd_setattr > > in total, and only half of these might cause size changes I'd rather > > deal with this properly, e.g. by taking both the fh_verify into the > > callers. > > Maybe so. Gah, I found clearing out my invoice that a) I'd forgotten this, b) I'd already committed and pushed out the patch. And I'd rather leave the fix as is and the cleanup to be done later. But it's not OK to just drop review like that and if you think it warrants reverting or rebasing I can do that. --b. -- 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
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 6d7be3f..eea5ad1 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -404,6 +404,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, umode_t ftype = 0; __be32 err; int host_err; + bool get_write_count; int size_change = 0; if (iap->ia_valid & (ATTR_ATIME | ATTR_MTIME | ATTR_SIZE)) @@ -411,10 +412,18 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap, if (iap->ia_valid & ATTR_SIZE) ftype = S_IFREG; + /* Callers that do fh_verify should do the fh_want_write: */ + get_write_count = !fhp->fh_dentry; + /* Get inode */ err = fh_verify(rqstp, fhp, ftype, accmode); if (err) goto out; + if (get_write_count) { + host_err = fh_want_write(fhp); + if (host_err) + return nfserrno(host_err); + } dentry = fhp->fh_dentry; inode = dentry->d_inode;