Re: [PATCH v3 27/33] 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 to consider:
- [High] gfs2: hang in gfs2_make_fs_ro() if withdraw occurs before
  SB_BORN
--

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

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

This commit serializes setting SDF_JOURNAL_LIVE in gfs2_make_fs_rw() with
do_withdraw() under sd_log_flush_lock, and makes gfs2_make_fs_ro() wait for
SDF_JOURNAL_LIVE to clear 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);
>  	gfs2_quota_cleanup(sdp);
>  }

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

During initial read-write mount, gfs2_fill_super() starts the logd and
quotad threads, registers the sysfs interface, and calls gfs2_make_fs_rw()
before SB_BORN is set on sb->s_flags.  If gfs2_withdraw() runs after
gfs2_make_fs_rw() sets SDF_JOURNAL_LIVE and before vfs_get_tree() sets
SB_BORN, it sets SDF_WITHDRAWN and returns early without scheduling
sd_withdraw_work:

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);
    ...
}

CPU0 (mount / remount)            CPU1 (kthread / workqueue / sysfs)
vfs_get_tree()
  ...
  gfs2_fill_super()
    gfs2_make_fs_rw()
      set_bit(SDF_JOURNAL_LIVE)
                                  gfs2_withdraw()
                                    test_and_set_bit(SDF_WITHDRAWN)
                                    if (!(s_flags & SB_BORN))
                                      return;
  sb->s_flags |= SB_BORN

gfs2_reconfigure()
  gfs2_make_fs_ro()
    gfs2_log_flush()
      if (gfs2_withdrawn(sdp))
        return;
    wait_on_bit(..., SDF_JOURNAL_LIVE, ...)
      <-- sleeps forever holding sb->s_umount

Because SDF_WITHDRAWN is already set, later calls to gfs2_withdraw() also
return early, so do_withdraw() never runs to clear SDF_JOURNAL_LIVE.  A
subsequent read-only remount bails out of gfs2_log_flush() and blocks
forever in wait_on_bit() with sb->s_umount held in write mode.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-work-fs-sb_rdonly-v3-0-0be92bb73709@kernel.org?part=27