Date:   Tue, 14 Dec 2021 13:03:17 +0100
From:   Jan Kara <jack@suse.cz>
To:     =?iso-8859-1?Q?Lu=EDs?= Henriques <lhenriques@suse.de>
Cc:     Theodore Ts'o <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jeroen van Wolffelaar <jeroen@wolffelaar.nl>
Subject: Re: [PATCH] ext4: set csum seed in tmp inode while migrating to
 extents
Message-ID: <20211214120317.GA5503@quack2.suse.cz>
References: <bug-213357-13602@https.bugzilla.kernel.org>
 <20211206143733.18918-1-lhenriques@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211206143733.18918-1-lhenriques@suse.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Mon 06-12-21 14:37:33, Lu?s Henriques wrote:
> When migrating to extents, the temporary inode will have it's own checksum
> seed.  This means that, when swapping the inodes data, the inode checksums
> will be incorrect.
> 
> This can be fixed by recalculating the extents checksums again.  Or simply
> by copying the seed into the temporary inode.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=213357
> Reported-by: Jeroen van Wolffelaar <jeroen@wolffelaar.nl>
> Signed-off-by: Lu?s Henriques <lhenriques@suse.de>

Thanks for debugging this! Two comments below:

> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 7e0b4f81c6c0..dd4ece38fc83 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -413,7 +413,7 @@ int ext4_ext_migrate(struct inode *inode)
>  	handle_t *handle;
>  	int retval = 0, i;
>  	__le32 *i_data;
> -	struct ext4_inode_info *ei;
> +	struct ext4_inode_info *ei, *tmp_ei;

Probably no need for the new tmp_ei variable when you use it only once...

> @@ -503,6 +503,10 @@ int ext4_ext_migrate(struct inode *inode)
>  	}
>  
>  	ei = EXT4_I(inode);
> +	tmp_ei = EXT4_I(tmp_inode);
> +	/* Use the right seed for checksumming */
> +	tmp_ei->i_csum_seed = ei->i_csum_seed;
> +

I think this is subtly broken in another way: If we crash in the middle of
migration, tmp_inode (and possibly attached extent tree blocks) will have
wrong checksums (remember that i_csum_seed is computed from inode number)
and so orphan cleanup will fail. On the other hand in that case the orphan
cleanup will free blocks we have already managed to attach to the tmp_inode
although they are still properly attached to the old 'inode'. So the
recovery from a crash in the middle of the migration seems to be broken
anyway. So I guess what you do is an improvement. But can you perhaps:

1) Move i_csum_seed initialization to a bit earlier in ext4_ext_migrate()
just after we have got the tmp_inode from  ext4_new_inode()? That way all
inode writes will at least happen with the same csum.

2) Add a comment you are updating the csum seed so that metadata blocks get
proper checksum for 'inode' and that recovery from a crash in the middle of
migration is currently broken.

Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR