On Thu, Feb 06, 2020 at 10:35:12AM -0500, Mike Marshall wrote:
> I looked at my code while thinking about your questions, and
> they seem like good ones. I have a couple of questions that will
> help me when I return to this in a few days:
>
> >> it used to be possible to do
> >> orangefs_set_acl(inode, NULL, ACL_TYPE_ACCESS)
>
> The way I tested (which maybe misses important stuff?) usually
> caused posix_acl_xattr_set -> set_posix_acl -> orangefs_set_acl ...
> Is there a simple userspace command that would send a NULL? When
> would there be a NULL?
setfattr -x system.posix_acl_access <filename>
works on ext4 and I don't see any way for it to work with current
orangefs_set_acl().
> I don't remember having trouble before, but now when I try to set
> an acl (on orangefs or ext4) that I think is expressible in pure mode,
> the mode doesn't change, rather the acl is still set... can you
> suggest a simple setfacl (or other) example I can use to test?
setfacl -b <filename>
works on ext4, goes by setxattr() with non-NULL acl that gets folded
into NULL by posix_acl_update_mode(). Sure, you call __orangefs_setattr()
there, so mode does get updated. What you don't do is telling the
server to get rid of xattr on that file. And I don't see where the
cached acl is dropped, but I might be missing something.
Note that e.g. ext4 does this:
if ((type == ACL_TYPE_ACCESS) && acl) {
error = posix_acl_update_mode(inode, &mode, &acl);
if (error)
goto out_stop;
if (mode != inode->i_mode)
update_mode = 1;
}
error = __ext4_set_acl(handle, inode, type, acl, 0 /* xattr_flags */);
if (!error && update_mode) {
inode->i_mode = mode;
inode->i_ctime = current_time(inode);
ext4_mark_inode_dirty(handle, inode);
}
the first part is more or less what your commit tries to do, but
note that __ext4_set_acl() is called in all cases; changing i_mode
is done after it, not instead of it. And __ext4_set_acl() does
set_cached_acl() in the end (on success, that is). So does
__orangefs_set_acl(), but you don't can it in that case; _maybe_
something else deals with that, but I don't see any plausible
candidates in there.
Sorry for the lack of direct orangefs testcases - I don't have
orangefs testbed set up right now, and IIRC setting it up had been
an interesting exercise...
