Re: [PATCH v2 1/8] fs: don't use IOP_XATTR for posix acls

[Christoph Hellwig <hch@lst.de>]

On Mon, Jan 30, 2023 at 05:41:57PM +0100, Christian Brauner wrote:
> The POSIX ACL api doesn't use the xattr handler infrastructure anymore.
> If we keep relying on IOP_XATTR we will have to find a way to raise
> IOP_XATTR during inode_init_always() if a filesystem doesn't implement
> any xattrs other than POSIX ACLs. That's not done today but is in
> principle possible. A prior version introduced SB_I_XATTR to this end.
> Instead of this affecting all filesystems let those filesystems that
> explicitly disable xattrs for some inodes disable POSIX ACLs by raising
> IOP_NOACL.

I'm still a little confused about this, and also about
inode_xattr_disable.  More below:

> -	if (!(old->d_inode->i_opflags & IOP_XATTR) ||
> -	    !(new->d_inode->i_opflags & IOP_XATTR))
> +	if (inode_xattr_disabled(old->d_inode) ||
> +	    inode_xattr_disabled(new->d_inode))

This code shouldn't care about ACLs because the copy up itself
should be all based around the ACL API, no?

> +	if (!(inode->i_opflags & IOP_NOACL))
>  		error = set_posix_acl(mnt_userns, dentry, acl_type, kacl);
>  	else if (unlikely(is_bad_inode(inode)))
>  		error = -EIO;
> @@ -1205,7 +1205,7 @@ int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
>  	if (error)
>  		goto out_inode_unlock;
>  
> -	if (inode->i_opflags & IOP_XATTR)
> +	if (!(inode->i_opflags & IOP_NOACL))
>  		error = set_posix_acl(mnt_userns, dentry, acl_type, NULL);

And here the lack of get/set methods should be all we need unless
I'm missing something?

> diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
> index c7d1fa526dea..2a7037b165f0 100644
> --- a/fs/reiserfs/inode.c
> +++ b/fs/reiserfs/inode.c
> @@ -2089,7 +2089,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th,
>  	 */
>  	if (IS_PRIVATE(dir) || dentry == REISERFS_SB(sb)->priv_root) {
>  		inode->i_flags |= S_PRIVATE;
> -		inode->i_opflags &= ~IOP_XATTR;
> +		inode_xattr_disable(inode);

I'll need to hear from the reiserfs maintainers, but this also seems
like something that would be better done based on method presence.

> diff --git a/fs/xattr.c b/fs/xattr.c
> index adab9a70b536..89b6c122056d 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -468,7 +468,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
>  	error = security_inode_listxattr(dentry);
>  	if (error)
>  		return error;
> -	if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) {
> +	if (inode->i_op->listxattr && !inode_xattr_disabled(inode)) {
>  		error = inode->i_op->listxattr(dentry, list, size);

So once listing ACLs is moved out of ->listxattr there should be no
need to check anything for ACLs here either.