Hi,Bruce
I ran pynfs on the RHEL7.4GA with different kernels(3.10.0-830.el7,4.15). The testLargeData:WRT5 failed with broken pipe. I investigated pynfs code and got the following questions.
1.The intent of WRT5
To check whether the server could handle a too-large value over NFSSVC_MAXBLKSIZE.
2.The expected procedure when test gets passed
The server could write down the oversize data successfully sent by client and return NFS4_OK.
Then, client could read back the data from server.
(Actually, the test failed with broken pipe and did not return packet information.)
3.The nfs server's standrd procedure of handling oversize data
It seems that the related error is not defined in the RFC3530. Whether the server should return any NFSERR before ceasing receving ?
Thanks for check the above questions.
Best regards!
--
Lu Xinyu
--
On Fri, Feb 09, 2018 at 09:27:00AM +0800, Lu Xinyu wrote:
> I ran pynfs on the RHEL7.4GA with different
> kernels(3.10.0-830.el7,4.15). The testLargeData:WRT5 failed with
> broken pipe. I investigated pynfs code and got the following
> questions.
>
> 1.The intent of WRT5 To check whether the server could handle a
> too-large value over NFSSVC_MAXBLKSIZE.
>
> 2.The expected procedure when test gets passed The server could write
> down the oversize data successfully sent by client and return NFS4_OK.
> Then, client could read back the data from server.
Yes, that test is definitely wrong. It might happen to work on servers
that support larger IO size, but the Linux server doesn't. And, anyway,
it's perfectly legal for a server to only support lower read/write size.
So, the test needs check the server's advertised maximum read and write
sizes, and then either:
1) use that maximum size, and expect success, or
2) use a larger size and expect some sort of error.
> (Actually, the test failed with broken pipe and did not return packet
> information.)
>
> 3.The nfs server's standrd procedure of handling oversize data It
> seems that the related error is not defined in the RFC3530. Whether
> the server should return any NFSERR before ceasing receving ?
I'm not sure if the RFC's specify the behavior here. I'll go do some
research. I have a feeling they don't. In that case I think the best
option will be 1) above.
--b.
T24gRnJpLCAyMDE4LTAyLTA5IGF0IDA5OjQ5IC0wNTAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6
DQo+IE9uIEZyaSwgRmViIDA5LCAyMDE4IGF0IDA5OjI3OjAwQU0gKzA4MDAsIEx1IFhpbnl1IHdy
b3RlOg0KPiA+IEkgcmFuIHB5bmZzIG9uIHRoZSBSSEVMNy40R0Egd2l0aCBkaWZmZXJlbnQNCj4g
PiBrZXJuZWxzKDMuMTAuMC04MzAuZWw3LDQuMTUpLiBUaGUgdGVzdExhcmdlRGF0YTpXUlQ1IGZh
aWxlZCB3aXRoDQo+ID4gYnJva2VuIHBpcGUuIEkgaW52ZXN0aWdhdGVkIHB5bmZzIGNvZGUgYW5k
IGdvdCB0aGUgZm9sbG93aW5nDQo+ID4gcXVlc3Rpb25zLg0KPiA+IA0KPiA+IDEuVGhlIGludGVu
dCBvZiBXUlQ1IFRvIGNoZWNrIHdoZXRoZXIgdGhlIHNlcnZlciBjb3VsZCBoYW5kbGUgYQ0KPiA+
IHRvby1sYXJnZSB2YWx1ZSBvdmVyIE5GU1NWQ19NQVhCTEtTSVpFLg0KPiA+IA0KPiA+IDIuVGhl
IGV4cGVjdGVkIHByb2NlZHVyZSB3aGVuIHRlc3QgZ2V0cyBwYXNzZWQgVGhlIHNlcnZlciBjb3Vs
ZA0KPiA+IHdyaXRlDQo+ID4gZG93biB0aGUgb3ZlcnNpemUgZGF0YSBzdWNjZXNzZnVsbHkgc2Vu
dCBieSBjbGllbnQgYW5kIHJldHVybg0KPiA+IE5GUzRfT0suDQo+ID4gVGhlbiwgY2xpZW50IGNv
dWxkIHJlYWQgYmFjayB0aGUgZGF0YSBmcm9tIHNlcnZlci4NCj4gDQo+IFllcywgdGhhdCB0ZXN0
IGlzIGRlZmluaXRlbHkgd3JvbmcuICBJdCBtaWdodCBoYXBwZW4gdG8gd29yayBvbg0KPiBzZXJ2
ZXJzDQo+IHRoYXQgc3VwcG9ydCBsYXJnZXIgSU8gc2l6ZSwgYnV0IHRoZSBMaW51eCBzZXJ2ZXIg
ZG9lc24ndC4gIEFuZCwNCj4gYW55d2F5LA0KPiBpdCdzIHBlcmZlY3RseSBsZWdhbCBmb3IgYSBz
ZXJ2ZXIgdG8gb25seSBzdXBwb3J0IGxvd2VyIHJlYWQvd3JpdGUNCj4gc2l6ZS4NCj4gDQo+IFNv
LCB0aGUgdGVzdCBuZWVkcyBjaGVjayB0aGUgc2VydmVyJ3MgYWR2ZXJ0aXNlZCBtYXhpbXVtIHJl
YWQgYW5kDQo+IHdyaXRlDQo+IHNpemVzLCBhbmQgdGhlbiBlaXRoZXI6DQo+IA0KPiAJMSkgdXNl
IHRoYXQgbWF4aW11bSBzaXplLCBhbmQgZXhwZWN0IHN1Y2Nlc3MsIG9yDQo+IAkyKSB1c2UgYSBs
YXJnZXIgc2l6ZSBhbmQgZXhwZWN0IHNvbWUgc29ydCBvZiBlcnJvci4NCj4gDQo+ID4gKEFjdHVh
bGx5LCB0aGUgdGVzdCBmYWlsZWQgd2l0aCBicm9rZW4gcGlwZSBhbmQgZGlkIG5vdCByZXR1cm4N
Cj4gPiBwYWNrZXQNCj4gPiBpbmZvcm1hdGlvbi4pDQo+ID4gDQo+ID4gMy5UaGUgbmZzIHNlcnZl
cidzIHN0YW5kcmQgcHJvY2VkdXJlIG9mIGhhbmRsaW5nIG92ZXJzaXplIGRhdGEgSXQNCj4gPiBz
ZWVtcyB0aGF0IHRoZSByZWxhdGVkIGVycm9yIGlzIG5vdCBkZWZpbmVkIGluIHRoZSBSRkMzNTMw
LiBXaGV0aGVyDQo+ID4gdGhlIHNlcnZlciBzaG91bGQgcmV0dXJuIGFueSBORlNFUlIgYmVmb3Jl
IGNlYXNpbmcgcmVjZXZpbmcgPw0KPiANCj4gSSdtIG5vdCBzdXJlIGlmIHRoZSBSRkMncyBzcGVj
aWZ5IHRoZSBiZWhhdmlvciBoZXJlLiAgSSdsbCBnbyBkbyBzb21lDQo+IHJlc2VhcmNoLiAgSSBo
YXZlIGEgZmVlbGluZyB0aGV5IGRvbid0LiAgSW4gdGhhdCBjYXNlIEkgdGhpbmsgdGhlDQo+IGJl
c3QNCj4gb3B0aW9uIHdpbGwgYmUgMSkgYWJvdmUuDQo+ICANCg0KRm9yIE5GU3Y0LCB5b3Ugd291
bGQgaGF2ZSB0aGUgb3B0aW9uIG9mIHJldHVybmluZyBORlM0RVJSX1JFU09VUkNFIG9yDQpwb3Nz
aWJseSBORlM0RVJSX0lOVkFMIChwZXJzb25hbGx5LCBJJ2QgcHJlZmVyIHRoZSBsYXR0ZXIsIHNp
bmNlDQpORlM0RVJSX1JFU09VUkNFIGlzIHRvbyB3b29seSkuDQoNCkZvciBORlN2NC4xIG9yIGFi
b3ZlLCB5b3UgcHJvYmFibHkgaGF2ZSBhIGNob2ljZSBvZiBORlM0RVJSX1JFUV9UT09fQklHDQoo
aWYgdGhlIHNpemUgaXMgZ3JlYXRlciB0aGFuIGNhX21heHJlcXVlc3RzaXplKSBvciBORlM0RVJS
X0lOVkFMLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFp
bmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K
On Fri, Feb 09, 2018 at 03:04:42PM +0000, Trond Myklebust wrote:
> For NFSv4, you would have the option of returning NFS4ERR_RESOURCE or
> possibly NFS4ERR_INVAL (personally, I'd prefer the latter, since
> NFS4ERR_RESOURCE is too wooly).
>
> For NFSv4.1 or above, you probably have a choice of NFS4ERR_REQ_TOO_BIG
> (if the size is greater than ca_maxrequestsize) or NFS4ERR_INVAL.
I think this sort of problem is typically found early in the rpc code
(probably the check against sv_max_mesg in
net/sunrpc/svcsock.c:svc_tcp_recv_record()). It drops the connection.
I'm not sure why.
Returning an NFS-level error does sound friendlier. I guess I'd do that
by throwing away the rest of the request data and setting a flag on the
svc_rqst saying "this request was too long, don't trust the data" and
then making NFS check for that flag somewhere early on.
The only case I can recall seeing this in practice is when the limits
change across reboots. Which is something to avoid. But the choice of
failure mode might make this more or less easy to diagnose or recover
from....
--b.
> On Fri, Feb 09, 2018 at 09:27:00AM +0800, Lu Xinyu wrote:
> > I ran pynfs on the RHEL7.4GA with different
> > kernels(3.10.0-830.el7,4.15). The testLargeData:WRT5 failed with
> > broken pipe. I investigated pynfs code and got the following
> > questions.
> >
> > 1.The intent of WRT5 To check whether the server could handle a
> > too-large value over NFSSVC_MAXBLKSIZE.
> >
> > 2.The expected procedure when test gets passed The server could write
> > down the oversize data successfully sent by client and return NFS4_OK.
> > Then, client could read back the data from server.
>
> Yes, that test is definitely wrong. It might happen to work on servers
that
> support larger IO size, but the Linux server doesn't. And, anyway, it's
perfectly
> legal for a server to only support lower read/write size.
The test does happen to pass on Ganesha.
> So, the test needs check the server's advertised maximum read and write
sizes,
> and then either:
>
> 1) use that maximum size, and expect success, or
> 2) use a larger size and expect some sort of error.
>
> > (Actually, the test failed with broken pipe and did not return packet
> > information.)
> >
> > 3.The nfs server's standrd procedure of handling oversize data It
> > seems that the related error is not defined in the RFC3530. Whether
> > the server should return any NFSERR before ceasing receving ?
>
> I'm not sure if the RFC's specify the behavior here. I'll go do some
research. I
> have a feeling they don't. In that case I think the best option will be
1) above.
That's probably the right thing to do.
Frank
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
On Fri, 9 Feb 2018 13:31:12 -0500, J. Bruce Fields wrote:
>> For NFSv4, you would have the option of returning NFS4ERR_RESOURCE or
>> possibly NFS4ERR_INVAL (personally, I'd prefer the latter, since
>> NFS4ERR_RESOURCE is too wooly).
>>
>> For NFSv4.1 or above, you probably have a choice of NFS4ERR_REQ_TOO_BIG
>> (if the size is greater than ca_maxrequestsize) or NFS4ERR_INVAL.
>
> I think this sort of problem is typically found early in the rpc code
> (probably the check against sv_max_mesg in
> net/sunrpc/svcsock.c:svc_tcp_recv_record()). It drops the connection.
> I'm not sure why.
It looks implemented long time ago.
[PATCH] PATCH 11/16: NFSD: TCP: close bad connections
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=84e9f22a623a
Regards,
Ikarashi
On Tue, Feb 13, 2018 at 12:19:58PM +0900, Seiichi Ikarashi wrote:
>
> On Fri, 9 Feb 2018 13:31:12 -0500, J. Bruce Fields wrote:
>
> >> For NFSv4, you would have the option of returning NFS4ERR_RESOURCE or
> >> possibly NFS4ERR_INVAL (personally, I'd prefer the latter, since
> >> NFS4ERR_RESOURCE is too wooly).
> >>
> >> For NFSv4.1 or above, you probably have a choice of NFS4ERR_REQ_TOO_BIG
> >> (if the size is greater than ca_maxrequestsize) or NFS4ERR_INVAL.
> >
> > I think this sort of problem is typically found early in the rpc code
> > (probably the check against sv_max_mesg in
> > net/sunrpc/svcsock.c:svc_tcp_recv_record()). It drops the connection.
> > I'm not sure why.
>
> It looks implemented long time ago.
>
> [PATCH] PATCH 11/16: NFSD: TCP: close bad connections
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=84e9f22a623a
Thanks for tracking that down!
If the length is just ridiculous then closing is our best option, but if
it's within a few megabytes (as might happen if the maximum size changes
over reboot), then I think it might still be reasonable to receive the
whole thing, throw away the data, and return an error.
But, I'm not sure whether it's really an improvement. The spec's
unlikely to provide much guidance, so it'd probably be a question of
figuring out what the likely client behavior is.
So, if somebody wanted to make a case for it I'd look at patches, but I
think it's not a priority.
WRT5 is wrong in any case, I'll fix it.
--b.
On Tue, Feb 13, 2018 at 11:15:40AM -0500, J. Bruce Fields wrote:
> WRT5 is wrong in any case, I'll fix it.
How about this?
--b.
commit 30f1fff66da3
Author: J. Bruce Fields <[email protected]>
Date: Tue Feb 13 16:30:27 2018 -0500
4.0 server tests: replace WRT5 by better tests
This test is sending a write that (at least in the knfsd case) is larger
than the server's maximum write size, and expecting it to succeed.
That's weird.
Remove WRT5 and replace it by two tests. Both first query the maximum
write size. One does a large (but not too large) write and checks that
the data was written correctly. The other does a too-large write and
expects nothing. (A wide variety of server behavior would be in spec;
we do this just in case a buggy server might crash.)
Signed-off-by: J. Bruce Fields <[email protected]>
diff --git a/nfs4.0/servertests/st_write.py b/nfs4.0/servertests/st_write.py
index 710452efa11d..004562d2f52b 100644
--- a/nfs4.0/servertests/st_write.py
+++ b/nfs4.0/servertests/st_write.py
@@ -120,17 +120,22 @@ def testNoData(t, env):
if compareTimes(time_prior,time_after) != 0:
t.fail("WRITE with no data affected time_modify")
+#WRT5 permanently retired
+
def testLargeData(t, env):
- """WRITE with a large amount of data
+ """WRITE with the maximum size, READ it back and compare
FLAGS: write read all
DEPEND: MKFILE
- CODE: WRT5
+ CODE: WRT5a
"""
c = env.c1
c.init_connection()
+ maxread, maxwrite = _get_iosize(t, c, c.homedir)
+ maxio = min(maxread, maxwrite)
fh, stateid = c.create_confirm(t.code)
- data = "abcdefghijklmnopq" * 0x10000
+ pattern="abcdefghijklmnop"
+ data = pattern * (maxio / len(pattern)) + "q" * (maxio % len(pattern))
# Write the data
pos = 0
while pos < len(data):
@@ -150,6 +155,27 @@ def testLargeData(t, env):
if data != newdata:
t.fail("READ did not correspond to WRITE with large dataset")
+def testTooLargeData(t, env):
+ """WRITE with more than the maximum size
+
+ FLAGS: write read all
+ DEPEND: MKFILE
+ CODE: WRT5b
+ """
+ c = env.c1
+ c.init_connection()
+ maxread, maxwrite = _get_iosize(t, c, c.homedir)
+ fh, stateid = c.create_confirm(t.code)
+ data = "a" * (maxwrite + 1000000)
+ try:
+ # We don't care much what the server does, this is just a check
+ # to make sure it doesn't crash.
+ res = c.write_file(fh, data, 0, stateid)
+ except IOError:
+ # Linux knfsd closes the socket when the write is too large.
+ # That's OK.
+ pass
+
def testDir(t, env):
"""WRITE to a dir should return NFS4ERR_ISDIR
This looks sane, and like a good improvement.
Daniel
On 02/14/2018 10:49 AM, J. Bruce Fields wrote:
> On Tue, Feb 13, 2018 at 11:15:40AM -0500, J. Bruce Fields wrote:
>> WRT5 is wrong in any case, I'll fix it.
>
> How about this?
>
> --b.
>
> commit 30f1fff66da3
> Author: J. Bruce Fields <[email protected]>
> Date: Tue Feb 13 16:30:27 2018 -0500
>
> 4.0 server tests: replace WRT5 by better tests
>
> This test is sending a write that (at least in the knfsd case) is larger
> than the server's maximum write size, and expecting it to succeed.
> That's weird.
>
> Remove WRT5 and replace it by two tests. Both first query the maximum
> write size. One does a large (but not too large) write and checks that
> the data was written correctly. The other does a too-large write and
> expects nothing. (A wide variety of server behavior would be in spec;
> we do this just in case a buggy server might crash.)
>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> diff --git a/nfs4.0/servertests/st_write.py b/nfs4.0/servertests/st_write.py
> index 710452efa11d..004562d2f52b 100644
> --- a/nfs4.0/servertests/st_write.py
> +++ b/nfs4.0/servertests/st_write.py
> @@ -120,17 +120,22 @@ def testNoData(t, env):
> if compareTimes(time_prior,time_after) != 0:
> t.fail("WRITE with no data affected time_modify")
>
> +#WRT5 permanently retired
> +
> def testLargeData(t, env):
> - """WRITE with a large amount of data
> + """WRITE with the maximum size, READ it back and compare
>
> FLAGS: write read all
> DEPEND: MKFILE
> - CODE: WRT5
> + CODE: WRT5a
> """
> c = env.c1
> c.init_connection()
> + maxread, maxwrite = _get_iosize(t, c, c.homedir)
> + maxio = min(maxread, maxwrite)
> fh, stateid = c.create_confirm(t.code)
> - data = "abcdefghijklmnopq" * 0x10000
> + pattern="abcdefghijklmnop"
> + data = pattern * (maxio / len(pattern)) + "q" * (maxio % len(pattern))
> # Write the data
> pos = 0
> while pos < len(data):
> @@ -150,6 +155,27 @@ def testLargeData(t, env):
> if data != newdata:
> t.fail("READ did not correspond to WRITE with large dataset")
>
> +def testTooLargeData(t, env):
> + """WRITE with more than the maximum size
> +
> + FLAGS: write read all
> + DEPEND: MKFILE
> + CODE: WRT5b
> + """
> + c = env.c1
> + c.init_connection()
> + maxread, maxwrite = _get_iosize(t, c, c.homedir)
> + fh, stateid = c.create_confirm(t.code)
> + data = "a" * (maxwrite + 1000000)
> + try:
> + # We don't care much what the server does, this is just a check
> + # to make sure it doesn't crash.
> + res = c.write_file(fh, data, 0, stateid)
> + except IOError:
> + # Linux knfsd closes the socket when the write is too large.
> + # That's OK.
> + pass
> +
> def testDir(t, env):
> """WRITE to a dir should return NFS4ERR_ISDIR
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 2018-02-15 00:49, J. Bruce Fields wrote:
> On Tue, Feb 13, 2018 at 11:15:40AM -0500, J. Bruce Fields wrote:
>> WRT5 is wrong in any case, I'll fix it.
>
> How about this?
>
> --b.
>
> commit 30f1fff66da3
> Author: J. Bruce Fields <[email protected]>
> Date: Tue Feb 13 16:30:27 2018 -0500
>
> 4.0 server tests: replace WRT5 by better tests
>
> This test is sending a write that (at least in the knfsd case) is larger
> than the server's maximum write size, and expecting it to succeed.
> That's weird.
>
> Remove WRT5 and replace it by two tests. Both first query the maximum
Really good to split it into WRT5a and WRT5b!
> write size. One does a large (but not too large) write and checks that
> the data was written correctly. The other does a too-large write and
> expects nothing. (A wide variety of server behavior would be in spec;
> we do this just in case a buggy server might crash.)
>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> diff --git a/nfs4.0/servertests/st_write.py b/nfs4.0/servertests/st_write.py
> index 710452efa11d..004562d2f52b 100644
> --- a/nfs4.0/servertests/st_write.py
> +++ b/nfs4.0/servertests/st_write.py
> @@ -120,17 +120,22 @@ def testNoData(t, env):
> if compareTimes(time_prior,time_after) != 0:
> t.fail("WRITE with no data affected time_modify")
>
> +#WRT5 permanently retired
> +
> def testLargeData(t, env):
How about the name "testMaximumData()" or something?
It's not just "Large".
> - """WRITE with a large amount of data
> + """WRITE with the maximum size, READ it back and compare
>
> FLAGS: write read all
> DEPEND: MKFILE
> - CODE: WRT5
> + CODE: WRT5a
> """
> c = env.c1
> c.init_connection()
> + maxread, maxwrite = _get_iosize(t, c, c.homedir)
> + maxio = min(maxread, maxwrite)
> fh, stateid = c.create_confirm(t.code)
> - data = "abcdefghijklmnopq" * 0x10000
> + pattern="abcdefghijklmnop"
> + data = pattern * (maxio / len(pattern)) + "q" * (maxio % len(pattern))
> # Write the data
> pos = 0
> while pos < len(data):
> @@ -150,6 +155,27 @@ def testLargeData(t, env):
> if data != newdata:
> t.fail("READ did not correspond to WRITE with large dataset")
>
> +def testTooLargeData(t, env):
> + """WRITE with more than the maximum size
with 10^6 bytes beyond the maximum size
Regards,
Ikarashi
> +
> + FLAGS: write read all
> + DEPEND: MKFILE
> + CODE: WRT5b
> + """
> + c = env.c1
> + c.init_connection()
> + maxread, maxwrite = _get_iosize(t, c, c.homedir)
> + fh, stateid = c.create_confirm(t.code)
> + data = "a" * (maxwrite + 1000000)
> + try:
> + # We don't care much what the server does, this is just a check
> + # to make sure it doesn't crash.
> + res = c.write_file(fh, data, 0, stateid)
> + except IOError:
> + # Linux knfsd closes the socket when the write is too large.
> + # That's OK.
> + pass
> +
> def testDir(t, env):
> """WRITE to a dir should return NFS4ERR_ISDIR
>
>
>
> .
>
On Thu, Feb 15, 2018 at 11:08:51AM +0900, Seiichi Ikarashi wrote:
> How about the name "testMaximumData()" or something?
> It's not just "Large".
...
> > +def testTooLargeData(t, env):
> > + """WRITE with more than the maximum size
>
> with 10^6 bytes beyond the maximum size
Both done, thanks.--b.