[PATCH 0/5] some trivial cleanup about btrfs_delete_subvolume

[PATCH 0/5] some trivial cleanup about btrfs_delete_subvolume

[Lu Fengqi]

During I am working on the online undelete of subvolume, I found something
in the btrfs_delete_subvolume need to be clean or fix. So, the patchset
is mostly about the callee or itself of btrfs_delete_subvolume.

Patch 1,3-5 some trivial cleanup
Patch 2 fix a problem about qgroup_free_meta_prealloc() in
btrfs_subvolume_reserve_metadata()

Lu Fengqi (5):
  btrfs: simplify the send_in_progress check in btrfs_delete_subvolume()
  btrfs: use a separate variable to store the num_bytes of the
    qgroup_reserve
  btrfs: switch update_size to bool in both of btrfs_block_rsv_migrate
    and btrfs_rsv_add_bytes
  btrfs: remove a useless return statement in btrfs_block_rsv_add
  btrfs: Remove root parameter from btrfs_insert_dir_item

 fs/btrfs/ctree.h         |  5 ++---
 fs/btrfs/delayed-inode.c |  4 ++--
 fs/btrfs/dir-item.c      |  8 ++++----
 fs/btrfs/extent-tree.c   | 36 ++++++++++++++++--------------------
 fs/btrfs/file.c          |  4 ++--
 fs/btrfs/inode.c         | 20 +++++++++-----------
 fs/btrfs/ioctl.c         |  3 +--
 fs/btrfs/relocation.c    |  2 +-
 fs/btrfs/transaction.c   |  7 +++----
 9 files changed, 40 insertions(+), 49 deletions(-)

-- 
2.18.0

[PATCH 1/5] btrfs: simplify the send_in_progress check in btrfs_delete_subvolume()

[Lu Fengqi]

Only when send_in_progress, we have to do something different such as
btrfs_warn() and return -EPERM. Therefore, we could check
send_in_progress first and process error handling, after the
root_item_lock has been got.

Just for better readability. No Functional Change.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/inode.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3f51ddc18f98..a7d9691a681f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4287,18 +4287,17 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
 	 * 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 {
+	if (dest->send_in_progress) {
 		spin_unlock(&dest->root_item_lock);
 		btrfs_warn(fs_info,
 			   "attempt to delete subvolume %llu during send",
 			   dest->root_key.objectid);
 		return -EPERM;
 	}
+	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);
 
 	down_write(&fs_info->subvol_sem);
 
-- 
2.18.0

[PATCH 2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve

[Lu Fengqi]

After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
fails, we cannot properly free the num_bytes of the previous
qgroup_reserve.

Delete the comment for the qgroup_reserved that does not exist and add a
comment about use_global_rsv.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/extent-tree.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index de6f75f5547b..8b009ae964cc 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5800,7 +5800,7 @@ void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans)
  * root: the root of the parent directory
  * rsv: block reservation
  * items: the number of items that we need do reservation
- * qgroup_reserved: used to return the reserved size in qgroup
+ * use_global_rsv: allow fallback to the global block reservation
  *
  * This function is used to reserve the space for snapshot/subvolume
  * creation and deletion. Those operations are different with the
@@ -5810,10 +5810,10 @@ void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans)
  * the space reservation mechanism in start_transaction().
  */
 int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
-				     struct btrfs_block_rsv *rsv,
-				     int items,
+				     struct btrfs_block_rsv *rsv, int items,
 				     bool use_global_rsv)
 {
+	u64 qgroup_num_bytes = 0;
 	u64 num_bytes;
 	int ret;
 	struct btrfs_fs_info *fs_info = root->fs_info;
@@ -5821,12 +5821,10 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 
 	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) {
 		/* One for parent inode, two for dir entries */
-		num_bytes = 3 * fs_info->nodesize;
-		ret = btrfs_qgroup_reserve_meta_prealloc(root, num_bytes, true);
+		qgroup_num_bytes = 3 * fs_info->nodesize;
+		ret = btrfs_qgroup_reserve_meta_prealloc(root, qgroup_num_bytes, true);
 		if (ret)
 			return ret;
-	} else {
-		num_bytes = 0;
 	}
 
 	num_bytes = btrfs_calc_trans_metadata_size(fs_info, items);
@@ -5838,8 +5836,8 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 	if (ret == -ENOSPC && use_global_rsv)
 		ret = btrfs_block_rsv_migrate(global_rsv, rsv, num_bytes, 1);
 
-	if (ret && num_bytes)
-		btrfs_qgroup_free_meta_prealloc(root, num_bytes);
+	if (ret && qgroup_num_bytes)
+		btrfs_qgroup_free_meta_prealloc(root, qgroup_num_bytes);
 
 	return ret;
 }
-- 
2.18.0

[PATCH 3/5] btrfs: switch update_size to bool in both of btrfs_block_rsv_migrate and btrfs_rsv_add_bytes

[Lu Fengqi]

Using true and false here is more semantic than using 0 and 1.

No functional change.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/ctree.h         |  2 +-
 fs/btrfs/delayed-inode.c |  4 ++--
 fs/btrfs/extent-tree.c   | 16 ++++++++--------
 fs/btrfs/file.c          |  4 ++--
 fs/btrfs/inode.c         |  6 +++---
 fs/btrfs/relocation.c    |  2 +-
 6 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 318be7864072..4e8a41ef683c 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2770,7 +2770,7 @@ int btrfs_block_rsv_refill(struct btrfs_root *root,
 			   enum btrfs_reserve_flush_enum flush);
 int btrfs_block_rsv_migrate(struct btrfs_block_rsv *src_rsv,
 			    struct btrfs_block_rsv *dst_rsv, u64 num_bytes,
-			    int update_size);
+			    bool update_size);
 int btrfs_cond_migrate_bytes(struct btrfs_fs_info *fs_info,
 			     struct btrfs_block_rsv *dest, u64 num_bytes,
 			     int min_factor);
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index f51b509f2d9b..584cb103955e 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -559,7 +559,7 @@ static int btrfs_delayed_item_reserve_metadata(struct btrfs_trans_handle *trans,
 	 * reserved space when starting a transaction.  So no need to reserve
 	 * qgroup space here.
 	 */
-	ret = btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes, 1);
+	ret = btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes, true);
 	if (!ret) {
 		trace_btrfs_space_reservation(fs_info, "delayed_item",
 					      item->key.objectid,
@@ -647,7 +647,7 @@ static int btrfs_delayed_inode_reserve_metadata(
 		return ret;
 	}
 
-	ret = btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes, 1);
+	ret = btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes, true);
 	if (!ret) {
 		trace_btrfs_space_reservation(fs_info, "delayed_inode",
 					      btrfs_ino(inode), num_bytes, 1);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 8b009ae964cc..4fb20f77d6b1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5284,7 +5284,7 @@ static int block_rsv_use_bytes(struct btrfs_block_rsv *block_rsv,
 }
 
 static void block_rsv_add_bytes(struct btrfs_block_rsv *block_rsv,
-				u64 num_bytes, int update_size)
+				u64 num_bytes, bool update_size)
 {
 	spin_lock(&block_rsv->lock);
 	block_rsv->reserved += num_bytes;
@@ -5316,7 +5316,7 @@ int btrfs_cond_migrate_bytes(struct btrfs_fs_info *fs_info,
 		global_rsv->full = 0;
 	spin_unlock(&global_rsv->lock);
 
-	block_rsv_add_bytes(dest, num_bytes, 1);
+	block_rsv_add_bytes(dest, num_bytes, true);
 	return 0;
 }
 
@@ -5479,7 +5479,7 @@ static u64 block_rsv_release_bytes(struct btrfs_fs_info *fs_info,
 
 int btrfs_block_rsv_migrate(struct btrfs_block_rsv *src,
 			    struct btrfs_block_rsv *dst, u64 num_bytes,
-			    int update_size)
+			    bool update_size)
 {
 	int ret;
 
@@ -5540,7 +5540,7 @@ int btrfs_block_rsv_add(struct btrfs_root *root,
 
 	ret = reserve_metadata_bytes(root, block_rsv, num_bytes, flush);
 	if (!ret) {
-		block_rsv_add_bytes(block_rsv, num_bytes, 1);
+		block_rsv_add_bytes(block_rsv, num_bytes, true);
 		return 0;
 	}
 
@@ -5587,7 +5587,7 @@ int btrfs_block_rsv_refill(struct btrfs_root *root,
 
 	ret = reserve_metadata_bytes(root, block_rsv, num_bytes, flush);
 	if (!ret) {
-		block_rsv_add_bytes(block_rsv, num_bytes, 0);
+		block_rsv_add_bytes(block_rsv, num_bytes, false);
 		return 0;
 	}
 
@@ -5629,7 +5629,7 @@ static int btrfs_inode_rsv_refill(struct btrfs_inode *inode,
 		return ret;
 	ret = reserve_metadata_bytes(root, block_rsv, num_bytes, flush);
 	if (!ret) {
-		block_rsv_add_bytes(block_rsv, num_bytes, 0);
+		block_rsv_add_bytes(block_rsv, num_bytes, false);
 		trace_btrfs_space_reservation(root->fs_info, "delalloc",
 					      btrfs_ino(inode), num_bytes, 1);
 
@@ -5834,7 +5834,7 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 				  BTRFS_RESERVE_FLUSH_ALL);
 
 	if (ret == -ENOSPC && use_global_rsv)
-		ret = btrfs_block_rsv_migrate(global_rsv, rsv, num_bytes, 1);
+		ret = btrfs_block_rsv_migrate(global_rsv, rsv, num_bytes, true);
 
 	if (ret && qgroup_num_bytes)
 		btrfs_qgroup_free_meta_prealloc(root, qgroup_num_bytes);
@@ -8214,7 +8214,7 @@ use_block_rsv(struct btrfs_trans_handle *trans,
 static void unuse_block_rsv(struct btrfs_fs_info *fs_info,
 			    struct btrfs_block_rsv *block_rsv, u32 blocksize)
 {
-	block_rsv_add_bytes(block_rsv, blocksize, 0);
+	block_rsv_add_bytes(block_rsv, blocksize, false);
 	block_rsv_release_bytes(fs_info, block_rsv, NULL, 0, NULL);
 }
 
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 2be00e873e92..d254cf94545f 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2544,7 +2544,7 @@ static int btrfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 	}
 
 	ret = btrfs_block_rsv_migrate(&fs_info->trans_block_rsv, rsv,
-				      min_size, 0);
+				      min_size, false);
 	BUG_ON(ret);
 	trans->block_rsv = rsv;
 
@@ -2594,7 +2594,7 @@ static int btrfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 		}
 
 		ret = btrfs_block_rsv_migrate(&fs_info->trans_block_rsv,
-					      rsv, min_size, 0);
+					      rsv, min_size, false);
 		BUG_ON(ret);	/* shouldn't happen */
 		trans->block_rsv = rsv;
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a7d9691a681f..a86d2f4effc6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5351,7 +5351,7 @@ static struct btrfs_trans_handle *evict_refill_and_join(struct btrfs_root *root,
 		 * it.
 		 */
 		if (!btrfs_check_space_for_delayed_refs(trans, fs_info) &&
-		    !btrfs_block_rsv_migrate(global_rsv, rsv, min_size, 0))
+		    !btrfs_block_rsv_migrate(global_rsv, rsv, min_size, false))
 			return trans;
 
 		/* If not, commit and try again. */
@@ -9045,7 +9045,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
 
 	/* Migrate the slack space for the truncate to our reserve */
 	ret = btrfs_block_rsv_migrate(&fs_info->trans_block_rsv, rsv,
-				      min_size, 0);
+				      min_size, false);
 	BUG_ON(ret);
 
 	/*
@@ -9082,7 +9082,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
 
 		btrfs_block_rsv_release(fs_info, rsv, -1);
 		ret = btrfs_block_rsv_migrate(&fs_info->trans_block_rsv,
-					      rsv, min_size, 0);
+					      rsv, min_size, false);
 		BUG_ON(ret);	/* shouldn't happen */
 		trans->block_rsv = rsv;
 	}
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8783a1776540..995a28a724ce 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4669,7 +4669,7 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
 	if (rc->merge_reloc_tree) {
 		ret = btrfs_block_rsv_migrate(&pending->block_rsv,
 					      rc->block_rsv,
-					      rc->nodes_relocated, 1);
+					      rc->nodes_relocated, true);
 		if (ret)
 			return ret;
 	}
