Re: [PATCH] ovl: Allow mount options to be parsed on remount

["André Almeida" <andrealmeid@igalia.com>]

Em 22/05/2025 06:52, Amir Goldstein escreveu:
> On Thu, May 22, 2025 at 8:20 AM André Almeida <andrealmeid@igalia.com> wrote:
>>
>> Hi Christian, Amir,
>>
>> Thanks for the feedback :)
>>
>> Em 21/05/2025 08:20, Christian Brauner escreveu:
>>> On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
>>>> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
>>>>>
>>
>> [...]
>>
>>>>
>>>> I see the test generic/623 failure - this test needs to be fixed for overlay
>>>> or not run on overlayfs.
>>>>
>>>> I do not see those other 5 failures although before running the test I did:
>>>> export LIBMOUNT_FORCE_MOUNT2=always
>>>>
>>>> Not sure what I am doing differently.
>>>>
>>
>> I have created a smaller reproducer for this, have a look:
>>
>>    mkdir -p ovl/lower ovl/upper ovl/merge ovl/work ovl/mnt
>>    sudo mount -t overlay overlay -o lowerdir=ovl/lower,upperdir=ovl/
>> upper,workdir=ovl/work ovl/mnt
>>    sudo mount ovl/mnt -o remount,ro
>>
> 
> Why would you use this command?
> Why would you want to re-specify the lower/upperdir when remounting ro?
> And more specifically, fstests does not use this command in the tests
> that you mention that they fail, so what am I missing?
> 

I've added "set -x" to tests/generic/294 to see exactly which mount 
parameters were being used and I got this from the output:

+ _try_scratch_mount -o remount,ro
+ local mount_ret
+ '[' overlay == overlay ']'
+ _overlay_scratch_mount -o remount,ro
+ echo '-o remount,ro'
+ grep -q remount
+ /usr/bin/mount /tmp/dir2/ovl-mnt -o remount,ro
mount: /tmp/dir2/ovl-mnt: fsconfig() failed: ...

So, from what I can see, fstests is using this command. Not sure if I 
did something wrong when setting up fstests.

>> And this returns:
>>
>>    mount: /tmp/ovl/mnt: fsconfig() failed: overlay: No changes allowed in
>>    reconfigure.
>>          dmesg(1) may have more information after failed mount system call.
>>
>> However, when I use mount like this:
>>
>>    sudo mount -t overlay overlay -o remount,ro ovl/mnt
>>
>> mount succeeds. Having a look at strace, I found out that the first
>> mount command tries to set lowerdir to "ovl/lower" again, which will to
>> return -EINVAL from ovl_parse_param():
>>
>>      fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
>>      fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) =
>> -1 EINVAL (Invalid argument)
>>
>> Now, the second mount command sets just the "ro" flag, which will return
>> after vfs_parse_sb_flag(), before getting to ovl_parse_param():
>>
>>      fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
>>      fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
>>
>> After applying my patch and running the first mount command again, we
>> can set that this flag is set only after setting all the strings:
>>
>>      fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) = 0
>>      fsconfig(4, FSCONFIG_SET_STRING, "upperdir", "/tmp/ovl/upper", 0) = 0
>>      fsconfig(4, FSCONFIG_SET_STRING, "workdir", "/tmp/ovl/work", 0) = 0
>>      fsconfig(4, FSCONFIG_SET_STRING, "uuid", "on", 0) = 0
>>      fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
>>
>> I understood that the patch that I proposed is wrong, and now I wonder
>> if the kernel needs to be fixed at all, or if the bug is how mount is
>> using fsconfig() in the first mount command?
> 
> Maybe I am not reading your report correctly, but as this commands works:
> 
> mount -t overlay overlay -o remount,ro ovl/mnt
> 
> and the fstests that call _scratch_remount() work
> I don't think there is anything to fix and I do not understand
> what is the complaint.
> 
> Thanks,
> Amir.