Subject: Re: [nfsv4] What is field 'attrset' in struct OPEN4resok designed for ?

From: "J. Bruce Fields" <[email-masked]>
Date: Fri, 5 Jun 2009 11:31:54 -0400

On Fri, Jun 05, 2009 at 04:23:45PM +0200, DENIEL Philippe wrote:
> at the page 171 of RFC3530, there is the definition for the structure  
> OPEN4resok. It looks like this:
>
>          struct OPEN4resok {
>               stateid4        stateid;        /* Stateid for open */
>               change_info4    cinfo;          /* Directory Change Info */
>               uint32_t        rflags;         /* Result flags */
>               _*bitmap4         attrset;   *_     /* attributes on
>    create */
>               open_delegation4 delegation;    /* Info on any open
>                                                                    
> delegation */
>       };
>
> There is a 'attrset' field in this structure. When doing a  
> 'OPEN4_NOCREATE' the server replies with OPEN4resok::attrset != 0, which  
> means there is a useful information located here (I do not understand  
> why since they are described as 'attributes on create' in the comments).  
> What is this field made for ? How should it be used ? What is the  
> client's behavior regarding this value ?

I think the only legimate use of the create attributes in the NO_CREATE
case is to implement O_TRUNC: so the size attribute can be specified and
set to 0, but other attributes should be ignored.

But that isn't spelled out in the spec.  The closest I see is:

	When an UNCHECKED create encounters an existing file, the
	attributes specified by createattrs are not used, except that
	when an size of zero is specified, the existing file is
	truncated.

Also the current linux server may not enforce this.

--b.

---

From: "Noveck, Dave" <[email-masked]>
Date: Mon, 8 Jun 2009 11:39:16 -0400

>	when an size of zero is specified, the existing file is

It doesn't get a 10 on a 0-10 how-clear-is-it scale, but if does imply
what you specified above.  It says the attributes are not used which
sounds like "ignored" to me except that specifying a zero for size
causes a truncate.

Clearly attributes other than size are ignored.

And if the attibute size is specified and is not zero, it says the
attributes are not used.

I'll give it a 6.

-----Original Message-----
From: J. Bruce Fields [mailto:[email-masked]] 
Sent: Friday, June 05, 2009 11:32 AM
To: DENIEL Philippe
Cc: [email-masked]; [email-masked]
Subject: Re: What is field 'attrset' in struct OPEN4resok designed for ?

--b.
_______________________________________________
NFSv4 mailing list
[email-masked]
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

---

From: "J. Bruce Fields" <[email-masked]>
Date: Mon, 8 Jun 2009 16:45:53 -0400

> And if the attibute size is specified and is not zero, it says the

OK!  So I guess the right thing for a server to do on an non-creating
open would be to clear all but the SIZE bit in the returned attribute
bitmap?

(And returning EINVAL in the NOCREATE case with anything other than the
SIZE bit set would also seem reasonable.)

Hm.  On a quick glance, the linux server code doesn't look right here.

--b.

---

From: "Noveck, Dave" <[email-masked]>
Date: Mon, 8 Jun 2009 17:03:53 -0400

Now I see the ambiguity which I didn't see before.  I interpreted this
from the server's point of view.  No big surprise.  So that the
attributes not being used meant that the server didn't look at them. So,
NFS4ERR_INVAL wouldn't be right in this situation since it depends on
looking at attributes it isn't supposed to.

If you look at it from the client's point of view, the attributes other
than size not being set, means that the client should not set them,
which everybody agrees with, but it then implies that if the client sets
them, it is doing something wrong, and so INVAL makes sense to return in
this case.

> I'll give it a 6.

I think that should go down to a three.  There is ambiguity about what
the server should do but it is clear that the client should not mess
with fields other than size and the size if set should be set to zero.
And if the client does obey that, he isn't hurt by ambiguity about what
the server should do in other cases. 

I think there are probably other cases like this where we describe how
the client and server work together to do the right thing.  That often
tends to lead one to omit various cases where one or the other don't
work together as they should.

-----Original Message-----
From: J. Bruce Fields [mailto:[email-masked]] 
Sent: Monday, June 08, 2009 4:46 PM
To: Noveck, Dave
Cc: [email-masked]; [email-masked]; DENIEL Philippe
Subject: Re: [nfsv4] What is field 'attrset' in struct OPEN4resok
designedfor ?

--b.
_______________________________________________
nfsv4 mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/nfsv4

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 9 Jun 2009 14:24:30 -0400

> with fields other than size and the size if set should be set to zero.

Sounds fine, but I don't *think* that's explicitly stated anywhere.

--b.

---

From: "Noveck, Dave" <[email-masked]>
Date: Tue, 9 Jun 2009 15:42:55 -0400

Actually I don't think that is right because the client may not know if
a file is to be created or truncated.

