Re: [PATCH v2 0/3] virtiofsd: Add options to enable/disable posix acl

[Luis Henriques <lhenriques@suse.de>]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Fri, Feb 19, 2021 at 3:34 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>>
>> On Fri, Feb 19, 2021 at 11:50:54AM +0000, Luis Henriques wrote:
>> > Vivek Goyal <vgoyal@redhat.com> writes:
>> >
>> > > Hi,
>> > >
>> > > This is V2 of the patches. Changes since v1 are.
>> > >
>> > > - Rebased on top of latest master.
>> > > - Took care of Miklos's comments to block acl xattrs if user
>> > >   explicitly disabled posix acl.
>> > >
>> > > Luis Henriques reported that fstest generic/099 fails with virtiofs.
>> > > Little debugging showed that we don't enable acl support. So this
>> > > patch series provides option to enable/disable posix acl support. By
>> > > default it is disabled.
>> > >
>> > > I have run blogbench and pjdfstests with posix acl enabled and
>> > > things work fine.
>> > >
>> > > Luis, can you please apply these patches, and run virtiofsd with
>> > > "-o posix_acl" and see if it fixes the failure you are seeing. I
>> > > ran the steps you provided manually and it fixes the issue for
>> > > me.
>> >
>> > Sorry for the delay.  I've finally tested these patches and they indeed
>> > fix the problem I reported.  My only question about this fix is why is
>> > this option not enabled by default, since this is the documented behavior
>> > in acl(5) and umask(2)?  In fact, why is this an option at all?
>>
>> You mean why to not enable acl by default?
>>
>> I am concerned about performance drop this can lead to because extra
>> GETXATTR(system.posix_acl_*) messages which will trigger if acls are enabled.
>> And not all users might require these. That's why I preferred to not enable
>> acl by default. Those who need it can enable it explicitly.
>>
>> Another example is xattr support. Due to performance concerns, we don't
>> enable xattrs by default either.
>
> Actually generic xattr is much worse, since there's no caching for
> them currently, as opposed to posix acls, which are cached both when
> positive and negative.
>
> If we enable ACL by default in case xattrs are enabled, we should be
> safe, I think.  Having an option to disable acls still makes sense,
> but it's an optional plus.

Great, thanks for clarifying that the reason for having these options is
really for performance.

Anyway, thanks a lot for looking at this and fixing it.

Cheers,
-- 
Luis