On Mon, Oct 16, 2023 at 12:27 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Sun, 15 Oct 2023 at 08:58, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> > + for (nr = 0; nr < nr_added_lower; nr++, lowerdirs++) {
> > + if (nr < nr_merged_lower)
> > + seq_show_option(m, "lowerdir+", *lowerdirs);
> > + else
> > + seq_show_option(m, "datadir+", *lowerdirs);
>
> Good.
>
> I did some testing and it turns out libmount still regresses on
> 6.6-rc6 for the escaped comma case. The reason is that libmount
> doesn't understand escaping of commas, hence the '-oupper=upper\,1'
> will result in two fsconfig() calls: 'upper=upper\' and '1'. Prior
> to 6.5 these were nicely reconstructed into the original
> 'upper=upper\,1' by legacy_parse_param().
>
Technically, I think this is a libmount regression, not a kernel regression.
Since libmount 2.39, libmount will split the commas differently than
overlayfs always did.
If a user using libmount 2.39 upgrades to kernel 6.5 and reports
a regression, we can tell that user to opt-out of new mount api via
LIBMOUNT_FORCE_MOUNT2=never.
> The same reconstructing could be done by ovl_parse_param() when
> detecting an option ending with a backslash. But I guess we only need
> to do this if there's a report of such a regression.
>
I really hope we will not need to do that.
> But this raises the question: shouldn't we turn off comma unescaping,
> since it's useless?
>
Not sure I understand what this means.
> Going further, unescaping in general for upperdir and workdir could be
> turned off (lowerdir+ and datadir+ are naturally escape free), leaving
> only unescaping in lowerdir?
>
> Yes, that also has the potential to regress something out there. But
> it also has the potential to clean up the interface further if no such
> regression happens. And I don't think we need to hurry, the 6.7
> cycle would be good for experimenting.
>
I am ok with doing this experiment in 6.7.
I saw that your patch converted upperdir/workdir to path type
without unescaping. Don't we need to keep the support for
string type params for ->parse_monolothic()?
Or is this the reason for the change in fs_lookup_param()?
Anyway, please make sure to split the patch that changes
upperdir/workdir params to be without unescaping, so if we
get regression reports in the future, it will be easy to revert
this change independently from lowerdir+,datadir+.
BTW, can lowerdir+,datadir+ be provided to mount(8)
via command line? or does that require a libmount change?
If they work with mount(8), I can write up an fstests.
FYI, I rebased overlayfs-next over 6.6-rc6.
Thanks,
Amir.
