[PATCH 0/9] btrfs: some header cleanups and move things around

[PATCH 0/9] btrfs: some header cleanups and move things around

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Move some misplaced prototypes, macros and functions around and some
header cleanups. Trivial changes, details in the change logs.

Filipe Manana (9):
  btrfs: move abort_should_print_stack() to transaction.h
  btrfs: move csum related functions from ctree.c into fs.c
  btrfs: move the exclusive operation functions into fs.c
  btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c
  btrfs: move the folio ordered helpers from ctree.h into fs.h
  btrfs: move BTRFS_BYTES_TO_BLKS() into fs.h
  btrfs: move btrfs_alloc_write_mask() into fs.h
  btrfs: move extent-tree function declarations out of ctree.h
  btrfs: remove pointless comment from ctree.h

 fs/btrfs/ctree.c            |  67 -----------------
 fs/btrfs/ctree.h            |  29 --------
 fs/btrfs/extent-tree.h      |   4 ++
 fs/btrfs/free-space-cache.c |   2 +-
 fs/btrfs/fs.c               | 139 ++++++++++++++++++++++++++++++++++++
 fs/btrfs/fs.h               |  24 +++++++
 fs/btrfs/ioctl.c            |  91 -----------------------
 fs/btrfs/ioctl.h            |   1 -
 fs/btrfs/transaction.h      |  18 ++++-
 fs/btrfs/volumes.c          |   2 +-
 10 files changed, 185 insertions(+), 192 deletions(-)

-- 
2.45.2

[PATCH 1/9] btrfs: move abort_should_print_stack() to transaction.h

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The function abort_should_print_stack() is declared in transaction.h but
its definition is in ctree.c, which doesn't make sense since ctree.c is
the btree implementation and the function is related to the transaction
code. Move its definition into transaction.h as an inline function since
it's a very short and trivial function, and also add the 'btrfs_' prefix
into its name.

This change also reduces the module size.

Before this change:

  $ size fs/btrfs/btrfs.ko
     text	   data	    bss	    dec	    hex	filename
  1783148	 161137	  16920	1961205	 1decf5	fs/btrfs/btrfs.ko

After this change:

  $ size fs/btrfs/btrfs.ko
     text	   data	    bss	    dec	    hex	filename
  1782126	 161045	  16920	1960091	 1de89b	fs/btrfs/btrfs.ko

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.c       | 16 ----------------
 fs/btrfs/transaction.h | 18 ++++++++++++++++--
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 185985a337b3..99a58ede387e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -225,22 +225,6 @@ noinline void btrfs_release_path(struct btrfs_path *p)
 	}
 }
 
-/*
- * We want the transaction abort to print stack trace only for errors where the
- * cause could be a bug, eg. due to ENOSPC, and not for common errors that are
- * caused by external factors.
- */
-bool __cold abort_should_print_stack(int error)
-{
-	switch (error) {
-	case -EIO:
-	case -EROFS:
-	case -ENOMEM:
-		return false;
-	}
-	return true;
-}
-
 /*
  * safely gets a reference on the root node of a tree.  A lock
  * is not taken, so a concurrent writer may put a different node
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 184fa5c0062a..9f7c777af635 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -227,7 +227,21 @@ static inline void btrfs_clear_skip_qgroup(struct btrfs_trans_handle *trans)
 	delayed_refs->qgroup_to_skip = 0;
 }
 
-bool __cold abort_should_print_stack(int error);
+/*
+ * We want the transaction abort to print stack trace only for errors where the
+ * cause could be a bug, eg. due to ENOSPC, and not for common errors that are
+ * caused by external factors.
+ */
+static inline bool btrfs_abort_should_print_stack(int error)
+{
+	switch (error) {
+	case -EIO:
+	case -EROFS:
+	case -ENOMEM:
+		return false;
+	}
+	return true;
+}
 
 /*
  * Call btrfs_abort_transaction as early as possible when an error condition is
@@ -240,7 +254,7 @@ do {								\
 	if (!test_and_set_bit(BTRFS_FS_STATE_TRANS_ABORTED,	\
 			&((trans)->fs_info->fs_state))) {	\
 		__first = true;					\
-		if (WARN(abort_should_print_stack(error),	\
+		if (WARN(btrfs_abort_should_print_stack(error),	\
 			KERN_ERR				\
 			"BTRFS: Transaction aborted (error %d)\n",	\
 			(error))) {					\
-- 
2.45.2

[PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The ctree module is about the implementation of the btree data structure
and not a place holder for generic filesystem things like the csum
algorithm details. Move the functions related to the csum algorithm
details away from ctree.c and into fs.c, which is a far better place for
them. Also fix missing punctuation in comments and change one multiline
comment to a single line comment since everything fits in under 80
characters.

For some reason this also sligthly reduces the module's size.

Before this change:

  $ size fs/btrfs/btrfs.ko
     text	   data	    bss	    dec	    hex	filename
  1782126	 161045	  16920	1960091	 1de89b	fs/btrfs/btrfs.ko

After this change:

  $ size fs/btrfs/btrfs.ko
     text	   data	    bss	    dec	    hex	filename
  1782094	 161045	  16920	1960059	 1de87b	fs/btrfs/btrfs.ko

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.c | 51 ------------------------------------------------
 fs/btrfs/ctree.h |  6 ------
 fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/fs.h    |  6 ++++++
 4 files changed, 55 insertions(+), 57 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 99a58ede387e..c93f52a30a16 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -37,19 +37,6 @@ static int push_node_left(struct btrfs_trans_handle *trans,
 static int balance_node_right(struct btrfs_trans_handle *trans,
 			      struct extent_buffer *dst_buf,
 			      struct extent_buffer *src_buf);
-
-static const struct btrfs_csums {
-	u16		size;
-	const char	name[10];
-	const char	driver[12];
-} btrfs_csums[] = {
-	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
-	[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
-	[BTRFS_CSUM_TYPE_SHA256] = { .size = 32, .name = "sha256" },
-	[BTRFS_CSUM_TYPE_BLAKE2] = { .size = 32, .name = "blake2b",
-				     .driver = "blake2b-256" },
-};
-
 /*
  * The leaf data grows from end-to-front in the node.  this returns the address
  * of the start of the last item, which is the stop of the leaf data stack.
@@ -148,44 +135,6 @@ static inline void copy_leaf_items(const struct extent_buffer *dst,
 			      nr_items * sizeof(struct btrfs_item));
 }
 
-/* This exists for btrfs-progs usages. */
-u16 btrfs_csum_type_size(u16 type)
-{
-	return btrfs_csums[type].size;
-}
-
-int btrfs_super_csum_size(const struct btrfs_super_block *s)
-{
-	u16 t = btrfs_super_csum_type(s);
-	/*
-	 * csum type is validated at mount time
-	 */
-	return btrfs_csum_type_size(t);
-}
-
-const char *btrfs_super_csum_name(u16 csum_type)
-{
-	/* csum type is validated at mount time */
-	return btrfs_csums[csum_type].name;
-}
-
-/*
- * Return driver name if defined, otherwise the name that's also a valid driver
- * name
- */
-const char *btrfs_super_csum_driver(u16 csum_type)
-{
-	/* csum type is validated at mount time */
-	return btrfs_csums[csum_type].driver[0] ?
-		btrfs_csums[csum_type].driver :
-		btrfs_csums[csum_type].name;
-}
-
-size_t __attribute_const__ btrfs_get_num_csums(void)
-{
-	return ARRAY_SIZE(btrfs_csums);
-}
-
 struct btrfs_path *btrfs_alloc_path(void)
 {
 	might_sleep();
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2c341956a01c..a1bab0b3f193 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -756,12 +756,6 @@ static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
 	return root->root_key.objectid == BTRFS_DATA_RELOC_TREE_OBJECTID;
 }
 
