Re: [PATCH v2 12/18] btrfs: add get_tree callback for new mount API

David Sterba
2023-11-14T18:43:00Z

On Wed, Nov 08, 2023 at 02:08:47PM -0500, Josef Bacik wrote:
> This is the actual mounting callback for the new mount API. Implement
> this using our current fill super as a guideline, making the appropriate
> adjustments for the new mount API.
>
> Our old mount operation had two fs_types, one to handle the actual
> opening, and the one that we called to handle the actual opening and
> then did the subvol lookup for returning the actual root dentry. This
> is mirrored here, but simply with different behaviors for ->get_tree.
> We use the existence of ->s_fs_info to tell which part we're in. The
> initial call allocates the fs_info, then call mount_fc() with a
> duplicated fc to do the actual open_ctree part. Then we take that
> vfsmount and use it to look up our subvolume that we're mounting and
> return that as our s_root. This idea was taken from Christians attempt
> to convert us to the new mount api.
>
> References: https://lore.kernel.org/all/20230626-fs-btrfs-mount-api-v1-2-045e9735a00b@kernel.org/
> Reviewed-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> fs/btrfs/super.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 206 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index b5067cf637a2..4ace42e08bff 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -95,6 +95,7 @@ struct btrfs_fs_context {
> unsigned long mount_opt;
> unsigned long compress_type:4;
> unsigned int compress_level;
> + refcount_t refs;
> };
>
> enum {
> @@ -2833,6 +2834,181 @@ static int btrfs_statfs(struct dentry *dentry, struct kstatfs *buf)
> return 0;
> }
>
> +static int btrfs_fc_test_super(struct super_block *s, struct fs_context *fc)
> +{
> + struct btrfs_fs_info *p = fc->s_fs_info;

That's a confusing variable name

> + struct btrfs_fs_info *fs_info = btrfs_sb(s);
> +
> + return fs_info->fs_devices == p->fs_devices;
> +}
> +
> +static int btrfs_get_tree_super(struct fs_context *fc)
> +{
> + struct btrfs_fs_info *fs_info = fc->s_fs_info;
> + struct btrfs_fs_context *ctx = fc->fs_private;
> + struct btrfs_fs_devices *fs_devices = NULL;
> + struct block_device *bdev;
> + struct btrfs_device *device;
> + struct super_block *s;

Please use 'sb' for super block.

> + blk_mode_t mode = sb_open_mode(fc->sb_flags);
> + int ret;
> +
> + btrfs_ctx_to_info(fs_info, ctx);
> + mutex_lock(&uuid_mutex);
> +
> + /*
> + * With 'true' passed to btrfs_scan_one_device() (mount time) we expect
> + * either a valid device or an error.
> + */
> + device = btrfs_scan_one_device(fc->source, mode, true);
> + ASSERT(device != NULL);
> + if (IS_ERR(device)) {
> + mutex_unlock(&uuid_mutex);
> + return PTR_ERR(device);
> + }
> +
> + fs_devices = device->fs_devices;
> + fs_info->fs_devices = fs_devices;
> +
> + ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
> + mutex_unlock(&uuid_mutex);

Regarding the previous comments about mount and scanning, here the
device is scanned and opened in one go, so all the other devices are
expected to be scanned independently from before.

This is not a prolbem, although it allows to something race in between
the mount option scanning and here and call 'forget' on the devices.
We've seen udev to race with mkfs to register the device, which is not a
problem here, but if there's something calling 'forget' automatically
then it will be.

Since systemd started to mess with background mounts and scans we can
never ber sure what's going to happen when triggered by system events.

> + if (ret)
> + return ret;
> +
> + if (!(fc->sb_flags & SB_RDONLY) && fs_devices->rw_devices == 0) {
> + ret = -EACCES;
> + goto error;
> + }
> +
> + bdev = fs_devices->latest_dev->bdev;
> +
> + /*
> + * If successful, this will transfer the fs_info into the super block,
> + * and fc->s_fs_info will be NULL. However if there's an existing
> + * super, we'll still have fc->s_fs_info populated. If we error
> + * completely out it'll be cleaned up when we drop the fs_context,
> + * otherwise it's tied to the lifetime of the super_block.
> + *
> + * Adding this comment because I was horribly confused about the error
> + * handling from here on out.

The last sentence does not need to be there, that we add comments to
avoid confusion is kind of implicit.

> + */
> + s = sget_fc(fc, btrfs_fc_test_super, set_anon_super_fc);
> + if (IS_ERR(s)) {
> + ret = PTR_ERR(s);
> + goto error;
> + }
> +
> + if (s->s_root) {
> + btrfs_close_devices(fs_devices);
> + if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY)
> + ret = -EBUSY;
> + } else {
> + snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
> + shrinker_debugfs_rename(&s->s_shrink, "sb-btrfs:%s", s->s_id);

In 6.7-rc1 there's a change do allocate shrinkers dynamically so this
will need to be adjusted

shrinker_debugfs_rename(s->s_shrink, ...

> + btrfs_sb(s)->bdev_holder = &btrfs_fs_type;
> + ret = btrfs_fill_super(s, fs_devices, NULL);
> + }
> +
> + if (ret) {
> + deactivate_locked_super(s);
> + return ret;
> + }
> +
> + fc->root = dget(s->s_root);
> + return 0;
> +
> +error:
> + btrfs_close_devices(fs_devices);
> + return ret;
> +}