From 880b9577855edddda1e732748e849c63199d489b Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Mon, 17 Jul 2023 09:00:09 -0700 Subject: [PATCH 01/47] fs: distinguish between user initiated freeze and kernel initiated freeze Userspace can freeze a filesystem using the FIFREEZE ioctl or by suspending the block device; this state persists until userspace thaws the filesystem with the FITHAW ioctl or resuming the block device. Since commit 18e9e5104fcd ("Introduce freeze_super and thaw_super for the fsfreeze ioctl") we only allow the first freeze command to succeed. The kernel may decide that it is necessary to freeze a filesystem for its own internal purposes, such as suspends in progress, filesystem fsck activities, or quiescing a device prior to removal. Userspace thaw commands must never break a kernel freeze, and kernel thaw commands shouldn't undo userspace's freeze command. Introduce a couple of freeze holder flags and wire it into the sb_writers state. One kernel and one userspace freeze are allowed to coexist at the same time; the filesystem will not thaw until both are lifted. I wonder if the f2fs/gfs2 code should be using a kernel freeze here, but for now we'll use FREEZE_HOLDER_USERSPACE to preserve existing behaviors. Cc: mcgrof@kernel.org Cc: jack@suse.cz Cc: hch@infradead.org Cc: ruansy.fnst@fujitsu.com Signed-off-by: Darrick J. Wong Reviewed-by: Christoph Hellwig Reviewed-by: Dave Chinner Reviewed-by: Jan Kara --- Documentation/filesystems/vfs.rst | 6 ++- block/bdev.c | 8 ++-- fs/f2fs/gc.c | 8 ++-- fs/gfs2/super.c | 12 +++-- fs/gfs2/sys.c | 4 +- fs/ioctl.c | 8 ++-- fs/super.c | 79 +++++++++++++++++++++++++++---- include/linux/fs.h | 15 ++++-- 8 files changed, 106 insertions(+), 34 deletions(-) diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst index cb2a97e49872..176dd4606d10 100644 --- a/Documentation/filesystems/vfs.rst +++ b/Documentation/filesystems/vfs.rst @@ -260,9 +260,11 @@ filesystem. The following members are defined: void (*evict_inode) (struct inode *); void (*put_super) (struct super_block *); int (*sync_fs)(struct super_block *sb, int wait); - int (*freeze_super) (struct super_block *); + int (*freeze_super) (struct super_block *sb, + enum freeze_holder who); int (*freeze_fs) (struct super_block *); - int (*thaw_super) (struct super_block *); + int (*thaw_super) (struct super_block *sb, + enum freeze_wholder who); int (*unfreeze_fs) (struct super_block *); int (*statfs) (struct dentry *, struct kstatfs *); int (*remount_fs) (struct super_block *, int *, char *); diff --git a/block/bdev.c b/block/bdev.c index 979e28a46b98..80ea3fa3593b 100644 --- a/block/bdev.c +++ b/block/bdev.c @@ -248,9 +248,9 @@ int freeze_bdev(struct block_device *bdev) if (!sb) goto sync; if (sb->s_op->freeze_super) - error = sb->s_op->freeze_super(sb); + error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE); else - error = freeze_super(sb); + error = freeze_super(sb, FREEZE_HOLDER_USERSPACE); deactivate_super(sb); if (error) { @@ -291,9 +291,9 @@ int thaw_bdev(struct block_device *bdev) goto out; if (sb->s_op->thaw_super) - error = sb->s_op->thaw_super(sb); + error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE); else - error = thaw_super(sb); + error = thaw_super(sb, FREEZE_HOLDER_USERSPACE); if (error) bdev->bd_fsfreeze_count++; else diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 01effd3fcb6c..a1ca394bc327 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -2181,12 +2181,14 @@ out_drop_write: if (err) return err; - err = freeze_super(sbi->sb); + err = freeze_super(sbi->sb, FREEZE_HOLDER_USERSPACE); if (err) return err; if (f2fs_readonly(sbi->sb)) { - thaw_super(sbi->sb); + err = thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE); + if (err) + return err; return -EROFS; } @@ -2240,6 +2242,6 @@ recover_out: out_err: f2fs_up_write(&sbi->cp_global_sem); f2fs_up_write(&sbi->gc_lock); - thaw_super(sbi->sb); + thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE); return err; } diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c index 9f4d5d6549ee..ae7fedc6ddcd 100644 --- a/fs/gfs2/super.c +++ b/fs/gfs2/super.c @@ -689,7 +689,7 @@ static int gfs2_freeze_locally(struct gfs2_sbd *sdp) struct super_block *sb = sdp->sd_vfs; int error; - error = freeze_super(sb); + error = freeze_super(sb, FREEZE_HOLDER_USERSPACE); if (error) return error; @@ -697,7 +697,9 @@ static int gfs2_freeze_locally(struct gfs2_sbd *sdp) gfs2_log_flush(sdp, NULL, GFS2_LOG_HEAD_FLUSH_FREEZE | GFS2_LFC_FREEZE_GO_SYNC); if (gfs2_withdrawn(sdp)) { - thaw_super(sb); + error = thaw_super(sb, FREEZE_HOLDER_USERSPACE); + if (error) + return error; return -EIO; } } @@ -712,7 +714,7 @@ static int gfs2_do_thaw(struct gfs2_sbd *sdp) error = gfs2_freeze_lock_shared(sdp); if (error) goto fail; - error = thaw_super(sb); + error = thaw_super(sb, FREEZE_HOLDER_USERSPACE); if (!error) return 0; @@ -761,7 +763,7 @@ out: * */ -static int gfs2_freeze_super(struct super_block *sb) +static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who) { struct gfs2_sbd *sdp = sb->s_fs_info; int error; @@ -816,7 +818,7 @@ out: * */ -static int gfs2_thaw_super(struct super_block *sb) +static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who) { struct gfs2_sbd *sdp = sb->s_fs_info; int error; diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c index 2dfbe2f188dd..c60bc7f628e1 100644 --- a/fs/gfs2/sys.c +++ b/fs/gfs2/sys.c @@ -168,10 +168,10 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len) switch (n) { case 0: - error = thaw_super(sdp->sd_vfs); + error = thaw_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE); break; case 1: - error = freeze_super(sdp->sd_vfs); + error = freeze_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE); break; default: return -EINVAL; diff --git a/fs/ioctl.c b/fs/ioctl.c index 5b2481cd4750..a56cbceedcd1 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -396,8 +396,8 @@ static int ioctl_fsfreeze(struct file *filp) /* Freeze */ if (sb->s_op->freeze_super) - return sb->s_op->freeze_super(sb); - return freeze_super(sb); + return sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE); + return freeze_super(sb, FREEZE_HOLDER_USERSPACE); } static int ioctl_fsthaw(struct file *filp) @@ -409,8 +409,8 @@ static int ioctl_fsthaw(struct file *filp) /* Thaw */ if (sb->s_op->thaw_super) - return sb->s_op->thaw_super(sb); - return thaw_super(sb); + return sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE); + return thaw_super(sb, FREEZE_HOLDER_USERSPACE); } static int ioctl_file_dedupe_range(struct file *file, diff --git a/fs/super.c b/fs/super.c index e781226e2880..e6db39aadaee 100644 --- a/fs/super.c +++ b/fs/super.c @@ -39,7 +39,7 @@ #include #include "internal.h" -static int thaw_super_locked(struct super_block *sb); +static int thaw_super_locked(struct super_block *sb, enum freeze_holder who); static LIST_HEAD(super_blocks); static DEFINE_SPINLOCK(sb_lock); @@ -1030,7 +1030,7 @@ static void do_thaw_all_callback(struct super_block *sb) down_write(&sb->s_umount); if (sb->s_root && sb->s_flags & SB_BORN) { emergency_thaw_bdev(sb); - thaw_super_locked(sb); + thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE); } else { up_write(&sb->s_umount); } @@ -1647,11 +1647,22 @@ static void sb_freeze_unlock(struct super_block *sb, int level) /** * freeze_super - lock the filesystem and force it into a consistent state * @sb: the super to lock + * @who: context that wants to freeze * * Syncs the super to make sure the filesystem is consistent and calls the fs's - * freeze_fs. Subsequent calls to this without first thawing the fs will return + * freeze_fs. Subsequent calls to this without first thawing the fs may return * -EBUSY. * + * @who should be: + * * %FREEZE_HOLDER_USERSPACE if userspace wants to freeze the fs; + * * %FREEZE_HOLDER_KERNEL if the kernel wants to freeze the fs. + * + * The @who argument distinguishes between the kernel and userspace trying to + * freeze the filesystem. Although there cannot be multiple kernel freezes or + * multiple userspace freezes in effect at any given time, the kernel and + * userspace can both hold a filesystem frozen. The filesystem remains frozen + * until there are no kernel or userspace freezes in effect. + * * During this function, sb->s_writers.frozen goes through these values: * * SB_UNFROZEN: File system is normal, all writes progress as usual. @@ -1677,12 +1688,30 @@ static void sb_freeze_unlock(struct super_block *sb, int level) * * sb->s_writers.frozen is protected by sb->s_umount. */ -int freeze_super(struct super_block *sb) +int freeze_super(struct super_block *sb, enum freeze_holder who) { int ret; atomic_inc(&sb->s_active); down_write(&sb->s_umount); + + if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) { + if (sb->s_writers.freeze_holders & who) { + deactivate_locked_super(sb); + return -EBUSY; + } + + WARN_ON(sb->s_writers.freeze_holders == 0); + + /* + * Someone else already holds this type of freeze; share the + * freeze and assign the active ref to the freeze. + */ + sb->s_writers.freeze_holders |= who; + up_write(&sb->s_umount); + return 0; + } + if (sb->s_writers.frozen != SB_UNFROZEN) { deactivate_locked_super(sb); return -EBUSY; @@ -1695,6 +1724,7 @@ int freeze_super(struct super_block *sb) if (sb_rdonly(sb)) { /* Nothing to do really... */ + sb->s_writers.freeze_holders |= who; sb->s_writers.frozen = SB_FREEZE_COMPLETE; up_write(&sb->s_umount); return 0; @@ -1738,6 +1768,7 @@ int freeze_super(struct super_block *sb) * For debugging purposes so that fs can warn if it sees write activity * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super(). */ + sb->s_writers.freeze_holders |= who; sb->s_writers.frozen = SB_FREEZE_COMPLETE; lockdep_sb_freeze_release(sb); up_write(&sb->s_umount); @@ -1745,16 +1776,39 @@ int freeze_super(struct super_block *sb) } EXPORT_SYMBOL(freeze_super); -static int thaw_super_locked(struct super_block *sb) +/* + * Undoes the effect of a freeze_super_locked call. If the filesystem is + * frozen both by userspace and the kernel, a thaw call from either source + * removes that state without releasing the other state or unlocking the + * filesystem. + */ +static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) { int error; - if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) { + if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) { + if (!(sb->s_writers.freeze_holders & who)) { + up_write(&sb->s_umount); + return -EINVAL; + } + + /* + * Freeze is shared with someone else. Release our hold and + * drop the active ref that freeze_super assigned to the + * freezer. + */ + if (sb->s_writers.freeze_holders & ~who) { + sb->s_writers.freeze_holders &= ~who; + deactivate_locked_super(sb); + return 0; + } + } else { up_write(&sb->s_umount); return -EINVAL; } if (sb_rdonly(sb)) { + sb->s_writers.freeze_holders &= ~who; sb->s_writers.frozen = SB_UNFROZEN; goto out; } @@ -1772,6 +1826,7 @@ static int thaw_super_locked(struct super_block *sb) } } + sb->s_writers.freeze_holders &= ~who; sb->s_writers.frozen = SB_UNFROZEN; sb_freeze_unlock(sb, SB_FREEZE_FS); out: @@ -1782,13 +1837,19 @@ out: /** * thaw_super -- unlock filesystem * @sb: the super to thaw + * @who: context that wants to freeze * - * Unlocks the filesystem and marks it writeable again after freeze_super(). + * Unlocks the filesystem and marks it writeable again after freeze_super() + * if there are no remaining freezes on the filesystem. + * + * @who should be: + * * %FREEZE_HOLDER_USERSPACE if userspace wants to thaw the fs; + * * %FREEZE_HOLDER_KERNEL if the kernel wants to thaw the fs. */ -int thaw_super(struct super_block *sb) +int thaw_super(struct super_block *sb, enum freeze_holder who) { down_write(&sb->s_umount); - return thaw_super_locked(sb); + return thaw_super_locked(sb, who); } EXPORT_SYMBOL(thaw_super); diff --git a/include/linux/fs.h b/include/linux/fs.h index 6867512907d6..5e8d81212abc 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1147,7 +1147,8 @@ enum { #define SB_FREEZE_LEVELS (SB_FREEZE_COMPLETE - 1) struct sb_writers { - int frozen; /* Is sb frozen? */ + unsigned short frozen; /* Is sb frozen? */ + unsigned short freeze_holders; /* Who froze fs? */ struct percpu_rw_semaphore rw_sem[SB_FREEZE_LEVELS]; }; @@ -1902,6 +1903,10 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos, struct file *dst_file, loff_t dst_pos, loff_t len, unsigned int remap_flags); +enum freeze_holder { + FREEZE_HOLDER_KERNEL = (1U << 0), + FREEZE_HOLDER_USERSPACE = (1U << 1), +}; struct super_operations { struct inode *(*alloc_inode)(struct super_block *sb); @@ -1914,9 +1919,9 @@ struct super_operations { void (*evict_inode) (struct inode *); void (*put_super) (struct super_block *); int (*sync_fs)(struct super_block *sb, int wait); - int (*freeze_super) (struct super_block *); + int (*freeze_super) (struct super_block *, enum freeze_holder who); int (*freeze_fs) (struct super_block *); - int (*thaw_super) (struct super_block *); + int (*thaw_super) (struct super_block *, enum freeze_holder who); int (*unfreeze_fs) (struct super_block *); int (*statfs) (struct dentry *, struct kstatfs *); int (*remount_fs) (struct super_block *, int *, char *); @@ -2290,8 +2295,8 @@ extern int unregister_filesystem(struct file_system_type *); extern int vfs_statfs(const struct path *, struct kstatfs *); extern int user_statfs(const char __user *, struct kstatfs *); extern int fd_statfs(int, struct kstatfs *); -extern int freeze_super(struct super_block *super); -extern int thaw_super(struct super_block *super); +int freeze_super(struct super_block *super, enum freeze_holder who); +int thaw_super(struct super_block *super, enum freeze_holder who); extern __printf(2, 3) int super_setup_bdi_name(struct super_block *sb, char *fmt, ...); extern int super_setup_bdi(struct super_block *sb); From 59ba4fdd2d1f9dd7af98f5168c846150c9aec56d Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Mon, 17 Jul 2023 09:00:10 -0700 Subject: [PATCH 02/47] fs: wait for partially frozen filesystems Jan Kara suggested that when one thread is in the middle of freezing a filesystem, another thread trying to freeze the same fs but with a different freeze_holder should wait until the freezer reaches either end state (UNFROZEN or COMPLETE) instead of returning EBUSY immediately. Neither caller can do anything sensible with this race other than retry but they cannot really distinguish EBUSY as in "some other holder of the same type has the sb already frozen" from "freezing raced with holder of a different type". Plumb in the extra code needed to wait for the fs freezer to reach an end state and try the freeze again. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner Reviewed-by: Christoph Hellwig Reviewed-by: Jan Kara --- fs/super.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/fs/super.c b/fs/super.c index e6db39aadaee..da68584815e4 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1644,6 +1644,24 @@ static void sb_freeze_unlock(struct super_block *sb, int level) percpu_up_write(sb->s_writers.rw_sem + level); } +static int wait_for_partially_frozen(struct super_block *sb) +{ + int ret = 0; + + do { + unsigned short old = sb->s_writers.frozen; + + up_write(&sb->s_umount); + ret = wait_var_event_killable(&sb->s_writers.frozen, + sb->s_writers.frozen != old); + down_write(&sb->s_umount); + } while (ret == 0 && + sb->s_writers.frozen != SB_UNFROZEN && + sb->s_writers.frozen != SB_FREEZE_COMPLETE); + + return ret; +} + /** * freeze_super - lock the filesystem and force it into a consistent state * @sb: the super to lock @@ -1695,6 +1713,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who) atomic_inc(&sb->s_active); down_write(&sb->s_umount); +retry: if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) { if (sb->s_writers.freeze_holders & who) { deactivate_locked_super(sb); @@ -1713,8 +1732,13 @@ int freeze_super(struct super_block *sb, enum freeze_holder who) } if (sb->s_writers.frozen != SB_UNFROZEN) { - deactivate_locked_super(sb); - return -EBUSY; + ret = wait_for_partially_frozen(sb); + if (ret) { + deactivate_locked_super(sb); + return ret; + } + + goto retry; } if (!(sb->s_flags & SB_BORN)) { @@ -1726,6 +1750,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who) /* Nothing to do really... */ sb->s_writers.freeze_holders |= who; sb->s_writers.frozen = SB_FREEZE_COMPLETE; + wake_up_var(&sb->s_writers.frozen); up_write(&sb->s_umount); return 0; } @@ -1745,6 +1770,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who) if (ret) { sb->s_writers.frozen = SB_UNFROZEN; sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT); + wake_up_var(&sb->s_writers.frozen); deactivate_locked_super(sb); return ret; } @@ -1760,6 +1786,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who) "VFS:Filesystem freeze failed\n"); sb->s_writers.frozen = SB_UNFROZEN; sb_freeze_unlock(sb, SB_FREEZE_FS); + wake_up_var(&sb->s_writers.frozen); deactivate_locked_super(sb); return ret; } @@ -1770,6 +1797,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who) */ sb->s_writers.freeze_holders |= who; sb->s_writers.frozen = SB_FREEZE_COMPLETE; + wake_up_var(&sb->s_writers.frozen); lockdep_sb_freeze_release(sb); up_write(&sb->s_umount); return 0; @@ -1810,6 +1838,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) if (sb_rdonly(sb)) { sb->s_writers.freeze_holders &= ~who; sb->s_writers.frozen = SB_UNFROZEN; + wake_up_var(&sb->s_writers.frozen); goto out; } @@ -1828,6 +1857,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who) sb->s_writers.freeze_holders &= ~who; sb->s_writers.frozen = SB_UNFROZEN; + wake_up_var(&sb->s_writers.frozen); sb_freeze_unlock(sb, SB_FREEZE_FS); out: deactivate_locked_super(sb); From ce85a1e04645b1ed386b074297df27ab5b8801c0 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Fri, 4 Aug 2023 08:20:57 -0700 Subject: [PATCH 03/47] xfs: stabilize fs summary counters for online fsck If the fscounters scrubber notices incorrect summary counters, it's entirely possible that scrub is simply racing with other threads that are updating the incore counters. There isn't a good way to stabilize percpu counters or set ourselves up to observe live updates with hooks like we do for the quotacheck or nlinks scanners, so we instead choose to freeze the filesystem long enough to walk the incore per-AG structures. Past me thought that it was going to be commonplace to have to freeze the filesystem to perform some kind of repair and set up a whole separate infrastructure to freeze the filesystem in such a way that userspace could not unfreeze while we were running. This involved adding a mutex and freeze_super/thaw_super functions and dealing with the fact that the VFS freeze/thaw functions can free the VFS superblock references on return. This was all very overwrought, since fscounters turned out to be the only user of scrub freezes, and it doesn't require the log to quiesce, only the incore superblock counters. We prevent other threads from changing the freeze level by calling freeze_super_excl with a custom freeze cookie to keep everyone else out of the filesystem. The end result is that fscounters should be much more efficient. When we're checking a busy system and we can't stabilize the counters, the custom freeze will do less work, which should result in less downtime. Repair should be similarly speedy, but that's in a later patch. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/scrub/fscounters.c | 188 ++++++++++++++++++++++++++++++-------- fs/xfs/scrub/scrub.c | 6 +- fs/xfs/scrub/scrub.h | 1 + fs/xfs/scrub/trace.h | 26 ++++++ 4 files changed, 183 insertions(+), 38 deletions(-) diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c index e382a35e98d8..05be757668bb 100644 --- a/fs/xfs/scrub/fscounters.c +++ b/fs/xfs/scrub/fscounters.c @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: GPL-2.0+ +// SPDX-License-Identifier: GPL-2.0-or-later /* * Copyright (C) 2019-2023 Oracle. All Rights Reserved. * Author: Darrick J. Wong @@ -8,6 +8,8 @@ #include "xfs_shared.h" #include "xfs_format.h" #include "xfs_trans_resv.h" +#include "xfs_log_format.h" +#include "xfs_trans.h" #include "xfs_mount.h" #include "xfs_alloc.h" #include "xfs_ialloc.h" @@ -16,6 +18,7 @@ #include "xfs_ag.h" #include "xfs_rtalloc.h" #include "xfs_inode.h" +#include "xfs_icache.h" #include "scrub/scrub.h" #include "scrub/common.h" #include "scrub/trace.h" @@ -53,6 +56,7 @@ struct xchk_fscounters { uint64_t frextents; unsigned long long icount_min; unsigned long long icount_max; + bool frozen; }; /* @@ -123,6 +127,82 @@ xchk_fscount_warmup( return error; } +static inline int +xchk_fsfreeze( + struct xfs_scrub *sc) +{ + int error; + + error = freeze_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL); + trace_xchk_fsfreeze(sc, error); + return error; +} + +static inline int +xchk_fsthaw( + struct xfs_scrub *sc) +{ + int error; + + /* This should always succeed, we have a kernel freeze */ + error = thaw_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL); + trace_xchk_fsthaw(sc, error); + return error; +} + +/* + * We couldn't stabilize the filesystem long enough to sample all the variables + * that comprise the summary counters and compare them to the percpu counters. + * We need to disable all writer threads, which means taking the first two + * freeze levels to put userspace to sleep, and the third freeze level to + * prevent background threads from starting new transactions. Take one level + * more to prevent other callers from unfreezing the filesystem while we run. + */ +STATIC int +xchk_fscounters_freeze( + struct xfs_scrub *sc) +{ + struct xchk_fscounters *fsc = sc->buf; + int error = 0; + + if (sc->flags & XCHK_HAVE_FREEZE_PROT) { + sc->flags &= ~XCHK_HAVE_FREEZE_PROT; + mnt_drop_write_file(sc->file); + } + + /* Try to grab a kernel freeze. */ + while ((error = xchk_fsfreeze(sc)) == -EBUSY) { + if (xchk_should_terminate(sc, &error)) + return error; + + delay(HZ / 10); + } + if (error) + return error; + + fsc->frozen = true; + return 0; +} + +/* Thaw the filesystem after checking or repairing fscounters. */ +STATIC void +xchk_fscounters_cleanup( + void *buf) +{ + struct xchk_fscounters *fsc = buf; + struct xfs_scrub *sc = fsc->sc; + int error; + + if (!fsc->frozen) + return; + + error = xchk_fsthaw(sc); + if (error) + xfs_emerg(sc->mp, "still frozen after scrub, err=%d", error); + else + fsc->frozen = false; +} + int xchk_setup_fscounters( struct xfs_scrub *sc) @@ -140,6 +220,7 @@ xchk_setup_fscounters( sc->buf = kzalloc(sizeof(struct xchk_fscounters), XCHK_GFP_FLAGS); if (!sc->buf) return -ENOMEM; + sc->buf_cleanup = xchk_fscounters_cleanup; fsc = sc->buf; fsc->sc = sc; @@ -150,7 +231,18 @@ xchk_setup_fscounters( if (error) return error; - return xchk_trans_alloc(sc, 0); + /* + * Pause all writer activity in the filesystem while we're scrubbing to + * reduce the likelihood of background perturbations to the counters + * throwing off our calculations. + */ + if (sc->flags & XCHK_TRY_HARDER) { + error = xchk_fscounters_freeze(sc); + if (error) + return error; + } + + return xfs_trans_alloc_empty(sc->mp, &sc->tp); } /* @@ -290,8 +382,7 @@ retry: if (fsc->ifree > fsc->icount) { if (tries--) goto retry; - xchk_set_incomplete(sc); - return 0; + return -EDEADLOCK; } return 0; @@ -367,6 +458,8 @@ xchk_fscount_count_frextents( * Otherwise, we /might/ have a problem. If the change in the summations is * more than we want to tolerate, the filesystem is probably busy and we should * just send back INCOMPLETE and see if userspace will try again. + * + * If we're repairing then we require an exact match. */ static inline bool xchk_fscount_within_range( @@ -396,21 +489,7 @@ xchk_fscount_within_range( if (expected >= min_value && expected <= max_value) return true; - /* - * If the difference between the two summations is too large, the fs - * might just be busy and so we'll mark the scrub incomplete. Return - * true here so that we don't mark the counter corrupt. - * - * XXX: In the future when userspace can grant scrub permission to - * quiesce the filesystem to solve the outsized variance problem, this - * check should be moved up and the return code changed to signal to - * userspace that we need quiesce permission. - */ - if (max_value - min_value >= XCHK_FSCOUNT_MIN_VARIANCE) { - xchk_set_incomplete(sc); - return true; - } - + /* Everything else is bad. */ return false; } @@ -422,6 +501,7 @@ xchk_fscounters( struct xfs_mount *mp = sc->mp; struct xchk_fscounters *fsc = sc->buf; int64_t icount, ifree, fdblocks, frextents; + bool try_again = false; int error; /* Snapshot the percpu counters. */ @@ -431,9 +511,26 @@ xchk_fscounters( frextents = percpu_counter_sum(&mp->m_frextents); /* No negative values, please! */ - if (icount < 0 || ifree < 0 || fdblocks < 0 || frextents < 0) + if (icount < 0 || ifree < 0) xchk_set_corrupt(sc); + /* + * If the filesystem is not frozen, the counter summation calls above + * can race with xfs_mod_freecounter, which subtracts a requested space + * reservation from the counter and undoes the subtraction if that made + * the counter go negative. Therefore, it's possible to see negative + * values here, and we should only flag that as a corruption if we + * froze the fs. This is much more likely to happen with frextents + * since there are no reserved pools. + */ + if (fdblocks < 0 || frextents < 0) { + if (!fsc->frozen) + return -EDEADLOCK; + + xchk_set_corrupt(sc); + return 0; + } + /* See if icount is obviously wrong. */ if (icount < fsc->icount_min || icount > fsc->icount_max) xchk_set_corrupt(sc); @@ -446,12 +543,6 @@ xchk_fscounters( if (frextents > mp->m_sb.sb_rextents) xchk_set_corrupt(sc); - /* - * XXX: We can't quiesce percpu counter updates, so exit early. - * This can be re-enabled when we gain exclusive freeze functionality. - */ - return 0; - /* * If ifree exceeds icount by more than the minimum variance then * something's probably wrong with the counters. @@ -463,8 +554,6 @@ xchk_fscounters( error = xchk_fscount_aggregate_agcounts(sc, fsc); if (!xchk_process_error(sc, 0, XFS_SB_BLOCK(mp), &error)) return error; - if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_INCOMPLETE) - return 0; /* Count the free extents counter for rt volumes. */ error = xchk_fscount_count_frextents(sc, fsc); @@ -473,20 +562,45 @@ xchk_fscounters( if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_INCOMPLETE) return 0; - /* Compare the in-core counters with whatever we counted. */ - if (!xchk_fscount_within_range(sc, icount, &mp->m_icount, fsc->icount)) - xchk_set_corrupt(sc); + /* + * Compare the in-core counters with whatever we counted. If the fs is + * frozen, we treat the discrepancy as a corruption because the freeze + * should have stabilized the counter values. Otherwise, we need + * userspace to call us back having granted us freeze permission. + */ + if (!xchk_fscount_within_range(sc, icount, &mp->m_icount, + fsc->icount)) { + if (fsc->frozen) + xchk_set_corrupt(sc); + else + try_again = true; + } - if (!xchk_fscount_within_range(sc, ifree, &mp->m_ifree, fsc->ifree)) - xchk_set_corrupt(sc); + if (!xchk_fscount_within_range(sc, ifree, &mp->m_ifree, fsc->ifree)) { + if (fsc->frozen) + xchk_set_corrupt(sc); + else + try_again = true; + } if (!xchk_fscount_within_range(sc, fdblocks, &mp->m_fdblocks, - fsc->fdblocks)) - xchk_set_corrupt(sc); + fsc->fdblocks)) { + if (fsc->frozen) + xchk_set_corrupt(sc); + else + try_again = true; + } if (!xchk_fscount_within_range(sc, frextents, &mp->m_frextents, - fsc->frextents)) - xchk_set_corrupt(sc); + fsc->frextents)) { + if (fsc->frozen) + xchk_set_corrupt(sc); + else + try_again = true; + } + + if (try_again) + return -EDEADLOCK; return 0; } diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c index 3d98f604765e..a0fffbcd022b 100644 --- a/fs/xfs/scrub/scrub.c +++ b/fs/xfs/scrub/scrub.c @@ -184,8 +184,10 @@ xchk_teardown( xchk_irele(sc, sc->ip); sc->ip = NULL; } - if (sc->sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR) + if (sc->flags & XCHK_HAVE_FREEZE_PROT) { + sc->flags &= ~XCHK_HAVE_FREEZE_PROT; mnt_drop_write_file(sc->file); + } if (sc->buf) { if (sc->buf_cleanup) sc->buf_cleanup(sc->buf); @@ -505,6 +507,8 @@ retry_op: error = mnt_want_write_file(sc->file); if (error) goto out_sc; + + sc->flags |= XCHK_HAVE_FREEZE_PROT; } /* Set up for the operation. */ diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h index e113f2f5c254..f8ba00e51ca9 100644 --- a/fs/xfs/scrub/scrub.h +++ b/fs/xfs/scrub/scrub.h @@ -106,6 +106,7 @@ struct xfs_scrub { /* XCHK state flags grow up from zero, XREP state flags grown down from 2^31 */ #define XCHK_TRY_HARDER (1U << 0) /* can't get resources, try again */ +#define XCHK_HAVE_FREEZE_PROT (1U << 1) /* do we have freeze protection? */ #define XCHK_FSGATES_DRAIN (1U << 2) /* defer ops draining enabled */ #define XCHK_NEED_DRAIN (1U << 3) /* scrub needs to drain defer ops */ #define XREP_ALREADY_FIXED (1U << 31) /* checking our repair work */ diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h index b3894daeb86a..0b54f1a1cf0c 100644 --- a/fs/xfs/scrub/trace.h +++ b/fs/xfs/scrub/trace.h @@ -98,6 +98,7 @@ TRACE_DEFINE_ENUM(XFS_SCRUB_TYPE_FSCOUNTERS); #define XFS_SCRUB_STATE_STRINGS \ { XCHK_TRY_HARDER, "try_harder" }, \ + { XCHK_HAVE_FREEZE_PROT, "nofreeze" }, \ { XCHK_FSGATES_DRAIN, "fsgates_drain" }, \ { XCHK_NEED_DRAIN, "need_drain" }, \ { XREP_ALREADY_FIXED, "already_fixed" } @@ -693,6 +694,31 @@ TRACE_EVENT(xchk_fscounters_within_range, __entry->old_value) ) +DECLARE_EVENT_CLASS(xchk_fsfreeze_class, + TP_PROTO(struct xfs_scrub *sc, int error), + TP_ARGS(sc, error), + TP_STRUCT__entry( + __field(dev_t, dev) + __field(unsigned int, type) + __field(int, error) + ), + TP_fast_assign( + __entry->dev = sc->mp->m_super->s_dev; + __entry->type = sc->sm->sm_type; + __entry->error = error; + ), + TP_printk("dev %d:%d type %s error %d", + MAJOR(__entry->dev), MINOR(__entry->dev), + __print_symbolic(__entry->type, XFS_SCRUB_TYPE_STRINGS), + __entry->error) +); +#define DEFINE_XCHK_FSFREEZE_EVENT(name) \ +DEFINE_EVENT(xchk_fsfreeze_class, name, \ + TP_PROTO(struct xfs_scrub *sc, int error), \ + TP_ARGS(sc, error)) +DEFINE_XCHK_FSFREEZE_EVENT(xchk_fsfreeze); +DEFINE_XCHK_FSFREEZE_EVENT(xchk_fsthaw); + TRACE_EVENT(xchk_refcount_incorrect, TP_PROTO(struct xfs_perag *pag, const struct xfs_refcount_irec *irec, xfs_nlink_t seen), From 4b2201dad26742c92decd920471b7185088624f5 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 7 Aug 2023 12:26:22 +0100 Subject: [PATCH 04/47] fs: stop using bdev->bd_super in mark_buffer_write_io_error bdev->bd_super is a somewhat awkward backpointer from a block device to an owning file system with unclear rules. For the buffer_head code we already have a good backpointer for the inode that the buffer_head is associated with, even if it lives on the block device mapping: b_assoc_map. It is used track dirty buffers associated with an inode but living on the block device mapping like directory buffers in ext4. mark_buffer_write_io_error already uses it for the call to mapping_set_error, and should be doing the same for the per-sb error sequence. Signed-off-by: Christoph Hellwig Message-Id: <20230807112625.652089-2-hch@lst.de> Signed-off-by: Christian Brauner --- fs/buffer.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/fs/buffer.c b/fs/buffer.c index bd091329026c..f36ef03c078a 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -1225,19 +1225,14 @@ EXPORT_SYMBOL(mark_buffer_dirty); void mark_buffer_write_io_error(struct buffer_head *bh) { - struct super_block *sb; - set_buffer_write_io_error(bh); /* FIXME: do we need to set this in both places? */ if (bh->b_folio && bh->b_folio->mapping) mapping_set_error(bh->b_folio->mapping, -EIO); - if (bh->b_assoc_map) + if (bh->b_assoc_map) { mapping_set_error(bh->b_assoc_map, -EIO); - rcu_read_lock(); - sb = READ_ONCE(bh->b_bdev->bd_super); - if (sb) - errseq_set(&sb->s_wb_err, -EIO); - rcu_read_unlock(); + errseq_set(&bh->b_assoc_map->host->i_sb->s_wb_err, -EIO); + } } EXPORT_SYMBOL(mark_buffer_write_io_error); From 01efe93a5aa20a19b390426718dc214898a7c2ec Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 7 Aug 2023 12:26:23 +0100 Subject: [PATCH 05/47] ext4: don't use bdev->bd_super in __ext4_journal_get_write_access __ext4_journal_get_write_access already has a super_block available, and there is no need to go from that to the bdev to go back to the owning super_block. Signed-off-by: Christoph Hellwig Message-Id: <20230807112625.652089-3-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ext4/ext4_jbd2.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c index 77f318ec8abb..b38d59581411 100644 --- a/fs/ext4/ext4_jbd2.c +++ b/fs/ext4/ext4_jbd2.c @@ -234,8 +234,7 @@ int __ext4_journal_get_write_access(const char *where, unsigned int line, might_sleep(); - if (bh->b_bdev->bd_super) - ext4_check_bdev_write_error(bh->b_bdev->bd_super); + ext4_check_bdev_write_error(sb); if (ext4_handle_valid(handle)) { err = jbd2_journal_get_write_access(handle, bh); From 8887b94d93224e0ef7e1bc6369640e313b8b12f4 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 7 Aug 2023 12:26:24 +0100 Subject: [PATCH 06/47] ocfs2: stop using bdev->bd_super for journal error logging All ocfs2 journal error handling and logging is based on buffer_heads, and the owning inode and thus super_block can be retrieved through bh->b_assoc_map->host. Switch to using that to remove the last users of bdev->bd_super. Signed-off-by: Christoph Hellwig Acked-by: Joseph Qi Message-Id: <20230807112625.652089-4-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ocfs2/journal.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c index 25d8072ccfce..c19c730c26e2 100644 --- a/fs/ocfs2/journal.c +++ b/fs/ocfs2/journal.c @@ -557,7 +557,7 @@ static void ocfs2_abort_trigger(struct jbd2_buffer_trigger_type *triggers, (unsigned long)bh, (unsigned long long)bh->b_blocknr); - ocfs2_error(bh->b_bdev->bd_super, + ocfs2_error(bh->b_assoc_map->host->i_sb, "JBD2 has aborted our journal, ocfs2 cannot continue\n"); } @@ -780,14 +780,14 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh) mlog_errno(status); if (!is_handle_aborted(handle)) { journal_t *journal = handle->h_transaction->t_journal; - struct super_block *sb = bh->b_bdev->bd_super; mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. " "Aborting transaction and journal.\n"); handle->h_err = status; jbd2_journal_abort_handle(handle); jbd2_journal_abort(journal, status); - ocfs2_abort(sb, "Journal already aborted.\n"); + ocfs2_abort(bh->b_assoc_map->host->i_sb, + "Journal already aborted.\n"); } } } From 6a3207395563f724d91231ec0aa7c4d95bf9591d Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 7 Aug 2023 12:26:25 +0100 Subject: [PATCH 07/47] fs, block: remove bdev->bd_super bdev->bd_super is unused now, remove it. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Message-Id: <20230807112625.652089-5-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ext4/super.c | 1 - fs/super.c | 3 --- include/linux/blk_types.h | 1 - 3 files changed, 5 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index c94ebf704616..b24f5b9f63e0 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -5572,7 +5572,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb) spin_lock_init(&sbi->s_bdev_wb_lock); errseq_check_and_advance(&sb->s_bdev->bd_inode->i_mapping->wb_err, &sbi->s_bdev_wb_err); - sb->s_bdev->bd_super = sb; EXT4_SB(sb)->s_mount_state |= EXT4_ORPHAN_FS; ext4_orphan_cleanup(sb, es); EXT4_SB(sb)->s_mount_state &= ~EXT4_ORPHAN_FS; diff --git a/fs/super.c b/fs/super.c index e781226e2880..7755cc2a3607 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1322,7 +1322,6 @@ int get_tree_bdev(struct fs_context *fc, } s->s_flags |= SB_ACTIVE; - bdev->bd_super = s; } BUG_ON(fc->root); @@ -1395,7 +1394,6 @@ struct dentry *mount_bdev(struct file_system_type *fs_type, } s->s_flags |= SB_ACTIVE; - bdev->bd_super = s; } return dget(s->s_root); @@ -1413,7 +1411,6 @@ void kill_block_super(struct super_block *sb) { struct block_device *bdev = sb->s_bdev; - bdev->bd_super = NULL; generic_shutdown_super(sb); sync_blockdev(bdev); blkdev_put(bdev, sb->s_type); diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 0bad62cca3d0..d5c5e59ddbd2 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -52,7 +52,6 @@ struct block_device { atomic_t bd_openers; spinlock_t bd_size_lock; /* for bd_inode->i_size updates */ struct inode * bd_inode; /* will die */ - struct super_block * bd_super; void * bd_claiming; void * bd_holder; const struct blk_holder_ops *bd_holder_ops; From dbbff489064d89391c4f0c7a73e77e61ce29fe96 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:33 -0700 Subject: [PATCH 08/47] xfs: reformat the xfs_fs_free prototype The xfs_fs_free prototype formatting is a weird mix of the classic XFS style and the Linux style. Fix it up to be consistent. Signed-off-by: Christoph Hellwig Reviewed-by: "Darrick J. Wong" Message-Id: <20230809220545.1308228-2-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_super.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 818510243130..79c1dd9435c2 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1934,7 +1934,8 @@ xfs_fs_reconfigure( return 0; } -static void xfs_fs_free( +static void +xfs_fs_free( struct fs_context *fc) { struct xfs_mount *mp = fc->s_fs_info; From 1aa2d074d4c777e2150382878d0a5611d829b380 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:34 -0700 Subject: [PATCH 09/47] xfs: remove a superfluous s_fs_info NULL check in xfs_fs_put_super ->put_super is only called when sb->s_root is set, and thus when fill_super succeeds. Thus drop the NULL check that can't happen in xfs_fs_put_super. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Reviewed-by: "Darrick J. Wong" Message-Id: <20230809220545.1308228-3-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_super.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 79c1dd9435c2..e621e0da87f3 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1133,10 +1133,6 @@ xfs_fs_put_super( { struct xfs_mount *mp = XFS_M(sb); - /* if ->fill_super failed, we have no mount to tear down */ - if (!sb->s_fs_info) - return; - xfs_notice(mp, "Unmounting Filesystem %pU", &mp->m_sb.sb_uuid); xfs_filestream_unmount(mp); xfs_unmountfs(mp); From 2a9311adb87c98599989b80405fe2c60cd4075dd Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:35 -0700 Subject: [PATCH 10/47] xfs: free the xfs_mount in ->kill_sb As a rule of thumb everything allocated to the fs_context and moved into the super_block should be freed by ->kill_sb so that the teardown handling doesn't need to be duplicated between the fill_super error path and put_super. Implement a XFS-specific kill_sb method to do that. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Reviewed-by: "Darrick J. Wong" Message-Id: <20230809220545.1308228-4-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_super.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index e621e0da87f3..31260fc09719 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1144,9 +1144,6 @@ xfs_fs_put_super( xfs_destroy_percpu_counters(mp); xfs_destroy_mount_workqueues(mp); xfs_close_devices(mp); - - sb->s_fs_info = NULL; - xfs_mount_free(mp); } static long @@ -1488,7 +1485,7 @@ xfs_fs_fill_super( error = xfs_fs_validate_params(mp); if (error) - goto out_free_names; + return error; sb_min_blocksize(sb, BBSIZE); sb->s_xattr = xfs_xattr_handlers; @@ -1515,7 +1512,7 @@ xfs_fs_fill_super( error = xfs_open_devices(mp); if (error) - goto out_free_names; + return error; error = xfs_init_mount_workqueues(mp); if (error) @@ -1735,9 +1732,6 @@ xfs_fs_fill_super( xfs_destroy_mount_workqueues(mp); out_close_devices: xfs_close_devices(mp); - out_free_names: - sb->s_fs_info = NULL; - xfs_mount_free(mp); return error; out_unmount: @@ -2000,12 +1994,20 @@ static int xfs_init_fs_context( return 0; } +static void +xfs_kill_sb( + struct super_block *sb) +{ + kill_block_super(sb); + xfs_mount_free(XFS_M(sb)); +} + static struct file_system_type xfs_fs_type = { .owner = THIS_MODULE, .name = "xfs", .init_fs_context = xfs_init_fs_context, .parameters = xfs_fs_parameters, - .kill_sb = kill_block_super, + .kill_sb = xfs_kill_sb, .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, }; MODULE_ALIAS_FS("xfs"); From d3ef7e94ee36adc8f0006d253a9ad45793b874cd Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:36 -0700 Subject: [PATCH 11/47] xfs: remove xfs_blkdev_put There isn't much use for this trivial wrapper, especially as the NULL check is only needed in a single call site. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Reviewed-by: "Darrick J. Wong" Message-Id: <20230809220545.1308228-5-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_super.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 31260fc09719..332e32d8ca5a 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -406,15 +406,6 @@ xfs_blkdev_get( return error; } -STATIC void -xfs_blkdev_put( - struct xfs_mount *mp, - struct block_device *bdev) -{ - if (bdev) - blkdev_put(bdev, mp); -} - STATIC void xfs_close_devices( struct xfs_mount *mp) @@ -423,13 +414,13 @@ xfs_close_devices( struct block_device *logdev = mp->m_logdev_targp->bt_bdev; xfs_free_buftarg(mp->m_logdev_targp); - xfs_blkdev_put(mp, logdev); + blkdev_put(logdev, mp); } if (mp->m_rtdev_targp) { struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev; xfs_free_buftarg(mp->m_rtdev_targp); - xfs_blkdev_put(mp, rtdev); + blkdev_put(rtdev, mp); } xfs_free_buftarg(mp->m_ddev_targp); } @@ -504,10 +495,11 @@ xfs_open_devices( out_free_ddev_targ: xfs_free_buftarg(mp->m_ddev_targp); out_close_rtdev: - xfs_blkdev_put(mp, rtdev); + if (rtdev) + blkdev_put(rtdev, mp); out_close_logdev: if (logdev && logdev != ddev) - xfs_blkdev_put(mp, logdev); + blkdev_put(logdev, mp); return error; } From 41233576e9a4515dc9b0bd1cbbb896b520a1f486 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:37 -0700 Subject: [PATCH 12/47] xfs: close the RT and log block devices in xfs_free_buftarg Closing the block devices logically belongs into xfs_free_buftarg, So instead of open coding it in the caller move it there and add a check for the s_bdev so that the main device isn't close as that's done by the VFS helper. Signed-off-by: Christoph Hellwig Reviewed-by: "Darrick J. Wong" Message-Id: <20230809220545.1308228-6-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_buf.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 15d1e5a7c2d3..65110df78e13 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1938,6 +1938,8 @@ void xfs_free_buftarg( struct xfs_buftarg *btp) { + struct block_device *bdev = btp->bt_bdev; + unregister_shrinker(&btp->bt_shrinker); ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0); percpu_counter_destroy(&btp->bt_io_count); @@ -1946,6 +1948,9 @@ xfs_free_buftarg( blkdev_issue_flush(btp->bt_bdev); invalidate_bdev(btp->bt_bdev); fs_put_dax(btp->bt_daxdev, btp->bt_mount); + /* the main block device is closed by kill_block_super */ + if (bdev != btp->bt_mount->m_super->s_bdev) + blkdev_put(bdev, btp->bt_mount); kmem_free(btp); } From 35a93b148b0363dca23c3db1cc9d48100eb8b276 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:38 -0700 Subject: [PATCH 13/47] xfs: close the external block devices in xfs_mount_free blkdev_put must not be called under sb->s_umount to avoid a lock order reversal with disk->open_mutex. Move closing the buftargs into ->kill_sb to archive that. Note that the flushing of the disk caches and block device mapping invalidated needs to stay in ->put_super as the main block device is closed in kill_block_super already. Signed-off-by: Christoph Hellwig Reviewed-by: "Darrick J. Wong" Message-Id: <20230809220545.1308228-7-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_buf.c | 2 -- fs/xfs/xfs_super.c | 36 ++++++++++++++++++++++-------------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 65110df78e13..f2e5a2c78dc4 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1945,8 +1945,6 @@ xfs_free_buftarg( percpu_counter_destroy(&btp->bt_io_count); list_lru_destroy(&btp->bt_lru); - blkdev_issue_flush(btp->bt_bdev); - invalidate_bdev(btp->bt_bdev); fs_put_dax(btp->bt_daxdev, btp->bt_mount); /* the main block device is closed by kill_block_super */ if (bdev != btp->bt_mount->m_super->s_bdev) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 332e32d8ca5a..18d34e802aac 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -407,22 +407,19 @@ xfs_blkdev_get( } STATIC void -xfs_close_devices( +xfs_shutdown_devices( struct xfs_mount *mp) { if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) { - struct block_device *logdev = mp->m_logdev_targp->bt_bdev; - - xfs_free_buftarg(mp->m_logdev_targp); - blkdev_put(logdev, mp); + blkdev_issue_flush(mp->m_logdev_targp->bt_bdev); + invalidate_bdev(mp->m_logdev_targp->bt_bdev); } if (mp->m_rtdev_targp) { - struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev; - - xfs_free_buftarg(mp->m_rtdev_targp); - blkdev_put(rtdev, mp); + blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev); + invalidate_bdev(mp->m_rtdev_targp->bt_bdev); } - xfs_free_buftarg(mp->m_ddev_targp); + blkdev_issue_flush(mp->m_ddev_targp->bt_bdev); + invalidate_bdev(mp->m_ddev_targp->bt_bdev); } /* @@ -750,6 +747,17 @@ static void xfs_mount_free( struct xfs_mount *mp) { + /* + * Free the buftargs here because blkdev_put needs to be called outside + * of sb->s_umount, which is held around the call to ->put_super. + */ + if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) + xfs_free_buftarg(mp->m_logdev_targp); + if (mp->m_rtdev_targp) + xfs_free_buftarg(mp->m_rtdev_targp); + if (mp->m_ddev_targp) + xfs_free_buftarg(mp->m_ddev_targp); + kfree(mp->m_rtname); kfree(mp->m_logname); kmem_free(mp); @@ -1135,7 +1143,7 @@ xfs_fs_put_super( xfs_inodegc_free_percpu(mp); xfs_destroy_percpu_counters(mp); xfs_destroy_mount_workqueues(mp); - xfs_close_devices(mp); + xfs_shutdown_devices(mp); } static long @@ -1508,7 +1516,7 @@ xfs_fs_fill_super( error = xfs_init_mount_workqueues(mp); if (error) - goto out_close_devices; + goto out_shutdown_devices; error = xfs_init_percpu_counters(mp); if (error) @@ -1722,8 +1730,8 @@ xfs_fs_fill_super( xfs_destroy_percpu_counters(mp); out_destroy_workqueues: xfs_destroy_mount_workqueues(mp); - out_close_devices: - xfs_close_devices(mp); + out_shutdown_devices: + xfs_shutdown_devices(mp); return error; out_unmount: From 1a0a5dad67b60250dce151c9533ccbecdfd822d4 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:39 -0700 Subject: [PATCH 14/47] xfs: document the invalidate_bdev call in invalidate_bdev Copy and paste the commit message from Darrick into a comment to explain the seemingly odd invalidate_bdev in xfs_shutdown_devices. Signed-off-by: Christoph Hellwig Reviewed-by: "Darrick J. Wong" Message-Id: <20230809220545.1308228-8-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_super.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 18d34e802aac..46d0cbe2b5f8 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -410,6 +410,32 @@ STATIC void xfs_shutdown_devices( struct xfs_mount *mp) { + /* + * Udev is triggered whenever anyone closes a block device or unmounts + * a file systemm on a block device. + * The default udev rules invoke blkid to read the fs super and create + * symlinks to the bdev under /dev/disk. For this, it uses buffered + * reads through the page cache. + * + * xfs_db also uses buffered reads to examine metadata. There is no + * coordination between xfs_db and udev, which means that they can run + * concurrently. Note there is no coordination between the kernel and + * blkid either. + * + * On a system with 64k pages, the page cache can cache the superblock + * and the root inode (and hence the root directory) with the same 64k + * page. If udev spawns blkid after the mkfs and the system is busy + * enough that it is still running when xfs_db starts up, they'll both + * read from the same page in the pagecache. + * + * The unmount writes updated inode metadata to disk directly. The XFS + * buffer cache does not use the bdev pagecache, so it needs to + * invalidate that pagecache on unmount. If the above scenario occurs, + * the pagecache no longer reflects what's on disk, xfs_db reads the + * stale metadata, and fails to find /a. Most of the time this succeeds + * because closing a bdev invalidates the page cache, but when processes + * race, everyone loses. + */ if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) { blkdev_issue_flush(mp->m_logdev_targp->bt_bdev); invalidate_bdev(mp->m_logdev_targp->bt_bdev); From 1489dffd51d7e4519855723b00516719d5aa3a6a Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:40 -0700 Subject: [PATCH 15/47] ext4: close the external journal device in ->kill_sb blkdev_put must not be called under sb->s_umount to avoid a lock order reversal with disk->open_mutex. Move closing the external journal device into ->kill_sb to archive that. Signed-off-by: Christoph Hellwig Message-Id: <20230809220545.1308228-9-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ext4/super.c | 50 ++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index b24f5b9f63e0..17c3e792d07e 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -93,6 +93,7 @@ static int ext4_get_tree(struct fs_context *fc); static int ext4_reconfigure(struct fs_context *fc); static void ext4_fc_free(struct fs_context *fc); static int ext4_init_fs_context(struct fs_context *fc); +static void ext4_kill_sb(struct super_block *sb); static const struct fs_parameter_spec ext4_param_specs[]; /* @@ -135,7 +136,7 @@ static struct file_system_type ext2_fs_type = { .name = "ext2", .init_fs_context = ext4_init_fs_context, .parameters = ext4_param_specs, - .kill_sb = kill_block_super, + .kill_sb = ext4_kill_sb, .fs_flags = FS_REQUIRES_DEV, }; MODULE_ALIAS_FS("ext2"); @@ -151,7 +152,7 @@ static struct file_system_type ext3_fs_type = { .name = "ext3", .init_fs_context = ext4_init_fs_context, .parameters = ext4_param_specs, - .kill_sb = kill_block_super, + .kill_sb = ext4_kill_sb, .fs_flags = FS_REQUIRES_DEV, }; MODULE_ALIAS_FS("ext3"); @@ -1125,25 +1126,6 @@ fail: return NULL; } -/* - * Release the journal device - */ -static void ext4_blkdev_remove(struct ext4_sb_info *sbi) -{ - struct block_device *bdev; - bdev = sbi->s_journal_bdev; - if (bdev) { - /* - * Invalidate the journal device's buffers. We don't want them - * floating about in memory - the physical journal device may - * hotswapped, and it breaks the `ro-after' testing code. - */ - invalidate_bdev(bdev); - blkdev_put(bdev, sbi->s_sb); - sbi->s_journal_bdev = NULL; - } -} - static inline struct inode *orphan_list_entry(struct list_head *l) { return &list_entry(l, struct ext4_inode_info, i_orphan)->vfs_inode; @@ -1339,8 +1321,13 @@ static void ext4_put_super(struct super_block *sb) sync_blockdev(sb->s_bdev); invalidate_bdev(sb->s_bdev); if (sbi->s_journal_bdev) { + /* + * Invalidate the journal device's buffers. We don't want them + * floating about in memory - the physical journal device may + * hotswapped, and it breaks the `ro-after' testing code. + */ sync_blockdev(sbi->s_journal_bdev); - ext4_blkdev_remove(sbi); + invalidate_bdev(sbi->s_journal_bdev); } ext4_xattr_destroy_cache(sbi->s_ea_inode_cache); @@ -5663,9 +5650,11 @@ failed_mount: kfree(get_qf_name(sb, sbi, i)); #endif fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy); - /* ext4_blkdev_remove() calls kill_bdev(), release bh before it. */ brelse(sbi->s_sbh); - ext4_blkdev_remove(sbi); + if (sbi->s_journal_bdev) { + invalidate_bdev(sbi->s_journal_bdev); + blkdev_put(sbi->s_journal_bdev, sb); + } out_fail: invalidate_bdev(sb->s_bdev); sb->s_fs_info = NULL; @@ -7272,12 +7261,23 @@ static inline int ext3_feature_set_ok(struct super_block *sb) return 1; } +static void ext4_kill_sb(struct super_block *sb) +{ + struct ext4_sb_info *sbi = EXT4_SB(sb); + struct block_device *journal_bdev = sbi ? sbi->s_journal_bdev : NULL; + + kill_block_super(sb); + + if (journal_bdev) + blkdev_put(journal_bdev, sb); +} + static struct file_system_type ext4_fs_type = { .owner = THIS_MODULE, .name = "ext4", .init_fs_context = ext4_init_fs_context, .parameters = ext4_param_specs, - .kill_sb = kill_block_super, + .kill_sb = ext4_kill_sb, .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, }; MODULE_ALIAS_FS("ext4"); From c934dc927e8efa9e0dc3cba8787912d6c23d272f Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:41 -0700 Subject: [PATCH 16/47] exfat: don't RCU-free the sbi There are no RCU critical sections for accessing any information in the sbi, so drop the call_rcu indirection for freeing the sbi. Signed-off-by: Christoph Hellwig Message-Id: <20230809220545.1308228-10-hch@lst.de> Signed-off-by: Christian Brauner --- fs/exfat/exfat_fs.h | 2 -- fs/exfat/super.c | 15 ++++----------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index 729ada9e26e8..f55498e5c23d 100644 --- a/fs/exfat/exfat_fs.h +++ b/fs/exfat/exfat_fs.h @@ -273,8 +273,6 @@ struct exfat_sb_info { spinlock_t inode_hash_lock; struct hlist_head inode_hashtable[EXFAT_HASH_SIZE]; - - struct rcu_head rcu; }; #define EXFAT_CACHE_VALID 0 diff --git a/fs/exfat/super.c b/fs/exfat/super.c index 8c32460e031e..3c6aec96d0dc 100644 --- a/fs/exfat/super.c +++ b/fs/exfat/super.c @@ -31,16 +31,6 @@ static void exfat_free_iocharset(struct exfat_sb_info *sbi) kfree(sbi->options.iocharset); } -static void exfat_delayed_free(struct rcu_head *p) -{ - struct exfat_sb_info *sbi = container_of(p, struct exfat_sb_info, rcu); - - unload_nls(sbi->nls_io); - exfat_free_iocharset(sbi); - exfat_free_upcase_table(sbi); - kfree(sbi); -} - static void exfat_put_super(struct super_block *sb) { struct exfat_sb_info *sbi = EXFAT_SB(sb); @@ -50,7 +40,10 @@ static void exfat_put_super(struct super_block *sb) brelse(sbi->boot_bh); mutex_unlock(&sbi->s_lock); - call_rcu(&sbi->rcu, exfat_delayed_free); + unload_nls(sbi->nls_io); + exfat_free_iocharset(sbi); + exfat_free_upcase_table(sbi); + kfree(sbi); } static int exfat_sync_fs(struct super_block *sb, int wait) From 4abc9a43d99ccab7bd71742b86d2f48d8be798c3 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:42 -0700 Subject: [PATCH 17/47] exfat: free the sbi and iocharset in ->kill_sb As a rule of thumb everything allocated to the fs_context and moved into the super_block should be freed by ->kill_sb so that the teardown handling doesn't need to be duplicated between the fill_super error path and put_super. Implement an exfat-specific kill_sb method to do that and share the code with the mount contex free helper for the mount error handling case. Signed-off-by: Christoph Hellwig Message-Id: <20230809220545.1308228-11-hch@lst.de> Signed-off-by: Christian Brauner --- fs/exfat/super.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/fs/exfat/super.c b/fs/exfat/super.c index 3c6aec96d0dc..85b04a4064af 100644 --- a/fs/exfat/super.c +++ b/fs/exfat/super.c @@ -41,9 +41,7 @@ static void exfat_put_super(struct super_block *sb) mutex_unlock(&sbi->s_lock); unload_nls(sbi->nls_io); - exfat_free_iocharset(sbi); exfat_free_upcase_table(sbi); - kfree(sbi); } static int exfat_sync_fs(struct super_block *sb, int wait) @@ -703,9 +701,6 @@ free_table: check_nls_io: unload_nls(sbi->nls_io); - exfat_free_iocharset(sbi); - sb->s_fs_info = NULL; - kfree(sbi); return err; } @@ -714,14 +709,18 @@ static int exfat_get_tree(struct fs_context *fc) return get_tree_bdev(fc, exfat_fill_super); } +static void exfat_free_sbi(struct exfat_sb_info *sbi) +{ + exfat_free_iocharset(sbi); + kfree(sbi); +} + static void exfat_free(struct fs_context *fc) { struct exfat_sb_info *sbi = fc->s_fs_info; - if (sbi) { - exfat_free_iocharset(sbi); - kfree(sbi); - } + if (sbi) + exfat_free_sbi(sbi); } static int exfat_reconfigure(struct fs_context *fc) @@ -766,12 +765,21 @@ static int exfat_init_fs_context(struct fs_context *fc) return 0; } +static void exfat_kill_sb(struct super_block *sb) +{ + struct exfat_sb_info *sbi = sb->s_fs_info; + + kill_block_super(sb); + if (sbi) + exfat_free_sbi(sbi); +} + static struct file_system_type exfat_fs_type = { .owner = THIS_MODULE, .name = "exfat", .init_fs_context = exfat_init_fs_context, .parameters = exfat_parameters, - .kill_sb = kill_block_super, + .kill_sb = exfat_kill_sb, .fs_flags = FS_REQUIRES_DEV, }; From 126dbf8a1b9cb2d764a314716a8ea98398a6fcb2 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:43 -0700 Subject: [PATCH 18/47] ntfs3: rename put_ntfs ntfs3_free_sbi put_ntfs is a rather unconventional name for a function that frees the sbi and associated resources. Give it a more descriptive name and drop the duplicate name in the top of the function comment. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Message-Id: <20230809220545.1308228-12-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ntfs3/super.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c index 1a02072b6b0e..bb985d3756d9 100644 --- a/fs/ntfs3/super.c +++ b/fs/ntfs3/super.c @@ -569,9 +569,9 @@ static void init_once(void *foo) } /* - * put_ntfs - Noinline to reduce binary size. + * Noinline to reduce binary size. */ -static noinline void put_ntfs(struct ntfs_sb_info *sbi) +static noinline void ntfs3_free_sbi(struct ntfs_sb_info *sbi) { kfree(sbi->new_rec); kvfree(ntfs_put_shared(sbi->upcase)); @@ -627,7 +627,7 @@ static void ntfs_put_super(struct super_block *sb) ntfs_set_state(sbi, NTFS_DIRTY_CLEAR); put_mount_options(sbi->options); - put_ntfs(sbi); + ntfs3_free_sbi(sbi); sb->s_fs_info = NULL; sync_blockdev(sb->s_bdev); @@ -1569,7 +1569,7 @@ out: * ntfs_fs_free will be called with fc->s_fs_info = NULL */ put_mount_options(sbi->options); - put_ntfs(sbi); + ntfs3_free_sbi(sbi); sb->s_fs_info = NULL; kfree(boot2); @@ -1659,7 +1659,7 @@ static void ntfs_fs_free(struct fs_context *fc) struct ntfs_sb_info *sbi = fc->s_fs_info; if (sbi) - put_ntfs(sbi); + ntfs3_free_sbi(sbi); if (opts) put_mount_options(opts); From 5f0fb2210bb34ecd3f7bfde0d8f0068b79b2e094 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:44 -0700 Subject: [PATCH 19/47] ntfs3: don't call sync_blockdev in ntfs_put_super kill_block_super will call sync_blockdev just a tad later already. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Message-Id: <20230809220545.1308228-13-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ntfs3/super.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c index bb985d3756d9..727138933a93 100644 --- a/fs/ntfs3/super.c +++ b/fs/ntfs3/super.c @@ -629,8 +629,6 @@ static void ntfs_put_super(struct super_block *sb) put_mount_options(sbi->options); ntfs3_free_sbi(sbi); sb->s_fs_info = NULL; - - sync_blockdev(sb->s_bdev); } static int ntfs_statfs(struct dentry *dentry, struct kstatfs *buf) From a4f64a300a299f884a1da55d99c97a87061201cd Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 9 Aug 2023 15:05:45 -0700 Subject: [PATCH 20/47] ntfs3: free the sbi in ->kill_sb As a rule of thumb everything allocated to the fs_context and moved into the super_block should be freed by ->kill_sb so that the teardown handling doesn't need to be duplicated between the fill_super error path and put_super. Implement an ntfs3-specific kill_sb method to do that. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Message-Id: <20230809220545.1308228-14-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ntfs3/super.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c index 727138933a93..5fffddea554f 100644 --- a/fs/ntfs3/super.c +++ b/fs/ntfs3/super.c @@ -625,10 +625,6 @@ static void ntfs_put_super(struct super_block *sb) /* Mark rw ntfs as clear, if possible. */ ntfs_set_state(sbi, NTFS_DIRTY_CLEAR); - - put_mount_options(sbi->options); - ntfs3_free_sbi(sbi); - sb->s_fs_info = NULL; } static int ntfs_statfs(struct dentry *dentry, struct kstatfs *buf) @@ -1562,15 +1558,7 @@ load_root: put_inode_out: iput(inode); out: - /* - * Free resources here. - * ntfs_fs_free will be called with fc->s_fs_info = NULL - */ - put_mount_options(sbi->options); - ntfs3_free_sbi(sbi); - sb->s_fs_info = NULL; kfree(boot2); - return err; } @@ -1726,13 +1714,24 @@ free_opts: return -ENOMEM; } +static void ntfs3_kill_sb(struct super_block *sb) +{ + struct ntfs_sb_info *sbi = sb->s_fs_info; + + kill_block_super(sb); + + if (sbi->options) + put_mount_options(sbi->options); + ntfs3_free_sbi(sbi); +} + // clang-format off static struct file_system_type ntfs_fs_type = { .owner = THIS_MODULE, .name = "ntfs3", .init_fs_context = ntfs_init_fs_context, .parameters = ntfs_fs_parameters, - .kill_sb = kill_block_super, + .kill_sb = ntfs3_kill_sb, .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, }; // clang-format on From aca740cecbe57b12bd9c1fc632092af5ebacda0c Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 24 Jul 2023 10:51:45 -0700 Subject: [PATCH 21/47] fs: open block device after superblock creation Currently get_tree_bdev and mount_bdev open the block device before committing to allocating a super block. That creates problems for restricting the number of writers to a device, and also leads to a unusual and not very helpful holder (the fs_type). Reorganize the super block code to first look whether the superblock for a particular device does already exist and open the block device only if it doesn't. [hch: port to before the bdev_handle changes, duplicate the bdev read-only check from blkdev_get_by_path, extend the fsfree_mutex coverage to protect against freezes, fix an open bdev leak when the bdev is frozen, use the bdev local variable more, rename the s variable to sb to be more descriptive] [brauner: remove references to mounts as they're mostly irrelevant] [brauner & hch: fold fixes for romfs and cramfs for syzbot+2faac0423fdc9692822b@syzkaller.appspotmail.com] Signed-off-by: Jan Kara Signed-off-by: Christoph Hellwig Message-Id: <20230724175145.201318-1-hch@lst.de> Signed-off-by: Christian Brauner --- fs/cramfs/inode.c | 8 +- fs/romfs/super.c | 10 ++- fs/super.c | 188 +++++++++++++++++++++++----------------------- 3 files changed, 107 insertions(+), 99 deletions(-) diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c index 27c6597aa1be..bc99d0fb8ad4 100644 --- a/fs/cramfs/inode.c +++ b/fs/cramfs/inode.c @@ -485,12 +485,16 @@ static void cramfs_kill_sb(struct super_block *sb) { struct cramfs_sb_info *sbi = CRAMFS_SB(sb); + generic_shutdown_super(sb); + if (IS_ENABLED(CONFIG_CRAMFS_MTD) && sb->s_mtd) { if (sbi && sbi->mtd_point_size) mtd_unpoint(sb->s_mtd, 0, sbi->mtd_point_size); - kill_mtd_super(sb); + put_mtd_device(sb->s_mtd); + sb->s_mtd = NULL; } else if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV) && sb->s_bdev) { - kill_block_super(sb); + sync_blockdev(sb->s_bdev); + blkdev_put(sb->s_bdev, sb->s_type); } kfree(sbi); } diff --git a/fs/romfs/super.c b/fs/romfs/super.c index c59b230d55b4..42d7a344472f 100644 --- a/fs/romfs/super.c +++ b/fs/romfs/super.c @@ -583,16 +583,18 @@ static int romfs_init_fs_context(struct fs_context *fc) */ static void romfs_kill_sb(struct super_block *sb) { + generic_shutdown_super(sb); + #ifdef CONFIG_ROMFS_ON_MTD if (sb->s_mtd) { - kill_mtd_super(sb); - return; + put_mtd_device(sb->s_mtd); + sb->s_mtd = NULL; } #endif #ifdef CONFIG_ROMFS_ON_BLOCK if (sb->s_bdev) { - kill_block_super(sb); - return; + sync_blockdev(sb->s_bdev); + blkdev_put(sb->s_bdev, sb->s_type); } #endif } diff --git a/fs/super.c b/fs/super.c index 7755cc2a3607..249558ecfd77 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1228,12 +1228,7 @@ static const struct blk_holder_ops fs_holder_ops = { static int set_bdev_super(struct super_block *s, void *data) { - s->s_bdev = data; - s->s_dev = s->s_bdev->bd_dev; - s->s_bdi = bdi_get(s->s_bdev->bd_disk->bdi); - - if (bdev_stable_writes(s->s_bdev)) - s->s_iflags |= SB_I_STABLE_WRITES; + s->s_dev = *(dev_t *)data; return 0; } @@ -1244,7 +1239,61 @@ static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc) static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc) { - return !(s->s_iflags & SB_I_RETIRED) && s->s_bdev == fc->sget_key; + return !(s->s_iflags & SB_I_RETIRED) && + s->s_dev == *(dev_t *)fc->sget_key; +} + +static int setup_bdev_super(struct super_block *sb, int sb_flags, + struct fs_context *fc) +{ + blk_mode_t mode = sb_open_mode(sb_flags); + struct block_device *bdev; + + bdev = blkdev_get_by_dev(sb->s_dev, mode, sb->s_type, &fs_holder_ops); + if (IS_ERR(bdev)) { + if (fc) + errorf(fc, "%s: Can't open blockdev", fc->source); + return PTR_ERR(bdev); + } + + /* + * This really should be in blkdev_get_by_dev, but right now can't due + * to legacy issues that require us to allow opening a block device node + * writable from userspace even for a read-only block device. + */ + if ((mode & BLK_OPEN_WRITE) && bdev_read_only(bdev)) { + blkdev_put(bdev, sb->s_type); + return -EACCES; + } + + /* + * Until SB_BORN flag is set, there can be no active superblock + * references and thus no filesystem freezing. get_active_super() will + * just loop waiting for SB_BORN so even freeze_bdev() cannot proceed. + * + * It is enough to check bdev was not frozen before we set s_bdev. + */ + mutex_lock(&bdev->bd_fsfreeze_mutex); + if (bdev->bd_fsfreeze_count > 0) { + mutex_unlock(&bdev->bd_fsfreeze_mutex); + if (fc) + warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev); + blkdev_put(bdev, sb->s_type); + return -EBUSY; + } + spin_lock(&sb_lock); + sb->s_bdev = bdev; + sb->s_bdi = bdi_get(bdev->bd_disk->bdi); + if (bdev_stable_writes(bdev)) + sb->s_iflags |= SB_I_STABLE_WRITES; + spin_unlock(&sb_lock); + mutex_unlock(&bdev->bd_fsfreeze_mutex); + + snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev); + shrinker_debugfs_rename(&sb->s_shrink, "sb-%s:%s", sb->s_type->name, + sb->s_id); + sb_set_blocksize(sb, block_size(bdev)); + return 0; } /** @@ -1256,71 +1305,48 @@ int get_tree_bdev(struct fs_context *fc, int (*fill_super)(struct super_block *, struct fs_context *)) { - struct block_device *bdev; struct super_block *s; int error = 0; + dev_t dev; if (!fc->source) return invalf(fc, "No source specified"); - bdev = blkdev_get_by_path(fc->source, sb_open_mode(fc->sb_flags), - fc->fs_type, &fs_holder_ops); - if (IS_ERR(bdev)) { - errorf(fc, "%s: Can't open blockdev", fc->source); - return PTR_ERR(bdev); - } - - /* Once the superblock is inserted into the list by sget_fc(), s_umount - * will protect the lockfs code from trying to start a snapshot while - * we are mounting - */ - mutex_lock(&bdev->bd_fsfreeze_mutex); - if (bdev->bd_fsfreeze_count > 0) { - mutex_unlock(&bdev->bd_fsfreeze_mutex); - warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev); - blkdev_put(bdev, fc->fs_type); - return -EBUSY; + error = lookup_bdev(fc->source, &dev); + if (error) { + errorf(fc, "%s: Can't lookup blockdev", fc->source); + return error; } fc->sb_flags |= SB_NOSEC; - fc->sget_key = bdev; + fc->sget_key = &dev; s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc); - mutex_unlock(&bdev->bd_fsfreeze_mutex); - if (IS_ERR(s)) { - blkdev_put(bdev, fc->fs_type); + if (IS_ERR(s)) return PTR_ERR(s); - } if (s->s_root) { /* Don't summarily change the RO/RW state. */ if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY) { - warnf(fc, "%pg: Can't mount, would change RO state", bdev); + warnf(fc, "%pg: Can't mount, would change RO state", s->s_bdev); deactivate_locked_super(s); - blkdev_put(bdev, fc->fs_type); return -EBUSY; } - + } else { /* - * s_umount nests inside open_mutex during - * __invalidate_device(). blkdev_put() acquires - * open_mutex and can't be called under s_umount. Drop - * s_umount temporarily. This is safe as we're - * holding an active reference. + * We drop s_umount here because we need to open the bdev and + * bdev->open_mutex ranks above s_umount (blkdev_put() -> + * __invalidate_device()). It is safe because we have active sb + * reference and SB_BORN is not set yet. */ up_write(&s->s_umount); - blkdev_put(bdev, fc->fs_type); + error = setup_bdev_super(s, fc->sb_flags, fc); down_write(&s->s_umount); - } else { - snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev); - shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", - fc->fs_type->name, s->s_id); - sb_set_blocksize(s, block_size(bdev)); - error = fill_super(s, fc); + if (!error) + error = fill_super(s, fc); if (error) { deactivate_locked_super(s); return error; } - s->s_flags |= SB_ACTIVE; } @@ -1332,78 +1358,52 @@ EXPORT_SYMBOL(get_tree_bdev); static int test_bdev_super(struct super_block *s, void *data) { - return !(s->s_iflags & SB_I_RETIRED) && (void *)s->s_bdev == data; + return !(s->s_iflags & SB_I_RETIRED) && s->s_dev == *(dev_t *)data; } struct dentry *mount_bdev(struct file_system_type *fs_type, int flags, const char *dev_name, void *data, int (*fill_super)(struct super_block *, void *, int)) { - struct block_device *bdev; struct super_block *s; - int error = 0; + int error; + dev_t dev; - bdev = blkdev_get_by_path(dev_name, sb_open_mode(flags), fs_type, - &fs_holder_ops); - if (IS_ERR(bdev)) - return ERR_CAST(bdev); + error = lookup_bdev(dev_name, &dev); + if (error) + return ERR_PTR(error); - /* - * once the super is inserted into the list by sget, s_umount - * will protect the lockfs code from trying to start a snapshot - * while we are mounting - */ - mutex_lock(&bdev->bd_fsfreeze_mutex); - if (bdev->bd_fsfreeze_count > 0) { - mutex_unlock(&bdev->bd_fsfreeze_mutex); - error = -EBUSY; - goto error_bdev; - } - s = sget(fs_type, test_bdev_super, set_bdev_super, flags | SB_NOSEC, - bdev); - mutex_unlock(&bdev->bd_fsfreeze_mutex); + flags |= SB_NOSEC; + s = sget(fs_type, test_bdev_super, set_bdev_super, flags, &dev); if (IS_ERR(s)) - goto error_s; + return ERR_CAST(s); if (s->s_root) { if ((flags ^ s->s_flags) & SB_RDONLY) { deactivate_locked_super(s); - error = -EBUSY; - goto error_bdev; + return ERR_PTR(-EBUSY); } - + } else { /* - * s_umount nests inside open_mutex during - * __invalidate_device(). blkdev_put() acquires - * open_mutex and can't be called under s_umount. Drop - * s_umount temporarily. This is safe as we're - * holding an active reference. + * We drop s_umount here because we need to open the bdev and + * bdev->open_mutex ranks above s_umount (blkdev_put() -> + * __invalidate_device()). It is safe because we have active sb + * reference and SB_BORN is not set yet. */ up_write(&s->s_umount); - blkdev_put(bdev, fs_type); + error = setup_bdev_super(s, flags, NULL); down_write(&s->s_umount); - } else { - snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev); - shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", - fs_type->name, s->s_id); - sb_set_blocksize(s, block_size(bdev)); - error = fill_super(s, data, flags & SB_SILENT ? 1 : 0); + if (!error) + error = fill_super(s, data, flags & SB_SILENT ? 1 : 0); if (error) { deactivate_locked_super(s); - goto error; + return ERR_PTR(error); } s->s_flags |= SB_ACTIVE; } return dget(s->s_root); - -error_s: - error = PTR_ERR(s); -error_bdev: - blkdev_put(bdev, fs_type); -error: - return ERR_PTR(error); } EXPORT_SYMBOL(mount_bdev); @@ -1412,8 +1412,10 @@ void kill_block_super(struct super_block *sb) struct block_device *bdev = sb->s_bdev; generic_shutdown_super(sb); - sync_blockdev(bdev); - blkdev_put(bdev, sb->s_type); + if (bdev) { + sync_blockdev(bdev); + blkdev_put(bdev, sb->s_type); + } } EXPORT_SYMBOL(kill_block_super); From cf6da236c27a73ab91b657232cd3841aab27c37a Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:20 +0200 Subject: [PATCH 22/47] fs: export setup_bdev_super We'll want to use setup_bdev_super instead of duplicating it in nilfs2. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Message-Id: <20230802154131.2221419-2-hch@lst.de> Signed-off-by: Christian Brauner --- fs/super.c | 3 ++- include/linux/fs_context.h | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/super.c b/fs/super.c index 249558ecfd77..a366bc65886e 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1243,7 +1243,7 @@ static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc) s->s_dev == *(dev_t *)fc->sget_key; } -static int setup_bdev_super(struct super_block *sb, int sb_flags, +int setup_bdev_super(struct super_block *sb, int sb_flags, struct fs_context *fc) { blk_mode_t mode = sb_open_mode(sb_flags); @@ -1295,6 +1295,7 @@ static int setup_bdev_super(struct super_block *sb, int sb_flags, sb_set_blocksize(sb, block_size(bdev)); return 0; } +EXPORT_SYMBOL_GPL(setup_bdev_super); /** * get_tree_bdev - Get a superblock based on a single block device diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index ff6341e09925..58ef8433a94b 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -158,6 +158,8 @@ extern int get_tree_keyed(struct fs_context *fc, struct fs_context *fc), void *key); +int setup_bdev_super(struct super_block *sb, int sb_flags, + struct fs_context *fc); extern int get_tree_bdev(struct fs_context *fc, int (*fill_super)(struct super_block *sb, struct fs_context *fc)); From c1e012ea9e835f81ac6062e8aa9e72940a671b95 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:21 +0200 Subject: [PATCH 23/47] nilfs2: use setup_bdev_super to de-duplicate the mount code Use the generic setup_bdev_super helper to open the main block device and do various bits of superblock setup instead of duplicating the logic. This includes moving to the new scheme implemented in common code that only opens the block device after the superblock has allocated. It does not yet convert nilfs2 to the new mount API, but doing so will become a bit simpler after this first step. Signed-off-by: Christoph Hellwig Acked-by: Ryusuke Konishi Message-Id: <20230802154131.2221419-3-hch@lst.de> Signed-off-by: Christian Brauner --- fs/nilfs2/super.c | 81 ++++++++++++++++++----------------------------- 1 file changed, 30 insertions(+), 51 deletions(-) diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c index 0ef8c71bde8e..a5d1fa4e7552 100644 --- a/fs/nilfs2/super.c +++ b/fs/nilfs2/super.c @@ -35,6 +35,7 @@ #include #include #include +#include #include "nilfs.h" #include "export.h" #include "mdt.h" @@ -1216,7 +1217,6 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data) } struct nilfs_super_data { - struct block_device *bdev; __u64 cno; int flags; }; @@ -1283,64 +1283,49 @@ static int nilfs_identify(char *data, struct nilfs_super_data *sd) static int nilfs_set_bdev_super(struct super_block *s, void *data) { - s->s_bdev = data; - s->s_dev = s->s_bdev->bd_dev; + s->s_dev = *(dev_t *)data; return 0; } static int nilfs_test_bdev_super(struct super_block *s, void *data) { - return (void *)s->s_bdev == data; + return !(s->s_iflags & SB_I_RETIRED) && s->s_dev == *(dev_t *)data; } static struct dentry * nilfs_mount(struct file_system_type *fs_type, int flags, const char *dev_name, void *data) { - struct nilfs_super_data sd; + struct nilfs_super_data sd = { .flags = flags }; struct super_block *s; - struct dentry *root_dentry; - int err, s_new = false; + dev_t dev; + int err; - sd.bdev = blkdev_get_by_path(dev_name, sb_open_mode(flags), fs_type, - NULL); - if (IS_ERR(sd.bdev)) - return ERR_CAST(sd.bdev); + if (nilfs_identify(data, &sd)) + return ERR_PTR(-EINVAL); - sd.cno = 0; - sd.flags = flags; - if (nilfs_identify((char *)data, &sd)) { - err = -EINVAL; - goto failed; - } + err = lookup_bdev(dev_name, &dev); + if (err) + return ERR_PTR(err); - /* - * once the super is inserted into the list by sget, s_umount - * will protect the lockfs code from trying to start a snapshot - * while we are mounting - */ - mutex_lock(&sd.bdev->bd_fsfreeze_mutex); - if (sd.bdev->bd_fsfreeze_count > 0) { - mutex_unlock(&sd.bdev->bd_fsfreeze_mutex); - err = -EBUSY; - goto failed; - } s = sget(fs_type, nilfs_test_bdev_super, nilfs_set_bdev_super, flags, - sd.bdev); - mutex_unlock(&sd.bdev->bd_fsfreeze_mutex); - if (IS_ERR(s)) { - err = PTR_ERR(s); - goto failed; - } + &dev); + if (IS_ERR(s)) + return ERR_CAST(s); if (!s->s_root) { - s_new = true; - - /* New superblock instance created */ - snprintf(s->s_id, sizeof(s->s_id), "%pg", sd.bdev); - sb_set_blocksize(s, block_size(sd.bdev)); - - err = nilfs_fill_super(s, data, flags & SB_SILENT ? 1 : 0); + /* + * We drop s_umount here because we need to open the bdev and + * bdev->open_mutex ranks above s_umount (blkdev_put() -> + * __invalidate_device()). It is safe because we have active sb + * reference and SB_BORN is not set yet. + */ + up_write(&s->s_umount); + err = setup_bdev_super(s, flags, NULL); + down_write(&s->s_umount); + if (!err) + err = nilfs_fill_super(s, data, + flags & SB_SILENT ? 1 : 0); if (err) goto failed_super; @@ -1366,24 +1351,18 @@ nilfs_mount(struct file_system_type *fs_type, int flags, } if (sd.cno) { + struct dentry *root_dentry; + err = nilfs_attach_snapshot(s, sd.cno, &root_dentry); if (err) goto failed_super; - } else { - root_dentry = dget(s->s_root); + return root_dentry; } - if (!s_new) - blkdev_put(sd.bdev, fs_type); - - return root_dentry; + return dget(s->s_root); failed_super: deactivate_locked_super(s); - - failed: - if (!s_new) - blkdev_put(sd.bdev, fs_type); return ERR_PTR(err); } From 4b41828be268544286794c18200e83861de3554e Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:24 +0200 Subject: [PATCH 24/47] ext4: make the IS_EXT2_SB/IS_EXT3_SB checks more robust Check for sb->s_type which is the right place to look at the file system type, not the holder, which is just an implementation detail in the VFS helpers. Signed-off-by: Christoph Hellwig Acked-by: Theodore Ts'o Reviewed-by: Jan Kara Reviewed-by: Christian Brauner Message-Id: <20230802154131.2221419-6-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ext4/super.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 17c3e792d07e..fa8888f0fed9 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -141,7 +141,7 @@ static struct file_system_type ext2_fs_type = { }; MODULE_ALIAS_FS("ext2"); MODULE_ALIAS("ext2"); -#define IS_EXT2_SB(sb) ((sb)->s_bdev->bd_holder == &ext2_fs_type) +#define IS_EXT2_SB(sb) ((sb)->s_type == &ext2_fs_type) #else #define IS_EXT2_SB(sb) (0) #endif @@ -157,7 +157,7 @@ static struct file_system_type ext3_fs_type = { }; MODULE_ALIAS_FS("ext3"); MODULE_ALIAS("ext3"); -#define IS_EXT3_SB(sb) ((sb)->s_bdev->bd_holder == &ext3_fs_type) +#define IS_EXT3_SB(sb) ((sb)->s_type == &ext3_fs_type) static inline void __ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags, From 2ea6f68932f73a6a9d82160d3ad0a49a5a6bb183 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:25 +0200 Subject: [PATCH 25/47] fs: use the super_block as holder when mounting file systems The file system type is not a very useful holder as it doesn't allow us to go back to the actual file system instance. Pass the super_block instead which is useful when passed back to the file system driver. Signed-off-by: Christoph Hellwig Reviewed-by: Jan Kara Reviewed-by: Christian Brauner Message-Id: <20230802154131.2221419-7-hch@lst.de> Signed-off-by: Christian Brauner --- fs/cramfs/inode.c | 2 +- fs/f2fs/super.c | 7 +++---- fs/romfs/super.c | 2 +- fs/super.c | 8 ++++---- fs/xfs/xfs_buf.c | 2 +- 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c index bc99d0fb8ad4..569f88dcb2f1 100644 --- a/fs/cramfs/inode.c +++ b/fs/cramfs/inode.c @@ -494,7 +494,7 @@ static void cramfs_kill_sb(struct super_block *sb) sb->s_mtd = NULL; } else if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV) && sb->s_bdev) { sync_blockdev(sb->s_bdev); - blkdev_put(sb->s_bdev, sb->s_type); + blkdev_put(sb->s_bdev, sb); } kfree(sbi); } diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index ca31163da00a..05c90fdb7a6c 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1561,7 +1561,7 @@ static void destroy_device_list(struct f2fs_sb_info *sbi) int i; for (i = 0; i < sbi->s_ndevs; i++) { - blkdev_put(FDEV(i).bdev, sbi->sb->s_type); + blkdev_put(FDEV(i).bdev, sbi->sb); #ifdef CONFIG_BLK_DEV_ZONED kvfree(FDEV(i).blkz_seq); #endif @@ -4198,7 +4198,7 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi) /* Single zoned block device mount */ FDEV(0).bdev = blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev, mode, - sbi->sb->s_type, NULL); + sbi->sb, NULL); } else { /* Multi-device mount */ memcpy(FDEV(i).path, RDEV(i).path, MAX_PATH_LEN); @@ -4217,8 +4217,7 @@ static int f2fs_scan_devices(struct f2fs_sb_info *sbi) sbi->log_blocks_per_seg) - 1; } FDEV(i).bdev = blkdev_get_by_path(FDEV(i).path, mode, - sbi->sb->s_type, - NULL); + sbi->sb, NULL); } if (IS_ERR(FDEV(i).bdev)) return PTR_ERR(FDEV(i).bdev); diff --git a/fs/romfs/super.c b/fs/romfs/super.c index 42d7a344472f..22cdb9a86a57 100644 --- a/fs/romfs/super.c +++ b/fs/romfs/super.c @@ -594,7 +594,7 @@ static void romfs_kill_sb(struct super_block *sb) #ifdef CONFIG_ROMFS_ON_BLOCK if (sb->s_bdev) { sync_blockdev(sb->s_bdev); - blkdev_put(sb->s_bdev, sb->s_type); + blkdev_put(sb->s_bdev, sb); } #endif } diff --git a/fs/super.c b/fs/super.c index a366bc65886e..3c7e4633efeb 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1249,7 +1249,7 @@ int setup_bdev_super(struct super_block *sb, int sb_flags, blk_mode_t mode = sb_open_mode(sb_flags); struct block_device *bdev; - bdev = blkdev_get_by_dev(sb->s_dev, mode, sb->s_type, &fs_holder_ops); + bdev = blkdev_get_by_dev(sb->s_dev, mode, sb, &fs_holder_ops); if (IS_ERR(bdev)) { if (fc) errorf(fc, "%s: Can't open blockdev", fc->source); @@ -1262,7 +1262,7 @@ int setup_bdev_super(struct super_block *sb, int sb_flags, * writable from userspace even for a read-only block device. */ if ((mode & BLK_OPEN_WRITE) && bdev_read_only(bdev)) { - blkdev_put(bdev, sb->s_type); + blkdev_put(bdev, sb); return -EACCES; } @@ -1278,7 +1278,7 @@ int setup_bdev_super(struct super_block *sb, int sb_flags, mutex_unlock(&bdev->bd_fsfreeze_mutex); if (fc) warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev); - blkdev_put(bdev, sb->s_type); + blkdev_put(bdev, sb); return -EBUSY; } spin_lock(&sb_lock); @@ -1415,7 +1415,7 @@ void kill_block_super(struct super_block *sb) generic_shutdown_super(sb); if (bdev) { sync_blockdev(bdev); - blkdev_put(bdev, sb->s_type); + blkdev_put(bdev, sb); } } diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index f2e5a2c78dc4..3b903f6bce98 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1948,7 +1948,7 @@ xfs_free_buftarg( fs_put_dax(btp->bt_daxdev, btp->bt_mount); /* the main block device is closed by kill_block_super */ if (bdev != btp->bt_mount->m_super->s_bdev) - blkdev_put(bdev, btp->bt_mount); + blkdev_put(bdev, btp->bt_mount->m_super); kmem_free(btp); } From 9c09a7cf6220a11dac0f4845b7e8925706cdc458 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:26 +0200 Subject: [PATCH 26/47] fs: stop using get_super in fs_mark_dead fs_mark_dead currently uses get_super to find the superblock for the block device that is going away. This means it is limited to the main device stored in sb->s_dev, leading to a lot of code duplication for file systems that can use multiple block devices. Now that the holder for all block devices used by file systems is set to the super_block, we can instead look at that holder and then check if the file system is born and active, so do that instead. Signed-off-by: Christoph Hellwig Reviewed-by: Jan Kara Reviewed-by: Christian Brauner Message-Id: <20230802154131.2221419-8-hch@lst.de> Signed-off-by: Christian Brauner --- fs/super.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/fs/super.c b/fs/super.c index 3c7e4633efeb..9cf7fc67727b 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1209,17 +1209,39 @@ int get_tree_keyed(struct fs_context *fc, EXPORT_SYMBOL(get_tree_keyed); #ifdef CONFIG_BLOCK +/* + * Lock a super block that the callers holds a reference to. + * + * The caller needs to ensure that the super_block isn't being freed while + * calling this function, e.g. by holding a lock over the call to this function + * and the place that clears the pointer to the superblock used by this function + * before freeing the superblock. + */ +static bool lock_active_super(struct super_block *sb) +{ + down_read(&sb->s_umount); + if (!sb->s_root || + (sb->s_flags & (SB_ACTIVE | SB_BORN)) != (SB_ACTIVE | SB_BORN)) { + up_read(&sb->s_umount); + return false; + } + return true; +} + static void fs_mark_dead(struct block_device *bdev) { - struct super_block *sb; + struct super_block *sb = bdev->bd_holder; - sb = get_super(bdev); - if (!sb) + /* bd_holder_lock ensures that the sb isn't freed */ + lockdep_assert_held(&bdev->bd_holder_lock); + + if (!lock_active_super(sb)) return; if (sb->s_op->shutdown) sb->s_op->shutdown(sb); - drop_super(sb); + + up_read(&sb->s_umount); } static const struct blk_holder_ops fs_holder_ops = { From 7ecd0b6f510005e120f5bc198bacb5951814cf36 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:27 +0200 Subject: [PATCH 27/47] fs: export fs_holder_ops Export fs_holder_ops so that file systems that open additional block devices can use it as well. Signed-off-by: Christoph Hellwig Reviewed-by: Jan Kara Reviewed-by: Christian Brauner Message-Id: <20230802154131.2221419-9-hch@lst.de> Signed-off-by: Christian Brauner --- fs/super.c | 3 ++- include/linux/blkdev.h | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/super.c b/fs/super.c index 9cf7fc67727b..f72a1112a31b 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1244,9 +1244,10 @@ static void fs_mark_dead(struct block_device *bdev) up_read(&sb->s_umount); } -static const struct blk_holder_ops fs_holder_ops = { +const struct blk_holder_ops fs_holder_ops = { .mark_dead = fs_mark_dead, }; +EXPORT_SYMBOL_GPL(fs_holder_ops); static int set_bdev_super(struct super_block *s, void *data) { diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index ed44a997f629..83262702eea7 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1464,6 +1464,8 @@ struct blk_holder_ops { void (*mark_dead)(struct block_device *bdev); }; +extern const struct blk_holder_ops fs_holder_ops; + /* * Return the correct open flags for blkdev_get_by_* for super block flags * as stored in sb->s_flags. From 6f5fc7de98857c75dad05e9f3fe2e6f358d4583e Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:28 +0200 Subject: [PATCH 28/47] ext4: drop s_umount over opening the log device Just like get_tree_bdev needs to drop s_umount when opening the main device, we need to do the same for the ext4 log device to avoid a potential lock order reversal with s_unmount for the mark_dead path. It might be preferable to just drop s_umount over ->fill_super entirely, but that will require a fairly massive audit first, so we'll do the easy version here first. Signed-off-by: Christoph Hellwig Acked-by: Theodore Ts'o Reviewed-by: Jan Kara Reviewed-by: Christian Brauner Message-Id: <20230802154131.2221419-10-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ext4/super.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index fa8888f0fed9..47e3272e0c5b 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -5842,7 +5842,10 @@ static journal_t *ext4_get_dev_journal(struct super_block *sb, if (WARN_ON_ONCE(!ext4_has_feature_journal(sb))) return NULL; + /* see get_tree_bdev why this is needed and safe */ + up_write(&sb->s_umount); bdev = ext4_blkdev_get(j_dev, sb); + down_write(&sb->s_umount); if (bdev == NULL) return NULL; From 8bed1783751fc8d0a5a263dcf87838a2fa570953 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:29 +0200 Subject: [PATCH 29/47] ext4: use fs_holder_ops for the log device Use the generic fs_holder_ops to shut down the file system when the log device goes away instead of duplicating the logic. Signed-off-by: Christoph Hellwig Reviewed-by: Jan Kara Message-Id: <20230802154131.2221419-11-hch@lst.de> Signed-off-by: Christian Brauner --- fs/ext4/super.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 47e3272e0c5b..60d2815a0b7e 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1097,15 +1097,6 @@ void ext4_update_dynamic_rev(struct super_block *sb) */ } -static void ext4_bdev_mark_dead(struct block_device *bdev) -{ - ext4_force_shutdown(bdev->bd_holder, EXT4_GOING_FLAGS_NOLOGFLUSH); -} - -static const struct blk_holder_ops ext4_holder_ops = { - .mark_dead = ext4_bdev_mark_dead, -}; - /* * Open the external journal device */ @@ -1114,7 +1105,7 @@ static struct block_device *ext4_blkdev_get(dev_t dev, struct super_block *sb) struct block_device *bdev; bdev = blkdev_get_by_dev(dev, BLK_OPEN_READ | BLK_OPEN_WRITE, sb, - &ext4_holder_ops); + &fs_holder_ops); if (IS_ERR(bdev)) goto fail; return bdev; From 8d945b595ed07db13fef1f3311ad456c97941930 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:30 +0200 Subject: [PATCH 30/47] xfs: drop s_umount over opening the log and RT devices Just like get_tree_bdev needs to drop s_umount when opening the main device, we need to do the same for the xfs log and RT devices to avoid a potential lock order reversal with s_unmount for the mark_dead path. It might be preferable to just drop s_umount over ->fill_super entirely, but that will require a fairly massive audit first, so we'll do the easy version here first. Signed-off-by: Christoph Hellwig Reviewed-by: "Darrick J. Wong" Message-Id: <20230802154131.2221419-12-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_super.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 46d0cbe2b5f8..97ef41acb03d 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -462,17 +462,24 @@ STATIC int xfs_open_devices( struct xfs_mount *mp) { - struct block_device *ddev = mp->m_super->s_bdev; + struct super_block *sb = mp->m_super; + struct block_device *ddev = sb->s_bdev; struct block_device *logdev = NULL, *rtdev = NULL; int error; + /* + * blkdev_put() can't be called under s_umount, see the comment + * in get_tree_bdev() for more details + */ + up_write(&sb->s_umount); + /* * Open real time and log devices - order is important. */ if (mp->m_logname) { error = xfs_blkdev_get(mp, mp->m_logname, &logdev); if (error) - return error; + goto out_relock; } if (mp->m_rtname) { @@ -510,7 +517,10 @@ xfs_open_devices( mp->m_logdev_targp = mp->m_ddev_targp; } - return 0; + error = 0; +out_relock: + down_write(&sb->s_umount); + return error; out_free_rtdev_targ: if (mp->m_rtdev_targp) @@ -523,7 +533,7 @@ xfs_open_devices( out_close_logdev: if (logdev && logdev != ddev) blkdev_put(logdev, mp); - return error; + goto out_relock; } /* From 8ffa54e3370c5a8b9538dbe4077fc9c4b5a08f45 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 2 Aug 2023 17:41:31 +0200 Subject: [PATCH 31/47] xfs use fs_holder_ops for the log and RT devices Use the generic fs_holder_ops to shut down the file system when the log or RT device goes away instead of duplicating the logic. Signed-off-by: Christoph Hellwig Reviewed-by: "Darrick J. Wong" Message-Id: <20230802154131.2221419-13-hch@lst.de> Signed-off-by: Christian Brauner --- fs/xfs/xfs_super.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 97ef41acb03d..8fee15292499 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -377,17 +377,6 @@ disable_dax: return 0; } -static void -xfs_bdev_mark_dead( - struct block_device *bdev) -{ - xfs_force_shutdown(bdev->bd_holder, SHUTDOWN_DEVICE_REMOVED); -} - -static const struct blk_holder_ops xfs_holder_ops = { - .mark_dead = xfs_bdev_mark_dead, -}; - STATIC int xfs_blkdev_get( xfs_mount_t *mp, @@ -396,8 +385,8 @@ xfs_blkdev_get( { int error = 0; - *bdevp = blkdev_get_by_path(name, BLK_OPEN_READ | BLK_OPEN_WRITE, mp, - &xfs_holder_ops); + *bdevp = blkdev_get_by_path(name, BLK_OPEN_READ | BLK_OPEN_WRITE, + mp->m_super, &fs_holder_ops); if (IS_ERR(*bdevp)) { error = PTR_ERR(*bdevp); xfs_warn(mp, "Invalid device [%s], error=%d", name, error); @@ -529,10 +518,10 @@ out_relock: xfs_free_buftarg(mp->m_ddev_targp); out_close_rtdev: if (rtdev) - blkdev_put(rtdev, mp); + blkdev_put(rtdev, sb); out_close_logdev: if (logdev && logdev != ddev) - blkdev_put(logdev, mp); + blkdev_put(logdev, sb); goto out_relock; } From 0c1c9a27ce909e3988f8c6407e26a22a7e1cd276 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:18 +0200 Subject: [PATCH 32/47] nbd: call blk_mark_disk_dead in nbd_clear_sock_ioctl nbd_clear_sock_ioctl kills the socket and with that the block device. Instead of just invalidating file system buffers, mark the device as dead, which will also invalidate the buffers as part of the proper shutdown sequence. This also includes invalidating partitions if there are any. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-8-hch@lst.de> Signed-off-by: Christian Brauner --- drivers/block/nbd.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 8576d696c7a2..42e0159bb258 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -1434,12 +1434,10 @@ static int nbd_start_device_ioctl(struct nbd_device *nbd) return ret; } -static void nbd_clear_sock_ioctl(struct nbd_device *nbd, - struct block_device *bdev) +static void nbd_clear_sock_ioctl(struct nbd_device *nbd) { + blk_mark_disk_dead(nbd->disk); nbd_clear_sock(nbd); - __invalidate_device(bdev, true); - nbd_bdev_reset(nbd); if (test_and_clear_bit(NBD_RT_HAS_CONFIG_REF, &nbd->config->runtime_flags)) nbd_config_put(nbd); @@ -1465,7 +1463,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, case NBD_DISCONNECT: return nbd_disconnect(nbd); case NBD_CLEAR_SOCK: - nbd_clear_sock_ioctl(nbd, bdev); + nbd_clear_sock_ioctl(nbd); return 0; case NBD_SET_SOCK: return nbd_add_socket(nbd, arg, false); From ab6860f62bfe329e11e5b5b7295b673c9c3a62d0 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:19 +0200 Subject: [PATCH 33/47] block: simplify the disk_force_media_change interface Hard code the events to DISK_EVENT_MEDIA_CHANGE as that is the only useful use case, and drop the superfluous return value. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-9-hch@lst.de> Signed-off-by: Christian Brauner --- block/disk-events.c | 15 ++++----------- drivers/block/loop.c | 6 +++--- include/linux/blkdev.h | 2 +- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/block/disk-events.c b/block/disk-events.c index 0cfac464e6d1..6189b819b235 100644 --- a/block/disk-events.c +++ b/block/disk-events.c @@ -294,25 +294,18 @@ EXPORT_SYMBOL(disk_check_media_change); * @disk: the disk which will raise the event * @events: the events to raise * - * Generate uevents for the disk. If DISK_EVENT_MEDIA_CHANGE is present, - * attempt to free all dentries and inodes and invalidates all block + * Should be called when the media changes for @disk. Generates a uevent + * and attempts to free all dentries and inodes and invalidates all block * device page cache entries in that case. - * - * Returns %true if DISK_EVENT_MEDIA_CHANGE was raised, or %false if not. */ -bool disk_force_media_change(struct gendisk *disk, unsigned int events) +void disk_force_media_change(struct gendisk *disk) { - disk_event_uevent(disk, events); - - if (!(events & DISK_EVENT_MEDIA_CHANGE)) - return false; - + disk_event_uevent(disk, DISK_EVENT_MEDIA_CHANGE); inc_diskseq(disk); if (__invalidate_device(disk->part0, true)) pr_warn("VFS: busy inodes on changed media %s\n", disk->disk_name); set_bit(GD_NEED_PART_SCAN, &disk->state); - return true; } EXPORT_SYMBOL_GPL(disk_force_media_change); diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 37511d2b2caf..705a0effa7d8 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -603,7 +603,7 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, goto out_err; /* and ... switch */ - disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE); + disk_force_media_change(lo->lo_disk); blk_mq_freeze_queue(lo->lo_queue); mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask); lo->lo_backing_file = file; @@ -1067,7 +1067,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode, /* suppress uevents while reconfiguring the device */ dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1); - disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE); + disk_force_media_change(lo->lo_disk); set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0); lo->use_dio = lo->lo_flags & LO_FLAGS_DIRECT_IO; @@ -1171,7 +1171,7 @@ static void __loop_clr_fd(struct loop_device *lo, bool release) if (!release) blk_mq_unfreeze_queue(lo->lo_queue); - disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE); + disk_force_media_change(lo->lo_disk); if (lo->lo_flags & LO_FLAGS_PARTSCAN) { int err; diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 83262702eea7..c8eab6effc22 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -750,7 +750,7 @@ static inline int bdev_read_only(struct block_device *bdev) } bool set_capacity_and_notify(struct gendisk *disk, sector_t size); -bool disk_force_media_change(struct gendisk *disk, unsigned int events); +void disk_force_media_change(struct gendisk *disk); void add_disk_randomness(struct gendisk *disk) __latent_entropy; void rand_initialize_disk(struct gendisk *disk); From a47145f2361976c83bb7d3198e7ff0e13a29fb7e Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:20 +0200 Subject: [PATCH 34/47] floppy: call disk_force_media_change when changing the format While changing the format of a floppy isn't strictly speaking a media change, the effects are the same in that the content of the media changes and the diskseq should be increased and uevent should be sent. Switch from calling __invalidate_device to disk_force_media_change to do so. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-10-hch@lst.de> Signed-off-by: Christian Brauner --- drivers/block/floppy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c index 2db9b186b977..ea4eb88a2e45 100644 --- a/drivers/block/floppy.c +++ b/drivers/block/floppy.c @@ -3255,7 +3255,7 @@ static int set_geometry(unsigned int cmd, struct floppy_struct *g, if (!disk || ITYPE(drive_state[cnt].fd_device) != type) continue; - __invalidate_device(disk->part0, true); + disk_force_media_change(disk); } mutex_unlock(&open_lock); } else { From 2c0326c587965a40c4013361b1f4d0e5cca5194e Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:21 +0200 Subject: [PATCH 35/47] amiflop: don't call fsync_bdev in FDFMTBEG FDFMTBEG is used by fdformat to calibrate before formatting a disk. Neither the atari nor PC floppy driver sync data, which also seems a bit pointless for a disk hat is about to get formatted. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-11-hch@lst.de> Signed-off-by: Christian Brauner --- drivers/block/amiflop.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c index e460c9799d9f..2b98114a9fe0 100644 --- a/drivers/block/amiflop.c +++ b/drivers/block/amiflop.c @@ -1547,7 +1547,6 @@ static int fd_locked_ioctl(struct block_device *bdev, blk_mode_t mode, rel_fdc(); return -EBUSY; } - fsync_bdev(bdev); if (fd_motor_on(drive) == 0) { rel_fdc(); return -ENODEV; From 2527fd38772fea30c1d1cbf94839a0bbf4122133 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:22 +0200 Subject: [PATCH 36/47] dasd: also call __invalidate_device when setting the device offline Don't just write out the data, but also invalidate all caches when setting the device offline. Stop canceling the offlining when writeback fails as there is no way to recover from that anyway. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-12-hch@lst.de> Signed-off-by: Christian Brauner --- drivers/s390/block/dasd.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c index edcbf77852c3..675b38ad00dc 100644 --- a/drivers/s390/block/dasd.c +++ b/drivers/s390/block/dasd.c @@ -3627,9 +3627,8 @@ int dasd_generic_set_offline(struct ccw_device *cdev) * empty */ if (device->block) { - rc = fsync_bdev(device->block->bdev); - if (rc != 0) - goto interrupted; + fsync_bdev(device->block->bdev); + __invalidate_device(device->block->bdev, true); } dasd_schedule_device_bh(device); rc = wait_event_interruptible(shutdown_waitq, From 127a5093c79d9cdea9e6edc016ab346e6dd16c23 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:23 +0200 Subject: [PATCH 37/47] block: drop the "busy inodes on changed media" log message This message isn't exactly helpful, and file systems already print way more useful messages when shut down while active. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-13-hch@lst.de> Signed-off-by: Christian Brauner --- block/disk-events.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/block/disk-events.c b/block/disk-events.c index 6189b819b235..6b858d350477 100644 --- a/block/disk-events.c +++ b/block/disk-events.c @@ -281,9 +281,7 @@ bool disk_check_media_change(struct gendisk *disk) if (!(events & DISK_EVENT_MEDIA_CHANGE)) return false; - if (__invalidate_device(disk->part0, true)) - pr_warn("VFS: busy inodes on changed media %s\n", - disk->disk_name); + __invalidate_device(disk->part0, true); set_bit(GD_NEED_PART_SCAN, &disk->state); return true; } @@ -302,9 +300,7 @@ void disk_force_media_change(struct gendisk *disk) { disk_event_uevent(disk, DISK_EVENT_MEDIA_CHANGE); inc_diskseq(disk); - if (__invalidate_device(disk->part0, true)) - pr_warn("VFS: busy inodes on changed media %s\n", - disk->disk_name); + __invalidate_device(disk->part0, true); set_bit(GD_NEED_PART_SCAN, &disk->state); } EXPORT_SYMBOL_GPL(disk_force_media_change); From 560e20e4bf6484a0c12f9f3c7a1aa55056948e1e Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:24 +0200 Subject: [PATCH 38/47] block: consolidate __invalidate_device and fsync_bdev We currently have two interfaces that take a block_devices and the find a mounted file systems to flush or invaldidate data on it. Both are a bit problematic because they only work for the "main" block devices that is used as s_dev for the super_block, and because they don't call into the file system at all. Merge the two into a new bdev_mark_dead helper that does both the syncing and invalidation and which is properly documented. This is in preparation of merging the functionality into the ->mark_dead holder operation so that it will work on additional block devices used by a file systems and give us a single entry point for invalidation of dead devices or media. Note that a single standalone fsync_bdev call for an obscure ioctl remains for now, but that one will also be deal with in a bit. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-14-hch@lst.de> Signed-off-by: Christian Brauner --- block/bdev.c | 33 ++++++++++++++++++++++++++++----- block/disk-events.c | 4 ++-- block/genhd.c | 3 +-- block/partitions/core.c | 5 +---- drivers/s390/block/dasd.c | 6 ++---- fs/super.c | 4 ++-- include/linux/blkdev.h | 2 +- 7 files changed, 37 insertions(+), 20 deletions(-) diff --git a/block/bdev.c b/block/bdev.c index 979e28a46b98..074a9ffa9d8b 100644 --- a/block/bdev.c +++ b/block/bdev.c @@ -221,7 +221,6 @@ int fsync_bdev(struct block_device *bdev) } return sync_blockdev(bdev); } -EXPORT_SYMBOL(fsync_bdev); /** * freeze_bdev - lock a filesystem and force it into a consistent state @@ -960,12 +959,27 @@ out_path_put: } EXPORT_SYMBOL(lookup_bdev); -int __invalidate_device(struct block_device *bdev, bool kill_dirty) +/** + * bdev_mark_dead - mark a block device as dead + * @bdev: block device to operate on + * @surprise: indicate a surprise removal + * + * Tell the file system that this devices or media is dead. If @surprise is set + * to %true the device or media is already gone, if not we are preparing for an + * orderly removal. + * + * This syncs out all dirty data and writes back inodes and then invalidates any + * cached data in the inodes on the file system, the inodes themselves and the + * block device mapping. + */ +void bdev_mark_dead(struct block_device *bdev, bool surprise) { struct super_block *sb = get_super(bdev); int res = 0; if (sb) { + if (!surprise) + sync_filesystem(sb); /* * no need to lock the super, get_super holds the * read mutex so the filesystem cannot go away @@ -973,13 +987,22 @@ int __invalidate_device(struct block_device *bdev, bool kill_dirty) * hold). */ shrink_dcache_sb(sb); - res = invalidate_inodes(sb, kill_dirty); + res = invalidate_inodes(sb, true); drop_super(sb); + } else { + if (!surprise) + sync_blockdev(bdev); } invalidate_bdev(bdev); - return res; } -EXPORT_SYMBOL(__invalidate_device); +#ifdef CONFIG_DASD_MODULE +/* + * Drivers should not use this directly, but the DASD driver has historically + * had a shutdown to offline mode that doesn't actually remove the gendisk + * that otherwise looks a lot like a safe device removal. + */ +EXPORT_SYMBOL_GPL(bdev_mark_dead); +#endif void sync_bdevs(bool wait) { diff --git a/block/disk-events.c b/block/disk-events.c index 6b858d350477..422db8292d09 100644 --- a/block/disk-events.c +++ b/block/disk-events.c @@ -281,7 +281,7 @@ bool disk_check_media_change(struct gendisk *disk) if (!(events & DISK_EVENT_MEDIA_CHANGE)) return false; - __invalidate_device(disk->part0, true); + bdev_mark_dead(disk->part0, true); set_bit(GD_NEED_PART_SCAN, &disk->state); return true; } @@ -300,7 +300,7 @@ void disk_force_media_change(struct gendisk *disk) { disk_event_uevent(disk, DISK_EVENT_MEDIA_CHANGE); inc_diskseq(disk); - __invalidate_device(disk->part0, true); + bdev_mark_dead(disk->part0, true); set_bit(GD_NEED_PART_SCAN, &disk->state); } EXPORT_SYMBOL_GPL(disk_force_media_change); diff --git a/block/genhd.c b/block/genhd.c index 3d287b32d50d..afc2cb09eb94 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -647,8 +647,7 @@ void del_gendisk(struct gendisk *disk) mutex_lock(&disk->open_mutex); xa_for_each(&disk->part_tbl, idx, part) { remove_inode_hash(part->bd_inode); - fsync_bdev(part); - __invalidate_device(part, true); + bdev_mark_dead(part, false); } mutex_unlock(&disk->open_mutex); diff --git a/block/partitions/core.c b/block/partitions/core.c index 13a7341299a9..e137a87f4db0 100644 --- a/block/partitions/core.c +++ b/block/partitions/core.c @@ -281,10 +281,7 @@ static void delete_partition(struct block_device *part) * looked up any more even when openers still hold references. */ remove_inode_hash(part->bd_inode); - - fsync_bdev(part); - __invalidate_device(part, true); - + bdev_mark_dead(part, false); drop_partition(part); } diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c index 675b38ad00dc..1f642be840c3 100644 --- a/drivers/s390/block/dasd.c +++ b/drivers/s390/block/dasd.c @@ -3626,10 +3626,8 @@ int dasd_generic_set_offline(struct ccw_device *cdev) * so sync bdev first and then wait for our queues to become * empty */ - if (device->block) { - fsync_bdev(device->block->bdev); - __invalidate_device(device->block->bdev, true); - } + if (device->block) + bdev_mark_dead(device->block->bdev, false); dasd_schedule_device_bh(device); rc = wait_event_interruptible(shutdown_waitq, _wait_for_empty_queues(device)); diff --git a/fs/super.c b/fs/super.c index f72a1112a31b..9b2188e08bcc 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1359,7 +1359,7 @@ int get_tree_bdev(struct fs_context *fc, /* * We drop s_umount here because we need to open the bdev and * bdev->open_mutex ranks above s_umount (blkdev_put() -> - * __invalidate_device()). It is safe because we have active sb + * bdev_mark_dead()). It is safe because we have active sb * reference and SB_BORN is not set yet. */ up_write(&s->s_umount); @@ -1411,7 +1411,7 @@ struct dentry *mount_bdev(struct file_system_type *fs_type, /* * We drop s_umount here because we need to open the bdev and * bdev->open_mutex ranks above s_umount (blkdev_put() -> - * __invalidate_device()). It is safe because we have active sb + * bdev_mark_dead()). It is safe because we have active sb * reference and SB_BORN is not set yet. */ up_write(&s->s_umount); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index c8eab6effc22..6721595b9f97 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -751,6 +751,7 @@ static inline int bdev_read_only(struct block_device *bdev) bool set_capacity_and_notify(struct gendisk *disk, sector_t size); void disk_force_media_change(struct gendisk *disk); +void bdev_mark_dead(struct block_device *bdev, bool surprise); void add_disk_randomness(struct gendisk *disk) __latent_entropy; void rand_initialize_disk(struct gendisk *disk); @@ -809,7 +810,6 @@ int __register_blkdev(unsigned int major, const char *name, void unregister_blkdev(unsigned int major, const char *name); bool disk_check_media_change(struct gendisk *disk); -int __invalidate_device(struct block_device *bdev, bool kill_dirty); void set_capacity(struct gendisk *disk, sector_t size); #ifdef CONFIG_BLOCK_HOLDER_DEPRECATED From d8530de5a6e82be0ce17a5fdf727a394bcf6444c Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:25 +0200 Subject: [PATCH 39/47] block: call into the file system for bdev_mark_dead Combine the newly merged bdev_mark_dead helper with the existing mark_dead holder operation so that all operations that invalidate a device that is dead or being removed now go through the holder ops. This allows file systems to explicitly shutdown either ASAP (for a surprise removal) or after writing back data (for an orderly removal), and do so not only for the main device. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-15-hch@lst.de> Signed-off-by: Christian Brauner --- block/bdev.c | 30 +++++++++------------------- block/genhd.c | 44 +++++++++++++++++++++++------------------- fs/super.c | 8 ++++++-- include/linux/blkdev.h | 2 +- 4 files changed, 40 insertions(+), 44 deletions(-) diff --git a/block/bdev.c b/block/bdev.c index 074a9ffa9d8b..4d580083a114 100644 --- a/block/bdev.c +++ b/block/bdev.c @@ -968,31 +968,19 @@ EXPORT_SYMBOL(lookup_bdev); * to %true the device or media is already gone, if not we are preparing for an * orderly removal. * - * This syncs out all dirty data and writes back inodes and then invalidates any - * cached data in the inodes on the file system, the inodes themselves and the - * block device mapping. + * This calls into the file system, which then typicall syncs out all dirty data + * and writes back inodes and then invalidates any cached data in the inodes on + * the file system. In addition we also invalidate the block device mapping. */ void bdev_mark_dead(struct block_device *bdev, bool surprise) { - struct super_block *sb = get_super(bdev); - int res = 0; + mutex_lock(&bdev->bd_holder_lock); + if (bdev->bd_holder_ops && bdev->bd_holder_ops->mark_dead) + bdev->bd_holder_ops->mark_dead(bdev, surprise); + else + sync_blockdev(bdev); + mutex_unlock(&bdev->bd_holder_lock); - if (sb) { - if (!surprise) - sync_filesystem(sb); - /* - * no need to lock the super, get_super holds the - * read mutex so the filesystem cannot go away - * under us (->put_super runs with the write lock - * hold). - */ - shrink_dcache_sb(sb); - res = invalidate_inodes(sb, true); - drop_super(sb); - } else { - if (!surprise) - sync_blockdev(bdev); - } invalidate_bdev(bdev); } #ifdef CONFIG_DASD_MODULE diff --git a/block/genhd.c b/block/genhd.c index afc2cb09eb94..cc32a0c704eb 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -554,7 +554,7 @@ out_exit_elevator: } EXPORT_SYMBOL(device_add_disk); -static void blk_report_disk_dead(struct gendisk *disk) +static void blk_report_disk_dead(struct gendisk *disk, bool surprise) { struct block_device *bdev; unsigned long idx; @@ -565,10 +565,7 @@ static void blk_report_disk_dead(struct gendisk *disk) continue; rcu_read_unlock(); - mutex_lock(&bdev->bd_holder_lock); - if (bdev->bd_holder_ops && bdev->bd_holder_ops->mark_dead) - bdev->bd_holder_ops->mark_dead(bdev); - mutex_unlock(&bdev->bd_holder_lock); + bdev_mark_dead(bdev, surprise); put_device(&bdev->bd_device); rcu_read_lock(); @@ -576,14 +573,7 @@ static void blk_report_disk_dead(struct gendisk *disk) rcu_read_unlock(); } -/** - * blk_mark_disk_dead - mark a disk as dead - * @disk: disk to mark as dead - * - * Mark as disk as dead (e.g. surprise removed) and don't accept any new I/O - * to this disk. - */ -void blk_mark_disk_dead(struct gendisk *disk) +static void __blk_mark_disk_dead(struct gendisk *disk) { /* * Fail any new I/O. @@ -603,8 +593,19 @@ void blk_mark_disk_dead(struct gendisk *disk) * Prevent new I/O from crossing bio_queue_enter(). */ blk_queue_start_drain(disk->queue); +} - blk_report_disk_dead(disk); +/** + * blk_mark_disk_dead - mark a disk as dead + * @disk: disk to mark as dead + * + * Mark as disk as dead (e.g. surprise removed) and don't accept any new I/O + * to this disk. + */ +void blk_mark_disk_dead(struct gendisk *disk) +{ + __blk_mark_disk_dead(disk); + blk_report_disk_dead(disk, true); } EXPORT_SYMBOL_GPL(blk_mark_disk_dead); @@ -641,17 +642,20 @@ void del_gendisk(struct gendisk *disk) disk_del_events(disk); /* - * Prevent new openers by unlinked the bdev inode, and write out - * dirty data before marking the disk dead and stopping all I/O. + * Prevent new openers by unlinked the bdev inode. */ mutex_lock(&disk->open_mutex); - xa_for_each(&disk->part_tbl, idx, part) { + xa_for_each(&disk->part_tbl, idx, part) remove_inode_hash(part->bd_inode); - bdev_mark_dead(part, false); - } mutex_unlock(&disk->open_mutex); - blk_mark_disk_dead(disk); + /* + * Tell the file system to write back all dirty data and shut down if + * it hasn't been notified earlier. + */ + if (!test_bit(GD_DEAD, &disk->state)) + blk_report_disk_dead(disk, false); + __blk_mark_disk_dead(disk); /* * Drop all partitions now that the disk is marked dead. diff --git a/fs/super.c b/fs/super.c index 9b2188e08bcc..11fa21da130c 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1228,7 +1228,7 @@ static bool lock_active_super(struct super_block *sb) return true; } -static void fs_mark_dead(struct block_device *bdev) +static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise) { struct super_block *sb = bdev->bd_holder; @@ -1238,6 +1238,10 @@ static void fs_mark_dead(struct block_device *bdev) if (!lock_active_super(sb)) return; + if (!surprise) + sync_filesystem(sb); + shrink_dcache_sb(sb); + invalidate_inodes(sb, true); if (sb->s_op->shutdown) sb->s_op->shutdown(sb); @@ -1245,7 +1249,7 @@ static void fs_mark_dead(struct block_device *bdev) } const struct blk_holder_ops fs_holder_ops = { - .mark_dead = fs_mark_dead, + .mark_dead = fs_bdev_mark_dead, }; EXPORT_SYMBOL_GPL(fs_holder_ops); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 6721595b9f97..cdd03c612d39 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1461,7 +1461,7 @@ void blkdev_show(struct seq_file *seqf, off_t offset); #endif struct blk_holder_ops { - void (*mark_dead)(struct block_device *bdev); + void (*mark_dead)(struct block_device *bdev, bool surprise); }; extern const struct blk_holder_ops fs_holder_ops; From 2142b88c37a3e49fbca4a36b8674626917d9bf40 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:26 +0200 Subject: [PATCH 40/47] block: call into the file system for ioctl BLKFLSBUF BLKFLSBUF is a historic ioctl that is called on a file handle to a block device and syncs either the file system mounted on that block device if there is one, or otherwise the just the data on the block device. Replace the get_super based syncing with a holder operation to remove the last usage of get_super, and to also support syncing the file system if the block device is not the main block device stored in s_dev. Signed-off-by: Christoph Hellwig Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-16-hch@lst.de> Signed-off-by: Christian Brauner --- block/bdev.c | 16 ---------------- block/ioctl.c | 9 ++++++++- fs/super.c | 13 +++++++++++++ include/linux/blkdev.h | 7 +++++-- 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/block/bdev.c b/block/bdev.c index 4d580083a114..a20263fa27a4 100644 --- a/block/bdev.c +++ b/block/bdev.c @@ -206,22 +206,6 @@ int sync_blockdev_range(struct block_device *bdev, loff_t lstart, loff_t lend) } EXPORT_SYMBOL(sync_blockdev_range); -/* - * Write out and wait upon all dirty data associated with this - * device. Filesystem data as well as the underlying block - * device. Takes the superblock lock. - */ -int fsync_bdev(struct block_device *bdev) -{ - struct super_block *sb = get_super(bdev); - if (sb) { - int res = sync_filesystem(sb); - drop_super(sb); - return res; - } - return sync_blockdev(bdev); -} - /** * freeze_bdev - lock a filesystem and force it into a consistent state * @bdev: blockdevice to lock diff --git a/block/ioctl.c b/block/ioctl.c index 3be11941fb2d..648670ddb164 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -364,7 +364,14 @@ static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd, { if (!capable(CAP_SYS_ADMIN)) return -EACCES; - fsync_bdev(bdev); + + mutex_lock(&bdev->bd_holder_lock); + if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync) + bdev->bd_holder_ops->sync(bdev); + else + sync_blockdev(bdev); + mutex_unlock(&bdev->bd_holder_lock); + invalidate_bdev(bdev); return 0; } diff --git a/fs/super.c b/fs/super.c index 11fa21da130c..1a369fa3f0a6 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1248,8 +1248,21 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise) up_read(&sb->s_umount); } +static void fs_bdev_sync(struct block_device *bdev) +{ + struct super_block *sb = bdev->bd_holder; + + lockdep_assert_held(&bdev->bd_holder_lock); + + if (!lock_active_super(sb)) + return; + sync_filesystem(sb); + up_read(&sb->s_umount); +} + const struct blk_holder_ops fs_holder_ops = { .mark_dead = fs_bdev_mark_dead, + .sync = fs_bdev_sync, }; EXPORT_SYMBOL_GPL(fs_holder_ops); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index cdd03c612d39..fa462d48ee96 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1462,6 +1462,11 @@ void blkdev_show(struct seq_file *seqf, off_t offset); struct blk_holder_ops { void (*mark_dead)(struct block_device *bdev, bool surprise); + + /* + * Sync the file system mounted on the block device. + */ + void (*sync)(struct block_device *bdev); }; extern const struct blk_holder_ops fs_holder_ops; @@ -1524,8 +1529,6 @@ static inline int early_lookup_bdev(const char *pathname, dev_t *dev) } #endif /* CONFIG_BLOCK */ -int fsync_bdev(struct block_device *bdev); - int freeze_bdev(struct block_device *bdev); int thaw_bdev(struct block_device *bdev); From 38bcdd38935350abceace901313e007e84d84456 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:27 +0200 Subject: [PATCH 41/47] fs: remove get_super get_super is unused now, remove it. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-17-hch@lst.de> Signed-off-by: Christian Brauner --- fs/super.c | 37 ------------------------------------- include/linux/fs.h | 1 - 2 files changed, 38 deletions(-) diff --git a/fs/super.c b/fs/super.c index 1a369fa3f0a6..1c7c74855437 100644 --- a/fs/super.c +++ b/fs/super.c @@ -790,43 +790,6 @@ void iterate_supers_type(struct file_system_type *type, EXPORT_SYMBOL(iterate_supers_type); -/** - * get_super - get the superblock of a device - * @bdev: device to get the superblock for - * - * Scans the superblock list and finds the superblock of the file system - * mounted on the device given. %NULL is returned if no match is found. - */ -struct super_block *get_super(struct block_device *bdev) -{ - struct super_block *sb; - - if (!bdev) - return NULL; - - spin_lock(&sb_lock); -rescan: - list_for_each_entry(sb, &super_blocks, s_list) { - if (hlist_unhashed(&sb->s_instances)) - continue; - if (sb->s_bdev == bdev) { - sb->s_count++; - spin_unlock(&sb_lock); - down_read(&sb->s_umount); - /* still alive? */ - if (sb->s_root && (sb->s_flags & SB_BORN)) - return sb; - up_read(&sb->s_umount); - /* nope, got unmounted */ - spin_lock(&sb_lock); - __put_super(sb); - goto rescan; - } - } - spin_unlock(&sb_lock); - return NULL; -} - /** * get_active_super - get an active reference to the superblock of a device * @bdev: device to get the superblock for diff --git a/include/linux/fs.h b/include/linux/fs.h index 6867512907d6..14b5777a24a0 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2913,7 +2913,6 @@ extern int vfs_readlink(struct dentry *, char __user *, int); extern struct file_system_type *get_filesystem(struct file_system_type *fs); extern void put_filesystem(struct file_system_type *fs); extern struct file_system_type *get_fs_type(const char *name); -extern struct super_block *get_super(struct block_device *); extern struct super_block *get_active_super(struct block_device *bdev); extern void drop_super(struct super_block *sb); extern void drop_super_exclusive(struct super_block *sb); From e127b9bccdb04e5fc4444431de37309a68aedafa Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Fri, 11 Aug 2023 12:08:28 +0200 Subject: [PATCH 42/47] fs: simplify invalidate_inodes kill_dirty has always been true for a long time, so hard code it and remove the unused return value. Signed-off-by: Christoph Hellwig Reviewed-by: Christian Brauner Reviewed-by: Josef Bacik Message-Id: <20230811100828.1897174-18-hch@lst.de> Signed-off-by: Christian Brauner --- fs/inode.c | 17 ++--------------- fs/internal.h | 2 +- fs/super.c | 2 +- 3 files changed, 4 insertions(+), 17 deletions(-) diff --git a/fs/inode.c b/fs/inode.c index 8fefb69e1f84..25bd2635c161 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -752,16 +752,11 @@ EXPORT_SYMBOL_GPL(evict_inodes); /** * invalidate_inodes - attempt to free all inodes on a superblock * @sb: superblock to operate on - * @kill_dirty: flag to guide handling of dirty inodes * - * Attempts to free all inodes for a given superblock. If there were any - * busy inodes return a non-zero value, else zero. - * If @kill_dirty is set, discard dirty inodes too, otherwise treat - * them as busy. + * Attempts to free all inodes (including dirty inodes) for a given superblock. */ -int invalidate_inodes(struct super_block *sb, bool kill_dirty) +void invalidate_inodes(struct super_block *sb) { - int busy = 0; struct inode *inode, *next; LIST_HEAD(dispose); @@ -773,14 +768,8 @@ again: spin_unlock(&inode->i_lock); continue; } - if (inode->i_state & I_DIRTY_ALL && !kill_dirty) { - spin_unlock(&inode->i_lock); - busy = 1; - continue; - } if (atomic_read(&inode->i_count)) { spin_unlock(&inode->i_lock); - busy = 1; continue; } @@ -798,8 +787,6 @@ again: spin_unlock(&sb->s_inode_list_lock); dispose_list(&dispose); - - return busy; } /* diff --git a/fs/internal.h b/fs/internal.h index f7a3dc111026..b94290f61714 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -201,7 +201,7 @@ void lock_two_inodes(struct inode *inode1, struct inode *inode2, * fs-writeback.c */ extern long get_nr_dirty_inodes(void); -extern int invalidate_inodes(struct super_block *, bool); +void invalidate_inodes(struct super_block *sb); /* * dcache.c diff --git a/fs/super.c b/fs/super.c index 1c7c74855437..c878e7373f93 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1204,7 +1204,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise) if (!surprise) sync_filesystem(sb); shrink_dcache_sb(sb); - invalidate_inodes(sb, true); + invalidate_inodes(sb); if (sb->s_op->shutdown) sb->s_op->shutdown(sb); From 0ed33598ddf308782ca621755df5d23dcff34b64 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 18 Aug 2023 16:00:48 +0200 Subject: [PATCH 43/47] super: use locking helpers Replace the open-coded {down,up}_{read,write}() calls with simple wrappers. Follow-up patches will benefit from this as well. Reviewed-by: Jan Kara Message-Id: <20230818-vfs-super-fixes-v3-v3-1-9f0b1876e46b@kernel.org> Signed-off-by: Christian Brauner --- fs/super.c | 126 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 78 insertions(+), 48 deletions(-) diff --git a/fs/super.c b/fs/super.c index c878e7373f93..b12e2f247e1e 100644 --- a/fs/super.c +++ b/fs/super.c @@ -50,6 +50,42 @@ static char *sb_writers_name[SB_FREEZE_LEVELS] = { "sb_internal", }; +static inline void super_lock(struct super_block *sb, bool excl) +{ + if (excl) + down_write(&sb->s_umount); + else + down_read(&sb->s_umount); +} + +static inline void super_unlock(struct super_block *sb, bool excl) +{ + if (excl) + up_write(&sb->s_umount); + else + up_read(&sb->s_umount); +} + +static inline void super_lock_excl(struct super_block *sb) +{ + super_lock(sb, true); +} + +static inline void super_lock_shared(struct super_block *sb) +{ + super_lock(sb, false); +} + +static inline void super_unlock_excl(struct super_block *sb) +{ + super_unlock(sb, true); +} + +static inline void super_unlock_shared(struct super_block *sb) +{ + super_unlock(sb, false); +} + /* * One thing we have to be careful of with a per-sb shrinker is that we don't * drop the last active reference to the superblock from within the shrinker. @@ -110,7 +146,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink, freed += sb->s_op->free_cached_objects(sb, sc); } - up_read(&sb->s_umount); + super_unlock_shared(sb); return freed; } @@ -176,7 +212,7 @@ static void destroy_unused_super(struct super_block *s) { if (!s) return; - up_write(&s->s_umount); + super_unlock_excl(s); list_lru_destroy(&s->s_dentry_lru); list_lru_destroy(&s->s_inode_lru); security_sb_free(s); @@ -340,7 +376,7 @@ void deactivate_locked_super(struct super_block *s) put_filesystem(fs); put_super(s); } else { - up_write(&s->s_umount); + super_unlock_excl(s); } } @@ -357,7 +393,7 @@ EXPORT_SYMBOL(deactivate_locked_super); void deactivate_super(struct super_block *s) { if (!atomic_add_unless(&s->s_active, -1, 1)) { - down_write(&s->s_umount); + super_lock_excl(s); deactivate_locked_super(s); } } @@ -381,12 +417,12 @@ static int grab_super(struct super_block *s) __releases(sb_lock) { s->s_count++; spin_unlock(&sb_lock); - down_write(&s->s_umount); + super_lock_excl(s); if ((s->s_flags & SB_BORN) && atomic_inc_not_zero(&s->s_active)) { put_super(s); return 1; } - up_write(&s->s_umount); + super_unlock_excl(s); put_super(s); return 0; } @@ -414,7 +450,7 @@ bool trylock_super(struct super_block *sb) if (!hlist_unhashed(&sb->s_instances) && sb->s_root && (sb->s_flags & SB_BORN)) return true; - up_read(&sb->s_umount); + super_unlock_shared(sb); } return false; @@ -439,13 +475,13 @@ bool trylock_super(struct super_block *sb) void retire_super(struct super_block *sb) { WARN_ON(!sb->s_bdev); - down_write(&sb->s_umount); + super_lock_excl(sb); if (sb->s_iflags & SB_I_PERSB_BDI) { bdi_unregister(sb->s_bdi); sb->s_iflags &= ~SB_I_PERSB_BDI; } sb->s_iflags |= SB_I_RETIRED; - up_write(&sb->s_umount); + super_unlock_excl(sb); } EXPORT_SYMBOL(retire_super); @@ -521,7 +557,7 @@ void generic_shutdown_super(struct super_block *sb) /* should be initialized for __put_super_and_need_restart() */ hlist_del_init(&sb->s_instances); spin_unlock(&sb_lock); - up_write(&sb->s_umount); + super_unlock_excl(sb); if (sb->s_bdi != &noop_backing_dev_info) { if (sb->s_iflags & SB_I_PERSB_BDI) bdi_unregister(sb->s_bdi); @@ -685,7 +721,7 @@ EXPORT_SYMBOL(sget); void drop_super(struct super_block *sb) { - up_read(&sb->s_umount); + super_unlock_shared(sb); put_super(sb); } @@ -693,7 +729,7 @@ EXPORT_SYMBOL(drop_super); void drop_super_exclusive(struct super_block *sb) { - up_write(&sb->s_umount); + super_unlock_excl(sb); put_super(sb); } EXPORT_SYMBOL(drop_super_exclusive); @@ -739,10 +775,10 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg) sb->s_count++; spin_unlock(&sb_lock); - down_read(&sb->s_umount); + super_lock_shared(sb); if (sb->s_root && (sb->s_flags & SB_BORN)) f(sb, arg); - up_read(&sb->s_umount); + super_unlock_shared(sb); spin_lock(&sb_lock); if (p) @@ -773,10 +809,10 @@ void iterate_supers_type(struct file_system_type *type, sb->s_count++; spin_unlock(&sb_lock); - down_read(&sb->s_umount); + super_lock_shared(sb); if (sb->s_root && (sb->s_flags & SB_BORN)) f(sb, arg); - up_read(&sb->s_umount); + super_unlock_shared(sb); spin_lock(&sb_lock); if (p) @@ -813,7 +849,7 @@ restart: if (sb->s_bdev == bdev) { if (!grab_super(sb)) goto restart; - up_write(&sb->s_umount); + super_unlock_excl(sb); return sb; } } @@ -833,17 +869,11 @@ rescan: if (sb->s_dev == dev) { sb->s_count++; spin_unlock(&sb_lock); - if (excl) - down_write(&sb->s_umount); - else - down_read(&sb->s_umount); + super_lock(sb, excl); /* still alive? */ if (sb->s_root && (sb->s_flags & SB_BORN)) return sb; - if (excl) - up_write(&sb->s_umount); - else - up_read(&sb->s_umount); + super_unlock(sb, excl); /* nope, got unmounted */ spin_lock(&sb_lock); __put_super(sb); @@ -889,9 +919,9 @@ int reconfigure_super(struct fs_context *fc) if (remount_ro) { if (!hlist_empty(&sb->s_pins)) { - up_write(&sb->s_umount); + super_unlock_excl(sb); group_pin_kill(&sb->s_pins); - down_write(&sb->s_umount); + super_lock_excl(sb); if (!sb->s_root) return 0; if (sb->s_writers.frozen != SB_UNFROZEN) @@ -954,7 +984,7 @@ cancel_readonly: static void do_emergency_remount_callback(struct super_block *sb) { - down_write(&sb->s_umount); + super_lock_excl(sb); if (sb->s_root && sb->s_bdev && (sb->s_flags & SB_BORN) && !sb_rdonly(sb)) { struct fs_context *fc; @@ -967,7 +997,7 @@ static void do_emergency_remount_callback(struct super_block *sb) put_fs_context(fc); } } - up_write(&sb->s_umount); + super_unlock_excl(sb); } static void do_emergency_remount(struct work_struct *work) @@ -990,12 +1020,12 @@ void emergency_remount(void) static void do_thaw_all_callback(struct super_block *sb) { - down_write(&sb->s_umount); + super_lock_excl(sb); if (sb->s_root && sb->s_flags & SB_BORN) { emergency_thaw_bdev(sb); thaw_super_locked(sb); } else { - up_write(&sb->s_umount); + super_unlock_excl(sb); } } @@ -1182,10 +1212,10 @@ EXPORT_SYMBOL(get_tree_keyed); */ static bool lock_active_super(struct super_block *sb) { - down_read(&sb->s_umount); + super_lock_shared(sb); if (!sb->s_root || (sb->s_flags & (SB_ACTIVE | SB_BORN)) != (SB_ACTIVE | SB_BORN)) { - up_read(&sb->s_umount); + super_unlock_shared(sb); return false; } return true; @@ -1208,7 +1238,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise) if (sb->s_op->shutdown) sb->s_op->shutdown(sb); - up_read(&sb->s_umount); + super_unlock_shared(sb); } static void fs_bdev_sync(struct block_device *bdev) @@ -1220,7 +1250,7 @@ static void fs_bdev_sync(struct block_device *bdev) if (!lock_active_super(sb)) return; sync_filesystem(sb); - up_read(&sb->s_umount); + super_unlock_shared(sb); } const struct blk_holder_ops fs_holder_ops = { @@ -1342,9 +1372,9 @@ int get_tree_bdev(struct fs_context *fc, * bdev_mark_dead()). It is safe because we have active sb * reference and SB_BORN is not set yet. */ - up_write(&s->s_umount); + super_unlock_excl(s); error = setup_bdev_super(s, fc->sb_flags, fc); - down_write(&s->s_umount); + super_lock_excl(s); if (!error) error = fill_super(s, fc); if (error) { @@ -1394,9 +1424,9 @@ struct dentry *mount_bdev(struct file_system_type *fs_type, * bdev_mark_dead()). It is safe because we have active sb * reference and SB_BORN is not set yet. */ - up_write(&s->s_umount); + super_unlock_excl(s); error = setup_bdev_super(s, flags, NULL); - down_write(&s->s_umount); + super_lock_excl(s); if (!error) error = fill_super(s, data, flags & SB_SILENT ? 1 : 0); if (error) { @@ -1685,29 +1715,29 @@ int freeze_super(struct super_block *sb) int ret; atomic_inc(&sb->s_active); - down_write(&sb->s_umount); + super_lock_excl(sb); if (sb->s_writers.frozen != SB_UNFROZEN) { deactivate_locked_super(sb); return -EBUSY; } if (!(sb->s_flags & SB_BORN)) { - up_write(&sb->s_umount); + super_unlock_excl(sb); return 0; /* sic - it's "nothing to do" */ } if (sb_rdonly(sb)) { /* Nothing to do really... */ sb->s_writers.frozen = SB_FREEZE_COMPLETE; - up_write(&sb->s_umount); + super_unlock_excl(sb); return 0; } sb->s_writers.frozen = SB_FREEZE_WRITE; /* Release s_umount to preserve sb_start_write -> s_umount ordering */ - up_write(&sb->s_umount); + super_unlock_excl(sb); sb_wait_write(sb, SB_FREEZE_WRITE); - down_write(&sb->s_umount); + super_lock_excl(sb); /* Now we go and block page faults... */ sb->s_writers.frozen = SB_FREEZE_PAGEFAULT; @@ -1743,7 +1773,7 @@ int freeze_super(struct super_block *sb) */ sb->s_writers.frozen = SB_FREEZE_COMPLETE; lockdep_sb_freeze_release(sb); - up_write(&sb->s_umount); + super_unlock_excl(sb); return 0; } EXPORT_SYMBOL(freeze_super); @@ -1753,7 +1783,7 @@ static int thaw_super_locked(struct super_block *sb) int error; if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) { - up_write(&sb->s_umount); + super_unlock_excl(sb); return -EINVAL; } @@ -1770,7 +1800,7 @@ static int thaw_super_locked(struct super_block *sb) printk(KERN_ERR "VFS:Filesystem thaw failed\n"); lockdep_sb_freeze_release(sb); - up_write(&sb->s_umount); + super_unlock_excl(sb); return error; } } @@ -1790,7 +1820,7 @@ out: */ int thaw_super(struct super_block *sb) { - down_write(&sb->s_umount); + super_lock_excl(sb); return thaw_super_locked(sb); } EXPORT_SYMBOL(thaw_super); From d8ce82efdece373b570f35acc8a29487b2087b84 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 18 Aug 2023 16:00:49 +0200 Subject: [PATCH 44/47] super: make locking naming consistent Make the naming consistent with the earlier introduced super_lock_{read,write}() helpers. Reviewed-by: Jan Kara Message-Id: <20230818-vfs-super-fixes-v3-v3-2-9f0b1876e46b@kernel.org> Signed-off-by: Christian Brauner --- fs/fs-writeback.c | 4 ++-- fs/internal.h | 2 +- fs/super.c | 28 ++++++++++++++-------------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index aca4b4811394..969ce991b0b0 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -1953,9 +1953,9 @@ static long __writeback_inodes_wb(struct bdi_writeback *wb, struct inode *inode = wb_inode(wb->b_io.prev); struct super_block *sb = inode->i_sb; - if (!trylock_super(sb)) { + if (!super_trylock_shared(sb)) { /* - * trylock_super() may fail consistently due to + * super_trylock_shared() may fail consistently due to * s_umount being grabbed by someone else. Don't use * requeue_io() to avoid busy retrying the inode/sb. */ diff --git a/fs/internal.h b/fs/internal.h index b94290f61714..74d3b161dd2c 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -115,7 +115,7 @@ static inline void put_file_access(struct file *file) * super.c */ extern int reconfigure_super(struct fs_context *); -extern bool trylock_super(struct super_block *sb); +extern bool super_trylock_shared(struct super_block *sb); struct super_block *user_get_super(dev_t, bool excl); void put_super(struct super_block *sb); extern bool mount_capable(struct fs_context *); diff --git a/fs/super.c b/fs/super.c index b12e2f247e1e..ba5d813c5804 100644 --- a/fs/super.c +++ b/fs/super.c @@ -112,7 +112,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink, if (!(sc->gfp_mask & __GFP_FS)) return SHRINK_STOP; - if (!trylock_super(sb)) + if (!super_trylock_shared(sb)) return SHRINK_STOP; if (sb->s_op->nr_cached_objects) @@ -159,17 +159,17 @@ static unsigned long super_cache_count(struct shrinker *shrink, sb = container_of(shrink, struct super_block, s_shrink); /* - * We don't call trylock_super() here as it is a scalability bottleneck, - * so we're exposed to partial setup state. The shrinker rwsem does not - * protect filesystem operations backing list_lru_shrink_count() or - * s_op->nr_cached_objects(). Counts can change between - * super_cache_count and super_cache_scan, so we really don't need locks - * here. + * We don't call super_trylock_shared() here as it is a scalability + * bottleneck, so we're exposed to partial setup state. The shrinker + * rwsem does not protect filesystem operations backing + * list_lru_shrink_count() or s_op->nr_cached_objects(). Counts can + * change between super_cache_count and super_cache_scan, so we really + * don't need locks here. * * However, if we are currently mounting the superblock, the underlying * filesystem might be in a state of partial construction and hence it - * is dangerous to access it. trylock_super() uses a SB_BORN check to - * avoid this situation, so do the same here. The memory barrier is + * is dangerous to access it. super_trylock_shared() uses a SB_BORN check + * to avoid this situation, so do the same here. The memory barrier is * matched with the one in mount_fs() as we don't hold locks here. */ if (!(sb->s_flags & SB_BORN)) @@ -428,7 +428,7 @@ static int grab_super(struct super_block *s) __releases(sb_lock) } /* - * trylock_super - try to grab ->s_umount shared + * super_trylock_shared - try to grab ->s_umount shared * @sb: reference we are trying to grab * * Try to prevent fs shutdown. This is used in places where we @@ -444,7 +444,7 @@ static int grab_super(struct super_block *s) __releases(sb_lock) * of down_read(). There's a couple of places that are OK with that, but * it's very much not a general-purpose interface. */ -bool trylock_super(struct super_block *sb) +bool super_trylock_shared(struct super_block *sb) { if (down_read_trylock(&sb->s_umount)) { if (!hlist_unhashed(&sb->s_instances) && @@ -1210,7 +1210,7 @@ EXPORT_SYMBOL(get_tree_keyed); * and the place that clears the pointer to the superblock used by this function * before freeing the superblock. */ -static bool lock_active_super(struct super_block *sb) +static bool super_lock_shared_active(struct super_block *sb) { super_lock_shared(sb); if (!sb->s_root || @@ -1228,7 +1228,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise) /* bd_holder_lock ensures that the sb isn't freed */ lockdep_assert_held(&bdev->bd_holder_lock); - if (!lock_active_super(sb)) + if (!super_lock_shared_active(sb)) return; if (!surprise) @@ -1247,7 +1247,7 @@ static void fs_bdev_sync(struct block_device *bdev) lockdep_assert_held(&bdev->bd_holder_lock); - if (!lock_active_super(sb)) + if (!super_lock_shared_active(sb)) return; sync_filesystem(sb); super_unlock_shared(sb); From 5e87491415217d6bec0bcae08a3156622be2b177 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 18 Aug 2023 16:00:50 +0200 Subject: [PATCH 45/47] super: wait for nascent superblocks Recent patches experiment with making it possible to allocate a new superblock before opening the relevant block device. Naturally this has intricate side-effects that we get to learn about while developing this. Superblock allocators such as sget{_fc}() return with s_umount of the new superblock held and lock ordering currently requires that block level locks such as bdev_lock and open_mutex rank above s_umount. Before aca740cecbe5 ("fs: open block device after superblock creation") ordering was guaranteed to be correct as block devices were opened prior to superblock allocation and thus s_umount wasn't held. But now s_umount must be dropped before opening block devices to avoid locking violations. This has consequences. The main one being that iterators over @super_blocks and @fs_supers that grab a temporary reference to the superblock can now also grab s_umount before the caller has managed to open block devices and called fill_super(). So whereas before such iterators or concurrent mounts would have simply slept on s_umount until SB_BORN was set or the superblock was discard due to initalization failure they can now needlessly spin through sget{_fc}(). If the caller is sleeping on bdev_lock or open_mutex one caller waiting on SB_BORN will always spin somewhere and potentially this can go on for quite a while. It should be possible to drop s_umount while allowing iterators to wait on a nascent superblock to either be born or discarded. This patch implements a wait_var_event() mechanism allowing iterators to sleep until they are woken when the superblock is born or discarded. This also allows us to avoid relooping through @fs_supers and @super_blocks if a superblock isn't yet born or dying. Link: aca740cecbe5 ("fs: open block device after superblock creation") Reviewed-by: Jan Kara Message-Id: <20230818-vfs-super-fixes-v3-v3-3-9f0b1876e46b@kernel.org> Signed-off-by: Christian Brauner --- fs/super.c | 204 +++++++++++++++++++++++++++++++++------------ include/linux/fs.h | 1 + 2 files changed, 154 insertions(+), 51 deletions(-) diff --git a/fs/super.c b/fs/super.c index ba5d813c5804..e2630fe4928a 100644 --- a/fs/super.c +++ b/fs/super.c @@ -50,7 +50,7 @@ static char *sb_writers_name[SB_FREEZE_LEVELS] = { "sb_internal", }; -static inline void super_lock(struct super_block *sb, bool excl) +static inline void __super_lock(struct super_block *sb, bool excl) { if (excl) down_write(&sb->s_umount); @@ -66,14 +66,9 @@ static inline void super_unlock(struct super_block *sb, bool excl) up_read(&sb->s_umount); } -static inline void super_lock_excl(struct super_block *sb) +static inline void __super_lock_excl(struct super_block *sb) { - super_lock(sb, true); -} - -static inline void super_lock_shared(struct super_block *sb) -{ - super_lock(sb, false); + __super_lock(sb, true); } static inline void super_unlock_excl(struct super_block *sb) @@ -86,6 +81,99 @@ static inline void super_unlock_shared(struct super_block *sb) super_unlock(sb, false); } +static inline bool wait_born(struct super_block *sb) +{ + unsigned int flags; + + /* + * Pairs with smp_store_release() in super_wake() and ensures + * that we see SB_BORN or SB_DYING after we're woken. + */ + flags = smp_load_acquire(&sb->s_flags); + return flags & (SB_BORN | SB_DYING); +} + +/** + * super_lock - wait for superblock to become ready and lock it + * @sb: superblock to wait for + * @excl: whether exclusive access is required + * + * If the superblock has neither passed through vfs_get_tree() or + * generic_shutdown_super() yet wait for it to happen. Either superblock + * creation will succeed and SB_BORN is set by vfs_get_tree() or we're + * woken and we'll see SB_DYING. + * + * The caller must have acquired a temporary reference on @sb->s_count. + * + * Return: This returns true if SB_BORN was set, false if SB_DYING was + * set. The function acquires s_umount and returns with it held. + */ +static __must_check bool super_lock(struct super_block *sb, bool excl) +{ + + lockdep_assert_not_held(&sb->s_umount); + +relock: + __super_lock(sb, excl); + + /* + * Has gone through generic_shutdown_super() in the meantime. + * @sb->s_root is NULL and @sb->s_active is 0. No one needs to + * grab a reference to this. Tell them so. + */ + if (sb->s_flags & SB_DYING) + return false; + + /* Has called ->get_tree() successfully. */ + if (sb->s_flags & SB_BORN) + return true; + + super_unlock(sb, excl); + + /* wait until the superblock is ready or dying */ + wait_var_event(&sb->s_flags, wait_born(sb)); + + /* + * Neither SB_BORN nor SB_DYING are ever unset so we never loop. + * Just reacquire @sb->s_umount for the caller. + */ + goto relock; +} + +/* wait and acquire read-side of @sb->s_umount */ +static inline bool super_lock_shared(struct super_block *sb) +{ + return super_lock(sb, false); +} + +/* wait and acquire write-side of @sb->s_umount */ +static inline bool super_lock_excl(struct super_block *sb) +{ + return super_lock(sb, true); +} + +/* wake waiters */ +#define SUPER_WAKE_FLAGS (SB_BORN | SB_DYING) +static void super_wake(struct super_block *sb, unsigned int flag) +{ + WARN_ON_ONCE((flag & ~SUPER_WAKE_FLAGS)); + WARN_ON_ONCE(hweight32(flag & SUPER_WAKE_FLAGS) > 1); + + /* + * Pairs with smp_load_acquire() in super_lock() to make sure + * all initializations in the superblock are seen by the user + * seeing SB_BORN sent. + */ + smp_store_release(&sb->s_flags, sb->s_flags | flag); + /* + * Pairs with the barrier in prepare_to_wait_event() to make sure + * ___wait_var_event() either sees SB_BORN set or + * waitqueue_active() check in wake_up_var() sees the waiter. + */ + smp_mb(); + wake_up_var(&sb->s_flags); +} + /* * One thing we have to be careful of with a per-sb shrinker is that we don't * drop the last active reference to the superblock from within the shrinker. @@ -393,7 +481,7 @@ EXPORT_SYMBOL(deactivate_locked_super); void deactivate_super(struct super_block *s) { if (!atomic_add_unless(&s->s_active, -1, 1)) { - super_lock_excl(s); + __super_lock_excl(s); deactivate_locked_super(s); } } @@ -415,10 +503,12 @@ EXPORT_SYMBOL(deactivate_super); */ static int grab_super(struct super_block *s) __releases(sb_lock) { + bool born; + s->s_count++; spin_unlock(&sb_lock); - super_lock_excl(s); - if ((s->s_flags & SB_BORN) && atomic_inc_not_zero(&s->s_active)) { + born = super_lock_excl(s); + if (born && atomic_inc_not_zero(&s->s_active)) { put_super(s); return 1; } @@ -447,8 +537,8 @@ static int grab_super(struct super_block *s) __releases(sb_lock) bool super_trylock_shared(struct super_block *sb) { if (down_read_trylock(&sb->s_umount)) { - if (!hlist_unhashed(&sb->s_instances) && - sb->s_root && (sb->s_flags & SB_BORN)) + if (!(sb->s_flags & SB_DYING) && sb->s_root && + (sb->s_flags & SB_BORN)) return true; super_unlock_shared(sb); } @@ -475,7 +565,7 @@ bool super_trylock_shared(struct super_block *sb) void retire_super(struct super_block *sb) { WARN_ON(!sb->s_bdev); - super_lock_excl(sb); + __super_lock_excl(sb); if (sb->s_iflags & SB_I_PERSB_BDI) { bdi_unregister(sb->s_bdi); sb->s_iflags &= ~SB_I_PERSB_BDI; @@ -557,6 +647,13 @@ void generic_shutdown_super(struct super_block *sb) /* should be initialized for __put_super_and_need_restart() */ hlist_del_init(&sb->s_instances); spin_unlock(&sb_lock); + /* + * Broadcast to everyone that grabbed a temporary reference to this + * superblock before we removed it from @fs_supers that the superblock + * is dying. Every walker of @fs_supers outside of sget{_fc}() will now + * discard this superblock and treat it as dead. + */ + super_wake(sb, SB_DYING); super_unlock_excl(sb); if (sb->s_bdi != &noop_backing_dev_info) { if (sb->s_iflags & SB_I_PERSB_BDI) @@ -631,6 +728,11 @@ retry: s->s_type = fc->fs_type; s->s_iflags |= fc->s_iflags; strscpy(s->s_id, s->s_type->name, sizeof(s->s_id)); + /* + * Make the superblock visible on @super_blocks and @fs_supers. + * It's in a nascent state and users should wait on SB_BORN or + * SB_DYING to be set. + */ list_add_tail(&s->s_list, &super_blocks); hlist_add_head(&s->s_instances, &s->s_type->fs_supers); spin_unlock(&sb_lock); @@ -740,7 +842,8 @@ static void __iterate_supers(void (*f)(struct super_block *)) spin_lock(&sb_lock); list_for_each_entry(sb, &super_blocks, s_list) { - if (hlist_unhashed(&sb->s_instances)) + /* Pairs with memory marrier in super_wake(). */ + if (smp_load_acquire(&sb->s_flags) & SB_DYING) continue; sb->s_count++; spin_unlock(&sb_lock); @@ -770,13 +873,13 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg) spin_lock(&sb_lock); list_for_each_entry(sb, &super_blocks, s_list) { - if (hlist_unhashed(&sb->s_instances)) - continue; + bool born; + sb->s_count++; spin_unlock(&sb_lock); - super_lock_shared(sb); - if (sb->s_root && (sb->s_flags & SB_BORN)) + born = super_lock_shared(sb); + if (born && sb->s_root) f(sb, arg); super_unlock_shared(sb); @@ -806,11 +909,13 @@ void iterate_supers_type(struct file_system_type *type, spin_lock(&sb_lock); hlist_for_each_entry(sb, &type->fs_supers, s_instances) { + bool born; + sb->s_count++; spin_unlock(&sb_lock); - super_lock_shared(sb); - if (sb->s_root && (sb->s_flags & SB_BORN)) + born = super_lock_shared(sb); + if (born && sb->s_root) f(sb, arg); super_unlock_shared(sb); @@ -841,14 +946,11 @@ struct super_block *get_active_super(struct block_device *bdev) if (!bdev) return NULL; -restart: spin_lock(&sb_lock); list_for_each_entry(sb, &super_blocks, s_list) { - if (hlist_unhashed(&sb->s_instances)) - continue; if (sb->s_bdev == bdev) { if (!grab_super(sb)) - goto restart; + return NULL; super_unlock_excl(sb); return sb; } @@ -862,22 +964,21 @@ struct super_block *user_get_super(dev_t dev, bool excl) struct super_block *sb; spin_lock(&sb_lock); -rescan: list_for_each_entry(sb, &super_blocks, s_list) { - if (hlist_unhashed(&sb->s_instances)) - continue; if (sb->s_dev == dev) { + bool born; + sb->s_count++; spin_unlock(&sb_lock); - super_lock(sb, excl); /* still alive? */ - if (sb->s_root && (sb->s_flags & SB_BORN)) + born = super_lock(sb, excl); + if (born && sb->s_root) return sb; super_unlock(sb, excl); /* nope, got unmounted */ spin_lock(&sb_lock); __put_super(sb); - goto rescan; + break; } } spin_unlock(&sb_lock); @@ -921,7 +1022,7 @@ int reconfigure_super(struct fs_context *fc) if (!hlist_empty(&sb->s_pins)) { super_unlock_excl(sb); group_pin_kill(&sb->s_pins); - super_lock_excl(sb); + __super_lock_excl(sb); if (!sb->s_root) return 0; if (sb->s_writers.frozen != SB_UNFROZEN) @@ -984,9 +1085,9 @@ cancel_readonly: static void do_emergency_remount_callback(struct super_block *sb) { - super_lock_excl(sb); - if (sb->s_root && sb->s_bdev && (sb->s_flags & SB_BORN) && - !sb_rdonly(sb)) { + bool born = super_lock_excl(sb); + + if (born && sb->s_root && sb->s_bdev && !sb_rdonly(sb)) { struct fs_context *fc; fc = fs_context_for_reconfigure(sb->s_root, @@ -1020,8 +1121,9 @@ void emergency_remount(void) static void do_thaw_all_callback(struct super_block *sb) { - super_lock_excl(sb); - if (sb->s_root && sb->s_flags & SB_BORN) { + bool born = super_lock_excl(sb); + + if (born && sb->s_root) { emergency_thaw_bdev(sb); thaw_super_locked(sb); } else { @@ -1212,9 +1314,9 @@ EXPORT_SYMBOL(get_tree_keyed); */ static bool super_lock_shared_active(struct super_block *sb) { - super_lock_shared(sb); - if (!sb->s_root || - (sb->s_flags & (SB_ACTIVE | SB_BORN)) != (SB_ACTIVE | SB_BORN)) { + bool born = super_lock_shared(sb); + + if (!born || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) { super_unlock_shared(sb); return false; } @@ -1374,7 +1476,7 @@ int get_tree_bdev(struct fs_context *fc, */ super_unlock_excl(s); error = setup_bdev_super(s, fc->sb_flags, fc); - super_lock_excl(s); + __super_lock_excl(s); if (!error) error = fill_super(s, fc); if (error) { @@ -1426,7 +1528,7 @@ struct dentry *mount_bdev(struct file_system_type *fs_type, */ super_unlock_excl(s); error = setup_bdev_super(s, flags, NULL); - super_lock_excl(s); + __super_lock_excl(s); if (!error) error = fill_super(s, data, flags & SB_SILENT ? 1 : 0); if (error) { @@ -1566,13 +1668,13 @@ int vfs_get_tree(struct fs_context *fc) WARN_ON(!sb->s_bdi); /* - * Write barrier is for super_cache_count(). We place it before setting - * SB_BORN as the data dependency between the two functions is the - * superblock structure contents that we just set up, not the SB_BORN - * flag. + * super_wake() contains a memory barrier which also care of + * ordering for super_cache_count(). We place it before setting + * SB_BORN as the data dependency between the two functions is + * the superblock structure contents that we just set up, not + * the SB_BORN flag. */ - smp_wmb(); - sb->s_flags |= SB_BORN; + super_wake(sb, SB_BORN); error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL); if (unlikely(error)) { @@ -1715,7 +1817,7 @@ int freeze_super(struct super_block *sb) int ret; atomic_inc(&sb->s_active); - super_lock_excl(sb); + __super_lock_excl(sb); if (sb->s_writers.frozen != SB_UNFROZEN) { deactivate_locked_super(sb); return -EBUSY; @@ -1737,7 +1839,7 @@ int freeze_super(struct super_block *sb) /* Release s_umount to preserve sb_start_write -> s_umount ordering */ super_unlock_excl(sb); sb_wait_write(sb, SB_FREEZE_WRITE); - super_lock_excl(sb); + __super_lock_excl(sb); /* Now we go and block page faults... */ sb->s_writers.frozen = SB_FREEZE_PAGEFAULT; @@ -1820,7 +1922,7 @@ out: */ int thaw_super(struct super_block *sb) { - super_lock_excl(sb); + __super_lock_excl(sb); return thaw_super_locked(sb); } EXPORT_SYMBOL(thaw_super); diff --git a/include/linux/fs.h b/include/linux/fs.h index 14b5777a24a0..173672645156 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1095,6 +1095,7 @@ extern int send_sigurg(struct fown_struct *fown); #define SB_LAZYTIME BIT(25) /* Update the on-disk [acm]times lazily */ /* These sb flags are internal to the kernel */ +#define SB_DYING BIT(24) #define SB_SUBMOUNT BIT(26) #define SB_FORCE BIT(27) #define SB_NOSEC BIT(28) From 2c18a63b760a0f68f14cb8bb4c3840bb0b63b73e Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 18 Aug 2023 16:00:51 +0200 Subject: [PATCH 46/47] super: wait until we passed kill super Recent rework moved block device closing out of sb->put_super() and into sb->kill_sb() to avoid deadlocks as s_umount is held in put_super() and blkdev_put() can end up taking s_umount again. That means we need to move the removal of the superblock from @fs_supers out of generic_shutdown_super() and into deactivate_locked_super() to ensure that concurrent mounters don't fail to open block devices that are still in use because blkdev_put() in sb->kill_sb() hasn't been called yet. We can now do this as we can make iterators through @fs_super and @super_blocks wait without holding s_umount. Concurrent mounts will wait until a dying superblock is fully dead so until sb->kill_sb() has been called and SB_DEAD been set. Concurrent iterators can already discard any SB_DYING superblock. Reviewed-by: Jan Kara Message-Id: <20230818-vfs-super-fixes-v3-v3-4-9f0b1876e46b@kernel.org> Signed-off-by: Christian Brauner --- fs/super.c | 71 +++++++++++++++++++++++++++++++++++++++++----- include/linux/fs.h | 1 + 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/fs/super.c b/fs/super.c index e2630fe4928a..2f604a6494fb 100644 --- a/fs/super.c +++ b/fs/super.c @@ -153,7 +153,7 @@ static inline bool super_lock_excl(struct super_block *sb) } /* wake waiters */ -#define SUPER_WAKE_FLAGS (SB_BORN | SB_DYING) +#define SUPER_WAKE_FLAGS (SB_BORN | SB_DYING | SB_DEAD) static void super_wake(struct super_block *sb, unsigned int flag) { WARN_ON_ONCE((flag & ~SUPER_WAKE_FLAGS)); @@ -461,6 +461,25 @@ void deactivate_locked_super(struct super_block *s) list_lru_destroy(&s->s_dentry_lru); list_lru_destroy(&s->s_inode_lru); + /* + * Remove it from @fs_supers so it isn't found by new + * sget{_fc}() walkers anymore. Any concurrent mounter still + * managing to grab a temporary reference is guaranteed to + * already see SB_DYING and will wait until we notify them about + * SB_DEAD. + */ + spin_lock(&sb_lock); + hlist_del_init(&s->s_instances); + spin_unlock(&sb_lock); + + /* + * Let concurrent mounts know that this thing is really dead. + * We don't need @sb->s_umount here as every concurrent caller + * will see SB_DYING and either discard the superblock or wait + * for SB_DEAD. + */ + super_wake(s, SB_DEAD); + put_filesystem(fs); put_super(s); } else { @@ -517,6 +536,45 @@ static int grab_super(struct super_block *s) __releases(sb_lock) return 0; } +static inline bool wait_dead(struct super_block *sb) +{ + unsigned int flags; + + /* + * Pairs with memory barrier in super_wake() and ensures + * that we see SB_DEAD after we're woken. + */ + flags = smp_load_acquire(&sb->s_flags); + return flags & SB_DEAD; +} + +/** + * grab_super_dead - acquire an active reference to a superblock + * @sb: superblock to acquire + * + * Acquire a temporary reference on a superblock and try to trade it for + * an active reference. This is used in sget{_fc}() to wait for a + * superblock to either become SB_BORN or for it to pass through + * sb->kill() and be marked as SB_DEAD. + * + * Return: This returns true if an active reference could be acquired, + * false if not. + */ +static bool grab_super_dead(struct super_block *sb) +{ + + sb->s_count++; + if (grab_super(sb)) { + put_super(sb); + lockdep_assert_held(&sb->s_umount); + return true; + } + wait_var_event(&sb->s_flags, wait_dead(sb)); + put_super(sb); + lockdep_assert_not_held(&sb->s_umount); + return false; +} + /* * super_trylock_shared - try to grab ->s_umount shared * @sb: reference we are trying to grab @@ -643,15 +701,14 @@ void generic_shutdown_super(struct super_block *sb) spin_unlock(&sb->s_inode_list_lock); } } - spin_lock(&sb_lock); - /* should be initialized for __put_super_and_need_restart() */ - hlist_del_init(&sb->s_instances); - spin_unlock(&sb_lock); /* * Broadcast to everyone that grabbed a temporary reference to this * superblock before we removed it from @fs_supers that the superblock * is dying. Every walker of @fs_supers outside of sget{_fc}() will now * discard this superblock and treat it as dead. + * + * We leave the superblock on @fs_supers so it can be found by + * sget{_fc}() until we passed sb->kill_sb(). */ super_wake(sb, SB_DYING); super_unlock_excl(sb); @@ -746,7 +803,7 @@ share_extant_sb: destroy_unused_super(s); return ERR_PTR(-EBUSY); } - if (!grab_super(old)) + if (!grab_super_dead(old)) goto retry; destroy_unused_super(s); return old; @@ -790,7 +847,7 @@ retry: destroy_unused_super(s); return ERR_PTR(-EBUSY); } - if (!grab_super(old)) + if (!grab_super_dead(old)) goto retry; destroy_unused_super(s); return old; diff --git a/include/linux/fs.h b/include/linux/fs.h index 173672645156..a63da68305e9 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1095,6 +1095,7 @@ extern int send_sigurg(struct fown_struct *fown); #define SB_LAZYTIME BIT(25) /* Update the on-disk [acm]times lazily */ /* These sb flags are internal to the kernel */ +#define SB_DEAD BIT(21) #define SB_DYING BIT(24) #define SB_SUBMOUNT BIT(26) #define SB_FORCE BIT(27) From 051178c366bbc1bf8b4aba5ca5519d7da453c95f Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Tue, 22 Aug 2023 13:32:50 +0200 Subject: [PATCH 47/47] super: use higher-level helper for {freeze,thaw} It's not necessary to use low-level locking helpers here. Use the higher-level locking helpers and log if the superblock is dying. Since the caller is assumed to already hold an active reference it isn't possible to observe a dying superblock. Suggested-by: Jan Kara Signed-off-by: Christian Brauner --- fs/super.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/fs/super.c b/fs/super.c index 2f604a6494fb..a284052ebab2 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1873,8 +1873,13 @@ int freeze_super(struct super_block *sb) { int ret; + /* Since the caller must already have an active reference... */ atomic_inc(&sb->s_active); - __super_lock_excl(sb); + + /* ...@sb definitely can't be dying. */ + if (!super_lock_excl(sb)) + WARN(1, "Dying superblock while freezing!"); + if (sb->s_writers.frozen != SB_UNFROZEN) { deactivate_locked_super(sb); return -EBUSY; @@ -1896,7 +1901,10 @@ int freeze_super(struct super_block *sb) /* Release s_umount to preserve sb_start_write -> s_umount ordering */ super_unlock_excl(sb); sb_wait_write(sb, SB_FREEZE_WRITE); - __super_lock_excl(sb); + + /* We're still holding an active reference. */ + if (!super_lock_excl(sb)) + WARN(1, "Dying superblock while freezing!"); /* Now we go and block page faults... */ sb->s_writers.frozen = SB_FREEZE_PAGEFAULT; @@ -1979,7 +1987,8 @@ out: */ int thaw_super(struct super_block *sb) { - __super_lock_excl(sb); + if (!super_lock_excl(sb)) + WARN(1, "Dying superblock while thawing!"); return thaw_super_locked(sb); } EXPORT_SYMBOL(thaw_super);