On Thu, Oct 12, 2023 at 12:49 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Thu, Oct 12, 2023 at 12:27:26PM +0300, Amir Goldstein wrote:
> > On Thu, Oct 12, 2023 at 11:26 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > >
> > > > > Christian,
> > > > >
> > > > > Do you know any userspace that already uses your new append prefixes?
> > > > > Do we have any good reason to support "lowerdir_first"
> > > > > so a lower dir stack could be reset before creating the sb?
> > > >
> > > > If that is a requirement, I suggest extending fsconfig(2) to allow
> > > > resetting an option.
> > >
> > > Overlayfs does already support this. If you pass:
> > > fsconfig(FSCONFIG_SET_STRING, "lowerdir", "", ...)
> > > then the lower layer stack is reset. I've implemented it that way in
> > > ovl_parse_param_lowerdir().
> > >
> >
> > Yes, I noticed that. Cool.
> >
> > > >
> > > > > > > > >
> > > > > > > > > Anyway, let's focus on what you would like best.
> > > > > > > > > If you prefer to just fix the regression, it is doable.
> > > > > > > > > If you prefer the upperdirfd, workdirfd, lowerdirfd API, I think we can
> > > > > > > > > find a volunteer to write it up.
> > > >
> > > > Can't the existing option names be overloaded if a separate cmd
> > > > (FSCONFIG_SET_PATH or FSCONFIG_SET_PATH_EMPTY) is used in fsconfig()?
> > >
> > > Yes, they can and filesystems do do that today depending on whether they
> > > want to e.g., take an fd or a path or something.
> >
> > Nice. It seems like Miklos has volunteered to implement the
> > dirfd and/or unescaped API variants for the new mount API :)
> >
> > What is your opinion about the original regression report
> > regarding escaping of commas in ->parse_monolithic()?
> >
> > It's easy to implement ovl_parse_monolithic() that will
> > conform to the old ovl_next_opt() behavior, but it does not
> > solve the problem long term.
> >
> > If there are currently setups in the wild that pass arguments
> > like [lowerdir=/tmp/a\,b/], even if I do fix up ovl_parse_monolithic()
> > those setups will regress when they upgrade to libmount v2.39,
> > because AFAICT, libmount does not respect "\," to escape option split,
> > it respects [lowerdir="/tmp/a,b/"] to escape option split.
>
> For full backward compatibility we would probably need to fix both the
> kernel and libmount. Because libmount/mount(8) is encouraged to split a
> lowerdir=/a:/b:/c:/d option into separate fsconfig calls, especially
> when dealing with really long paths. So libmount would need to be aware
> of overlayfs parsing behavior that includes escaping \, even if we fix
> the kernel itself.
>
> I don't think that would be a big deal because libmount already has to
> deal with all kinds of filesystems specific quirks.
>
> However, libmount also added LIBMOUNT_FORCE_MOUNT2={always,never,auto}
> which can be used to disable using the new mount api and makes it use
> the old mount api which is available in libmount 2.39.
>
> So I think complementing overlayfs with a ->parse_monolithic() option
> might be something that we could consider doing but this is a judgement
> call there's not clear right and wrong with so many moving parts...
>
OK. it was quite simple so I posted the regression fix.
> >
> > If we do decide that we need to or want to fix ->parse_monolithic()
> > then do you think it would make sense to respect "\," escaping in
> > generic_parse_monolithic()?
> > I cannot imagine any workload that would get regressed by this
> > (famous last words).
>
> I'm pretty sure that this would break something so I would be hesitant
> doing this.
OK. Instead of changing generic_parse_monolithic() I re-factored it:
https://lore.kernel.org/linux-unionfs/20231012134428.1874373-1-amir73il@xxxxxxxxx/
This may end up being useful for other fs.
If you agree with the new helper, please ACK and I will send it for 6.6-rc6
along with the two fixes I have on ovl-fixes.
Also updated the xfstest to test escaped commas:
https://github.com/amir73il/xfstests/commits/overlayfs-devel
Thanks,
Amir.
