openzfs: Add support for Solaris style extended attributes

[rmacklem]

Summary

I've put the patch here first. Once this review is completed, I will
take it over to OpenZFS and discuss it there, plus propose a pull request.

Since ZFS was developed for Solaris, Solaris style extended attributes
(which I call named attributes, which is the NFSv4 name) are already
implemented in OpenZFS. This VFS/KAPI interface presents a named
attribute directory that is associated with a file object, which holds
regular files that are the attributes. The directory is read with getdents(2)
or getdirentries(2) and the attributes found in the directory with regular
file I/O such as read(2), write(2), lseek(2) and ftruncate(2).

This patch provides the changes to use this alternate KAPI to access
and modify extended attributes through the FreeBSD VFS, as patched
by commit 2ec2ba7e232d to main/freebsd-current.

A few notes:

• Since the named attribute directory is associated with a file object

found under the same name in the directory tree, name caching must
be disabled.

• Two new flags called LOOKUP_NAMED_ATTR and V_NAMEDATTR are

used to indicate that zfs_zaccess() must check permissions for the
attributes.

• The permission checking code in zfs_zaccess() that is used when the

above flag(s) are set is cribbed directly from zfs_zaccess() in the
Linux branch. (openzfs/module/os/linux/zfs/zfs_acl.c).

• zfs_check_attrname() was not changed. It was simply moved up

in the source file so that it could be called in zfs_freebsd_create().

The new named attribute KAPI is supported when the "xattr" property
is set to "on"/"dir" (I think they are synonymous?). Setting "xattr" to "sa"
or "off" disables it.
If this is not felt to be sufficient, a new setting for the "xattr" property
could be defined.

Note that the extended attributes can be manipulated by either KAPI
(this Solaris-like one or the FreeBSD/Linux extended attribute model).
The only limitation is the size restriction enforced for extattr_get_[fd|file]()
and extattr_set_[fd|file](). Storage of extended attributes does not change
and this works fine for the "dir" version (but not the "sa" version, as noted above).

Test Plan

This has seen a modicum of testing in a FreeBSD kernel
patched with 2ec2ba7e232d. The FreeBSD NFSv4 client
and server have also been patched to use them over NFSv4.

Further testing with versions of NFSv4 (Solaris) that already
supports named attributes will be done in the next few weeks.
(I might also find someone with a Mac who can test them
using their NFSv4 client.)

[rmacklem]

Fixed a bug when open'ng "." in a named attribute directory.

I also added a check to enforce the requirement for O_NAMEDATTR
be specified whenever open'ng either a named attribute directory
or a named attribute.

[rmacklem]

Cleanup up zfs_freebsd_lookup(), including adding some
comments to try and explain the named attribute case.

The only semantics change is using cn_lkflags instead
of LK_SHARED when locking the named attribute directory.

[rmacklem]

Checking for VIRF_NAMEDATTR in zfs_freebsd_open()
was not needed and broke the runat(1) utility I am writing.

This patch removes that check.

[rmacklem]

I am now convinced that the check in zfs_freebsd_open()
for O_NAMEDATTR being set is not necessary at all.

This version of the patch has the check deleted.

[rmacklem]

I misinterpreted the Solaris documentation for O_XATTR.
It is used for an openat() where the path is for a file in
the file system's namespace, to open a named attribute
for the file, not a named attribute directory.

To open a named attribute directory, the openat() is
given a path argument of ".".

Since Solaris compatibility seems appropriate where
practicable, this patch changes the behavior to the above
for O_NAMEDATTR.

[rmacklem]

Added support for _PC_HAS_NAMEDATTR, which is
mainly a new function called zfs_has_namedattr().

I also changed the #if's from using VIRF_NAMEDDIR
to __FreeBSD_version >= 1500040, which is more
consistent with the rest of the code.

[mav]

sys/contrib/openzfs/include/os/freebsd/spl/sys/vnode_impl.h

226–231

Are names attributes different from extended attributes here? Do we need the additional flag?

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c

1265

ZSB_XATTR and z_xattr_sa might change in run time, with the xattr property. It is not read-only. The extended attributes code checks both locations on read, so switching it live is not fatal for already existing ones. With names attributes only accessing directories the two KPIs may start returning different values if the property value is changed live. May need consideration.

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c

4704

Will this list attributes with "forbidden" prefixes? Should it?

4779–4780

is_nameddir said twice.

[rmacklem]

Good questions! Maybe they need to be asked on a mailing
list for FreeBSD, but at least I'd like to hear your thoughts.

Thanks for looking at it, rick

sys/contrib/openzfs/include/os/freebsd/spl/sys/vnode_impl.h

