[PATCH 00/10] Cleanups and struct packing

[PATCH 00/10] Cleanups and struct packing

[David Sterba]

A few cleanups in comments and size reduction of several structures.

- btrfs_inode	1112 -> 1104
- btrfs_ref	  64 ->   56
- extent_buffer	 240 ->  232
- prelim_ref	  88 ->   80

David Sterba (10):
  btrfs: move functions comments from qgroup.h to qgroup.c
  btrfs: reformat remaining kdoc style comments
  btrfs: drop __must_check annotations
  btrfs: reduce parameters of btrfs_pin_reserved_extent
  btrfs: reduce parameters of btrfs_pin_extent_for_log_replay
  btrfs: reduce arguments of helpers space accounting root item
  btrfs: reduce size of prelim_ref::level
  btrfs: reduce size and reorder compression members in struct
    btrfs_inode
  btrfs: reduce size of struct btrfs_ref
  btrfs: move extent_buffer::lock_owner to debug section

 fs/btrfs/backref.h       |  2 +-
 fs/btrfs/btrfs_inode.h   | 20 +++++------
 fs/btrfs/ctree.c         | 27 +++++++-------
 fs/btrfs/delayed-inode.c |  6 ++--
 fs/btrfs/delayed-ref.c   |  3 +-
 fs/btrfs/delayed-ref.h   | 18 ++++++----
 fs/btrfs/disk-io.c       | 11 +++---
 fs/btrfs/extent-tree.c   | 56 ++++++++++++++++++-----------
 fs/btrfs/extent-tree.h   |  7 ++--
 fs/btrfs/extent_io.c     | 22 ++++++------
 fs/btrfs/extent_io.h     |  4 +--
 fs/btrfs/inode-item.c    |  2 +-
 fs/btrfs/inode.c         |  9 ++---
 fs/btrfs/locking.c       | 18 +++++++---
 fs/btrfs/messages.c      |  8 ++---
 fs/btrfs/qgroup.c        | 71 +++++++++++++++++++++++++++++++++++++
 fs/btrfs/qgroup.h        | 76 ----------------------------------------
 fs/btrfs/ref-verify.c    |  2 +-
 fs/btrfs/relocation.c    |  2 +-
 fs/btrfs/root-tree.c     |  6 ++--
 fs/btrfs/root-tree.h     |  6 ++--
 fs/btrfs/space-info.c    |  3 +-
 fs/btrfs/transaction.c   |  4 +--
 fs/btrfs/tree-log.c      | 16 ++++-----
 fs/btrfs/ulist.c         |  3 +-
 fs/btrfs/volumes.c       |  3 +-
 fs/btrfs/zstd.c          | 11 +++---
 27 files changed, 223 insertions(+), 193 deletions(-)

-- 
2.41.0

[PATCH 01/10] btrfs: move functions comments from qgroup.h to qgroup.c

[David Sterba]

We keep the comments next to the implementation, there were some left
to move.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/qgroup.c | 71 +++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/qgroup.h | 76 -----------------------------------------------
 2 files changed, 71 insertions(+), 76 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 127b91290e9a..a51f1ceb867a 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1793,6 +1793,17 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid,
 	return ret;
 }
 
+/*
+ * Inform qgroup to trace one dirty extent, its info is recorded in @record.
+ * So qgroup can account it at transaction committing time.
+ *
+ * No lock version, caller must acquire delayed ref lock and allocated memory,
+ * then call btrfs_qgroup_trace_extent_post() after exiting lock context.
+ *
+ * Return 0 for success insert
+ * Return >0 for existing record, caller can free @record safely.
+ * Error is not possible
+ */
 int btrfs_qgroup_trace_extent_nolock(struct btrfs_fs_info *fs_info,
 				struct btrfs_delayed_ref_root *delayed_refs,
 				struct btrfs_qgroup_extent_record *record)
@@ -1828,6 +1839,27 @@ int btrfs_qgroup_trace_extent_nolock(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
+/*
+ * Post handler after qgroup_trace_extent_nolock().
+ *
+ * NOTE: Current qgroup does the expensive backref walk at transaction
+ * committing time with TRANS_STATE_COMMIT_DOING, this blocks incoming
+ * new transaction.
+ * This is designed to allow btrfs_find_all_roots() to get correct new_roots
+ * result.
+ *
+ * However for old_roots there is no need to do backref walk at that time,
+ * since we search commit roots to walk backref and result will always be
+ * correct.
+ *
+ * Due to the nature of no lock version, we can't do backref there.
+ * So we must call btrfs_qgroup_trace_extent_post() after exiting
+ * spinlock context.
+ *
+ * TODO: If we can fix and prove btrfs_find_all_roots() can get correct result
+ * using current root, then we can move all expensive backref walk out of
+ * transaction committing, but not now as qgroup accounting will be wrong again.
+ */
 int btrfs_qgroup_trace_extent_post(struct btrfs_trans_handle *trans,
 				   struct btrfs_qgroup_extent_record *qrecord)
 {
@@ -1881,6 +1913,19 @@ int btrfs_qgroup_trace_extent_post(struct btrfs_trans_handle *trans,
 	return 0;
 }
 
+/*
+ * Inform qgroup to trace one dirty extent, specified by @bytenr and
+ * @num_bytes.
+ * So qgroup can account it at commit trans time.
+ *
+ * Better encapsulated version, with memory allocation and backref walk for
+ * commit roots.
+ * So this can sleep.
+ *
+ * Return 0 if the operation is done.
+ * Return <0 for error, like memory allocation failure or invalid parameter
+ * (NULL trans)
+ */
 int btrfs_qgroup_trace_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 			      u64 num_bytes)
 {
@@ -1911,6 +1956,12 @@ int btrfs_qgroup_trace_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 	return btrfs_qgroup_trace_extent_post(trans, record);
 }
 
+/*
+ * Inform qgroup to trace all leaf items of data
+ *
+ * Return 0 for success
+ * Return <0 for error(ENOMEM)
+ */
 int btrfs_qgroup_trace_leaf_items(struct btrfs_trans_handle *trans,
 				  struct extent_buffer *eb)
 {
@@ -2341,6 +2392,16 @@ static int qgroup_trace_subtree_swap(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+/*
+ * Inform qgroup to trace a whole subtree, including all its child tree
+ * blocks and data.
+ * The root tree block is specified by @root_eb.
+ *
+ * Normally used by relocation(tree block swap) and subvolume deletion.
+ *
+ * Return 0 for success
+ * Return <0 for error(ENOMEM or tree search error)
+ */
 int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
 			       struct extent_buffer *root_eb,
 			       u64 root_gen, int root_level)
@@ -4050,6 +4111,10 @@ int __btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes,
 	return btrfs_qgroup_reserve_meta(root, num_bytes, type, enforce);
 }
 
+/*
+ * Per-transaction meta reservation should be all freed at transaction commit
+ * time
+ */
 void btrfs_qgroup_free_meta_all_pertrans(struct btrfs_root *root)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
@@ -4119,6 +4184,12 @@ static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
 	spin_unlock(&fs_info->qgroup_lock);
 }
 
+/*
+ * Convert @num_bytes of META_PREALLOCATED reservation to META_PERTRANS.
+ *
+ * This is called when preallocated meta reservation needs to be used.
+ * Normally after btrfs_join_transaction() call.
+ */
 void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index b4416d5be47d..12614bc1e70b 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -294,80 +294,16 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info);
 void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info);
 struct btrfs_delayed_extent_op;
 
-/*
- * Inform qgroup to trace one dirty extent, its info is recorded in @record.
- * So qgroup can account it at transaction committing time.
- *
- * No lock version, caller must acquire delayed ref lock and allocated memory,
- * then call btrfs_qgroup_trace_extent_post() after exiting lock context.
- *
- * Return 0 for success insert
- * Return >0 for existing record, caller can free @record safely.
- * Error is not possible
- */
 int btrfs_qgroup_trace_extent_nolock(
 		struct btrfs_fs_info *fs_info,
 		struct btrfs_delayed_ref_root *delayed_refs,
 		struct btrfs_qgroup_extent_record *record);
-
-/*
- * Post handler after qgroup_trace_extent_nolock().
- *
- * NOTE: Current qgroup does the expensive backref walk at transaction
- * committing time with TRANS_STATE_COMMIT_DOING, this blocks incoming
- * new transaction.
- * This is designed to allow btrfs_find_all_roots() to get correct new_roots
- * result.
- *
- * However for old_roots there is no need to do backref walk at that time,
- * since we search commit roots to walk backref and result will always be
- * correct.
- *
- * Due to the nature of no lock version, we can't do backref there.
- * So we must call btrfs_qgroup_trace_extent_post() after exiting
- * spinlock context.
- *
- * TODO: If we can fix and prove btrfs_find_all_roots() can get correct result
- * using current root, then we can move all expensive backref walk out of
- * transaction committing, but not now as qgroup accounting will be wrong again.
- */
 int btrfs_qgroup_trace_extent_post(struct btrfs_trans_handle *trans,
 				   struct btrfs_qgroup_extent_record *qrecord);
-
-/*
- * Inform qgroup to trace one dirty extent, specified by @bytenr and
- * @num_bytes.
- * So qgroup can account it at commit trans time.
- *
- * Better encapsulated version, with memory allocation and backref walk for
- * commit roots.
- * So this can sleep.
- *
- * Return 0 if the operation is done.
- * Return <0 for error, like memory allocation failure or invalid parameter
- * (NULL trans)
- */
 int btrfs_qgroup_trace_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 			      u64 num_bytes);
-
-/*
- * Inform qgroup to trace all leaf items of data
- *
- * Return 0 for success
- * Return <0 for error(ENOMEM)
- */
 int btrfs_qgroup_trace_leaf_items(struct btrfs_trans_handle *trans,
 				  struct extent_buffer *eb);
-/*
- * Inform qgroup to trace a whole subtree, including all its child tree
- * blocks and data.
- * The root tree block is specified by @root_eb.
- *
- * Normally used by relocation(tree block swap) and subvolume deletion.
- *
- * Return 0 for success
- * Return <0 for error(ENOMEM or tree search error)
- */
 int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
 			       struct extent_buffer *root_eb,
 			       u64 root_gen, int root_level);
@@ -435,20 +371,8 @@ static inline void btrfs_qgroup_free_meta_prealloc(struct btrfs_root *root,
 			BTRFS_QGROUP_RSV_META_PREALLOC);
 }
 
-/*
- * Per-transaction meta reservation should be all freed at transaction commit
- * time
- */
 void btrfs_qgroup_free_meta_all_pertrans(struct btrfs_root *root);
-
-/*
- * Convert @num_bytes of META_PREALLOCATED reservation to META_PERTRANS.
- *
- * This is called when preallocated meta reservation needs to be used.
- * Normally after btrfs_join_transaction() call.
- */
 void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes);
-
 void btrfs_qgroup_check_reserved_leak(struct btrfs_inode *inode);
 
 /* btrfs_qgroup_swapped_blocks related functions */
-- 
2.41.0

Re: [PATCH 01/10] btrfs: move functions comments from qgroup.h to qgroup.c

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> We keep the comments next to the implementation, there were some left
> to move.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/qgroup.c | 71 +++++++++++++++++++++++++++++++++++++++++++
>   fs/btrfs/qgroup.h | 76 -----------------------------------------------
>   2 files changed, 71 insertions(+), 76 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 127b91290e9a..a51f1ceb867a 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1793,6 +1793,17 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid,
>   	return ret;
>   }
>
> +/*
> + * Inform qgroup to trace one dirty extent, its info is recorded in @record.
> + * So qgroup can account it at transaction committing time.
> + *
> + * No lock version, caller must acquire delayed ref lock and allocated memory,
> + * then call btrfs_qgroup_trace_extent_post() after exiting lock context.
> + *
> + * Return 0 for success insert
> + * Return >0 for existing record, caller can free @record safely.
> + * Error is not possible
> + */
>   int btrfs_qgroup_trace_extent_nolock(struct btrfs_fs_info *fs_info,
>   				struct btrfs_delayed_ref_root *delayed_refs,
>   				struct btrfs_qgroup_extent_record *record)
> @@ -1828,6 +1839,27 @@ int btrfs_qgroup_trace_extent_nolock(struct btrfs_fs_info *fs_info,
>   	return 0;
>   }
>
> +/*
> + * Post handler after qgroup_trace_extent_nolock().
> + *
> + * NOTE: Current qgroup does the expensive backref walk at transaction
> + * committing time with TRANS_STATE_COMMIT_DOING, this blocks incoming
> + * new transaction.
> + * This is designed to allow btrfs_find_all_roots() to get correct new_roots
> + * result.
> + *
> + * However for old_roots there is no need to do backref walk at that time,
> + * since we search commit roots to walk backref and result will always be
> + * correct.
> + *
> + * Due to the nature of no lock version, we can't do backref there.
> + * So we must call btrfs_qgroup_trace_extent_post() after exiting
> + * spinlock context.
> + *
> + * TODO: If we can fix and prove btrfs_find_all_roots() can get correct result
> + * using current root, then we can move all expensive backref walk out of
> + * transaction committing, but not now as qgroup accounting will be wrong again.
> + */
>   int btrfs_qgroup_trace_extent_post(struct btrfs_trans_handle *trans,
>   				   struct btrfs_qgroup_extent_record *qrecord)
>   {
> @@ -1881,6 +1913,19 @@ int btrfs_qgroup_trace_extent_post(struct btrfs_trans_handle *trans,
>   	return 0;
>   }
>
> +/*
> + * Inform qgroup to trace one dirty extent, specified by @bytenr and
> + * @num_bytes.
> + * So qgroup can account it at commit trans time.
> + *
> + * Better encapsulated version, with memory allocation and backref walk for
> + * commit roots.
> + * So this can sleep.
> + *
> + * Return 0 if the operation is done.
> + * Return <0 for error, like memory allocation failure or invalid parameter
> + * (NULL trans)
> + */
>   int btrfs_qgroup_trace_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>   			      u64 num_bytes)
>   {
> @@ -1911,6 +1956,12 @@ int btrfs_qgroup_trace_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>   	return btrfs_qgroup_trace_extent_post(trans, record);
>   }
>
> +/*
> + * Inform qgroup to trace all leaf items of data
> + *
> + * Return 0 for success
> + * Return <0 for error(ENOMEM)
> + */
>   int btrfs_qgroup_trace_leaf_items(struct btrfs_trans_handle *trans,
>   				  struct extent_buffer *eb)
>   {
> @@ -2341,6 +2392,16 @@ static int qgroup_trace_subtree_swap(struct btrfs_trans_handle *trans,
>   	return ret;
>   }
>
> +/*
> + * Inform qgroup to trace a whole subtree, including all its child tree
> + * blocks and data.
> + * The root tree block is specified by @root_eb.
> + *
> + * Normally used by relocation(tree block swap) and subvolume deletion.
> + *
> + * Return 0 for success
> + * Return <0 for error(ENOMEM or tree search error)
> + */
>   int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
>   			       struct extent_buffer *root_eb,
>   			       u64 root_gen, int root_level)
> @@ -4050,6 +4111,10 @@ int __btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes,
>   	return btrfs_qgroup_reserve_meta(root, num_bytes, type, enforce);
>   }
>
> +/*
> + * Per-transaction meta reservation should be all freed at transaction commit
> + * time
> + */
>   void btrfs_qgroup_free_meta_all_pertrans(struct btrfs_root *root)
>   {
>   	struct btrfs_fs_info *fs_info = root->fs_info;
> @@ -4119,6 +4184,12 @@ static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
>   	spin_unlock(&fs_info->qgroup_lock);
>   }
>
> +/*
> + * Convert @num_bytes of META_PREALLOCATED reservation to META_PERTRANS.
> + *
> + * This is called when preallocated meta reservation needs to be used.
> + * Normally after btrfs_join_transaction() call.
> + */
>   void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
>   {
>   	struct btrfs_fs_info *fs_info = root->fs_info;
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index b4416d5be47d..12614bc1e70b 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -294,80 +294,16 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info);
>   void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info);
>   struct btrfs_delayed_extent_op;
>
> -/*
> - * Inform qgroup to trace one dirty extent, its info is recorded in @record.
> - * So qgroup can account it at transaction committing time.
> - *
> - * No lock version, caller must acquire delayed ref lock and allocated memory,
> - * then call btrfs_qgroup_trace_extent_post() after exiting lock context.
> - *
> - * Return 0 for success insert
> - * Return >0 for existing record, caller can free @record safely.
> - * Error is not possible
> - */
>   int btrfs_qgroup_trace_extent_nolock(
>   		struct btrfs_fs_info *fs_info,
>   		struct btrfs_delayed_ref_root *delayed_refs,
>   		struct btrfs_qgroup_extent_record *record);
> -
> -/*
> - * Post handler after qgroup_trace_extent_nolock().
> - *
> - * NOTE: Current qgroup does the expensive backref walk at transaction
> - * committing time with TRANS_STATE_COMMIT_DOING, this blocks incoming
> - * new transaction.
> - * This is designed to allow btrfs_find_all_roots() to get correct new_roots
> - * result.
> - *
> - * However for old_roots there is no need to do backref walk at that time,
> - * since we search commit roots to walk backref and result will always be
> - * correct.
> - *
> - * Due to the nature of no lock version, we can't do backref there.
> - * So we must call btrfs_qgroup_trace_extent_post() after exiting
> - * spinlock context.
> - *
> - * TODO: If we can fix and prove btrfs_find_all_roots() can get correct result
> - * using current root, then we can move all expensive backref walk out of
> - * transaction committing, but not now as qgroup accounting will be wrong again.
> - */
>   int btrfs_qgroup_trace_extent_post(struct btrfs_trans_handle *trans,
>   				   struct btrfs_qgroup_extent_record *qrecord);
> -
> -/*
> - * Inform qgroup to trace one dirty extent, specified by @bytenr and
> - * @num_bytes.
> - * So qgroup can account it at commit trans time.
> - *
> - * Better encapsulated version, with memory allocation and backref walk for
> - * commit roots.
> - * So this can sleep.
> - *
> - * Return 0 if the operation is done.
> - * Return <0 for error, like memory allocation failure or invalid parameter
> - * (NULL trans)
> - */
>   int btrfs_qgroup_trace_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>   			      u64 num_bytes);
> -
> -/*
> - * Inform qgroup to trace all leaf items of data
> - *
> - * Return 0 for success
> - * Return <0 for error(ENOMEM)
> - */
>   int btrfs_qgroup_trace_leaf_items(struct btrfs_trans_handle *trans,
>   				  struct extent_buffer *eb);
> -/*
> - * Inform qgroup to trace a whole subtree, including all its child tree
> - * blocks and data.
> - * The root tree block is specified by @root_eb.
> - *
> - * Normally used by relocation(tree block swap) and subvolume deletion.
> - *
> - * Return 0 for success
> - * Return <0 for error(ENOMEM or tree search error)
> - */
>   int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
>   			       struct extent_buffer *root_eb,
>   			       u64 root_gen, int root_level);
> @@ -435,20 +371,8 @@ static inline void btrfs_qgroup_free_meta_prealloc(struct btrfs_root *root,
>   			BTRFS_QGROUP_RSV_META_PREALLOC);
>   }
>
> -/*
> - * Per-transaction meta reservation should be all freed at transaction commit
> - * time
> - */
>   void btrfs_qgroup_free_meta_all_pertrans(struct btrfs_root *root);
> -
> -/*
> - * Convert @num_bytes of META_PREALLOCATED reservation to META_PERTRANS.
> - *
> - * This is called when preallocated meta reservation needs to be used.
> - * Normally after btrfs_join_transaction() call.
> - */
>   void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes);
> -
>   void btrfs_qgroup_check_reserved_leak(struct btrfs_inode *inode);
>
>   /* btrfs_qgroup_swapped_blocks related functions */

[PATCH 02/10] btrfs: reformat remaining kdoc style comments

[David Sterba]

Function name in the comment does not bring much value to code not
exposed as API and we don't stick to the kdoc format anymore. Update
formatting of parameter descriptions.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c         |  4 ++--
 fs/btrfs/delayed-inode.c |  6 +++---
 fs/btrfs/delayed-ref.c   |  3 +--
 fs/btrfs/disk-io.c       | 11 ++++++-----
 fs/btrfs/extent-tree.c   |  6 +++---
 fs/btrfs/extent_io.c     | 22 ++++++++++++----------
 fs/btrfs/inode-item.c    |  2 +-
 fs/btrfs/inode.c         |  9 +++++----
 fs/btrfs/locking.c       |  3 ++-
 fs/btrfs/messages.c      |  8 ++++----
 fs/btrfs/ref-verify.c    |  2 +-
 fs/btrfs/root-tree.c     |  6 ++++--
 fs/btrfs/space-info.c    |  3 ++-
 fs/btrfs/transaction.c   |  4 ++--
 fs/btrfs/tree-log.c      |  7 +++----
 fs/btrfs/ulist.c         |  3 ++-
 fs/btrfs/volumes.c       |  3 ++-
 fs/btrfs/zstd.c          | 11 +++++++----
 18 files changed, 62 insertions(+), 51 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index a4cb4b642987..792f9e3afad8 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2054,8 +2054,8 @@ static int search_leaf(struct btrfs_trans_handle *trans,
 }
 
 /*
- * btrfs_search_slot - look for a key in a tree and perform necessary
- * modifications to preserve tree invariants.
+ * Look for a key in a tree and perform necessary modifications to preserve
+ * tree invariants.
  *
  * @trans:	Handle of transaction, used when modifying the tree
  * @p:		Holds all btree nodes along the search path
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index d7ac39a3dd10..3e4fd354d458 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -328,7 +328,8 @@ static struct btrfs_delayed_item *btrfs_alloc_delayed_item(u16 data_len,
 }
 
 /*
- * __btrfs_lookup_delayed_item - look up the delayed item by key
+ * Look up the delayed item by key.
+ *
  * @delayed_node: pointer to the delayed node
  * @index:	  the dir index value to lookup (offset of a dir index key)
  *
@@ -1760,8 +1761,7 @@ int btrfs_should_delete_dir_index(struct list_head *del_list,
 }
 
 /*
- * btrfs_readdir_delayed_dir_index - read dir info stored in the delayed tree
- *
+ * Read dir info stored in the delayed tree.
  */
 int btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
 				    struct list_head *ins_list)
diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 6a13cf00218b..c468148fc437 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -813,8 +813,7 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
 }
 
 /*
- * init_delayed_ref_common - Initialize the structure which represents a
- *			     modification to a an extent.
+ * Initialize the structure which represents a modification to a an extent.
  *
  * @fs_info:    Internal to the mounted filesystem mount structure.
  *
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 6404b17a5bdc..489589052f27 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1403,7 +1403,8 @@ struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info,
 }
 
 /*
- * btrfs_get_fs_root_commit_root - return a root for the given objectid
+ * Return a root for the given objectid.
+ *
  * @fs_info:	the fs_info
  * @objectid:	the objectid we need to lookup
  *
@@ -1700,11 +1701,11 @@ static void backup_super_roots(struct btrfs_fs_info *info)
 }
 
 /*
- * read_backup_root - Reads a backup root based on the passed priority. Prio 0
- * is the newest, prio 1/2/3 are 2nd newest/3rd newest/4th (oldest) backup roots
+ * Reads a backup root based on the passed priority. Prio 0 is the newest, prio
+ * 1/2/3 are 2nd newest/3rd newest/4th (oldest) backup roots
  *
- * fs_info - filesystem whose backup roots need to be read
- * priority - priority of backup root required
+ * @fs_info:  filesystem whose backup roots need to be read
+ * @priority: priority of backup root required
  *
  * Returns backup root index on success and -EINVAL otherwise.
  */
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index e4d337b78e76..5ef4e852ae2e 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1435,7 +1435,7 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
 }
 
 /*
- * __btrfs_inc_extent_ref - insert backreference for a given extent
+ * Insert backreference for a given extent.
  *
  * The counterpart is in __btrfs_free_extent(), with examples and more details
  * how it works.
@@ -4440,8 +4440,8 @@ static noinline int find_free_extent(struct btrfs_root *root,
 }
 
 /*
- * btrfs_reserve_extent - entry point to the extent allocator. Tries to find a
- *			  hole that is at least as big as @num_bytes.
+ * Entry point to the extent allocator. Tries to find a hole that is at least
+ * as big as @num_bytes.
  *
  * @root           -	The root that will contain this extent
  *
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index ac3fca5a5e41..8156dcc4b1fa 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4249,14 +4249,14 @@ void copy_extent_buffer(const struct extent_buffer *dst,
 }
 
 /*
- * eb_bitmap_offset() - calculate the page and offset of the byte containing the
- * given bit number
- * @eb: the extent buffer
- * @start: offset of the bitmap item in the extent buffer
- * @nr: bit number
- * @page_index: return index of the page in the extent buffer that contains the
- * given bit number
- * @page_offset: return offset into the page given by page_index
+ * Calculate the page and offset of the byte containing the given bit number.
+ *
+ * @eb:           the extent buffer
+ * @start:        offset of the bitmap item in the extent buffer
+ * @nr:           bit number
+ * @page_index:   return index of the page in the extent buffer that contains
+ *                the given bit number
+ * @page_offset:  return offset into the page given by page_index
  *
  * This helper hides the ugliness of finding the byte in an extent buffer which
  * contains a given bit.
@@ -4615,7 +4615,8 @@ int try_release_extent_buffer(struct page *page)
 }
 
 /*
- * btrfs_readahead_tree_block - attempt to readahead a child block
+ * Attempt to readahead a child block.
+ *
  * @fs_info:	the fs_info
  * @bytenr:	bytenr to read
  * @owner_root: objectid of the root that owns this eb
@@ -4654,7 +4655,8 @@ void btrfs_readahead_tree_block(struct btrfs_fs_info *fs_info,
 }
 
 /*
- * btrfs_readahead_node_child - readahead a node's child block
+ * Readahead a node's child block.
+ *
  * @node:	parent node we're reading from
  * @slot:	slot in the parent node for the child we want to read
  *
diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 4c322b720a80..c19c0f10f0e2 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -247,7 +247,7 @@ int btrfs_del_inode_ref(struct btrfs_trans_handle *trans,
 }
 
 /*
- * btrfs_insert_inode_extref() - Inserts an extended inode ref into a tree.
+ * Insert an extended inode ref into a tree.
  *
  * The caller must have checked against BTRFS_LINK_MAX already.
  */
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f09fbdc43f0f..032265d0946a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -348,7 +348,7 @@ static void __cold btrfs_print_data_csum_error(struct btrfs_inode *inode,
 }
 
 /*
- * btrfs_inode_lock - lock inode i_rwsem based on arguments passed
+ * Lock inode i_rwsem based on arguments passed.
  *
  * ilock_flags can have the following bit set:
  *
@@ -382,7 +382,7 @@ int btrfs_inode_lock(struct btrfs_inode *inode, unsigned int ilock_flags)
 }
 
 /*
- * btrfs_inode_unlock - unock inode i_rwsem
+ * Unock inode i_rwsem.
  *
  * ilock_flags should contain the same bits set as passed to btrfs_inode_lock()
  * to decide whether the lock acquired is shared or exclusive.
@@ -3310,7 +3310,7 @@ bool btrfs_data_csum_ok(struct btrfs_bio *bbio, struct btrfs_device *dev,
 }
 
 /*
- * btrfs_add_delayed_iput - perform a delayed iput on @inode
+ * Perform a delayed iput on @inode.
  *
  * @inode: The inode we want to perform iput on
  *
@@ -4645,7 +4645,8 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
 }
 
 /*
- * btrfs_truncate_block - read, zero a chunk and write a block
+ * Read, zero a chunk and write a block.
+ *
  * @inode - inode that we're zeroing
  * @from - the offset to start zeroing
  * @len - the length to zero, 0 to zero the entire range respective to the
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 79a125c0f4a2..c3128cdf1177 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -182,7 +182,8 @@ void btrfs_tree_read_unlock(struct extent_buffer *eb)
 }
 
 /*
- * __btrfs_tree_lock - lock eb for write
+ * Lock eb for write.
+ *
  * @eb:		the eb to lock
  * @nest:	the nesting to use for the lock
  *
diff --git a/fs/btrfs/messages.c b/fs/btrfs/messages.c
index 7695decc7243..5be060cb6ef5 100644
--- a/fs/btrfs/messages.c
+++ b/fs/btrfs/messages.c
@@ -110,8 +110,8 @@ const char * __attribute_const__ btrfs_decode_error(int errno)
 }
 
 /*
- * __btrfs_handle_fs_error decodes expected errors from the caller and
- * invokes the appropriate error response.
+ * Decodes expected errors from the caller and invokes the appropriate error
+ * response.
  */
 __cold
 void __btrfs_handle_fs_error(struct btrfs_fs_info *fs_info, const char *function,
@@ -283,8 +283,8 @@ void __cold btrfs_err_32bit_limit(struct btrfs_fs_info *fs_info)
 #endif
 
 /*
- * __btrfs_panic decodes unexpected, fatal errors from the caller, issues an
- * alert, and either panics or BUGs, depending on mount options.
+ * Decode unexpected, fatal errors from the caller, issue an alert, and either
+ * panic or BUGs, depending on mount options.
  */
 __cold
 void __btrfs_panic(struct btrfs_fs_info *fs_info, const char *function,
diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
index 95d28497de7c..26a7fb655f71 100644
--- a/fs/btrfs/ref-verify.c
+++ b/fs/btrfs/ref-verify.c
@@ -652,7 +652,7 @@ static void dump_block_entry(struct btrfs_fs_info *fs_info,
 }
 
 /*
- * btrfs_ref_tree_mod: called when we modify a ref for a bytenr
+ * Called when we modify a ref for a bytenr.
  *
  * This will add an action item to the given bytenr and do sanity checks to make
  * sure we haven't messed something up.  If we are making a new allocation and
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 859874579456..db992f7a5d38 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -51,7 +51,8 @@ static void btrfs_read_root_item(struct extent_buffer *eb, int slot,
 }
 
 /*
- * btrfs_find_root - lookup the root by the key.
+ * Lookup the root by the key.
+ *
  * root: the root of the root tree
  * search_key: the key to search
  * path: the path we search
@@ -485,7 +486,8 @@ void btrfs_update_root_times(struct btrfs_trans_handle *trans,
 }
 
 /*
- * btrfs_subvolume_reserve_metadata() - reserve space for subvolume operation
+ * Reserve space for subvolume operation.
+ *
  * root: the root of the parent directory
  * rsv: block reservation
  * items: the number of items that we need do reservation
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index d7e8cd4f140c..58de9a14e525 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -978,7 +978,8 @@ static bool steal_from_global_rsv(struct btrfs_fs_info *fs_info,
 }
 
 /*
- * maybe_fail_all_tickets - we've exhausted our flushing, start failing tickets
+ * We've exhausted our flushing, start failing tickets.
+ *
  * @fs_info - fs_info for this fs
  * @space_info - the space info we were flushing
  *
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 0bf42dccb041..035e7f5747cd 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -817,7 +817,7 @@ struct btrfs_trans_handle *btrfs_join_transaction_nostart(struct btrfs_root *roo
 }
 
 /*
- * btrfs_attach_transaction() - catch the running transaction
+ * Catch the running transaction.
  *
  * It is used when we want to commit the current the transaction, but
  * don't want to start a new one.
@@ -836,7 +836,7 @@ struct btrfs_trans_handle *btrfs_attach_transaction(struct btrfs_root *root)
 }
 
 /*
- * btrfs_attach_transaction_barrier() - catch the running transaction
+ * Catch the running transaction.
  *
  * It is similar to the above function, the difference is this one
  * will wait for all the inactive transactions until they fully
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index cbb17b542131..1834a6ec12bd 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2848,10 +2848,9 @@ static inline void btrfs_remove_all_log_ctxs(struct btrfs_root *root,
 }
 
 /*
- * btrfs_sync_log does sends a given tree log down to the disk and
- * updates the super blocks to record it.  When this call is done,
- * you know that any inodes previously logged are safely on disk only
- * if it returns 0.
+ * Sends a given tree log down to the disk and updates the super blocks to
+ * record it.  When this call is done, you know that any inodes previously
+ * logged are safely on disk only if it returns 0.
  *
  * Any other return value means you need to call btrfs_commit_transaction.
  * Some of the edge cases for fsyncing directories that have had unlinks
diff --git a/fs/btrfs/ulist.c b/fs/btrfs/ulist.c
index 33606025513d..b4ac2b0cd235 100644
--- a/fs/btrfs/ulist.c
+++ b/fs/btrfs/ulist.c
@@ -223,7 +223,8 @@ int ulist_add_merge(struct ulist *ulist, u64 val, u64 aux,
 }
 
 /*
- * ulist_del - delete one node from ulist
+ * Delete one node from ulist.
+ *
  * @ulist:	ulist to remove node from
  * @val:	value to delete
  * @aux:	aux to delete
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1325f76fbd50..871a55d36e32 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3027,7 +3027,8 @@ static int btrfs_del_sys_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset)
 }
 
 /*
- * btrfs_get_chunk_map() - Find the mapping containing the given logical extent.
+ * Find the mapping containing the given logical extent.
+ *
  * @logical: Logical block offset in bytes.
  * @length: Length of extent in bytes.
  *
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index e7ac4ec809a4..5511766485cd 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -145,7 +145,7 @@ static void zstd_reclaim_timer_fn(struct timer_list *timer)
 }
 
 /*
- * zstd_calc_ws_mem_sizes - calculate monotonic memory bounds
+ * Calculate monotonic memory bounds.
  *
  * It is possible based on the level configurations that a higher level
  * workspace uses less memory than a lower level workspace.  In order to reuse
@@ -218,7 +218,8 @@ void zstd_cleanup_workspace_manager(void)
 }
 
 /*
- * zstd_find_workspace - find workspace
+ * Find workspace for given level.
+ *
  * @level: compression level
  *
  * This iterates over the set bits in the active_map beginning at the requested
@@ -256,7 +257,8 @@ static struct list_head *zstd_find_workspace(unsigned int level)
 }
 
 /*
- * zstd_get_workspace - zstd's get_workspace
+ * Zstd get_workspace for level.
+ *
  * @level: compression level
  *
  * If @level is 0, then any compression level can be used.  Therefore, we begin
@@ -296,7 +298,8 @@ struct list_head *zstd_get_workspace(unsigned int level)
 }
 
 /*
- * zstd_put_workspace - zstd put_workspace
+ * Zstd put_workspace.
+ *
  * @ws: list_head for the workspace
  *
  * When putting back a workspace, we only need to update the LRU if we are of
-- 
2.41.0

Re: [PATCH 02/10] btrfs: reformat remaining kdoc style comments

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> Function name in the comment does not bring much value to code not
> exposed as API and we don't stick to the kdoc format anymore. Update
> formatting of parameter descriptions.

Mind to share which tool did you use to expose those comments?

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

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

Thanks,
Qu
> ---
>   fs/btrfs/ctree.c         |  4 ++--
>   fs/btrfs/delayed-inode.c |  6 +++---
>   fs/btrfs/delayed-ref.c   |  3 +--
>   fs/btrfs/disk-io.c       | 11 ++++++-----
>   fs/btrfs/extent-tree.c   |  6 +++---
>   fs/btrfs/extent_io.c     | 22 ++++++++++++----------
>   fs/btrfs/inode-item.c    |  2 +-
>   fs/btrfs/inode.c         |  9 +++++----
>   fs/btrfs/locking.c       |  3 ++-
>   fs/btrfs/messages.c      |  8 ++++----
>   fs/btrfs/ref-verify.c    |  2 +-
>   fs/btrfs/root-tree.c     |  6 ++++--
>   fs/btrfs/space-info.c    |  3 ++-
>   fs/btrfs/transaction.c   |  4 ++--
>   fs/btrfs/tree-log.c      |  7 +++----
>   fs/btrfs/ulist.c         |  3 ++-
>   fs/btrfs/volumes.c       |  3 ++-
>   fs/btrfs/zstd.c          | 11 +++++++----
>   18 files changed, 62 insertions(+), 51 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index a4cb4b642987..792f9e3afad8 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2054,8 +2054,8 @@ static int search_leaf(struct btrfs_trans_handle *trans,
>   }
>
>   /*
> - * btrfs_search_slot - look for a key in a tree and perform necessary
> - * modifications to preserve tree invariants.
> + * Look for a key in a tree and perform necessary modifications to preserve
> + * tree invariants.
>    *
>    * @trans:	Handle of transaction, used when modifying the tree
>    * @p:		Holds all btree nodes along the search path
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index d7ac39a3dd10..3e4fd354d458 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -328,7 +328,8 @@ static struct btrfs_delayed_item *btrfs_alloc_delayed_item(u16 data_len,
>   }
>
>   /*
> - * __btrfs_lookup_delayed_item - look up the delayed item by key
> + * Look up the delayed item by key.
> + *
>    * @delayed_node: pointer to the delayed node
>    * @index:	  the dir index value to lookup (offset of a dir index key)
>    *
> @@ -1760,8 +1761,7 @@ int btrfs_should_delete_dir_index(struct list_head *del_list,
>   }
>
>   /*
> - * btrfs_readdir_delayed_dir_index - read dir info stored in the delayed tree
> - *
> + * Read dir info stored in the delayed tree.
>    */
>   int btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
>   				    struct list_head *ins_list)
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 6a13cf00218b..c468148fc437 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -813,8 +813,7 @@ add_delayed_ref_head(struct btrfs_trans_handle *trans,
>   }
>
>   /*
> - * init_delayed_ref_common - Initialize the structure which represents a
> - *			     modification to a an extent.
> + * Initialize the structure which represents a modification to a an extent.
>    *
>    * @fs_info:    Internal to the mounted filesystem mount structure.
>    *
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 6404b17a5bdc..489589052f27 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1403,7 +1403,8 @@ struct btrfs_root *btrfs_get_new_fs_root(struct btrfs_fs_info *fs_info,
>   }
>
>   /*
> - * btrfs_get_fs_root_commit_root - return a root for the given objectid
> + * Return a root for the given objectid.
> + *
>    * @fs_info:	the fs_info
>    * @objectid:	the objectid we need to lookup
>    *
> @@ -1700,11 +1701,11 @@ static void backup_super_roots(struct btrfs_fs_info *info)
>   }
>
>   /*
> - * read_backup_root - Reads a backup root based on the passed priority. Prio 0
> - * is the newest, prio 1/2/3 are 2nd newest/3rd newest/4th (oldest) backup roots
> + * Reads a backup root based on the passed priority. Prio 0 is the newest, prio
> + * 1/2/3 are 2nd newest/3rd newest/4th (oldest) backup roots
>    *
> - * fs_info - filesystem whose backup roots need to be read
> - * priority - priority of backup root required
> + * @fs_info:  filesystem whose backup roots need to be read
> + * @priority: priority of backup root required
>    *
>    * Returns backup root index on success and -EINVAL otherwise.
>    */
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index e4d337b78e76..5ef4e852ae2e 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1435,7 +1435,7 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
>   }
>
>   /*
> - * __btrfs_inc_extent_ref - insert backreference for a given extent
> + * Insert backreference for a given extent.
>    *
>    * The counterpart is in __btrfs_free_extent(), with examples and more details
>    * how it works.
> @@ -4440,8 +4440,8 @@ static noinline int find_free_extent(struct btrfs_root *root,
>   }
>
>   /*
> - * btrfs_reserve_extent - entry point to the extent allocator. Tries to find a
> - *			  hole that is at least as big as @num_bytes.
> + * Entry point to the extent allocator. Tries to find a hole that is at least
> + * as big as @num_bytes.
>    *
>    * @root           -	The root that will contain this extent
>    *
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index ac3fca5a5e41..8156dcc4b1fa 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4249,14 +4249,14 @@ void copy_extent_buffer(const struct extent_buffer *dst,
>   }
>
>   /*
> - * eb_bitmap_offset() - calculate the page and offset of the byte containing the
> - * given bit number
> - * @eb: the extent buffer
> - * @start: offset of the bitmap item in the extent buffer
> - * @nr: bit number
> - * @page_index: return index of the page in the extent buffer that contains the
> - * given bit number
> - * @page_offset: return offset into the page given by page_index
> + * Calculate the page and offset of the byte containing the given bit number.
> + *
> + * @eb:           the extent buffer
> + * @start:        offset of the bitmap item in the extent buffer
> + * @nr:           bit number
> + * @page_index:   return index of the page in the extent buffer that contains
> + *                the given bit number
> + * @page_offset:  return offset into the page given by page_index
>    *
>    * This helper hides the ugliness of finding the byte in an extent buffer which
>    * contains a given bit.
> @@ -4615,7 +4615,8 @@ int try_release_extent_buffer(struct page *page)
>   }
>
>   /*
> - * btrfs_readahead_tree_block - attempt to readahead a child block
> + * Attempt to readahead a child block.
> + *
>    * @fs_info:	the fs_info
>    * @bytenr:	bytenr to read
>    * @owner_root: objectid of the root that owns this eb
> @@ -4654,7 +4655,8 @@ void btrfs_readahead_tree_block(struct btrfs_fs_info *fs_info,
>   }
>
>   /*
> - * btrfs_readahead_node_child - readahead a node's child block
> + * Readahead a node's child block.
> + *
>    * @node:	parent node we're reading from
>    * @slot:	slot in the parent node for the child we want to read
>    *
> diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
> index 4c322b720a80..c19c0f10f0e2 100644
> --- a/fs/btrfs/inode-item.c
> +++ b/fs/btrfs/inode-item.c
> @@ -247,7 +247,7 @@ int btrfs_del_inode_ref(struct btrfs_trans_handle *trans,
>   }
>
>   /*
> - * btrfs_insert_inode_extref() - Inserts an extended inode ref into a tree.
> + * Insert an extended inode ref into a tree.
>    *
>    * The caller must have checked against BTRFS_LINK_MAX already.
>    */
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index f09fbdc43f0f..032265d0946a 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -348,7 +348,7 @@ static void __cold btrfs_print_data_csum_error(struct btrfs_inode *inode,
>   }
>
>   /*
> - * btrfs_inode_lock - lock inode i_rwsem based on arguments passed
> + * Lock inode i_rwsem based on arguments passed.
>    *
>    * ilock_flags can have the following bit set:
>    *
> @@ -382,7 +382,7 @@ int btrfs_inode_lock(struct btrfs_inode *inode, unsigned int ilock_flags)
>   }
>
>   /*
> - * btrfs_inode_unlock - unock inode i_rwsem
> + * Unock inode i_rwsem.
>    *
>    * ilock_flags should contain the same bits set as passed to btrfs_inode_lock()
>    * to decide whether the lock acquired is shared or exclusive.
> @@ -3310,7 +3310,7 @@ bool btrfs_data_csum_ok(struct btrfs_bio *bbio, struct btrfs_device *dev,
>   }
>
>   /*
> - * btrfs_add_delayed_iput - perform a delayed iput on @inode
> + * Perform a delayed iput on @inode.
>    *
>    * @inode: The inode we want to perform iput on
>    *
> @@ -4645,7 +4645,8 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
>   }
>
>   /*
> - * btrfs_truncate_block - read, zero a chunk and write a block
> + * Read, zero a chunk and write a block.
> + *
>    * @inode - inode that we're zeroing
>    * @from - the offset to start zeroing
>    * @len - the length to zero, 0 to zero the entire range respective to the
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index 79a125c0f4a2..c3128cdf1177 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -182,7 +182,8 @@ void btrfs_tree_read_unlock(struct extent_buffer *eb)
>   }
>
>   /*
> - * __btrfs_tree_lock - lock eb for write
> + * Lock eb for write.
> + *
>    * @eb:		the eb to lock
>    * @nest:	the nesting to use for the lock
>    *
> diff --git a/fs/btrfs/messages.c b/fs/btrfs/messages.c
> index 7695decc7243..5be060cb6ef5 100644
> --- a/fs/btrfs/messages.c
> +++ b/fs/btrfs/messages.c
> @@ -110,8 +110,8 @@ const char * __attribute_const__ btrfs_decode_error(int errno)
>   }
>
>   /*
> - * __btrfs_handle_fs_error decodes expected errors from the caller and
> - * invokes the appropriate error response.
> + * Decodes expected errors from the caller and invokes the appropriate error
> + * response.
>    */
>   __cold
>   void __btrfs_handle_fs_error(struct btrfs_fs_info *fs_info, const char *function,
> @@ -283,8 +283,8 @@ void __cold btrfs_err_32bit_limit(struct btrfs_fs_info *fs_info)
>   #endif
>
>   /*
> - * __btrfs_panic decodes unexpected, fatal errors from the caller, issues an
> - * alert, and either panics or BUGs, depending on mount options.
> + * Decode unexpected, fatal errors from the caller, issue an alert, and either
> + * panic or BUGs, depending on mount options.
>    */
>   __cold
>   void __btrfs_panic(struct btrfs_fs_info *fs_info, const char *function,
> diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
> index 95d28497de7c..26a7fb655f71 100644
> --- a/fs/btrfs/ref-verify.c
> +++ b/fs/btrfs/ref-verify.c
> @@ -652,7 +652,7 @@ static void dump_block_entry(struct btrfs_fs_info *fs_info,
>   }
>
>   /*
> - * btrfs_ref_tree_mod: called when we modify a ref for a bytenr
> + * Called when we modify a ref for a bytenr.
>    *
>    * This will add an action item to the given bytenr and do sanity checks to make
>    * sure we haven't messed something up.  If we are making a new allocation and
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 859874579456..db992f7a5d38 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -51,7 +51,8 @@ static void btrfs_read_root_item(struct extent_buffer *eb, int slot,
>   }
>
>   /*
> - * btrfs_find_root - lookup the root by the key.
> + * Lookup the root by the key.
> + *
>    * root: the root of the root tree
>    * search_key: the key to search
>    * path: the path we search
> @@ -485,7 +486,8 @@ void btrfs_update_root_times(struct btrfs_trans_handle *trans,
>   }
>
>   /*
> - * btrfs_subvolume_reserve_metadata() - reserve space for subvolume operation
> + * Reserve space for subvolume operation.
> + *
>    * root: the root of the parent directory
>    * rsv: block reservation
>    * items: the number of items that we need do reservation
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index d7e8cd4f140c..58de9a14e525 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -978,7 +978,8 @@ static bool steal_from_global_rsv(struct btrfs_fs_info *fs_info,
>   }
>
>   /*
> - * maybe_fail_all_tickets - we've exhausted our flushing, start failing tickets
> + * We've exhausted our flushing, start failing tickets.
> + *
>    * @fs_info - fs_info for this fs
>    * @space_info - the space info we were flushing
>    *
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 0bf42dccb041..035e7f5747cd 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -817,7 +817,7 @@ struct btrfs_trans_handle *btrfs_join_transaction_nostart(struct btrfs_root *roo
>   }
>
>   /*
> - * btrfs_attach_transaction() - catch the running transaction
> + * Catch the running transaction.
>    *
>    * It is used when we want to commit the current the transaction, but
>    * don't want to start a new one.
> @@ -836,7 +836,7 @@ struct btrfs_trans_handle *btrfs_attach_transaction(struct btrfs_root *root)
>   }
>
>   /*
> - * btrfs_attach_transaction_barrier() - catch the running transaction
> + * Catch the running transaction.
>    *
>    * It is similar to the above function, the difference is this one
>    * will wait for all the inactive transactions until they fully
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index cbb17b542131..1834a6ec12bd 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -2848,10 +2848,9 @@ static inline void btrfs_remove_all_log_ctxs(struct btrfs_root *root,
>   }
>
>   /*
> - * btrfs_sync_log does sends a given tree log down to the disk and
> - * updates the super blocks to record it.  When this call is done,
> - * you know that any inodes previously logged are safely on disk only
> - * if it returns 0.
> + * Sends a given tree log down to the disk and updates the super blocks to
> + * record it.  When this call is done, you know that any inodes previously
> + * logged are safely on disk only if it returns 0.
>    *
>    * Any other return value means you need to call btrfs_commit_transaction.
>    * Some of the edge cases for fsyncing directories that have had unlinks
> diff --git a/fs/btrfs/ulist.c b/fs/btrfs/ulist.c
> index 33606025513d..b4ac2b0cd235 100644
> --- a/fs/btrfs/ulist.c
> +++ b/fs/btrfs/ulist.c
> @@ -223,7 +223,8 @@ int ulist_add_merge(struct ulist *ulist, u64 val, u64 aux,
>   }
>
>   /*
> - * ulist_del - delete one node from ulist
> + * Delete one node from ulist.
> + *
>    * @ulist:	ulist to remove node from
>    * @val:	value to delete
>    * @aux:	aux to delete
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1325f76fbd50..871a55d36e32 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3027,7 +3027,8 @@ static int btrfs_del_sys_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset)
>   }
>
>   /*
> - * btrfs_get_chunk_map() - Find the mapping containing the given logical extent.
> + * Find the mapping containing the given logical extent.
> + *
>    * @logical: Logical block offset in bytes.
>    * @length: Length of extent in bytes.
>    *
> diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> index e7ac4ec809a4..5511766485cd 100644
> --- a/fs/btrfs/zstd.c
> +++ b/fs/btrfs/zstd.c
> @@ -145,7 +145,7 @@ static void zstd_reclaim_timer_fn(struct timer_list *timer)
>   }
>
>   /*
> - * zstd_calc_ws_mem_sizes - calculate monotonic memory bounds
> + * Calculate monotonic memory bounds.
>    *
>    * It is possible based on the level configurations that a higher level
>    * workspace uses less memory than a lower level workspace.  In order to reuse
> @@ -218,7 +218,8 @@ void zstd_cleanup_workspace_manager(void)
>   }
>
>   /*
> - * zstd_find_workspace - find workspace
> + * Find workspace for given level.
> + *
>    * @level: compression level
>    *
>    * This iterates over the set bits in the active_map beginning at the requested
> @@ -256,7 +257,8 @@ static struct list_head *zstd_find_workspace(unsigned int level)
>   }
>
>   /*
> - * zstd_get_workspace - zstd's get_workspace
> + * Zstd get_workspace for level.
> + *
>    * @level: compression level
>    *
>    * If @level is 0, then any compression level can be used.  Therefore, we begin
> @@ -296,7 +298,8 @@ struct list_head *zstd_get_workspace(unsigned int level)
>   }
>
>   /*
> - * zstd_put_workspace - zstd put_workspace
> + * Zstd put_workspace.
> + *
>    * @ws: list_head for the workspace
>    *
>    * When putting back a workspace, we only need to update the LRU if we are of

Re: [PATCH 02/10] btrfs: reformat remaining kdoc style comments

[David Sterba]

On Fri, Sep 08, 2023 at 07:51:01AM +0800, Qu Wenruo wrote:
> On 2023/9/8 07:09, David Sterba wrote:
> > Function name in the comment does not bring much value to code not
> > exposed as API and we don't stick to the kdoc format anymore. Update
> > formatting of parameter descriptions.
> 
> Mind to share which tool did you use to expose those comments?

vim in a loop on *.c, the comments really stand out and with the
foldmethod=syntax it's easy to page through the files.

[PATCH 03/10] btrfs: drop __must_check annotations

[David Sterba]

Drop all __must_check annotations because they're used in random
functions and not consistently. All errors should be handled.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent-tree.h | 2 +-
 fs/btrfs/relocation.c  | 2 +-
 fs/btrfs/root-tree.h   | 6 ++----
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
index ab2016db17e8..2109c72aea2a 100644
--- a/fs/btrfs/extent-tree.h
+++ b/fs/btrfs/extent-tree.h
@@ -142,7 +142,7 @@ int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start, u64 len);
 int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
 int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, struct btrfs_ref *generic_ref);
-int __must_check btrfs_drop_snapshot(struct btrfs_root *root, int update_ref,
+int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref,
 				     int for_reloc);
 int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
 			struct btrfs_root *root,
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 9951a0caf5bb..ad67a88f2bbf 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -631,7 +631,7 @@ static int clone_backref_node(struct btrfs_trans_handle *trans,
 /*
  * helper to add 'address of tree root -> reloc tree' mapping
  */
-static int __must_check __add_reloc_root(struct btrfs_root *root)
+static int __add_reloc_root(struct btrfs_root *root)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct rb_node *rb_node;
diff --git a/fs/btrfs/root-tree.h b/fs/btrfs/root-tree.h
index eb15768b9170..8b2c3859e464 100644
--- a/fs/btrfs/root-tree.h
+++ b/fs/btrfs/root-tree.h
@@ -20,10 +20,8 @@ int btrfs_del_root(struct btrfs_trans_handle *trans, const struct btrfs_key *key
 int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		      const struct btrfs_key *key,
 		      struct btrfs_root_item *item);
-int __must_check btrfs_update_root(struct btrfs_trans_handle *trans,
-				   struct btrfs_root *root,
-				   struct btrfs_key *key,
-				   struct btrfs_root_item *item);
+int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
+		      struct btrfs_key *key, struct btrfs_root_item *item);
 int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
 		    struct btrfs_path *path, struct btrfs_root_item *root_item,
 		    struct btrfs_key *root_key);
-- 
2.41.0

Re: [PATCH 03/10] btrfs: drop __must_check annotations

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> Drop all __must_check annotations because they're used in random
> functions and not consistently. All errors should be handled.

Is there any compiler warning option to warn about unchecked return value?

In fact recently when working on qgroup GFP_ATOMIC, I found a call site
that we didn't handle error at all (qgroup_update_counters()).

I'm pretty sure that is not the last one.

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

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

Thanks,
Qu
> ---
>   fs/btrfs/extent-tree.h | 2 +-
>   fs/btrfs/relocation.c  | 2 +-
>   fs/btrfs/root-tree.h   | 6 ++----
>   3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
> index ab2016db17e8..2109c72aea2a 100644
> --- a/fs/btrfs/extent-tree.h
> +++ b/fs/btrfs/extent-tree.h
> @@ -142,7 +142,7 @@ int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>   int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start, u64 len);
>   int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
>   int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, struct btrfs_ref *generic_ref);
> -int __must_check btrfs_drop_snapshot(struct btrfs_root *root, int update_ref,
> +int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref,
>   				     int for_reloc);
>   int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
>   			struct btrfs_root *root,
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 9951a0caf5bb..ad67a88f2bbf 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -631,7 +631,7 @@ static int clone_backref_node(struct btrfs_trans_handle *trans,
>   /*
>    * helper to add 'address of tree root -> reloc tree' mapping
>    */
> -static int __must_check __add_reloc_root(struct btrfs_root *root)
> +static int __add_reloc_root(struct btrfs_root *root)
>   {
>   	struct btrfs_fs_info *fs_info = root->fs_info;
>   	struct rb_node *rb_node;
> diff --git a/fs/btrfs/root-tree.h b/fs/btrfs/root-tree.h
> index eb15768b9170..8b2c3859e464 100644
> --- a/fs/btrfs/root-tree.h
> +++ b/fs/btrfs/root-tree.h
> @@ -20,10 +20,8 @@ int btrfs_del_root(struct btrfs_trans_handle *trans, const struct btrfs_key *key
>   int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>   		      const struct btrfs_key *key,
>   		      struct btrfs_root_item *item);
> -int __must_check btrfs_update_root(struct btrfs_trans_handle *trans,
> -				   struct btrfs_root *root,
> -				   struct btrfs_key *key,
> -				   struct btrfs_root_item *item);
> +int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
> +		      struct btrfs_key *key, struct btrfs_root_item *item);
>   int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
>   		    struct btrfs_path *path, struct btrfs_root_item *root_item,
>   		    struct btrfs_key *root_key);

