Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

Al Viro
2025-05-05T19:21:13Z

On Mon, May 05, 2025 at 07:58:07PM +0200, David Sterba wrote:

> > - if (fc->sb_flags & SB_RDONLY)
> > - return ret;
> > -
> > - down_write(&mnt->mnt_sb->s_umount);
> > - if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
> > + if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
> > ret = btrfs_reconfigure(fc);
> > - up_write(&mnt->mnt_sb->s_umount);

> So this open codes fc_mount(), which is vfs_get_tree() + vfs_create_mount(),
> the only difference I see in the new code is that
> btrfs_reconfigure_for_mount() dropped the SB_RDONLY check.
>
> Why the check is there is explained in the lengthy comment above
> btrfs_reconfigure_for_mount(), so it should stay. If it can be removed
> then it should be a separate patch from the cleanup.

What do you mean, dropped? It's still right there - the current
variant checks it *twice*, once before grabbing ->s_umount, then
after it's been grabbed. Checking it before down_write() makes sense
if we are called after ->s_umount had been dropped (by fc_mount()).
I'm not sure why you recheck it after down_write(), since it's not
going to change, but you do recheck it. In this variant we don't need
to bother grabbing the rwsem, since that thing is called while ->s_umount
is still held...

I can turn that into
if (fc->sb_flags & SB_RDONLY)
return ret;
if (fc->root->d_sb->s_flags & SB_RDONLY)
ret = btrfs_reconfigure(fc);
return ret;
but I don't see how it's better than the variant posted; up to you, of course...