226–231

More accurately, the mechanism used to manipulate
the extended attributes is different.
This flag indicates that the "Solaris style mechanism"
(called named attributes in the NFSv4 RFCs) is being used.

Maybe the name should be changed somewhat. Any suggestions?
(LOOKUP_XATTR_NAMED_ATTR maybe??)

Btw, the main purpose of the flag is to indicate that the other
related flag V_NAMEDATTR gets set for the zfs_zaccess() call.

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c

1265

Yep. Flipping back and forth between "dir" and "sa" is a little
weird. "setextattr system ..." can only be done if xattr is set to "sa".
Attributes created via setextattr when xattr=sa are invisible to
named attributes (the Solaris mechanism), but ones created when
xattr=dir are visible as named attributes and via lsextattr and friends.

There is also some weirdness w.r.t. "zfs get xattr" I do not understand.
Sometimes it reports "sa" or "dir". Others it reports "on". I need to look
at that at some point.

If you think this will be too confusing, I could create a new setting for
xattr (maybe xattr=named) and then this setting would only take effect during
startup or remount and would disallow lsextattr and friends.
--> But that sounds confusing too.

So, I'm not sure what the right answer is for this?

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c

4704

I think it currently does list them.
However, since they are created via Linux, I don't have
any way to test it. (The FreeBSD system space ones can
only be done when xattr=sa and don't show up when
xattr=dir, as noted in the other reply.)

Should it list them?
I don't know, since Solaris lists everything,
but assumes that there won't be any weird/forbidden
ones.

Since zfs_listextattr_dir() strips out the forbidden ones,
I am thinking that zfs_readdir() should do so as well
for the named attribute dir, for consistency.

What do you think?

Good catch, btw. I filtered out the forbidden ones in
zfs_has_namedattr(), but did not add code to zfs_readdir()
to do so and I am now thinking it should filter them out?

4779–4780

For emphasis;-)

I'll fix it.

[rmacklem]

The main change is to zfs_readdir(), so that it skips over
forbidden names for a named attribute directory.

It also fixes a bug my patch introduced, where a
"setextattr system ..." would fail for the xattr=dir property case
and a trivial typo fix to a comment that mav@ spotted.

[rmacklem]

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c

1265

This version of the patch allows "setextattr system ..." to work
for xattr=dir. Not allowing it was a bug in my previous patch.

I think that "setextattr system" needs to work, since some
applications may set "system" attributes and those cannot
be done via the named attribute mechanism.

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c

4704

This version of the patch skips over forbidden names,
which I now believe is correct, since it is consistent
with what "lsextattr user ..." does.

[mav]

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c

1265

There is also some weirdness w.r.t. "zfs get xattr" I do not understand.
Sometimes it reports "sa" or "dir". Others it reports "on". I need to look
at that at some point.

Unfortunately, unlike "on" for "compression", which is a separate value, "on" for "xattr" is just another name for one of explicit options. Originally it mapped to "dir", but lately was changed to "sa", which only increased the confusion.

If you think this will be too confusing, I could create a new setting for
xattr (maybe xattr=named) and then this setting would only take effect during
startup or remount and would disallow lsextattr and friends.

I don't think adding more options would make it any cleaner. I am trying to think towards any possible better integration between "sa" and "dir". For the existing xattr API it is not very important what way "xattr" property is set -- on get it can get any available (expecting only one to exist), and on set, it makes sure to wipe possible other variant. Sure having some of attributes stores in "sa" via old API invisible via new is annoying, but I think having two variants stored same time may be even worse. I wonder if we could delete "sa" attributes when creating "dir" with the same name via the new API. Though if some attributes are not visible in some way, it may create difficulties and uncertainty for some copy/replication software.

[rmacklem]

sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c

1265

All I can think of is documenting that switching back and forth
will cause grief and, if a file system is going to use the Solaris
way (or named attributes, if you prefer), xattr=dir needs to be
set right away and left that way.

Something like this could go in the zfsprops man page.
(I haven't patched it yet, but assumed I would at some point.)

I don't think copying the sa attributes into dir attributes will work
well, given the prefixes on the sa attributes and the case where there
are two attributes (one sa and one dir) with the same name.

Since for xattr=sa there can be dir attributes (ones too big to fit in
the sa block), I suspect the software will work now. I haven't played
with tar yet, but so long as it works for large ones (using the current
Linux style model), it should get everything.

The problematic case occurs when a small attribute is created when
xattr=sa and then an attribute of the same name is created after
changing to xattr=dir.
Maybe the Solaris style create should check to see if the same name
already exists in the sa block and if it does, refuse to create it?
(Or would that just create more confusion?)