[PATCH pynfs] Allow server to reject maximum commit offsets

[PATCH pynfs] Allow server to reject maximum commit offsets

[J. Bruce Fields]

From: J. Bruce Fields <bfields@citi.umich.edu>

These two tests insist that a server must accept commits with offset
2^64-1 and 2^64-2.  I can find no justification in the spec for this
requirement.

The linux server was recently changed to reject (with INVAL) offsets
over 2^63-1, which is the maximum that the vfs commit routine can
accept.  That behavior is consistent with the NFSv3 commit
implementation and with the NFSv4 write implementation.

The maximum offset allowed may vary depending on filesystem or server,
so I think it's simplest just to delete these tests.  (Alternatively, we
could replace the offset by whatever we consider a reasonable least
common denominator, e.g. 2^31-1.)

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 lib/nfs4/servertests/st_commit.py |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/lib/nfs4/servertests/st_commit.py b/lib/nfs4/servertests/st_commit.py
index 2955d79..0005e54 100644
--- a/lib/nfs4/servertests/st_commit.py
+++ b/lib/nfs4/servertests/st_commit.py
@@ -35,24 +35,6 @@ def testCommitOffset1(t, env):
     """
     _commit(t, env.c1, 1)
 
-def testCommitOffsetMax1(t, env):
-    """COMMIT
-
-    FLAGS: commit all
-    DEPEND: MKFILE
-    CODE: CMT1c
-    """
-    _commit(t, env.c1, 0xffffffffffffffffL)
-
-def testCommitOffsetMax2(t, env):
-    """COMMIT
-
-    FLAGS: commit all
-    DEPEND: MKFILE
-    CODE: CMT1d
-    """
-    _commit(t, env.c1, 0xfffffffffffffffeL)
-
 def testCommitCount1(t, env):
     """COMMIT
 
-- 
1.6.3.3

Re: [PATCH pynfs] Allow server to reject maximum commit offsets

[J. Bruce Fields]

On Tue, Feb 09, 2010 at 03:12:31PM -0500, bfields wrote:
> From: J. Bruce Fields <bfields@citi.umich.edu>
> 
> These two tests insist that a server must accept commits with offset
> 2^64-1 and 2^64-2.  I can find no justification in the spec for this
> requirement.

You can also get this and another old patch by:

	git pull git://linux-nfs.org/~bfields/pynfs.git master

--b.

Re: [PATCH pynfs] Allow server to reject maximum commit offsets

[Peter Staubach]

J. Bruce Fields wrote:
> From: J. Bruce Fields <bfields@citi.umich.edu>
> 
> These two tests insist that a server must accept commits with offset
> 2^64-1 and 2^64-2.  I can find no justification in the spec for this
> requirement.
> 
> The linux server was recently changed to reject (with INVAL) offsets
> over 2^63-1, which is the maximum that the vfs commit routine can
> accept.  That behavior is consistent with the NFSv3 commit
> implementation and with the NFSv4 write implementation.
> 

I guess that I am missing something.  At least the NFSv3
COMMIT operation takes an unsigned 64 bit quantity.  That
would seem to make the test correct, would it not?

		ps

> The maximum offset allowed may vary depending on filesystem or server,
> so I think it's simplest just to delete these tests.  (Alternatively, we
> could replace the offset by whatever we consider a reasonable least
> common denominator, e.g. 2^31-1.)
> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  lib/nfs4/servertests/st_commit.py |   18 ------------------
>  1 files changed, 0 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/nfs4/servertests/st_commit.py b/lib/nfs4/servertests/st_commit.py
> index 2955d79..0005e54 100644
> --- a/lib/nfs4/servertests/st_commit.py
> +++ b/lib/nfs4/servertests/st_commit.py
> @@ -35,24 +35,6 @@ def testCommitOffset1(t, env):
>      """
>      _commit(t, env.c1, 1)
>  
> -def testCommitOffsetMax1(t, env):
> -    """COMMIT
> -
> -    FLAGS: commit all
> -    DEPEND: MKFILE
> -    CODE: CMT1c
> -    """
> -    _commit(t, env.c1, 0xffffffffffffffffL)
> -
> -def testCommitOffsetMax2(t, env):
> -    """COMMIT
> -
> -    FLAGS: commit all
> -    DEPEND: MKFILE
> -    CODE: CMT1d
> -    """
> -    _commit(t, env.c1, 0xfffffffffffffffeL)
> -
>  def testCommitCount1(t, env):
>      """COMMIT
>  

Re: [PATCH pynfs] Allow server to reject maximum commit offsets

[J. Bruce Fields]

On Tue, Feb 09, 2010 at 05:28:52PM -0500, Peter Staubach wrote:
> J. Bruce Fields wrote:
> > From: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > These two tests insist that a server must accept commits with offset
> > 2^64-1 and 2^64-2.  I can find no justification in the spec for this
> > requirement.
> > 
> > The linux server was recently changed to reject (with INVAL) offsets
> > over 2^63-1, which is the maximum that the vfs commit routine can
> > accept.  That behavior is consistent with the NFSv3 commit
> > implementation and with the NFSv4 write implementation.
> > 
> 
> I guess that I am missing something.  At least the NFSv3
> COMMIT operation takes an unsigned 64 bit quantity.  That
> would seem to make the test correct, would it not?