-u16 btrfs_csum_type_size(u16 type);
-int btrfs_super_csum_size(const struct btrfs_super_block *s);
-const char *btrfs_super_csum_name(u16 csum_type);
-const char *btrfs_super_csum_driver(u16 csum_type);
-size_t __attribute_const__ btrfs_get_num_csums(void);
-
 /*
  * We use folio flag owner_2 to indicate there is an ordered extent with
  * unfinished IO.
diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
index 31c1648bc0b4..3756a3b9c9da 100644
--- a/fs/btrfs/fs.c
+++ b/fs/btrfs/fs.c
@@ -5,6 +5,55 @@
 #include "fs.h"
 #include "accessors.h"
 
+static const struct btrfs_csums {
+	u16		size;
+	const char	name[10];
+	const char	driver[12];
+} btrfs_csums[] = {
+	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
+	[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
+	[BTRFS_CSUM_TYPE_SHA256] = { .size = 32, .name = "sha256" },
+	[BTRFS_CSUM_TYPE_BLAKE2] = { .size = 32, .name = "blake2b",
+				     .driver = "blake2b-256" },
+};
+
+/* This exists for btrfs-progs usages. */
+u16 btrfs_csum_type_size(u16 type)
+{
+	return btrfs_csums[type].size;
+}
+
+int btrfs_super_csum_size(const struct btrfs_super_block *s)
+{
+	u16 t = btrfs_super_csum_type(s);
+
+	/* csum type is validated at mount time. */
+	return btrfs_csum_type_size(t);
+}
+
+const char *btrfs_super_csum_name(u16 csum_type)
+{
+	/* csum type is validated at mount time. */
+	return btrfs_csums[csum_type].name;
+}
+
+/*
+ * Return driver name if defined, otherwise the name that's also a valid driver
+ * name.
+ */
+const char *btrfs_super_csum_driver(u16 csum_type)
+{
+	/* csum type is validated at mount time */
+	return btrfs_csums[csum_type].driver[0] ?
+		btrfs_csums[csum_type].driver :
+		btrfs_csums[csum_type].name;
+}
+
+size_t __attribute_const__ btrfs_get_num_csums(void)
+{
+	return ARRAY_SIZE(btrfs_csums);
+}
+
 void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
 			     const char *name)
 {
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index 79a1a3d6f04d..b05f2af97140 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -982,6 +982,12 @@ void btrfs_exclop_balance(struct btrfs_fs_info *fs_info,
 
 int btrfs_check_ioctl_vol_args_path(const struct btrfs_ioctl_vol_args *vol_args);
 
+u16 btrfs_csum_type_size(u16 type);
+int btrfs_super_csum_size(const struct btrfs_super_block *s);
+const char *btrfs_super_csum_name(u16 csum_type);
+const char *btrfs_super_csum_driver(u16 csum_type);
+size_t __attribute_const__ btrfs_get_num_csums(void);
+
 /* Compatibility and incompatibility defines */
 void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
 			     const char *name);
-- 
2.45.2

[PATCH 3/9] btrfs: move the exclusive operation functions into fs.c

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The declarations for the exclusive operation functions are located at fs.h
but their definitions are in ioctl.c, which doesn't make much sense since
(most of them) are used in several files other than ioctl.c. Since they
are used in several files and they are generic enough, move them out of
ioctl.c and into fs.c, even the ones that are currently only used at
ioctl.c, for the sake of having them all in the same C file.

This also reduces the module's size.

Before this change:

  $ size fs/btrfs/btrfs.ko
     text	   data	    bss	    dec	    hex	filename
  1782094	 161045	  16920	1960059	 1de87b	fs/btrfs/btrfs.ko

After this change:

  $ size fs/btrfs/btrfs.ko
     text	   data	    bss	    dec	    hex	filename
  1781492	 161037	  16920	1959449	 1de619	fs/btrfs/btrfs.ko

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/fs.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/ioctl.c | 80 -----------------------------------------------
 2 files changed, 81 insertions(+), 80 deletions(-)

diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
index 3756a3b9c9da..09cfb43580cb 100644
--- a/fs/btrfs/fs.c
+++ b/fs/btrfs/fs.c
@@ -4,6 +4,7 @@
 #include "ctree.h"
 #include "fs.h"
 #include "accessors.h"
+#include "volumes.h"
 
 static const struct btrfs_csums {
 	u16		size;
@@ -54,6 +55,86 @@ size_t __attribute_const__ btrfs_get_num_csums(void)
 	return ARRAY_SIZE(btrfs_csums);
 }
 
+/*
+ * Start exclusive operation @type, return true on success.
+ */
+bool btrfs_exclop_start(struct btrfs_fs_info *fs_info,
+			enum btrfs_exclusive_operation type)
+{
+	bool ret = false;
+
+	spin_lock(&fs_info->super_lock);
+	if (fs_info->exclusive_operation == BTRFS_EXCLOP_NONE) {
+		fs_info->exclusive_operation = type;
+		ret = true;
+	}
+	spin_unlock(&fs_info->super_lock);
+
+	return ret;
+}
+
+/*
+ * Conditionally allow to enter the exclusive operation in case it's compatible
+ * with the running one.  This must be paired with btrfs_exclop_start_unlock()
+ * and btrfs_exclop_finish().
+ *
+ * Compatibility:
+ * - the same type is already running
+ * - when trying to add a device and balance has been paused
+ * - not BTRFS_EXCLOP_NONE - this is intentionally incompatible and the caller
+ *   must check the condition first that would allow none -> @type
+ */
+bool btrfs_exclop_start_try_lock(struct btrfs_fs_info *fs_info,
+				 enum btrfs_exclusive_operation type)
+{
+	spin_lock(&fs_info->super_lock);
+	if (fs_info->exclusive_operation == type ||
+	    (fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED &&
+	     type == BTRFS_EXCLOP_DEV_ADD))
+		return true;
+
+	spin_unlock(&fs_info->super_lock);
+	return false;
+}
+
+void btrfs_exclop_start_unlock(struct btrfs_fs_info *fs_info)
+{
+	spin_unlock(&fs_info->super_lock);
+}
+
+void btrfs_exclop_finish(struct btrfs_fs_info *fs_info)
+{
+	spin_lock(&fs_info->super_lock);
+	WRITE_ONCE(fs_info->exclusive_operation, BTRFS_EXCLOP_NONE);
+	spin_unlock(&fs_info->super_lock);
+	sysfs_notify(&fs_info->fs_devices->fsid_kobj, NULL, "exclusive_operation");
+}
+
+void btrfs_exclop_balance(struct btrfs_fs_info *fs_info,
+			  enum btrfs_exclusive_operation op)
+{
+	switch (op) {
+	case BTRFS_EXCLOP_BALANCE_PAUSED:
+		spin_lock(&fs_info->super_lock);
+		ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE ||
+		       fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD ||
+		       fs_info->exclusive_operation == BTRFS_EXCLOP_NONE ||
+		       fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED);
+		fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE_PAUSED;
+		spin_unlock(&fs_info->super_lock);
+		break;
+	case BTRFS_EXCLOP_BALANCE:
+		spin_lock(&fs_info->super_lock);
+		ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED);
+		fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE;
+		spin_unlock(&fs_info->super_lock);
+		break;
+	default:
+		btrfs_warn(fs_info,
+			"invalid exclop balance operation %d requested", op);
+	}
+}
+
 void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
 			     const char *name)
 {
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index dc5faa89cdba..0ede6a5524c2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -403,86 +403,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 	return ret;
 }
 
-/*
- * Start exclusive operation @type, return true on success
- */
-bool btrfs_exclop_start(struct btrfs_fs_info *fs_info,
-			enum btrfs_exclusive_operation type)
-{
-	bool ret = false;
-
-	spin_lock(&fs_info->super_lock);
-	if (fs_info->exclusive_operation == BTRFS_EXCLOP_NONE) {
-		fs_info->exclusive_operation = type;
-		ret = true;
-	}
-	spin_unlock(&fs_info->super_lock);
-
-	return ret;
-}
-
-/*
- * Conditionally allow to enter the exclusive operation in case it's compatible
- * with the running one.  This must be paired with btrfs_exclop_start_unlock and
- * btrfs_exclop_finish.
- *
- * Compatibility:
- * - the same type is already running
- * - when trying to add a device and balance has been paused
- * - not BTRFS_EXCLOP_NONE - this is intentionally incompatible and the caller
- *   must check the condition first that would allow none -> @type
- */
-bool btrfs_exclop_start_try_lock(struct btrfs_fs_info *fs_info,
-				 enum btrfs_exclusive_operation type)
-{
-	spin_lock(&fs_info->super_lock);
-	if (fs_info->exclusive_operation == type ||
-	    (fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED &&
-	     type == BTRFS_EXCLOP_DEV_ADD))
-		return true;
-
-	spin_unlock(&fs_info->super_lock);
-	return false;
-}
-
-void btrfs_exclop_start_unlock(struct btrfs_fs_info *fs_info)
-{
-	spin_unlock(&fs_info->super_lock);
-}
-
-void btrfs_exclop_finish(struct btrfs_fs_info *fs_info)
-{
-	spin_lock(&fs_info->super_lock);
-	WRITE_ONCE(fs_info->exclusive_operation, BTRFS_EXCLOP_NONE);
-	spin_unlock(&fs_info->super_lock);
-	sysfs_notify(&fs_info->fs_devices->fsid_kobj, NULL, "exclusive_operation");
-}
-
-void btrfs_exclop_balance(struct btrfs_fs_info *fs_info,
-			  enum btrfs_exclusive_operation op)
-{
-	switch (op) {
-	case BTRFS_EXCLOP_BALANCE_PAUSED:
-		spin_lock(&fs_info->super_lock);
-		ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE ||
-		       fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD ||
-		       fs_info->exclusive_operation == BTRFS_EXCLOP_NONE ||
-		       fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED);
-		fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE_PAUSED;
-		spin_unlock(&fs_info->super_lock);
-		break;
-	case BTRFS_EXCLOP_BALANCE:
-		spin_lock(&fs_info->super_lock);
-		ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED);
-		fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE;
-		spin_unlock(&fs_info->super_lock);
-		break;
-	default:
-		btrfs_warn(fs_info,
-			"invalid exclop balance operation %d requested", op);
-	}
-}
-
 static int btrfs_ioctl_getversion(struct inode *inode, int __user *arg)
 {
 	return put_user(inode->i_generation, arg);
-- 
2.45.2

[PATCH 4/9] btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

It's a generic helper not specific to ioctls and used in several places,
so move it out from ioctl.c and into fs.c. While at it change its return
type from int to bool and declare the loop variable in the loop itself.

This also slightly reduces the module's size.

Before this change:

  $ size fs/btrfs/btrfs.ko
     text	   data	    bss	    dec	    hex	filename
  1781492	 161037	  16920	1959449	 1de619	fs/btrfs/btrfs.ko

After this change:

  $ size fs/btrfs/btrfs.ko
     text	   data	    bss	    dec	    hex	filename
  1781340	 161037	  16920	1959297	 1de581	fs/btrfs/btrfs.ko

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/fs.c    |  9 +++++++++
 fs/btrfs/fs.h    |  2 ++
 fs/btrfs/ioctl.c | 11 -----------
 fs/btrfs/ioctl.h |  1 -
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
index 09cfb43580cb..06a863252a85 100644
--- a/fs/btrfs/fs.c
+++ b/fs/btrfs/fs.c
@@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void)
 	return ARRAY_SIZE(btrfs_csums);
 }
 
