[PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[Jan Kara]

From: Christoph Hellwig <hch@lst.de>

The file system type is not a very useful holder as it doesn't allow us
to go back to the actual file system instance.  Pass the super_block
instead which is useful when passed back to the file system driver.

This matches what is done for all other block device based file systems and it
also fixes an issue that block device freezing (as used e.g. by LVM when
performing device snapshots) starts working for btrfs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Christian Brauner <brauner@kernel.org>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Message-Id: <20230811100828.1897174-7-hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/btrfs/super.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Hello,

I'm resending this btrfs fix. Can you please merge it David? It's the only bit
remaining from the original Christoph's block device opening patches and is
blocking me in pushing out the opening of block devices using bdev_handle.
Thanks!

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 4577cd64da2e..94ca2236e57f 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -70,8 +70,6 @@ static const struct super_operations btrfs_super_ops;
  * requested by subvol=/path. That way the callchain is straightforward and we
  * don't have to play tricks with the mount options and recursive calls to
  * btrfs_mount.
- *
- * The new btrfs_root_fs_type also servers as a tag for the bdev_holder.
  */
 static struct file_system_type btrfs_fs_type;
 static struct file_system_type btrfs_root_fs_type;
@@ -1462,8 +1460,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 
 		mutex_lock(&uuid_mutex);
-		error = btrfs_open_devices(fs_devices, sb_open_mode(flags),
-					   fs_type);
+		error = btrfs_open_devices(fs_devices, sb_open_mode(flags), s);
 		mutex_unlock(&uuid_mutex);
 		if (error)
 			goto error_deactivate;
@@ -1477,7 +1474,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 			 fs_devices->latest_dev->bdev);
 		shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", fs_type->name,
 					s->s_id);
-		btrfs_sb(s)->bdev_holder = fs_type;
+		btrfs_sb(s)->bdev_holder = s;
 		error = btrfs_fill_super(s, fs_devices, data);
 	}
 	if (!error)
-- 
2.35.3