Yes, it's an unsigned 64 bit integer, but so what: there's no rule that
says the server has to return NFS4ERR_OK to any operation for which all
the arguments are valid xdr.

And just because the protocol allows 64-bit offsets doesn't mean every
server and filesystem is going to support the full range.

--b.

> 
> 		ps
> 
> > The maximum offset allowed may vary depending on filesystem or server,
> > so I think it's simplest just to delete these tests.  (Alternatively, we
> > could replace the offset by whatever we consider a reasonable least
> > common denominator, e.g. 2^31-1.)
> > 
> > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > ---
> >  lib/nfs4/servertests/st_commit.py |   18 ------------------
> >  1 files changed, 0 insertions(+), 18 deletions(-)
> > 
> > diff --git a/lib/nfs4/servertests/st_commit.py b/lib/nfs4/servertests/st_commit.py
> > index 2955d79..0005e54 100644
> > --- a/lib/nfs4/servertests/st_commit.py
> > +++ b/lib/nfs4/servertests/st_commit.py
> > @@ -35,24 +35,6 @@ def testCommitOffset1(t, env):
> >      """
> >      _commit(t, env.c1, 1)
> >  
> > -def testCommitOffsetMax1(t, env):
> > -    """COMMIT
> > -
> > -    FLAGS: commit all
> > -    DEPEND: MKFILE
> > -    CODE: CMT1c
> > -    """
> > -    _commit(t, env.c1, 0xffffffffffffffffL)
> > -
> > -def testCommitOffsetMax2(t, env):
> > -    """COMMIT
> > -
> > -    FLAGS: commit all
> > -    DEPEND: MKFILE
> > -    CODE: CMT1d
> > -    """
> > -    _commit(t, env.c1, 0xfffffffffffffffeL)
> > -
> >  def testCommitCount1(t, env):
> >      """COMMIT
> >  
> 

Re: [PATCH pynfs] Allow server to reject maximum commit offsets

[Trond Myklebust]

On Tue, 2010-02-09 at 17:28 -0500, Peter Staubach wrote: 
> J. Bruce Fields wrote:
> > From: J. Bruce Fields <bfields@citi.umich.edu>
> > 
> > These two tests insist that a server must accept commits with offset
> > 2^64-1 and 2^64-2.  I can find no justification in the spec for this
> > requirement.
> > 
> > The linux server was recently changed to reject (with INVAL) offsets
> > over 2^63-1, which is the maximum that the vfs commit routine can
> > accept.  That behavior is consistent with the NFSv3 commit
> > implementation and with the NFSv4 write implementation.
> > 
> 
> I guess that I am missing something.  At least the NFSv3
> COMMIT operation takes an unsigned 64 bit quantity.  That
> would seem to make the test correct, would it not?

The server will not allow you to write to an offset > 2^63-1 (it will
return NFS3ERR_INVAL), so it makes zero sense for the client to try to
issue a COMMIT for an offset starting in that range.

Cheers
  Trond

Re: [PATCH pynfs] Allow server to reject maximum commit offsets

[Peter Staubach]

J. Bruce Fields wrote:
> On Tue, Feb 09, 2010 at 05:28:52PM -0500, Peter Staubach wrote:
>> J. Bruce Fields wrote:
>>> From: J. Bruce Fields <bfields@citi.umich.edu>
>>>
>>> These two tests insist that a server must accept commits with offset
>>> 2^64-1 and 2^64-2.  I can find no justification in the spec for this
>>> requirement.
>>>
>>> The linux server was recently changed to reject (with INVAL) offsets
>>> over 2^63-1, which is the maximum that the vfs commit routine can
>>> accept.  That behavior is consistent with the NFSv3 commit
>>> implementation and with the NFSv4 write implementation.
>>>
>> I guess that I am missing something.  At least the NFSv3
>> COMMIT operation takes an unsigned 64 bit quantity.  That
>> would seem to make the test correct, would it not?
> 
> Yes, it's an unsigned 64 bit integer, but so what: there's no rule that
> says the server has to return NFS4ERR_OK to any operation for which all
> the arguments are valid xdr.
> 
> And just because the protocol allows 64-bit offsets doesn't mean every
> server and filesystem is going to support the full range.
> 

No, that's certainly true.  However, you are tailoring the
testsuite to a Linux specific implementation, which strikes
me as vaguely not so good.  What if you just commented out
the tests, with a comment indicating that the Linux server
can not handle the full 64 bits of range, instead of
deleting them completely?

	Thanx...

		ps


