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 <[email protected]>
---
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
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 <[email protected]>
>>
>> ---
>> 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
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
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 <[email protected]>
>
> ---
> 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
>
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 <[email protected]>
> >>
> >> ---
> >> 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
>