+bool __pure btrfs_is_empty_uuid(const u8 *uuid)
+{
+	for (int i = 0; i < BTRFS_UUID_SIZE; i++) {
+		if (uuid[i] != 0)
+			return false;
+	}
+	return true;
+}
+
 /*
  * Start exclusive operation @type, return true on success.
  */
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index b05f2af97140..15c26c6f4d6e 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -988,6 +988,8 @@ const char *btrfs_super_csum_name(u16 csum_type);
 const char *btrfs_super_csum_driver(u16 csum_type);
 size_t __attribute_const__ btrfs_get_num_csums(void);
 
+bool __pure btrfs_is_empty_uuid(const u8 *uuid);
+
 /* Compatibility and incompatibility defines */
 void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
 			     const char *name);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0ede6a5524c2..7872de140489 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -471,17 +471,6 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
-int __pure btrfs_is_empty_uuid(const u8 *uuid)
-{
-	int i;
-
-	for (i = 0; i < BTRFS_UUID_SIZE; i++) {
-		if (uuid[i])
-			return 0;
-	}
-	return 1;
-}
-
 /*
  * Calculate the number of transaction items to reserve for creating a subvolume
  * or snapshot, not including the inode, directory entries, or parent directory.
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index 2b760c8778f8..ce915fcda43b 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -19,7 +19,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 		       struct dentry *dentry, struct fileattr *fa);
 int btrfs_ioctl_get_supported_features(void __user *arg);
 void btrfs_sync_inode_flags_to_i_flags(struct inode *inode);
-int __pure btrfs_is_empty_uuid(const u8 *uuid);
 void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
 				     struct btrfs_ioctl_balance_args *bargs);
 int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags);
-- 
2.45.2

[PATCH 5/9] btrfs: move the folio ordered helpers from ctree.h into fs.h

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The folio ordered helper macros are defined at ctree.h but this is not
the best place since ctree.{h,c} is all about the btree data structure
implementation and not a generic module. So move these macros into the
fs.h header.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h | 8 --------
 fs/btrfs/fs.h    | 8 ++++++++
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a1bab0b3f193..3d9855d30057 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -756,12 +756,4 @@ static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
 	return root->root_key.objectid == BTRFS_DATA_RELOC_TREE_OBJECTID;
 }
 
-/*
- * We use folio flag owner_2 to indicate there is an ordered extent with
- * unfinished IO.
- */
-#define folio_test_ordered(folio)	folio_test_owner_2(folio)
-#define folio_set_ordered(folio)	folio_set_owner_2(folio)
-#define folio_clear_ordered(folio)	folio_clear_owner_2(folio)
-
 #endif
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index 15c26c6f4d6e..7a27f5fe9bc2 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -1066,6 +1066,14 @@ static inline void btrfs_wake_unfinished_drop(struct btrfs_fs_info *fs_info)
 	(unlikely(test_bit(BTRFS_FS_STATE_LOG_CLEANUP_ERROR,		\
 			   &(fs_info)->fs_state)))
 
+/*
+ * We use folio flag owner_2 to indicate there is an ordered extent with
+ * unfinished IO.
+ */
+#define folio_test_ordered(folio)	folio_test_owner_2(folio)
+#define folio_set_ordered(folio)	folio_set_owner_2(folio)
+#define folio_clear_ordered(folio)	folio_clear_owner_2(folio)
+
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 
 #define EXPORT_FOR_TESTS
-- 
2.45.2

[PATCH 6/9] btrfs: move BTRFS_BYTES_TO_BLKS() into fs.h

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Currently BTRFS_BYTES_TO_BLKS() is defined in ctree.h but it's not related
at all to the btree data structure, so move it into fs.h.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h | 3 ---
 fs/btrfs/fs.h    | 2 ++
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 3d9855d30057..bf054470dcd0 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -506,9 +506,6 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
 	return BTRFS_MAX_ITEM_SIZE(info) - sizeof(struct btrfs_dir_item);
 }
 
-#define BTRFS_BYTES_TO_BLKS(fs_info, bytes) \
-				((bytes) >> (fs_info)->sectorsize_bits)
-
 static inline gfp_t btrfs_alloc_write_mask(struct address_space *mapping)
 {
 	return mapping_gfp_constraint(mapping, ~__GFP_FS);
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index 7a27f5fe9bc2..dd1a82297d4c 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -953,6 +953,8 @@ static inline u64 btrfs_calc_metadata_size(const struct btrfs_fs_info *fs_info,
 #define BTRFS_MAX_EXTENT_ITEM_SIZE(r) ((BTRFS_LEAF_DATA_SIZE(r->fs_info) >> 4) - \
 					sizeof(struct btrfs_item))
 
+#define BTRFS_BYTES_TO_BLKS(fs_info, bytes) ((bytes) >> (fs_info)->sectorsize_bits)
+
 static inline bool btrfs_is_zoned(const struct btrfs_fs_info *fs_info)
 {
 	return IS_ENABLED(CONFIG_BLK_DEV_ZONED) && fs_info->zone_size > 0;
-- 
2.45.2

[PATCH 7/9] btrfs: move btrfs_alloc_write_mask() into fs.h

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Currently btrfs_alloc_write_mask() is defined in ctree.h but it's not
related at all to the btree data structure, so move it into fs.h.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h | 6 ------
 fs/btrfs/fs.h    | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index bf054470dcd0..53f9fc04f66f 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -7,7 +7,6 @@
 #define BTRFS_CTREE_H
 
 #include "linux/cleanup.h"
-#include <linux/pagemap.h>
 #include <linux/spinlock.h>
 #include <linux/rbtree.h>
 #include <linux/mutex.h>
@@ -506,11 +505,6 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
 	return BTRFS_MAX_ITEM_SIZE(info) - sizeof(struct btrfs_dir_item);
 }
 
-static inline gfp_t btrfs_alloc_write_mask(struct address_space *mapping)
-{
-	return mapping_gfp_constraint(mapping, ~__GFP_FS);
-}
-
 void btrfs_error_unpin_extent_range(struct btrfs_fs_info *fs_info, u64 start, u64 end);
 int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
 			 u64 num_bytes, u64 *actual_bytes);
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index dd1a82297d4c..1113646374f3 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -18,6 +18,7 @@
 #include <linux/rwsem.h>
 #include <linux/semaphore.h>
 #include <linux/list.h>
+#include <linux/pagemap.h>
 #include <linux/radix-tree.h>
 #include <linux/workqueue.h>
 #include <linux/wait.h>
@@ -887,6 +888,11 @@ struct btrfs_fs_info {
 #define inode_to_fs_info(_inode) (BTRFS_I(_Generic((_inode),			\
 					   struct inode *: (_inode)))->root->fs_info)
 
+static inline gfp_t btrfs_alloc_write_mask(struct address_space *mapping)
+{
+	return mapping_gfp_constraint(mapping, ~__GFP_FS);
+}
+
 static inline u64 btrfs_get_fs_generation(const struct btrfs_fs_info *fs_info)
 {
 	return READ_ONCE(fs_info->generation);
-- 
2.45.2

[PATCH 8/9] btrfs: move extent-tree function declarations out of ctree.h

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have 3 functions that have their prototypes declared in ctree.h but
they are defined at extent-tree.c and they are unrelated to the btree
data structure. Move the prototypes out of ctree.h and into extent-tree.h.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h            | 5 -----
 fs/btrfs/extent-tree.h      | 4 ++++
 fs/btrfs/free-space-cache.c | 2 +-
 fs/btrfs/volumes.c          | 2 +-
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 53f9fc04f66f..cdf10cca8194 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -505,11 +505,6 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
 	return BTRFS_MAX_ITEM_SIZE(info) - sizeof(struct btrfs_dir_item);
 }
 
-void btrfs_error_unpin_extent_range(struct btrfs_fs_info *fs_info, u64 start, u64 end);
-int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
-			 u64 num_bytes, u64 *actual_bytes);
-int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range);
-
 /* ctree.c */
 int __init btrfs_ctree_init(void);
 void __cold btrfs_ctree_exit(void);
diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
index 46b8e19022df..cfa52264f678 100644
--- a/fs/btrfs/extent-tree.h
+++ b/fs/btrfs/extent-tree.h
@@ -162,5 +162,9 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 			struct btrfs_root *root,
 			struct extent_buffer *node,
 			struct extent_buffer *parent);
+void btrfs_error_unpin_extent_range(struct btrfs_fs_info *fs_info, u64 start, u64 end);
+int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
+			 u64 num_bytes, u64 *actual_bytes);
+int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range);
 
 #endif
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index cfa52ef40b06..17707c898eae 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -12,7 +12,7 @@
 #include <linux/error-injection.h>
 #include <linux/sched/mm.h>
 #include <linux/string_choices.h>
-#include "ctree.h"
+#include "extent-tree.h"
 #include "fs.h"
 #include "messages.h"
 #include "misc.h"
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1cccaf9c2b0d..7ebfc978cade 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -13,8 +13,8 @@
 #include <linux/list_sort.h>
 #include <linux/namei.h>
 #include "misc.h"
-#include "ctree.h"
 #include "disk-io.h"
+#include "extent-tree.h"
 #include "transaction.h"
 #include "volumes.h"
 #include "raid56.h"
-- 
2.45.2

[PATCH 9/9] btrfs: remove pointless comment from ctree.h

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

It's pointless to have a comment above the prototype declarations of
btrfs_ctree_init() and btrfs_ctree_exit() mentioning that they are
declared in ctree.c. This is from the old days when ctree.h was used
to place anything that didn't fit in any other file. So remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index cdf10cca8194..1096a80a64e7 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -505,7 +505,6 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
 	return BTRFS_MAX_ITEM_SIZE(info) - sizeof(struct btrfs_dir_item);
 }
 
-/* ctree.c */
 int __init btrfs_ctree_init(void);
 void __cold btrfs_ctree_exit(void);
 
-- 
2.45.2

Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[Qu Wenruo]

在 2024/12/17 03:47, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
>
> The ctree module is about the implementation of the btree data structure
> and not a place holder for generic filesystem things like the csum
> algorithm details. Move the functions related to the csum algorithm
> details away from ctree.c and into fs.c, which is a far better place for
> them. Also fix missing punctuation in comments and change one multiline
> comment to a single line comment since everything fits in under 80
> characters.
>
> For some reason this also sligthly reduces the module's size.
>
> Before this change:
>
>    $ size fs/btrfs/btrfs.ko
>       text	   data	    bss	    dec	    hex	filename
>    1782126	 161045	  16920	1960091	 1de89b	fs/btrfs/btrfs.ko
>
> After this change:
>
>    $ size fs/btrfs/btrfs.ko
>       text	   data	    bss	    dec	    hex	filename
>    1782094	 161045	  16920	1960059	 1de87b	fs/btrfs/btrfs.ko
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>   fs/btrfs/ctree.c | 51 ------------------------------------------------
>   fs/btrfs/ctree.h |  6 ------
>   fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>   fs/btrfs/fs.h    |  6 ++++++
>   4 files changed, 55 insertions(+), 57 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 99a58ede387e..c93f52a30a16 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -37,19 +37,6 @@ static int push_node_left(struct btrfs_trans_handle *trans,
>   static int balance_node_right(struct btrfs_trans_handle *trans,
>   			      struct extent_buffer *dst_buf,
>   			      struct extent_buffer *src_buf);
> -
> -static const struct btrfs_csums {
> -	u16		size;
> -	const char	name[10];
> -	const char	driver[12];
> -} btrfs_csums[] = {
> -	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
> -	[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
> -	[BTRFS_CSUM_TYPE_SHA256] = { .size = 32, .name = "sha256" },
> -	[BTRFS_CSUM_TYPE_BLAKE2] = { .size = 32, .name = "blake2b",
> -				     .driver = "blake2b-256" },
> -};
> -
>   /*
>    * The leaf data grows from end-to-front in the node.  this returns the address
>    * of the start of the last item, which is the stop of the leaf data stack.
> @@ -148,44 +135,6 @@ static inline void copy_leaf_items(const struct extent_buffer *dst,
>   			      nr_items * sizeof(struct btrfs_item));
>   }
>
> -/* This exists for btrfs-progs usages. */
> -u16 btrfs_csum_type_size(u16 type)
> -{
> -	return btrfs_csums[type].size;
> -}
> -
> -int btrfs_super_csum_size(const struct btrfs_super_block *s)
> -{
> -	u16 t = btrfs_super_csum_type(s);
> -	/*
> -	 * csum type is validated at mount time
> -	 */
> -	return btrfs_csum_type_size(t);
> -}
> -
> -const char *btrfs_super_csum_name(u16 csum_type)
> -{
> -	/* csum type is validated at mount time */
> -	return btrfs_csums[csum_type].name;
> -}
> -
> -/*
> - * Return driver name if defined, otherwise the name that's also a valid driver
> - * name
> - */
> -const char *btrfs_super_csum_driver(u16 csum_type)
> -{
> -	/* csum type is validated at mount time */
> -	return btrfs_csums[csum_type].driver[0] ?
> -		btrfs_csums[csum_type].driver :
> -		btrfs_csums[csum_type].name;
> -}
> -
> -size_t __attribute_const__ btrfs_get_num_csums(void)
> -{
> -	return ARRAY_SIZE(btrfs_csums);
> -}
> -
>   struct btrfs_path *btrfs_alloc_path(void)
>   {
>   	might_sleep();
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 2c341956a01c..a1bab0b3f193 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -756,12 +756,6 @@ static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
>   	return root->root_key.objectid == BTRFS_DATA_RELOC_TREE_OBJECTID;
>   }
>
> -u16 btrfs_csum_type_size(u16 type);
> -int btrfs_super_csum_size(const struct btrfs_super_block *s);
> -const char *btrfs_super_csum_name(u16 csum_type);
> -const char *btrfs_super_csum_driver(u16 csum_type);
> -size_t __attribute_const__ btrfs_get_num_csums(void);
> -
>   /*
>    * We use folio flag owner_2 to indicate there is an ordered extent with
>    * unfinished IO.
> diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
> index 31c1648bc0b4..3756a3b9c9da 100644
> --- a/fs/btrfs/fs.c
> +++ b/fs/btrfs/fs.c
> @@ -5,6 +5,55 @@
>   #include "fs.h"
>   #include "accessors.h"
>
> +static const struct btrfs_csums {
> +	u16		size;
> +	const char	name[10];
> +	const char	driver[12];
> +} btrfs_csums[] = {
> +	[BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
> +	[BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
> +	[BTRFS_CSUM_TYPE_SHA256] = { .size = 32, .name = "sha256" },
> +	[BTRFS_CSUM_TYPE_BLAKE2] = { .size = 32, .name = "blake2b",
> +				     .driver = "blake2b-256" },
> +};
> +
> +/* This exists for btrfs-progs usages. */
> +u16 btrfs_csum_type_size(u16 type)
> +{
> +	return btrfs_csums[type].size;
> +}
> +
> +int btrfs_super_csum_size(const struct btrfs_super_block *s)
> +{
> +	u16 t = btrfs_super_csum_type(s);
> +
> +	/* csum type is validated at mount time. */
> +	return btrfs_csum_type_size(t);
> +}
> +
> +const char *btrfs_super_csum_name(u16 csum_type)
> +{
> +	/* csum type is validated at mount time. */
> +	return btrfs_csums[csum_type].name;
> +}
> +
> +/*
> + * Return driver name if defined, otherwise the name that's also a valid driver
> + * name.
> + */
> +const char *btrfs_super_csum_driver(u16 csum_type)
> +{
> +	/* csum type is validated at mount time */
> +	return btrfs_csums[csum_type].driver[0] ?
> +		btrfs_csums[csum_type].driver :
> +		btrfs_csums[csum_type].name;
> +}
> +
> +size_t __attribute_const__ btrfs_get_num_csums(void)
> +{
> +	return ARRAY_SIZE(btrfs_csums);
> +}
> +

I'm wondering if those simple functions can be converted to inline.

Although btrfs_csums[] array has to be put inside fs.c, those functions
seems be fine defined as inline ones inside the header.

Otherwise looks good to me.

Thanks,
Qu
>   void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
>   			     const char *name)
>   {
> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index 79a1a3d6f04d..b05f2af97140 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -982,6 +982,12 @@ void btrfs_exclop_balance(struct btrfs_fs_info *fs_info,
>
>   int btrfs_check_ioctl_vol_args_path(const struct btrfs_ioctl_vol_args *vol_args);
>
> +u16 btrfs_csum_type_size(u16 type);
> +int btrfs_super_csum_size(const struct btrfs_super_block *s);
> +const char *btrfs_super_csum_name(u16 csum_type);
> +const char *btrfs_super_csum_driver(u16 csum_type);
> +size_t __attribute_const__ btrfs_get_num_csums(void);
> +
>   /* Compatibility and incompatibility defines */
>   void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
>   			     const char *name);

Re: [PATCH 4/9] btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c

[Qu Wenruo]

在 2024/12/17 03:47, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
>
> It's a generic helper not specific to ioctls and used in several places,
> so move it out from ioctl.c and into fs.c. While at it change its return
> type from int to bool and declare the loop variable in the loop itself.
>
> This also slightly reduces the module's size.
>
> Before this change:
>
>    $ size fs/btrfs/btrfs.ko
>       text	   data	    bss	    dec	    hex	filename
>    1781492	 161037	  16920	1959449	 1de619	fs/btrfs/btrfs.ko
>
> After this change:
>
>    $ size fs/btrfs/btrfs.ko
>       text	   data	    bss	    dec	    hex	filename
>    1781340	 161037	  16920	1959297	 1de581	fs/btrfs/btrfs.ko
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>   fs/btrfs/fs.c    |  9 +++++++++
>   fs/btrfs/fs.h    |  2 ++
>   fs/btrfs/ioctl.c | 11 -----------
>   fs/btrfs/ioctl.h |  1 -
>   4 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
> index 09cfb43580cb..06a863252a85 100644
> --- a/fs/btrfs/fs.c
> +++ b/fs/btrfs/fs.c
> @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void)
>   	return ARRAY_SIZE(btrfs_csums);
>   }
>
> +bool __pure btrfs_is_empty_uuid(const u8 *uuid)
> +{
> +	for (int i = 0; i < BTRFS_UUID_SIZE; i++) {
> +		if (uuid[i] != 0)
> +			return false;
> +	}

Since we're here, would it be possible to go with
mem_is_zero()/memchr_inv() which contains some extra optimization.

And if we go calling mem_is_zero()/memchr_inv(), can we change this to
an inline?

Otherwise looks good to me.

Thanks,
Qu
> +	return true;
> +}
> +
>   /*
>    * Start exclusive operation @type, return true on success.
>    */
> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index b05f2af97140..15c26c6f4d6e 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -988,6 +988,8 @@ const char *btrfs_super_csum_name(u16 csum_type);
>   const char *btrfs_super_csum_driver(u16 csum_type);
>   size_t __attribute_const__ btrfs_get_num_csums(void);
>
> +bool __pure btrfs_is_empty_uuid(const u8 *uuid);
> +
>   /* Compatibility and incompatibility defines */
>   void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
>   			     const char *name);
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0ede6a5524c2..7872de140489 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -471,17 +471,6 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info,
>   	return ret;
>   }
>
> -int __pure btrfs_is_empty_uuid(const u8 *uuid)
> -{
> -	int i;
> -
> -	for (i = 0; i < BTRFS_UUID_SIZE; i++) {
> -		if (uuid[i])
> -			return 0;
> -	}
> -	return 1;
> -}
> -
>   /*
>    * Calculate the number of transaction items to reserve for creating a subvolume
>    * or snapshot, not including the inode, directory entries, or parent directory.
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index 2b760c8778f8..ce915fcda43b 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -19,7 +19,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
>   		       struct dentry *dentry, struct fileattr *fa);
>   int btrfs_ioctl_get_supported_features(void __user *arg);
>   void btrfs_sync_inode_flags_to_i_flags(struct inode *inode);
> -int __pure btrfs_is_empty_uuid(const u8 *uuid);
>   void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
>   				     struct btrfs_ioctl_balance_args *bargs);
>   int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags);

Re: [PATCH 0/9] btrfs: some header cleanups and move things around

[Qu Wenruo]

在 2024/12/17 03:47, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Move some misplaced prototypes, macros and functions around and some
> header cleanups. Trivial changes, details in the change logs.

Except some minor comments on possible inline functions and a possible 
optimization using mem_is_zero(), all mentioned in the corresponding 
patches, the series looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> 
> Filipe Manana (9):
>    btrfs: move abort_should_print_stack() to transaction.h
>    btrfs: move csum related functions from ctree.c into fs.c
>    btrfs: move the exclusive operation functions into fs.c
>    btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c
>    btrfs: move the folio ordered helpers from ctree.h into fs.h
>    btrfs: move BTRFS_BYTES_TO_BLKS() into fs.h
>    btrfs: move btrfs_alloc_write_mask() into fs.h
>    btrfs: move extent-tree function declarations out of ctree.h
>    btrfs: remove pointless comment from ctree.h
> 
>   fs/btrfs/ctree.c            |  67 -----------------
>   fs/btrfs/ctree.h            |  29 --------
>   fs/btrfs/extent-tree.h      |   4 ++
>   fs/btrfs/free-space-cache.c |   2 +-
>   fs/btrfs/fs.c               | 139 ++++++++++++++++++++++++++++++++++++
>   fs/btrfs/fs.h               |  24 +++++++
>   fs/btrfs/ioctl.c            |  91 -----------------------
>   fs/btrfs/ioctl.h            |   1 -
>   fs/btrfs/transaction.h      |  18 ++++-
>   fs/btrfs/volumes.c          |   2 +-
>   10 files changed, 185 insertions(+), 192 deletions(-)
> 

Re: [PATCH 4/9] btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c

[Johannes Thumshirn]

On 17.12.24 01:32, Qu Wenruo wrote:
> 
> 
> 在 2024/12/17 03:47, fdmanana@kernel.org 写道:
>> From: Filipe Manana <fdmanana@suse.com>
>>
>> It's a generic helper not specific to ioctls and used in several places,
>> so move it out from ioctl.c and into fs.c. While at it change its return
>> type from int to bool and declare the loop variable in the loop itself.
>>
>> This also slightly reduces the module's size.
>>
>> Before this change:
>>
>>     $ size fs/btrfs/btrfs.ko
>>        text	   data	    bss	    dec	    hex	filename
>>     1781492	 161037	  16920	1959449	 1de619	fs/btrfs/btrfs.ko
>>
>> After this change:
>>
>>     $ size fs/btrfs/btrfs.ko
>>        text	   data	    bss	    dec	    hex	filename
>>     1781340	 161037	  16920	1959297	 1de581	fs/btrfs/btrfs.ko
>>
>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>> ---
>>    fs/btrfs/fs.c    |  9 +++++++++
>>    fs/btrfs/fs.h    |  2 ++
>>    fs/btrfs/ioctl.c | 11 -----------
>>    fs/btrfs/ioctl.h |  1 -
>>    4 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
>> index 09cfb43580cb..06a863252a85 100644
>> --- a/fs/btrfs/fs.c
>> +++ b/fs/btrfs/fs.c
>> @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void)
>>    	return ARRAY_SIZE(btrfs_csums);
>>    }
>>
>> +bool __pure btrfs_is_empty_uuid(const u8 *uuid)
>> +{
>> +	for (int i = 0; i < BTRFS_UUID_SIZE; i++) {
>> +		if (uuid[i] != 0)
>> +			return false;
>> +	}
> 
> Since we're here, would it be possible to go with
> mem_is_zero()/memchr_inv() which contains some extra optimization.
> 
> And if we go calling mem_is_zero()/memchr_inv(), can we change this to
> an inline?
> 
> Otherwise looks good to me.

The generic uuid code also has a uuid_is_null():

bool __pure btrfs_is_empty_uuid(const u8 *uuid)
{
	return uuid_is_null((const uuid_t *)uuid));
}

should work as well, but I've not tested it just eyeballed.

Re: [PATCH 4/9] btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c

[Qu Wenruo]

在 2024/12/17 18:23, Johannes Thumshirn 写道:
> On 17.12.24 01:32, Qu Wenruo wrote:
>>
>>
>> 在 2024/12/17 03:47, fdmanana@kernel.org 写道:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> It's a generic helper not specific to ioctls and used in several places,
>>> so move it out from ioctl.c and into fs.c. While at it change its return
>>> type from int to bool and declare the loop variable in the loop itself.
>>>
>>> This also slightly reduces the module's size.
>>>
>>> Before this change:
>>>
>>>      $ size fs/btrfs/btrfs.ko
>>>         text	   data	    bss	    dec	    hex	filename
>>>      1781492	 161037	  16920	1959449	 1de619	fs/btrfs/btrfs.ko
>>>
>>> After this change:
>>>
>>>      $ size fs/btrfs/btrfs.ko
>>>         text	   data	    bss	    dec	    hex	filename
>>>      1781340	 161037	  16920	1959297	 1de581	fs/btrfs/btrfs.ko
>>>
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>     fs/btrfs/fs.c    |  9 +++++++++
>>>     fs/btrfs/fs.h    |  2 ++
>>>     fs/btrfs/ioctl.c | 11 -----------
>>>     fs/btrfs/ioctl.h |  1 -
>>>     4 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
>>> index 09cfb43580cb..06a863252a85 100644
>>> --- a/fs/btrfs/fs.c
>>> +++ b/fs/btrfs/fs.c
>>> @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void)
>>>     	return ARRAY_SIZE(btrfs_csums);
>>>     }
>>>
>>> +bool __pure btrfs_is_empty_uuid(const u8 *uuid)
>>> +{
>>> +	for (int i = 0; i < BTRFS_UUID_SIZE; i++) {
>>> +		if (uuid[i] != 0)
>>> +			return false;
>>> +	}
>>
>> Since we're here, would it be possible to go with
>> mem_is_zero()/memchr_inv() which contains some extra optimization.
>>
>> And if we go calling mem_is_zero()/memchr_inv(), can we change this to
>> an inline?
>>
>> Otherwise looks good to me.
>
> The generic uuid code also has a uuid_is_null():
>
> bool __pure btrfs_is_empty_uuid(const u8 *uuid)
> {
> 	return uuid_is_null((const uuid_t *)uuid));
> }
>
> should work as well, but I've not tested it just eyeballed.

Wow, uuid_is_null() is better, because it goes easier to understand
memcmp(), which also has extra comparison optimization.

Thanks,
Qu

Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[Filipe Manana]

On Tue, Dec 17, 2024 at 12:26 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2024/12/17 03:47, fdmanana@kernel.org 写道:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > The ctree module is about the implementation of the btree data structure
> > and not a place holder for generic filesystem things like the csum
> > algorithm details. Move the functions related to the csum algorithm
> > details away from ctree.c and into fs.c, which is a far better place for
> > them. Also fix missing punctuation in comments and change one multiline
> > comment to a single line comment since everything fits in under 80
> > characters.
> >
> > For some reason this also sligthly reduces the module's size.
> >
> > Before this change:
> >
> >    $ size fs/btrfs/btrfs.ko
> >       text       data     bss     dec     hex filename
> >    1782126     161045   16920 1960091  1de89b fs/btrfs/btrfs.ko
> >
> > After this change:
> >
> >    $ size fs/btrfs/btrfs.ko
> >       text       data     bss     dec     hex filename
> >    1782094     161045   16920 1960059  1de87b fs/btrfs/btrfs.ko
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >   fs/btrfs/ctree.c | 51 ------------------------------------------------
> >   fs/btrfs/ctree.h |  6 ------
> >   fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> >   fs/btrfs/fs.h    |  6 ++++++
> >   4 files changed, 55 insertions(+), 57 deletions(-)
> >
> > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> > index 99a58ede387e..c93f52a30a16 100644
> > --- a/fs/btrfs/ctree.c
> > +++ b/fs/btrfs/ctree.c
> > @@ -37,19 +37,6 @@ static int push_node_left(struct btrfs_trans_handle *trans,
> >   static int balance_node_right(struct btrfs_trans_handle *trans,
> >                             struct extent_buffer *dst_buf,
> >                             struct extent_buffer *src_buf);
> > -
> > -static const struct btrfs_csums {
> > -     u16             size;
> > -     const char      name[10];
> > -     const char      driver[12];
> > -} btrfs_csums[] = {
> > -     [BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
> > -     [BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
> > -     [BTRFS_CSUM_TYPE_SHA256] = { .size = 32, .name = "sha256" },
> > -     [BTRFS_CSUM_TYPE_BLAKE2] = { .size = 32, .name = "blake2b",
> > -                                  .driver = "blake2b-256" },
> > -};
> > -
> >   /*
> >    * The leaf data grows from end-to-front in the node.  this returns the address
> >    * of the start of the last item, which is the stop of the leaf data stack.
> > @@ -148,44 +135,6 @@ static inline void copy_leaf_items(const struct extent_buffer *dst,
> >                             nr_items * sizeof(struct btrfs_item));
> >   }
> >
> > -/* This exists for btrfs-progs usages. */
> > -u16 btrfs_csum_type_size(u16 type)
> > -{
> > -     return btrfs_csums[type].size;
> > -}
> > -
> > -int btrfs_super_csum_size(const struct btrfs_super_block *s)
> > -{
> > -     u16 t = btrfs_super_csum_type(s);
> > -     /*
> > -      * csum type is validated at mount time
> > -      */
> > -     return btrfs_csum_type_size(t);
> > -}
> > -
> > -const char *btrfs_super_csum_name(u16 csum_type)
> > -{
> > -     /* csum type is validated at mount time */
> > -     return btrfs_csums[csum_type].name;
> > -}
> > -
> > -/*
> > - * Return driver name if defined, otherwise the name that's also a valid driver
> > - * name
> > - */
> > -const char *btrfs_super_csum_driver(u16 csum_type)
> > -{
> > -     /* csum type is validated at mount time */
> > -     return btrfs_csums[csum_type].driver[0] ?
> > -             btrfs_csums[csum_type].driver :
> > -             btrfs_csums[csum_type].name;
> > -}
> > -
> > -size_t __attribute_const__ btrfs_get_num_csums(void)
> > -{
> > -     return ARRAY_SIZE(btrfs_csums);
> > -}
> > -
> >   struct btrfs_path *btrfs_alloc_path(void)
> >   {
> >       might_sleep();
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 2c341956a01c..a1bab0b3f193 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -756,12 +756,6 @@ static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
> >       return root->root_key.objectid == BTRFS_DATA_RELOC_TREE_OBJECTID;
> >   }
> >
> > -u16 btrfs_csum_type_size(u16 type);
> > -int btrfs_super_csum_size(const struct btrfs_super_block *s);
> > -const char *btrfs_super_csum_name(u16 csum_type);
> > -const char *btrfs_super_csum_driver(u16 csum_type);
> > -size_t __attribute_const__ btrfs_get_num_csums(void);
> > -
> >   /*
> >    * We use folio flag owner_2 to indicate there is an ordered extent with
> >    * unfinished IO.
> > diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
> > index 31c1648bc0b4..3756a3b9c9da 100644
> > --- a/fs/btrfs/fs.c
> > +++ b/fs/btrfs/fs.c
> > @@ -5,6 +5,55 @@
> >   #include "fs.h"
> >   #include "accessors.h"
> >
> > +static const struct btrfs_csums {
> > +     u16             size;
> > +     const char      name[10];
> > +     const char      driver[12];
> > +} btrfs_csums[] = {
> > +     [BTRFS_CSUM_TYPE_CRC32] = { .size = 4, .name = "crc32c" },
> > +     [BTRFS_CSUM_TYPE_XXHASH] = { .size = 8, .name = "xxhash64" },
> > +     [BTRFS_CSUM_TYPE_SHA256] = { .size = 32, .name = "sha256" },
> > +     [BTRFS_CSUM_TYPE_BLAKE2] = { .size = 32, .name = "blake2b",
> > +                                  .driver = "blake2b-256" },
> > +};
> > +
> > +/* This exists for btrfs-progs usages. */
> > +u16 btrfs_csum_type_size(u16 type)
> > +{
> > +     return btrfs_csums[type].size;
> > +}
> > +
> > +int btrfs_super_csum_size(const struct btrfs_super_block *s)
> > +{
> > +     u16 t = btrfs_super_csum_type(s);
> > +
> > +     /* csum type is validated at mount time. */
> > +     return btrfs_csum_type_size(t);
> > +}
> > +
> > +const char *btrfs_super_csum_name(u16 csum_type)
> > +{
> > +     /* csum type is validated at mount time. */
> > +     return btrfs_csums[csum_type].name;
> > +}
> > +
> > +/*
> > + * Return driver name if defined, otherwise the name that's also a valid driver
> > + * name.
> > + */
> > +const char *btrfs_super_csum_driver(u16 csum_type)
> > +{
> > +     /* csum type is validated at mount time */
> > +     return btrfs_csums[csum_type].driver[0] ?
> > +             btrfs_csums[csum_type].driver :
> > +             btrfs_csums[csum_type].name;
> > +}
> > +
> > +size_t __attribute_const__ btrfs_get_num_csums(void)
> > +{
> > +     return ARRAY_SIZE(btrfs_csums);
> > +}
> > +
>
> I'm wondering if those simple functions can be converted to inline.
>
> Although btrfs_csums[] array has to be put inside fs.c, those functions
> seems be fine defined as inline ones inside the header.

Yes, the array being inside fs.c makes it hard to inline in the header.

Keep in mind that this is a change just to move things around, the
goal is not to change the implementation or optimize anything.
Plus it's not like these functions are called in hot code paths where
saving a function call has any significant impact.

Thanks.

>
> Otherwise looks good to me.
>
> Thanks,
> Qu
> >   void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
> >                            const char *name)
> >   {
> > diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> > index 79a1a3d6f04d..b05f2af97140 100644
> > --- a/fs/btrfs/fs.h
> > +++ b/fs/btrfs/fs.h
> > @@ -982,6 +982,12 @@ void btrfs_exclop_balance(struct btrfs_fs_info *fs_info,
> >
> >   int btrfs_check_ioctl_vol_args_path(const struct btrfs_ioctl_vol_args *vol_args);
> >
> > +u16 btrfs_csum_type_size(u16 type);
> > +int btrfs_super_csum_size(const struct btrfs_super_block *s);
> > +const char *btrfs_super_csum_name(u16 csum_type);
> > +const char *btrfs_super_csum_driver(u16 csum_type);
> > +size_t __attribute_const__ btrfs_get_num_csums(void);
> > +
> >   /* Compatibility and incompatibility defines */
> >   void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
> >                            const char *name);
>

Re: [PATCH 4/9] btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c

[Filipe Manana]

On Tue, Dec 17, 2024 at 12:31 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2024/12/17 03:47, fdmanana@kernel.org 写道:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > It's a generic helper not specific to ioctls and used in several places,
> > so move it out from ioctl.c and into fs.c. While at it change its return
> > type from int to bool and declare the loop variable in the loop itself.
> >
> > This also slightly reduces the module's size.
> >
> > Before this change:
> >
> >    $ size fs/btrfs/btrfs.ko
> >       text       data     bss     dec     hex filename
> >    1781492     161037   16920 1959449  1de619 fs/btrfs/btrfs.ko
> >
> > After this change:
> >
> >    $ size fs/btrfs/btrfs.ko
> >       text       data     bss     dec     hex filename
> >    1781340     161037   16920 1959297  1de581 fs/btrfs/btrfs.ko
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >   fs/btrfs/fs.c    |  9 +++++++++
> >   fs/btrfs/fs.h    |  2 ++
> >   fs/btrfs/ioctl.c | 11 -----------
> >   fs/btrfs/ioctl.h |  1 -
> >   4 files changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
> > index 09cfb43580cb..06a863252a85 100644
> > --- a/fs/btrfs/fs.c
> > +++ b/fs/btrfs/fs.c
> > @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void)
> >       return ARRAY_SIZE(btrfs_csums);
> >   }
> >
> > +bool __pure btrfs_is_empty_uuid(const u8 *uuid)
> > +{
> > +     for (int i = 0; i < BTRFS_UUID_SIZE; i++) {
> > +             if (uuid[i] != 0)
> > +                     return false;
> > +     }
>
> Since we're here, would it be possible to go with
> mem_is_zero()/memchr_inv() which contains some extra optimization.
>
> And if we go calling mem_is_zero()/memchr_inv(), can we change this to
> an inline?

I'll send that as a separate change, using uuid_is_null() as Johannes
suggested, on top of this.
The goal here was just to move code around and not change the
implementation while doing it.

Thanks.

>
> Otherwise looks good to me.
>
> Thanks,
> Qu
> > +     return true;
> > +}
> > +
> >   /*
> >    * Start exclusive operation @type, return true on success.
> >    */
> > diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> > index b05f2af97140..15c26c6f4d6e 100644
> > --- a/fs/btrfs/fs.h
> > +++ b/fs/btrfs/fs.h
> > @@ -988,6 +988,8 @@ const char *btrfs_super_csum_name(u16 csum_type);
> >   const char *btrfs_super_csum_driver(u16 csum_type);
> >   size_t __attribute_const__ btrfs_get_num_csums(void);
> >
> > +bool __pure btrfs_is_empty_uuid(const u8 *uuid);
> > +
> >   /* Compatibility and incompatibility defines */
> >   void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
> >                            const char *name);
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 0ede6a5524c2..7872de140489 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -471,17 +471,6 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info,
> >       return ret;
> >   }
> >
> > -int __pure btrfs_is_empty_uuid(const u8 *uuid)
> > -{
> > -     int i;
> > -
> > -     for (i = 0; i < BTRFS_UUID_SIZE; i++) {
> > -             if (uuid[i])
> > -                     return 0;
> > -     }
> > -     return 1;
> > -}
> > -
> >   /*
> >    * Calculate the number of transaction items to reserve for creating a subvolume
> >    * or snapshot, not including the inode, directory entries, or parent directory.
> > diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> > index 2b760c8778f8..ce915fcda43b 100644
> > --- a/fs/btrfs/ioctl.h
> > +++ b/fs/btrfs/ioctl.h
> > @@ -19,7 +19,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
> >                      struct dentry *dentry, struct fileattr *fa);
> >   int btrfs_ioctl_get_supported_features(void __user *arg);
> >   void btrfs_sync_inode_flags_to_i_flags(struct inode *inode);
> > -int __pure btrfs_is_empty_uuid(const u8 *uuid);
> >   void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
> >                                    struct btrfs_ioctl_balance_args *bargs);
> >   int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags);
>

Re: [PATCH 4/9] btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c

[Qu Wenruo]

在 2024/12/17 19:27, Filipe Manana 写道:
> On Tue, Dec 17, 2024 at 12:31 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> 在 2024/12/17 03:47, fdmanana@kernel.org 写道:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> It's a generic helper not specific to ioctls and used in several places,
>>> so move it out from ioctl.c and into fs.c. While at it change its return
>>> type from int to bool and declare the loop variable in the loop itself.
>>>
>>> This also slightly reduces the module's size.
>>>
>>> Before this change:
>>>
>>>     $ size fs/btrfs/btrfs.ko
>>>        text       data     bss     dec     hex filename
>>>     1781492     161037   16920 1959449  1de619 fs/btrfs/btrfs.ko
>>>
>>> After this change:
>>>
>>>     $ size fs/btrfs/btrfs.ko
>>>        text       data     bss     dec     hex filename
>>>     1781340     161037   16920 1959297  1de581 fs/btrfs/btrfs.ko
>>>
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>    fs/btrfs/fs.c    |  9 +++++++++
>>>    fs/btrfs/fs.h    |  2 ++
>>>    fs/btrfs/ioctl.c | 11 -----------
>>>    fs/btrfs/ioctl.h |  1 -
>>>    4 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c
>>> index 09cfb43580cb..06a863252a85 100644
>>> --- a/fs/btrfs/fs.c
>>> +++ b/fs/btrfs/fs.c
>>> @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void)
>>>        return ARRAY_SIZE(btrfs_csums);
>>>    }
>>>
>>> +bool __pure btrfs_is_empty_uuid(const u8 *uuid)
>>> +{
>>> +     for (int i = 0; i < BTRFS_UUID_SIZE; i++) {
>>> +             if (uuid[i] != 0)
>>> +                     return false;
>>> +     }
>>
>> Since we're here, would it be possible to go with
>> mem_is_zero()/memchr_inv() which contains some extra optimization.
>>
>> And if we go calling mem_is_zero()/memchr_inv(), can we change this to
>> an inline?
> 
> I'll send that as a separate change, using uuid_is_null() as Johannes
> suggested, on top of this.
> The goal here was just to move code around and not change the
> implementation while doing it.

Got it, feel free to add the reviewed-by tags to the whole series.

Thanks,
Qu

> 
> Thanks.
> 
>>
>> Otherwise looks good to me.
>>
>> Thanks,
>> Qu
>>> +     return true;
>>> +}
>>> +
>>>    /*
>>>     * Start exclusive operation @type, return true on success.
>>>     */
>>> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
>>> index b05f2af97140..15c26c6f4d6e 100644
>>> --- a/fs/btrfs/fs.h
>>> +++ b/fs/btrfs/fs.h
>>> @@ -988,6 +988,8 @@ const char *btrfs_super_csum_name(u16 csum_type);
>>>    const char *btrfs_super_csum_driver(u16 csum_type);
>>>    size_t __attribute_const__ btrfs_get_num_csums(void);
>>>
>>> +bool __pure btrfs_is_empty_uuid(const u8 *uuid);
>>> +
>>>    /* Compatibility and incompatibility defines */
>>>    void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag,
>>>                             const char *name);
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index 0ede6a5524c2..7872de140489 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -471,17 +471,6 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info,
>>>        return ret;
>>>    }
>>>
>>> -int __pure btrfs_is_empty_uuid(const u8 *uuid)
>>> -{
>>> -     int i;
>>> -
>>> -     for (i = 0; i < BTRFS_UUID_SIZE; i++) {
>>> -             if (uuid[i])
>>> -                     return 0;
>>> -     }
>>> -     return 1;
>>> -}
>>> -
>>>    /*
>>>     * Calculate the number of transaction items to reserve for creating a subvolume
>>>     * or snapshot, not including the inode, directory entries, or parent directory.
>>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>>> index 2b760c8778f8..ce915fcda43b 100644
>>> --- a/fs/btrfs/ioctl.h
>>> +++ b/fs/btrfs/ioctl.h
>>> @@ -19,7 +19,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
>>>                       struct dentry *dentry, struct fileattr *fa);
>>>    int btrfs_ioctl_get_supported_features(void __user *arg);
>>>    void btrfs_sync_inode_flags_to_i_flags(struct inode *inode);
>>> -int __pure btrfs_is_empty_uuid(const u8 *uuid);
>>>    void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
>>>                                     struct btrfs_ioctl_balance_args *bargs);
>>>    int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags);
>>
> 

Re: [PATCH 0/9] btrfs: some header cleanups and move things around

[Johannes Thumshirn]

On 16.12.24 18:18, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Move some misplaced prototypes, macros and functions around and some
> header cleanups. Trivial changes, details in the change logs.
> 
> Filipe Manana (9):
>    btrfs: move abort_should_print_stack() to transaction.h
>    btrfs: move csum related functions from ctree.c into fs.c
>    btrfs: move the exclusive operation functions into fs.c
>    btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c
>    btrfs: move the folio ordered helpers from ctree.h into fs.h
>    btrfs: move BTRFS_BYTES_TO_BLKS() into fs.h
>    btrfs: move btrfs_alloc_write_mask() into fs.h
>    btrfs: move extent-tree function declarations out of ctree.h
>    btrfs: remove pointless comment from ctree.h
> 
>   fs/btrfs/ctree.c            |  67 -----------------
>   fs/btrfs/ctree.h            |  29 --------
>   fs/btrfs/extent-tree.h      |   4 ++
>   fs/btrfs/free-space-cache.c |   2 +-
>   fs/btrfs/fs.c               | 139 ++++++++++++++++++++++++++++++++++++
>   fs/btrfs/fs.h               |  24 +++++++
>   fs/btrfs/ioctl.c            |  91 -----------------------
>   fs/btrfs/ioctl.h            |   1 -
>   fs/btrfs/transaction.h      |  18 ++++-
>   fs/btrfs/volumes.c          |   2 +-
>   10 files changed, 185 insertions(+), 192 deletions(-)
> 

For the series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[David Sterba]

On Tue, Dec 17, 2024 at 08:55:21AM +0000, Filipe Manana wrote:
> > I'm wondering if those simple functions can be converted to inline.
> >
> > Although btrfs_csums[] array has to be put inside fs.c, those functions
> > seems be fine defined as inline ones inside the header.
> 
> Yes, the array being inside fs.c makes it hard to inline in the header.
> 
> Keep in mind that this is a change just to move things around, the
> goal is not to change the implementation or optimize anything.
> Plus it's not like these functions are called in hot code paths where
> saving a function call has any significant impact.

Yeah the checksum related functions are not on any hot path, usually
called once or twice in a function. Exporting the btrfs_csum array to
make a few functions inilne should have some measurable impact.

Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[David Sterba]

On Mon, Dec 16, 2024 at 05:17:17PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The ctree module is about the implementation of the btree data structure
> and not a place holder for generic filesystem things like the csum
> algorithm details. Move the functions related to the csum algorithm
> details away from ctree.c and into fs.c, which is a far better place for
> them. Also fix missing punctuation in comments and change one multiline
> comment to a single line comment since everything fits in under 80
> characters.
> 
> For some reason this also sligthly reduces the module's size.
> 
> Before this change:
> 
>   $ size fs/btrfs/btrfs.ko
>      text	   data	    bss	    dec	    hex	filename
>   1782126	 161045	  16920	1960091	 1de89b	fs/btrfs/btrfs.ko
> 
> After this change:
> 
>   $ size fs/btrfs/btrfs.ko
>      text	   data	    bss	    dec	    hex	filename
>   1782094	 161045	  16920	1960059	 1de87b	fs/btrfs/btrfs.ko
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/ctree.c | 51 ------------------------------------------------
>  fs/btrfs/ctree.h |  6 ------
>  fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/fs.h    |  6 ++++++

Can you please create a new file for checksums? Moving everything to
fs.c looks like we're going to have another ctree.c.

Re: [PATCH 0/9] btrfs: some header cleanups and move things around

[David Sterba]

On Mon, Dec 16, 2024 at 05:17:15PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Move some misplaced prototypes, macros and functions around and some
> header cleanups. Trivial changes, details in the change logs.
> 
> Filipe Manana (9):
>   btrfs: move abort_should_print_stack() to transaction.h
>   btrfs: move csum related functions from ctree.c into fs.c
>   btrfs: move the exclusive operation functions into fs.c
>   btrfs: move btrfs_is_empty_uuid() from ioctl.c into fs.c
>   btrfs: move the folio ordered helpers from ctree.h into fs.h
>   btrfs: move BTRFS_BYTES_TO_BLKS() into fs.h
>   btrfs: move btrfs_alloc_write_mask() into fs.h
>   btrfs: move extent-tree function declarations out of ctree.h
>   btrfs: remove pointless comment from ctree.h

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

With the comment to consider creating a file for checksum helpers
(checksum.c/.h).

Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[Filipe Manana]

On Wed, Dec 18, 2024 at 8:21 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Mon, Dec 16, 2024 at 05:17:17PM +0000, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > The ctree module is about the implementation of the btree data structure
> > and not a place holder for generic filesystem things like the csum
> > algorithm details. Move the functions related to the csum algorithm
> > details away from ctree.c and into fs.c, which is a far better place for
> > them. Also fix missing punctuation in comments and change one multiline
> > comment to a single line comment since everything fits in under 80
> > characters.
> >
> > For some reason this also sligthly reduces the module's size.
> >
> > Before this change:
> >
> >   $ size fs/btrfs/btrfs.ko
> >      text        data     bss     dec     hex filename
> >   1782126      161045   16920 1960091  1de89b fs/btrfs/btrfs.ko
> >
> > After this change:
> >
> >   $ size fs/btrfs/btrfs.ko
> >      text        data     bss     dec     hex filename
> >   1782094      161045   16920 1960059  1de87b fs/btrfs/btrfs.ko
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >  fs/btrfs/ctree.c | 51 ------------------------------------------------
> >  fs/btrfs/ctree.h |  6 ------
> >  fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/btrfs/fs.h    |  6 ++++++
>
> Can you please create a new file for checksums? Moving everything to
> fs.c looks like we're going to have another ctree.c.

Is it really worth it? After this patchset fs.c is only 229 lines and
the csum related functions are just a few and very short.
My idea would be to do such a thing either when fs.c gets a lot larger
or we get more csum functions (and/or they get larger).

But sure, why not, I can do that on top or send a new version of this patch.

Thanks.

Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[Qu Wenruo]

在 2024/12/19 07:15, Filipe Manana 写道:
> On Wed, Dec 18, 2024 at 8:21 PM David Sterba <dsterba@suse.cz> wrote:
>>
>> On Mon, Dec 16, 2024 at 05:17:17PM +0000, fdmanana@kernel.org wrote:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> The ctree module is about the implementation of the btree data structure
>>> and not a place holder for generic filesystem things like the csum
>>> algorithm details. Move the functions related to the csum algorithm
>>> details away from ctree.c and into fs.c, which is a far better place for
>>> them. Also fix missing punctuation in comments and change one multiline
>>> comment to a single line comment since everything fits in under 80
>>> characters.
>>>
>>> For some reason this also sligthly reduces the module's size.
>>>
>>> Before this change:
>>>
>>>    $ size fs/btrfs/btrfs.ko
>>>       text        data     bss     dec     hex filename
>>>    1782126      161045   16920 1960091  1de89b fs/btrfs/btrfs.ko
>>>
>>> After this change:
>>>
>>>    $ size fs/btrfs/btrfs.ko
>>>       text        data     bss     dec     hex filename
>>>    1782094      161045   16920 1960059  1de87b fs/btrfs/btrfs.ko
>>>
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>   fs/btrfs/ctree.c | 51 ------------------------------------------------
>>>   fs/btrfs/ctree.h |  6 ------
>>>   fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   fs/btrfs/fs.h    |  6 ++++++
>>
>> Can you please create a new file for checksums? Moving everything to
>> fs.c looks like we're going to have another ctree.c.
> 
> Is it really worth it? After this patchset fs.c is only 229 lines and
> the csum related functions are just a few and very short.
> My idea would be to do such a thing either when fs.c gets a lot larger
> or we get more csum functions (and/or they get larger).
> 
> But sure, why not, I can do that on top or send a new version of this patch.

Personally speaking, I'm not a huge fan of a lot of small files/headers.

It makes the include path less clear, and make the path completion 
easier to hit conflicts.
(That's also why I hate the "mode-" prefix in btrfs-progs check/ directory)

The current fs.[ch] is definitely acceptable, thus I'm pretty happy with 
the current move.

Thanks,
Qu

> 
> Thanks.
> 

Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[David Sterba]

On Wed, Dec 18, 2024 at 08:45:33PM +0000, Filipe Manana wrote:
> On Wed, Dec 18, 2024 at 8:21 PM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Mon, Dec 16, 2024 at 05:17:17PM +0000, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > The ctree module is about the implementation of the btree data structure
> > > and not a place holder for generic filesystem things like the csum
> > > algorithm details. Move the functions related to the csum algorithm
> > > details away from ctree.c and into fs.c, which is a far better place for
> > > them. Also fix missing punctuation in comments and change one multiline
> > > comment to a single line comment since everything fits in under 80
> > > characters.
> > >
> > > For some reason this also sligthly reduces the module's size.
> > >
> > > Before this change:
> > >
> > >   $ size fs/btrfs/btrfs.ko
> > >      text        data     bss     dec     hex filename
> > >   1782126      161045   16920 1960091  1de89b fs/btrfs/btrfs.ko
> > >
> > > After this change:
> > >
> > >   $ size fs/btrfs/btrfs.ko
> > >      text        data     bss     dec     hex filename
> > >   1782094      161045   16920 1960059  1de87b fs/btrfs/btrfs.ko
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > >  fs/btrfs/ctree.c | 51 ------------------------------------------------
> > >  fs/btrfs/ctree.h |  6 ------
> > >  fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  fs/btrfs/fs.h    |  6 ++++++
> >
> > Can you please create a new file for checksums? Moving everything to
> > fs.c looks like we're going to have another ctree.c.
> 
> Is it really worth it? After this patchset fs.c is only 229 lines and
> the csum related functions are just a few and very short.
> My idea would be to do such a thing either when fs.c gets a lot larger
> or we get more csum functions (and/or they get larger).
> 
> But sure, why not, I can do that on top or send a new version of this patch.

Ok leave it as is, if we have more code we can move it. I have some WIP
with checksums so I can move it once it makes more sense.

Re: [PATCH 2/9] btrfs: move csum related functions from ctree.c into fs.c

[David Sterba]

On Thu, Dec 19, 2024 at 07:58:16AM +1030, Qu Wenruo wrote:
> >>>       text        data     bss     dec     hex filename
> >>>    1782094      161045   16920 1960059  1de87b fs/btrfs/btrfs.ko
> >>>
> >>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> >>> ---
> >>>   fs/btrfs/ctree.c | 51 ------------------------------------------------
> >>>   fs/btrfs/ctree.h |  6 ------
> >>>   fs/btrfs/fs.c    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>   fs/btrfs/fs.h    |  6 ++++++
> >>
> >> Can you please create a new file for checksums? Moving everything to
> >> fs.c looks like we're going to have another ctree.c.
> > 
> > Is it really worth it? After this patchset fs.c is only 229 lines and
> > the csum related functions are just a few and very short.
> > My idea would be to do such a thing either when fs.c gets a lot larger
> > or we get more csum functions (and/or they get larger).
> > 
> > But sure, why not, I can do that on top or send a new version of this patch.
> 
> Personally speaking, I'm not a huge fan of a lot of small files/headers.

Agreed if the files are too small, there's some percieved limit where it
makes sense to split the functionality. I've suggested that because
there are more changes for that so we could avoid moving the code twice.
For now I'm ok keeping it in fs.c.

> It makes the include path less clear, and make the path completion 
> easier to hit conflicts.
> (That's also why I hate the "mode-" prefix in btrfs-progs check/ directory)

Well that's a matter of taste and style. The path completion is not the
criteria I'd put first, file name namespacing may help to navigate by
the common functionality or a subsystem. But we've never agreed on that.