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

From: Christoph Hellwig <[email-masked]>
Date: Sat, 4 Oct 2014 18:17:50 +0200

Hi Manoj,

can you explain what xattr model your draft is based on?  It seems to
have extremly complicated semantics that include a lot of semantics
not present in Linux/IRIX/FreeBSD xattrs.

On the GETXATTR side you conflate the LISTXATTR operation with GETXATTR,
as well as adding a new bulk get one that isn't present in any OS API
I know.  Both the argument and return structures are basically entirely
different for the different cases.

I think we would be much better of starting with:

GETXATTR:

struct GETXATTR4args {
	/* CURRENT_FH: file */
	xattrname4	ga_name;
}

struct GETXATTR4res {
	xattrvalue4	gr_value;
}

LISTXATTR:

struct LISTXATTR4args {
	/* CURRENT_FH: file */
}

struct LISTXATTR4res {
	xattrname4	gr_names<>
};

Also how is the client supposed to cope with sizes of both each
xattr and the list?  Don't we need a maxcount/mincount scheme similar
to READDIR or GETDEVICEINFO?

In the SETXATTR I would suggest to remove the _ALL types, and the
multiplexing not present in any OS API.  Additionally we need an
anything goes type that can create or replace to properly support
the existing Linux/IRIX/FreeBSD semantics.  The remove case could
be roled into this - while none of the OS APIs do this, the Linux
kernel internal API does and it works fairly well, although I still
think a separate operation would be cleaner protocol wise:

enum setxattr_type4 {
	SETXATTR4_ANY		= 0,
	SETXATTR4_CREATE	= 1,
	SETXATTR4_REPLACE	= 2,
}

struct SETXATTR4args {
	/* CURRENT_FH: file */
	setxattr_type4	sa_type;
	xattrname4	sa_xattrname;
}

struct SETXATTR4res {
	nfsstat4 sr_res;
}

struct REMOVEXATTR4args {
	/* CURRENT_FH: file */
	xattrname4	sa_xattrname;
}

struct REMOVEXATTR4res {
	nfsstat4 sr_res;
}

A few more comments on the standard outside of this high level model:

In Section 2. mentioning Solaris is wrong - Solaris never exposed xattr-like
APIs, just name attributes which are different and supported in NFSv4.

Instead of mentioning NTFS you should mention the API, preferably with
an informative reference.

Asking about not interpreting the name is a desaster waiting to happen
given that Linux differs from the other implementations in how it handles
the name.  I sent a separate mail about this earlier today, but I think
trying to handle anything but the "user" xattrs in this protocol would
be a grave mistake, so a Linux client would have to strip the "user."
and a Linux server would have to add this "user."

Section 5.1:  a very useful attribute missing here is the maximum size
of each individual attribute.

---

From: Manoj Naik <[email-masked]>
Date: Sat, 4 Oct 2014 13:34:04 -0700

Christoph Hellwig <[email-masked]> wrote on 10/04/2014 09:17:50 AM:

> From: Christoph Hellwig <[email-masked]>
> To: Manoj Naik/Almaden/IBM@IBMUS, [email-masked]
> Date: 10/04/2014 09:17 AM
> Subject: xattr model in draft-naik-nfsv4-xattrs-01?
>
> Hi Manoj,
>

Why do you say they are complicated? Section 5.3 lists what we need:

   Applications need to perform the following operations on a given
   file's extended attributes [5]:

   o  Given a file, return a list of all of the file's assigned extended
      attribute keys.

   o  Given a file and a key, return the corresponding value.

   o  Given a file, a key, and a value, assign that value to the key.

   o  Given a file and a key, remove that extended attribute from the
      file.

The operations capture those requirements.

> different for the different cases.

Agree that GETXATTR includes what is traditionally provided by listxattr()
call. The idea was to limit the number of new operations provided to two -
GETXATTR and SETXATTR. But if there is consensus, we don't have an issue
with adding a LISTXATTR instead.

I think the ability to get/set all xattrs of a given file
(getallxattrs/setallxattrs?) is useful, especially for backup/restore
programs. Granted that there isn't an OS API that exposes this, but perhaps
there should be one? I know some backup utilities that work directly with a
filesystem API to do this. Especially over NFS, I believe a single RPC that
accomplishes this may be desirable. I know somebody on the list or at the
IETF meeting asked for this specifically too.

> to READDIR or GETDEVICEINFO?

Explain? There is no limit imposed on the size of each xattr (as long as it
is <= total size of all xattrs, i.e. maxxattrsize).

> struct REMOVEXATTR4args {

Don't have a problem with this - we can add separate LISTXATTR/REMOVEXATTR
if there is consensus. I'd still like to retain the ALL option though.

> APIs, just name attributes which are different and supported in NFSv4.

OK. See the referenced
http://docs.oracle.com/cd/E19253-01/816-5175/6mbba7f02/. Solaris still
calls them "extended file attributes", but you're right and I'll remove it.

> an informative reference.

Probably - I can't find any informative references to this though.
Pointers?

> and a Linux server would have to add this "user."

I agree. Tom pointed out in his review of the first draft that it might
make sense to encapsulate some security attrs as xattrs if they could be
generalized to be platform-independent. Hence I added this to Section 3:

                                               Attributes from other
   namespaces are typically vendor-specific, but some of them may be
   generalized into well-defined set of names that promote interoperable
   implementations.

What wording can we add to enforce this? We've said the xattrs are
completely opaque (both in terms of key names and values), so adding a
namespace prefix in the key should be avoided. I like the idea of the
(Linux) client/server stripping off the "user." prefix, but can we do
better than adding strong verbiage that only attributes from the user
namespace can be specified?

> of each individual attribute.

Why do we need this? I see file systems limit the total space taken by
xattrs, but not the maximum size of individual attributes. Of course, we
can add it if needed.

Thanks for the comments.

Manoj.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Sat, 4 Oct 2014 21:12:20 -0400

> IETF meeting asked for this specifically too.

What does this allow you to do that you can't do by sending multiple
RPCs, or a single RPC with multiple getxattr/setxattr ops.

--b.

---

From: Christoph Hellwig <[email-masked]>
Date: Sun, 5 Oct 2014 09:22:28 +0200

> The operations capture those requirements.

While they happen to also implement those the focus seems to be on a lot
of more complicated cruft, thus making the straight forward
implementation of those requirements unessecarily complex.

> with adding a LISTXATTR instead.

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.

> accomplishes this may be desirable.

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.

> is <= total size of all xattrs, i.e. maxxattrsize).

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.

All of them get passed a fixed sized pointer for the return buffer.  On
IRIX the size paramter for the attr_get operation is an IN/OUT argument
using a pointer, so when the size is not large enough it returns the
expected size. This baiscally is a 1:1 mapping to the maxcount/mincount
scheme in XDR.  Linux and FreeBSD aren't quite as smart and instead
just return an error, but then expect the user to do a get or list
call with a 0 size and NULL buffer to query the size of the attribute
or attribute list.

This also is a reason why the get all argument operation was never added
to any OS - expressing this concept as a C API is incredibly complex
with all those buffer sizing issue - you basically have to pass in the
biggest possible buffers for the largest possible number of attributes,
unless you want to do callee allocated buffers which isn't possible
for syscall-level APIs.

> if there is consensus. I'd still like to retain the ALL option though.

Again they aren't in OS APIs for a good reason.  Besides an utterly
horrible and crufty inferface they also are against the concept of the
attr model where each attribute stands on it's own.  The concept of
replacing or deleting all xattrs does not fit into any of the OS ABIs.

I strongly disagree with NFS standards creating new unimplementable
operations that further increase the deadwood in the specification.

> Probably - I can't find any informative references to this though.

Ask the Microsoft representatives on the WG?  If it's not important enough
we might as well strike the reference, as an ondisk format isn't very
relevant for NFS standardization.

> namespace can be specified?

I don't think standardizing anything else is even remotely possible,
all other attrs are higly OS-specific non-opaque data structures, so
even leaving the options for them open is contrary to wording in much
of the draft.

> xattrs, but not the maximum size of individual attributes. Of course, we

I don't known of a any (Linux) filesystem that has limits on the total
size of all extended attributes.  There limits on the size of each
attribute, further complicated by the fact that the extN limitations
if for key+value combined, while the others I know only have individual
limits for key and value.

---

From: Nico Williams <[email-masked]>
Date: Mon, 6 Oct 2014 00:06:49 -0500

IMO:

 - for listing attributes the READDIR model should be followed (a
   pageable list+get operation)

 - adding new filesystem-relatedsystem calls and [NFS, ...] protocol
   functionality is a catch-22 kind of deal, and if we always require
   the other to be present first then the one can't ever happen;

   I'm inclined to say that a get/setallxattrs() should be supported,
   BUT:

    - it must be clear that it won't be atomic because it can't be
      atomic

    - because it has to be pageable (and the protocol in general doesn't
      support atomicity in such cases)

Nico
--

---

From: Casey Bodley <[email-masked]>
Date: Mon, 6 Oct 2014 14:26:21 -0400

> IETF meeting asked for this specifically too.

The Windows system calls and kernel interfaces operate on linked lists
of extended attributes. You can see their interfaces in the msdn
documentation for ZwQueryEaFile/ZwSetEaFile and
IRP_MJ_QUERY_EA/IRP_MJ_SET_EA.

---

From: Manoj Naik <[email-masked]>
Date: Mon, 6 Oct 2014 13:59:49 -0700

