[PATCH] Server should allow offset larger than LLONG_MAX at commit procedure

[PATCH] Server should allow offset larger than LLONG_MAX at commit procedure

[Bian Naimeng]

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

Re: [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure

[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.

--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
> 

Re: [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure

[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 <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

Re: [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure

[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.

>   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
> 

Re: [PATCH] Server should allow offset larger than LLONG_MAX at commit procedure

[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