Store common Linux xattrs as native SA with xattr=sa (WIP)

[dweeezil]

There are 2 commits here. The first, dweeezil/zfs@c53847e, fixes deletion of system attributes.

The second is a intended to be the foundation for fixing #2718 but it should otherwise be able to stand on its own.

I've given this patch stack fairly light manual testing as well as running the "pjd" posixacl test suite and some short ztest runs.

I'm submitting this now as a functional WIP to get feedback from other developers regarding the overall concept and its implementation. There are a handful of issues I'll point out about the code:

• The list of xattrs to store as native SAs is stored statically in zpl_xattr.c which made it difficult to share with zdb. The same list (in a different format) exists within zdb.c as part of the new dump_znode_native_sa_xattr() function.
• The management of existing ZPL_DXATTR SAs required a helper function to translate from a zpl_attr_t to its name. As far as I could tell, there were no existing "upcalls" from the ZoL ZPL to the native Solaris ZPL. Rather than trying to #include the ZPL definitions in zfs_sa.c, I simply added an appropriate extern declaration. This goes against the current coding style.
• The ZPL_DXATTR SA cleanup performed by zfs_sa_remove_xattr adds overhead which could be avoided if it was known that no ZPL_DXATTRs exist. To that end, it's only invoked after determining whether a ZPL_DXATTR exists (which should be a low-overhead operation).
• I'd like to add a feature flag which is activated when a Native SA xattr is set for the first time; the feature flag would not be able to be deactivated. We might also want a feature flag to do the same with the existing ZPL_DXATTR SAs.

I'd also like feedback on the first commit in this stack. I discovered the problem during testing. My concern is that all other OpenZFS implementations likely share the identical code. I think the problem isn't seen often because SAs are rarely deleted. My test system created sort of a "perfect storm" by "stacking" all the variably-sized SAs at the ends of the registration.

[dweeezil]

@maxximino That's a very good question and is something I've been meaning to test. I whipped up a little script to set a bunch of posix acls:

#!/bin/sh

X=1000
echo blah > blah
while true; do
    setfacl -m u:$X:rw blah
    if [ $? -ne 0 ]; then
        echo failed at $X
        exit 0
    fi
    X=$(( $X + 1 ))
done

And I ran it on ext4, xfs and zfs with my patch.

(ext4)
# ./setfacl-max.sh 
setfacl: blah: No space left on device
failed at 1503

(xfs)
# ~/setfacl-max.sh 
setfacl: blah: Argument list too long
failed at 1021

(zfs with patch)
# ~/setfacl-max.sh 
^C
# getfacl blah | wc
   3211    3216   44925

I ended up interrupting it under zfs because the following was being logged:

[  278.899844] SPL: large kmem_alloc(25636, 0xd0) at zpl_set_acl:874 (3354738/3382344)
[  278.900921] SPL: large kmem_alloc(25644, 0xd0) at zpl_set_acl:874 (3355248/3382862)
[  278.902128] SPL: large kmem_alloc(25652, 0xd0) at zpl_set_acl:874 (3355758/3383380)
[  278.903303] SPL: large kmem_alloc(25660, 0xd0) at zpl_set_acl:874 (3356268/3383898)

Interesting, I didn't realize kmem_alloc() was being used like that up in the SPL layer. I suppose that can be fixed, or we can cap the size at some reasonable limit.

I suppose another question would be to ask what happens on a "normal" ZFS system when its native ZPL_DACL_ACES is too large. Clearly this is something that could happen under Solaris or Illumos.

It doesn't look like, at least for posix acls, that the size of an SA we can hold in a spill block is going to be a limiting factor relative to other file systems in common use.

Of course, this test is only working with posix ACLs, when selinux xattrs are added to the mix, the amount of space available for posix acls will be a bit lower.

That all said, the main reason for this scheme of storing these xattrs is to more easily allow them to be created as an atomic part of file/directory/symlink/whatever creation. Creating them atomically as either directory-style or ZPL_DXATTR-style xattrs would be a major pain in the butt, whereas creating them as native SAs will be fairly trivial. It wouldn't be terribly difficult to let a "set" operation allow to overflow into a traditional directory-style xattr in the context of my patch, but during the creation of a new object, they'd be created as native SA xattrs.

I'll work on refining the patch based on this information.

[dweeezil]

I replaced the kmem_alloc with vmem_alloc in zpl_set_acl() and zpl_get_acl() to get an idea how many posix acls we can fit:

# ~/setfacl-max.sh 
setfacl: blah: Argument list too long
failed at 9187

I'm going to poke around in the other native file systems to see how they handle storing the output of posix_acl_to_xattr().

[dweeezil]

@behlendorf Thanks for the review. I'd like to try to get a second version of this pushed within the next day.

[behlendorf]

I think the general design here is solid it just needs a little refinement.

As you mentioned in your original comment we should add a new feature flag for this. I think we should consider using the same feature flag for both the old ZPL_DXATTR SA functionality and the new native SAs added by this patch. One nice way to go about this would be to use the spa_feature_incr and spa_feature_decr functionality to count the number of znodes which are using the feature. This would allow us to disable the feature if there were no longer any consumers of it. One small wrinkle with this plan would be how to handle existing filesystems with SA xattrs.

It would be nice to use this pull request to build up a patch stack which completely addresses #2718. Patches early in the stack like this one can lay the needed ground work and subsequent ones can leverage it. Then the whole branch could be merged once everyone was happy with it.

It's also probably the opportunity to determine if we're really happy with the current xattr=SA behavior. I think there's probably a legitimate case to be made that the existing behavior is confusing and should perhaps be simplified.

[dweeezil]

@behlendorf My plan was definitely to layer the fix for #2718 atop this foundation. It would make perfect sense for it all to be done as part of this pull request.

[behlendorf]

@dweeezil I'm going to close out this PR for now but I do think this is an issue we'll need to eventually address. By all means open a PR when you have time to work on this again. There's some overlap with existing already merged patches.

21f604d Prevent duplicated xattr between SA and dir
43b4935 Prevent SA length overflow