Re: [PATCH] debugfs: fix mount options not being applied

[Charalampos Mitrodimas]

Eric Sandeen <sandeen@xxxxxxxxxxx> writes:

> On 8/8/25 9:13 AM, Christian Brauner wrote:
>> On Wed, Aug 06, 2025 at 11:33:11AM -0500, Eric Sandeen wrote:
>>> On 8/5/25 12:22 PM, Charalampos Mitrodimas wrote:
>
> ...
>
>>>> Hi, thanks for the review, and yes you're right.
>>>>
>>>> Maybe a potential systemic fix would be to make get_tree_single() always
>>>> call fc->ops->reconfigure() after vfs_get_super() when reusing an
>>>> existing superblock, fixing all affected filesystems at once.
>>>
>>> Yep, I'm looking into that. mount_single used to do this, and IIRC we discussed
>>> it before but for some reason opted not to. It seems a bit trickier than I first
>>> expected, but I might just be dense. ;)
>> 
>> If we can make it work generically, we should. I too don't remember what
>> the reasons were for not doing it that way.
>
> Sorry for the long delay here. Talked to dhowells about this and his
> POV (which is convincing, I think) is that even though mount_single used to
> call do_remount_sb for an extant single sb, this was probably Bad(tm).
> Bad, IIUC, because it's not a given that options are safe to be changed
> in this way, and that policy really should be up to each individual
> filesystem.
>
> So while we still need to audit and fix any get_tree_single()
> filesystems that changed behavior with the new mount api, may as well
> fix up debugfs for now since the bug was reported.

What if we add a new flag (.fs_flags), say FS_SINGLE_RECONF, to
file_system_type that makes get_tree_single() automatically call
reconfigure() when reusing an existing superblock? Filesystems could
then just opt-in by adding it to .fs_flags.

>
> Charalampos - 
>
> Your patch oopses on boot for me - I think that when you added
>
> 	sb->s_fs_info = fc->s_fs_info;

Yes, did take notice of this yesterday when I revisited it.

>
> in debugfs_fill_super, you're actually NULLing out the one in the sb,
> because sget_fc has already transferred fc->s_fs_info to sb->s_fs_info,
> and NULLed fc->s_fs_info prior to this. Then when we get to
> _debugfs_apply_options, *fsi = sb->s_fs_info; is also NULL so using it
> there oopses.
>
> If you want to send a V2 with fixed up stable cc: I'd suggest following the
> pattern of what was done for tracefs in e4d32142d1de, which I think works
> OK and would at least lend some consistency, as the code is similar.
>
> If not, let me know and I'll work on an update.

As a matter of fact, I have a v2 exactly like this ready to sent. Doing
so in a bit.

>
> Thanks,
> -Eric 

Thanks!
C. Mitrodimas