Re: [PATCH] ext4: do not WARN when starting a journal on a frozen filesystem

[Jan Kara]

On Thu 23-07-26 20:50:36, Luyao Bai wrote:
> A WARNING is triggered in ext4_journal_check_start() when a background
> writeback thread attempts to start a journal transaction while the
> filesystem is in the SB_FREEZE_COMPLETE state:
> 
>   WARNING: CPU: 1 PID: 2903 at fs/ext4/ext4_jbd2.c:76
>            ext4_journal_check_start+0x1f8/0x250
>   Call Trace:
>    __ext4_journal_start_sb+0x181/0x600 fs/ext4/ext4_jbd2.c:105
>    __ext4_journal_start fs/ext4/ext4_jbd2.h:326 [inline]
>    ext4_do_writepages+0x112c/0x3d20 fs/ext4/inode.c:2707
>    ext4_writepages+0x213/0x3c0 fs/ext4/inode.c:2813
>    do_writepages+0x35f/0x870 mm/page-writeback.c:2683
>    __writeback_single_inode+0x14f/0x10d0 fs/fs-writeback.c:1658
>    writeback_sb_inodes+0x80c/0x1370 fs/fs-writeback.c:1954
>    wb_writeback+0x41b/0xbd0 fs/fs-writeback.c:2134
>    wb_workfn+0x410/0x1090 fs/fs-writeback.c:2321

How this patch is broken has been already explained by others. Let me just
add that usually bugs like these are caused by stale PAGECACHE_TAG_DIRTY
tags in the xarray (so tags are still there but folios aren't really dirty
anymore so writeback wouldn't do anything in the end). We had several fixes
for those lately. So that's the direction you might want to explore and I
also suggest verifying whether the problem still reproduces with current
Ted's tree because it might well be it got already fixed.

								Honza


> 
> The background writeback flusher does not take freeze protection, so it
> can legitimately reach ext4_do_writepages() and try to start a journal
> handle while the filesystem is frozen. This happens, for example, when
> ext4_writepages() failed earlier (e.g. -ENOSPC or -EDQUOT) during the
> sync_filesystem() phase of the freeze: sync_filesystem() can still
> return 0, the freeze completes, but dirty pages are left behind. A later
> writeback pass then tries to write them out on the now-frozen
> filesystem.
> 
> The same state is reachable through the EXT4_IOC_SHUTDOWN path: if
> fs_bdev_freeze() succeeds in freezing the superblock but the subsequent
> sync_blockdev() fails, the filesystem is left in SB_FREEZE_COMPLETE and
> dirty pages remain, so writeback eventually retries and trips the WARN.
> 
> Because this state can be reached without any kernel bug, WARN_ON() is
> the wrong tool here: WARN_ON() must only fire on conditions that should
> never happen. Replace it with an ext4_msg() error message and return
> -EROFS, rejecting the transaction cleanly. All callers of
> __ext4_journal_start_sb() already handle an error return (it is the same
> path used for the existing is_journal_aborted() -EROFS case), so the
> dirty pages are simply kept and written back once the filesystem is
> thawed.
> 
> This patch is intentionally limited to the ext4 journal-start check,
> which is the direct cause of the reported warning. Any improvements to
> the generic VFS freeze/thaw error handling in fs/super.c are a separate
> concern and are deliberately left out of this fix.
> 
> Reported-by: syzbot+b75d75f957975f3d40e3@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=b75d75f957975f3d40e3
> Fixes: 49ef8832fb1a ("bdev: implement freeze and thaw holder operations")
> Signed-off-by: Luyao Bai <bailuyao1997@xxxxxxxxx>
> ---
> Tested with the syzbot reproducer under QEMU: the WARNING at
> fs/ext4/ext4_jbd2.c fires on a clean mainline build and no longer fires
> with this patch applied.
> 
>  fs/ext4/ext4_jbd2.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index 9a8c225f2..1756039b1 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -75,7 +75,12 @@ static int ext4_journal_check_start(struct super_block *sb)
>  	if (WARN_ON_ONCE(sb_rdonly(sb)))
>  		return -EROFS;
> 
> -	WARN_ON(sb->s_writers.frozen == SB_FREEZE_COMPLETE);
> +	if (unlikely(sb->s_writers.frozen == SB_FREEZE_COMPLETE)) {
> +		ext4_msg(sb, KERN_ERR,
> +			 "Attempt to start a journal transaction on a frozen filesystem");
> +		return -EROFS;
> +	}
> +
>  	journal = EXT4_SB(sb)->s_journal;
>  	/*
>  	 * Special case here: if the journal has aborted behind our
> --
> 2.43.0
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR