mbox series

[v3,0/2] virtiofsd: Enable posix_acl by default

Message ID 20210223225250.23945-1-vgoyal@redhat.com
Headers show
Series virtiofsd: Enable posix_acl by default | expand

Message

Vivek Goyal Feb. 23, 2021, 10:52 p.m. UTC 
Hi,

This is V3 of the patches. Changes since v2 are.

- I dropped the patch to give user an option to enable/disable acls.
  Now acls are enabled by default if xattrs are enabled and fuse
  client offers FUSE_POSIX_ACL capability.
 
Miklos mentioned that ACLS might not have lot of overhead as these
can be cached. So it might make sense to enable these by default.

If we run into performance issues, then we can add another patch to
give option to enable/disable and disable it by default.

Luis Henriques reported that fstest generic/099 fails with virtiofs.
Little debugging showed that we don't enable acl support. This
patch series should fix the issue

Vivek Goyal (2):
  virtiofsd: Add umask to seccom allow list
  virtiofsd: Enable posix_acls by default if xattrs are enabled

 tools/virtiofsd/passthrough_ll.c      | 29 +++++++++++++++++++++------
 tools/virtiofsd/passthrough_seccomp.c |  1 +
 2 files changed, 24 insertions(+), 6 deletions(-)

Comments

Vivek Goyal Feb. 24, 2021, 2:58 p.m. UTC | #1 
On Tue, Feb 23, 2021 at 05:52:48PM -0500, Vivek Goyal wrote:
> Hi,
> 
> This is V3 of the patches. Changes since v2 are.
> 
> - I dropped the patch to give user an option to enable/disable acls.
>   Now acls are enabled by default if xattrs are enabled and fuse
>   client offers FUSE_POSIX_ACL capability.
>  
> Miklos mentioned that ACLS might not have lot of overhead as these
> can be cached. So it might make sense to enable these by default.

Maybe ACL caching will only work well with cache=auto and cache=always.
With cache=none it probably will show most extra overhead.

For example, with cache=none, I did "su test". And then did "cat foo.txt"
and every time it results in two GETXATTR(system.posix_acl_access) calls.

2021-02-24 09:56:09.45-0500] [ID: 00000004] lo_getxattr(ino=1, name=system.posix_acl_access size=4096)
[2021-02-24 09:56:09.45-0500] [ID: 00000004] lo_getxattr(ino=2, name=system.posix_acl_access size=4096)

So this will definitely impact the performance with cache=none negatively.

I am now inclined to respin the patches and add options to enable/disable
acl and keep acl disabled by default. Those who need it, can enable it.

Vivek

> 
> If we run into performance issues, then we can add another patch to
> give option to enable/disable and disable it by default.
> 
> Luis Henriques reported that fstest generic/099 fails with virtiofs.
> Little debugging showed that we don't enable acl support. This
> patch series should fix the issue
> 
> Vivek Goyal (2):
>   virtiofsd: Add umask to seccom allow list
>   virtiofsd: Enable posix_acls by default if xattrs are enabled
> 
>  tools/virtiofsd/passthrough_ll.c      | 29 +++++++++++++++++++++------
>  tools/virtiofsd/passthrough_seccomp.c |  1 +
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> -- 
> 2.25.4
>