On Thu, Nov 30, 2023 at 09:00:06PM -0500, Kent Overstreet wrote: > On Thu, Nov 30, 2023 at 02:17:11PM -0500, Brian Foster wrote: > > bcachefs grabs s_umount and sets SB_RDONLY when the fs is shutdown > > via the ioctl() interface. This has a couple issues related to > > interactions between shutdown and freeze: > > > > 1. The flags == FSOP_GOING_FLAGS_DEFAULT case is a deadlock vector > > because freeze_bdev() calls into freeze_super(), which also > > acquires s_umount. > > > > 2. If an explicit shutdown occurs while the sb is frozen, SB_RDONLY > > alters the thaw path as if the sb was read-only at freeze time. > > This effectively leaks the frozen state and leaves the sb frozen > > indefinitely. > > > > The usage of SB_RDONLY here goes back to the initial bcachefs commit > > and AFAICT is simply historical behavior. This behavior is unique to > > bcachefs relative to the handful of other filesystems that support > > the shutdown ioctl(). Typically, SB_RDONLY is reserved for the > > proper remount path, which itself is restricted from modifying > > frozen superblocks in reconfigure_super(). Drop the unnecessary sb > > lock and flags update bch2_ioc_goingdown() to address both of these > > issues. > > Nice, I noticed a deadlock issue recently with s_umount here but didn't > have time to fully debug it - applied! >
Thanks! Hmm.. got a reference to that observed deadlock handy? I'd be a little cautious associating here because 1. I dont think any test tooling currently uses the default shutdown mode for the ioctl and 2. the non-ioctl emergency shutdown paths dont set SB_RDONLY, so shouldn't trigger the freeze issue. So IOW while this path was broken in the ways described in the commit log, it still seems rather unlikely for a normal use or test case to trigger this. It would be good to know if there's still a lingering issue to resolve.. Brian
