[prev in list] [next in list] [prev in thread] [next in thread] 

List:       ietf-nfsv4
Subject:    Some proposed changes for the spec
From:       "Noveck, Dave" <Dave.Noveck () netapp ! com>
Date:       2002-06-06 23:37:35
Message-ID: 8C610D86AF6CD4119C9800B0D0499E336A8E67 () red ! nane ! netapp ! com
[Download RAW message or body]

Spencer has asked me to flesh out my proposals in 

http://playground.sun.com/pub/nfsv4/nfsv4-wg-archive/2002/0200.html

Those changes are now available at

http://www.nfsv4.org/rfc3010updates/input/June5AsModifiedByDaveNoveck.txt

which is based on the June 5 spec.  Below is a running list of the 
nature of all the changes layed out in spec order (for all of the 
items combined).

For Proposal 6, in section 8.1.4, create a new first paragraph
before the existing one, to clarify the role of SETATTR in
all this (i.e. stateid's are not just for READ and WRITE any more).

For Proposal 1, in section 8.1.4, modified the (old) first paragraph
to clarify return of NFS4ERR_LOCKED.

For proposal 6, revised the first sentence of the (current)
second paragraph for greater clarity.

For Proposal 6, in section 8.1.4, add a new parapgraph, after 
the first, explaining mandatory and advisory locks.

For Proposal 5, in section 8.1.4, add two new paragraphs (now the
fourth and fifth).

For Proposal 2, in section 8.1.4, modify the (old) second paragraph
(now the sixth), as specified below.

For proposal 6, in section 8.1.4, modify the current sixth paragraph
(was the second) by adding a sentence to make things clearer.

For Proposal 6, in section 8.1.4, add a sentence to the last paragraph 
make things less vague.

For Proposal 5, in section 12, add a description of NFS4ERR_OPENMODE

For Proposal 1, in section 14.2.23 (READ), under ERRORS, delete
NFS4ERR_DENIED.

For Proposal 15, in section 14.2.23 (READ), under ERRORS, add
NFS4ERR_OPENMODE.

For Proposal 3, in section 14.2.32 (SETATTR), rewrite the second
paragraph under DESCRIPTION.

For Proposal 4, in section 14.2.32 (SETATTR), rewrite the last
paragraph under IMPLEMENTATION.

For proposal 1, in section 14.2.32 (SETATTR), under errors delete
NFS4ERR_DENIED and add NFS4ERR_LOCKED

For Proposal 5, in section 14.2.32 (SETATTR), under ERRORS, add
NFS4ERR_OPENMODE

For Proposal 1, in Section 14.2.36 (WRITE), under ERRORS, delete
NFS4ERR_DENIED

For Proposal 5, in section 14.2.36 (WRITE), under ERRORS, add
NFS4ERR_OPENMODE

For Proposal 5, in section 18, add a definition of NFS4ERR_OPENMODE

-----Original Message-----
From: Spencer Shepler [mailto:shepler@eng.sun.com]
Sent: Tuesday, June 04, 2002 3:28 PM
To: Noveck, Dave
Cc: beepy@netapp.com
Subject: Re: Some issues in the spec



Dave,
  I would like to solicit your help with updates as you suggest at the
end of this note.  If your updates can be against any of the most
recent spec updates, that would be helpful.  I keep all of the
intermediate drafts so I should be able compare things.
  I have inserted comments within your original note.

On Sat, Noveck, Dave wrote:
> I started looking at the spec to find out the proper 
> error code for something and wound up finding a bunch 
> of stuff that I think needs to be fixed somehow in 
> the spec.
> 
> Issue 1) What error is correct if, for example, you 
> try do do a write on a file and there is a conflicting 
> lock?  Section 8.1.4 talks about returning NFS4ERR_LOCKED 
> in that case, but the context of the paragraph is talking
> about stateid of all 0's, so it is not clear if this
> holds for all cases.  (I think it should).  The error
> list for READ and WRITE has NFS4ERR_LOCKED but also
> NFS4ERR_DENIED and it is unclear in what cases that
> latter error would be returned.  DENIED is defined as
> involving being unable to get a lock which READ and WRITE
> don't really do.  You could argue that they get an "implicit"
> lock which would justifiy DENIED but then what is LOCKED
> for?  Seems like there is room for DENIED or LOCKED
> but not both.  (just noticed that SETATTR has DENIED but
> not LOCKED)
> 
> Proposal 1) Get rid of DENIED under READ and WRITE and
> revise 8.1.4 to make clear that LOCKED applies to all
> cases of a conflicting lock with READ and WRITE.  Get
> rid of DENIED under SETATTR amd add LOCKED.
> 
> or the reverse if we decide that DENIED is better.

LOCKED seems appropriate and the text should include the fact
that the server's behavior is driven by its offering of
mandatory or advisory locking.

> Issue 2) The last sentence of the second paragraph of
> section 8.1.4 doesn't seem to make sense.  The previous
> sentence says that writes with all-1 stateid's do not
> bypass record locking checks.  So what about share
> locking checks?  Are they bypassed or not?  It just
> says they're handled by the open operation which doesn't
> seem to be all that relevant given that we are talking
> about a special stateid for which there presumably was
> no open.  If you did an open, you'd have a real stateid
> and the all-1's stateid would have no purpose (at least
> for write since it doesn't override locking checks for
> write).
> 
> Proposal 2) Scratch "record" in the first and second sentences
> of this paragraph and get rid of the third sentence entirely.

