[v4,3/4] btrfs: Refactor btrfs_ioctl_snap_destroy() by using btrfs_delete_subvolume()
diff mbox

Message ID e4e8ecc3-fc4f-5967-1bf3-9f5abbac0c62@jp.fujitsu.com
State New
Headers show

Commit Message

Misono Tomohiro April 11, 2018, 5:20 a.m. UTC
Use btrfs_delete_subvolume() to refactor btrfs_ioctl_snap_destroy().
The permission check is still done in btrfs_ioctl_snap_destroy(). Also,
call of d_delete() is still required since btrfs_delete_subvolume()
does not call it.

As a result, btrfs_unlink_subvol() and may_destroy_subvol()
become static functions. No functional change happens.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
---
 fs/btrfs/ctree.h |   5 ---
 fs/btrfs/inode.c |   4 +-
 fs/btrfs/ioctl.c | 131 +------------------------------------------------------
 3 files changed, 4 insertions(+), 136 deletions(-)

Comments

David Sterba April 16, 2018, 5:53 p.m. UTC | #1
On Wed, Apr 11, 2018 at 02:20:49PM +0900, Misono Tomohiro wrote:
> Use btrfs_delete_subvolume() to refactor btrfs_ioctl_snap_destroy().
> The permission check is still done in btrfs_ioctl_snap_destroy(). Also,
> call of d_delete() is still required since btrfs_delete_subvolume()
> does not call it.
> 
> As a result, btrfs_unlink_subvol() and may_destroy_subvol()
> become static functions. No functional change happens.
> 
> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>

Why is this still split into two patches? Factoring out a function
should happen in one patch, ie 2 and 3 in one go. Do you have a reason
not to do it like that?
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Misono Tomohiro April 17, 2018, 12:21 a.m. UTC | #2
On 2018/04/17 2:53, David Sterba wrote:
> On Wed, Apr 11, 2018 at 02:20:49PM +0900, Misono Tomohiro wrote:
>> Use btrfs_delete_subvolume() to refactor btrfs_ioctl_snap_destroy().
>> The permission check is still done in btrfs_ioctl_snap_destroy(). Also,
>> call of d_delete() is still required since btrfs_delete_subvolume()
>> does not call it.
>>
>> As a result, btrfs_unlink_subvol() and may_destroy_subvol()
>> become static functions. No functional change happens.
>>
>> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
> 
> Why is this still split into two patches? Factoring out a function
> should happen in one patch, ie 2 and 3 in one go. Do you have a reason
> not to do it like that?

I thought this reduces code change in one patch and might be good, but
I'm completely fine with merging 2nd and 3rd patches. Should I send v5?

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba April 17, 2018, 5:03 p.m. UTC | #3
On Tue, Apr 17, 2018 at 09:21:16AM +0900, Misono Tomohiro wrote:
> On 2018/04/17 2:53, David Sterba wrote:
> > On Wed, Apr 11, 2018 at 02:20:49PM +0900, Misono Tomohiro wrote:
> >> Use btrfs_delete_subvolume() to refactor btrfs_ioctl_snap_destroy().
> >> The permission check is still done in btrfs_ioctl_snap_destroy(). Also,
> >> call of d_delete() is still required since btrfs_delete_subvolume()
> >> does not call it.
> >>
> >> As a result, btrfs_unlink_subvol() and may_destroy_subvol()
> >> become static functions. No functional change happens.
> >>
> >> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
> > 
> > Why is this still split into two patches? Factoring out a function
> > should happen in one patch, ie 2 and 3 in one go. Do you have a reason
> > not to do it like that?
> 
> I thought this reduces code change in one patch and might be good, but
> I'm completely fine with merging 2nd and 3rd patches.

This makes the rewiew harder, if the code is moved the diff can be long
but we have both pieces to compare. Moving code is considered one
logical change and should not introduce any other changes, besides
renaming or formatting fixups.

> Should I send v5?

Yes please, this is not a trivial change and the changelogs need to be
adjusted. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 618365eb9d84..6f23f0694ac3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3162,11 +3162,6 @@  int btrfs_unlink_inode(struct btrfs_trans_handle *trans,
 int btrfs_add_link(struct btrfs_trans_handle *trans,
 		   struct btrfs_inode *parent_inode, struct btrfs_inode *inode,
 		   const char *name, int name_len, int add_backref, u64 index);
-int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
-			struct btrfs_root *root,
-			struct inode *dir, u64 objectid,
-			const char *name, int name_len);
-noinline int may_destroy_subvol(struct btrfs_root *root);
 int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry);
 int btrfs_truncate_block(struct inode *inode, loff_t from, loff_t len,
 			int front);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 54c50a9fa2ee..2db2e860ba90 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4252,7 +4252,7 @@  static int btrfs_unlink(struct inode *dir, struct dentry *dentry)
 	return ret;
 }
 
-int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
+static int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
 			struct btrfs_root *root,
 			struct inode *dir, u64 objectid,
 			const char *name, int name_len)
@@ -4336,7 +4336,7 @@  int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
 /*
  * helper to check if the subvolume references other subvolumes
  */
-noinline int may_destroy_subvol(struct btrfs_root *root)
+static noinline int may_destroy_subvol(struct btrfs_root *root)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_path *path;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 11af9eb449ef..7559a1a82e6d 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2266,12 +2266,7 @@  static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	struct btrfs_root *root = BTRFS_I(dir)->root;
 	struct btrfs_root *dest = NULL;
 	struct btrfs_ioctl_vol_args *vol_args;
-	struct btrfs_trans_handle *trans;
-	struct btrfs_block_rsv block_rsv;
-	u64 root_flags;
-	u64 qgroup_reserved;
 	int namelen;
-	int ret;
 	int err = 0;
 
 	if (!S_ISDIR(dir->i_mode))
@@ -2355,133 +2350,11 @@  static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	}
 
 	inode_lock(inode);
-
-	/*
-	 * Don't allow to delete a subvolume with send in progress. This is
-	 * inside the i_mutex so the error handling that has to drop the bit
-	 * again is not run concurrently.
-	 */
-	spin_lock(&dest->root_item_lock);
-	root_flags = btrfs_root_flags(&dest->root_item);
-	if (dest->send_in_progress == 0) {
-		btrfs_set_root_flags(&dest->root_item,
-				root_flags | BTRFS_ROOT_SUBVOL_DEAD);
-		spin_unlock(&dest->root_item_lock);
-	} else {
-		spin_unlock(&dest->root_item_lock);
-		btrfs_warn(fs_info,
-			   "Attempt to delete subvolume %llu during send",
-			   dest->root_key.objectid);
-		err = -EPERM;
-		goto out_unlock_inode;
-	}
-
-	down_write(&fs_info->subvol_sem);
-
-	err = may_destroy_subvol(dest);
-	if (err)
-		goto out_up_write;
-
-	btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
-	/*
-	 * One for dir inode, two for dir entries, two for root
-	 * ref/backref.
-	 */
-	err = btrfs_subvolume_reserve_metadata(root, &block_rsv,
-					       5, &qgroup_reserved, true);
-	if (err)
-		goto out_up_write;
-
-	trans = btrfs_start_transaction(root, 0);
-	if (IS_ERR(trans)) {
-		err = PTR_ERR(trans);
-		goto out_release;
-	}
-	trans->block_rsv = &block_rsv;
-	trans->bytes_reserved = block_rsv.size;
-
-	btrfs_record_snapshot_destroy(trans, BTRFS_I(dir));
-
-	ret = btrfs_unlink_subvol(trans, root, dir,
-				dest->root_key.objectid,
-				dentry->d_name.name,
-				dentry->d_name.len);
-	if (ret) {
-		err = ret;
-		btrfs_abort_transaction(trans, ret);
-		goto out_end_trans;
-	}
-
-	btrfs_record_root_in_trans(trans, dest);
-
-	memset(&dest->root_item.drop_progress, 0,
-		sizeof(dest->root_item.drop_progress));
-	dest->root_item.drop_level = 0;
-	btrfs_set_root_refs(&dest->root_item, 0);
-
-	if (!test_and_set_bit(BTRFS_ROOT_ORPHAN_ITEM_INSERTED, &dest->state)) {
-		ret = btrfs_insert_orphan_item(trans,
-					fs_info->tree_root,
-					dest->root_key.objectid);
-		if (ret) {
-			btrfs_abort_transaction(trans, ret);
-			err = ret;
-			goto out_end_trans;
-		}
-	}
-
-	ret = btrfs_uuid_tree_rem(trans, fs_info, dest->root_item.uuid,
-				  BTRFS_UUID_KEY_SUBVOL,
-				  dest->root_key.objectid);
-	if (ret && ret != -ENOENT) {
-		btrfs_abort_transaction(trans, ret);
-		err = ret;
-		goto out_end_trans;
-	}
-	if (!btrfs_is_empty_uuid(dest->root_item.received_uuid)) {
-		ret = btrfs_uuid_tree_rem(trans, fs_info,
-					  dest->root_item.received_uuid,
-					  BTRFS_UUID_KEY_RECEIVED_SUBVOL,
-					  dest->root_key.objectid);
-		if (ret && ret != -ENOENT) {
-			btrfs_abort_transaction(trans, ret);
-			err = ret;
-			goto out_end_trans;
-		}
-	}
-
-out_end_trans:
-	trans->block_rsv = NULL;
-	trans->bytes_reserved = 0;
-	ret = btrfs_end_transaction(trans);
-	if (ret && !err)
-		err = ret;
-	inode->i_flags |= S_DEAD;
-out_release:
-	btrfs_subvolume_release_metadata(fs_info, &block_rsv);
-out_up_write:
-	up_write(&fs_info->subvol_sem);
-	if (err) {
-		spin_lock(&dest->root_item_lock);
-		root_flags = btrfs_root_flags(&dest->root_item);
-		btrfs_set_root_flags(&dest->root_item,
-				root_flags & ~BTRFS_ROOT_SUBVOL_DEAD);
-		spin_unlock(&dest->root_item_lock);
-	}
-out_unlock_inode:
+	err = btrfs_delete_subvolume(dir, dentry);
 	inode_unlock(inode);
-	if (!err) {
-		d_invalidate(dentry);
-		btrfs_invalidate_inodes(dest);
+	if (!err)
 		d_delete(dentry);
-		ASSERT(dest->send_in_progress == 0);
 
-		/* the last ref */
-		if (dest->ino_cache_inode) {
-			iput(dest->ino_cache_inode);
-			dest->ino_cache_inode = NULL;
-		}
-	}
 out_dput:
 	dput(dentry);
 out_unlock_dir: