Re: [PATCH] ubifs: Convert ubifs to use the new mount API

Zhihao Cheng
2024-09-29T01:57:21Z

在 2024/9/27 23:56, Eric Sandeen 写道:
> On 9/27/24 9:12 AM, Zhihao Cheng wrote:
>> 在 2024/9/27 4:36, Eric Sandeen 写道:
>> Hi Eric, two comments below.
>>> From: David Howells <dhowells@redhat.com>
>>>
>>> Convert the ubifs filesystem to the new internal mount API as the old
>>> one will be obsoleted and removed.  This allows greater flexibility in
>>> communication of mount parameters between userspace, the VFS and the
>>> filesystem.
>>>
>>> See Documentation/filesystems/mount_api.txt for more information.
>>>

[...]
>>
>> We cannot overwrite old configurations with non-fully initialized new configurations directly, otherwise some old options will disappear, for example:
>> [root@localhost ~]# mount -ocompr=lzo /dev/ubi0_0 temp
>> [root@localhost ~]# mount | grep ubifs
>> /dev/ubi0_0 on /root/temp type ubifs (rw,relatime,compr=lzo,assert=read-only,ubi=0,vol=0)
>> The compressor is set as lzo.
>> [root@localhost ~]# mount -oremount /dev/ubi0_0 temp
>> [root@localhost ~]# mount | grep ubifs
>> /dev/ubi0_0 on /root/temp type ubifs (rw,relatime,assert=read-only,ubi=0,vol=0)
>> The compressor is not lzo anymore.
>
> Ah, yes. reconfigure is surprisingly tricky at times for reasons like this.
>
> Is mount here from busybox by chance? I think usually util-linux mount respecifies every
> existing mount option on remount which tends to hide this sort of thing; you could
> double check this by stracing fsconfig calls during remount.

I think we shouldn't depend on the userspace utils, let's just consider
the semantics of remounting from the view of syscall, because linux user
can call remount by syscall directly.
>
> That said, we can preserve mount options internally as well. Does this patch on top
> of the first one fix it for you?
>
> (the other option would be to make an ->fs_private struct that holds only mount
> options, rather than re-using s_fs_info as it does now - dhowells, any thoughts?)
>
> Thanks for the review and testing!
>
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index cf2e9104baff..be8154359772 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -2256,44 +2256,56 @@ static int ubifs_init_fs_context(struct fs_context *fc)
> if (!c)
> return -ENOMEM;
>
[...]
> + INIT_LIST_HEAD(&c->orph_new);
> + c->no_chk_data_crc = 1;
> + c->assert_action = ASSACT_RO;
> + c->highest_inum = UBIFS_FIRST_INO;
> + c->lhead_lnum = c->ltail_lnum = UBIFS_LOG_LNUM;
> + } else {
> + struct ubifs_info *c_orig = fc->root->d_sb->s_fs_info;
> +
> + /* Preserve existing mount options across remounts */
> + c->mount_opts = c_orig->mount_opts;
> + c->bulk_read = c_orig->bulk_read;
> + c->no_chk_data_crc = c_orig->no_chk_data_crc;
> + c->default_compr = c_orig->default_compr;
> + c->assert_action = c_orig->assert_action;
> + }
>
> fc->s_fs_info = c;
> fc->ops = &ubifs_context_ops;
> +
> return 0;
> }
>
>

Above modification can fix the problem, but it looks not so clean.
There are two main steps for mount/remount in the new API framework:
1. Get filesystem configurations by parsing the paramaters from the user
data.
2. Apply the filesystem configurations to superblock(For mount, a
superblock should be allocated first.).

So, how about doing that like ext4 does:
1) the filesystem specified superblock is allocated in fill_super(), not
in init_fs_context(), it looks more reasonable, because filesystem
doesn't need a new private superblock in remounting process. But, UBIFS
has onething different, sb_test() needs volume information which comes
from private superblock, so UBFIS private superblock(struct ubifs_info)
is allocated in ubifs_mount(). Here, I prefer to keep private superblock
allocation still in ubifs_mount(for new mount API, it is called
ubifs_get_tree).
2) the filesystem configurations(contains mount options) are stored in a
separated structure(in ext4, it is called ext4_fs_context), which is
allocated in init_fs_context(). For UBIFS, we can define a similar
structure to store the filesystem configurations.