[PATCH RFC 21/28] gfs2: don't set SB_RDONLY on withdraw
Andreas Gruenbacher
agruenba at redhat.com
Thu Oct 15 17:01:00 PDT 2026
Christian,
thanks for looking into this. I agree with a lot of the points you are
making. This message is still difficult to reply to because it
conflates multiple problems.
On Mon, Oct 12, 2026 at 1:06 PM Christian Brauner <brauner at kernel.org> wrote:
> do_withdraw() sets SB_RDONLY from the withdraw work item without holding
> sb->s_umount. gfs2_withdraw() sets SDF_WITHDRAWN right away and queues
> gfs2_withdraw_func() which runs do_withdraw(). That also happens on a
> frozen filesystem. SDF_JOURNAL_LIVE is still set then because the
> freeze log flush doesn't clear it.
Agreed, do_withdraw() shouldn't set SB_RDONLY.
> freeze_super() and thaw_super() both look at sb_rdonly() to figure out
> whether the s_writers rwsems need to be taken or released. gfs2 doesn't
> even get that far. FITHAW goes through gfs2_thaw_super() to
> gfs2_do_thaw() whose gfs2_freeze_lock_shared() fails with -EIO for a
> withdrawn filesystem. So FITHAW never gets to thaw_super() and the
> filesystem stays frozen.
So when gfs2_thaw_super() is called on a withdrawn filesystem, it
should still call thaw_super(). Fair enough.
> The only ways out are emergency thaw, which
> calls thaw_super_locked() directly, and writing 0 to the gfs2 "freeze"
> sysfs file, which calls thaw_super() directly. Both see SB_RDONLY and
> skip sb_freeze_unlock(). The rwsems stay write-locked, writes through
> file descriptors opened before the withdraw hang and umount warns:
>
> gfs2: fsid=s2test:s2fs.0: can't lock the freeze glock: -5
> gfs2: fsid=s2test:s2fs.0: GFS2: couldn't thaw filesystem: -5
> sysrq: Emergency Thaw of all frozen filesystems
> WARNING: kernel/rcu/sync.c:177 at rcu_sync_dtor+0x143/0x190
>
> [<0>] percpu_rwsem_wait+0x203/0x500
> [<0>] vfs_write+0xb57/0xfa0
According to commit d564053f0746 ("GFS2: Clean up freeze code"), the
idea of the freeze sysfs file was to make it behave like the FIFREEZE
/ FITHAW ioctls. This isn't what the code is doing at all.
> Withdrawing first and freezing afterwards isn't any better.
> freeze_super() freezes the read-only superblock without the rwsems,
> gfs2_lock_fs_check_clean() fails with -EIO and gfs2_do_thaw() can't get
> the freeze glock. FIFREEZE fails with -EIO and the superblock stays
> frozen and pinned for good.
>
> SDF_WITHDRAWN already stops all modifications. Glocks fail with -EIO,
> gfs2_trans_begin() with -EROFS and log flushes and metadata reads are
> refused. So stop setting SB_RDONLY in do_withdraw() like ext4 did in
> commit d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem
> errors").
Yes, do_withdraw() shouldn't set SB_RDONLY.
> A few things have to come with that:
>
> (1) gfs2_evict_inode() and gfs2_quota_sync() check gfs2_withdrawn() next
> to sb_rdonly(). Eviction writes the inode glock's metadata in place
> and umount evicts everything.
>
> (2) gfs2_write_inode() returns -EIO once withdrawn. sync(2), syncfs(2),
> freeze and umount skipped writeback for the read-only superblock.
> Now they reach ->write_inode() which writes the glock address space
> in place without a withdrawn check, and gfs2_ail_drain() leaves
> committed buffers dirty. Data writeback needs no check because the
> block mapping goes through gfs2_meta_read() which fails once
> withdrawn.
Okay, (1) and (2) are related to the SB_RDONLY change.
> (3) gfs2_do_thaw() thaws even if the freeze glock can't be taken again
> because the filesystem withdrew. A withdrawn filesystem can't write
> anything and its lockspace is gone anyway.
>
> (4) gfs2_freeze_super() refuses a withdrawn filesystem with -EIO before
> calling freeze_super(). It can't take the freeze glock anymore and
> freeze_super() would now take the rwsems of the read-write
> superblock before gfs2_lock_fs_check_clean() fails.
And (3) and (4) are related to thawing a withdrawn filesystem.
> Note that the SB_RDONLY change and (3) have to go in together and the
> rest has to be in place by then. With only the SB_RDONLY change a freeze
> after a withdraw takes the rwsems of the now read-write superblock and
> stays frozen for good with writers blocked. With only (3) FITHAW reaches
> thaw_super() on the read-only superblock and leaks the rwsems. And
> without (4) a freeze after a withdraw goes through the error path of
> gfs2_freeze_super() which re-locks sd_freeze_gh while
> gfs2_lock_fs_check_clean() still holds it. The umount that is now
> possible then hangs in gfs2_wait_glocks().
Not sure what should prevent us from fixing withdrawn filesystem
thawing and the SB_RDONLY issue in separate commits. Both need fixing
in the end, but that's okay.
> While withdrawn a remount to read-write fails with -EROFS instead of the
> -EIO from gfs2_make_fs_rw().
So before this patch, 'mount -o remount,rw' of a withdrawn filesystem
fails with -EIO, and this patch changes it to -EROFS. What's wrong
with -EIO? Once do_withdraw() doesn't set SB_RDONLY anymore, the
filesystem will remain read-write mounted; getting an -EROFS error in
that state seems odd.
> A remount to read-only goes through
> gfs2_make_fs_ro() which writes nothing once do_withdraw() has run, and
> since the previous patch gfs2_make_fs_ro() waits for that. So emergency
> remount (sysrq u), which now forces every withdrawn gfs2 read-only,
> works without a WARN.
Alright.
> We show "withdrawn" in the mount options next to the existing
> "demote_interface_used". A withdrawn filesystem stays "rw" so statfs()
> doesn't report ST_RDONLY anymore, and the "Mounted RO" line in the sysfs
> status file and the RDONLY= uevent field read 0 too. A second mount of
> the device and a gfs2meta mount have to be read-write now. And like with
> "emergency_ro" on ext4, remount helpers that pass the shown options back
> fail. busybox mount for example now gets "Unknown parameter
> 'withdrawn'".
>
> Path based operations fail with -EIO as before. The mount root is the
> exception because it isn't looked up. So operations on it now fail with
> -EIO from the glocks instead of -EROFS from mnt_want_write(), and the
> same goes for fd based chmod, chown, utimes, setxattr and fileattr
> changes. syncfs(2) on a withdrawn filesystem returns gfs2_sync_fs()'s
> sd_log_error, e.g. -EIO, instead of 0. FITHAW of a filesystem that
> withdrew while frozen works now and FIFREEZE of a withdrawn filesystem
> fails with -EIO without freezing it.
>
> Reproducer (the sb_rdonly selftest at the end of this series covers it
> as well):
>
> mkfs.gfs2 -O -p lock_nolock -j 1 -t test:fs img; mount -o loop img /mnt
> exec 3>>/mnt/a 4</mnt # paths stop working after the withdraw
> fsfreeze -f /mnt
> echo 1 > /sys/fs/gfs2/test:fs/withdraw; sleep 8
> <FITHAW on fd 4> # before: EIO, stays frozen; after: thawed
> echo j > /proc/sysrq-trigger # before: the only way out, leaks the rwsems
> echo x >&3 # before: hangs in percpu_rwsem_wait(); after: EIO
> umount /mnt # before: WARNING at rcu_sync_dtor()
>
> Fixes: 601ef0d52e96 ("gfs2: Force withdraw to replay journals and wait for it to finish")
> Fixes: b77b4a4815a9 ("gfs2: Rework freeze / thaw logic")
> Cc: stable at vger.kernel.org
> Signed-off-by: Christian Brauner (Amutable) <brauner at kernel.org>
> ---
> fs/gfs2/ops_fstype.c | 6 +++++-
> fs/gfs2/quota.c | 2 +-
> fs/gfs2/super.c | 19 +++++++++++++++++--
> fs/gfs2/util.c | 6 ++++--
> 4 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 718e0da7dfce..d7f7e8b75971 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -1594,7 +1594,11 @@ static int gfs2_reconfigure(struct fs_context *fc)
> if (fc->sb_flags & SB_RDONLY) {
> gfs2_make_fs_ro(sdp);
> } else {
> - error = gfs2_make_fs_rw(sdp);
> + /* A withdrawn filesystem can't go back to read-write. */
> + if (gfs2_withdrawn(sdp))
> + error = -EROFS;
> + else
> + error = gfs2_make_fs_rw(sdp);
>
> if (error)
> errorfc(fc, "unable to remount read-write");
> }
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index 001c8b39ca55..7fac70bae129 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -1329,7 +1329,7 @@ int gfs2_quota_sync(struct super_block *sb, int type)
> u64 sync_gen;
> int error = 0;
>
> - if (sb_rdonly(sdp->sd_vfs))
> + if (sb_rdonly(sdp->sd_vfs) || gfs2_withdrawn(sdp))
> return 0;
>
> qda = kzalloc_objs(struct gfs2_quota_data *, max_qd);
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 1d7b39354d65..ec7bce124376 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -456,6 +456,9 @@ static int gfs2_write_inode(struct inode *inode, struct writeback_control *wbc)
> int ret = 0;
> bool flush_all = (wbc->sync_mode == WB_SYNC_ALL || gfs2_is_jdata(ip));
>
> + if (gfs2_withdrawn(sdp))
> + return -EIO;
> +
> if (flush_all)
> gfs2_log_flush(GFS2_SB(inode), ip->i_gl,
> GFS2_LOG_HEAD_FLUSH_NORMAL |
> @@ -686,7 +689,12 @@ static int gfs2_do_thaw(struct gfs2_sbd *sdp, enum freeze_holder who, const void
> int error;
>
> error = gfs2_freeze_lock_shared(sdp);
> - if (error)
> + /*
> + * A withdrawn filesystem can't get the freeze glock back, but it won't
> + * write anything either. Thaw it anyway instead of leaving it frozen
> + * for good.
> + */
> + if (error && !gfs2_withdrawn(sdp))
> goto fail;
> error = thaw_super(sb, who, freeze_owner);
> if (!error)
> @@ -751,6 +759,11 @@ static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who,
> mutex_unlock(&sdp->sd_freeze_mutex);
> return -EBUSY;
> }
> + /* A withdrawn filesystem can't take the freeze glock anymore. */
> + if (gfs2_withdrawn(sdp)) {
> + mutex_unlock(&sdp->sd_freeze_mutex);
> + return -EIO;
> + }
This is racy; the filesystem can still be withdrawn after this
gfs2_withdrawn() check. It should make more sense to bail out of the
for loop in gfs2_freeze_super() on withdrawn filesystems.
> for (;;) {
> error = freeze_super(sb, who, freeze_owner);
> @@ -1165,6 +1178,8 @@ static int gfs2_show_options(struct seq_file *s, struct dentry *root)
> seq_puts(s, ",nobarrier");
> if (test_bit(SDF_DEMOTE, &sdp->sd_flags))
> seq_puts(s, ",demote_interface_used");
> + if (gfs2_withdrawn(sdp))
> + seq_puts(s, ",withdrawn");
I'm not a fan of those fake mount options. It doesn't matter all that
much for "withdrawn" because remounting will fail anyway, but it's
still messy.
Filesystems with "demote_interface_used" can be remounted, so that
fake option is worse.
"demote_rq" and "demote_interface_used" should never have been added
in the first place, but here we are.
> if (args->ar_rgrplvb)
> seq_puts(s, ",rgrplvb");
> if (args->ar_loccookie)
> @@ -1466,7 +1481,7 @@ static void gfs2_evict_inode(struct inode *inode)
> int ret;
>
> gfs2_holder_mark_uninitialized(&gh);
> - if (sb_rdonly(sb) || !ip->i_no_addr || !ip->i_gl)
> + if (sb_rdonly(sb) || gfs2_withdrawn(sdp) || !ip->i_no_addr || !ip->i_gl)
> goto out;
>
> /*
> diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
> index 1535ce2b7aa8..990552f42a0b 100644
> --- a/fs/gfs2/util.c
> +++ b/fs/gfs2/util.c
> @@ -133,9 +133,11 @@ static void do_withdraw(struct gfs2_sbd *sdp)
> gfs2_log_is_empty(sdp),
> HZ * 5);
>
> - sdp->sd_vfs->s_flags |= SB_RDONLY;
> -
> /*
> + * We don't set SB_RDONLY. That requires sb->s_umount, and changing it
> + * under freeze_super() and thaw_super() leaves the freeze protection
> + * unbalanced. SDF_WITHDRAWN already stops all modifications.
> + *
> * Dequeue any pending non-system glock holders that can no
> * longer be granted because the file system is withdrawn.
> */
>
> --
> 2.53.0
>
Thanks,
Andreas
More information about the linux-mtd
mailing list