Subject: Feedback on draft-ietf-nfsv4-01 - general

From: Neil Brown <[email-masked]>
Date: Mon, 11 Oct 1999 14:11:51 +1000 (EST)

Hi,
 I first came across the NFSv4 proposal recently when Brent Callaghan
 mentioned the URL for draft-ietf-nfsv4-01 on
 [email-masked]

 While reading the draft I scribbled down numerous comments which I
 have tidied up and present here.

 I have looked through the mail archive a bit, but not exhaustively,
 so if I address an issue that has already been discussed, please
 forgive me. 

 This Email is the first of 4.  This one contains numerous little
 points that bear more on the clarity of the document than on the
 protocol itself.  The following three items are more ambitious and
 try to address issues with the protocol itself.

 But before I get into details, I would first like to thank everybody
 who has got the protocol this far.  I has lots of good ideas and I
 look forward to using it in the not-to-distant future.

NeilBrown

Comments on draft-ietf-nfsv4-01

-------------------------------

The COMPOUND nature of all NFSv4 requests if quite different to the
approach used in earlier versions.  However it doesn't get mentioned
until chapter 11.  Up until then a first-time reader (as I was) could
easily be confused by comments such as
  'The PUTROOTFH procedure instructs the server to set the "current"
   file handle.....'
in section 3.1.1.  as they have no idea of what a "current" file
handle is.

Maybe a "summary of differences" could go into a new section after the
introductions (as the NFSv3 RFC does) which mentions the COMPOUND
request approach and referred to reader to a later section.

---------------------------------

In section 3.2.2 "Persistent File Handle"

  "If the server restarts or reboots, or the filesystem is
   migrated, the NFS server must honor and present the same file handle
                                 ^^^^^     ^^^^^^^
   value as it did in the server's previous instantiation.

There are two aspects of persistence which are mentioned, but seem to
be a bit blurred.
A file handle can be persistently "honoured" (meaning you never get
NFS4ERR_EXPIRED), and it can be persistently "presented" meaning that
whenever a given named is LOOKUPed in a given directory, you get the
same file handle.  As the spec allows (though discourages) situations
where a given file may be represented by two different file handles
(even though they may be persistent handles) requiring that the same
filehandle is persistently "presented" seem un-necessary.  That
requirement should be tided to the unique_handles attribute.

----------------------------------

3.2.3 Volatile file handle.

I don't get what the table is trying to say.

From looking through the list archives (1253 and following) I'm
guessing that the P and V don't refer to Persistent and Volatile file
handles, put whether a given volatile file handle with persist or
change (volate?) and a given point in time.  It appears that this is
meant to be a general proforma for a table and each server/filesystem
will give a specific value in each entry.

If this is what the table means, then it needs to be spelled out a bit
more.  If not, them I'm lost.

-----------------------------------

wcc4_info is mentioned 8 times in sections 9.8 and 9.9 (name caching
and directory caching) but is not defined anywhere.

-------------------------------------------------------------

In section 5.4, the "change" attribute is allowed to be "mtime",
with unspecified resolution.  This doesn't provide a guarantee of "no
other client has made a change" in the face of multiple changes in one
second. 

I think that the change attribute should be required to change at least
whenever the file changes (it may change at other times too, such as
server restart).
It we really want to allow "mtime" to be usable, then I would suggest
that "change" have a special value "0" which means "the file is
changing.  i.e. if "change" reads as '0', then the client must assume
that the file has changed since the last time the change attribute was
read.   Then the formula:

    (mtime == current_time) ? 0 : mtime

would work.

----------------------------------------------------------

Section 5.5 defines a recommended "hidden" attributes.
What does this mean?
Does it mean "Hidden in the sense of the WIN32 API", in which case it
should say so.
Does it mean "Hidden in the sense that the name name starts with a
period"?
Does it mean "Hidden in any sense the client chooses to interpret and
any sense the server chooses to provide"?

In any case,  the spec should tell me what "hidden" means.

Similarly the "system" attribute should be defined.

---------------------------------------------------

Section 5.5 defines a "volatility" attribute which seems interesting,
but there is nowhere any suggestion on how it might be computed, or
how it might be used.  I suspect that this situation would lead to it
not being computed and not being used.

-----------------------------------------------------

Section 5.5 talks about {file,space}_{avail,free,total} and
quota_{hard,soft,used}

The _avail attributes are defined in terms of "this user".  Is that
the owner of the file, or the user identified in the credentials of
the GETATTR request?  Why has the user got anything to do with it?  (I
guess it is the "10% reserved for root" idea)

Quota_hard talks about declining the allocation of new space, but
doesn't say to who: Is it the principal in the credentials, or the
owner of the file, or (more likely) 'this file and any other "similar"
file with some server-specific definition of similar such as "same
user" or "same group" or "same file tree".

Similarly quota_soft talks about warning a user - but who? 

quota_used talks about the "owner" of an object using quota, but
of course it may be the group-owner of objects which have quota.

I think that if a server is to provide quota information through the
NFSv4 protocol it should provide some well defined and useful values,
without trying to cover all possibilities.
I would suggest:

 quota_avail_soft:  amount of extra disc space that can be allocated
         to this file before the user may reasonably be warned.  It is
	 understood that this space may be consumed by allocations to
	 other files, though there is rule as to which other files.

 quota_avail_hard: amount of extra disc space that can be allocated to
         this file before further allocations will be declined.  It is
	 understood that this space may be consumed by allocations to
	 other files.

 quota_used:  amount of disc space used by this file and possibly a
         number of other similar files, where the set of "similar"
	 files meets at least the criterion that allocating space to
	 any file in the set will reduce the "quota_avail_hard" of
	 every other file in the set.
         Note that there may be a number of distinct but overlapping
	 sets of files for which a quota_used value is
	 maintained. E.g. "all files with a given owner", "all files
	 with a given group owner". etc.
	 The server is at liberty to choose any of those sets, but
	 should do so in a repeatable way.  The rule may be configured
	 per-filesystem, or may be "choose the set wit the lowest
	 quota".

Given these three, then if a server supports only one style of quotas,
the implementation is obvious and expected.  If the server implements
multiple quota options, then the "avail" is clearly meaningful, and
"used" is at least indicative.

------------------------------------------

Section 8.2.4 "Sequencing of Lock Requests"
mentions a "monotonically increasing" sequence of integers.  However
the discussion implies that the sequence of integers must be
consecutive. 
( 1 2 5 7 8 9 200 is a monotonic sequence)

Also, the sequence gets initialised when the client verifier changes,
but we don't know what it gets initialised to.

-----------------------------------------------------

Section 8.2.3  Use of stateid

It is not clear to me what "changed" means in
 
   "The byte range of a lock is indivisible.  A range may be locked,
   unlocked, or changed between read and write but may not have
   subranges unlocked or changed between read and write."

does it mean "changed from READ to WRITE or vice versa".  In fact, is
one allowed to change a lock from READ to WRITE without unlocking
first?

---------------------------------

Section 8.5.2 "Server Failure and Recovery".

consider this scenario:
  A client has some locks on files on a server.
  The server restarts and returns "NFS4ERR_GRACE" to the client.
  The client dutifully reclaims all of its locks and then needs to make
  a new lock on some file.  

If it tries to request the lock straight away, the server may still be
in its "grace period" and so will return NFS4ERR_GRACE.  According to
paragraph 2, the client must now reclaim all of it's locks.

Either the client needs to wait for the grace period to pass, which
seems to require an inappropriate level of synchronisation, or there
are two different "sorts" of NFS4ERR_GRACE, one for attempts to use a
lock, meaning "please reclaim locks", and one for attempts to create
locks, meaning "please try again soon".  Should these be two distinct
error codes?

----------------------------------

Section 8.8 OPEN/CLOSE Procedures

The last paragraphs states that

        a file opened with deny READ/WRITE cannot be accessed using
        a file handle obtained through LOOKUP.

But a file handle from LOOKUP is (or can be) the same as a file handle
from OPEN?  Presumably it means:

        a file opened with deny READ/WRITE cannot be accessed using
	a stateid of all 0's or all 1's.

-------------------------------------------

Section 9.3 "Delegation and Callbacks", paragraph 3,  suggests that a
client may "request" a delegation.  This is confusing as the protocol
does not allow a client to request, only for a server to grant.

-------------------------------------

Section 3.2.3 Volatile File Handle
First paragraph ends:

   In all other cases where the server determines that a
   volatile file handle can no longer be used, it should return an error
   of NFS4ERR_EXPIRED.

This should be NFS4ERR_FHEXPIRED

----------------------------------------

Section 12.2.1 ACCESS

  ACCESS_DELETE: bit 5   Delete an existing directory entry.

Is this an access permission on the directory from which the file will
be delete, or on the file (or other object) which will be deleted from
the directory?

Also, I note that there is no "ACCESS_READLOCK" or
"ACCESS_WRITELOCK".  Can we assume that "ACCESS_MODIFY" implies
"ACCESS_WRITELOCK" and the "ACCESS_READ" implies "ACCESS_READLOCK"? 
What about "ACCESS_EXTEND"? Does that also implies "ACCESS_WRITELOCK"?

I would want the situation where I could give away read access to a
file, but still be confident that I would always write
it. i.e. nobody else should be able to lock it.  How does this fit
into the permission model?

----------------------------------

Section 12.2.10 LOCK

Lock returns an "access".  What does it tell me?

----------------------------------

Section 12.2.13  LOOKUP
Section 12.2.22  READDIR

We are told that "." and ".." are not special.
So, is it permitted for a
     LOOKUP "."
request to succeed and return the filehandle that it was given?  I
suspect not, but this should be explicitly stated.

Is READDIR allowed to return "." and ".." entries that refer to
current and parent directory (or indeed any entries that refer to
these directories)?  I suspect not but it should be stated.

----------------------------------

Section 12.2.32 VERIFY

The example, of using VERIFY to guard an append-mode write is a poor
example because due to the non-atomicity of COMPOUND requests (section
11.2), this doesn't guarantee proper append semantics.
For this to be useful, the server would need to guarantee that if
VERIFY succeeds, then it is executed atomically with the following
request.

------------------------------------

Section 13 "NFS Version 4 Callback Procedures"

States:
   The procedures used for callbacks are defined in the following
   sections.  In the interest of clarity, the terms "client" and
   "server" refer to NFS clients and servers, despite the fact that for
   an individual callback RPC, the sense of these terms would be
   precisely the opposite.

which by my reading says that the document will speak about a server
making a CALLBACK to a client.  However in Section 13.2 it twice talks
about a "server" executing  parts of a CB_COMPOUND procedure.  It
should say "client".

-----------------------------------------
Section 12.2.31 SETCLIENTID

The SETCLIENTID4args do not mention the cb_client callback
information, though the definition in Section 17 does.
Also the discussion in 8.2.1 doesn't mention it either.

It is not clear what sort of credential should accompany SETCLIENTID.
Presumably it is a credential for the client host, rather than for any
user (as all other credentials are). Is this true? Should this is stated?

------------------------------------------

---

From: Mike Eisler <[email-masked]>
Date: Wed, 13 Oct 1999 10:41:34 -0700 (PDT)

Neil,

I thought that most of the issues you raised around clarity of the specification
were quite valid. On some issues I have specific comments.

> other client has made a change" in the face of multiple changes in one

Then the language of the spec should state that mtime is permissable,
provided that file cannot be updated more frequently than resolution of
of mtime.

>     (mtime == current_time) ? 0 : mtime

While, NFS V4 is decidely quite stateful, in terms of GETATTR, it
it remains stateless. How is the server going to know when the 
client last read the attribute? Instead, I suggest the specification beef
up the language around the suggestion for using mtime, or
delete the reference to mtime.

> these directories)?  I suspect not but it should be stated.

Definitely "no" to either question.

> user (as all other credentials are). Is this true? Should this is stated?

It can be any credential. However, to reset the client id using the same value
of nfs_client_id.id, the same credential must be used. See section 8.2.1.

Practically speaking, a fat client like a UNIX system will likely use a constant
per host principal for generating the credential. Other types of clients
may not. The specification permits either, and it also permits the co-existance
of a a user-level NFS V4 client with a "native" client. It would be a mistake
to mandate a constant per host principal.

	-mre

---

From: Neil Brown <[email-masked]>
Date: Thu, 14 Oct 1999 13:22:59 +1000 (EST)

> delete the reference to mtime.

No statefulness was implied.  The meaning of "change == 0" was
intended to be:
   I cannot give you a repliable change value at the moment, because
   if I did and the file changed in a few milliseconds, then the only
   "change" number that I would would not be affected by that change,
   and I know you don't like that.

Essentially it is a way out for a server who cannot provide any
guarantees about whether the file has changed to not.  You're
wrist-watch server might always set change to 0, and require the
client to never do any caching.

I not particularly saying that having this is a good thing, but it is
important that "change" comes with guarantees, and this is a way to
allow a simple server to provide minimal guarantees.

> to mandate a constant per host principal.

I completely agree.  My point is that the spec should make clear what
is required of this credential.  Any rogue client that has access to
the same credential as was used for SETCLIENTID can launch a
denial-of-service attach by telling the server to forget all state
associated with that CLIENTID.  This should be clearly stated in the
document (currently it is implied) so that client implementers know
what is needed.

Thanks,
NeilBrown

---

From: Mike Eisler <[email-masked]>
Date: Thu, 14 Oct 1999 04:11:37 -0700 (PDT)

> client to never do any caching.

The change attribute is mandatory, so "no way out" is not allowed. :-)
The requirement is that if the file has changed, the change attribute
must change. If the file hans't changed, but the server can't
tell, then it should err on the side of caution and update the
change attribute.

	-mre