Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1355932 > unrolled thread

Re: [PATCH v18 11/22] vfs: Cache base_acl objects in inodes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v18 11/22] vfs: Cache base_acl objects in inodes Christoph Hellwig <hch@infradead.org> - 2016-03-11 15:10 +0100
    Re: [PATCH v18 11/22] vfs: Cache base_acl objects in inodes Andreas Gruenbacher <agruenba@redhat.com> - 2016-03-11 17:30 +0100
      Re: [PATCH v18 11/22] vfs: Cache base_acl objects in inodes Christoph Hellwig <hch@infradead.org> - 2016-03-15 08:20 +0100
        Re: [PATCH v18 11/22] vfs: Cache base_acl objects in inodes Andreas Gruenbacher <agruenba@redhat.com> - 2016-03-16 23:40 +0100

#1355932 — Re: [PATCH v18 11/22] vfs: Cache base_acl objects in inodes

On Mon, Feb 29, 2016 at 09:17:16AM +0100, Andreas Gruenbacher wrote:
> POSIX ACLs and richacls are both objects allocated by kmalloc() with a
> reference count which are freed by kfree_rcu().  An inode can either
> cache an access and a default POSIX ACL, or a richacl (richacls do not
> have default acls).  To allow an inode to cache either of the two kinds
> of acls, introduce a new base_acl type and convert i_acl and
> i_default_acl to that type. In most cases, the vfs then doesn't care which
> kind of acl an inode caches (if any).

This base_acl object is pointless.  I've asked in the past to have
a proper container for the ACLs in common code, but a union
of a refcount and a rcu head doesn't really fit that category.

But this points out that the f2fs folks really need a couple of
slaps on their hands.  Not if generic funtionality doesn't
fit your needs you are not going to blindly copy and paste it,
please talk to find a solution instead of duplicating it.

Folks, please come up with a suggestion to get rid of f2fs_acl_clone,
f2fs_acl_create_masq and f2fs_acl_create ASAP.

[toc] | [next] | [standalone]

#1356031

On Fri, Mar 11, 2016 at 3:07 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Feb 29, 2016 at 09:17:16AM +0100, Andreas Gruenbacher wrote:
>> POSIX ACLs and richacls are both objects allocated by kmalloc() with a
>> reference count which are freed by kfree_rcu().  An inode can either
>> cache an access and a default POSIX ACL, or a richacl (richacls do not
>> have default acls).  To allow an inode to cache either of the two kinds
>> of acls, introduce a new base_acl type and convert i_acl and
>> i_default_acl to that type. In most cases, the vfs then doesn't care which
>> kind of acl an inode caches (if any).
>
> This base_acl object is pointless.  I've asked in the past to have
> a proper container for the ACLs in common code, but a union
> of a refcount and a rcu head doesn't really fit that category.

POSIX ACLs and RichACLs are different objects, with different members
and different algorithms operating on them. The only commonality is
that they are both kmalloc()ed, reference counted objects, and when an
inode is destroyed, both kinds of ACLs can be put in the same way,
avoiding an unnecessary if. What kind of common-code container beyond
that are you still dreaming about?

Thanks,
Andreas

[toc] | [prev] | [next] | [standalone]

#1357843

On Fri, Mar 11, 2016 at 05:24:45PM +0100, Andreas Gruenbacher wrote:
> POSIX ACLs and RichACLs are different objects, with different members
> and different algorithms operating on them. The only commonality is
> that they are both kmalloc()ed, reference counted objects, and when an
> inode is destroyed, both kinds of ACLs can be put in the same way,
> avoiding an unnecessary if. What kind of common-code container beyond
> that are you still dreaming about?

We still have a main object that is simply a list of ACEs.  But if that
doesn't work out (I suspect it should) I don't think the common base
object is a good idea.  It just leads to a lot of crazy container_of
calls.  If the common object abstraction doesn't work out we'll need
a procedural one instead that has common acl_* calls that decide what
do to based on the file system acl flag.

[toc] | [prev] | [next] | [standalone]

#1359425

On Tue, Mar 15, 2016 at 8:12 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Mar 11, 2016 at 05:24:45PM +0100, Andreas Gruenbacher wrote:
>> POSIX ACLs and RichACLs are different objects, with different members
>> and different algorithms operating on them. The only commonality is
>> that they are both kmalloc()ed, reference counted objects, and when an
>> inode is destroyed, both kinds of ACLs can be put in the same way,
>> avoiding an unnecessary if. What kind of common-code container beyond
>> that are you still dreaming about?
>
> We still have a main object that is simply a list of ACEs.  But if that
> doesn't work out (I suspect it should) I don't think the common base
> object is a good idea.  It just leads to a lot of crazy container_of
> calls.

There are two such container_of calls for POSIX ACLs in fs/jffs2/acl.c
[which could be replaced by get_acl()], two in fs/posix_acl.c for
POSIX ACLs, and two in fs/richacl.c for RichACLs. That's it.

> If the common object abstraction doesn't work out we'll need
> a procedural one instead that has common acl_* calls that decide what
> do to based on the file system acl flag.

I've already made such abstractions where it made sense; if you can
find more, I don't see why we shouldn't add them.

Thanks,
Andreas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web