Date:   Wed, 15 Dec 2021 10:46:30 +0000
From:   =?iso-8859-1?Q?Lu=EDs?= Henriques <lhenriques@suse.de>
To:     "Darrick J. Wong" <djwong@kernel.org>
Cc:     Theodore Ts'o <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        Jan Kara <jack@suse.cz>, linux-ext4@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Jeroen van Wolffelaar <jeroen@wolffelaar.nl>
Subject: Re: [PATCH v2] ext4: set csum seed in tmp inode while migrating to
 extents
Message-ID: <YbnHhgYgcrA8KrwB@suse.de>
References: <20211214175058.19511-1-lhenriques@suse.de>
 <20211215004945.GD69182@magnolia>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211215004945.GD69182@magnolia>
Precedence: bulk

On Tue, Dec 14, 2021 at 04:49:45PM -0800, Darrick J. Wong wrote:
> On Tue, Dec 14, 2021 at 05:50:58PM +0000, 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>
> > ---
> >  fs/ext4/migrate.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > changes since v1:
> > 
> > * Dropped tmp_ei variable
> > * ->i_csum_seed is now initialised immediately after tmp_inode is created
> > * New comment about the seed initialization and stating that recovery
> >   needs to be fixed.
> > 
> > Cheers,
> > --
> > Lu?s
> > 
> > diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> > index 7e0b4f81c6c0..36dfc88ce05b 100644
> > --- a/fs/ext4/migrate.c
> > +++ b/fs/ext4/migrate.c
> > @@ -459,6 +459,17 @@ int ext4_ext_migrate(struct inode *inode)
> >  		ext4_journal_stop(handle);
> >  		goto out_unlock;
> >  	}
> > +	/*
> > +	 * Use the correct seed for checksum (i.e. the seed from 'inode').  This
> > +	 * is so that the metadata blocks will have the correct checksum after
> > +	 * the migration.
> > +	 *
> > +	 * Note however that, if a crash occurs during the migration process,
> > +	 * the recovery process is broken because the tmp_inode checksums will
> > +	 * be wrong and the orphans cleanup will fail.
> 
> ...and then what does the user do?

I can't really say I know what is the right thing for a user to do.  But
my understanding is that my patch doesn't change a lot: the recovery
process would still fail in a slightly different way, and will need to be
fixed at some point.

I believe the userspace tools already have support to handle orphan file,
but a quick look at the latest version of the patchset show that these
tools can't handle this either, and the recovery will also fail.  But I
may be wrong.

Cheers,
--
Lu?s

> 
> --D
> 
> > +	 */
> > +	ei = EXT4_I(inode);
> > +	EXT4_I(tmp_inode)->i_csum_seed = ei->i_csum_seed;
> >  	i_size_write(tmp_inode, i_size_read(inode));
> >  	/*
> >  	 * Set the i_nlink to zero so it will be deleted later
> > @@ -502,7 +513,6 @@ int ext4_ext_migrate(struct inode *inode)
> >  		goto out_tmp_inode;
> >  	}
> >  
> > -	ei = EXT4_I(inode);
> >  	i_data = ei->i_data;
> >  	memset(&lb, 0, sizeof(lb));
> >