| Message ID | 20210217233046.81418-1-vgoyal@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | virtiofsd: Add options to enable/disable posix acl | expand |
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? Cheers,
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. Thanks Vivek
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. Thanks, Miklos
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,
On Fri, Feb 19, 2021 at 04:55:06PM +0100, Miklos Szeredi wrote: > 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. Hi Miklos, Ok, this sounds reasonable. I am running some quick tests and if I don't notice any serious performance regression, I will respin my patch. > Having an option to disable acls still makes sense, > but it's an optional plus. Agreed. If there are no serious negative performance issues with enabling ACL, then an option to disable is an optional plus. May be I will drop this for now and add this when somebody needs an option to disable ACL. Thanks Vivek
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? Ah! An obvious thing that's missing: change tools/virtiofsd/helper.c to include the new option (and also the manpage) ;-) Cheers, -- Luís