Can this be tied to the broader isssue of what we should do with
special stateids?  What I mean is that should we resolve the issue of
what special stateids should be defined and accepted and then rework
this behavior?

> Issue 3) SETATTR has a stateid to deal with the fact that
> changes in size are modifying data just a write does, but 
> this isn't explained very clearly
> 
> Proposal 3) Replace the second paragraph under DESCRIPTION for
> SETATTR by the following:
> 
> The stateid argument for SETATTR is used to provide file
> locking context that is necessary for SETATTR requests that
> set the size attribute.  Since setting the size attribute 
> modifies the file's data, it has the same locking requirements
> as a corresponding WRITE.  Any SETATTR that sets the size
> attribute is incompatible with a share lock that specifies
> DENY_WRITE.  The area between the old end-of-file and the new
> end-of-file is considered to be modified just as would have 
> been the case had the area in question been specified in
> WRITE, for the purpose of checking conflicts with record locks.
> A valid stateid should always be specified.  When the file 
> size attribute is not set, the special stateid consisting of 
> all bits zero should be passed.

This seems reasonable.  Please include in your diffs for the SPEC
update.

> Issue 4) The last paragraph of the IMPLEMENTATION section of
> SETATTR, besides the practical problems in that following
> the recommendation of returning INVAL (you did something wrong
> and I'm not going to tell you what it is.  Nya Nya Nya), 
> doesn't fit the error list that follows.  It does have things 
> in it other than INVAL, which presumably would prevent you
> from successfully setting the attributes.  If you can't
> set the attributes because of an IO error, you return IO,
> not INVAL.  Also FBIG is there.  FBIG says the error is 
> extending a file beyond the server limit.  Why doesn't
> extending it beyond 4 Gig when the server can't handle that 
> use precisely this error, rather than INVAL. (Returning 
> FBIG if the client exceeds the servers limit of 3G or 10G
> but INVAL if it exceeds the server's limit of 4G is kind
> of insane).
> 
> Proposal 4) Delete this paragraph.  Add the following text
> to the end of the second paragraph of the IMPLEMENTATION
> section.
> 
> A mask of the attibutes actually set is returned by SETATTR
> in all cases.  That mask must not include attributes bits not
> requested to be set by the client, and must be equal to the 
> mask of attributes requested to be set only if the SETATTR 
> completes without error.

Agreed.  Please include in updates.

> Issue 5) I couldn't find the error to return if I open a file
> for read and do a write.  If someone else has opened it with
> WRITE-DENY, you get NFS4ERR_LOCKED (or NFS4ERR_DENIED), but what
> if there is no such conflicting lock?  I can't believe that it
> is intended that this go through without error.  But if not,
> what is the error?  The whole apparatus of checking share locks 
> at open time is predicated on open representing properly the 
> operations that may be perfomred within the scope of the open.
> It's true that we already violate this by allowing the special
> stateid's (that I love so well), but going farther and treating
> a write done under an open for read as if it were done with stateid
> zero seems way too much of a bad thing.
> 
> Proposal 5) Add a new error NFS4ERR_OPENMODE:
> 
> Add to section 12 the following,
> 
> NFS4ERR_OPENMODE      The client attempted a READ, WRITE, or SETATTR
>                       operation not sanctioned by the stateid passed
>                       (e.g. writing to a file opend only for read).
> 
> and replace in the .x stuff,
> 
>            NFS4ERR_LOCKS_HELD      = 10037
> 
> by 
> 
>            NFS4ERR_LOCKS_HELD      = 10037,
>            NFS4ERR_OPENMODE        = 10038
> 
> Add NFS4ERR_OPENMODE to the error lists for READ, WRITE, SETATTR.
> 
> Add the following paragraph as the third paragraph of section 8.1.4,
> 
> Every stateid other than the special stateid values noted above,
> whether returned by an OPEN-type operation (i.e. OPEN, OPEN_DOWNGRADE),
> or by a LOCK-type operation (i.e. LOCK or LOCKU), defines an access
> mode for the file (i.e. READ, WRITE, or READ_WRITE) as established
> by the original OPEN which began the stateid sequence, and as modified 
> by subsequent OPEN's and OPEN_DOWNGRADE's within that stateid sequence.
> When a READ, WRITE, or SETATTR which specifies the size attribute 
> is done, if the access mode for the stateid does not allow the 
> operation to be perofmred (for these purposes SETATTR which sets
> the size attribute is treated like a WRITE), the operation should
> fail with an NFS4ERR_OPENMODE.

This seems reasonable.  However, there is the issue of clients that
use the VM to manage file cache.  For example, there may be an
application that opens a file for WRITE only.  The NFS client
implementation will read an entire page of file data to allow a client
to write one byte if the client is implemented using traditional VM
access to manage file cache.  Therefore, it would be helpful in client
implementations if they were allowed to READ file data even if the
file was open with WRITE only.

Ideas.

> Issue 6) Section 8.1.4 is a mess and the hacking suggested above 
> hasn't improved its clarity.
> 
> Proposal 6) Once we come to agreement on the stuff above, I will
> try to redraft section 8.1.4 so that it is clearer and more readable.
>  

Redrafting would be great.  Thanks for the help.

Spencer


[prev in list] [next in list] [prev in thread] [next in thread]