-- 
2.18.0

[PATCH 4/5] btrfs: remove a useless return statement in btrfs_block_rsv_add

[Lu Fengqi]

Since ret must be 0 here, don't have to return separately in advance.

No functional change.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/extent-tree.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 4fb20f77d6b1..8bebf122d4b3 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5539,10 +5539,8 @@ int btrfs_block_rsv_add(struct btrfs_root *root,
 		return 0;
 
 	ret = reserve_metadata_bytes(root, block_rsv, num_bytes, flush);
-	if (!ret) {
+	if (!ret)
 		block_rsv_add_bytes(block_rsv, num_bytes, true);
-		return 0;
-	}
 
 	return ret;
 }
-- 
2.18.0

[PATCH 5/5] btrfs: Remove root parameter from btrfs_insert_dir_item

[Lu Fengqi]

All callers pass the root tree of dir, we can push that down to the
function itself.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/ctree.h       | 3 +--
 fs/btrfs/dir-item.c    | 8 ++++----
 fs/btrfs/inode.c       | 3 +--
 fs/btrfs/ioctl.c       | 3 +--
 fs/btrfs/transaction.c | 7 +++----
 5 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4e8a41ef683c..c49718b33558 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3020,8 +3020,7 @@ int btrfs_uuid_tree_iterate(struct btrfs_fs_info *fs_info,
 /* dir-item.c */
 int btrfs_check_dir_item_collision(struct btrfs_root *root, u64 dir,
 			  const char *name, int name_len);
-int btrfs_insert_dir_item(struct btrfs_trans_handle *trans,
-			  struct btrfs_root *root, const char *name,
+int btrfs_insert_dir_item(struct btrfs_trans_handle *trans, const char *name,
 			  int name_len, struct btrfs_inode *dir,
 			  struct btrfs_key *location, u8 type, u64 index);
 struct btrfs_dir_item *btrfs_lookup_dir_item(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index a678b07fcf01..8de74d835dba 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -105,13 +105,13 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
  * to use for the second index (if one is created).
  * Will return 0 or -ENOMEM
  */
-int btrfs_insert_dir_item(struct btrfs_trans_handle *trans, struct btrfs_root
-			  *root, const char *name, int name_len,
-			  struct btrfs_inode *dir, struct btrfs_key *location,
-			  u8 type, u64 index)
+int btrfs_insert_dir_item(struct btrfs_trans_handle *trans, const char *name,
+			  int name_len, struct btrfs_inode *dir,
+			  struct btrfs_key *location, u8 type, u64 index)
 {
 	int ret = 0;
 	int ret2 = 0;
+	struct btrfs_root *root = dir->root;
 	struct btrfs_path *path;
 	struct btrfs_dir_item *dir_item;
 	struct extent_buffer *leaf;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a86d2f4effc6..1f5842ab11cc 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6405,8 +6405,7 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
 	if (ret)
 		return ret;
 
-	ret = btrfs_insert_dir_item(trans, root, name, name_len,
-				    parent_inode, &key,
+	ret = btrfs_insert_dir_item(trans, name, name_len, parent_inode, &key,
 				    btrfs_inode_type(&inode->vfs_inode), index);
 	if (ret == -EEXIST || ret == -EOVERFLOW)
 		goto fail_dir_item;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d3a5d2a41e5f..eaff768cd5d4 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -686,8 +686,7 @@ static noinline int create_subvol(struct inode *dir,
 		goto fail;
 	}
 
-	ret = btrfs_insert_dir_item(trans, root,
-				    name, namelen, BTRFS_I(dir), &key,
+	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
 				    BTRFS_FT_DIR, index);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 3b84f5015029..bd784d8f5215 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1613,10 +1613,9 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	if (ret < 0)
 		goto fail;
 
-	ret = btrfs_insert_dir_item(trans, parent_root,
-				    dentry->d_name.name, dentry->d_name.len,
-				    BTRFS_I(parent_inode), &key,
-				    BTRFS_FT_DIR, index);
+	ret = btrfs_insert_dir_item(trans, dentry->d_name.name,
+				    dentry->d_name.len, BTRFS_I(parent_inode),
+				    &key, BTRFS_FT_DIR, index);
 	/* We have check then name at the beginning, so it is impossible. */
 	BUG_ON(ret == -EEXIST || ret == -EOVERFLOW);
 	if (ret) {
-- 
2.18.0

[PATCH v2 2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve

[Lu Fengqi]

After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
fails, we cannot properly free the num_bytes of the previous
qgroup_reserve.

Delete the comment for the qgroup_reserved that does not exist and add a
comment about use_global_rsv.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
Changelog:
v1->v2: break the line that exceed 80 char

 fs/btrfs/extent-tree.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index de6f75f5547b..2d9074295d7f 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5800,7 +5800,7 @@ void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans)
  * root: the root of the parent directory
  * rsv: block reservation
  * items: the number of items that we need do reservation
- * qgroup_reserved: used to return the reserved size in qgroup
+ * use_global_rsv: allow fallback to the global block reservation
  *
  * This function is used to reserve the space for snapshot/subvolume
  * creation and deletion. Those operations are different with the
@@ -5810,10 +5810,10 @@ void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans)
  * the space reservation mechanism in start_transaction().
  */
 int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
-				     struct btrfs_block_rsv *rsv,
-				     int items,
+				     struct btrfs_block_rsv *rsv, int items,
 				     bool use_global_rsv)
 {
+	u64 qgroup_num_bytes = 0;
 	u64 num_bytes;
 	int ret;
 	struct btrfs_fs_info *fs_info = root->fs_info;
@@ -5821,12 +5821,11 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 
 	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) {
 		/* One for parent inode, two for dir entries */
-		num_bytes = 3 * fs_info->nodesize;
-		ret = btrfs_qgroup_reserve_meta_prealloc(root, num_bytes, true);
+		qgroup_num_bytes = 3 * fs_info->nodesize;
+		ret = btrfs_qgroup_reserve_meta_prealloc(root,
+				qgroup_num_bytes, true);
 		if (ret)
 			return ret;
-	} else {
-		num_bytes = 0;
 	}
 
 	num_bytes = btrfs_calc_trans_metadata_size(fs_info, items);
@@ -5838,8 +5837,8 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 	if (ret == -ENOSPC && use_global_rsv)
 		ret = btrfs_block_rsv_migrate(global_rsv, rsv, num_bytes, 1);
 
-	if (ret && num_bytes)
-		btrfs_qgroup_free_meta_prealloc(root, num_bytes);
+	if (ret && qgroup_num_bytes)
+		btrfs_qgroup_free_meta_prealloc(root, qgroup_num_bytes);
 
 	return ret;
 }
-- 
2.18.0

Re: [PATCH 5/5] btrfs: Remove root parameter from btrfs_insert_dir_item

[David Sterba]

On Sat, Aug 04, 2018 at 09:10:57PM +0800, Lu Fengqi wrote:
> All callers pass the root tree of dir, we can push that down to the
> function itself.
> 
> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 3/5] btrfs: switch update_size to bool in both of btrfs_block_rsv_migrate and btrfs_rsv_add_bytes

[David Sterba]

On Sat, Aug 04, 2018 at 09:10:55PM +0800, Lu Fengqi wrote:
> Using true and false here is more semantic than using 0 and 1.
> 
> No functional change.
> 
> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 1/5] btrfs: simplify the send_in_progress check in btrfs_delete_subvolume()

[David Sterba]

On Sat, Aug 04, 2018 at 09:10:53PM +0800, Lu Fengqi wrote:
> Only when send_in_progress, we have to do something different such as
> btrfs_warn() and return -EPERM. Therefore, we could check
> send_in_progress first and process error handling, after the
> root_item_lock has been got.
> 
> Just for better readability. No Functional Change.
> 
> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve

[David Sterba]

On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
> fails, we cannot properly free the num_bytes of the previous
> qgroup_reserve.

This does not look like a trivial cleanup at all. There was an unused
parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
parameter qgroup_reserved"), that introduced the bug.  This was in this
rc1 so it's a regression and I'll consider pushing it to the 4.18 final.

Re: [PATCH 2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve

[Lu Fengqi]

On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote:
>On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
>> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
>> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
>> fails, we cannot properly free the num_bytes of the previous
>> qgroup_reserve.
>
>This does not look like a trivial cleanup at all. There was an unused
>parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
>parameter qgroup_reserved"), that introduced the bug.  This was in this
>rc1 so it's a regression and I'll consider pushing it to the 4.18 final.
>
>

I apologize for the inconvenience. I should add the Fixes tag, and really
shouldn't mix it into the trivial cleanup patch set.

-- 
Thanks,
Lu

RE: [PATCH 2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve

[Gu, Jinxiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1243 bytes --]



> -----Original Message-----
> From: David Sterba [mailto:dsterba@suse.cz]
> Sent: Wednesday, August 08, 2018 12:19 AM
> To: Lu, Fengqi/½ ·á÷è <lufq.fnst@cn.fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org; Gu, Jinxiang/¹Ë ½ðÏã <gujx@cn.fujitsu.com>
> Subject: Re: [PATCH 2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve
> 
> On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
> > After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
> > again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
> > fails, we cannot properly free the num_bytes of the previous
> > qgroup_reserve.
> 
> This does not look like a trivial cleanup at all. There was an unused
> parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
> parameter qgroup_reserved"), that introduced the bug.  
Yes. It was introduced by fix above.
I missed the second assignment of num_bytes.
And it should use two variable to record reserve size of qgroup and blockgroup.


>This was in this
> rc1 so it's a regression and I'll consider pushing it to the 4.18 final.
> 



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [PATCH 2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve

[David Sterba]

On Wed, Aug 08, 2018 at 11:04:37AM +0800, Lu Fengqi wrote:
> On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote:
> >On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
> >> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
> >> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
> >> fails, we cannot properly free the num_bytes of the previous
> >> qgroup_reserve.
> >
> >This does not look like a trivial cleanup at all. There was an unused
> >parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
> >parameter qgroup_reserved"), that introduced the bug.  This was in this
> >rc1 so it's a regression and I'll consider pushing it to the 4.18 final.
> 
> I apologize for the inconvenience. I should add the Fixes tag, and really
> shouldn't mix it into the trivial cleanup patch set.

As the bug does not qualify as urgent regression, I'm not going to
forward it to 4.18. Please update the subject and changelog so it
reflects that's an actual fix. I'll add it to the 4.19 queue then.
Thanks.

Re: [PATCH 2/5] btrfs: use a separate variable to store the num_bytes of the qgroup_reserve

[Lu Fengqi]

David Sterba <dsterba@suse.cz> 于2018年8月8日周三 下午9:57写道：
>
> On Wed, Aug 08, 2018 at 11:04:37AM +0800, Lu Fengqi wrote:
> > On Tue, Aug 07, 2018 at 06:19:12PM +0200, David Sterba wrote:
> > >On Sat, Aug 04, 2018 at 09:10:54PM +0800, Lu Fengqi wrote:
> > >> After btrfs_qgroup_reserve_meta_prealloc(), num_bytes will be assigned
> > >> again by btrfs_calc_trans_metadata_size(). Therefore, once block_rsv
> > >> fails, we cannot properly free the num_bytes of the previous
> > >> qgroup_reserve.
> > >
> > >This does not look like a trivial cleanup at all. There was an unused
> > >parameter, removed in c4c129db5da8f070147f175 ("btrfs: drop unused
> > >parameter qgroup_reserved"), that introduced the bug.  This was in this
> > >rc1 so it's a regression and I'll consider pushing it to the 4.18 final.
> >
> > I apologize for the inconvenience. I should add the Fixes tag, and really
> > shouldn't mix it into the trivial cleanup patch set.
>
> As the bug does not qualify as urgent regression, I'm not going to
> forward it to 4.18. Please update the subject and changelog so it
> reflects that's an actual fix. I'll add it to the 4.19 queue then.
> Thanks.

No problem. I will send it tomorrow.

-------------------------------------------------------------
Thanks,
Lu

> --
> 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

Re: [PATCH 4/5] btrfs: remove a useless return statement in btrfs_block_rsv_add

[David Sterba]

On Sat, Aug 04, 2018 at 09:10:56PM +0800, Lu Fengqi wrote:
> Since ret must be 0 here, don't have to return separately in advance.
> 
> No functional change.
> 
> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

Added to misc-next too, the change is fairly trivial so I was pondering
about code readability impacts, but it's ok.