| Message ID | 20201127113405.26867-3-jack@suse.cz |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | ext4: Various fixes of ext4 handling of fs errors | expand |
On Nov 27, 2020, at 4:33 AM, Jan Kara <jack@suse.cz> wrote: > > Superblock is written out either through ext4_commit_super() or through > ext4_handle_dirty_super(). In both cases we recompute the checksum so it > is not necessary to recompute it after updating superblock free inodes & > blocks counters. I searched through the code to see where s_sbh is being used, and it looks like there is one case that doesn't update the checksum using ext4_handle_dirty_super(), namely: ext4_file_ioctl(cmd=FS_IOC_GET_ENCRYPTION_PWSALT) { err = ext4_journal_get_write_access(handle, sbi->s_sbh); if (err) goto pwsalt_err_journal; generate_random_uuid(sbi->s_es->s_encrypt_pw_salt); err = ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh); I don't think that is a problem with this patch, per se, but looks like a bug that could be hit in rare cases with fscrypt + metadata_csum. It would only happen once per filesystem, and would normally be hidden by later superblock updates, but should probably be fixed anyway. Reviewed-by: Andreas Dilger <adilger@dilger.ca> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/ext4/super.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 2b08b162075c..61e6e5f156f3 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -5004,13 +5004,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > block = ext4_count_free_clusters(sb); > ext4_free_blocks_count_set(sbi->s_es, > EXT4_C2B(sbi, block)); > - ext4_superblock_csum_set(sb); > err = percpu_counter_init(&sbi->s_freeclusters_counter, block, > GFP_KERNEL); > if (!err) { > unsigned long freei = ext4_count_free_inodes(sb); > sbi->s_es->s_free_inodes_count = cpu_to_le32(freei); > - ext4_superblock_csum_set(sb); > err = percpu_counter_init(&sbi->s_freeinodes_counter, freei, > GFP_KERNEL); > } > -- > 2.16.4 > Cheers, Andreas
On Sun 29-11-20 15:11:05, Andreas Dilger wrote: > On Nov 27, 2020, at 4:33 AM, Jan Kara <jack@suse.cz> wrote: > > > > Superblock is written out either through ext4_commit_super() or through > > ext4_handle_dirty_super(). In both cases we recompute the checksum so it > > is not necessary to recompute it after updating superblock free inodes & > > blocks counters. > > I searched through the code to see where s_sbh is being used, and it > looks like there is one case that doesn't update the checksum using > ext4_handle_dirty_super(), namely: > > ext4_file_ioctl(cmd=FS_IOC_GET_ENCRYPTION_PWSALT) > { > err = ext4_journal_get_write_access(handle, sbi->s_sbh); > if (err) > goto pwsalt_err_journal; > generate_random_uuid(sbi->s_es->s_encrypt_pw_salt); > err = ext4_handle_dirty_metadata(handle, NULL, > sbi->s_sbh); > > I don't think that is a problem with this patch, per se, but looks like > a bug that could be hit in rare cases with fscrypt + metadata_csum. It > would only happen once per filesystem, and would normally be hidden by > later superblock updates, but should probably be fixed anyway. Yeah, good spotting. I'll write a fix for this. > Reviewed-by: Andreas Dilger <adilger@dilger.ca> Thanks for review! Honza > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/ext4/super.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index 2b08b162075c..61e6e5f156f3 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -5004,13 +5004,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) > > block = ext4_count_free_clusters(sb); > > ext4_free_blocks_count_set(sbi->s_es, > > EXT4_C2B(sbi, block)); > > - ext4_superblock_csum_set(sb); > > err = percpu_counter_init(&sbi->s_freeclusters_counter, block, > > GFP_KERNEL); > > if (!err) { > > unsigned long freei = ext4_count_free_inodes(sb); > > sbi->s_es->s_free_inodes_count = cpu_to_le32(freei); > > - ext4_superblock_csum_set(sb); > > err = percpu_counter_init(&sbi->s_freeinodes_counter, freei, > > GFP_KERNEL); > > } > > -- > > 2.16.4 > > > > > Cheers, Andreas > > > > >
On Fri, Nov 27, 2020 at 12:33:55PM +0100, Jan Kara wrote: > Superblock is written out either through ext4_commit_super() or through > ext4_handle_dirty_super(). In both cases we recompute the checksum so it > is not necessary to recompute it after updating superblock free inodes & > blocks counters. > > Signed-off-by: Jan Kara <jack@suse.cz> Thanks, applied. - Ted
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 2b08b162075c..61e6e5f156f3 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -5004,13 +5004,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent) block = ext4_count_free_clusters(sb); ext4_free_blocks_count_set(sbi->s_es, EXT4_C2B(sbi, block)); - ext4_superblock_csum_set(sb); err = percpu_counter_init(&sbi->s_freeclusters_counter, block, GFP_KERNEL); if (!err) { unsigned long freei = ext4_count_free_inodes(sb); sbi->s_es->s_free_inodes_count = cpu_to_le32(freei); - ext4_superblock_csum_set(sb); err = percpu_counter_init(&sbi->s_freeinodes_counter, freei, GFP_KERNEL); }
Superblock is written out either through ext4_commit_super() or through ext4_handle_dirty_super(). In both cases we recompute the checksum so it is not necessary to recompute it after updating superblock free inodes & blocks counters. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ext4/super.c | 2 -- 1 file changed, 2 deletions(-)