* [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure @ 2010-08-18 8:09 Bian Naimeng 2010-08-19 0:16 ` J. Bruce Fields 0 siblings, 1 reply; 5+ messages in thread From: Bian Naimeng @ 2010-08-18 8:09 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs, Christoph Hellwig When offset larger than LLONG_MAX, it's better to sync all the data of file than return nfserr_inval. Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com> --- fs/nfsd/vfs.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 96360a8..f67fe31 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1174,20 +1174,23 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, { struct file *file; loff_t end = LLONG_MAX; - __be32 err = nfserr_inval; + __be32 err; + + if ((u64)count > ~(u64)offset) + return nfserr_inval; if (offset < 0) - goto out; - if (count != 0) { + offset = 0; + else if (count != 0) { end = offset + (loff_t)count - 1; if (end < offset) - goto out; + end = LLONG_MAX; } err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_WRITE|NFSD_MAY_NOT_BREAK_LEASE, &file); if (err) - goto out; + return err; if (EX_ISSYNC(fhp->fh_export)) { int err2 = vfs_fsync_range(file, offset, end, 0); @@ -1198,7 +1201,6 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, } nfsd_close(file); -out: return err; } #endif /* CONFIG_NFSD_V3 */ -- 1.6.5.2 -- Regards Bian Naimeng ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure 2010-08-18 8:09 [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure Bian Naimeng @ 2010-08-19 0:16 ` J. Bruce Fields 2010-08-19 3:38 ` Bian Naimeng 0 siblings, 1 reply; 5+ messages in thread From: J. Bruce Fields @ 2010-08-19 0:16 UTC (permalink / raw) To: Bian Naimeng; +Cc: Trond Myklebust, linux-nfs, Christoph Hellwig On Wed, Aug 18, 2010 at 04:09:28PM +0800, Bian Naimeng wrote: > When offset larger than LLONG_MAX, it's better to sync all the data of file > than return nfserr_inval. I believe the current behavior is correct. See http://marc.info/?l=linux-nfs&m=128200558207974&w=2 for a pynfs-side fix. --b. > > Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com> > > --- > fs/nfsd/vfs.c | 14 ++++++++------ > 1 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 96360a8..f67fe31 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1174,20 +1174,23 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, > { > struct file *file; > loff_t end = LLONG_MAX; > - __be32 err = nfserr_inval; > + __be32 err; > + > + if ((u64)count > ‾(u64)offset) > + return nfserr_inval; > > if (offset < 0) > - goto out; > - if (count != 0) { > + offset = 0; > + else if (count != 0) { > end = offset + (loff_t)count - 1; > if (end < offset) > - goto out; > + end = LLONG_MAX; > } > > err = nfsd_open(rqstp, fhp, S_IFREG, > NFSD_MAY_WRITE|NFSD_MAY_NOT_BREAK_LEASE, &file); > if (err) > - goto out; > + return err; > if (EX_ISSYNC(fhp->fh_export)) { > int err2 = vfs_fsync_range(file, offset, end, 0); > > @@ -1198,7 +1201,6 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, > } > > nfsd_close(file); > -out: > return err; > } > #endif /* CONFIG_NFSD_V3 */ > -- > 1.6.5.2 > > > > > -- > Regards > Bian Naimeng > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure 2010-08-19 0:16 ` J. Bruce Fields @ 2010-08-19 3:38 ` Bian Naimeng 2010-08-19 22:19 ` J. Bruce Fields 0 siblings, 1 reply; 5+ messages in thread From: Bian Naimeng @ 2010-08-19 3:38 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs, Christoph Hellwig J. Bruce Fields 写道: > On Wed, Aug 18, 2010 at 04:09:28PM +0800, Bian Naimeng wrote: >> When offset larger than LLONG_MAX, it's better to sync all the data of file >> than return nfserr_inval. > > I believe the current behavior is correct. > > See http://marc.info/?l=linux-nfs&m=128200558207974&w=2 for a pynfs-side > fix. > Thanks. But why we must return nfserr_inval at nfs layer, the commitarg.offset and writearg.offset are the U64 type, i think maybe we should set the vfs as the authority not nfs for whether the offset is valid when it over 2^63-1. Maybe some day, VFS can support lager offset, we need modify our nfs code again to fit it. So i think which check offset at nfsd4_write and nfsd4_commit is unnecessary, looks like VFS can return EINVAL for it. -- Regards Bian Naimeng > --b. > >> Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com> >> >> --- >> fs/nfsd/vfs.c | 14 ++++++++------ >> 1 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >> index 96360a8..f67fe31 100644 >> --- a/fs/nfsd/vfs.c >> +++ b/fs/nfsd/vfs.c >> @@ -1174,20 +1174,23 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, >> { >> struct file *file; >> loff_t end = LLONG_MAX; >> - __be32 err = nfserr_inval; >> + __be32 err; >> + >> + if ((u64)count > ~(u64)offset) >> + return nfserr_inval; >> >> if (offset < 0) >> - goto out; >> - if (count != 0) { >> + offset = 0; >> + else if (count != 0) { >> end = offset + (loff_t)count - 1; >> if (end < offset) >> - goto out; >> + end = LLONG_MAX; >> } >> >> err = nfsd_open(rqstp, fhp, S_IFREG, >> NFSD_MAY_WRITE|NFSD_MAY_NOT_BREAK_LEASE, &file); >> if (err) >> - goto out; >> + return err; >> if (EX_ISSYNC(fhp->fh_export)) { >> int err2 = vfs_fsync_range(file, offset, end, 0); >> >> @@ -1198,7 +1201,6 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, >> } >> >> nfsd_close(file); >> -out: >> return err; >> } >> #endif /* CONFIG_NFSD_V3 */ >> -- >> 1.6.5.2 -- Regards Bian Naimeng ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure 2010-08-19 3:38 ` Bian Naimeng @ 2010-08-19 22:19 ` J. Bruce Fields 2010-08-20 7:20 ` Bian Naimeng 0 siblings, 1 reply; 5+ messages in thread From: J. Bruce Fields @ 2010-08-19 22:19 UTC (permalink / raw) To: Bian Naimeng; +Cc: Trond Myklebust, linux-nfs, Christoph Hellwig On Thu, Aug 19, 2010 at 11:38:33AM +0800, Bian Naimeng wrote: > > > J. Bruce Fields 写道: > > On Wed, Aug 18, 2010 at 04:09:28PM +0800, Bian Naimeng wrote: > >> When offset larger than LLONG_MAX, it's better to sync all the data of file > >> than return nfserr_inval. > > > > I believe the current behavior is correct. > > > > See http://marc.info/?l=linux-nfs&m=128200558207974&w=2 for a pynfs-side > > fix. > > > > Thanks. > > But why we must return nfserr_inval at nfs layer, the commitarg.offset and > writearg.offset are the U64 type, i think maybe we should set the vfs as the > authority not nfs for whether the offset is valid when it over 2^63-1. Hm, good question. I took a quick look at vfs_fsync_range() and its other callers but couldn't immediately tell whether checking the validity of the range is its responsibility or the caller's. If you can demonstrate that vfs_fsync_range() takes responsibility for the range-checking, then I'd be fine with removing the checks here. > Maybe some day, VFS can support lager offset, But then I think LLONG_MAX and/or the definition of loff_t would have to change. --b. > we need modify our nfs code again > to fit it. So i think which check offset at nfsd4_write and nfsd4_commit is > unnecessary, looks like VFS can return EINVAL for it. > > -- > Regards > Bian Naimeng > > > --b. > > > >> Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com> > >> > >> --- > >> fs/nfsd/vfs.c | 14 ++++++++------ > >> 1 files changed, 8 insertions(+), 6 deletions(-) > >> > >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > >> index 96360a8..f67fe31 100644 > >> --- a/fs/nfsd/vfs.c > >> +++ b/fs/nfsd/vfs.c > >> @@ -1174,20 +1174,23 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, > >> { > >> struct file *file; > >> loff_t end = LLONG_MAX; > >> - __be32 err = nfserr_inval; > >> + __be32 err; > >> + > >> + if ((u64)count > ‾(u64)offset) > >> + return nfserr_inval; > >> > >> if (offset < 0) > >> - goto out; > >> - if (count != 0) { > >> + offset = 0; > >> + else if (count != 0) { > >> end = offset + (loff_t)count - 1; > >> if (end < offset) > >> - goto out; > >> + end = LLONG_MAX; > >> } > >> > >> err = nfsd_open(rqstp, fhp, S_IFREG, > >> NFSD_MAY_WRITE|NFSD_MAY_NOT_BREAK_LEASE, &file); > >> if (err) > >> - goto out; > >> + return err; > >> if (EX_ISSYNC(fhp->fh_export)) { > >> int err2 = vfs_fsync_range(file, offset, end, 0); > >> > >> @@ -1198,7 +1201,6 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, > >> } > >> > >> nfsd_close(file); > >> -out: > >> return err; > >> } > >> #endif /* CONFIG_NFSD_V3 */ > >> -- > >> 1.6.5.2 > > > -- > Regards > Bian Naimeng > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure 2010-08-19 22:19 ` J. Bruce Fields @ 2010-08-20 7:20 ` Bian Naimeng 0 siblings, 0 replies; 5+ messages in thread From: Bian Naimeng @ 2010-08-20 7:20 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Trond Myklebust, linux-nfs, Christoph Hellwig J. Bruce Fields 写道: > On Thu, Aug 19, 2010 at 11:38:33AM +0800, Bian Naimeng wrote: >> >> J. Bruce Fields 写道: >>> On Wed, Aug 18, 2010 at 04:09:28PM +0800, Bian Naimeng wrote: >>>> When offset larger than LLONG_MAX, it's better to sync all the data of file >>>> than return nfserr_inval. >>> I believe the current behavior is correct. >>> >>> See http://marc.info/?l=linux-nfs&m=128200558207974&w=2 for a pynfs-side >>> fix. >>> >> Thanks. >> >> But why we must return nfserr_inval at nfs layer, the commitarg.offset and >> writearg.offset are the U64 type, i think maybe we should set the vfs as the >> authority not nfs for whether the offset is valid when it over 2^63-1. > > Hm, good question. I took a quick look at vfs_fsync_range() and its > other callers but couldn't immediately tell whether checking the > validity of the range is its responsibility or the caller's. > > If you can demonstrate that vfs_fsync_range() takes responsibility for > the range-checking, then I'd be fine with removing the checks here. > It looks like that vfs_fsync_range has not the range-checking, but i think vfs_fsync_range should support the function of range-checking. And NFSv4 write procedure will do the range-checking at rw_verify_area. -- Regards Bian Naimeng ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-08-20 7:22 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-18 8:09 [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure Bian Naimeng 2010-08-19 0:16 ` J. Bruce Fields 2010-08-19 3:38 ` Bian Naimeng 2010-08-19 22:19 ` J. Bruce Fields 2010-08-20 7:20 ` Bian Naimeng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).