Re: [PATCH v2 25/34] gfs2: wait for a pending withdraw before remounting read-only

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gfs2: hang in gfs2_make_fs_ro() if withdraw occurs before SB_BORN
--

commit 4dbc43efc316d1e6209f721b5c7c0e13b2e81f95
Author: Christian Brauner <brauner@kernel.org>

gfs2: wait for a pending withdraw before remounting read-only

This commit fixes a race between asynchronous filesystem withdraw and
read-only remount by synchronizing SDF_JOURNAL_LIVE under sd_log_flush_lock in
gfs2_make_fs_rw(), waking waiters when do_withdraw() clears SDF_JOURNAL_LIVE,
and waiting for SDF_JOURNAL_LIVE to clear in gfs2_make_fs_ro() before calling
gfs2_quota_cleanup().

> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 04bb4cf787d4d..f61c309368ef1 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -146,12 +146,21 @@ int gfs2_make_fs_rw(struct gfs2_sbd *sdp)
>  	}
>  
>  	error = gfs2_quota_init(sdp);
> -	if (!error && gfs2_withdrawn(sdp)) {
> -		gfs2_quota_cleanup(sdp);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * Check for a withdraw under the lock do_withdraw() holds while it
> +	 * takes a live journal down, so one of them sees the other.
> +	 */
> +	down_write(&sdp->sd_log_flush_lock);
> +	if (gfs2_withdrawn(sdp))
>  		error = -EIO;
> -	}
> -	if (!error)
> +	else
>  		set_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
> +	up_write(&sdp->sd_log_flush_lock);
> +	if (error)
> +		gfs2_quota_cleanup(sdp);
>  	return error;
>  }
>  
> @@ -567,6 +576,9 @@ void gfs2_make_fs_ro(struct gfs2_sbd *sdp)
>  				   HZ * 5);
>  		gfs2_assert_warn(sdp, gfs2_log_is_empty(sdp));
>  	}
> +	/* A withdraw that do_withdraw() didn't finish yet leaves it live. */
> +	if (!test_bit(SDF_NORECOVERY, &sdp->sd_flags))
> +		wait_on_bit(&sdp->sd_flags, SDF_JOURNAL_LIVE, TASK_UNINTERRUPTIBLE);

[Severity: High]
Can wait_on_bit() in gfs2_make_fs_ro() hang forever in TASK_UNINTERRUPTIBLE
if a withdraw happens after gfs2_make_fs_rw() sets SDF_JOURNAL_LIVE during
mount, but before vfs_get_tree() sets SB_BORN?

During a read-write mount, vfs_get_tree() calls gfs2_get_tree() ->
get_tree_bdev_flags() -> gfs2_fill_super() before setting SB_BORN on
sb->s_flags. Inside gfs2_fill_super(), sysfs and the daemon threads are
started before gfs2_make_fs_rw() is called:

fs/gfs2/ops_fstype.c:gfs2_fill_super() {
	...
	if (!sb_rdonly(sb))
		error = gfs2_make_fs_rw(sdp);

	if (error) {
		gfs2_freeze_unlock(sdp);
		fs_err(sdp, "can't make FS RW: %d\n", error);
		goto fail_per_node;
	}
	gfs2_glock_dq_uninit(&mount_gh);
	gfs2_online_uevent(sdp);
	return 0;
}

Once gfs2_make_fs_rw() sets SDF_JOURNAL_LIVE under sd_log_flush_lock, there
is still a window before vfs_get_tree() sets SB_BORN where gfs2_logd,
gfs2_quotad, glock workqueues, or sysfs can trigger gfs2_withdraw().

In gfs2_withdraw(), SDF_WITHDRAWN is set, and then it returns early without
queuing sd_withdraw_work because SB_BORN is not yet set:

fs/gfs2/util.c:gfs2_withdraw() {
	...
		if (test_and_set_bit(SDF_WITHDRAWN, &sdp->sd_flags))
			return;

		dump_stack();
		/*
		 * There is no need to withdraw when the superblock hasn't been
		 * fully initialized, yet.
		 */
		if (!(sdp->sd_vfs->s_flags & SB_BORN))
			return;
		fs_err(sdp, "about to withdraw this file system\n");
		schedule_work(&sdp->sd_withdraw_work);
	...
}

Because SDF_WITHDRAWN is already set, no later gfs2_withdraw() call can
queue sd_withdraw_work, and gfs2_fill_super() returns 0 with both
SDF_WITHDRAWN and SDF_JOURNAL_LIVE set while do_withdraw() never runs.

If the filesystem is subsequently remounted read-only via
gfs2_reconfigure() -> gfs2_make_fs_ro(), gfs2_log_flush() bails out early on
the withdrawn filesystem without clearing SDF_JOURNAL_LIVE, leaving
wait_on_bit() in gfs2_make_fs_ro() sleeping forever while holding
sb->s_umount.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-work-fs-sb_rdonly-v2-0-2f44f64f0b66@kernel.org?part=25