[PATCH 0/2] fs: btrfs: Introduce deleting subvolume by subvolid

[PATCH 0/2] fs: btrfs: Introduce deleting subvolume by subvolid

[Marcos Paulo de Souza]

From: Marcos Paulo de Souza <mpdesouza@suse.com>

The first patch separates the subvolume lookup and deletion into a new function,
and second only get the dentry related to the subvolume id and calls this
function.

Please let me know if these patches need improvement. Thanks!

Marcos Paulo de Souza (2):
  btrfs: ioctl: Move the subvolume deleter code into a new function
  btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY_V2 ioctl

 fs/btrfs/ctree.h           |   8 +++
 fs/btrfs/export.c          |   4 +-
 fs/btrfs/ioctl.c           | 119 ++++++++++++++++++++++++++++---------
 fs/btrfs/super.c           |   2 +-
 include/uapi/linux/btrfs.h |  12 +++-
 5 files changed, 113 insertions(+), 32 deletions(-)

-- 
2.24.0

[PATCH 1/2] btrfs: ioctl: Move the subvolume deleter code into a new function

[Marcos Paulo de Souza]

From: Marcos Paulo de Souza <mpdesouza@suse.com>

This new function will be used by the next patch.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/ioctl.c | 66 ++++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0fa1c386d020..dcceae4c5d28 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2835,44 +2835,27 @@ static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp)
 	return ret;
 }
 
-static noinline int btrfs_ioctl_snap_destroy(struct file *file,
-					     void __user *arg)
+static noinline int btrfs_subvolume_deleter(struct file *file,
+				struct dentry *parent, const char *subvol_name,
+				size_t namelen)
 {
-	struct dentry *parent = file->f_path.dentry;
-	struct btrfs_fs_info *fs_info = btrfs_sb(parent->d_sb);
 	struct dentry *dentry;
-	struct inode *dir = d_inode(parent);
+	struct btrfs_root *dest;
 	struct inode *inode;
+	struct inode *dir = d_inode(parent);
 	struct btrfs_root *root = BTRFS_I(dir)->root;
-	struct btrfs_root *dest = NULL;
-	struct btrfs_ioctl_vol_args *vol_args;
-	int namelen;
+	struct btrfs_fs_info *fs_info = root->fs_info;
 	int err = 0;
 
-	if (!S_ISDIR(dir->i_mode))
-		return -ENOTDIR;
-
-	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;
-	}
-
 	err = mnt_want_write_file(file);
 	if (err)
-		goto out;
-
+		return err;
 
 	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(subvol_name, parent, namelen);
 	if (IS_ERR(dentry)) {
 		err = PTR_ERR(dentry);
 		goto out_unlock_dir;
@@ -2880,7 +2863,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 
 	if (d_really_is_negative(dentry)) {
 		err = -ENOENT;
-		goto out_dput;
+		goto out_unlock_dir;
 	}
 
 	inode = d_inode(dentry);
@@ -2943,6 +2926,35 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	inode_unlock(dir);
 out_drop_write:
 	mnt_drop_write_file(file);
+
+	return err;
+}
+
+static noinline int btrfs_ioctl_snap_destroy(struct file *file,
+					     void __user *arg)
+{
+	struct dentry *parent = file->f_path.dentry;
+	struct inode *dir = d_inode(parent);
+	struct btrfs_ioctl_vol_args *vol_args;
+	int namelen;
+	int err = 0;
+
+	if (!S_ISDIR(dir->i_mode))
+		return -ENOTDIR;
+
+	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;
+	}
+
+	err = btrfs_subvolume_deleter(file, parent, vol_args->name, namelen);
 out:
 	kfree(vol_args);
 	return err;
-- 
2.24.0

[PATCH 2/2] btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY_V2 ioctl

[Marcos Paulo de Souza]

From: Marcos Paulo de Souza <mpdesouza@suse.com>

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.
This patch introduces a new flag to allow BTRFS_IOC_SNAP_DESTORY_V2
to delete subvolume using subvolid.

Also in this patch, add BTRFS_SUBVOL_DELETE_BY_ID flag and add subvolid
as a union member of fd in struct btrfs_ioctl_vol_args_v2.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 fs/btrfs/ctree.h           |  8 ++++++
 fs/btrfs/export.c          |  4 +--
 fs/btrfs/ioctl.c           | 53 ++++++++++++++++++++++++++++++++++++++
 fs/btrfs/super.c           |  2 +-
 include/uapi/linux/btrfs.h | 12 +++++++--
 5 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 569931dd0ce5..421a2f57f9ec 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 *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, ...)
@@ -3442,6 +3444,12 @@ int btrfs_reada_wait(void *handle);
 void btrfs_reada_detach(void *handle);
 int btree_readahead_hook(struct extent_buffer *eb, int err);
 
+/* export.c */
+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);
+
 static inline int is_fstree(u64 rootid)
 {
 	if (rootid == BTRFS_FS_TREE_OBJECTID ||
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/ioctl.c b/fs/btrfs/ioctl.c
index dcceae4c5d28..68da45ad4904 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2960,6 +2960,57 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	return err;
 }
 
+static noinline int btrfs_ioctl_snap_destroy_v2(struct file *file,
+					     void __user *arg)
+{
+	struct btrfs_fs_info *fs_info = btrfs_sb(file->f_path.dentry->d_sb);
+	struct dentry *dentry, *pdentry;
+	struct btrfs_ioctl_vol_args_v2 *vol_args;
+	char *name, *p;
+	size_t namelen;
+	int err = 0;
+
+	vol_args = memdup_user(arg, sizeof(*vol_args));
+	if (IS_ERR(vol_args))
+		return PTR_ERR(vol_args);
+
+	if (vol_args->subvolid == 0)
+		return -EINVAL;
+
+	if (!(vol_args->flags & BTRFS_SUBVOL_DELETE_BY_ID))
+		return -EINVAL;
+
+	dentry = btrfs_get_dentry(fs_info->sb, BTRFS_FIRST_FREE_OBJECTID,
+				vol_args->subvolid, 0, 0);
+	if (IS_ERR(dentry)) {
+		err = PTR_ERR(dentry);
+		return err;
+	}
+
+	pdentry = btrfs_get_parent(dentry);
+	if (IS_ERR(pdentry)) {
+		err = PTR_ERR(pdentry);
+		goto out_dentry;
+	}
+
+	name = get_subvol_name_from_objectid(fs_info, vol_args->subvolid);
+	if (IS_ERR(name)) {
+		err = PTR_ERR(name);
+		goto out_pdentry;
+	}
+	p = (char *)kbasename(name);
+	namelen = strlen(p);
+
+	err = btrfs_subvolume_deleter(file, pdentry, p, namelen);
+
+	kfree(name);
+out_pdentry:
+	dput(pdentry);
+out_dentry:
+	dput(dentry);
+	return err;
+}
+
 static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
 {
 	struct inode *inode = file_inode(file);
@@ -5465,6 +5516,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_snap_create_v2(file, argp, 1);
 	case BTRFS_IOC_SNAP_DESTROY:
 		return btrfs_ioctl_snap_destroy(file, argp);
+	case BTRFS_IOC_SNAP_DESTROY_V2:
+		return btrfs_ioctl_snap_destroy_v2(file, argp);
 	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..a448d2bb93e6 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 *get_subvol_name_from_objectid(struct btrfs_fs_info *fs_info,
 					   u64 subvol_objectid)
 {
 	struct btrfs_root *root = fs_info->tree_root;
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index 7a8bc8b920f5..1be03082e49a 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_DELETE_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_DELETE_BY_ID)
 
 #define BTRFS_FSID_SIZE 16
 #define BTRFS_UUID_SIZE 16
@@ -108,7 +111,10 @@ struct btrfs_ioctl_qgroup_limit_args {
  */
 
 struct btrfs_ioctl_vol_args_v2 {
-	__s64 fd;
+	union {
+		__s64 fd;
+		__u64 subvolid;
+	};
 	__u64 transid;
 	__u64 flags;
 	union {
@@ -949,5 +955,7 @@ enum btrfs_err_code {
 				struct btrfs_ioctl_get_subvol_rootref_args)
 #define BTRFS_IOC_INO_LOOKUP_USER _IOWR(BTRFS_IOCTL_MAGIC, 62, \
 				struct btrfs_ioctl_ino_lookup_user_args)
+#define BTRFS_IOC_SNAP_DESTROY_V2 _IOW(BTRFS_IOCTL_MAGIC, 63, \
+				struct btrfs_ioctl_vol_args_v2)
 
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
2.24.0

Re: [PATCH 2/2] btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY_V2 ioctl

[Josef Bacik]

On 1/10/20 8:39 PM, Marcos Paulo de Souza wrote:
> From: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
> 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.
> This patch introduces a new flag to allow BTRFS_IOC_SNAP_DESTORY_V2
> to delete subvolume using subvolid.
> 
> Also in this patch, add BTRFS_SUBVOL_DELETE_BY_ID flag and add subvolid
> as a union member of fd in struct btrfs_ioctl_vol_args_v2.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>   fs/btrfs/ctree.h           |  8 ++++++
>   fs/btrfs/export.c          |  4 +--
>   fs/btrfs/ioctl.c           | 53 ++++++++++++++++++++++++++++++++++++++
>   fs/btrfs/super.c           |  2 +-
>   include/uapi/linux/btrfs.h | 12 +++++++--
>   5 files changed, 74 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 569931dd0ce5..421a2f57f9ec 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 *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, ...)
> @@ -3442,6 +3444,12 @@ int btrfs_reada_wait(void *handle);
>   void btrfs_reada_detach(void *handle);
>   int btree_readahead_hook(struct extent_buffer *eb, int err);
>   
> +/* export.c */
> +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);
> +
>   static inline int is_fstree(u64 rootid)
>   {
>   	if (rootid == BTRFS_FS_TREE_OBJECTID ||
> 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/ioctl.c b/fs/btrfs/ioctl.c
> index dcceae4c5d28..68da45ad4904 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2960,6 +2960,57 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>   	return err;
>   }
>   
> +static noinline int btrfs_ioctl_snap_destroy_v2(struct file *file,
> +					     void __user *arg)
> +{
> +	struct btrfs_fs_info *fs_info = btrfs_sb(file->f_path.dentry->d_sb);
> +	struct dentry *dentry, *pdentry;
> +	struct btrfs_ioctl_vol_args_v2 *vol_args;
> +	char *name, *p;
> +	size_t namelen;
> +	int err = 0;
> +
> +	vol_args = memdup_user(arg, sizeof(*vol_args));
> +	if (IS_ERR(vol_args))
> +		return PTR_ERR(vol_args);
> +
> +	if (vol_args->subvolid == 0)
> +		return -EINVAL;
> +
> +	if (!(vol_args->flags & BTRFS_SUBVOL_DELETE_BY_ID))
> +		return -EINVAL;
> +
> +	dentry = btrfs_get_dentry(fs_info->sb, BTRFS_FIRST_FREE_OBJECTID,
> +				vol_args->subvolid, 0, 0);
> +	if (IS_ERR(dentry)) {
> +		err = PTR_ERR(dentry);
> +		return err;
> +	}
> +
> +	pdentry = btrfs_get_parent(dentry);
> +	if (IS_ERR(pdentry)) {
> +		err = PTR_ERR(pdentry);
> +		goto out_dentry;
> +	}

What happens if we have something like

/subvol
/subvol2
/subvol3/subvol4
/subvol5

and we mount /subvol5, and then we try to delete subvol4?  We aren't going to be 
able to find the parent dentry for subvol3 right?  Cause that thing isn't linked 
into our currently mounted tree, and things will go wonky right?  I'm only 
working on like 4 hours of sleep so I could be missing something obvious here.

> +
> +	name = get_subvol_name_from_objectid(fs_info, vol_args->subvolid);
> +	if (IS_ERR(name)) {
> +		err = PTR_ERR(name);
> +		goto out_pdentry;
> +	}
> +	p = (char *)kbasename(name);
> +	namelen = strlen(p);
> +
> +	err = btrfs_subvolume_deleter(file, pdentry, p, namelen);

We looked up the dentry to send the name into btrfs_subvolume_deleter(), which 
just takes the name and looks up the dentry again?  Have the common function 
just take both dentries and have v1 and v2 do their lookup shenanigans.  Thanks,

Josef

Re: [PATCH 2/2] btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY_V2 ioctl

[Marcos Paulo de Souza]

On Mon, 2020-01-13 at 09:37 -0800, Josef Bacik wrote:
> On 1/10/20 8:39 PM, Marcos Paulo de Souza wrote:
> > From: Marcos Paulo de Souza <mpdesouza@suse.com>
> > 
> > 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.
> > This patch introduces a new flag to allow BTRFS_IOC_SNAP_DESTORY_V2
> > to delete subvolume using subvolid.
> > 
> > Also in this patch, add BTRFS_SUBVOL_DELETE_BY_ID flag and add
> subvolid
> > as a union member of fd in struct btrfs_ioctl_vol_args_v2.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >   fs/btrfs/ctree.h           |  8 ++++++
> >   fs/btrfs/export.c          |  4 +--
> >   fs/btrfs/ioctl.c           | 53
> ++++++++++++++++++++++++++++++++++++++
> >   fs/btrfs/super.c           |  2 +-
> >   include/uapi/linux/btrfs.h | 12 +++++++--
> >   5 files changed, 74 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 569931dd0ce5..421a2f57f9ec 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 *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, ...)
> > @@ -3442,6 +3444,12 @@ int btrfs_reada_wait(void *handle);
> >   void btrfs_reada_detach(void *handle);
> >   int btree_readahead_hook(struct extent_buffer *eb, int err);
> >   
> > +/* export.c */
> > +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);
> > +
> >   static inline int is_fstree(u64 rootid)
> >   {
> >   	if (rootid == BTRFS_FS_TREE_OBJECTID ||
> > 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/ioctl.c b/fs/btrfs/ioctl.c
> > index dcceae4c5d28..68da45ad4904 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -2960,6 +2960,57 @@ static noinline int
> btrfs_ioctl_snap_destroy(struct file *file,
> >   	return err;
> >   }
> >   
> > +static noinline int btrfs_ioctl_snap_destroy_v2(struct file *file,
> > +					     void __user *arg)
> > +{
> > +	struct btrfs_fs_info *fs_info = btrfs_sb(file->f_path.dentry-
> >d_sb);
> > +	struct dentry *dentry, *pdentry;
> > +	struct btrfs_ioctl_vol_args_v2 *vol_args;
> > +	char *name, *p;
> > +	size_t namelen;
> > +	int err = 0;
> > +
> > +	vol_args = memdup_user(arg, sizeof(*vol_args));
> > +	if (IS_ERR(vol_args))
> > +		return PTR_ERR(vol_args);
> > +
> > +	if (vol_args->subvolid == 0)
> > +		return -EINVAL;
> > +
> > +	if (!(vol_args->flags & BTRFS_SUBVOL_DELETE_BY_ID))
> > +		return -EINVAL;
> > +
> > +	dentry = btrfs_get_dentry(fs_info->sb,
> BTRFS_FIRST_FREE_OBJECTID,
> > +				vol_args->subvolid, 0, 0);
> > +	if (IS_ERR(dentry)) {
> > +		err = PTR_ERR(dentry);
> > +		return err;
> > +	}
> > +
> > +	pdentry = btrfs_get_parent(dentry);
> > +	if (IS_ERR(pdentry)) {
> > +		err = PTR_ERR(pdentry);
> > +		goto out_dentry;
> > +	}
> 
> What happens if we have something like
> 
> /subvol
> /subvol2
> /subvol3/subvol4
> /subvol5
> 
> and we mount /subvol5, and then we try to delete subvol4?  We aren't
> going to be 
> able to find the parent dentry for subvol3 right?  Cause that thing
> isn't linked 
> into our currently mounted tree, and things will go wonky right?  I'm
> only 
> working on like 4 hours of sleep so I could be missing something
> obvious here.

It works because we don't account the dentry based in the filp pointer,
but rather we go through the fs tree by the subvolid and get the dentry
from there. Using that dentry we then access the tree again to grab the
parent. Am I missing something? It worked in my tests :)

> 
> > +
> > +	name = get_subvol_name_from_objectid(fs_info, vol_args-
> >subvolid);
> > +	if (IS_ERR(name)) {
> > +		err = PTR_ERR(name);
> > +		goto out_pdentry;
> > +	}
> > +	p = (char *)kbasename(name);
> > +	namelen = strlen(p);
> > +
> > +	err = btrfs_subvolume_deleter(file, pdentry, p, namelen);
> 
> We looked up the dentry to send the name into
> btrfs_subvolume_deleter(), which 
> just takes the name and looks up the dentry again?  Have the common
> function 
> just take both dentries and have v1 and v2 do their lookup
> shenanigans.  Thanks,

So, this part is only necessary in the deletion by subvolid, since we
cannot trust the file pointer of the current subvol delete ioctl
because of the possible differented mounted tree, but I think it's
doable to have the same function and only check for the SUBVOL_DELETE
flag and the subvolid vol_args_v2 member.

> 
> Josef

Re: [PATCH 2/2] btrfs: Introduce new BTRFS_IOC_SNAP_DESTROY_V2 ioctl

[Josef Bacik]

On 1/13/20 6:31 PM, Marcos Paulo de Souza wrote:
> On Mon, 2020-01-13 at 09:37 -0800, Josef Bacik wrote:
>> On 1/10/20 8:39 PM, Marcos Paulo de Souza wrote:
>>> From: Marcos Paulo de Souza <mpdesouza@suse.com>
>>>
>>> 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.
>>> This patch introduces a new flag to allow BTRFS_IOC_SNAP_DESTORY_V2
>>> to delete subvolume using subvolid.
>>>
>>> Also in this patch, add BTRFS_SUBVOL_DELETE_BY_ID flag and add
>> subvolid
>>> as a union member of fd in struct btrfs_ioctl_vol_args_v2.
>>>
>>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>>> ---
>>>    fs/btrfs/ctree.h           |  8 ++++++
>>>    fs/btrfs/export.c          |  4 +--
>>>    fs/btrfs/ioctl.c           | 53
>> ++++++++++++++++++++++++++++++++++++++
>>>    fs/btrfs/super.c           |  2 +-
>>>    include/uapi/linux/btrfs.h | 12 +++++++--
>>>    5 files changed, 74 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index 569931dd0ce5..421a2f57f9ec 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 *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, ...)
>>> @@ -3442,6 +3444,12 @@ int btrfs_reada_wait(void *handle);
>>>    void btrfs_reada_detach(void *handle);
>>>    int btree_readahead_hook(struct extent_buffer *eb, int err);
>>>    
>>> +/* export.c */
>>> +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);
>>> +
>>>    static inline int is_fstree(u64 rootid)
>>>    {
>>>    	if (rootid == BTRFS_FS_TREE_OBJECTID ||
>>> 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/ioctl.c b/fs/btrfs/ioctl.c
>>> index dcceae4c5d28..68da45ad4904 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -2960,6 +2960,57 @@ static noinline int
>> btrfs_ioctl_snap_destroy(struct file *file,
>>>    	return err;
>>>    }
>>>    
>>> +static noinline int btrfs_ioctl_snap_destroy_v2(struct file *file,
>>> +					     void __user *arg)
>>> +{
>>> +	struct btrfs_fs_info *fs_info = btrfs_sb(file->f_path.dentry-
>>> d_sb);
>>> +	struct dentry *dentry, *pdentry;
>>> +	struct btrfs_ioctl_vol_args_v2 *vol_args;
>>> +	char *name, *p;
>>> +	size_t namelen;
>>> +	int err = 0;
>>> +
>>> +	vol_args = memdup_user(arg, sizeof(*vol_args));
>>> +	if (IS_ERR(vol_args))
>>> +		return PTR_ERR(vol_args);
>>> +
>>> +	if (vol_args->subvolid == 0)
>>> +		return -EINVAL;
>>> +
>>> +	if (!(vol_args->flags & BTRFS_SUBVOL_DELETE_BY_ID))
>>> +		return -EINVAL;
>>> +
>>> +	dentry = btrfs_get_dentry(fs_info->sb,
>> BTRFS_FIRST_FREE_OBJECTID,
>>> +				vol_args->subvolid, 0, 0);
>>> +	if (IS_ERR(dentry)) {
>>> +		err = PTR_ERR(dentry);
>>> +		return err;
>>> +	}
>>> +
>>> +	pdentry = btrfs_get_parent(dentry);
>>> +	if (IS_ERR(pdentry)) {
>>> +		err = PTR_ERR(pdentry);
>>> +		goto out_dentry;
>>> +	}
>>
>> What happens if we have something like
>>
>> /subvol
>> /subvol2
>> /subvol3/subvol4
>> /subvol5
>>
>> and we mount /subvol5, and then we try to delete subvol4?  We aren't
>> going to be
>> able to find the parent dentry for subvol3 right?  Cause that thing
>> isn't linked
>> into our currently mounted tree, and things will go wonky right?  I'm
>> only
>> working on like 4 hours of sleep so I could be missing something
>> obvious here.
> 
> It works because we don't account the dentry based in the filp pointer,
> but rather we go through the fs tree by the subvolid and get the dentry
> from there. Using that dentry we then access the tree again to grab the
> parent. Am I missing something? It worked in my tests :)
> 

Idk, I guess if it works it works and I'll not question it.

>>
>>> +
>>> +	name = get_subvol_name_from_objectid(fs_info, vol_args-
>>> subvolid);
>>> +	if (IS_ERR(name)) {
>>> +		err = PTR_ERR(name);
>>> +		goto out_pdentry;
>>> +	}
>>> +	p = (char *)kbasename(name);
>>> +	namelen = strlen(p);
>>> +
>>> +	err = btrfs_subvolume_deleter(file, pdentry, p, namelen);
>>
>> We looked up the dentry to send the name into
>> btrfs_subvolume_deleter(), which
>> just takes the name and looks up the dentry again?  Have the common
>> function
>> just take both dentries and have v1 and v2 do their lookup
>> shenanigans.  Thanks,
> 
> So, this part is only necessary in the deletion by subvolid, since we
> cannot trust the file pointer of the current subvol delete ioctl
> because of the possible differented mounted tree, but I think it's
> doable to have the same function and only check for the SUBVOL_DELETE
> flag and the subvolid vol_args_v2 member.

Right but what I'm saying is we do the work to lookup the dentry here, so we 
might as well not duplicate that lookup in btrfs_subvolume_deleter.  Instead 
refactor the btrfs_subvolume_deleter to take both the parent and target 
dentries, and pass in the dentry you looked up here, and rework the v1 to do the 
dentry lookup by name.

Also I'd like to see btrfs-progs and xfstests patches along with this.  Thanks,

Josef