Sorry if my previous mail sounded hostile. I was just really confused
why a regression wasn't taken seriously.
Thank you very much for fixing it!
On Thu, Oct 12, 2023 at 3:55 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> 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.
>
