Re: [PATCH v3 2/3] ext4: avoid journaling sb update on error if journal is destroying

[Ojaswin Mujoo]

On Fri, Mar 14, 2025 at 11:10:40AM +0100, Jan Kara wrote:
> On Fri 14-03-25 13:14:10, Ojaswin Mujoo wrote:
> > Presently we always BUG_ON if trying to start a transaction on a journal marked
> > with JBD2_UNMOUNT, since this should never happen. However, while ltp running
> > stress tests, it was observed that in case of some error handling paths, it is
> > possible for update_super_work to start a transaction after the journal is
> > destroyed eg:
> > 
> > (umount)
> > ext4_kill_sb
> >   kill_block_super
> >     generic_shutdown_super
> >       sync_filesystem /* commits all txns */
> >       evict_inodes
> >         /* might start a new txn */
> >       ext4_put_super
> > 	flush_work(&sbi->s_sb_upd_work) /* flush the workqueue */
> >         jbd2_journal_destroy
> >           journal_kill_thread
> >             journal->j_flags |= JBD2_UNMOUNT;
> >           jbd2_journal_commit_transaction
> >             jbd2_journal_get_descriptor_buffer
> >               jbd2_journal_bmap
> >                 ext4_journal_bmap
> >                   ext4_map_blocks
> >                     ...
> >                     ext4_inode_error
> >                       ext4_handle_error
> >                         schedule_work(&sbi->s_sb_upd_work)
> > 
> >                                                /* work queue kicks in */
> >                                                update_super_work
> >                                                  jbd2_journal_start
> >                                                    start_this_handle
> >                                                      BUG_ON(journal->j_flags &
> >                                                             JBD2_UNMOUNT)
> > 
> > Hence, introduce a new mount flag to indicate journal is destroying and only do
> > a journaled (and deferred) update of sb if this flag is not set. Otherwise, just
> > fallback to an un-journaled commit.
> > 
> > Further, in the journal destroy path, we have the following sequence:
> > 
> >   1. Set mount flag indicating journal is destroying
> >   2. force a commit and wait for it
> >   3. flush pending sb updates
> > 
> > This sequence is important as it ensures that, after this point, there is no sb
> > update that might be journaled so it is safe to update the sb outside the
> > journal. (To avoid race discussed in 2d01ddc86606)
> > 
> > Also, we don't need a similar check in ext4_grp_locked_error since it is only
> > called from mballoc and AFAICT it would be always valid to schedule work here.
> > 
> > Fixes: 2d01ddc86606 ("ext4: save error info to sb through journal if available")
> > Reported-by: Mahesh Kumar <maheshkumar657g@xxxxxxxxx>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
> 
> Mostly looks good. Couple of small comments below:
> 
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -1179,6 +1179,7 @@ struct ext4_inode_info {
> >  #define	EXT4_ERROR_FS			0x0002	/* Errors detected */
> >  #define	EXT4_ORPHAN_FS			0x0004	/* Orphans being recovered */
> >  #define EXT4_FC_REPLAY			0x0020	/* Fast commit replay ongoing */
> > +#define EXT4_JOURNAL_DESTORY		0x0040	/* Journal is in process of destroying */
> 
> This should be defined as part of the following enum:
> 
> /*
>  * run-time mount flags
>  */
> enum {
>         EXT4_MF_MNTDIR_SAMPLED,
>         EXT4_MF_FC_INELIGIBLE   /* Fast commit ineligible */
> };

Right, I'll fix that
> 
> Also you have a typo in the flag name. I guess it should be
> EXT4_MF_JOURNAL_DESTROY.

I ended up making the same typo as before :) 
> 
> > @@ -4957,8 +4961,6 @@ static int ext4_load_and_init_journal(struct super_block *sb,
> >  	return 0;
> >  
> >  out:
> > -	/* flush s_sb_upd_work before destroying the journal. */
> > -	flush_work(&sbi->s_sb_upd_work);
> >  	ext4_journal_destroy(sbi, sbi->s_journal);
> >  	return -EINVAL;
> >  }
> > @@ -5648,8 +5650,6 @@ failed_mount8: __maybe_unused
> >  	sbi->s_ea_block_cache = NULL;
> >  
> >  	if (sbi->s_journal) {
> > -		/* flush s_sb_upd_work before journal destroy. */
> > -		flush_work(&sbi->s_sb_upd_work);
> >  		ext4_journal_destroy(sbi, sbi->s_journal);
> >  	}
> >  failed_mount3a:
> 
> These are good. I would also move the flush_work() in ext4_put_super()
> into else branch of:
> 
> 	if (sbi->s_journal)

Right I thought of that but then let it be for simplicity. But sure I'll
add this in the next revision.

Thanks for the review Jan.
> 
> 								Honza
> 
> -- 
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR