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

[Zhihao Cheng <chengzhihao1@huawei.com>]

在 2024/9/29 9:57, Zhihao Cheng 写道:
> 在 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).

Notice, if you do that, please be careful to check the resource 
releasing problems in kinds of error handling branches. I have checked 
that in v1, it looks sophisticated.
> 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.
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/