Re: [PATCH 03/10] btrfs: drop __must_check annotations

[David Sterba]

On Fri, Sep 08, 2023 at 07:56:24AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/9/8 07:09, David Sterba wrote:
> > Drop all __must_check annotations because they're used in random
> > functions and not consistently. All errors should be handled.
> 
> Is there any compiler warning option to warn about unchecked return value?

By default this is checked for functions annotated by warn_unused_result
attribute, which is exactly what __must_check does. The check can be
disabled but that's not what we want.

> In fact recently when working on qgroup GFP_ATOMIC, I found a call site
> that we didn't handle error at all (qgroup_update_counters()).

A quick way how to check if a given function is properly handled is to
add the __must_check attribute and run build. For qgroup_update_counters
this is found right away:

  CC [M]  fs/btrfs/qgroup.o
fs/btrfs/qgroup.c: In function ‘btrfs_qgroup_account_extent’:
fs/btrfs/qgroup.c:2736:9: warning: ignoring return value of ‘qgroup_update_counters’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
 2736 |         qgroup_update_counters(fs_info, qgroups, nr_old_roots, nr_new_roots,
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2737 |                                num_bytes, seq);
      |                                ~~~~~~~~~~~~~~~

> I'm pretty sure that is not the last one.

Yeah but we'd have to add the attribute to most if not all functions. It
could be possibly automated using coccinnelle scripts, list functions,
apply a semantic patch to add the attribute, compile it and get the
results. A quick test shows it's doable.

I think in general the error handling is good but if we want to make
sure we haven't missed anything important we can't do it manually
efficiently.

Re: [PATCH 03/10] btrfs: drop __must_check annotations

[David Sterba]

On Fri, Sep 08, 2023 at 02:34:15AM +0200, David Sterba wrote:
> On Fri, Sep 08, 2023 at 07:56:24AM +0800, Qu Wenruo wrote:
> > 
> > 
> > On 2023/9/8 07:09, David Sterba wrote:
> > > Drop all __must_check annotations because they're used in random
> > > functions and not consistently. All errors should be handled.
> > 
> > Is there any compiler warning option to warn about unchecked return value?
> 
> By default this is checked for functions annotated by warn_unused_result
> attribute, which is exactly what __must_check does. The check can be
> disabled but that's not what we want.
> 
> > In fact recently when working on qgroup GFP_ATOMIC, I found a call site
> > that we didn't handle error at all (qgroup_update_counters()).
> 
> A quick way how to check if a given function is properly handled is to
> add the __must_check attribute and run build. For qgroup_update_counters
> this is found right away:
> 
>   CC [M]  fs/btrfs/qgroup.o
> fs/btrfs/qgroup.c: In function ‘btrfs_qgroup_account_extent’:
> fs/btrfs/qgroup.c:2736:9: warning: ignoring return value of ‘qgroup_update_counters’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
>  2736 |         qgroup_update_counters(fs_info, qgroups, nr_old_roots, nr_new_roots,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  2737 |                                num_bytes, seq);
>       |                                ~~~~~~~~~~~~~~~
> 
> > I'm pretty sure that is not the last one.
> 
> Yeah but we'd have to add the attribute to most if not all functions. It
> could be possibly automated using coccinnelle scripts, list functions,
> apply a semantic patch to add the attribute, compile it and get the
> results. A quick test shows it's doable.

We have 3179 functions, lots of them return void (but they might ignore
return values from called functions so it does not mean they're safe).
There are 65 Functions that don't have all their return values checked,
which 2%.

There are several that do the extent bit handling and we knowingly don't
handle the errors because all fatal erros lead to panic. The remaining
are a mixed bag, I tried to check a few randomly and it seems that the
error handling could lead to bad situations.

Ideally we should audit the whole list, and also maybe identify some key
functions that should have the __must_check attribute, e.g.
btrfs_commit_transaction (which is not in the list).

add_async_extent
add_delayed_ref_head
add_extent_mapping
add_ra_bio_pages
addrm_unknown_feature_attrs
btree_release_folio
__btrfs_add_free_space
btrfs_block_rsv_release
btrfs_cache_block_group
btrfs_cleanup_transaction
__btrfs_commit_inode_delayed_items
btrfs_del_item
btrfs_do_readpage
btrfs_end_transaction
btrfs_finish_ordered_io
btrfs_forget_devices
btrfs_free_reserved_extent
btrfs_free_stale_devices
btrfs_grab_root
btrfs_inode_lock
btrfs_log_inode_parent
btrfs_orphan_del
btrfs_pin_extent
btrfs_ref_tree_mod
__btrfs_release_folio
btrfs_release_folio
btrfs_repair_eb_io_failure
btrfs_reset_sb_log_zones
__btrfs_run_defrag_inode
btrfs_wait_block_group_cache_done
btrfs_wq_run_delayed_node
btrfs_zone_activate
cache_save_setup
clear_extent_bit
clear_extent_bits
clear_extent_dirty
clear_extent_uptodate
clear_state_bit
del_qgroup_rb
del_qgroup_relation_item
del_relation_rb
dev_extent_hole_check
do_zone_finish
fill_writer_pointer_gap
find_next_key
get_raid56_logic_offset
inc_block_group_ro
insert_root_entry
invalidate_extent_cache
link_free_space
load_block_group_size_class
maybe_fail_all_tickets
pin_down_extent
process_page_range
push_for_double_split
qgroup_unreserve_range
qgroup_update_counters
release_extent_buffer
remove_from_discard_list
tree_insert_offset
try_merge_free_space
unlock_extent
unpin_extent_range
update_backref_cache
__update_reloc_root

[PATCH 04/10] btrfs: reduce parameters of btrfs_pin_reserved_extent

[David Sterba]

There is only one caller of btrfs_pin_reserved_extent that expands the
parameters to extent buffer members. We can simply pass the extent
buffer instead.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent-tree.c | 10 +++++-----
 fs/btrfs/extent-tree.h |  3 ++-
 fs/btrfs/tree-log.c    |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 5ef4e852ae2e..98d97c97ab8c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4560,20 +4560,20 @@ int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start,
-			      u64 len)
+int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans,
+			      const struct extent_buffer *eb)
 {
 	struct btrfs_block_group *cache;
 	int ret = 0;
 
-	cache = btrfs_lookup_block_group(trans->fs_info, start);
+	cache = btrfs_lookup_block_group(trans->fs_info, eb->start);
 	if (!cache) {
 		btrfs_err(trans->fs_info, "unable to find block group for %llu",
-			  start);
+			  eb->start);
 		return -ENOSPC;
 	}
 
-	ret = pin_down_extent(trans, cache, start, len, 1);
+	ret = pin_down_extent(trans, cache, eb->start, eb->len, 1);
 	btrfs_put_block_group(cache);
 	return ret;
 }
diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
index 2109c72aea2a..c56f616dcd1b 100644
--- a/fs/btrfs/extent-tree.h
+++ b/fs/btrfs/extent-tree.h
@@ -139,7 +139,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, struct btrfs_ref *ref);
 
 int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 			       u64 start, u64 len, int delalloc);
-int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start, u64 len);
+int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans,
+			      const struct extent_buffer *eb);
 int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
 int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, struct btrfs_ref *generic_ref);
 int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref,
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 1834a6ec12bd..4f85c435b793 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2574,7 +2574,7 @@ static int clean_log_buffer(struct btrfs_trans_handle *trans,
 	btrfs_tree_unlock(eb);
 
 	if (trans) {
-		ret = btrfs_pin_reserved_extent(trans, eb->start, eb->len);
+		ret = btrfs_pin_reserved_extent(trans, eb);
 		if (ret)
 			return ret;
 		btrfs_redirty_list_add(trans->transaction, eb);
-- 
2.41.0

Re: [PATCH 04/10] btrfs: reduce parameters of btrfs_pin_reserved_extent

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> There is only one caller of btrfs_pin_reserved_extent that expands the
> parameters to extent buffer members. We can simply pass the extent
> buffer instead.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/extent-tree.c | 10 +++++-----
>   fs/btrfs/extent-tree.h |  3 ++-
>   fs/btrfs/tree-log.c    |  2 +-
>   3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 5ef4e852ae2e..98d97c97ab8c 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4560,20 +4560,20 @@ int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>   	return 0;
>   }
>
> -int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start,
> -			      u64 len)
> +int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans,
> +			      const struct extent_buffer *eb)
>   {
>   	struct btrfs_block_group *cache;
>   	int ret = 0;
>
> -	cache = btrfs_lookup_block_group(trans->fs_info, start);
> +	cache = btrfs_lookup_block_group(trans->fs_info, eb->start);
>   	if (!cache) {
>   		btrfs_err(trans->fs_info, "unable to find block group for %llu",
> -			  start);
> +			  eb->start);
>   		return -ENOSPC;
>   	}
>
> -	ret = pin_down_extent(trans, cache, start, len, 1);
> +	ret = pin_down_extent(trans, cache, eb->start, eb->len, 1);
>   	btrfs_put_block_group(cache);
>   	return ret;
>   }
> diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
> index 2109c72aea2a..c56f616dcd1b 100644
> --- a/fs/btrfs/extent-tree.h
> +++ b/fs/btrfs/extent-tree.h
> @@ -139,7 +139,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, struct btrfs_ref *ref);
>
>   int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>   			       u64 start, u64 len, int delalloc);
> -int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start, u64 len);
> +int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans,
> +			      const struct extent_buffer *eb);
>   int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
>   int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, struct btrfs_ref *generic_ref);
>   int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref,
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 1834a6ec12bd..4f85c435b793 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -2574,7 +2574,7 @@ static int clean_log_buffer(struct btrfs_trans_handle *trans,
>   	btrfs_tree_unlock(eb);
>
>   	if (trans) {
> -		ret = btrfs_pin_reserved_extent(trans, eb->start, eb->len);
> +		ret = btrfs_pin_reserved_extent(trans, eb);
>   		if (ret)
>   			return ret;
>   		btrfs_redirty_list_add(trans->transaction, eb);

[PATCH 05/10] btrfs: reduce parameters of btrfs_pin_extent_for_log_replay

[David Sterba]

Both callers of btrfs_pin_extent_for_log_replay expand the parameters to
extent buffer members. We can simply pass the extent buffer instead.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent-tree.c | 8 ++++----
 fs/btrfs/extent-tree.h | 2 +-
 fs/btrfs/tree-log.c    | 7 ++-----
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 98d97c97ab8c..3e46bb4cc957 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2567,12 +2567,12 @@ int btrfs_pin_extent(struct btrfs_trans_handle *trans,
  * this function must be called within transaction
  */
 int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
-				    u64 bytenr, u64 num_bytes)
+				    const struct extent_buffer *eb)
 {
 	struct btrfs_block_group *cache;
 	int ret;
 
-	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
+	cache = btrfs_lookup_block_group(trans->fs_info, eb->start);
 	if (!cache)
 		return -EINVAL;
 
@@ -2584,10 +2584,10 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
 	if (ret)
 		goto out;
 
-	pin_down_extent(trans, cache, bytenr, num_bytes, 0);
+	pin_down_extent(trans, cache, eb->start, eb->len, 0);
 
 	/* remove us from the free space cache (if we're there at all) */
-	ret = btrfs_remove_free_space(cache, bytenr, num_bytes);
+	ret = btrfs_remove_free_space(cache, eb->start, eb->len);
 out:
 	btrfs_put_block_group(cache);
 	return ret;
diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
index c56f616dcd1b..dd31ee85f360 100644
--- a/fs/btrfs/extent-tree.h
+++ b/fs/btrfs/extent-tree.h
@@ -103,7 +103,7 @@ int btrfs_lookup_extent_info(struct btrfs_trans_handle *trans,
 int btrfs_pin_extent(struct btrfs_trans_handle *trans, u64 bytenr, u64 num,
 		     int reserved);
 int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
-				    u64 bytenr, u64 num_bytes);
+				    const struct extent_buffer *eb);
 int btrfs_exclude_logged_extents(struct extent_buffer *eb);
 int btrfs_cross_ref_exist(struct btrfs_root *root,
 			  u64 objectid, u64 offset, u64 bytenr, bool strict,
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 4f85c435b793..15c8cb6627fe 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -347,8 +347,7 @@ static int process_one_buffer(struct btrfs_root *log,
 	}
 
 	if (wc->pin) {
-		ret = btrfs_pin_extent_for_log_replay(wc->trans, eb->start,
-						      eb->len);
+		ret = btrfs_pin_extent_for_log_replay(wc->trans, eb);
 		if (ret)
 			return ret;
 
@@ -7203,9 +7202,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
 			 * each subsequent pass.
 			 */
 			if (ret == -ENOENT)
-				ret = btrfs_pin_extent_for_log_replay(trans,
-							log->node->start,
-							log->node->len);
+				ret = btrfs_pin_extent_for_log_replay(trans, log->node);
 			btrfs_put_root(log);
 
 			if (!ret)
-- 
2.41.0

Re: [PATCH 05/10] btrfs: reduce parameters of btrfs_pin_extent_for_log_replay

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> Both callers of btrfs_pin_extent_for_log_replay expand the parameters to
> extent buffer members. We can simply pass the extent buffer instead.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/extent-tree.c | 8 ++++----
>   fs/btrfs/extent-tree.h | 2 +-
>   fs/btrfs/tree-log.c    | 7 ++-----
>   3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 98d97c97ab8c..3e46bb4cc957 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2567,12 +2567,12 @@ int btrfs_pin_extent(struct btrfs_trans_handle *trans,
>    * this function must be called within transaction
>    */
>   int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
> -				    u64 bytenr, u64 num_bytes)
> +				    const struct extent_buffer *eb)
>   {
>   	struct btrfs_block_group *cache;
>   	int ret;
>
> -	cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
> +	cache = btrfs_lookup_block_group(trans->fs_info, eb->start);
>   	if (!cache)
>   		return -EINVAL;
>
> @@ -2584,10 +2584,10 @@ int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
>   	if (ret)
>   		goto out;
>
> -	pin_down_extent(trans, cache, bytenr, num_bytes, 0);
> +	pin_down_extent(trans, cache, eb->start, eb->len, 0);
>
>   	/* remove us from the free space cache (if we're there at all) */
> -	ret = btrfs_remove_free_space(cache, bytenr, num_bytes);
> +	ret = btrfs_remove_free_space(cache, eb->start, eb->len);
>   out:
>   	btrfs_put_block_group(cache);
>   	return ret;
> diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
> index c56f616dcd1b..dd31ee85f360 100644
> --- a/fs/btrfs/extent-tree.h
> +++ b/fs/btrfs/extent-tree.h
> @@ -103,7 +103,7 @@ int btrfs_lookup_extent_info(struct btrfs_trans_handle *trans,
>   int btrfs_pin_extent(struct btrfs_trans_handle *trans, u64 bytenr, u64 num,
>   		     int reserved);
>   int btrfs_pin_extent_for_log_replay(struct btrfs_trans_handle *trans,
> -				    u64 bytenr, u64 num_bytes);
> +				    const struct extent_buffer *eb);
>   int btrfs_exclude_logged_extents(struct extent_buffer *eb);
>   int btrfs_cross_ref_exist(struct btrfs_root *root,
>   			  u64 objectid, u64 offset, u64 bytenr, bool strict,
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 4f85c435b793..15c8cb6627fe 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -347,8 +347,7 @@ static int process_one_buffer(struct btrfs_root *log,
>   	}
>
>   	if (wc->pin) {
> -		ret = btrfs_pin_extent_for_log_replay(wc->trans, eb->start,
> -						      eb->len);
> +		ret = btrfs_pin_extent_for_log_replay(wc->trans, eb);
>   		if (ret)
>   			return ret;
>
> @@ -7203,9 +7202,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree)
>   			 * each subsequent pass.
>   			 */
>   			if (ret == -ENOENT)
> -				ret = btrfs_pin_extent_for_log_replay(trans,
> -							log->node->start,
> -							log->node->len);
> +				ret = btrfs_pin_extent_for_log_replay(trans, log->node);
>   			btrfs_put_root(log);
>
>   			if (!ret)

[PATCH 06/10] btrfs: reduce arguments of helpers space accounting root item

[David Sterba]

There are two helpers to increase used bytes of root items that add or
subtract one node size, we don't need to pass the argument for that.
Rename the function so it matches the root item member that gets
changed.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 792f9e3afad8..6d18f6d5a8b3 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -947,19 +947,19 @@ int btrfs_bin_search(struct extent_buffer *eb, int first_slot,
 	return 1;
 }
 
-static void root_add_used(struct btrfs_root *root, u32 size)
+static void root_add_used_bytes(struct btrfs_root *root)
 {
 	spin_lock(&root->accounting_lock);
 	btrfs_set_root_used(&root->root_item,
-			    btrfs_root_used(&root->root_item) + size);
+		btrfs_root_used(&root->root_item) + root->fs_info->nodesize);
 	spin_unlock(&root->accounting_lock);
 }
 
-static void root_sub_used(struct btrfs_root *root, u32 size)
+static void root_sub_used_bytes(struct btrfs_root *root)
 {
 	spin_lock(&root->accounting_lock);
 	btrfs_set_root_used(&root->root_item,
-			    btrfs_root_used(&root->root_item) - size);
+		btrfs_root_used(&root->root_item) - root->fs_info->nodesize);
 	spin_unlock(&root->accounting_lock);
 }
 
@@ -1075,7 +1075,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 		/* once for the path */
 		free_extent_buffer(mid);
 
-		root_sub_used(root, mid->len);
+		root_sub_used_bytes(root);
 		btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
 		/* once for the root ptr */
 		free_extent_buffer_stale(mid);
@@ -1145,7 +1145,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 				right = NULL;
 				goto out;
 			}
-			root_sub_used(root, right->len);
+			root_sub_used_bytes(root);
 			btrfs_free_tree_block(trans, btrfs_root_id(root), right,
 					      0, 1);
 			free_extent_buffer_stale(right);
@@ -1203,7 +1203,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 			mid = NULL;
 			goto out;
 		}
