Server should allow offset larger than LLONG_MAX at commit procedure

PATCH In review Bian Naimeng 2 participants 5 messages Aug 18, 2010 – Aug 20, 2010

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Bian Naimeng

author

2010-08-18 08:11 UTC #0 lore raw

PATCH 1/1 Server should allow offset larger than LLONG_MAX at commit procedure

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

fs/nfsd/vfs.c +8 -6

index 96360a8..f67fe31 100644
@@ -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 */

Signature · 8 lines

1.6.5.2




-- 
Regards
Bian Naimeng

J. Bruce Fields

2010-08-19 00:18 UTC #1 lore raw

Quoted 2 lines · Bian Naimeng

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.

Quoted 58 lines

Signed-off-by Bian Naimeng <[email protected]>

---
 fs/nfsd/vfs.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

fs/nfsd/vfs.c +8 -6

index 96360a8..f67fe31 100644
@@ -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 */

Signature · 8 lines

1.6.5.2




-- 
Regards
Bian Naimeng

Bian Naimeng

author

2010-08-19 03:40 UTC #2 lore raw

Quoted 9 lines · J. Bruce Fields

Quoted 2 lines · Bian Naimeng
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.

Signature · 2 lines

Regards
Bian Naimeng

Quoted 51 lines

--b.

Quoted 49 lines

Signed-off-by Bian Naimeng <[email protected]>

---
 fs/nfsd/vfs.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

fs/nfsd/vfs.c +8 -6

index 96360a8..f67fe31 100644
@@ -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 */

Signature · 1 line

1.6.5.2

Signature · 2 lines

Regards
Bian Naimeng

J. Bruce Fields

2010-08-19 22:21 UTC #3 lore raw

Quoted 13 lines · Bian Naimeng

Quoted 9 lines · J. Bruce Fields
Quoted 2 lines · Bian Naimeng
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.

Quoted 5 lines

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.

Quoted 1 line

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.

Quoted 67 lines

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.

Signature · 2 lines

Regards
Bian Naimeng

Quoted 51 lines

--b.

Quoted 49 lines

Signed-off-by Bian Naimeng <[email protected]>

---
 fs/nfsd/vfs.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

fs/nfsd/vfs.c +8 -6

index 96360a8..f67fe31 100644
@@ -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 */

Signature · 1 line

1.6.5.2

Signature · 2 lines

Regards
Bian Naimeng

Bian Naimeng

author

2010-08-20 07:22 UTC #4 lore raw

Quoted 17 lines · J. Bruce Fields

Quoted 7 lines · Bian Naimeng
Quoted 5 lines · J. Bruce Fields
Quoted 2 lines · Bian Naimeng
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.
Quoted 8 lines
Quoted 3 lines
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.

Quoted 7 lines

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.

Signature · 2 lines

Regards
Bian Naimeng