Re: [PATCHv2] btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY_V2 ioctl

From: David Sterba
Date: Tue Jan 28 2020 - 12:03:11 EST

On Sun, Jan 26, 2020 at 11:48:17PM -0300, Marcos Paulo de Souza wrote:
> From: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
>
> This ioctl will be responsible for deleting a subvolume using it's id.
> This can be used when a system has a file system mounted from a
> subvolume, rather than the root file system, like below:
>
> /
> |- @subvol1
> |- @subvol2
> \- @subvol_default
> If only @subvol_default is mounted, we have no path to reach
> @subvol1 and @subvol2, thus no way to delete them.

Here I'd expect a brief overview how is the "we have no path to reach"
problem solved. This is the "invisible" part,

> This patch introduces a new flag to allow BTRFS_IOC_SNAP_DESTORY_V2
> to delete subvolume using subvolid.

and this is the visible interface part, so some details are lacking.

> Also in this patch, export some functions, add BTRFS_SUBVOL_BY_ID flag
> and add subvolid as a union member of name in struct btrfs_ioctl_vol_args_v2.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> ---
> Changes from v1:
> * make btrfs_ioctl_snap_destroy handle both SNAP_DESTROY and SNAP_DESTROY_V2
> (suggested by Josef)
> * Change BTRFS_SUBVOL_DELETE_BY_ID to BTRFS_SUBVOL_BY_ID (David)
> * Send patches for btrfs-progs and xfstests along this change
>
> fs/btrfs/ctree.h | 2 +
> fs/btrfs/export.c | 4 +-
> fs/btrfs/export.h | 5 ++
> fs/btrfs/ioctl.c | 97 +++++++++++++++++++++++++++++++-------
> fs/btrfs/super.c | 4 +-
> include/uapi/linux/btrfs.h | 12 ++++-
> 6 files changed, 101 insertions(+), 23 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index f90b82050d2d..5847a34b5146 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3010,6 +3010,8 @@ int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
> int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
> unsigned long new_flags);
> int btrfs_sync_fs(struct super_block *sb, int wait);
> +char *btrfs_get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
> + u64 subvol_objectid);
>
> static inline __printf(2, 3) __cold
> void btrfs_no_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
> diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
> index 72e312cae69d..027411cdbae7 100644
> --- a/fs/btrfs/export.c
> +++ b/fs/btrfs/export.c
> @@ -57,7 +57,7 @@ static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
> return type;
> }
>
> -static struct dentry *btrfs_get_dentry(struct super_block *sb, u64 objectid,
> +struct dentry *btrfs_get_dentry(struct super_block *sb, u64 objectid,
> u64 root_objectid, u32 generation,
> int check_generation)
> {
> @@ -152,7 +152,7 @@ static struct dentry *btrfs_fh_to_dentry(struct super_block *sb, struct fid *fh,
> return btrfs_get_dentry(sb, objectid, root_objectid, generation, 1);
> }
>
> -static struct dentry *btrfs_get_parent(struct dentry *child)
> +struct dentry *btrfs_get_parent(struct dentry *child)
> {
> struct inode *dir = d_inode(child);
> struct btrfs_fs_info *fs_info = btrfs_sb(dir->i_sb);
> diff --git a/fs/btrfs/export.h b/fs/btrfs/export.h
> index 57488ecd7d4e..f981e8103d64 100644
> --- a/fs/btrfs/export.h
> +++ b/fs/btrfs/export.h
> @@ -18,4 +18,9 @@ struct btrfs_fid {
> u64 parent_root_objectid;
> } __attribute__ ((packed));
>
> +struct dentry *btrfs_get_dentry(struct super_block *sb, u64 objectid,
> + u64 root_objectid, u32 generation,
> + int check_generation);
> +struct dentry *btrfs_get_parent(struct dentry *child);
> +
> #endif
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 1b1b6ff855aa..889cb43149f9 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -28,6 +28,7 @@
> #include <linux/iversion.h>
> #include "ctree.h"
> #include "disk-io.h"
> +#include "export.h"
> #include "transaction.h"
> #include "btrfs_inode.h"
> #include "print-tree.h"
> @@ -2836,7 +2837,8 @@ static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp)
> }
>
> static noinline int btrfs_ioctl_snap_destroy(struct file *file,
> - void __user *arg)
> + void __user *arg,
> + bool destroy_v2)
> {
> struct dentry *parent = file->f_path.dentry;
> struct btrfs_fs_info *fs_info = btrfs_sb(parent->d_sb);
> @@ -2845,34 +2847,87 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
> struct inode *inode;
> struct btrfs_root *root = BTRFS_I(dir)->root;
> struct btrfs_root *dest = NULL;
> - struct btrfs_ioctl_vol_args *vol_args;
> + struct btrfs_ioctl_vol_args *vol_args = NULL;
> + struct btrfs_ioctl_vol_args_v2 *vol_args2 = NULL;
> + char *name, *name_ptr = NULL;
> int namelen;
> int err = 0;
>
> - if (!S_ISDIR(dir->i_mode))
> - return -ENOTDIR;
> + if (destroy_v2) {
> + vol_args2 = memdup_user(arg, sizeof(*vol_args2));
> + if (IS_ERR(vol_args2))
> + return PTR_ERR(vol_args2);
>
> - vol_args = memdup_user(arg, sizeof(*vol_args));
> - if (IS_ERR(vol_args))
> - return PTR_ERR(vol_args);
> + if (vol_args2->subvolid == 0) {
> + err = -EINVAL;
> + goto out;
> + }
>
> - vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
> - namelen = strlen(vol_args->name);
> - if (strchr(vol_args->name, '/') ||
> - strncmp(vol_args->name, "..", namelen) == 0) {
> - err = -EINVAL;
> - goto out;
> + if (!(vol_args2->flags & BTRFS_SUBVOL_BY_ID)) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + dentry = btrfs_get_dentry(fs_info->sb, BTRFS_FIRST_FREE_OBJECTID,
> + vol_args2->subvolid, 0, 0);
> + if (IS_ERR(dentry)) {
> + err = PTR_ERR(dentry);
> + goto out;
> + }
> +
> + /*
> + * change the default parent since the subvolume being deleted
> + * can be outside of the current mount point
> + */
> + parent = btrfs_get_parent(dentry);
> +
> + /*
> + * the only use of dentry was to get the parent, so we can
> + * release it now. Later on the dentry will be queried again to
> + * make sure the dentry will reside in the dentry cache
> + */
> + dput(dentry);
> + if (IS_ERR(parent)) {
> + err = PTR_ERR(parent);
> + goto out;
> + }
> + dir = d_inode(parent);
> +
> + name_ptr = btrfs_get_subvol_name_from_objectid(fs_info, vol_args2->subvolid);
> + if (IS_ERR(name_ptr)) {
> + err = PTR_ERR(name_ptr);
> + goto free_parent;
> + }
> + name = (char *)kbasename(name_ptr);
> + namelen = strlen(name);
> + } else {
> + vol_args = memdup_user(arg, sizeof(*vol_args));
> + if (IS_ERR(vol_args))
> + return PTR_ERR(vol_args);
> +
> + vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
> + namelen = strlen(vol_args->name);
> + if (strchr(vol_args->name, '/') ||
> + strncmp(vol_args->name, "..", namelen) == 0) {
> + err = -EINVAL;
> + goto out;
> + }
> + name = vol_args->name;
> + }
> +
> + if (!S_ISDIR(dir->i_mode)) {
> + err = -ENOTDIR;
> + goto free_subvol_name;
> }
>
> err = mnt_want_write_file(file);
> if (err)
> - goto out;
> -
> + goto free_subvol_name;
>
> err = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
> if (err == -EINTR)
> goto out_drop_write;
> - dentry = lookup_one_len(vol_args->name, parent, namelen);
> + dentry = lookup_one_len(name, parent, namelen);
> if (IS_ERR(dentry)) {
> err = PTR_ERR(dentry);
> goto out_unlock_dir;
> @@ -2943,7 +2998,13 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
> inode_unlock(dir);
> out_drop_write:
> mnt_drop_write_file(file);
> +free_subvol_name:
> + kfree(name_ptr);
> +free_parent:
> + if (destroy_v2)
> + dput(parent);
> out:
> + kfree(vol_args2);
> kfree(vol_args);
> return err;
> }
> @@ -5464,7 +5525,9 @@ long btrfs_ioctl(struct file *file, unsigned int
> case BTRFS_IOC_SUBVOL_CREATE_V2:
> return btrfs_ioctl_snap_create_v2(file, argp, 1);
> case BTRFS_IOC_SNAP_DESTROY:
> - return btrfs_ioctl_snap_destroy(file, argp);
> + return btrfs_ioctl_snap_destroy(file, argp, false);
> + case BTRFS_IOC_SNAP_DESTROY_V2:
> + return btrfs_ioctl_snap_destroy(file, argp, true);
> case BTRFS_IOC_SUBVOL_GETFLAGS:
> return btrfs_ioctl_subvol_getflags(file, argp);
> case BTRFS_IOC_SUBVOL_SETFLAGS:
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index a906315efd19..4a8ce475d906 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1024,7 +1024,7 @@ static int btrfs_parse_subvol_options(const char *options, char **subvol_name,
> return error;
> }
>
> -static char *get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
> +char *btrfs_get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
> u64 subvol_objectid)
> {
> struct btrfs_root *root = fs_info->tree_root;
> @@ -1438,7 +1438,7 @@ static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
> goto out;
> }
> }
> - subvol_name = get_subvol_name_from_objectid(btrfs_sb(mnt->mnt_sb),
> + subvol_name = btrfs_get_subvol_name_from_objectid(btrfs_sb(mnt->mnt_sb),
> subvol_objectid);
> if (IS_ERR(subvol_name)) {
> root = ERR_CAST(subvol_name);
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index 7a8bc8b920f5..d8619245f0dc 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -42,11 +42,14 @@ struct btrfs_ioctl_vol_args {
>
> #define BTRFS_DEVICE_SPEC_BY_ID (1ULL << 3)
>
> +#define BTRFS_SUBVOL_BY_ID (1ULL << 4)
> +
> #define BTRFS_VOL_ARG_V2_FLAGS_SUPPORTED \
> (BTRFS_SUBVOL_CREATE_ASYNC | \
> BTRFS_SUBVOL_RDONLY | \
> BTRFS_SUBVOL_QGROUP_INHERIT | \
> - BTRFS_DEVICE_SPEC_BY_ID)
> + BTRFS_DEVICE_SPEC_BY_ID | \
> + BTRFS_SUBVOL_BY_ID)

BTRFS_SUBVOL_SPEC_BY_ID

> @@ -119,7 +122,10 @@ struct btrfs_ioctl_vol_args_v2 {
> __u64 unused[4];
> };
> union {
> - char name[BTRFS_SUBVOL_NAME_MAX + 1];
> + union {
> + char name[BTRFS_SUBVOL_NAME_MAX + 1];
> + __u64 subvolid;
> + };

The subvolid can be added t othe first union just fine and duplicating
the name does not make sense to me.

union {
char name[BTRFS_SUBVOL_NAME_MAX + 1];
__u64 subvolid;
__u64 devid;
};

The argument structure is common for device and subvolume ioctls but I'm
not aware of any context were we would need both at the same time.
Which makes the SPEC_BY_ID bits mutually exclusive.


> __u64 devid;
> };