> --b.
> 
>> 		ps
>>
>>> The maximum offset allowed may vary depending on filesystem or server,
>>> so I think it's simplest just to delete these tests.  (Alternatively, we
>>> could replace the offset by whatever we consider a reasonable least
>>> common denominator, e.g. 2^31-1.)
>>>
>>> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
>>> ---
>>>  lib/nfs4/servertests/st_commit.py |   18 ------------------
>>>  1 files changed, 0 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/lib/nfs4/servertests/st_commit.py b/lib/nfs4/servertests/st_commit.py
>>> index 2955d79..0005e54 100644
>>> --- a/lib/nfs4/servertests/st_commit.py
>>> +++ b/lib/nfs4/servertests/st_commit.py
>>> @@ -35,24 +35,6 @@ def testCommitOffset1(t, env):
>>>      """
>>>      _commit(t, env.c1, 1)
>>>  
>>> -def testCommitOffsetMax1(t, env):
>>> -    """COMMIT
>>> -
>>> -    FLAGS: commit all
>>> -    DEPEND: MKFILE
>>> -    CODE: CMT1c
>>> -    """
>>> -    _commit(t, env.c1, 0xffffffffffffffffL)
>>> -
>>> -def testCommitOffsetMax2(t, env):
>>> -    """COMMIT
>>> -
>>> -    FLAGS: commit all
>>> -    DEPEND: MKFILE
>>> -    CODE: CMT1d
>>> -    """
>>> -    _commit(t, env.c1, 0xfffffffffffffffeL)
>>> -
>>>  def testCommitCount1(t, env):
>>>      """COMMIT
>>>  

Re: [PATCH pynfs] Allow server to reject maximum commit offsets

[J. Bruce Fields]

On Tue, Feb 09, 2010 at 05:45:06PM -0500, Peter Staubach wrote:
> J. Bruce Fields wrote:
> > On Tue, Feb 09, 2010 at 05:28:52PM -0500, Peter Staubach wrote:
> >> J. Bruce Fields wrote:
> >>> From: J. Bruce Fields <bfields@citi.umich.edu>
> >>>
> >>> These two tests insist that a server must accept commits with offset
> >>> 2^64-1 and 2^64-2.  I can find no justification in the spec for this
> >>> requirement.
> >>>
> >>> The linux server was recently changed to reject (with INVAL) offsets
> >>> over 2^63-1, which is the maximum that the vfs commit routine can
> >>> accept.  That behavior is consistent with the NFSv3 commit
> >>> implementation and with the NFSv4 write implementation.
> >>>
> >> I guess that I am missing something.  At least the NFSv3
> >> COMMIT operation takes an unsigned 64 bit quantity.  That
> >> would seem to make the test correct, would it not?
> > 
> > Yes, it's an unsigned 64 bit integer, but so what: there's no rule that
> > says the server has to return NFS4ERR_OK to any operation for which all
> > the arguments are valid xdr.
> > 
> > And just because the protocol allows 64-bit offsets doesn't mean every
> > server and filesystem is going to support the full range.
> > 
> 
> No, that's certainly true.  However, you are tailoring the
> testsuite to a Linux specific implementation, which strikes
> me as vaguely not so good.  What if you just commented out
> the tests, with a comment indicating that the Linux server
> can not handle the full 64 bits of range, instead of
> deleting them completely?

The tests are primarily meant to test protocol conformance.  So they
shouldn't be reporting failures on conforming behavior.

Perhaps it would also be interesting to run the tests in a mode which
probes and summarizes server characteristics (maximum supported offset,
supported features, etc.), but that's a job for another day.

--b.

Re: [PATCH pynfs] Allow server to reject maximum commit offsets

[J. Bruce Fields]

On Tue, Feb 09, 2010 at 05:50:18PM -0500, J. Bruce Fields wrote:
> The tests are primarily meant to test protocol conformance.  So they
> shouldn't be reporting failures on conforming behavior.
> 
> Perhaps it would also be interesting to run the tests in a mode which
> probes and summarizes server characteristics (maximum supported offset,
> supported features, etc.), but that's a job for another day.

Hm, actually another alternative would be just to keep these tests, but
to *always* allow them to succeed.  Or to fail only if the server
returns an error that really is totally wrong.

Even if we don't much care about the results, sending operations with
extreme values for the arguments may still help make sure server's don't
skimp on the range-checking and crash in some lower-level code.

--b.

Re: [PATCH pynfs] Allow server to reject maximum commit offsets

[Peter Staubach]

J. Bruce Fields wrote:
> On Tue, Feb 09, 2010 at 05:50:18PM -0500, J. Bruce Fields wrote:
>> The tests are primarily meant to test protocol conformance.  So they
>> shouldn't be reporting failures on conforming behavior.
>>
>> Perhaps it would also be interesting to run the tests in a mode which
>> probes and summarizes server characteristics (maximum supported offset,
>> supported features, etc.), but that's a job for another day.
> 
> Hm, actually another alternative would be just to keep these tests, but
> to *always* allow them to succeed.  Or to fail only if the server
> returns an error that really is totally wrong.
> 
> Even if we don't much care about the results, sending operations with
> extreme values for the arguments may still help make sure server's don't
> skimp on the range-checking and crash in some lower-level code.
> 

This sounds reasonable to me.

		ps