Re: [PATCH v2] ext4: set csum seed in tmp inode while migrating to extents

Luís Henriques
2021-12-15T10:46:40Z

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));
> >