[PATCH] fuse: cache POSIX ACLs when setting them

[PATCH] fuse: cache POSIX ACLs when setting them

[Luis Henriques]

When setting an ACL in an inode we can immediately add it to the cache.
This is a small optimisation, as currently an ACL is only added to the
cache when reading it again, which involves an extra GETXATTR hop into
user-space.

Signed-off-by: Luis Henriques <luis@igalia.com>
---
 fs/fuse/acl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
index 31fb50e16aed..c2584bb75ec7 100644
--- a/fs/fuse/acl.c
+++ b/fs/fuse/acl.c
@@ -155,6 +155,8 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
 		 */
 		forget_all_cached_acls(inode);
 		fuse_invalidate_attr(inode);
+		if (!ret)
+			set_cached_acl(inode, type, acl);
 	}
 
 	return ret;

Re: [PATCH] fuse: cache POSIX ACLs when setting them

[Horst Birthelmer]

On Wed, Jul 01, 2026 at 12:00:52PM +0100, Luis Henriques wrote:
> When setting an ACL in an inode we can immediately add it to the cache.
> This is a small optimisation, as currently an ACL is only added to the
> cache when reading it again, which involves an extra GETXATTR hop into
> user-space.
> 
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
>  fs/fuse/acl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> index 31fb50e16aed..c2584bb75ec7 100644
> --- a/fs/fuse/acl.c
> +++ b/fs/fuse/acl.c
> @@ -155,6 +155,8 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>  		 */
>  		forget_all_cached_acls(inode);
>  		fuse_invalidate_attr(inode);
> +		if (!ret)
> +			set_cached_acl(inode, type, acl);
>  	}
>  
>  	return ret;
> 

LGTM

Feel free to add
Reviewed-By: Horst Birthelmer <hbirthelmer@ddn.com>

Re: [PATCH] fuse: cache POSIX ACLs when setting them

[Amir Goldstein]

On Wed, Jul 1, 2026 at 1:05 PM Luis Henriques <luis@igalia.com> wrote:
>
> When setting an ACL in an inode we can immediately add it to the cache.
> This is a small optimisation, as currently an ACL is only added to the
> cache when reading it again, which involves an extra GETXATTR hop into
> user-space.
>
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
>  fs/fuse/acl.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> index 31fb50e16aed..c2584bb75ec7 100644
> --- a/fs/fuse/acl.c
> +++ b/fs/fuse/acl.c
> @@ -155,6 +155,8 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>                  */
>                 forget_all_cached_acls(inode);
>                 fuse_invalidate_attr(inode);
> +               if (!ret)
> +                       set_cached_acl(inode, type, acl);

This is undoing what forget_all_cached_acls() explicitly tries to do.
I think the reason is that kernel code cannot assume the server did
not modify ACL before storing them, not sure, but NFS does the same thing
with nfs_zap_acl_cache().

Thanks,
Amir.

Re: Re: [PATCH] fuse: cache POSIX ACLs when setting them

[Horst Birthelmer]

On Wed, Jul 01, 2026 at 01:41:22PM +0200, Amir Goldstein wrote:
> On Wed, Jul 1, 2026 at 1:05 PM Luis Henriques <luis@igalia.com> wrote:
> >
> > When setting an ACL in an inode we can immediately add it to the cache.
> > This is a small optimisation, as currently an ACL is only added to the
> > cache when reading it again, which involves an extra GETXATTR hop into
> > user-space.
> >
> > Signed-off-by: Luis Henriques <luis@igalia.com>
> > ---
> >  fs/fuse/acl.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> > index 31fb50e16aed..c2584bb75ec7 100644
> > --- a/fs/fuse/acl.c
> > +++ b/fs/fuse/acl.c
> > @@ -155,6 +155,8 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
> >                  */
> >                 forget_all_cached_acls(inode);
> >                 fuse_invalidate_attr(inode);
> > +               if (!ret)
> > +                       set_cached_acl(inode, type, acl);
> 
> This is undoing what forget_all_cached_acls() explicitly tries to do.
> I think the reason is that kernel code cannot assume the server did
> not modify ACL before storing them, not sure, but NFS does the same thing
> with nfs_zap_acl_cache().
> 
> Thanks,
> Amir.
> 

I just thought it would be a good idea to save us a trip to user space.

Horst

Re: [PATCH] fuse: cache POSIX ACLs when setting them

[Luis Henriques]

On Wed, Jul 01 2026, Amir Goldstein wrote:

> On Wed, Jul 1, 2026 at 1:05 PM Luis Henriques <luis@igalia.com> wrote:
>>
>> When setting an ACL in an inode we can immediately add it to the cache.
>> This is a small optimisation, as currently an ACL is only added to the
>> cache when reading it again, which involves an extra GETXATTR hop into
>> user-space.
>>
>> Signed-off-by: Luis Henriques <luis@igalia.com>
>> ---
>>  fs/fuse/acl.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
>> index 31fb50e16aed..c2584bb75ec7 100644
>> --- a/fs/fuse/acl.c
>> +++ b/fs/fuse/acl.c
>> @@ -155,6 +155,8 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>>                  */
>>                 forget_all_cached_acls(inode);
>>                 fuse_invalidate_attr(inode);
>> +               if (!ret)
>> +                       set_cached_acl(inode, type, acl);
>
> This is undoing what forget_all_cached_acls() explicitly tries to do.
> I think the reason is that kernel code cannot assume the server did
> not modify ACL before storing them, not sure, but NFS does the same thing
> with nfs_zap_acl_cache().

In my understanding, forget_all_cached_acls() is cleaning any previously
cached ACLs (even if the SETXATTR failed!).  So it made sense to me to
cache the new ACL just like other filesystems seem to be doing.

On the other hand, having a server modifying the ACL without notifying the
kernel didn't sound right to me, specially because it has explicitly set
the FUSE_POSIX_ACL flag -- otherwise this code wouldn't be executed.  And
that's why I assumed it would be acceptable to have this optimisation:
because user-space asked the VFS to do permission checking *and* caching
ACLs.

I did actually took a look into the NFS code, but it seems to be doing it's
own ACL caching. But yeah, I can't claim to have fully understood what
(and why) NFS is really doing :-)

Cheers,
-- 
Luís

Re: [PATCH] fuse: cache POSIX ACLs when setting them

[Amir Goldstein]

On Wed, Jul 1, 2026 at 2:24 PM Luis Henriques <luis@igalia.com> wrote:
>
> On Wed, Jul 01 2026, Amir Goldstein wrote:
>
> > On Wed, Jul 1, 2026 at 1:05 PM Luis Henriques <luis@igalia.com> wrote:
> >>
> >> When setting an ACL in an inode we can immediately add it to the cache.
> >> This is a small optimisation, as currently an ACL is only added to the
> >> cache when reading it again, which involves an extra GETXATTR hop into
> >> user-space.
> >>
> >> Signed-off-by: Luis Henriques <luis@igalia.com>
> >> ---
> >>  fs/fuse/acl.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
> >> index 31fb50e16aed..c2584bb75ec7 100644
> >> --- a/fs/fuse/acl.c
> >> +++ b/fs/fuse/acl.c
> >> @@ -155,6 +155,8 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
> >>                  */
> >>                 forget_all_cached_acls(inode);
> >>                 fuse_invalidate_attr(inode);
> >> +               if (!ret)
> >> +                       set_cached_acl(inode, type, acl);
> >
> > This is undoing what forget_all_cached_acls() explicitly tries to do.
> > I think the reason is that kernel code cannot assume the server did
> > not modify ACL before storing them, not sure, but NFS does the same thing
> > with nfs_zap_acl_cache().
>
> In my understanding, forget_all_cached_acls() is cleaning any previously
> cached ACLs (even if the SETXATTR failed!).  So it made sense to me to
> cache the new ACL just like other filesystems seem to be doing.

Local filesystems do, remote filesystems not always.

>
> On the other hand, having a server modifying the ACL without notifying the
> kernel didn't sound right to me, specially because it has explicitly set
> the FUSE_POSIX_ACL flag -- otherwise this code wouldn't be executed.  And
> that's why I assumed it would be acceptable to have this optimisation:
> because user-space asked the VFS to do permission checking *and* caching
> ACLs.

Your arguments make sense to me.
Only it appears so simple so it feels like I am missing something.
Why wasn't this implemented like that in the first place?

Thanks,
Amir.

Re: [PATCH] fuse: cache POSIX ACLs when setting them

[Luis Henriques]

On Thu, Jul 02 2026, Amir Goldstein wrote:

> On Wed, Jul 1, 2026 at 2:24 PM Luis Henriques <luis@igalia.com> wrote:
>>
>> On Wed, Jul 01 2026, Amir Goldstein wrote:
>>
>> > On Wed, Jul 1, 2026 at 1:05 PM Luis Henriques <luis@igalia.com> wrote:
>> >>
>> >> When setting an ACL in an inode we can immediately add it to the cache.
>> >> This is a small optimisation, as currently an ACL is only added to the
>> >> cache when reading it again, which involves an extra GETXATTR hop into
>> >> user-space.
>> >>
>> >> Signed-off-by: Luis Henriques <luis@igalia.com>
>> >> ---
>> >>  fs/fuse/acl.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
>> >> index 31fb50e16aed..c2584bb75ec7 100644
>> >> --- a/fs/fuse/acl.c
>> >> +++ b/fs/fuse/acl.c
>> >> @@ -155,6 +155,8 @@ int fuse_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
>> >>                  */
>> >>                 forget_all_cached_acls(inode);
>> >>                 fuse_invalidate_attr(inode);
>> >> +               if (!ret)
>> >> +                       set_cached_acl(inode, type, acl);
>> >
>> > This is undoing what forget_all_cached_acls() explicitly tries to do.
>> > I think the reason is that kernel code cannot assume the server did
>> > not modify ACL before storing them, not sure, but NFS does the same thing
>> > with nfs_zap_acl_cache().
>>
>> In my understanding, forget_all_cached_acls() is cleaning any previously
>> cached ACLs (even if the SETXATTR failed!).  So it made sense to me to
>> cache the new ACL just like other filesystems seem to be doing.
>
> Local filesystems do, remote filesystems not always.

True.  Even ceph, which is the remote filesystem I'm most familiar with,
caches ACLs when setting them only if the client has the right
capabilities for doing so.

>> On the other hand, having a server modifying the ACL without notifying the
>> kernel didn't sound right to me, specially because it has explicitly set
>> the FUSE_POSIX_ACL flag -- otherwise this code wouldn't be executed.  And
>> that's why I assumed it would be acceptable to have this optimisation:
>> because user-space asked the VFS to do permission checking *and* caching
>> ACLs.
>
> Your arguments make sense to me.
> Only it appears so simple so it feels like I am missing something.
> Why wasn't this implemented like that in the first place?

Yeah, maybe your right, maybe I'm missing something as well and there is a
good reason for not doing it.

I was going to CC Christian, but I see you did that already :-)

Maybe he remembers the reasons behind this while working on commit
facd61053cff ("fuse: fixes after adapting to new posix acl api").

Cheers,
-- 
Luís