Here's my version of the the existing text (possibly for rfc3530bis).

   UNCHECKED means that the file should be created if a file of that
   name does not exist and encountering an existing regular file of that
   name is not an error.  For this type of create, createattrs specifies
   the initial set of attributes for the file.  The set of attributes
   may include any writable attribute valid for regular files.  When an
   UNCHECKED create encounters an existing file, the attributes
   specified by createattrs are to be ignored by the server, except that

   when a size of zero is specified, the existing file is truncated as 
   part of processing the OPEN.  If GUARDED is specified, the server 
   checks for the presence of a duplicate object by name before 
   performing the create.  If such a duplicate exists, an error of 
   NFS4ERR_EXIST is returned as the status.  If the object does
   not exist, the request is performed as described for UNCHECKED.  

   For each of these cases (UNCHECKED and GUARDED) where the operation 
   is successful, the server will return to the client an attribute mask
   signifying which attributes were successfully set for the object.  In

   the case in which UNCHECKED was specified and an existing object was
   encountered, then the bits for all attributes other than size will be
   zero.  In the that case, the bit for size will be set if and only the

   file was truncated because the size attribute was specified as zero.

Does anyone disagree with this?

Is it clearer than what is there now?

-----Original Message-----
From: J. Bruce Fields [mailto:[email-masked]] 
Sent: Tuesday, June 09, 2009 2:25 PM
To: Noveck, Dave
Cc: [email-masked]; [email-masked]; DENIEL Philippe
Subject: Re: [nfsv4] What is field 'attrset' in struct
OPEN4resokdesignedfor ?

--b.

> > > case is to implement O_TRUNC: so the size attribute can be
specified
> > and

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 9 Jun 2009 16:12:12 -0400

> Is it clearer than what is there now?

Yes.  But, um, I realize I've been an idiot: Philippe was asking about
the NOCREATE case, which I'd forgotten doesn't take any attributes (the
client implements O_TRUNC with a separate SETATTR operation).  In that
case the returned attrset is useless and should be ignored, and if a
server isn't zeroing it out then that may indicate a bug, since that may
mean it's just returning uninitialized memory....

--b.

---

From: Trond Myklebust <[email-masked]>
Date: Tue, 09 Jun 2009 16:59:50 -0400

> mean it's just returning uninitialized memory....

Our client does that, but afaik the intention with OPEN was to allow
clients to do an atomic open(O_TRUNC). Why would you treat the case of
OPEN_NOCREATE as being different from the case of an UNCHECKED open
where the file already exists?

Trond

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 9 Jun 2009 17:03:16 -0400

On Tue, Jun 09, 2009 at 04:59:50PM -0400, Trond Myklebust wrote:
> clients to do an atomic open(O_TRUNC).

OPEN doesn't have any any attributes or truncate bit in the NOCREATE
case.  So if you're right about the intention, then that'd have to be
fixed in a future protocol revision.  (Or maybe I'm missing something.
The OPEN xdr isn't my favorite to read.)

--b.

> Why would you treat the case of

---

From: Trond Myklebust <[email-masked]>
Date: Tue, 09 Jun 2009 17:13:40 -0400

> The OPEN xdr isn't my favorite to read.)

Oops. You're right... I guess the only way to do it is to put the
SETATTR in the same compound and use the NFSv4.1 'current stateid'.

Trond

---

From: "Noveck, Dave" <[email-masked]>
Date: Tue, 9 Jun 2009 17:21:35 -0400

The big mistake was OPEN4_NOCREATE.  Instead of having that create a
second level of union, we should have just omitted that and had an
additional createmode, JUSTOPEN4 and either have a single bool for
whether we truncate or the same fattr4 as for {UNCHECKED,GUARDED}4.

This is kind of obvious ten years later.  Sigh.

-----Original Message-----
From: Trond Myklebust [mailto:[email-masked]] 
Sent: Tuesday, June 09, 2009 5:14 PM
To: J. Bruce Fields
Cc: DENIEL Philippe; [email-masked]; Noveck, Dave; [email-masked]
Subject: Re: [nfsv4] What is field 'attrset' in
structOPEN4resokdesignedfor ?

Trond

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 9 Jun 2009 17:24:22 -0400

> SETATTR in the same compound and use the NFSv4.1 'current stateid'.

Is that much different from sending the SETATTR separately?  I guess
you'd still have to handle the possibility of the SETATTR failing after
the OPEN succeeded.  If you implemented open(O_TRUNC) as

	send OPEN+SETATTR
	if OPEN failed:
		return error
	if SETATTR failed:
		send CLOSE
		return error
	return success

would an application be able to detect any lack of atomicity?  Seems
likely to be good enough.

--b.

---

From: Trond Myklebust <[email-masked]>
Date: Tue, 09 Jun 2009 17:44:33 -0400

> likely to be good enough.

It's not as good as a proper atomic OPEN, but it _is_ better than doing
2 fully synchronous RPC calls as in your example above.
So in NFSv4.1 it makes more sense to do something like

	PUTFH
        OPEN(OPEN_NOCREATE, "foo")
	GETFH
	GETATTR
	NVERIFY size==0
	SETATTR stateid=0x00000001 00000000 00000000 00000000, size=0

Trond