Date:   Wed, 8 Feb 2023 14:12:40 +0100
From:   Jan Kara <jack@suse.cz>
To:     zhanchengbin <zhanchengbin1@huawei.com>
Cc:     Jan Kara <jack@suse.cz>, tytso@mit.edu, jack@suse.com,
        linux-ext4@vger.kernel.org, yi.zhang@huawei.com,
        linfeilong@huawei.com, liuzhiqiang26@huawei.com
Subject: Re: [PATCH v3 2/2] ext4: restore len when ext4_ext_insert_extent
 failed
Message-ID: <20230208131240.gwfulolnvcz5omdi@quack3>
References: <20230207070931.2189663-1-zhanchengbin1@huawei.com>
 <20230207070931.2189663-3-zhanchengbin1@huawei.com>
 <20230207142356.frf4zzpqlh7mlwft@quack3>
 <c7a79489-c0d7-968b-4fa5-c698ceb63bf9@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c7a79489-c0d7-968b-4fa5-c698ceb63bf9@huawei.com>
Precedence: bulk

On Wed 08-02-23 15:10:35, zhanchengbin wrote:
> Thanks for your comments.
> I've analyzed this situation, If a failure occurs at a certain layer, the
> start of the upper and lower logical blocks is different, this's same as
> ext4_ext_rm_idx.
> If this happens data is not flushed to disks so data on disks is
> consistent, but data on the memory is inconsistent (have journal). In my
> opinion, we just need to ensure that we don't use the wrong data and flush
> to disk. Look code we can know if ext4_ext_get_access and ext4_ext_dirty
> faild, the verified flag of bh will be cleared, if read this bad inode
> again, read_extent_tree_block will check verified flag and goto
> __ext4_ext_check, finally, return error in the ext4_valid_extent_entries
> function if the logical block start is incorrect, So does not change the
> consistency of data on the disk. (Emmmmmm, I misunderstand the judgment in
> ext4_valid_extent_entries. Later, I will clear the verified flag from the
> modified bh when ext4_valid_extent_entries fails.)
> If no journal, the data on the disk is inconsistent, too. Can use fsck to
> fix it.
> What do you think?

So I agree that as soon as we abort the journal, modified data cannot get
to the disk and so we will not be writing inconsistent extent tree to the
disk. But we could still succeed in submitting requests to zero-out parts
of existing extent and that may corrupt the filesystem if the journal is
already aborted at that moment and gets replayed to some previous, not
quite known state.  So in that case is there any point in trying to fixup
anything?  The only occasions where it makes sense trying to keep extent
tree consistent is during some non-catastrophical errors - currently we
have ENOSPC, EDQUOT, ENOMEM - which make sense because from these we should
better recover without corrupting the filesystem. But I don't really see
any point in trying to fixup the "catastrophical" errors like EIO or
EFSCORRUPTED, it can do only harm.

								Honza
> On 2023/2/7 22:23, Jan Kara wrote:
> > On Tue 07-02-23 15:09:31, zhanchengbin wrote:
> > > Inside the ext4_ext_insert_extent function, every error returned will
> > > not destroy the consistency of the tree. Even if it fails after changing
> > > half of the tree, can also ensure that the tree is self-consistent, like
> > > function ext4_ext_create_new_leaf.
> > 
> > Hum, but e.g. if ext4_ext_correct_indexes() fails, we *will* end up with
> > corrupted extent tree pretty much without a chance for recovery, won't we?
> > 
> > 								Honza
> > 
> > > After ext4_ext_insert_extent fails, update extent status tree depends on
> > > the incoming split_flag. So restore the len of extent to be split when
> > > ext4_ext_insert_extent return failed in ext4_split_extent_at.
> > > 
> > > Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
> > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> > > ---
> > >   fs/ext4/extents.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > > index 3559ea6b0781..b926fef73de4 100644
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
> > >   		bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
> > >   		if (IS_ERR(bh)) {
> > > +			EXT4_ERROR_INODE(inode, "IO error reading extent block");
> > >   			ret = PTR_ERR(bh);
> > >   			goto err;
> > >   		}
> > > @@ -3251,7 +3252,7 @@ static int ext4_split_extent_at(handle_t *handle,
> > >   		ext4_ext_mark_unwritten(ex2);
> > >   	err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
> > > -	if (err != -ENOSPC && err != -EDQUOT && err != -ENOMEM)
> > > +	if (!err)
> > >   		goto out;
> > >   	if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> > > -- 
> > > 2.31.1
> > > 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR