Re: [nfsv4] xattr model in draft-naik-nfsv4-xattrs-01?

[Christoph Hellwig <[e-mail]>]

On Mon, Oct 06, 2014 at 01:59:49PM -0700, Manoj Naik wrote:
> > Why would you want to limit the number of operations?  Adding a new
> > compound operation just is a new entry in a new dispatch table, which
> > adding tons of unions complicates parsing at runtime, bloats data
> > structures and makes the standard unreadable, thus placing a burden on
> > human and machine.
> 
> I think you're referring to this:
> 
>         union getxattr_res4 switch (getxattr_type4 gr_type) {
>          case GETXATTR4_LIST:
>                 xattrname4    gr_names<>;
>          case GETXATTR4_ONE:
>                 xattrvalue4   gr_value;
>          case GETXATTR4_ALL:
>                 xattr4        gr_xattrs<>;
>         };
> 
> This isn't any more bloated/unreadable than a lot of existing protocol
> definitions and operations. I am not arguing against LISTXATTR/REMOVEXATTR
> being separate operations, but there is precedence in the existing spec for
> operations that are far more complicated that this :)

It's pretty awkward together with how the args structure is defined.
The actual GET operation has indeed zero overlap with a LIST one,
as the GET operates on a name, while LIST just operates on the implicit
FH.

> > Setting multiple attributes can easily be archives by multiple SETXATTR
> > operations in a compound.  Getting multiple attributes in one operation
> > might sound useful, but there is a reason why it hasn't be implemented
> > so far, and the main reason is that it's not sanely implementable at
> > the OS lewvel.  More on that below.
> 
> An operation that replaces ALL the xattr keys of a file with another set of
> keys could be done in a compound if we added a separate REMOVEXATTR that
> allowed deleting all the xattrs of a file. Otherwise this would need a
> LISTXATTR, delete individual xattrs and SETXATTR each one, and this cannot
> be achieved in a single COMPOUND.

What is the use case for a REPLACE_ALL again?  In the OS xattr model,
attributes are separate, so there never is need for a REPLACE_ALL.  This
gets even more complicated if you want to support additional xattr
namespaces, as you have to take permissions into account, or the
fact that some of the non-user namespaces can't even be replaced.

I stand to my opinion that these operations really do not belong into
the xattr model.  And just to be clear: this is different from the
list+get operation which does fit into the model, although I don't
see it as useful due to the lack of implementations.

> > Both setxattr and listxattr return data that basically is unbound for
> > a simple request.  While this isn't a problem at the XDR level it is a
> > problem at the syscall level.  If you look how the get and list
> operations
> > are specified you'll see that they take special care of this at the API
> > level.
> 
> This is not unbounded for the protocol either. Section 4 states:
> 
>    Similar to ACLs, the amount of xattr data exchanged between the
>    client and server for get/set operations can be considered to fit in
>    a single COMPOUND request, bounded by the channel's negotiated
>    maximum size for requests.
> 
> More on relaxing this requirement (for list/getall/setall) separately.

That's why I said basically unbounded and not unbounded.  The session
limit is still large enough that you don't want your application
to allocate each buffer for a get to fit the session limit.