Christoph Hellwig <[email-masked]> wrote on 10/05/2014 12:22:28 AM:

> To: Manoj Naik/Almaden/IBM@IBMUS
> Cc: [email-masked]
> Date: 10/05/2014 12:22 AM
> Subject: Re: xattr model in draft-naik-nfsv4-xattrs-01?
>

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 :)

> 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.

> are specified you'll see that they take special care of this at the API

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.

> limits for key and value.

OK, so we need an option to specify limit on individual xattr size. But
there are file systems that place limits on the total size as well.

Manoj.

---

From: Manoj Naik <[email-masked]>
Date: Mon, 6 Oct 2014 14:08:49 -0700

"J. Bruce Fields" <[email-masked]> wrote on 10/04/2014 06:12:20 PM:

> From: "J. Bruce Fields" <[email-masked]>
> Cc: Christoph Hellwig <[email-masked]>, [email-masked]
> Date: 10/04/2014 06:12 PM
> Subject: Re: [nfsv4] xattr model in draft-naik-nfsv4-xattrs-01?
>

You could certainly do this with multiple ops, but not with a single
COMPOUND. See SETXATTR4_REPLACE_ALL in Section 5.3.3.3.

Manoj.

---

From: Manoj Naik <[email-masked]>
Date: Mon, 6 Oct 2014 14:16:18 -0700

Nico Williams <[email-masked]> wrote on 10/05/2014 10:06:49 PM:

> From: Nico Williams <[email-masked]>
> To: Christoph Hellwig <[email-masked]>
> Cc: Manoj Naik/Almaden/IBM@IBMUS, [email-masked]
> Date: 10/05/2014 10:06 PM

OK, we've been trying to avoid this at least for the first couple of draft
revisions. The current draft (Section 5.2.1) states:

                      In addition, the total size of xattrs exchanged
   between the client and server for get/set operations is limited by
   the channel's negotiated maximum size for requests and responses.

Granted, we probably need to revisit this. Requiring a single xattr get/set
to fit in a single COMPOUND makes the specification much simpler. (This
also seems to be the implicit requirement for other file attributes like
ACLs). Perhaps we can (should?) allow list/getall/setall to follow the
READDIR model.

>     - it must be clear that it won't be atomic because it can't be

This is documented in the draft (Section 5.3.3.3):

   While the SETXATTR request MAY contain multiple attribute keys and/or
   values to be changed for a file, this does not impose any atomicity
   considerations on the server implementation. If the server cannot
   update all the attributes for the file atomically, it MUST set them
   in the order specified.

I'll make sure this is added for the GETXATTR section as well.

Manoj.

---

From: Nico Williams <[email-masked]>
Date: Mon, 6 Oct 2014 16:22:44 -0500

On Oct 6, 2014 4:16 PM, "Manoj Naik" <[email-masked]> wrote:
> OK, we've been trying to avoid this at least for the first couple of
draft revisions. The current draft (Section 5.2.1) states:
> Granted, we probably need to revisit this. Requiring a single xattr
get/set to fit in a single COMPOUND makes the specification much simpler.
(This also seems to be the implicit requirement for other file attributes
like ACLs). Perhaps we can (should?) allow list/getall/setall to follow the
READDIR model.

Yes, but the implementation also had to be simple.  And feasible.  IMO you
cannot avoid a paging requirement.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Mon, 6 Oct 2014 17:23:25 -0400

> be achieved in a single COMPOUND.

A separate REMOVEXATTR would also make it easier to make it optional.  A
server could just not implement the one operation.

And easier to remove from the draft when we eventually realize nobody
cares about it, hah.

--b.

---

From: Manoj Naik <[email-masked]>
Date: Mon, 6 Oct 2014 15:36:53 -0700

"J. Bruce Fields" <[email-masked]> wrote on 10/06/2014 02:23:25 PM:

> Date: 10/06/2014 02:23 PM

I am not sure if you meant to say that in jest, but I would think a server
that implements SETXATTR would implement REMOVEXATTR as well. The latter
would be the only way to delete xattrs, if we decide to separate it from
the current SETXATTR.

Manoj.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Mon, 6 Oct 2014 18:49:04 -0400

> would be the only way to delete xattrs, if we decide to separate it from

Oh, sorry.  Looking at the spec....  I assumed you were proposing a
separate op for DELETE_ALL.  Then REPLACE_ALL could be implemented as
DELETE_ALL plus a bunch of sets.

Anyway, clearly I should actually read the latest draft....

--b.

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 7 Oct 2014 11:37:23 +0200

> 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.

> 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.

> 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.

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 7 Oct 2014 11:38:47 +0200

>    pageable list+get operation)

While I'm still concerned about the lack of list+get in OS APIs
if we want to support a list+get that is indeed the way to go.  A real
GETXATTR similar to GETATTR and a LISTXATTR that is the readdir equivalent.