btrfs freezing question

btrfs freezing question

[Christian Brauner]

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.

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.

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.

Christian

Re: btrfs freezing question

[Josef Bacik]

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

Re: btrfs freezing question

[Christian Brauner]

> 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.

Thanks for taking a look, Josef!

> > 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,

This is independent of converting btrfs to the new mount api.

Christoph has landed a patch series this cycle that sets bdev->bd_holder
to the superblock owning the block device. Before his work
bdev->bd_holder was set to the filesystem type. Roughly, this has
enabled us to go from a block device straight to the owning superblock.

Associated with changing bdev->bd_holder is using a set of holder
operations: fs_holder_ops (see fs/super.c). These can be used to perform
various operations on the holder: in this case on the superblock.

IOW, we don't need to find the owning superblock anymore we just need to
go straight from block device to owning superblock. You can see this
e.g., in block/bdev.c:bdev_mark_dead() = fs/super.c:fs_bdev_mark_dead().

What we need is to switch btrfs to bdev->bd_holder = sb and the usage of
fs/super.c:fs_holder_ops if possible. We did this anyway to avoid
deadlocks, allow dropping of s_umount when opening block devices and
ultimately we can find matching superblocks before opening block devices
which will allow us to restrict writers to block devices in v6.7 (Jan's
work).

We converted all block-based filesystems to use this new mechanism but
David insisted on taking the btrfs portion through btrfs itself. So none
of the btrfs patches in the series linked below have made it upstream
for v6.6 and so you're unfortunately the only fs which still uses the
old mechanism:

Subject: [PATCH 05/17] btrfs: open block devices after superblock
https://lore.kernel.org/linux-block/20230811100828.1897174-6-hch@lst.de

What would help is if you would start to take these patches.