Re: btrfs freezing question

[Josef Bacik <josef@toxicpanda.com>]

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