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

[Eric Sandeen <sandeen@redhat.com>]

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.
>>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> [sandeen: forward-port old patch]
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> cc: Richard Weinberger <richard@nod.at>
>> cc: Zhihao Cheng <chengzhihao1@huawei.com>
>> cc: linux-mtd@lists.infradead.org
>> ---
>>   fs/ubifs/super.c | 459 +++++++++++++++++++++--------------------------
>>   1 file changed, 204 insertions(+), 255 deletions(-)
>>
>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>> index 291583005dd1..cf2e9104baff 100644
>> --- a/fs/ubifs/super.c
>> +++ b/fs/ubifs/super.c
> [...]
>> @@ -963,7 +963,7 @@ static int check_volume_empty(struct ubifs_info *c)
>>    * Opt_no_bulk_read: disable bulk-reads
>>    * Opt_chk_data_crc: check CRCs when reading data nodes
>>    * Opt_no_chk_data_crc: do not check CRCs when reading data nodes
>> - * Opt_override_compr: override default compressor
>> + * Opt_compr: override default compressor
>>    * Opt_assert: set ubifs_assert() action
>>    * Opt_auth_key: The key name used for authentication
>>    * Opt_auth_hash_name: The hash type used for authentication
>> @@ -976,54 +976,46 @@ enum {
>>       Opt_no_bulk_read,
>>       Opt_chk_data_crc,
>>       Opt_no_chk_data_crc,
>> -    Opt_override_compr,
>> +    Opt_compr,
> 
> IMO, I prefer the old name 'Opt_override_compr', it looks more closer to the semantics. UBIFS already set the default compressor in 'c->default_compr', the 'Opt_override_compr' is used to overwrite it.

Fair enough - TBH I did not notice that and not sure why David changed it, easy
enough to change it back!
 
> [...]
> 
>> -static int ubifs_remount_fs(struct super_block *sb, int *flags, char *data)
>> +static int ubifs_reconfigure(struct fs_context *fc)
>>   {
>> +    struct super_block *sb = fc->root->d_sb;
>>       int err;
>>       struct ubifs_info *c = sb->s_fs_info;
>> +    struct ubifs_info *reconf = fc->s_fs_info;
>>         sync_filesystem(sb);
>> -    dbg_gen("old flags %#lx, new flags %#x", sb->s_flags, *flags);
>> +    dbg_gen("old flags %#lx, new flags %#x", sb->s_flags, fc->sb_flags);
>>   -    err = ubifs_parse_options(c, data, 1);
>> -    if (err) {
>> -        ubifs_err(c, "invalid or unknown remount parameter");
>> -        return err;
>> -    }
>> +    /* Apply the mount option changes.
>> +     *
>> +     * [!] NOTE Replacing the auth_key_name and auth_hash_name is
>> +     *     very dodgy without locking.  Previously it just leaked
>> +     *     the old strings.  The strings only seem to be used
>> +     *     during mounting, so don't reconfigure those.
>> +     */
>> +    c->mount_opts        = reconf->mount_opts;
>> +    c->bulk_read        = reconf->bulk_read;
>> +    c->no_chk_data_crc    = reconf->no_chk_data_crc;
>> +    c->default_compr    = reconf->default_compr;
> 
> 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.

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;
 
-	spin_lock_init(&c->cnt_lock);
-	spin_lock_init(&c->cs_lock);
-	spin_lock_init(&c->buds_lock);
-	spin_lock_init(&c->space_lock);
-	spin_lock_init(&c->orphan_lock);
-	init_rwsem(&c->commit_sem);
-	mutex_init(&c->lp_mutex);
-	mutex_init(&c->tnc_mutex);
-	mutex_init(&c->log_mutex);
-	mutex_init(&c->umount_mutex);
-	mutex_init(&c->bu_mutex);
-	mutex_init(&c->write_reserve_mutex);
-	init_waitqueue_head(&c->cmt_wq);
-	init_waitqueue_head(&c->reserve_space_wq);
-	atomic_set(&c->need_wait_space, 0);
-	c->buds = RB_ROOT;
-	c->old_idx = RB_ROOT;
-	c->size_tree = RB_ROOT;
-	c->orph_tree = RB_ROOT;
-	INIT_LIST_HEAD(&c->infos_list);
-	INIT_LIST_HEAD(&c->idx_gc);
-	INIT_LIST_HEAD(&c->replay_list);
-	INIT_LIST_HEAD(&c->replay_buds);
-	INIT_LIST_HEAD(&c->uncat_list);
-	INIT_LIST_HEAD(&c->empty_list);
-	INIT_LIST_HEAD(&c->freeable_list);
-	INIT_LIST_HEAD(&c->frdi_idx_list);
-	INIT_LIST_HEAD(&c->unclean_leb_list);
-	INIT_LIST_HEAD(&c->old_buds);
-	INIT_LIST_HEAD(&c->orph_list);
-	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;
+	if (fc->purpose != FS_CONTEXT_FOR_RECONFIGURE) {
+		spin_lock_init(&c->cnt_lock);
+		spin_lock_init(&c->cs_lock);
+		spin_lock_init(&c->buds_lock);
+		spin_lock_init(&c->space_lock);
+		spin_lock_init(&c->orphan_lock);
+		init_rwsem(&c->commit_sem);
+		mutex_init(&c->lp_mutex);
+		mutex_init(&c->tnc_mutex);
+		mutex_init(&c->log_mutex);
+		mutex_init(&c->umount_mutex);
+		mutex_init(&c->bu_mutex);
+		mutex_init(&c->write_reserve_mutex);
+		init_waitqueue_head(&c->cmt_wq);
+		init_waitqueue_head(&c->reserve_space_wq);
+		atomic_set(&c->need_wait_space, 0);
+		c->buds = RB_ROOT;
+		c->old_idx = RB_ROOT;
+		c->size_tree = RB_ROOT;
+		c->orph_tree = RB_ROOT;
+		INIT_LIST_HEAD(&c->infos_list);
+		INIT_LIST_HEAD(&c->idx_gc);
+		INIT_LIST_HEAD(&c->replay_list);
+		INIT_LIST_HEAD(&c->replay_buds);
+		INIT_LIST_HEAD(&c->uncat_list);
+		INIT_LIST_HEAD(&c->empty_list);
+		INIT_LIST_HEAD(&c->freeable_list);
+		INIT_LIST_HEAD(&c->frdi_idx_list);
+		INIT_LIST_HEAD(&c->unclean_leb_list);
+		INIT_LIST_HEAD(&c->old_buds);
+		INIT_LIST_HEAD(&c->orph_list);
+		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;
 }
 




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