ext4: avoid journaling sb update on error if journal is destroying

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@gmail.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Link: https://patch.msgid.link/9613c465d6ff00cd315602f99283d5f24018c3f7.1742279837.git.ojaswin@linux.ibm.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 96dc30c9388efebb954783d5350d9add001f74a2..797b5b3d69ba5c96c820266791fb8d001ff522c7 100644 (file)
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1824,7 +1824,8 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
  */
 enum {
        EXT4_MF_MNTDIR_SAMPLED,
-       EXT4_MF_FC_INELIGIBLE   /* Fast commit ineligible */
+       EXT4_MF_FC_INELIGIBLE,  /* Fast commit ineligible */
+       EXT4_MF_JOURNAL_DESTROY /* Journal is in process of destroying */
 };
 
 static inline void ext4_set_mount_flag(struct super_block *sb, int bit)

diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 9b3c9df02a396db3815ef10bf207999e10ddb0b7..3221714d9901a62f51ce4865d72f96102d22a1b2 100644 (file)
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -437,6 +437,21 @@ static inline int ext4_journal_destroy(struct ext4_sb_info *sbi, journal_t *jour
 {
        int err = 0;
 
+       /*
+        * At this point only two things can be operating on the journal.
+        * JBD2 thread performing transaction commit and s_sb_upd_work
+        * issuing sb update through the journal. Once we set
+        * EXT4_JOURNAL_DESTROY, new ext4_handle_error() calls will not
+        * queue s_sb_upd_work and ext4_force_commit() makes sure any
+        * ext4_handle_error() calls from the running transaction commit are
+        * finished. Hence no new s_sb_upd_work can be queued after we
+        * flush it here.
+        */
+       ext4_set_mount_flag(sbi->s_sb, EXT4_MF_JOURNAL_DESTROY);
+
+       ext4_force_commit(sbi->s_sb);
+       flush_work(&sbi->s_sb_upd_work);
+
        err = jbd2_journal_destroy(journal);
        sbi->s_journal = NULL;
 

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 0fd801ca78626072b93a110d4821cb8a0c5a369f..20fb60b0f368eae1b2d0368b1d4e5471296e46b9 100644 (file)
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -702,9 +702,13 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
                 * In case the fs should keep running, we need to writeout
                 * superblock through the journal. Due to lock ordering
                 * constraints, it may not be safe to do it right here so we
-                * defer superblock flushing to a workqueue.
+                * defer superblock flushing to a workqueue. We just need to be
+                * careful when the journal is already shutting down. If we get
+                * here in that case, just update the sb directly as the last
+                * transaction won't commit anyway.
                 */
-               if (continue_fs && journal)
+               if (continue_fs && journal &&
+                   !ext4_test_mount_flag(sb, EXT4_MF_JOURNAL_DESTROY))
                        schedule_work(&EXT4_SB(sb)->s_sb_upd_work);
                else
                        ext4_commit_super(sb);
@@ -1290,7 +1294,6 @@ static void ext4_put_super(struct super_block *sb)
        ext4_unregister_li_request(sb);
        ext4_quotas_off(sb, EXT4_MAXQUOTAS);
 
-       flush_work(&sbi->s_sb_upd_work);
        destroy_workqueue(sbi->rsv_conversion_wq);
        ext4_release_orphan_info(sb);
 
@@ -1300,7 +1303,8 @@ static void ext4_put_super(struct super_block *sb)
                if ((err < 0) && !aborted) {
                        ext4_abort(sb, -err, "Couldn't clean up the journal");
                }
-       }
+       } else
+               flush_work(&sbi->s_sb_upd_work);
 
        ext4_es_unregister_shrinker(sbi);
        timer_shutdown_sync(&sbi->s_err_report);
@@ -4971,8 +4975,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;
 }
@@ -5669,8 +5671,6 @@ failed_mount_wq:
        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: