On Fri, Sep 08, 2023 at 11:41:40AM +0200, Christian Brauner wrote: > Hey everyone, > > I have a patch series unrelated to btrfs that moves block device > freezing and thawing to block device holder operations - Jan and > Christoph are aware. As part of that I took a look at various freezing > implementations to make sure that there are no regressions and that I'm > testing correctly. > > So what puzzled me with btrfs is that freezing operations triggered > through freeze_bdev() seem broken. > > For example, triggering a freeze through dm_ioctl() would currently do: > > freeze_bdev() > -> get_active_super() > -> sb->freeze_fs() > > And get_active_super() (which will go away with my patch series) walks > all super blocks on the systems and matches on sb->s_bdev to find any > superblock associated with that device. But afaict - at least on a > regular mount - btrfs doesn't set that pointer to anything right now. > Eesh, no you're right, seems like we only set this when we're moving devices around, so it must have gotten removed at some point. > IOW, get_active_super() can never find the btrfs superblock that is > associated with that device mapper device (sticking with the example). > That means while we freeze the underlying block device the btrfs > filesystem making use of that block device isn't. > > Is that known/expected? Am I missing something else why that's ok? Or am > I misanalysing? Probably not a very common use-case/scenario but still. > Nope this is for sure unexpected and a bug. > I'm pretty sure this would be fixable with my series. It just requires > that btrfs would finally move to the new model where bdev->bd_holder is > set to the superblock instead of the filesystem type and would start > using fs_holder_ops if that's possible. > > Because implementing block device freeze/thaw as holder operations > wouldn't need to match on s_bdev anymore at all. It can go straight from > bdev->bd_holder to the superblock and call the necessary ops. > > My series can proceed independent of fixing btrfs but I'm just trying to > make people aware in case that somehow wasn't known. Thanks for that, we definitely need to get this fixed. Is the bdev->bd_holder part of the new mount api, or is it some other thing that we can do right now and then be in a good spot when your new patchset lands? Let me know and we can prioritize that work. Thanks, Josef