Re: [PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[Christian Brauner]

On Thu, Sep 21, 2023 at 02:19:45PM +0200, Jan Kara wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> The file system type is not a very useful holder as it doesn't allow us
> to go back to the actual file system instance.  Pass the super_block
> instead which is useful when passed back to the file system driver.
> 
> This matches what is done for all other block device based file systems and it
> also fixes an issue that block device freezing (as used e.g. by LVM when
> performing device snapshots) starts working for btrfs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Christian Brauner <brauner@kernel.org>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Message-Id: <20230811100828.1897174-7-hch@lst.de>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/btrfs/super.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> Hello,
> 
> I'm resending this btrfs fix. Can you please merge it David? It's the only bit
> remaining from the original Christoph's block device opening patches and is
> blocking me in pushing out the opening of block devices using bdev_handle.
> Thanks!

Thanks for resending.

Next time we will ensure that a vfs triggered conversion must go through
a vfs tree as this half converted state with forgotten patches is not
something that we should repeat.

Christian

Re: [PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[Jan Kara]

On Thu 21-09-23 14:50:07, Christian Brauner wrote:
> On Thu, Sep 21, 2023 at 02:19:45PM +0200, Jan Kara wrote:
> > From: Christoph Hellwig <hch@lst.de>
> > 
> > The file system type is not a very useful holder as it doesn't allow us
> > to go back to the actual file system instance.  Pass the super_block
> > instead which is useful when passed back to the file system driver.
> > 
> > This matches what is done for all other block device based file systems and it
> > also fixes an issue that block device freezing (as used e.g. by LVM when
> > performing device snapshots) starts working for btrfs.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Acked-by: Christian Brauner <brauner@kernel.org>
> > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> > Message-Id: <20230811100828.1897174-7-hch@lst.de>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/btrfs/super.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > Hello,
> > 
> > I'm resending this btrfs fix. Can you please merge it David? It's the only bit
> > remaining from the original Christoph's block device opening patches and is
> > blocking me in pushing out the opening of block devices using bdev_handle.
> > Thanks!
> 
> Thanks for resending.
> 
> Next time we will ensure that a vfs triggered conversion must go through
> a vfs tree as this half converted state with forgotten patches is not
> something that we should repeat.

Yeah, I agree it would be good to find a smoother way to handle such
merges. I understand David's desire to give changes proper testing which
generally doesn't happen in linux-next but on maintainer's own branches and
infrastructure but perhaps some stable branch in VFS tree that filesystem
maintainers could pull when putting together the branch they push out to
the testing infrastructure would work?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[Christian Brauner]

On Thu, Sep 21, 2023 at 03:07:29PM +0200, Jan Kara wrote:
> On Thu 21-09-23 14:50:07, Christian Brauner wrote:
> > On Thu, Sep 21, 2023 at 02:19:45PM +0200, Jan Kara wrote:
> > > From: Christoph Hellwig <hch@lst.de>
> > > 
> > > The file system type is not a very useful holder as it doesn't allow us
> > > to go back to the actual file system instance.  Pass the super_block
> > > instead which is useful when passed back to the file system driver.
> > > 
> > > This matches what is done for all other block device based file systems and it
> > > also fixes an issue that block device freezing (as used e.g. by LVM when
> > > performing device snapshots) starts working for btrfs.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > Acked-by: Christian Brauner <brauner@kernel.org>
> > > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> > > Message-Id: <20230811100828.1897174-7-hch@lst.de>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/btrfs/super.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > 
> > > Hello,
> > > 
> > > I'm resending this btrfs fix. Can you please merge it David? It's the only bit
> > > remaining from the original Christoph's block device opening patches and is
> > > blocking me in pushing out the opening of block devices using bdev_handle.
> > > Thanks!
> > 
> > Thanks for resending.
> > 
> > Next time we will ensure that a vfs triggered conversion must go through
> > a vfs tree as this half converted state with forgotten patches is not
> > something that we should repeat.
> 
> Yeah, I agree it would be good to find a smoother way to handle such
> merges. I understand David's desire to give changes proper testing which
> generally doesn't happen in linux-next but on maintainer's own branches and
> infrastructure but perhaps some stable branch in VFS tree that filesystem
> maintainers could pull when putting together the branch they push out to
> the testing infrastructure would work?

Yes, I'm very happy to provide this. Also worth nothing that everything
that goes through vfs.git sees xfstests for all affected filesystems.

Ideally we'll automate this directly on that repo at some point for all
official branches.

Right now I personally have:

https://github.com/brauner/mkosi-kernel

which uses mkosi to build a GPT image fully unprivileged with an:

ESP partition
root partition (of chosen format)
2-4 testing partitions

for any distro I need with any kernel config I want:

mkosi -d debian -t disk -f

During boot it generates a bunch of unprivileged users via
systemd-userdb drop-ins placed in /etc/userdb/ during build:
https://github.com/brauner/mkosi-kernel/tree/main/mkosi.extra/etc/userdb
creating testing users (user1, fsgqa, fgqa2, 123456-fsgqa) for xfstests.

Then I just fire the relevant tests using incus (formerly LXD). All
branches get the same treatment usually for ext4, xfs, btrfs via
-g quick (btrfs gets multiple devices - that's why there's 4 partitions).

Re: [PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[David Sterba]

On Thu, Sep 21, 2023 at 02:50:07PM +0200, Christian Brauner wrote:
> On Thu, Sep 21, 2023 at 02:19:45PM +0200, Jan Kara wrote:
> > From: Christoph Hellwig <hch@lst.de>
> > 
> > The file system type is not a very useful holder as it doesn't allow us
> > to go back to the actual file system instance.  Pass the super_block
> > instead which is useful when passed back to the file system driver.
> > 
> > This matches what is done for all other block device based file systems and it
> > also fixes an issue that block device freezing (as used e.g. by LVM when
> > performing device snapshots) starts working for btrfs.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Acked-by: Christian Brauner <brauner@kernel.org>
> > Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> > Message-Id: <20230811100828.1897174-7-hch@lst.de>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/btrfs/super.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > Hello,
> > 
> > I'm resending this btrfs fix. Can you please merge it David? It's the only bit
> > remaining from the original Christoph's block device opening patches and is
> > blocking me in pushing out the opening of block devices using bdev_handle.
> > Thanks!
> 
> Thanks for resending.
> 
> Next time we will ensure that a vfs triggered conversion must go through
> a vfs tree as this half converted state with forgotten patches is not
> something that we should repeat.

Taking this as an example, I really don't want to let such patches go
through VFS git. I understand there are API-level cleanups done in
VFS that require reorganizing code in the filesystems but IMO the
conversions must be done in the filesystems first and the VFS cleanups
as a followup.

We have conflicting goals, you want better API and filesystems stand in
the way so you/somebody fix it in a seemingly correct way and you want
to merge it because it's ok for you. I view it as change from the
outside potentially introducing bugs that we'll have to deal and unless
we have somebody who's familiar with the changes to recognize the
problems and fix bugs eventually it's risky.

What I can do right now is that I'll keep the patches in linux-next,
with the additional fix so the series is complete. I'll let you know if
there's a change regarding the reviews or merge.

Re: [PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[Christian Brauner]

> > Next time we will ensure that a vfs triggered conversion must go through
> > a vfs tree as this half converted state with forgotten patches is not
> > something that we should repeat.
> 
> Taking this as an example, I really don't want to let such patches go
> through VFS git. I understand there are API-level cleanups done in
> VFS that require reorganizing code in the filesystems but IMO the
> conversions must be done in the filesystems first and the VFS cleanups
> as a followup.

Whether or not this order is possible depends on the patchset. If we do
VFS level conversions that impact all filesystems we will do changes to
all filesystems in one go if the changes need to be done all at the same
time as we have done many times in the past. There's really nothing new.

> there's a change regarding the reviews or merge.

We'll likely end pulling in the required btrfs changes from you anyway
so Jan's series isn't blocked indefinitely from getting testing in
-next.

Re: [PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[David Sterba]

On Thu, Sep 21, 2023 at 02:19:45PM +0200, Jan Kara wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> The file system type is not a very useful holder as it doesn't allow us
> to go back to the actual file system instance.  Pass the super_block
> instead which is useful when passed back to the file system driver.
> 
> This matches what is done for all other block device based file systems and it
> also fixes an issue that block device freezing (as used e.g. by LVM when
> performing device snapshots) starts working for btrfs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Christian Brauner <brauner@kernel.org>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Message-Id: <20230811100828.1897174-7-hch@lst.de>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/btrfs/super.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> Hello,
> 
> I'm resending this btrfs fix. Can you please merge it David? It's the only bit
> remaining from the original Christoph's block device opening patches and is
> blocking me in pushing out the opening of block devices using bdev_handle.

I'll add the patch to the rest of the series, thanks. I don't have a
timeframe when this will be moved to the main patch queue, until then
it'll be in linux-next. The series has been sitting in the review
backlog for a few weeks but no takers. I try to have a look from time
to time but the kind of changes done there are not trivial so I'm still
not confident enough to put to the main patch queue.

Re: [PATCH RESEND] btrfs: use the super_block as holder when mounting file systems

[Christian Brauner]

On Mon, Sep 25, 2023 at 05:54:09PM +0200, David Sterba wrote:
> > Reviewed-by: Josef Bacik <josef@toxicpanda.com>

> it'll be in linux-next. The series has been sitting in the review
> backlog for a few weeks but no takers. I try to have a look from time

I don't understand isn't Josef one of your reviewers/maintainers? Who is
the person that must review it?