Re: [PATCH] bcachefs: remove sb lock and flags update on explicit shutdown

[Brian Foster <bfoster@redhat.com>]

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