-		root_sub_used(root, mid->len);
+		root_sub_used_bytes(root);
 		btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
 		free_extent_buffer_stale(mid);
 		mid = NULL;
@@ -2937,7 +2937,6 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
 			   struct btrfs_root *root,
 			   struct btrfs_path *path, int level)
 {
-	struct btrfs_fs_info *fs_info = root->fs_info;
 	u64 lower_gen;
 	struct extent_buffer *lower;
 	struct extent_buffer *c;
@@ -2960,7 +2959,7 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
 	if (IS_ERR(c))
 		return PTR_ERR(c);
 
-	root_add_used(root, fs_info->nodesize);
+	root_add_used_bytes(root);
 
 	btrfs_set_header_nritems(c, 1);
 	btrfs_set_node_key(c, &lower_key, 0);
@@ -3104,7 +3103,7 @@ static noinline int split_node(struct btrfs_trans_handle *trans,
 	if (IS_ERR(split))
 		return PTR_ERR(split);
 
-	root_add_used(root, fs_info->nodesize);
+	root_add_used_bytes(root);
 	ASSERT(btrfs_header_level(c) == level);
 
 	ret = btrfs_tree_mod_log_eb_copy(split, c, 0, mid, c_nritems - mid);
@@ -3857,7 +3856,7 @@ static noinline int split_leaf(struct btrfs_trans_handle *trans,
 	if (IS_ERR(right))
 		return PTR_ERR(right);
 
-	root_add_used(root, fs_info->nodesize);
+	root_add_used_bytes(root);
 
 	if (split == 0) {
 		if (mid <= slot) {
@@ -4530,7 +4529,7 @@ static noinline int btrfs_del_leaf(struct btrfs_trans_handle *trans,
 	 */
 	btrfs_unlock_up_safe(path, 0);
 
-	root_sub_used(root, leaf->len);
+	root_sub_used_bytes(root);
 
 	atomic_inc(&leaf->refs);
 	btrfs_free_tree_block(trans, btrfs_root_id(root), leaf, 0, 1);
-- 
2.41.0

Re: [PATCH 06/10] btrfs: reduce arguments of helpers space accounting root item

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> There are two helpers to increase used bytes of root items that add or
> subtract one node size, we don't need to pass the argument for that.
> Rename the function so it matches the root item member that gets
> changed.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/ctree.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 792f9e3afad8..6d18f6d5a8b3 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -947,19 +947,19 @@ int btrfs_bin_search(struct extent_buffer *eb, int first_slot,
>   	return 1;
>   }
>
> -static void root_add_used(struct btrfs_root *root, u32 size)
> +static void root_add_used_bytes(struct btrfs_root *root)
>   {
>   	spin_lock(&root->accounting_lock);
>   	btrfs_set_root_used(&root->root_item,
> -			    btrfs_root_used(&root->root_item) + size);
> +		btrfs_root_used(&root->root_item) + root->fs_info->nodesize);
>   	spin_unlock(&root->accounting_lock);
>   }
>
> -static void root_sub_used(struct btrfs_root *root, u32 size)
> +static void root_sub_used_bytes(struct btrfs_root *root)
>   {
>   	spin_lock(&root->accounting_lock);
>   	btrfs_set_root_used(&root->root_item,
> -			    btrfs_root_used(&root->root_item) - size);
> +		btrfs_root_used(&root->root_item) - root->fs_info->nodesize);
>   	spin_unlock(&root->accounting_lock);
>   }
>
> @@ -1075,7 +1075,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
>   		/* once for the path */
>   		free_extent_buffer(mid);
>
> -		root_sub_used(root, mid->len);
> +		root_sub_used_bytes(root);
>   		btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
>   		/* once for the root ptr */
>   		free_extent_buffer_stale(mid);
> @@ -1145,7 +1145,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
>   				right = NULL;
>   				goto out;
>   			}
> -			root_sub_used(root, right->len);
> +			root_sub_used_bytes(root);
>   			btrfs_free_tree_block(trans, btrfs_root_id(root), right,
>   					      0, 1);
>   			free_extent_buffer_stale(right);
> @@ -1203,7 +1203,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
>   			mid = NULL;
>   			goto out;
>   		}
> -		root_sub_used(root, mid->len);
> +		root_sub_used_bytes(root);
>   		btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
>   		free_extent_buffer_stale(mid);
>   		mid = NULL;
> @@ -2937,7 +2937,6 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
>   			   struct btrfs_root *root,
>   			   struct btrfs_path *path, int level)
>   {
> -	struct btrfs_fs_info *fs_info = root->fs_info;
>   	u64 lower_gen;
>   	struct extent_buffer *lower;
>   	struct extent_buffer *c;
> @@ -2960,7 +2959,7 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
>   	if (IS_ERR(c))
>   		return PTR_ERR(c);
>
> -	root_add_used(root, fs_info->nodesize);
> +	root_add_used_bytes(root);
>
>   	btrfs_set_header_nritems(c, 1);
>   	btrfs_set_node_key(c, &lower_key, 0);
> @@ -3104,7 +3103,7 @@ static noinline int split_node(struct btrfs_trans_handle *trans,
>   	if (IS_ERR(split))
>   		return PTR_ERR(split);
>
> -	root_add_used(root, fs_info->nodesize);
> +	root_add_used_bytes(root);
>   	ASSERT(btrfs_header_level(c) == level);
>
>   	ret = btrfs_tree_mod_log_eb_copy(split, c, 0, mid, c_nritems - mid);
> @@ -3857,7 +3856,7 @@ static noinline int split_leaf(struct btrfs_trans_handle *trans,
>   	if (IS_ERR(right))
>   		return PTR_ERR(right);
>
> -	root_add_used(root, fs_info->nodesize);
> +	root_add_used_bytes(root);
>
>   	if (split == 0) {
>   		if (mid <= slot) {
> @@ -4530,7 +4529,7 @@ static noinline int btrfs_del_leaf(struct btrfs_trans_handle *trans,
>   	 */
>   	btrfs_unlock_up_safe(path, 0);
>
> -	root_sub_used(root, leaf->len);
> +	root_sub_used_bytes(root);
>
>   	atomic_inc(&leaf->refs);
>   	btrfs_free_tree_block(trans, btrfs_root_id(root), leaf, 0, 1);

[PATCH 07/10] btrfs: reduce size of prelim_ref::level

[David Sterba]

The values of level are bounded and fit into a byte so let's use it for
the structure to reduce size from 88 to 80 bytes on a release build,
which increases number of objects in the default 8K slab from 93 to 102.

struct prelim_ref {
        struct rb_node             rbnode __attribute__((__aligned__(8))); /*     0    24 */
        u64                        root_id;              /*    24     8 */
        struct btrfs_key           key_for_search;       /*    32    17 */
        u8                         level;                /*    49     1 */

        /* XXX 2 bytes hole, try to pack */

        int                        count;                /*    52     4 */
        struct extent_inode_elem * inode_list;           /*    56     8 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        u64                        parent;               /*    64     8 */
        u64                        wanted_disk_byte;     /*    72     8 */

        /* size: 80, cachelines: 2, members: 8 */
        /* sum members: 78, holes: 1, sum holes: 2 */
        /* forced alignments: 1 */
        /* last cacheline: 16 bytes */
} __attribute__((__aligned__(8)));

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
index 1616e3e3f1e4..79742935399f 100644
--- a/fs/btrfs/backref.h
+++ b/fs/btrfs/backref.h
@@ -247,7 +247,7 @@ struct prelim_ref {
 	struct rb_node rbnode;
 	u64 root_id;
 	struct btrfs_key key_for_search;
-	int level;
+	u8 level;
 	int count;
 	struct extent_inode_elem *inode_list;
 	u64 parent;
-- 
2.41.0

Re: [PATCH 07/10] btrfs: reduce size of prelim_ref::level

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> The values of level are bounded and fit into a byte so let's use it for
> the structure to reduce size from 88 to 80 bytes on a release build,
> which increases number of objects in the default 8K slab from 93 to 102.
>
> struct prelim_ref {
>          struct rb_node             rbnode __attribute__((__aligned__(8))); /*     0    24 */
>          u64                        root_id;              /*    24     8 */
>          struct btrfs_key           key_for_search;       /*    32    17 */
>          u8                         level;                /*    49     1 */
>
>          /* XXX 2 bytes hole, try to pack */
>
>          int                        count;                /*    52     4 */
>          struct extent_inode_elem * inode_list;           /*    56     8 */
>          /* --- cacheline 1 boundary (64 bytes) --- */
>          u64                        parent;               /*    64     8 */
>          u64                        wanted_disk_byte;     /*    72     8 */
>
>          /* size: 80, cachelines: 2, members: 8 */
>          /* sum members: 78, holes: 1, sum holes: 2 */
>          /* forced alignments: 1 */
>          /* last cacheline: 16 bytes */
> } __attribute__((__aligned__(8)));
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/backref.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
> index 1616e3e3f1e4..79742935399f 100644
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -247,7 +247,7 @@ struct prelim_ref {
>   	struct rb_node rbnode;
>   	u64 root_id;
>   	struct btrfs_key key_for_search;
> -	int level;
> +	u8 level;
>   	int count;
>   	struct extent_inode_elem *inode_list;
>   	u64 parent;

[PATCH 08/10] btrfs: reduce size and reorder compression members in struct btrfs_inode

[David Sterba]

Currently the compression type values are bounded and fit to an u8, we
can pack the btrfs_inode a bit by reordering them to the space created
by the location key. This reduces size from 1112 to 1104.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/btrfs_inode.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index b675dc09845d..2e1c0f68d704 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -81,6 +81,16 @@ struct btrfs_inode {
 	 */
 	struct btrfs_key location;
 
+	/*
+	 * Cached values of inode properties
+	 */
+	u8 prop_compress;		/* per-file compression algorithm */
+	/*
+	 * Force compression on the file using the defrag ioctl, could be
+	 * different from prop_compress and takes precedence if set
+	 */
+	u8 defrag_compress;
+
 	/*
 	 * Lock for counters and all fields used to determine if the inode is in
 	 * the log or not (last_trans, last_sub_trans, last_log_commit,
@@ -235,16 +245,6 @@ struct btrfs_inode {
 
 	struct btrfs_block_rsv block_rsv;
 
-	/*
-	 * Cached values of inode properties
-	 */
-	unsigned prop_compress;		/* per-file compression algorithm */
-	/*
-	 * Force compression on the file using the defrag ioctl, could be
-	 * different from prop_compress and takes precedence if set
-	 */
-	unsigned defrag_compress;
-
 	struct btrfs_delayed_node *delayed_node;
 
 	/* File creation time. */
-- 
2.41.0

Re: [PATCH 08/10] btrfs: reduce size and reorder compression members in struct btrfs_inode

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> Currently the compression type values are bounded and fit to an u8, we
> can pack the btrfs_inode a bit by reordering them to the space created
> by the location key. This reduces size from 1112 to 1104.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/btrfs_inode.h | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index b675dc09845d..2e1c0f68d704 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -81,6 +81,16 @@ struct btrfs_inode {
>   	 */
>   	struct btrfs_key location;
>
> +	/*
> +	 * Cached values of inode properties
> +	 */
> +	u8 prop_compress;		/* per-file compression algorithm */
> +	/*
> +	 * Force compression on the file using the defrag ioctl, could be
> +	 * different from prop_compress and takes precedence if set
> +	 */
> +	u8 defrag_compress;
> +
>   	/*
>   	 * Lock for counters and all fields used to determine if the inode is in
>   	 * the log or not (last_trans, last_sub_trans, last_log_commit,
> @@ -235,16 +245,6 @@ struct btrfs_inode {
>
>   	struct btrfs_block_rsv block_rsv;
>
> -	/*
> -	 * Cached values of inode properties
> -	 */
> -	unsigned prop_compress;		/* per-file compression algorithm */
> -	/*
> -	 * Force compression on the file using the defrag ioctl, could be
> -	 * different from prop_compress and takes precedence if set
> -	 */
> -	unsigned defrag_compress;
> -
>   	struct btrfs_delayed_node *delayed_node;
>
>   	/* File creation time. */

[PATCH 09/10] btrfs: reduce size of struct btrfs_ref

[David Sterba]

We can reduce two members' size that in turn reduce size of struct
btrfs_ref from 64 to 56 bytes. As the structure is often used as a local
variable several functions reduce their stack usage.

- make enum btrfs_ref_type packed, there are only 4 values

- switch action and its values to a packed enum

Final structure layout:

struct btrfs_ref {
        enum btrfs_ref_type        type;                 /*     0     1 */
        enum btrfs_delayed_ref_action action;            /*     1     1 */
        bool                       skip_qgroup;          /*     2     1 */

        /* XXX 5 bytes hole, try to pack */

        u64                        bytenr;               /*     8     8 */
        u64                        len;                  /*    16     8 */
        u64                        parent;               /*    24     8 */
        union {
                struct btrfs_data_ref data_ref;          /*    32    24 */
                struct btrfs_tree_ref tree_ref;          /*    32    16 */
        };                                               /*    32    24 */

        /* size: 56, cachelines: 1, members: 7 */
        /* sum members: 51, holes: 1, sum holes: 5 */
        /* last cacheline: 56 bytes */
};

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/delayed-ref.h | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index b8e14b0ba5f1..224278d4516f 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -9,10 +9,16 @@
 #include <linux/refcount.h>
 
 /* these are the possible values of struct btrfs_delayed_ref_node->action */
-#define BTRFS_ADD_DELAYED_REF    1 /* add one backref to the tree */
-#define BTRFS_DROP_DELAYED_REF   2 /* delete one backref from the tree */
-#define BTRFS_ADD_DELAYED_EXTENT 3 /* record a full extent allocation */
-#define BTRFS_UPDATE_DELAYED_HEAD 4 /* not changing ref count on head ref */
+enum btrfs_delayed_ref_action {
+	/* Add one backref to the tree */
+	BTRFS_ADD_DELAYED_REF = 1,
+	/* Delete one backref from the tree */
+	BTRFS_DROP_DELAYED_REF,
+	/* Record a full extent allocation */
+	BTRFS_ADD_DELAYED_EXTENT,
+	/* Not changing ref count on head ref */
+	BTRFS_UPDATE_DELAYED_HEAD,
+} __packed;
 
 struct btrfs_delayed_ref_node {
 	struct rb_node ref_node;
@@ -183,7 +189,7 @@ enum btrfs_ref_type {
 	BTRFS_REF_DATA,
 	BTRFS_REF_METADATA,
 	BTRFS_REF_LAST,
-};
+} __packed;
 
 struct btrfs_data_ref {
 	/* For EXTENT_DATA_REF */
@@ -223,7 +229,7 @@ struct btrfs_tree_ref {
 
 struct btrfs_ref {
 	enum btrfs_ref_type type;
-	int action;
+	enum btrfs_delayed_ref_action action;
 
 	/*
 	 * Whether this extent should go through qgroup record.
-- 
2.41.0

Re: [PATCH 09/10] btrfs: reduce size of struct btrfs_ref

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> We can reduce two members' size that in turn reduce size of struct
> btrfs_ref from 64 to 56 bytes. As the structure is often used as a local
> variable several functions reduce their stack usage.
>
> - make enum btrfs_ref_type packed, there are only 4 values
>
> - switch action and its values to a packed enum
>
> Final structure layout:
>
> struct btrfs_ref {
>          enum btrfs_ref_type        type;                 /*     0     1 */
>          enum btrfs_delayed_ref_action action;            /*     1     1 */

Considering both type and action have only 4 values, we can further pack
them into a single u8.

Although this means we can not directly use enum type, but u8 type instead.

Thanks,
Qu

>          bool                       skip_qgroup;          /*     2     1 */
>
>          /* XXX 5 bytes hole, try to pack */
>
>          u64                        bytenr;               /*     8     8 */
>          u64                        len;                  /*    16     8 */
>          u64                        parent;               /*    24     8 */
>          union {
>                  struct btrfs_data_ref data_ref;          /*    32    24 */
>                  struct btrfs_tree_ref tree_ref;          /*    32    16 */
>          };                                               /*    32    24 */
>
>          /* size: 56, cachelines: 1, members: 7 */
>          /* sum members: 51, holes: 1, sum holes: 5 */
>          /* last cacheline: 56 bytes */
> };
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/delayed-ref.h | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
> index b8e14b0ba5f1..224278d4516f 100644
> --- a/fs/btrfs/delayed-ref.h
> +++ b/fs/btrfs/delayed-ref.h
> @@ -9,10 +9,16 @@
>   #include <linux/refcount.h>
>
>   /* these are the possible values of struct btrfs_delayed_ref_node->action */
> -#define BTRFS_ADD_DELAYED_REF    1 /* add one backref to the tree */
> -#define BTRFS_DROP_DELAYED_REF   2 /* delete one backref from the tree */
> -#define BTRFS_ADD_DELAYED_EXTENT 3 /* record a full extent allocation */
> -#define BTRFS_UPDATE_DELAYED_HEAD 4 /* not changing ref count on head ref */
> +enum btrfs_delayed_ref_action {
> +	/* Add one backref to the tree */
> +	BTRFS_ADD_DELAYED_REF = 1,
> +	/* Delete one backref from the tree */
> +	BTRFS_DROP_DELAYED_REF,
> +	/* Record a full extent allocation */
> +	BTRFS_ADD_DELAYED_EXTENT,
> +	/* Not changing ref count on head ref */
> +	BTRFS_UPDATE_DELAYED_HEAD,
> +} __packed;
>
>   struct btrfs_delayed_ref_node {
>   	struct rb_node ref_node;
> @@ -183,7 +189,7 @@ enum btrfs_ref_type {
>   	BTRFS_REF_DATA,
>   	BTRFS_REF_METADATA,
>   	BTRFS_REF_LAST,
> -};
> +} __packed;
>
>   struct btrfs_data_ref {
>   	/* For EXTENT_DATA_REF */
> @@ -223,7 +229,7 @@ struct btrfs_tree_ref {
>
>   struct btrfs_ref {
>   	enum btrfs_ref_type type;
> -	int action;
> +	enum btrfs_delayed_ref_action action;
>
>   	/*
>   	 * Whether this extent should go through qgroup record.

Re: [PATCH 09/10] btrfs: reduce size of struct btrfs_ref

[David Sterba]

On Fri, Sep 08, 2023 at 08:06:04AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/9/8 07:09, David Sterba wrote:
> > We can reduce two members' size that in turn reduce size of struct
> > btrfs_ref from 64 to 56 bytes. As the structure is often used as a local
> > variable several functions reduce their stack usage.
> >
> > - make enum btrfs_ref_type packed, there are only 4 values
> >
> > - switch action and its values to a packed enum
> >
> > Final structure layout:
> >
> > struct btrfs_ref {
> >          enum btrfs_ref_type        type;                 /*     0     1 */
> >          enum btrfs_delayed_ref_action action;            /*     1     1 */
> 
> Considering both type and action have only 4 values, we can further pack
> them into a single u8.
> 
> Although this means we can not directly use enum type, but u8 type instead.

I consider byte as good enough for enums or bounded types, unless
there's a really good reason to do some ultra packing like in case we
could drop a gap of like 5-7 bytes due to alignment. In case of
btrfs_ref it does not help. With explict bit width this does not reduce
the size (still 56 bytes) and only increase code size:

 struct btrfs_ref {
-       enum btrfs_ref_type        type;                 /*     0     1 */
-       enum btrfs_delayed_ref_action action;            /*     1     1 */
-       bool                       skip_qgroup;          /*     2     1 */
+       enum btrfs_ref_type        type:2;               /*     0: 0  1 */
+       enum btrfs_delayed_ref_action action:3;          /*     0: 2  1 */
+       bool                       skip_qgroup:1;        /*     0: 5  1 */
 
-       /* XXX 5 bytes hole, try to pack */
+       /* XXX 2 bits hole, try to pack */
+       /* XXX 7 bytes hole, try to pack */
 
        u64                        bytenr;               /*     8     8 */
        u64                        len;                  /*    16     8 */
@@ -3116,7 +3117,8 @@ struct btrfs_ref {
        };                                               /*    32    24 */
 
        /* size: 56, cachelines: 1, members: 7 */
-       /* sum members: 51, holes: 1, sum holes: 5 */
+       /* sum members: 48, holes: 1, sum holes: 7 */
+       /* sum bitfield members: 6 bits, bit holes: 1, sum bit holes: 2 bits */
        /* last cacheline: 56 bytes */
 };

Code increase:

   text    data     bss     dec     hex filename
1268477   29813   16088 1314378  140e4a pre/btrfs.ko
1268915   29813   16088 1314816  141000 post/btrfs.ko

DELTA: +438

which means the simple byte comparisons are turned into a few
instructions that first need to mask off the other values, eg.
from btrfs_inc_extent_ref():

test   %al,%al		->		mov    %eax,%edx
					and    $0x3,%edx

so it needs more registers and may prevent some optimizations.

[PATCH 10/10] btrfs: move extent_buffer::lock_owner to debug section

[David Sterba]

The lock_owner is used for a rare corruption case and we haven't seen
any reports in years. Move it to the debugging section of eb.  To close
the holes also move log_index so the final layout looks like:

struct extent_buffer {
        u64                        start;                /*     0     8 */
        long unsigned int          len;                  /*     8     8 */
        long unsigned int          bflags;               /*    16     8 */
        struct btrfs_fs_info *     fs_info;              /*    24     8 */
        spinlock_t                 refs_lock;            /*    32     4 */
        atomic_t                   refs;                 /*    36     4 */
        int                        read_mirror;          /*    40     4 */
        s8                         log_index;            /*    44     1 */

        /* XXX 3 bytes hole, try to pack */

        struct callback_head       callback_head __attribute__((__aligned__(8))); /*    48    16 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        struct rw_semaphore        lock;                 /*    64    40 */
        struct page *              pages[16];            /*   104   128 */

        /* size: 232, cachelines: 4, members: 11 */
        /* sum members: 229, holes: 1, sum holes: 3 */
        /* forced alignments: 1, forced holes: 1, sum forced holes: 3 */
        /* last cacheline: 40 bytes */
} __attribute__((__aligned__(8)));

This saves 8 bytes in total and still keeps the lock on a separate cacheline.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent-tree.c | 32 +++++++++++++++++++++++---------
 fs/btrfs/extent_io.h   |  4 ++--
 fs/btrfs/locking.c     | 15 ++++++++++++---
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3e46bb4cc957..6f6838226fe7 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4801,6 +4801,28 @@ int btrfs_alloc_logged_file_extent(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+#ifdef CONFIG_BTRFS_DEBUG
+/*
+ * Extra safety check in case the extent tree is corrupted and extent allocator
+ * chooses to use a tree block which is already used and locked.
+ */
+static bool check_eb_lock_owner(const struct extent_buffer *eb)
+{
+	if (eb->lock_owner == current->pid) {
+		btrfs_err_rl(eb->fs_info,
+"tree block %llu owner %llu already locked by pid=%d, extent tree corruption detected",
+			     eb->start, btrfs_header_owner(eb), current->pid);
+		return true;
+	}
+	return false;
+}
+#else
+static bool check_eb_lock_owner(struct extent_buffer *eb)
+{
+	return false;
+}
+#endif
+
 static struct extent_buffer *
 btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		      u64 bytenr, int level, u64 owner,
@@ -4814,15 +4836,7 @@ btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 	if (IS_ERR(buf))
 		return buf;
 
-	/*
-	 * Extra safety check in case the extent tree is corrupted and extent
-	 * allocator chooses to use a tree block which is already used and
-	 * locked.
-	 */
-	if (buf->lock_owner == current->pid) {
-		btrfs_err_rl(fs_info,
-"tree block %llu owner %llu already locked by pid=%d, extent tree corruption detected",
-			buf->start, btrfs_header_owner(buf), current->pid);
+	if (check_eb_lock_owner(buf)) {
 		free_extent_buffer(buf);
 		return ERR_PTR(-EUCLEAN);
 	}
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 68368ba99321..2171057a4477 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -80,16 +80,16 @@ struct extent_buffer {
 	spinlock_t refs_lock;
 	atomic_t refs;
 	int read_mirror;
-	struct rcu_head rcu_head;
-	pid_t lock_owner;
 	/* >= 0 if eb belongs to a log tree, -1 otherwise */
 	s8 log_index;
+	struct rcu_head rcu_head;
 
 	struct rw_semaphore lock;
 
 	struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
 #ifdef CONFIG_BTRFS_DEBUG
 	struct list_head leak_list;
+	pid_t lock_owner;
 #endif
 };
 
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index c3128cdf1177..6ac4fd8cc8dc 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -103,6 +103,15 @@ void btrfs_maybe_reset_lockdep_class(struct btrfs_root *root, struct extent_buff
 
 #endif
 
+#ifdef CONFIG_BTRFS_DEBUG
+static void btrfs_set_eb_lock_owner(struct extent_buffer *eb, pid_t owner)
+{
+	eb->lock_owner = owner;
+}
+#else
+static void btrfs_set_eb_lock_owner(struct extent_buffer *eb, pid_t owner) { }
+#endif
+
 /*
  * Extent buffer locking
  * =====================
@@ -165,7 +174,7 @@ int btrfs_try_tree_read_lock(struct extent_buffer *eb)
 int btrfs_try_tree_write_lock(struct extent_buffer *eb)
 {
 	if (down_write_trylock(&eb->lock)) {
-		eb->lock_owner = current->pid;
+		btrfs_set_eb_lock_owner(eb, current->pid);
 		trace_btrfs_try_tree_write_lock(eb);
 		return 1;
 	}
@@ -198,7 +207,7 @@ void __btrfs_tree_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest)
 		start_ns = ktime_get_ns();
 
 	down_write_nested(&eb->lock, nest);
-	eb->lock_owner = current->pid;
+	btrfs_set_eb_lock_owner(eb, current->pid);
 	trace_btrfs_tree_lock(eb, start_ns);
 }
 
@@ -213,7 +222,7 @@ void btrfs_tree_lock(struct extent_buffer *eb)
 void btrfs_tree_unlock(struct extent_buffer *eb)
 {
 	trace_btrfs_tree_unlock(eb);
-	eb->lock_owner = 0;
+	btrfs_set_eb_lock_owner(eb, 0);
 	up_write(&eb->lock);
 }
 
-- 
2.41.0

Re: [PATCH 10/10] btrfs: move extent_buffer::lock_owner to debug section

[Qu Wenruo]

On 2023/9/8 07:09, David Sterba wrote:
> The lock_owner is used for a rare corruption case and we haven't seen
> any reports in years. Move it to the debugging section of eb.  To close
> the holes also move log_index so the final layout looks like:

Just found some extra space we can squish out:

> struct extent_buffer {
>          u64                        start;                /*     0     8 */
>          long unsigned int          len;                  /*     8     8 */

u32 is definitely enough.
(u16 is not enough for 64K nodesize, unfortunately)

>          long unsigned int          bflags;               /*    16     8 */

For now we don't need 64bit for bflags, 32bit is enough, but
unfortunately that's what the existing test/set/clear_bit() requires...

>          struct btrfs_fs_info *     fs_info;              /*    24     8 */
>          spinlock_t                 refs_lock;            /*    32     4 */
>          atomic_t                   refs;                 /*    36     4 */
>          int                        read_mirror;          /*    40     4 */

We don't really need int for read_mirror, but that would be another
patch(set) to change them.

>          s8                         log_index;            /*    44     1 */
>
>          /* XXX 3 bytes hole, try to pack */
>
>          struct callback_head       callback_head __attribute__((__aligned__(8))); /*    48    16 */
>          /* --- cacheline 1 boundary (64 bytes) --- */
>          struct rw_semaphore        lock;                 /*    64    40 */
>          struct page *              pages[16];            /*   104   128 */
>
>          /* size: 232, cachelines: 4, members: 11 */
>          /* sum members: 229, holes: 1, sum holes: 3 */
>          /* forced alignments: 1, forced holes: 1, sum forced holes: 3 */
>          /* last cacheline: 40 bytes */
> } __attribute__((__aligned__(8)));
>
> This saves 8 bytes in total and still keeps the lock on a separate cacheline.
>
> Signed-off-by: David Sterba <dsterba@suse.com>

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

Thanks,
Qu

> ---
>   fs/btrfs/extent-tree.c | 32 +++++++++++++++++++++++---------
>   fs/btrfs/extent_io.h   |  4 ++--
>   fs/btrfs/locking.c     | 15 ++++++++++++---
>   3 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 3e46bb4cc957..6f6838226fe7 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4801,6 +4801,28 @@ int btrfs_alloc_logged_file_extent(struct btrfs_trans_handle *trans,
>   	return ret;
>   }
>
> +#ifdef CONFIG_BTRFS_DEBUG
> +/*
> + * Extra safety check in case the extent tree is corrupted and extent allocator
> + * chooses to use a tree block which is already used and locked.
> + */
> +static bool check_eb_lock_owner(const struct extent_buffer *eb)
> +{
> +	if (eb->lock_owner == current->pid) {
> +		btrfs_err_rl(eb->fs_info,
> +"tree block %llu owner %llu already locked by pid=%d, extent tree corruption detected",
> +			     eb->start, btrfs_header_owner(eb), current->pid);
> +		return true;
> +	}
> +	return false;
> +}
> +#else
> +static bool check_eb_lock_owner(struct extent_buffer *eb)
> +{
> +	return false;
> +}
> +#endif
> +
>   static struct extent_buffer *
>   btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>   		      u64 bytenr, int level, u64 owner,
> @@ -4814,15 +4836,7 @@ btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>   	if (IS_ERR(buf))
>   		return buf;
>
> -	/*
> -	 * Extra safety check in case the extent tree is corrupted and extent
> -	 * allocator chooses to use a tree block which is already used and
> -	 * locked.
> -	 */
> -	if (buf->lock_owner == current->pid) {
> -		btrfs_err_rl(fs_info,
> -"tree block %llu owner %llu already locked by pid=%d, extent tree corruption detected",
> -			buf->start, btrfs_header_owner(buf), current->pid);
> +	if (check_eb_lock_owner(buf)) {
>   		free_extent_buffer(buf);
>   		return ERR_PTR(-EUCLEAN);
>   	}
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 68368ba99321..2171057a4477 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -80,16 +80,16 @@ struct extent_buffer {
>   	spinlock_t refs_lock;
>   	atomic_t refs;
>   	int read_mirror;
> -	struct rcu_head rcu_head;
> -	pid_t lock_owner;
>   	/* >= 0 if eb belongs to a log tree, -1 otherwise */
>   	s8 log_index;
> +	struct rcu_head rcu_head;
>
>   	struct rw_semaphore lock;
>
>   	struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
>   #ifdef CONFIG_BTRFS_DEBUG
>   	struct list_head leak_list;
> +	pid_t lock_owner;
>   #endif
>   };
>
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index c3128cdf1177..6ac4fd8cc8dc 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -103,6 +103,15 @@ void btrfs_maybe_reset_lockdep_class(struct btrfs_root *root, struct extent_buff
>
>   #endif
>
> +#ifdef CONFIG_BTRFS_DEBUG
> +static void btrfs_set_eb_lock_owner(struct extent_buffer *eb, pid_t owner)
> +{
> +	eb->lock_owner = owner;
> +}
> +#else
> +static void btrfs_set_eb_lock_owner(struct extent_buffer *eb, pid_t owner) { }
> +#endif
> +
>   /*
>    * Extent buffer locking
>    * =====================
> @@ -165,7 +174,7 @@ int btrfs_try_tree_read_lock(struct extent_buffer *eb)
>   int btrfs_try_tree_write_lock(struct extent_buffer *eb)
>   {
>   	if (down_write_trylock(&eb->lock)) {
> -		eb->lock_owner = current->pid;
> +		btrfs_set_eb_lock_owner(eb, current->pid);
>   		trace_btrfs_try_tree_write_lock(eb);
>   		return 1;
>   	}
> @@ -198,7 +207,7 @@ void __btrfs_tree_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest)
>   		start_ns = ktime_get_ns();
>
>   	down_write_nested(&eb->lock, nest);
> -	eb->lock_owner = current->pid;
> +	btrfs_set_eb_lock_owner(eb, current->pid);
>   	trace_btrfs_tree_lock(eb, start_ns);
>   }
>
> @@ -213,7 +222,7 @@ void btrfs_tree_lock(struct extent_buffer *eb)
>   void btrfs_tree_unlock(struct extent_buffer *eb)
>   {
>   	trace_btrfs_tree_unlock(eb);
> -	eb->lock_owner = 0;
> +	btrfs_set_eb_lock_owner(eb, 0);
>   	up_write(&eb->lock);
>   }
>

Re: [PATCH 10/10] btrfs: move extent_buffer::lock_owner to debug section

[David Sterba]

On Fri, Sep 08, 2023 at 08:11:43AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/9/8 07:09, David Sterba wrote:
> > The lock_owner is used for a rare corruption case and we haven't seen
> > any reports in years. Move it to the debugging section of eb.  To close
> > the holes also move log_index so the final layout looks like:
> 
> Just found some extra space we can squish out:

Yeah I have a few more patches to reduce extent buffer size.

> > struct extent_buffer {
> >          u64                        start;                /*     0     8 */
> >          long unsigned int          len;                  /*     8     8 */
> 
> u32 is definitely enough.
> (u16 is not enough for 64K nodesize, unfortunately)

One idea is to store the bit shift and then calculate the length on each
use but then we can simply not store the length in extent buffer at all
because it's always fs_info->nodesize. In some functions it can be
directly replaced by that, elsewhere it's needed to do
eb->fs_info->nodesize. One more pointer dereference but from likely a
cached memory, gaining 8 bytes.

> 
> >          long unsigned int          bflags;               /*    16     8 */
> 
> For now we don't need 64bit for bflags, 32bit is enough, but
> unfortunately that's what the existing test/set/clear_bit() requires...

Indeed, and it's not just here, in the inode or some other structures
taht store a few flags an u32 would suffice. I have a prototype to add
atomic bit manipulations for a u32 type, but this requires manually
written assembly so for now it's just x86. This could be useful outside
of btrfs so I'd need to make it a proper API and get some feedback from
wider audience.

> >          struct btrfs_fs_info *     fs_info;              /*    24     8 */
> >          spinlock_t                 refs_lock;            /*    32     4 */
> >          atomic_t                   refs;                 /*    36     4 */
> >          int                        read_mirror;          /*    40     4 */
> 
> We don't really need int for read_mirror, but that would be another
> patch(set) to change them.

Yeah that's another potential space saved, I tried to track down all the
use of mirror values but it's passed around to bios and back.

> >          s8                         log_index;            /*    44     1 */

And log_index takes 3 values, this can be encoded into the flags, also a
prototype but the code becomes ugly.

Re: [PATCH 10/10] btrfs: move extent_buffer::lock_owner to debug section

[Qu Wenruo]

On 2023/9/8 08:56, David Sterba wrote:
> On Fri, Sep 08, 2023 at 08:11:43AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2023/9/8 07:09, David Sterba wrote:
>>> The lock_owner is used for a rare corruption case and we haven't seen
>>> any reports in years. Move it to the debugging section of eb.  To close
>>> the holes also move log_index so the final layout looks like:
>>
>> Just found some extra space we can squish out:
>
> Yeah I have a few more patches to reduce extent buffer size.
>
>>> struct extent_buffer {
>>>           u64                        start;                /*     0     8 */
>>>           long unsigned int          len;                  /*     8     8 */
>>
>> u32 is definitely enough.
>> (u16 is not enough for 64K nodesize, unfortunately)
>
> One idea is to store the bit shift and then calculate the length on each
> use but then we can simply not store the length in extent buffer at all
> because it's always fs_info->nodesize. In some functions it can be
> directly replaced by that, elsewhere it's needed to do
> eb->fs_info->nodesize. One more pointer dereference but from likely a
> cached memory, gaining 8 bytes.

Oh, that's way better. We used to use dummy extent buffer with fixed
size (4K) for sys chunk array read, but since commit e959d3c1df3a
("btrfs: use dummy extent buffer for super block sys chunk array read")
we just use nodesize and leave the extra space untouched.

But it may be better to dig deeper, as I'm not 100% sure if every
(including dummy ones) extent buffer goes nodesize.

>
>>
>>>           long unsigned int          bflags;               /*    16     8 */
>>
>> For now we don't need 64bit for bflags, 32bit is enough, but
>> unfortunately that's what the existing test/set/clear_bit() requires...
>
> Indeed, and it's not just here, in the inode or some other structures
> taht store a few flags an u32 would suffice. I have a prototype to add
> atomic bit manipulations for a u32 type, but this requires manually
> written assembly so for now it's just x86. This could be useful outside
> of btrfs so I'd need to make it a proper API and get some feedback from
> wider audience.
>
>>>           struct btrfs_fs_info *     fs_info;              /*    24     8 */
>>>           spinlock_t                 refs_lock;            /*    32     4 */
>>>           atomic_t                   refs;                 /*    36     4 */
>>>           int                        read_mirror;          /*    40     4 */
>>
>> We don't really need int for read_mirror, but that would be another
>> patch(set) to change them.
>
> Yeah that's another potential space saved, I tried to track down all the
> use of mirror values but it's passed around to bios and back.

My current plan is to go s8, which still gives us 127 mirrors.
Which should be enough for RAID6 rebuilds.

Although we may need extra attention to prevent incorrect width
truncation/padding, as we're going to support minus number for
scrub_logical ioctl to grab P/Q stripes directly.

Thanks,
Qu
>
>>>           s8                         log_index;            /*    44     1 */
>
> And log_index takes 3 values, this can be encoded into the flags, also a
> prototype but the code becomes ugly.