ext4: set csum seed in tmp inode while migrating to extents
PATCH 1/1 ext4: set csum seed in tmp inode while migrating to extents
In reply to a message not in the archiveWhen 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.
--- fs/ext4/migrate.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
fs/ext4/migrate.c +5 -1
@@ -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;struct inode *tmp_inode =NULL ;struct migrate_struct lb;unsigned long max_entries;
@@ -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_data = ei->i_data;memset (&lb,0 ,sizeof (lb));
Jan Kara
reviewerQuoted 10 lines · Luís Henriques
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.
Thanks for debugging this! Two comments below:
fs/ext4/migrate.c int ext4_ext_migrate(struct inode *inode) · line 416
Quoted 10 lines
fs/ext4/migrate.c +1
-1@@ -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...
fs/ext4/migrate.c int ext4_ext_migrate(struct inode *inode) · line 509
Quoted 8 lines
fs/ext4/migrate.c +4
-0@@ -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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 5 lines · Jan Kara
Quoted 4 lines · Luís Henriques
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.
Quoted 8 lines
Quoted 6 lines
This can be fixed by recalculating the extents checksums again. Or simply by copying the seed into the temporary inode.
Thanks for debugging this! Two comments below:
And thanks for the review!
Quoted 4 lines
Quoted 4 lines
fs/ext4/migrate.c +0
-0
Quoted 8 lines
fs/ext4/migrate.c int ext4_ext_migrate(struct inode *inode) · line 416
Quoted 6 lines
fs/ext4/migrate.c +1
-1@@ -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_eivariable when you use it only once...
Sure, I'll drop that new variable in v2.
Quoted 18 lines
fs/ext4/migrate.c int ext4_ext_migrate(struct inode *inode) · line 509
Quoted 8 lines
fs/ext4/migrate.c +4
-0@@ -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 thati_csum_seedis 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 thetmp_inodealthough 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:
Quoted 7 lines
1) Move
i_csum_seedinitialization to a bit earlier inext4_ext_migrate()just after we have got thetmp_inodefromext4_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.
Obviously, I did not realize the recovery process was broken and I appreciate you took the time to explain _how_ it is broken. I'll add a new item to (the bottom of) my to-do list and maybe one of these days I get to look into it.
I'll send out v2 shortly, implementing your suggestions.
Cheers,
--
Luís
Quoted 7 lines
Thanks!
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR