btrfs: qgroup: Separate qgroup reservation types

Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv to restore different types of reservation.

This patch only updates the header and needed modification to pass compile.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 16 ++++++++++------
 fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 8 deletions(-)

fs/btrfs/qgroup.c +10 -6

index e172d4843eae..fe3adb996883 100644
@@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 }
 
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes)
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;

@@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
 			goto out;
 		freed += changeset.bytes_changed;
 	}
-	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
+	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
+				  BTRFS_QGROUP_RSV_DATA);
 	ret = freed;
 out:
 	extent_changeset_release(&changeset);

@@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free)
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 	ret = changeset.bytes_changed;
 out:
 	extent_changeset_release(&changeset);

@@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	if (reserved == 0)
 		return;
 	trace_qgroup_meta_reserve(root, -(s64)reserved);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)

@@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
 	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 /*

@@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 		}
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 
 	}
 	extent_changeset_release(&changeset);

fs/btrfs/qgroup.h +0 -0

index d9984e87cddf..0b04cbc5b5ce 100644

fs/btrfs/qgroup.h +4 -0

+};
+
+/*
+ * Represents how many bytes we reserved for this qgroup.

fs/btrfs/qgroup.h +7 -2

+};
+
 /*
  * one struct for each qgroup, organized in fs_info->qgroup_tree.
  */

@@ -88,6 +108,7 @@ struct btrfs_qgroup {
 	 * reservation tracking
 	 */
 	u64 reserved;
+	struct btrfs_qgroup_rsv rsv;
 
 	/*
 	 * lists

@@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
 			 struct btrfs_qgroup_inherit *inherit);
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes);
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type);
 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
+				  BTRFS_QGROUP_RSV_DATA);
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

Quoted 82 lines · Qu Wenruo

Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv to restore different types of reservation.

This patch only updates the header and needed modification to pass compile.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 16 ++++++++++------
 fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 8 deletions(-)

fs/btrfs/qgroup.c +10 -6

index e172d4843eae..fe3adb996883 100644
@@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 }
 
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes)
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;

@@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
 			goto out;
 		freed += changeset.bytes_changed;
 	}
-	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
+	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
+				  BTRFS_QGROUP_RSV_DATA);
 	ret = freed;
 out:
 	extent_changeset_release(&changeset);

@@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free)
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 	ret = changeset.bytes_changed;
 out:
 	extent_changeset_release(&changeset);

@@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	if (reserved == 0)
 		return;
 	trace_qgroup_meta_reserve(root, -(s64)reserved);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)

@@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
 	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 /*

@@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 		}
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 
 	}
 	extent_changeset_release(&changeset);

fs/btrfs/qgroup.h +0 -0

index d9984e87cddf..0b04cbc5b5ce 100644
@@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
 	struct ulist *old_roots;
 };
 

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 67

Quoted 4 lines

fs/btrfs/qgroup.h +4 -0

+enum btrfs_qgroup_rsv_type {
+	BTRFS_QGROUP_RSV_DATA = 0,
+	BTRFS_QGROUP_RSV_META = 1,
+	BTRFS_QGROUP_RSV_TYPES = 1,

nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;

Quoted 10 lines

fs/btrfs/qgroup.h +9 -0

+};
+
+/*
+ * Represents how many bytes we reserved for this qgroup.
+ *
+ * Each type should have different reservation behavior.
+ * E.g, data follows its io_tree flag modification, while
+ * *currently* meta is just reserve-and-clear during transcation.
+ *

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 81

Quoted 5 lines

fs/btrfs/qgroup.h +5 -0

+ * TODO: Add new type for delalloc, which can exist across several
+ * transaction.
+ */
+struct btrfs_qgroup_rsv {
+	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];

nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here, seems more idiomatic to me.

Quoted 22 lines

fs/btrfs/qgroup.h +5 -1

+};
+
 /*
  * one struct for each qgroup, organized in fs_info->qgroup_tree.
  */

@@ -88,6 +108,7 @@ struct btrfs_qgroup {
 	 * reservation tracking
 	 */
 	u64 reserved;
+	struct btrfs_qgroup_rsv rsv;
 
 	/*
 	 * lists

@@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
 			 struct btrfs_qgroup_inherit *inherit);
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes);
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type);

fs/btrfs/qgroup.h int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, · line 262

Quoted 10 lines

fs/btrfs/qgroup.h +2 -1

 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
+				  BTRFS_QGROUP_RSV_DATA);
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 85 lines · Nikolay Borisov

Quoted 82 lines · Qu Wenruo

Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv to restore different types of reservation.

This patch only updates the header and needed modification to pass compile.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 16 ++++++++++------
 fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 8 deletions(-)

fs/btrfs/qgroup.c +10 -6

index e172d4843eae..fe3adb996883 100644
@@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 }
 
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes)
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;

@@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
 			goto out;
 		freed += changeset.bytes_changed;
 	}
-	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
+	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
+				  BTRFS_QGROUP_RSV_DATA);
 	ret = freed;
 out:
 	extent_changeset_release(&changeset);

@@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free)
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 	ret = changeset.bytes_changed;
 out:
 	extent_changeset_release(&changeset);

@@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	if (reserved == 0)
 		return;
 	trace_qgroup_meta_reserve(root, -(s64)reserved);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)

@@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
 	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 /*

@@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 		}
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 
 	}
 	extent_changeset_release(&changeset);

fs/btrfs/qgroup.h +0 -0

index d9984e87cddf..0b04cbc5b5ce 100644
@@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
 	struct ulist *old_roots;
 };
 

Quoted 6 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 67

Quoted 4 lines

fs/btrfs/qgroup.h +4 -0

+enum btrfs_qgroup_rsv_type {
+	BTRFS_QGROUP_RSV_DATA = 0,
+	BTRFS_QGROUP_RSV_META = 1,
+	BTRFS_QGROUP_RSV_TYPES = 1,

nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;

My original plan is just as the same as yours.

However I still remember I did it before and David fixed it by using TYPES, so I follow his naming schema here.

Kernel is also using this naming schema else where:
d91876496bcf ("btrfs: compress: put variables defined per compress type in struct to make cache friendly")

Quoted 10 lines

Quoted 10 lines

fs/btrfs/qgroup.h +9 -0

+};
+
+/*
+ * Represents how many bytes we reserved for this qgroup.
+ *
+ * Each type should have different reservation behavior.
+ * E.g, data follows its io_tree flag modification, while
+ * *currently* meta is just reserve-and-clear during transcation.
+ *

Quoted 8 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 81

Quoted 5 lines

fs/btrfs/qgroup.h +5 -0

+ * TODO: Add new type for delalloc, which can exist across several
+ * transaction.
+ */
+struct btrfs_qgroup_rsv {
+	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];

nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here, seems more idiomatic to me.

To follow same naming schema from David.
(IIRC it was about tree-checker patchset, checking file extent type part)

In fact, I crashed kernel several times due to the tiny +1, without even a clue for hours just testing blindly, until latest gcc gives warning about it.

Thanks,
Qu

Quoted 37 lines

Quoted 22 lines

fs/btrfs/qgroup.h +5 -1

+};
+
 /*
  * one struct for each qgroup, organized in fs_info->qgroup_tree.
  */

@@ -88,6 +108,7 @@ struct btrfs_qgroup {
 	 * reservation tracking
 	 */
 	u64 reserved;
+	struct btrfs_qgroup_rsv rsv;
 
 	/*
 	 * lists

@@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
 			 struct btrfs_qgroup_inherit *inherit);
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes);
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type);

fs/btrfs/qgroup.h int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, · line 262

Quoted 10 lines

fs/btrfs/qgroup.h +2 -1

 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
+				  BTRFS_QGROUP_RSV_DATA);
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 95 lines · Qu Wenruo

Quoted 85 lines · Nikolay Borisov

Quoted 82 lines · Qu Wenruo

Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv to restore different types of reservation.

This patch only updates the header and needed modification to pass compile.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 16 ++++++++++------
 fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 8 deletions(-)

fs/btrfs/qgroup.c +10 -6

index e172d4843eae..fe3adb996883 100644
@@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 }
 
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes)
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;

@@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
 			goto out;
 		freed += changeset.bytes_changed;
 	}
-	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
+	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
+				  BTRFS_QGROUP_RSV_DATA);
 	ret = freed;
 out:
 	extent_changeset_release(&changeset);

@@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free)
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 	ret = changeset.bytes_changed;
 out:
 	extent_changeset_release(&changeset);

@@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	if (reserved == 0)
 		return;
 	trace_qgroup_meta_reserve(root, -(s64)reserved);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)

@@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
 	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 /*

@@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 		}
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 
 	}
 	extent_changeset_release(&changeset);

fs/btrfs/qgroup.h +0 -0

index d9984e87cddf..0b04cbc5b5ce 100644
@@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
 	struct ulist *old_roots;
 };
 

Quoted 6 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 67

Quoted 4 lines

fs/btrfs/qgroup.h +4 -0

+enum btrfs_qgroup_rsv_type {
+	BTRFS_QGROUP_RSV_DATA = 0,
+	BTRFS_QGROUP_RSV_META = 1,
+	BTRFS_QGROUP_RSV_TYPES = 1,

nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;

Quoted 4 lines

My original plan is just as the same as yours.

However I still remember I did it before and David fixed it by using TYPES, so I follow his naming schema here.

I don't really care about whether it's called TYPES or _MAX. The more important thing is the fact that we have types set to the same value as the last valid value and thus when writing loops or checking ranges we need to use <= and + 1 respectively which IMO is a bit counter-intuitive. But as I said it's a nit so I don't have strong preference either way if this is the convention of btrfs so be it.

Quoted 24 lines

Kernel is also using this naming schema else where:
d91876496bcf ("btrfs: compress: put variables defined per compress type in struct to make cache friendly")

Quoted 18 lines

Quoted 5 lines

fs/btrfs/qgroup.h +4 -0

+};
+
+/*
+ * Represents how many bytes we reserved for this qgroup.

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 81

Quoted 10 lines

fs/btrfs/qgroup.h +10 -0

+ *
+ * Each type should have different reservation behavior.
+ * E.g, data follows its io_tree flag modification, while
+ * *currently* meta is just reserve-and-clear during transcation.
+ *
+ * TODO: Add new type for delalloc, which can exist across several
+ * transaction.
+ */
+struct btrfs_qgroup_rsv {
+	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];

nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here, seems more idiomatic to me.

Quoted 6 lines

To follow same naming schema from David.
(IIRC it was about tree-checker patchset, checking file extent type part)

In fact, I crashed kernel several times due to the tiny +1, without even a clue for hours just testing blindly, until latest gcc gives warning about it.

So you crashed the kernel because you were missing it or not ?

Quoted 43 lines

Thanks,
Qu

Quoted 37 lines

Quoted 22 lines

fs/btrfs/qgroup.h +5 -1

+};
+
 /*
  * one struct for each qgroup, organized in fs_info->qgroup_tree.
  */

@@ -88,6 +108,7 @@ struct btrfs_qgroup {
 	 * reservation tracking
 	 */
 	u64 reserved;
+	struct btrfs_qgroup_rsv rsv;
 
 	/*
 	 * lists

@@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
 			 struct btrfs_qgroup_inherit *inherit);
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes);
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type);

fs/btrfs/qgroup.h int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, · line 262

Quoted 10 lines

fs/btrfs/qgroup.h +2 -1

 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
+				  BTRFS_QGROUP_RSV_DATA);
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 85 lines · Nikolay Borisov

Quoted 82 lines · Qu Wenruo

Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv to restore different types of reservation.

This patch only updates the header and needed modification to pass compile.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 16 ++++++++++------
 fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 8 deletions(-)

fs/btrfs/qgroup.c +10 -6

index e172d4843eae..fe3adb996883 100644
@@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 }
 
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes)
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;

@@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
 			goto out;
 		freed += changeset.bytes_changed;
 	}
-	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
+	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
+				  BTRFS_QGROUP_RSV_DATA);
 	ret = freed;
 out:
 	extent_changeset_release(&changeset);

@@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free)
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 	ret = changeset.bytes_changed;
 out:
 	extent_changeset_release(&changeset);

@@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	if (reserved == 0)
 		return;
 	trace_qgroup_meta_reserve(root, -(s64)reserved);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)

@@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
 	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 /*

@@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 		}
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 
 	}
 	extent_changeset_release(&changeset);

fs/btrfs/qgroup.h +0 -0

index d9984e87cddf..0b04cbc5b5ce 100644
@@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
 	struct ulist *old_roots;
 };
 

Quoted 6 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 67

Quoted 4 lines

fs/btrfs/qgroup.h +4 -0

+enum btrfs_qgroup_rsv_type {
+	BTRFS_QGROUP_RSV_DATA = 0,
+	BTRFS_QGROUP_RSV_META = 1,
+	BTRFS_QGROUP_RSV_TYPES = 1,

nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;

My original plan is just as the same as yours.

However I still remember I did it before and David fixed it by using TYPES, so I follow his naming schema here.

Kernel is also using this naming schema else where:
d91876496bcf ("btrfs: compress: put variables defined per compress type in struct to make cache friendly")

Quoted 4 lines

Quoted 4 lines

fs/btrfs/qgroup.h +4 -0

+};
+
+/*
+ * Represents how many bytes we reserved for this qgroup.

Quoted 8 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 79

Quoted 8 lines

fs/btrfs/qgroup.h +8 -0

+ *
+ * Each type should have different reservation behavior.
+ * E.g, data follows its io_tree flag modification, while
+ * *currently* meta is just reserve-and-clear during transcation.
+ *
+ * TODO: Add new type for delalloc, which can exist across several
+ * transaction.
+ */

Quoted 5 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 81

Quoted 2 lines

fs/btrfs/qgroup.h +2 -0

+struct btrfs_qgroup_rsv {
+	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];

nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here, seems more idiomatic to me.

To follow same naming schema from David.
(IIRC it was about tree-checker patchset, checking file extent type part)

In fact, I crashed kernel several times due to the tiny +1, without even a clue for hours just testing blindly, until latest gcc gives warning about it.

Quoted 37 lines

Quoted 22 lines

fs/btrfs/qgroup.h +5 -1

+};
+
 /*
  * one struct for each qgroup, organized in fs_info->qgroup_tree.
  */

@@ -88,6 +108,7 @@ struct btrfs_qgroup {
 	 * reservation tracking
 	 */
 	u64 reserved;
+	struct btrfs_qgroup_rsv rsv;
 
 	/*
 	 * lists

@@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
 			 struct btrfs_qgroup_inherit *inherit);
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes);
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type);

fs/btrfs/qgroup.h int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, · line 262

Quoted 10 lines

fs/btrfs/qgroup.h +2 -1

 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
+				  BTRFS_QGROUP_RSV_DATA);
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Qu Wenruo

Quoted 1 line · Nikolay Borisov

nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;

Quoted 8 lines

My original plan is just as the same as yours.

However I still remember I did it before and David fixed it by using TYPES, so I follow his naming schema here.

Kernel is also using this naming schema else where:
d91876496bcf ("btrfs: compress: put variables defined per compress type in struct to make cache friendly")

The COMPRESS_TYPES pattern isn't the right pattern to follow here. That's a special case since there's a _NONE that doesn't have anything associated with it, so we don't need to take a slot in the array.

We also don't care about any of the specific values, just that they start at 0. The BTRFS_COMPRESS_TYPES example also has a BTRFS_COMPRESS_LAST item in the enum, which serves the same purpose as MAX. I don't have a strong opinion on the naming, just that we don't play games with +1 when handling arrays since, as you say, that's just waiting for subtle bugs later.

enum btrfs_qgroup_rsv_type {
BTRFS_QGROUP_RSV_DATA = 0,
BTRFS_QGROUP_RSV_META,
BTRFS_QGROUP_RSV_LAST,
};

Quoted 95 lines · Qu Wenruo

Quoted 85 lines · Nikolay Borisov

Quoted 82 lines · Qu Wenruo

Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv to restore different types of reservation.

This patch only updates the header and needed modification to pass compile.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 16 ++++++++++------
 fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 8 deletions(-)

fs/btrfs/qgroup.c +10 -6

index e172d4843eae..fe3adb996883 100644
@@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 }
 
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes)
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;

@@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
 			goto out;
 		freed += changeset.bytes_changed;
 	}
-	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
+	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
+				  BTRFS_QGROUP_RSV_DATA);
 	ret = freed;
 out:
 	extent_changeset_release(&changeset);

@@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free)
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 	ret = changeset.bytes_changed;
 out:
 	extent_changeset_release(&changeset);

@@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	if (reserved == 0)
 		return;
 	trace_qgroup_meta_reserve(root, -(s64)reserved);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)

@@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
 	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 /*

@@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 		}
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 
 	}
 	extent_changeset_release(&changeset);

fs/btrfs/qgroup.h +0 -0

index d9984e87cddf..0b04cbc5b5ce 100644
@@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
 	struct ulist *old_roots;
 };
 

Quoted 6 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 67

Quoted 4 lines

fs/btrfs/qgroup.h +4 -0

+enum btrfs_qgroup_rsv_type {
+	BTRFS_QGROUP_RSV_DATA = 0,
+	BTRFS_QGROUP_RSV_META = 1,
+	BTRFS_QGROUP_RSV_TYPES = 1,

nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;

Quoted 8 lines

My original plan is just as the same as yours.

However I still remember I did it before and David fixed it by using TYPES, so I follow his naming schema here.

Kernel is also using this naming schema else where:
d91876496bcf ("btrfs: compress: put variables defined per compress type in struct to make cache friendly")

The COMPRESS_TYPES pattern isn't the right pattern to follow here. That's a special case since there's a _NONE that doesn't have anything associated with it, so we don't need to take a slot in the array.

We also don't care about any of the specific values, just that they start at 0. The BTRFS_COMPRESS_TYPES example also has a BTRFS_COMPRESS_LAST item in the enum, which serves the same purpose as MAX.

I don't have a strong opinion on the naming, just that we don't
play games with +1 when handling arrays since, as you say, that's just waiting for subtle bugs later.

enum btrfs_qgroup_rsv_type {
BTRFS_QGROUP_RSV_DATA = 0,
BTRFS_QGROUP_RSV_META,
BTRFS_QGROUP_RSV_LAST,
};

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

fs/btrfs/qgroup.h +9 -0

+};
+
+/*
+ * Represents how many bytes we reserved for this qgroup.
+ *
+ * Each type should have different reservation behavior.
+ * E.g, data follows its io_tree flag modification, while
+ * *currently* meta is just reserve-and-clear during transcation.
+ *

Quoted 3 lines

Quoted 3 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 79

Quoted 3 lines

fs/btrfs/qgroup.h +3 -0

+ * TODO: Add new type for delalloc, which can exist across several
+ * transaction.
+ */

Minor nit: It's not just delalloc. Delayed items and inodes can as well. The general rule is that qgroup reservations aren't essentially different from block reservations and follow the same usage patterns when operating on leaf nodes.

Quoted 7 lines

Quoted 6 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 81

Quoted 3 lines

fs/btrfs/qgroup.h +2 -0

+struct btrfs_qgroup_rsv {
+	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];

nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here, seems more idiomatic to me.

Quoted 6 lines

To follow same naming schema from David.
(IIRC it was about tree-checker patchset, checking file extent type part)

In fact, I crashed kernel several times due to the tiny +1, without even a clue for hours just testing blindly, until latest gcc gives warning about it.

BTRFS_QGROUP_RSV_LAST would do the job here.

-Jeff

Quoted 38 lines

Quoted 37 lines

Quoted 22 lines

fs/btrfs/qgroup.h +5 -1

+};
+
 /*
  * one struct for each qgroup, organized in fs_info->qgroup_tree.
  */

@@ -88,6 +108,7 @@ struct btrfs_qgroup {
 	 * reservation tracking
 	 */
 	u64 reserved;
+	struct btrfs_qgroup_rsv rsv;
 
 	/*
 	 * lists

@@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
 			 struct btrfs_qgroup_inherit *inherit);
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes);
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type);

fs/btrfs/qgroup.h int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, · line 262

Quoted 10 lines

fs/btrfs/qgroup.h +2 -1

 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
+				  BTRFS_QGROUP_RSV_DATA);
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Introduce helpers to:

1) Get total reserved space
For limit calculation

2) Increase reserved space for given type
2) Decrease reserved space for given type

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

fs/btrfs/qgroup.c +2 -0

index fe3adb996883..04fd145516ad 100644
@@ -47,6 +47,72 @@
  *  - check all ioctl parameters
  */
 
+/*
+ * Helpers to access qgroup reservation

fs/btrfs/qgroup.c +25 -0

+		ret += qgroup->rsv.values[i];
+
+	return ret;
+}
+
+static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
+{
+	if (type == BTRFS_QGROUP_RSV_DATA)
+		return "data";
+	if (type == BTRFS_QGROUP_RSV_META)
+		return "meta";
+	return NULL;
+}
+
+static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	qgroup->rsv.values[type] += num_bytes;
+}
+
+static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	if (qgroup->rsv.values[type] >= num_bytes) {
+		qgroup->rsv.values[type] -= num_bytes;

fs/btrfs/qgroup.c +8 -0

+
+static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_increase(dest, src->rsv.values[i], i);

fs/btrfs/qgroup.c +1 -0

+
 static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
 					   int mod)
 {

Quoted 21 lines · Qu Wenruo

Introduce helpers to:

1) Get total reserved space
For limit calculation

2) Increase reserved space for given type
2) Decrease reserved space for given type

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

fs/btrfs/qgroup.c +0 -0

index fe3adb996883..04fd145516ad 100644
@@ -47,6 +47,72 @@
  *  - check all ioctl parameters
  */
 

fs/btrfs/qgroup.c line 61

Quoted 12 lines

fs/btrfs/qgroup.c +12 -0

+/*
+ * Helpers to access qgroup reservation
+ *
+ * Callers should ensure the lock context and type are valid
+ */
+
+static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
+{
+	u64 ret = 0;
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)

So if you actually take up on my idea of having an explicit value which denotes the end, the loop here would be just < *_END rather than the <= which instantly looks suspicious of an off-by-one error.

Quoted 35 lines

fs/btrfs/qgroup.c +34 -0

+		ret += qgroup->rsv.values[i];
+
+	return ret;
+}
+
+static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
+{
+	if (type == BTRFS_QGROUP_RSV_DATA)
+		return "data";
+	if (type == BTRFS_QGROUP_RSV_META)
+		return "meta";
+	return NULL;
+}
+
+static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	qgroup->rsv.values[type] += num_bytes;
+}
+
+static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	if (qgroup->rsv.values[type] >= num_bytes) {
+		qgroup->rsv.values[type] -= num_bytes;
+		return;
+	}
+#ifdef CONFIG_BTRFS_DEBUG
+	WARN_RATELIMIT(1,
+		"qgroup %llu %s reserved space underflow, have %llu to free %llu",
+		qgroup->qgroupid, qgroup_rsv_type_str(type),
+		qgroup->rsv.values[type], num_bytes);
+#endif
+	qgroup->rsv.values[type] = 0;

fs/btrfs/qgroup.c line 96

Quoted 1 line

fs/btrfs/qgroup.c +1 -0

+}

Perhaps these could be modelled after block_rsv_use_bytes/block_rsv_add_bytes ? I'm not entirely sure of what increasing the value actually does - reserving bytes or using already reserved bytes, I guess it should be reserving. In this case what about qgroup_bytes_reserve/qgroup_bytes_(free|unreserve) ?

Quoted 17 lines

fs/btrfs/qgroup.c +15 -0

+
+static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_increase(dest, src->rsv.values[i], i);
+}
+
+static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+

fs/btrfs/qgroup.c line 114

Quoted 3 lines

fs/btrfs/qgroup.c +3 -0

+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
+}

Why don't you model these similar to what we already have for the block rsv. In this case I believe those would correspond to the btrfs_block_rsv_migrate. Admittedly we don't have a counterpart to rsv_decrease_by_qgroup.

fs/btrfs/qgroup.c line 118

Quoted 7 lines

fs/btrfs/qgroup.c +1 -0

+
 static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
 					   int mod)
 {

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 24 lines · Nikolay Borisov

Quoted 21 lines · Qu Wenruo

Introduce helpers to:

1) Get total reserved space
For limit calculation

2) Increase reserved space for given type
2) Decrease reserved space for given type

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

fs/btrfs/qgroup.c +0 -0

index fe3adb996883..04fd145516ad 100644
@@ -47,6 +47,72 @@
  *  - check all ioctl parameters
  */
 

Quoted 16 lines

fs/btrfs/qgroup.c line 61

Quoted 12 lines

fs/btrfs/qgroup.c +12 -0

+/*
+ * Helpers to access qgroup reservation
+ *
+ * Callers should ensure the lock context and type are valid
+ */
+
+static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
+{
+	u64 ret = 0;
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)

So if you actually take up on my idea of having an explicit value which denotes the end, the loop here would be just < *_END rather than the <= which instantly looks suspicious of an off-by-one error.

Yeah, I really like to do it, as mentioned in previous mail.

But to follow the schema used elsewhere, I had no choice.

Quoted 35 lines

Quoted 35 lines

fs/btrfs/qgroup.c +34 -0

+		ret += qgroup->rsv.values[i];
+
+	return ret;
+}
+
+static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
+{
+	if (type == BTRFS_QGROUP_RSV_DATA)
+		return "data";
+	if (type == BTRFS_QGROUP_RSV_META)
+		return "meta";
+	return NULL;
+}
+
+static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	qgroup->rsv.values[type] += num_bytes;
+}
+
+static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	if (qgroup->rsv.values[type] >= num_bytes) {
+		qgroup->rsv.values[type] -= num_bytes;
+		return;
+	}
+#ifdef CONFIG_BTRFS_DEBUG
+	WARN_RATELIMIT(1,
+		"qgroup %llu %s reserved space underflow, have %llu to free %llu",
+		qgroup->qgroupid, qgroup_rsv_type_str(type),
+		qgroup->rsv.values[type], num_bytes);
+#endif
+	qgroup->rsv.values[type] = 0;

Quoted 8 lines

fs/btrfs/qgroup.c line 96

Quoted 1 line

fs/btrfs/qgroup.c +1 -0

+}

Perhaps these could be modelled after block_rsv_use_bytes/block_rsv_add_bytes ? I'm not entirely sure of what increasing the value actually does - reserving bytes or using already reserved bytes, I guess it should be reserving. In this case what about qgroup_bytes_reserve/qgroup_bytes_(free|unreserve) ?

I'm really bad at naming functions.
My original idea is qgroup_rsv_add() and qgroup_rsv_rename(), but "rename" is 3 characters longer than "add", which makes me quite uncomfortable.
(Maybe 'add' and 'sub' better?)

For the "reserve|free", it can be easily confused with btrfs_qgroup_reserve|release_data().

But in fact, this qgroup_rsv_* functions are only used to maintain qgroup->rsv, so it's less meaningful compared to other functions, like btrfs_qgroup_reserve|release_data().

The value itself represents how many bytes it has already reserved for later operation.
So qgroup_rsv_increase() normally means qgroup is reserving more space, while qgroup_rsv_decrease() means qgroup reserved space is decreasing, either released or freed.

Hopes above explanation could inspire you about better naming ideas. (Because I really have no idea how to name it better while keeping the name the same length)

Quoted 15 lines

Quoted 15 lines

fs/btrfs/qgroup.c +15 -0

+
+static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_increase(dest, src->rsv.values[i], i);
+}
+
+static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+

Quoted 8 lines

fs/btrfs/qgroup.c line 114

Quoted 3 lines

fs/btrfs/qgroup.c +3 -0

+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
+}

Why don't you model these similar to what we already have for the block rsv. In this case I believe those would correspond to the btrfs_block_rsv_migrate. Admittedly we don't have a counterpart to rsv_decrease_by_qgroup.

Not sure about 'migrate', I think it's more like 'inherit', since @src is not modified at all.

'increase' and 'decrease' are preferred mainly because they are the same length, and represents the value change obviously enough.

I'm completely open to better naming schema.
(But it's better to have same length, I'm kind of paranoid about this)

Thanks,
Qu

Quoted 12 lines

fs/btrfs/qgroup.c line 118

Quoted 7 lines

fs/btrfs/qgroup.c +1 -0

+
 static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
 					   int mod)
 {

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 111 lines · Qu Wenruo

Quoted 24 lines · Nikolay Borisov

Quoted 21 lines · Qu Wenruo

Introduce helpers to:

1) Get total reserved space
For limit calculation

2) Increase reserved space for given type
2) Decrease reserved space for given type

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

fs/btrfs/qgroup.c +0 -0

index fe3adb996883..04fd145516ad 100644
@@ -47,6 +47,72 @@
  *  - check all ioctl parameters
  */
 

Quoted 16 lines

fs/btrfs/qgroup.c line 61

Quoted 12 lines

fs/btrfs/qgroup.c +12 -0

+/*
+ * Helpers to access qgroup reservation
+ *
+ * Callers should ensure the lock context and type are valid
+ */
+
+static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
+{
+	u64 ret = 0;
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)

So if you actually take up on my idea of having an explicit value which denotes the end, the loop here would be just < *_END rather than the <= which instantly looks suspicious of an off-by-one error.

Yeah, I really like to do it, as mentioned in previous mail.

But to follow the schema used elsewhere, I had no choice.

Quoted 35 lines

Quoted 35 lines

fs/btrfs/qgroup.c +34 -0

+		ret += qgroup->rsv.values[i];
+
+	return ret;
+}
+
+static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
+{
+	if (type == BTRFS_QGROUP_RSV_DATA)
+		return "data";
+	if (type == BTRFS_QGROUP_RSV_META)
+		return "meta";
+	return NULL;
+}
+
+static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	qgroup->rsv.values[type] += num_bytes;
+}
+
+static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	if (qgroup->rsv.values[type] >= num_bytes) {
+		qgroup->rsv.values[type] -= num_bytes;
+		return;
+	}
+#ifdef CONFIG_BTRFS_DEBUG
+	WARN_RATELIMIT(1,
+		"qgroup %llu %s reserved space underflow, have %llu to free %llu",
+		qgroup->qgroupid, qgroup_rsv_type_str(type),
+		qgroup->rsv.values[type], num_bytes);
+#endif
+	qgroup->rsv.values[type] = 0;

Quoted 8 lines

fs/btrfs/qgroup.c line 96

Quoted 1 line

fs/btrfs/qgroup.c +1 -0

+}

Perhaps these could be modelled after block_rsv_use_bytes/block_rsv_add_bytes ? I'm not entirely sure of what increasing the value actually does - reserving bytes or using already reserved bytes, I guess it should be reserving. In this case what about qgroup_bytes_reserve/qgroup_bytes_(free|unreserve) ?

I'm really bad at naming functions.
My original idea is qgroup_rsv_add() and qgroup_rsv_rename(), but "rename" is 3 characters longer than "add", which makes me quite uncomfortable.
(Maybe 'add' and 'sub' better?)

For the "reserve|free", it can be easily confused with btrfs_qgroup_reserve|release_data().

But in fact, this qgroup_rsv_* functions are only used to maintain qgroup->rsv, so it's less meaningful compared to other functions, like btrfs_qgroup_reserve|release_data().

The value itself represents how many bytes it has already reserved for later operation.
So qgroup_rsv_increase() normally means qgroup is reserving more space, while qgroup_rsv_decrease() means qgroup reserved space is decreasing, either released or freed.

Quoted 3 lines

Hopes above explanation could inspire you about better naming ideas. (Because I really have no idea how to name it better while keeping the name the same length)

I'm not sure I understand the concern about the length. These names do not seem excessive to me, so a few extra characters for the sake of clarity would be well worth it.

Ed

Quoted 27 lines

Quoted 15 lines

Quoted 15 lines

fs/btrfs/qgroup.c +15 -0

+
+static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_increase(dest, src->rsv.values[i], i);
+}
+
+static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+

Quoted 8 lines

fs/btrfs/qgroup.c line 114

Quoted 3 lines

fs/btrfs/qgroup.c +3 -0

+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
+}

Why don't you model these similar to what we already have for the block rsv. In this case I believe those would correspond to the btrfs_block_rsv_migrate. Admittedly we don't have a counterpart to rsv_decrease_by_qgroup.

Not sure about 'migrate', I think it's more like 'inherit', since @src is not modified at all.

Quoted 22 lines

'increase' and 'decrease' are preferred mainly because they are the same length, and represents the value change obviously enough.

I'm completely open to better naming schema.
(But it's better to have same length, I'm kind of paranoid about this)

Thanks,
Qu

Quoted 12 lines

fs/btrfs/qgroup.c line 118

Quoted 7 lines

fs/btrfs/qgroup.c +1 -0

+
 static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
 					   int mod)
 {

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

When modifying qgroup relationship, for qgroup which only owns exclusive extents, we will go through quick update path.

In quick update path, we will just adding/removing exclusive and reference number.

However we did the opposite for qgroup reservation from the very beginning.

In fact, we should also inherit the qgroup reservation space, just like exclusive and reference numbers.

Fix by using the newly introduced qgroup_rsv_increase/decrease_by_qgroup() function call.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

fs/btrfs/qgroup.c +7 -7

index 7b89da9589c1..ba6f60fd0e96 100644
@@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
 #endif
 	qgroup->reserved = 0;
 }
+
 /*
- * The easy accounting, if we are adding/removing the only ref for an extent
- * then this qgroup and all of the parent qgroups get their reference and
- * exclusive counts adjusted.
+ * The easy accounting, we're updating qgroup relationship whose child qgroup
+ * only have exclusive extents.
+ * In this case, we only need to update the rfer/excl, and inherit rsv from
+ * child qgroup (@src)
  *
  * Caller should hold fs_info->qgroup_lock.
  */
 static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 				    struct ulist *tmp, u64 ref_root,
-				    u64 num_bytes, int sign)
+				    struct btrfs_qgroup *src, int sign)
 {
 	struct btrfs_qgroup *qgroup;
 	struct btrfs_qgroup_list *glist;
 	struct ulist_node *unode;
 	struct ulist_iterator uiter;
+	u64 num_bytes = src->excl;
 	int ret = 0;
 
 	qgroup = find_qgroup_rb(fs_info, ref_root);

@@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
-	if (sign > 0) {
-		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
-		if (qgroup->reserved < num_bytes)

fs/btrfs/qgroup.c +5 -10

 	qgroup_dirty(fs_info, qgroup);
 

@@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		qgroup->rfer_cmpr += sign * num_bytes;
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
-		if (sign > 0) {
-			trace_qgroup_update_reserve(fs_info, qgroup,
-						    -(s64)num_bytes);
-			if (qgroup->reserved < num_bytes)
-				report_reserved_underflow(fs_info, qgroup,
-							  num_bytes);
-			else
-				qgroup->reserved -= num_bytes;
-		}
+		if (sign > 0)
+			qgroup_rsv_increase_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_decrease_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 

@@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 	if (qgroup->excl == qgroup->rfer) {
 		ret = 0;
 		err = __qgroup_excl_accounting(fs_info, tmp, dst,
-					       qgroup->excl, sign);
+					       qgroup, sign);
 		if (err < 0) {
 			ret = err;
 			goto out;

Quoted 3 lines · Qu Wenruo

When modifying qgroup relationship, for qgroup which only owns exclusive extents, we will go through quick update path.

Quoted 5 lines

In quick update path, we will just adding/removing exclusive and reference number.

However we did the opposite for qgroup reservation from the very beginning.

I'm afraid this sentence doesn't give much information about what's really going on.

Quoted 62 lines

In fact, we should also inherit the qgroup reservation space, just like exclusive and reference numbers.

Fix by using the newly introduced qgroup_rsv_increase/decrease_by_qgroup() function call.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

fs/btrfs/qgroup.c +12 -11

index 7b89da9589c1..ba6f60fd0e96 100644
@@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
 #endif
 	qgroup->reserved = 0;
 }
+
 /*
- * The easy accounting, if we are adding/removing the only ref for an extent
- * then this qgroup and all of the parent qgroups get their reference and
- * exclusive counts adjusted.
+ * The easy accounting, we're updating qgroup relationship whose child qgroup
+ * only have exclusive extents.
+ * In this case, we only need to update the rfer/excl, and inherit rsv from
+ * child qgroup (@src)
  *
  * Caller should hold fs_info->qgroup_lock.
  */
 static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 				    struct ulist *tmp, u64 ref_root,
-				    u64 num_bytes, int sign)
+				    struct btrfs_qgroup *src, int sign)
 {
 	struct btrfs_qgroup *qgroup;
 	struct btrfs_qgroup_list *glist;
 	struct ulist_node *unode;
 	struct ulist_iterator uiter;
+	u64 num_bytes = src->excl;
 	int ret = 0;
 
 	qgroup = find_qgroup_rb(fs_info, ref_root);

@@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
-	if (sign > 0) {
-		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
-		if (qgroup->reserved < num_bytes)
-			report_reserved_underflow(fs_info, qgroup, num_bytes);
-		else
-			qgroup->reserved -= num_bytes;
-	}
+
+	/* *Inherit* qgroup rsv info from @src */
+	if (sign > 0)
+		qgroup_rsv_increase_by_qgroup(qgroup, src);
+	else

fs/btrfs/qgroup.c static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, · line 1107

Quoted 1 line

fs/btrfs/qgroup.c +1 -0

+		qgroup_rsv_decrease_by_qgroup(qgroup, src);

I'm a bit confused by the semantics of the 'sign' variable. So what you are doing is that if sign is > 0 then you are "adding a relationship" i.e. adding 'src reservation to 'qgroup', presumably because the src is a child of qgroup? So you are handling both adding and deletion in the if statement?

However, before that apparently only deleting a relation ship was handled by that same if (And I believe that was wrong since if sign > 0 then we should be adding bytes but here we are subtracting). SO the bug being fixed by this commit are actually 2 bugs:

1. Completely missing the "adding a relation ship case"
2. Incorrect hanlding of sign < 0, since this was handled by the sign > 0 case?

Quoted 28 lines

fs/btrfs/qgroup.c +4 -9

 	qgroup_dirty(fs_info, qgroup);
 

@@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		qgroup->rfer_cmpr += sign * num_bytes;
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
-		if (sign > 0) {
-			trace_qgroup_update_reserve(fs_info, qgroup,
-						    -(s64)num_bytes);
-			if (qgroup->reserved < num_bytes)
-				report_reserved_underflow(fs_info, qgroup,
-							  num_bytes);
-			else
-				qgroup->reserved -= num_bytes;
-		}
+		if (sign > 0)
+			qgroup_rsv_increase_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_decrease_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 

fs/btrfs/qgroup.c static int quick_update_accounting(struct btrfs_fs_info *fs_info, · line 1176

Quoted 8 lines

fs/btrfs/qgroup.c +1 -1

@@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 	if (qgroup->excl == qgroup->rfer) {
 		ret = 0;
 		err = __qgroup_excl_accounting(fs_info, tmp, dst,
-					       qgroup->excl, sign);
+					       qgroup, sign);
 		if (err < 0) {
 			ret = err;
 			goto out;

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Nikolay Borisov

Quoted 3 lines · Qu Wenruo

When modifying qgroup relationship, for qgroup which only owns exclusive extents, we will go through quick update path.

Quoted 8 lines

Quoted 5 lines

In quick update path, we will just adding/removing exclusive and reference number.

However we did the opposite for qgroup reservation from the very beginning.

I'm afraid this sentence doesn't give much information about what's really going on.

I'll try to reorganize it to give a better explanation on this.

Quoted 62 lines

Quoted 62 lines

In fact, we should also inherit the qgroup reservation space, just like exclusive and reference numbers.

Fix by using the newly introduced qgroup_rsv_increase/decrease_by_qgroup() function call.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

fs/btrfs/qgroup.c +12 -11

index 7b89da9589c1..ba6f60fd0e96 100644
@@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
 #endif
 	qgroup->reserved = 0;
 }
+
 /*
- * The easy accounting, if we are adding/removing the only ref for an extent
- * then this qgroup and all of the parent qgroups get their reference and
- * exclusive counts adjusted.
+ * The easy accounting, we're updating qgroup relationship whose child qgroup
+ * only have exclusive extents.
+ * In this case, we only need to update the rfer/excl, and inherit rsv from
+ * child qgroup (@src)
  *
  * Caller should hold fs_info->qgroup_lock.
  */
 static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 				    struct ulist *tmp, u64 ref_root,
-				    u64 num_bytes, int sign)
+				    struct btrfs_qgroup *src, int sign)
 {
 	struct btrfs_qgroup *qgroup;
 	struct btrfs_qgroup_list *glist;
 	struct ulist_node *unode;
 	struct ulist_iterator uiter;
+	u64 num_bytes = src->excl;
 	int ret = 0;
 
 	qgroup = find_qgroup_rb(fs_info, ref_root);

@@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
-	if (sign > 0) {
-		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
-		if (qgroup->reserved < num_bytes)
-			report_reserved_underflow(fs_info, qgroup, num_bytes);
-		else
-			qgroup->reserved -= num_bytes;
-	}
+
+	/* *Inherit* qgroup rsv info from @src */
+	if (sign > 0)
+		qgroup_rsv_increase_by_qgroup(qgroup, src);
+	else

Quoted 8 lines

fs/btrfs/qgroup.c static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, · line 1107

Quoted 1 line

fs/btrfs/qgroup.c +1 -0

+		qgroup_rsv_decrease_by_qgroup(qgroup, src);

I'm a bit confused by the semantics of the 'sign' variable. So what you are doing is that if sign is > 0 then you are "adding a relationship" i.e. adding 'src reservation to 'qgroup', presumably because the src is a child of qgroup? So you are handling both adding and deletion in the if statement?

Yes, the original design of @sign is to allow single function to handle both relationship adding and deleting.
just like the rest code, which uses @sign to handle both adding and deleting without using if.

Quoted 9 lines

However, before that apparently only deleting a relation ship was handled by that same if (And I believe that was wrong since if sign > 0 then we should be adding bytes but here we are subtracting). SO the bug being fixed by this commit are actually 2 bugs:

1. Completely missing the "adding a relation ship case"
2. Incorrect hanlding of sign < 0, since this was handled by the sign > 0 case?

Yes, in fact 2 bugs.

Although the original code is acting like it's allocating space inside the new parent, so it reduces parent's reserved, and adding new excl/refer.

However it's not the case, it should do inheriting, not allocating from parent.

For sign > 0, (adding relationship) parent should inherit all excl/rfer and reserved space.
For sign < 0, (deleting relationshio) parent should have all its excl/rfer along with reserved space removed.

^^^ This should be the correct behavior.

The original code is just a copy of older code, as you can see in commit 9c8b35b1ba21 ("btrfs: quota: Automatically update related qgroups or mark INCONSISTENT flags when assigning/deleting a qgroup relations.").

So it's a bug dating back to ancient days and it's my fault I didn't expose it in the very beginning.

Thanks,
Qu

Quoted 41 lines

Quoted 28 lines

fs/btrfs/qgroup.c +4 -9

 	qgroup_dirty(fs_info, qgroup);
 

@@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		qgroup->rfer_cmpr += sign * num_bytes;
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
-		if (sign > 0) {
-			trace_qgroup_update_reserve(fs_info, qgroup,
-						    -(s64)num_bytes);
-			if (qgroup->reserved < num_bytes)
-				report_reserved_underflow(fs_info, qgroup,
-							  num_bytes);
-			else
-				qgroup->reserved -= num_bytes;
-		}
+		if (sign > 0)
+			qgroup_rsv_increase_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_decrease_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 

fs/btrfs/qgroup.c static int quick_update_accounting(struct btrfs_fs_info *fs_info, · line 1176

Quoted 8 lines

fs/btrfs/qgroup.c +1 -1

@@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 	if (qgroup->excl == qgroup->rfer) {
 		ret = 0;
 		err = __qgroup_excl_accounting(fs_info, tmp, dst,
-					       qgroup->excl, sign);
+					       qgroup, sign);
 		if (err < 0) {
 			ret = err;
 			goto out;

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

When modifying qgroup relationship, for qgroup which only owns exclusive extents, we will go through quick update path.

In quick update path, we will just adding/removing exclusive and reference number.

However we did the opposite for qgroup reservation from the very beginning.

In fact, we should also inherit the qgroup reservation space, just like exclusive and reference numbers.

Fix by using the newly introduced qgroup_rsv_increase/decrease_by_qgroup() function call.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

fs/btrfs/qgroup.c +0 -0

index 7b89da9589c1..ba6f60fd0e96 100644
@@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
 #endif
 	qgroup->reserved = 0;

fs/btrfs/qgroup.c +13 -18

  *
  * Caller should hold fs_info->qgroup_lock.
  */
 static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 				    struct ulist *tmp, u64 ref_root,
-				    u64 num_bytes, int sign)
+				    struct btrfs_qgroup *src, int sign)
 {
 	struct btrfs_qgroup *qgroup;
 	struct btrfs_qgroup_list *glist;
 	struct ulist_node *unode;
 	struct ulist_iterator uiter;
+	u64 num_bytes = src->excl;
 	int ret = 0;
 
 	qgroup = find_qgroup_rb(fs_info, ref_root);

@@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
-	if (sign > 0) {
-		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
-		if (qgroup->reserved < num_bytes)
-			report_reserved_underflow(fs_info, qgroup, num_bytes);
-		else
-			qgroup->reserved -= num_bytes;
-	}
+
+	/* *Inherit* qgroup rsv info from @src */
+	if (sign > 0)
+		qgroup_rsv_increase_by_qgroup(qgroup, src);
+	else
+		qgroup_rsv_decrease_by_qgroup(qgroup, src);
 
 	qgroup_dirty(fs_info, qgroup);
 

@@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		qgroup->rfer_cmpr += sign * num_bytes;
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
-		if (sign > 0) {
-			trace_qgroup_update_reserve(fs_info, qgroup,
-						    -(s64)num_bytes);
-			if (qgroup->reserved < num_bytes)
-				report_reserved_underflow(fs_info, qgroup,
-							  num_bytes);
-			else
-				qgroup->reserved -= num_bytes;
-		}
+		if (sign > 0)
+			qgroup_rsv_increase_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_decrease_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 

@@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 	if (qgroup->excl == qgroup->rfer) {
 		ret = 0;
 		err = __qgroup_excl_accounting(fs_info, tmp, dst,
-					       qgroup->excl, sign);
+					       qgroup, sign);
 		if (err < 0) {
 			ret = err;
 			goto out;

Quoted 34 lines · Qu Wenruo

When modifying qgroup relationship, for qgroup which only owns exclusive extents, we will go through quick update path.

In quick update path, we will just adding/removing exclusive and reference number.

However we did the opposite for qgroup reservation from the very beginning.

In fact, we should also inherit the qgroup reservation space, just like exclusive and reference numbers.

Fix by using the newly introduced qgroup_rsv_increase/decrease_by_qgroup() function call.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

fs/btrfs/qgroup.c +2 -3

index 7b89da9589c1..ba6f60fd0e96 100644
@@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
 #endif
 	qgroup->reserved = 0;
 }
+
 /*
- * The easy accounting, if we are adding/removing the only ref for an extent
- * then this qgroup and all of the parent qgroups get their reference and
- * exclusive counts adjusted.
+ * The easy accounting, we're updating qgroup relationship whose child qgroup

fs/btrfs/qgroup.c static void report_reserved_underflow(struct btrfs_fs_info *fs_info, · line 1077

Quoted 3 lines

fs/btrfs/qgroup.c +3 -0

+ * only have exclusive extents.
+ * In this case, we only need to update the rfer/excl, and inherit rsv from
+ * child qgroup (@src)

I'm not sure I understand this inheritance model. Typically a child will inherit from one (or more) parents, but this seems to go the opposite way. (Perhaps it's just terminology, but I may have missed something here.)

Can these relationships form a hierarchy (i.e., grandparents, grandchildren, etc.)?

Thanks,
Ed

Quoted 66 lines

fs/btrfs/qgroup.c +13 -18

  *
  * Caller should hold fs_info->qgroup_lock.
  */
 static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 				    struct ulist *tmp, u64 ref_root,
-				    u64 num_bytes, int sign)
+				    struct btrfs_qgroup *src, int sign)
 {
 	struct btrfs_qgroup *qgroup;
 	struct btrfs_qgroup_list *glist;
 	struct ulist_node *unode;
 	struct ulist_iterator uiter;
+	u64 num_bytes = src->excl;
 	int ret = 0;
 
 	qgroup = find_qgroup_rb(fs_info, ref_root);

@@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
-	if (sign > 0) {
-		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
-		if (qgroup->reserved < num_bytes)
-			report_reserved_underflow(fs_info, qgroup, num_bytes);
-		else
-			qgroup->reserved -= num_bytes;
-	}
+
+	/* *Inherit* qgroup rsv info from @src */
+	if (sign > 0)
+		qgroup_rsv_increase_by_qgroup(qgroup, src);
+	else
+		qgroup_rsv_decrease_by_qgroup(qgroup, src);
 
 	qgroup_dirty(fs_info, qgroup);
 

@@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		qgroup->rfer_cmpr += sign * num_bytes;
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
-		if (sign > 0) {
-			trace_qgroup_update_reserve(fs_info, qgroup,
-						    -(s64)num_bytes);
-			if (qgroup->reserved < num_bytes)
-				report_reserved_underflow(fs_info, qgroup,
-							  num_bytes);
-			else
-				qgroup->reserved -= num_bytes;
-		}
+		if (sign > 0)
+			qgroup_rsv_increase_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_decrease_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 

@@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 	if (qgroup->excl == qgroup->rfer) {
 		ret = 0;
 		err = __qgroup_excl_accounting(fs_info, tmp, dst,
-					       qgroup->excl, sign);
+					       qgroup, sign);
 		if (err < 0) {
 			ret = err;
 			goto out;