Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v2] jbd2: skip reading super block if it has been verified
To:     Theodore Ts'o <tytso@mit.edu>
Cc:     Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org,
        adilger.kernel@dilger.ca, yi.zhang@huawei.com,
        chengzhihao1@huawei.com, yukuai3@huawei.com
References: <20230616015547.3155195-1-yi.zhang@huaweicloud.com>
 <20230616132745.d3enqs4uni55abrj@quack3>
 <bfd1b9f3-7f0e-4b3c-9399-4d697be37a9e@huaweicloud.com>
 <20230617185057.GA343628@mit.edu>
From:   Zhang Yi <yi.zhang@huaweicloud.com>
Message-ID: <3e75763b-731f-98d2-a203-af4aab84f547@huaweicloud.com>
Date:   Tue, 20 Jun 2023 09:12:46 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <20230617185057.GA343628@mit.edu>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023/6/18 2:50, Theodore Ts'o wrote:
> On Sat, Jun 17, 2023 at 10:42:59AM +0800, Zhang Yi wrote:
>>> This works as a workaround. It is a bit kludgy but for now I guess it is
>>> good enough. Thanks for the fix and feel free to add:
>>
>> Thanks for the review. Yes, I suppose it's better to find a way to adjust
>> the sequence of journal load and feature checking in ocfs2_check_volume(),
>> so that we could completely remove the journal_get_superblock() in
>> jbd2_journal_check_used_features().
> 
> Indeed, thanks for the fix.
> 
> This is would be for after the merge window, but I think we can clean
> this up in the jbd2 layer by simply moving the call to
> load_superblock() from jbd2_journal_load() and jbd2_journal_wipe() to
> journal_init_common().  This change would mean the journal superblock
> gets read as part of the call to jbd2_journal_init_{dev,inode}.
> 
> That way, once the file system has a journal_t object, it's guaranteed
> that the j_sb_buffer contains valid data, and so we can drop the call
> to journal_get_superblock() from jbd2_journal_check_used_features().
> 
> And after we do that, we should be able to inline the code in
> load_superblock() and journal_get_superblock() into
> journal_init_common(), which would simplify things in
> jfs/jbd2/journal.c
> 
> Finally, so we can provide better error handling, we could change
> Jbd2_journal_init_{dev,inode} to return an ERR_PTR instead of a NULL
> if there is a failure.  And since it's a good idea to change the
> function name when changing the function signature, we could rename
> those functions to something like jbd2_open_{dev,inode} at the same
> time.
> 
> 						- Ted
> 
> P.S.  The only reason why we don't load the superblock in
> jbd2_journal_init_{dev,common} was that back in 2001, it was possible
> to create the journal by creating a zero length file in the file
> system, noting the inode number of the file system, unmounting the
> file system from ext2, and then remounting it with "mount -t ext3 -o
> journal=NNN ...".  In order to do this, the ext3 file system code
> called journal_init_inode() with the inode, and then follow it up with
> a call to journal_create(), which would actually write out the journal
> superblock.  For that reason, journal_init_inode() had to avoid
> reading the journal superblock, since it might not be initialized yet.
> 
> We removed jbd2_journal_create() from fs/jbd2 back in 2009, and it
> hadn't been in use for quite a while before that --- in fact, I'm not
> sure ext4 ever supported this ext3-style "let's create a journal
> without e2fsprogs support because Stephen Tweedie was implementing the
> ext3 journal kernel code without wanting to make changes to e2fsprogs
> first" feature.  :-)
> 

Thanks for the suggestion and historical details, we could do the cleanup
in jbd2 layer after the merge window.

Thanks,
Yi.