[PATCH 0/3] btrfs: some code cleanup and documentation

[PATCH 0/3] btrfs: some code cleanup and documentation

[Edmund Nadolski]

This series adds a bit of code cleanup and some documentation in the
form of comments. No operational change.

Edmund Nadolski (3):
  btrfs: btrfs_inode_log_parent should use defined inode_only values.
  btrfs: update some code documentation
  btrfs: remove dead code from btrfs_get_extent

 fs/btrfs/ctree.c                | 31 +++++++++++++++-------
 fs/btrfs/ctree.h                | 28 +++++++++++++++-----
 fs/btrfs/extent_map.h           | 58 ++++++++++++++++++++++++++++-------------
 fs/btrfs/file-item.c            |  3 +++
 fs/btrfs/inode.c                | 43 ++++++++++++------------------
 fs/btrfs/tree-log.c             |  7 +++--
 include/uapi/linux/btrfs_tree.h |  1 +
 7 files changed, 108 insertions(+), 63 deletions(-)

-- 
2.10.2

[PATCH 1/3] btrfs: btrfs_inode_log_parent should use defined inode_only values.

[Edmund Nadolski]

Replace hardcoded numeric argument values for inode_only with the
constants defined for that use.

Signed-off-by: Edmund Nadolski <enadolski@suse.com>
---
 fs/btrfs/tree-log.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 1f79405..95cff91 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -5403,11 +5403,10 @@ static int btrfs_log_inode_parent(struct btrfs_trans_handle *trans,
 				  struct dentry *parent,
 				  const loff_t start,
 				  const loff_t end,
-				  int exists_only,
+				  int inode_only,
 				  struct btrfs_log_ctx *ctx)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	int inode_only = exists_only ? LOG_INODE_EXISTS : LOG_INODE_ALL;
 	struct super_block *sb;
 	struct dentry *old_parent = NULL;
 	int ret = 0;
@@ -5573,7 +5572,7 @@ int btrfs_log_dentry_safe(struct btrfs_trans_handle *trans,
 	int ret;
 
 	ret = btrfs_log_inode_parent(trans, root, BTRFS_I(d_inode(dentry)),
-			parent, start, end, 0, ctx);
+			parent, start, end, LOG_INODE_ALL, ctx);
 	dput(parent);
 
 	return ret;
@@ -5836,6 +5835,6 @@ int btrfs_log_new_name(struct btrfs_trans_handle *trans,
 		return 0;
 
 	return btrfs_log_inode_parent(trans, root, inode, parent, 0,
-				      LLONG_MAX, 1, NULL);
+				      LLONG_MAX, LOG_INODE_EXISTS, NULL);
 }
 
-- 
2.10.2

Re: [PATCH 1/3] btrfs: btrfs_inode_log_parent should use defined inode_only values.

[Nikolay Borisov]

On 20.11.2017 22:24, Edmund Nadolski wrote:
> Replace hardcoded numeric argument values for inode_only with the
> constants defined for that use.
> 
> Signed-off-by: Edmund Nadolski <enadolski@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/tree-log.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 1f79405..95cff91 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -5403,11 +5403,10 @@ static int btrfs_log_inode_parent(struct btrfs_trans_handle *trans,
>  				  struct dentry *parent,
>  				  const loff_t start,
>  				  const loff_t end,
> -				  int exists_only,
> +				  int inode_only,
>  				  struct btrfs_log_ctx *ctx)
>  {
>  	struct btrfs_fs_info *fs_info = root->fs_info;
> -	int inode_only = exists_only ? LOG_INODE_EXISTS : LOG_INODE_ALL;
>  	struct super_block *sb;
>  	struct dentry *old_parent = NULL;
>  	int ret = 0;
> @@ -5573,7 +5572,7 @@ int btrfs_log_dentry_safe(struct btrfs_trans_handle *trans,
>  	int ret;
>  
>  	ret = btrfs_log_inode_parent(trans, root, BTRFS_I(d_inode(dentry)),
> -			parent, start, end, 0, ctx);
> +			parent, start, end, LOG_INODE_ALL, ctx);
>  	dput(parent);
>  
>  	return ret;
> @@ -5836,6 +5835,6 @@ int btrfs_log_new_name(struct btrfs_trans_handle *trans,
>  		return 0;
>  
>  	return btrfs_log_inode_parent(trans, root, inode, parent, 0,
> -				      LLONG_MAX, 1, NULL);
> +				      LLONG_MAX, LOG_INODE_EXISTS, NULL);
>  }
>  
> 

[PATCH 2/3] btrfs: update some code documentation

[Edmund Nadolski]

Improve code documentation by adding/expanding comments in
several places.

Signed-off-by: Edmund Nadolski <enadolski@suse.com>
---
 fs/btrfs/ctree.c                | 31 +++++++++++++++-------
 fs/btrfs/ctree.h                | 28 +++++++++++++++-----
 fs/btrfs/extent_map.h           | 58 ++++++++++++++++++++++++++++-------------
 fs/btrfs/file-item.c            |  3 +++
 fs/btrfs/inode.c                | 21 +++++++++++----
 include/uapi/linux/btrfs_tree.h |  1 +
 6 files changed, 104 insertions(+), 38 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 531e0a8..9331d02 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2654,17 +2654,30 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
 }
 
 /*
- * look for key in the tree.  path is filled in with nodes along the way
- * if key is found, we return zero and you can find the item in the leaf
- * level of the path (level 0)
+ * Search a btree for a specific key. Optionally update the btree for
+ * item insertion or removal.
  *
- * If the key isn't found, the path points to the slot where it should
- * be inserted, and 1 is returned.  If there are other errors during the
- * search a negative error number is returned.
+ *   @trans - Transaction handle (NULL for none, requires @cow == 0).
+ *   @root - The btree to be searched.
+ *   @key - key for search.
+ *   @p - Points to a btrfs_path to be populated with btree node and index
+ *      values encountered during traversal. (See also struct btrfs_path.)
+ *   @ins_len - byte length of item as follows:
+ *      >0: byte length of item for insertion.  Btree nodes/leaves are
+ *          split as required.
+ *      <0: byte length of item for deletion.  Btree nodes/leaves are
+ *          merged as required.
+ *       0: Do not modify the btree (search only).
+ *   @cow - 0 to nocow, or !0 to cow for btree update.
  *
- * if ins_len > 0, nodes and leaves will be split as we walk down the
- * tree.  if ins_len < 0, nodes will be merged as we walk down the tree (if
- * possible)
+ * Return values:
+ *   0: @key was found and @p was updated to indicate the leaf and item
+ *       slot where the item may be accessed.
+ *   1: @key was not found and @p was updated to indicate the leaf and
+ *       item slot where the item may be inserted.
+ *  <0: an error occurred.
+ *
+ * Note, insertion/removal updates will re-balance the btree as needed.
  */
 int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		      const struct btrfs_key *key, struct btrfs_path *p,
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2154b5a..e980caa 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -318,20 +318,36 @@ struct btrfs_node {
 } __attribute__ ((__packed__));
 
 /*
- * btrfs_paths remember the path taken from the root down to the leaf.
- * level 0 is always the leaf, and nodes[1...BTRFS_MAX_LEVEL] will point
- * to any other levels that are present.
- *
- * The slots array records the index of the item or block pointer
- * used while walking the tree.
+ * Used with btrfs_search_slot() to record a btree search path traversed from a
+ * root down to a leaf.
  */
 enum { READA_NONE = 0, READA_BACK, READA_FORWARD };
 struct btrfs_path {
+	/*
+	 * The nodes[] and slots[] arrays are indexed according to btree
+	 * levels. Index 0 corresponds to the lowest (leaf) level. Increasing
+	 * indexes correspond to interior btree nodes at subsequently higher
+	 * levels.
+	 *
+	 * nodes[0] always points to a leaf. nodes[1] points to a btree node
+	 * which contains a block pointer referencing that leaf. For higher
+	 * indexes, node[n] points to a btree node which contains a block
+	 * pointer referencing node[n-1].
+	 */
 	struct extent_buffer *nodes[BTRFS_MAX_LEVEL];
+
+	/*
+	 * The slots[0] value identifies an item index in the leaf at nodes[0].
+	 * Each slots[n] value identifies a block pointer index in the
+	 * corresponding tree node at nodes[n].
+	 */
 	int slots[BTRFS_MAX_LEVEL];
+
 	/* if there is real range locking, this locks field will change */
 	u8 locks[BTRFS_MAX_LEVEL];
+
 	u8 reada;
+
 	/* keep some upper locks as we walk down */
 	u8 lowest_level;
 
diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
index 64365bb..34ea85c3 100644
--- a/fs/btrfs/extent_map.h
+++ b/fs/btrfs/extent_map.h
@@ -5,35 +5,57 @@
 #include <linux/rbtree.h>
 #include <linux/refcount.h>
 
-#define EXTENT_MAP_LAST_BYTE ((u64)-4)
-#define EXTENT_MAP_HOLE ((u64)-3)
-#define EXTENT_MAP_INLINE ((u64)-2)
-#define EXTENT_MAP_DELALLOC ((u64)-1)
-
-/* bits for the flags field */
-#define EXTENT_FLAG_PINNED 0 /* this entry not yet on disk, don't free it */
-#define EXTENT_FLAG_COMPRESSED 1
-#define EXTENT_FLAG_VACANCY 2 /* no file extent item found */
-#define EXTENT_FLAG_PREALLOC 3 /* pre-allocated extent */
-#define EXTENT_FLAG_LOGGING 4 /* Logging this extent */
-#define EXTENT_FLAG_FILLING 5 /* Filling in a preallocated extent */
-#define EXTENT_FLAG_FS_MAPPING 6 /* filesystem extent mapping type */
+/*
+ * Reserve some special-case values for the extent_map .start, .block_start,
+ *: and .orig_start fields.
+ */
+#define EXTENT_MAP_LAST_BYTE	((u64)-4)	/* Lowest reserved value */
+#define EXTENT_MAP_HOLE		((u64)-3)	/* Unwritten extent */
+#define EXTENT_MAP_INLINE	((u64)-2)	/* Inlined file data */
+#define EXTENT_MAP_DELALLOC	((u64)-1)	/* Delayed block allocation */
 
+/*
+ * Bit flags for the extent_map .flags field.
+ */
+#define EXTENT_FLAG_PINNED	0 /* entry not yet on disk, don't free it */
+#define EXTENT_FLAG_COMPRESSED	1
+#define EXTENT_FLAG_VACANCY	2 /* no file extent item found */
+#define EXTENT_FLAG_PREALLOC	3 /* pre-allocated extent */
+#define EXTENT_FLAG_LOGGING	4 /* Logging this extent */
+#define EXTENT_FLAG_FILLING	5 /* Filling in a preallocated extent */
+#define EXTENT_FLAG_FS_MAPPING	6 /* filesystem extent mapping type */
+
+/*
+ * In-memory representation of a file extent (regular/prealloc/inline).
+ */
 struct extent_map {
 	struct rb_node rb_node;
 
 	/* all of these are in bytes */
-	u64 start;
-	u64 len;
+	u64 start;	/* logical byte offset in the file */
+	u64 len;	/* byte length of extent in the file */
 	u64 mod_start;
 	u64 mod_len;
 	u64 orig_start;
 	u64 orig_block_len;
+
+	/* max. bytes needed to hold the (uncompressed) extent in memory. */
 	u64 ram_bytes;
+
+	/*
+	 * For regular/prealloc, block_start is a logical address denoting the
+	 * start of the extent data, and block_len is the on-disk byte length
+	 * of the extent (which may be comressed). block_start may be
+	 * EXTENT_MAP_HOLE if the extent is unwritten.  For inline, block_start
+	 * is EXTENT_MAP_INLINE and block_len is (u64)-1.  See also
+	 * btrfs_extent_item_to_extent_map() and btrfs_get_extent().
+	 */
 	u64 block_start;
 	u64 block_len;
-	u64 generation;
-	unsigned long flags;
+
+	u64 generation;		/* transaction id */
+	unsigned long flags;	/* EXTENT_FLAG_* bit flags as above */
+
 	union {
 		struct block_device *bdev;
 
@@ -44,7 +66,7 @@ struct extent_map {
 		struct map_lookup *map_lookup;
 	};
 	refcount_t refs;
-	unsigned int compress_type;
+	unsigned int compress_type;	/* BTRFS_COMPRESS_* */
 	struct list_head list;
 };
 
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index fdcb410..5d63172 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -929,6 +929,9 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
 	goto out;
 }
 
+/*
+ * Populate an extent_map from the btrfs_file_extent_item contents.
+ */
 void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
 				     const struct btrfs_path *path,
 				     struct btrfs_file_extent_item *fi,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 72c7b38..b6d5d94 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6916,17 +6916,28 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 }
 
 /*
- * a bit scary, this does extent mapping from logical file offset to the disk.
- * the ugly parts come from merging extents from the disk with the in-ram
+ * btrfs_get_extent - find or create an extent_map for the (@start, @len) range.
+ *
+ *  @inode - inode containing the desired extent.
+ *  @page - page for inlined extent (NULL if none)
+ *  @page_offset - byte offset within @page
+ *  @start - logical byte offset in the file.
+ *  @len - byte length of the range.
+ *  @create - for inlined extents: 0 to update @page with extent data from
+ *            the item; 1 to bypass the update.
+ *
+ * A bit scary, this does extent mapping from logical file offset to the disk.
+ * The ugly parts come from merging extents from the disk with the in-ram
  * representation.  This gets more complex because of the data=ordered code,
  * where the in-ram extents might be locked pending data=ordered completion.
  *
  * This also copies inline extents directly into the page.
+ *
+ * Returns the extent_map, or error code.
  */
 struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
-		struct page *page,
-	    size_t pg_offset, u64 start, u64 len,
-		int create)
+				    struct page *page, size_t pg_offset,
+				    u64 start, u64 len, int create)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->vfs_inode.i_sb);
 	int ret;
diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
index 6d6e5da..693636a 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -730,6 +730,7 @@ struct btrfs_balance_item {
 	__le64 unused[4];
 } __attribute__ ((__packed__));
 
+/* Values for .type field in btrfs_file_extent_item */
 #define BTRFS_FILE_EXTENT_INLINE 0
 #define BTRFS_FILE_EXTENT_REG 1
 #define BTRFS_FILE_EXTENT_PREALLOC 2
-- 
2.10.2

Re: [PATCH 2/3] btrfs: update some code documentation

[Nikolay Borisov]

On 20.11.2017 22:24, Edmund Nadolski wrote:
> Improve code documentation by adding/expanding comments in
> several places.
> 
> Signed-off-by: Edmund Nadolski <enadolski@suse.com>
> ---
>  fs/btrfs/ctree.c                | 31 +++++++++++++++-------
>  fs/btrfs/ctree.h                | 28 +++++++++++++++-----
>  fs/btrfs/extent_map.h           | 58 ++++++++++++++++++++++++++++-------------
>  fs/btrfs/file-item.c            |  3 +++
>  fs/btrfs/inode.c                | 21 +++++++++++----
>  include/uapi/linux/btrfs_tree.h |  1 +
>  6 files changed, 104 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 531e0a8..9331d02 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2654,17 +2654,30 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
>  }
>  
>  /*
> - * look for key in the tree.  path is filled in with nodes along the way
> - * if key is found, we return zero and you can find the item in the leaf
> - * level of the path (level 0)
> + * Search a btree for a specific key. Optionally update the btree for
> + * item insertion or removal.
>   *
> - * If the key isn't found, the path points to the slot where it should
> - * be inserted, and 1 is returned.  If there are other errors during the
> - * search a negative error number is returned.
> + *   @trans - Transaction handle (NULL for none, requires @cow == 0).
> + *   @root - The btree to be searched.
> + *   @key - key for search.
> + *   @p - Points to a btrfs_path to be populated with btree node and index
> + *      values encountered during traversal. (See also struct btrfs_path.)
> + *   @ins_len - byte length of item as follows:
> + *      >0: byte length of item for insertion.  Btree nodes/leaves are
> + *          split as required.
> + *      <0: byte length of item for deletion.  Btree nodes/leaves are
> + *          merged as required.
> + *       0: Do not modify the btree (search only).
> + *   @cow - 0 to nocow, or !0 to cow for btree update.
>   *
> - * if ins_len > 0, nodes and leaves will be split as we walk down the
> - * tree.  if ins_len < 0, nodes will be merged as we walk down the tree (if
> - * possible)
> + * Return values:
> + *   0: @key was found and @p was updated to indicate the leaf and item
> + *       slot where the item may be accessed.
> + *   1: @key was not found and @p was updated to indicate the leaf and
> + *       item slot where the item may be inserted.
> + *  <0: an error occurred.
> + *
> + * Note, insertion/removal updates will re-balance the btree as needed.
>   */
>  int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>  		      const struct btrfs_key *key, struct btrfs_path *p,
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 2154b5a..e980caa 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -318,20 +318,36 @@ struct btrfs_node {
>  } __attribute__ ((__packed__));
>  
>  /*
> - * btrfs_paths remember the path taken from the root down to the leaf.
> - * level 0 is always the leaf, and nodes[1...BTRFS_MAX_LEVEL] will point
> - * to any other levels that are present.
> - *
> - * The slots array records the index of the item or block pointer
> - * used while walking the tree.
> + * Used with btrfs_search_slot() to record a btree search path traversed from a
> + * root down to a leaf.
>   */
>  enum { READA_NONE = 0, READA_BACK, READA_FORWARD };
>  struct btrfs_path {
> +	/*
> +	 * The nodes[] and slots[] arrays are indexed according to btree
> +	 * levels. Index 0 corresponds to the lowest (leaf) level. Increasing
> +	 * indexes correspond to interior btree nodes at subsequently higher
> +	 * levels.
> +	 *
> +	 * nodes[0] always points to a leaf. nodes[1] points to a btree node
> +	 * which contains a block pointer referencing that leaf. For higher
> +	 * indexes, node[n] points to a btree node which contains a block
> +	 * pointer referencing node[n-1].
> +	 */
>  	struct extent_buffer *nodes[BTRFS_MAX_LEVEL];
> +
> +	/*
> +	 * The slots[0] value identifies an item index in the leaf at nodes[0].
> +	 * Each slots[n] value identifies a block pointer index in the
> +	 * corresponding tree node at nodes[n].
> +	 */
>  	int slots[BTRFS_MAX_LEVEL];
> +
>  	/* if there is real range locking, this locks field will change */
>  	u8 locks[BTRFS_MAX_LEVEL];
> +
>  	u8 reada;
> +
>  	/* keep some upper locks as we walk down */
>  	u8 lowest_level;
>  
> diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
> index 64365bb..34ea85c3 100644
> --- a/fs/btrfs/extent_map.h
> +++ b/fs/btrfs/extent_map.h
> @@ -5,35 +5,57 @@
>  #include <linux/rbtree.h>
>  #include <linux/refcount.h>
>  
> -#define EXTENT_MAP_LAST_BYTE ((u64)-4)
> -#define EXTENT_MAP_HOLE ((u64)-3)
> -#define EXTENT_MAP_INLINE ((u64)-2)
> -#define EXTENT_MAP_DELALLOC ((u64)-1)
> -
> -/* bits for the flags field */
> -#define EXTENT_FLAG_PINNED 0 /* this entry not yet on disk, don't free it */
> -#define EXTENT_FLAG_COMPRESSED 1
> -#define EXTENT_FLAG_VACANCY 2 /* no file extent item found */
> -#define EXTENT_FLAG_PREALLOC 3 /* pre-allocated extent */
> -#define EXTENT_FLAG_LOGGING 4 /* Logging this extent */
> -#define EXTENT_FLAG_FILLING 5 /* Filling in a preallocated extent */
> -#define EXTENT_FLAG_FS_MAPPING 6 /* filesystem extent mapping type */
> +/*
> + * Reserve some special-case values for the extent_map .start, .block_start,
> + *: and .orig_start fields.
     ^ extra :
> + */
> +#define EXTENT_MAP_LAST_BYTE	((u64)-4)	/* Lowest reserved value */
> +#define EXTENT_MAP_HOLE		((u64)-3)	/* Unwritten extent */
> +#define EXTENT_MAP_INLINE	((u64)-2)	/* Inlined file data */
> +#define EXTENT_MAP_DELALLOC	((u64)-1)	/* Delayed block allocation */

These values are only set/checked on the .block_start parameter so you
can possibly remove the .start/.orig_start from the above comment.

>  
> +/*
> + * Bit flags for the extent_map .flags field.
> + */
> +#define EXTENT_FLAG_PINNED	0 /* entry not yet on disk, don't free it */
> +#define EXTENT_FLAG_COMPRESSED	1
> +#define EXTENT_FLAG_VACANCY	2 /* no file extent item found */
> +#define EXTENT_FLAG_PREALLOC	3 /* pre-allocated extent */
> +#define EXTENT_FLAG_LOGGING	4 /* Logging this extent */
> +#define EXTENT_FLAG_FILLING	5 /* Filling in a preallocated extent */
> +#define EXTENT_FLAG_FS_MAPPING	6 /* filesystem extent mapping type */
> +
> +/*
> + * In-memory representation of a file extent (regular/prealloc/inline).
> + */

<rant>
I just realise how badly named this struct is. Because we really have
on-disk extents and in-memory extents (similar to XFS nomenclature) and
the extent_map name doesn't really bring those 2 things together
</rant>

>  struct extent_map {
>  	struct rb_node rb_node;
>  
>  	/* all of these are in bytes */
> -	u64 start;
> -	u64 len;
> +	u64 start;	/* logical byte offset in the file */
> +	u64 len;	/* byte length of extent in the file */
>  	u64 mod_start;
>  	u64 mod_len;
>  	u64 orig_start;
>  	u64 orig_block_len;
> +
> +	/* max. bytes needed to hold the (uncompressed) extent in memory. */
>  	u64 ram_bytes;
> +
> +	/*
> +	 * For regular/prealloc, block_start is a logical address denoting the
> +	 * start of the extent data, and block_len is the on-disk byte length
> +	 * of the extent (which may be comressed). block_start may be
> +	 * EXTENT_MAP_HOLE if the extent is unwritten.  For inline, block_start
> +	 * is EXTENT_MAP_INLINE and block_len is (u64)-1.  See also
> +	 * btrfs_extent_item_to_extent_map() and btrfs_get_extent().
> +	 */
>  	u64 block_start;
>  	u64 block_len;
> -	u64 generation;
> -	unsigned long flags;
> +
> +	u64 generation;		/* transaction id */
> +	unsigned long flags;	/* EXTENT_FLAG_* bit flags as above */
> +
>  	union {
>  		struct block_device *bdev;
>  
> @@ -44,7 +66,7 @@ struct extent_map {
>  		struct map_lookup *map_lookup;
>  	};
>  	refcount_t refs;
> -	unsigned int compress_type;
> +	unsigned int compress_type;	/* BTRFS_COMPRESS_* */
>  	struct list_head list;
>  };
>  
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index fdcb410..5d63172 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -929,6 +929,9 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
>  	goto out;
>  }
>  
> +/*
> + * Populate an extent_map from the btrfs_file_extent_item contents.
> + */
>  void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>  				     const struct btrfs_path *path,
>  				     struct btrfs_file_extent_item *fi,
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 72c7b38..b6d5d94 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6916,17 +6916,28 @@ static noinline int uncompress_inline(struct btrfs_path *path,
>  }
>  
>  /*
> - * a bit scary, this does extent mapping from logical file offset to the disk.
> - * the ugly parts come from merging extents from the disk with the in-ram
> + * btrfs_get_extent - find or create an extent_map for the (@start, @len) range.
> + *
> + *  @inode - inode containing the desired extent.
> + *  @page - page for inlined extent (NULL if none)
> + *  @page_offset - byte offset within @page
> + *  @start - logical byte offset in the file.
> + *  @len - byte length of the range.
> + *  @create - for inlined extents: 0 to update @page with extent data from
> + *            the item; 1 to bypass the update.
> + *
> + * A bit scary, this does extent mapping from logical file offset to the disk.
> + * The ugly parts come from merging extents from the disk with the in-ram
>   * representation.  This gets more complex because of the data=ordered code,
>   * where the in-ram extents might be locked pending data=ordered completion.
>   *
>   * This also copies inline extents directly into the page.
> + *
> + * Returns the extent_map, or error code.
>   */>  struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
> -		struct page *page,
> -	    size_t pg_offset, u64 start, u64 len,
> -		int create)
> +				    struct page *page, size_t pg_offset,
> +				    u64 start, u64 len, int create)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->vfs_inode.i_sb);
>  	int ret;
> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
> index 6d6e5da..693636a 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -730,6 +730,7 @@ struct btrfs_balance_item {
>  	__le64 unused[4];
>  } __attribute__ ((__packed__));
>  
> +/* Values for .type field in btrfs_file_extent_item */
>  #define BTRFS_FILE_EXTENT_INLINE 0
>  #define BTRFS_FILE_EXTENT_REG 1
>  #define BTRFS_FILE_EXTENT_PREALLOC 2
> 

Re: [PATCH 2/3] btrfs: update some code documentation

[Edmund Nadolski]

On 11/21/2017 12:59 AM, Nikolay Borisov wrote:
> 
> 
> On 20.11.2017 22:24, Edmund Nadolski wrote:
>> Improve code documentation by adding/expanding comments in
>> several places.
>>
>> Signed-off-by: Edmund Nadolski <enadolski@suse.com>
>> ---
>>  fs/btrfs/ctree.c                | 31 +++++++++++++++-------
>>  fs/btrfs/ctree.h                | 28 +++++++++++++++-----
>>  fs/btrfs/extent_map.h           | 58 ++++++++++++++++++++++++++++-------------
>>  fs/btrfs/file-item.c            |  3 +++
>>  fs/btrfs/inode.c                | 21 +++++++++++----
>>  include/uapi/linux/btrfs_tree.h |  1 +
>>  6 files changed, 104 insertions(+), 38 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 531e0a8..9331d02 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -2654,17 +2654,30 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
>>  }
>>  
>>  /*
>> - * look for key in the tree.  path is filled in with nodes along the way
>> - * if key is found, we return zero and you can find the item in the leaf
>> - * level of the path (level 0)
>> + * Search a btree for a specific key. Optionally update the btree for
>> + * item insertion or removal.
>>   *
>> - * If the key isn't found, the path points to the slot where it should
>> - * be inserted, and 1 is returned.  If there are other errors during the
>> - * search a negative error number is returned.
>> + *   @trans - Transaction handle (NULL for none, requires @cow == 0).
>> + *   @root - The btree to be searched.
>> + *   @key - key for search.
>> + *   @p - Points to a btrfs_path to be populated with btree node and index
>> + *      values encountered during traversal. (See also struct btrfs_path.)
>> + *   @ins_len - byte length of item as follows:
>> + *      >0: byte length of item for insertion.  Btree nodes/leaves are
>> + *          split as required.
>> + *      <0: byte length of item for deletion.  Btree nodes/leaves are
>> + *          merged as required.
>> + *       0: Do not modify the btree (search only).
>> + *   @cow - 0 to nocow, or !0 to cow for btree update.
>>   *
>> - * if ins_len > 0, nodes and leaves will be split as we walk down the
>> - * tree.  if ins_len < 0, nodes will be merged as we walk down the tree (if
>> - * possible)
>> + * Return values:
>> + *   0: @key was found and @p was updated to indicate the leaf and item
>> + *       slot where the item may be accessed.
>> + *   1: @key was not found and @p was updated to indicate the leaf and
>> + *       item slot where the item may be inserted.
>> + *  <0: an error occurred.
>> + *
>> + * Note, insertion/removal updates will re-balance the btree as needed.
>>   */
>>  int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>  		      const struct btrfs_key *key, struct btrfs_path *p,
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 2154b5a..e980caa 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -318,20 +318,36 @@ struct btrfs_node {
>>  } __attribute__ ((__packed__));
>>  
>>  /*
>> - * btrfs_paths remember the path taken from the root down to the leaf.
>> - * level 0 is always the leaf, and nodes[1...BTRFS_MAX_LEVEL] will point
>> - * to any other levels that are present.
>> - *
>> - * The slots array records the index of the item or block pointer
>> - * used while walking the tree.
>> + * Used with btrfs_search_slot() to record a btree search path traversed from a
>> + * root down to a leaf.
>>   */
>>  enum { READA_NONE = 0, READA_BACK, READA_FORWARD };
>>  struct btrfs_path {
>> +	/*
>> +	 * The nodes[] and slots[] arrays are indexed according to btree
>> +	 * levels. Index 0 corresponds to the lowest (leaf) level. Increasing
>> +	 * indexes correspond to interior btree nodes at subsequently higher
>> +	 * levels.
>> +	 *
>> +	 * nodes[0] always points to a leaf. nodes[1] points to a btree node
>> +	 * which contains a block pointer referencing that leaf. For higher
>> +	 * indexes, node[n] points to a btree node which contains a block
>> +	 * pointer referencing node[n-1].
>> +	 */
>>  	struct extent_buffer *nodes[BTRFS_MAX_LEVEL];
>> +
>> +	/*
>> +	 * The slots[0] value identifies an item index in the leaf at nodes[0].
>> +	 * Each slots[n] value identifies a block pointer index in the
>> +	 * corresponding tree node at nodes[n].
>> +	 */
>>  	int slots[BTRFS_MAX_LEVEL];
>> +
>>  	/* if there is real range locking, this locks field will change */
>>  	u8 locks[BTRFS_MAX_LEVEL];
>> +
>>  	u8 reada;
>> +
>>  	/* keep some upper locks as we walk down */
>>  	u8 lowest_level;
>>  
>> diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
>> index 64365bb..34ea85c3 100644
>> --- a/fs/btrfs/extent_map.h
>> +++ b/fs/btrfs/extent_map.h
>> @@ -5,35 +5,57 @@
>>  #include <linux/rbtree.h>
>>  #include <linux/refcount.h>
>>  
>> -#define EXTENT_MAP_LAST_BYTE ((u64)-4)
>> -#define EXTENT_MAP_HOLE ((u64)-3)
>> -#define EXTENT_MAP_INLINE ((u64)-2)
>> -#define EXTENT_MAP_DELALLOC ((u64)-1)
>> -
>> -/* bits for the flags field */
>> -#define EXTENT_FLAG_PINNED 0 /* this entry not yet on disk, don't free it */
>> -#define EXTENT_FLAG_COMPRESSED 1
>> -#define EXTENT_FLAG_VACANCY 2 /* no file extent item found */
>> -#define EXTENT_FLAG_PREALLOC 3 /* pre-allocated extent */
>> -#define EXTENT_FLAG_LOGGING 4 /* Logging this extent */
>> -#define EXTENT_FLAG_FILLING 5 /* Filling in a preallocated extent */
>> -#define EXTENT_FLAG_FS_MAPPING 6 /* filesystem extent mapping type */
>> +/*
>> + * Reserve some special-case values for the extent_map .start, .block_start,
>> + *: and .orig_start fields.
>      ^ extra :

Thanks, will fix.


>> + */
>> +#define EXTENT_MAP_LAST_BYTE	((u64)-4)	/* Lowest reserved value */
>> +#define EXTENT_MAP_HOLE		((u64)-3)	/* Unwritten extent */
>> +#define EXTENT_MAP_INLINE	((u64)-2)	/* Inlined file data */
>> +#define EXTENT_MAP_DELALLOC	((u64)-1)	/* Delayed block allocation */
> 
> These values are only set/checked on the .block_start parameter so you
> can possibly remove the .start/.orig_start from the above comment.

Seems there still are a few tucked away:

$ grep -n " = EXTENT_MAP_" *.[ch] | grep -v block_start
file-item.c:996:		em->orig_start = EXTENT_MAP_HOLE;
inode.c:6969:	em->start = EXTENT_MAP_HOLE;
inode.c:6970:	em->orig_start = EXTENT_MAP_HOLE;

Thanks,
Ed


>>  
>> +/*
>> + * Bit flags for the extent_map .flags field.
>> + */
>> +#define EXTENT_FLAG_PINNED	0 /* entry not yet on disk, don't free it */
>> +#define EXTENT_FLAG_COMPRESSED	1
>> +#define EXTENT_FLAG_VACANCY	2 /* no file extent item found */
>> +#define EXTENT_FLAG_PREALLOC	3 /* pre-allocated extent */
>> +#define EXTENT_FLAG_LOGGING	4 /* Logging this extent */
>> +#define EXTENT_FLAG_FILLING	5 /* Filling in a preallocated extent */
>> +#define EXTENT_FLAG_FS_MAPPING	6 /* filesystem extent mapping type */
>> +
>> +/*
>> + * In-memory representation of a file extent (regular/prealloc/inline).
>> + */
> 
> <rant>
> I just realise how badly named this struct is. Because we really have
> on-disk extents and in-memory extents (similar to XFS nomenclature) and
> the extent_map name doesn't really bring those 2 things together
> </rant>
> 
>>  struct extent_map {
>>  	struct rb_node rb_node;
>>  
>>  	/* all of these are in bytes */
>> -	u64 start;
>> -	u64 len;
>> +	u64 start;	/* logical byte offset in the file */
>> +	u64 len;	/* byte length of extent in the file */
>>  	u64 mod_start;
>>  	u64 mod_len;
>>  	u64 orig_start;
>>  	u64 orig_block_len;
>> +
>> +	/* max. bytes needed to hold the (uncompressed) extent in memory. */
>>  	u64 ram_bytes;
>> +
>> +	/*
>> +	 * For regular/prealloc, block_start is a logical address denoting the
>> +	 * start of the extent data, and block_len is the on-disk byte length
>> +	 * of the extent (which may be comressed). block_start may be
>> +	 * EXTENT_MAP_HOLE if the extent is unwritten.  For inline, block_start
>> +	 * is EXTENT_MAP_INLINE and block_len is (u64)-1.  See also
>> +	 * btrfs_extent_item_to_extent_map() and btrfs_get_extent().
>> +	 */
>>  	u64 block_start;
>>  	u64 block_len;
>> -	u64 generation;
>> -	unsigned long flags;
>> +
>> +	u64 generation;		/* transaction id */
>> +	unsigned long flags;	/* EXTENT_FLAG_* bit flags as above */
>> +
>>  	union {
>>  		struct block_device *bdev;
>>  
>> @@ -44,7 +66,7 @@ struct extent_map {
>>  		struct map_lookup *map_lookup;
>>  	};
>>  	refcount_t refs;
>> -	unsigned int compress_type;
>> +	unsigned int compress_type;	/* BTRFS_COMPRESS_* */
>>  	struct list_head list;
>>  };
>>  
>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>> index fdcb410..5d63172 100644
>> --- a/fs/btrfs/file-item.c
>> +++ b/fs/btrfs/file-item.c
>> @@ -929,6 +929,9 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
>>  	goto out;
>>  }
>>  
>> +/*
>> + * Populate an extent_map from the btrfs_file_extent_item contents.
>> + */
>>  void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>>  				     const struct btrfs_path *path,
>>  				     struct btrfs_file_extent_item *fi,
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 72c7b38..b6d5d94 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -6916,17 +6916,28 @@ static noinline int uncompress_inline(struct btrfs_path *path,
>>  }
>>  
>>  /*
>> - * a bit scary, this does extent mapping from logical file offset to the disk.
>> - * the ugly parts come from merging extents from the disk with the in-ram
>> + * btrfs_get_extent - find or create an extent_map for the (@start, @len) range.
>> + *
>> + *  @inode - inode containing the desired extent.
>> + *  @page - page for inlined extent (NULL if none)
>> + *  @page_offset - byte offset within @page
>> + *  @start - logical byte offset in the file.
>> + *  @len - byte length of the range.
>> + *  @create - for inlined extents: 0 to update @page with extent data from
>> + *            the item; 1 to bypass the update.
>> + *
>> + * A bit scary, this does extent mapping from logical file offset to the disk.
>> + * The ugly parts come from merging extents from the disk with the in-ram
>>   * representation.  This gets more complex because of the data=ordered code,
>>   * where the in-ram extents might be locked pending data=ordered completion.
>>   *
>>   * This also copies inline extents directly into the page.
>> + *
>> + * Returns the extent_map, or error code.
>>   */>  struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>> -		struct page *page,
>> -	    size_t pg_offset, u64 start, u64 len,
>> -		int create)
>> +				    struct page *page, size_t pg_offset,
>> +				    u64 start, u64 len, int create)
>>  {
>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->vfs_inode.i_sb);
>>  	int ret;
>> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
>> index 6d6e5da..693636a 100644
>> --- a/include/uapi/linux/btrfs_tree.h
>> +++ b/include/uapi/linux/btrfs_tree.h
>> @@ -730,6 +730,7 @@ struct btrfs_balance_item {
>>  	__le64 unused[4];
>>  } __attribute__ ((__packed__));
>>  
>> +/* Values for .type field in btrfs_file_extent_item */
>>  #define BTRFS_FILE_EXTENT_INLINE 0
>>  #define BTRFS_FILE_EXTENT_REG 1
>>  #define BTRFS_FILE_EXTENT_PREALLOC 2
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] btrfs: update some code documentation

[Nikolay Borisov]

On 22.11.2017 00:20, Edmund Nadolski wrote:
> 
> 
> On 11/21/2017 12:59 AM, Nikolay Borisov wrote:
>>
>>
>> On 20.11.2017 22:24, Edmund Nadolski wrote:
>>> Improve code documentation by adding/expanding comments in
>>> several places.
>>>
>>> Signed-off-by: Edmund Nadolski <enadolski@suse.com>
>>> ---
>>>  fs/btrfs/ctree.c                | 31 +++++++++++++++-------
>>>  fs/btrfs/ctree.h                | 28 +++++++++++++++-----
>>>  fs/btrfs/extent_map.h           | 58 ++++++++++++++++++++++++++++-------------
>>>  fs/btrfs/file-item.c            |  3 +++
>>>  fs/btrfs/inode.c                | 21 +++++++++++----
>>>  include/uapi/linux/btrfs_tree.h |  1 +
>>>  6 files changed, 104 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>>> index 531e0a8..9331d02 100644
>>> --- a/fs/btrfs/ctree.c
>>> +++ b/fs/btrfs/ctree.c
>>> @@ -2654,17 +2654,30 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
>>>  }
>>>  
>>>  /*
>>> - * look for key in the tree.  path is filled in with nodes along the way
>>> - * if key is found, we return zero and you can find the item in the leaf
>>> - * level of the path (level 0)
>>> + * Search a btree for a specific key. Optionally update the btree for
>>> + * item insertion or removal.
>>>   *
>>> - * If the key isn't found, the path points to the slot where it should
>>> - * be inserted, and 1 is returned.  If there are other errors during the
>>> - * search a negative error number is returned.
>>> + *   @trans - Transaction handle (NULL for none, requires @cow == 0).
>>> + *   @root - The btree to be searched.
>>> + *   @key - key for search.
>>> + *   @p - Points to a btrfs_path to be populated with btree node and index
>>> + *      values encountered during traversal. (See also struct btrfs_path.)
>>> + *   @ins_len - byte length of item as follows:
>>> + *      >0: byte length of item for insertion.  Btree nodes/leaves are
>>> + *          split as required.
>>> + *      <0: byte length of item for deletion.  Btree nodes/leaves are
>>> + *          merged as required.
>>> + *       0: Do not modify the btree (search only).
>>> + *   @cow - 0 to nocow, or !0 to cow for btree update.
>>>   *
>>> - * if ins_len > 0, nodes and leaves will be split as we walk down the
>>> - * tree.  if ins_len < 0, nodes will be merged as we walk down the tree (if
>>> - * possible)
>>> + * Return values:
>>> + *   0: @key was found and @p was updated to indicate the leaf and item
>>> + *       slot where the item may be accessed.
>>> + *   1: @key was not found and @p was updated to indicate the leaf and
>>> + *       item slot where the item may be inserted.
>>> + *  <0: an error occurred.
>>> + *
>>> + * Note, insertion/removal updates will re-balance the btree as needed.
>>>   */
>>>  int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>>  		      const struct btrfs_key *key, struct btrfs_path *p,
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index 2154b5a..e980caa 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -318,20 +318,36 @@ struct btrfs_node {
>>>  } __attribute__ ((__packed__));
>>>  
>>>  /*
>>> - * btrfs_paths remember the path taken from the root down to the leaf.
>>> - * level 0 is always the leaf, and nodes[1...BTRFS_MAX_LEVEL] will point
>>> - * to any other levels that are present.
>>> - *
>>> - * The slots array records the index of the item or block pointer
>>> - * used while walking the tree.
>>> + * Used with btrfs_search_slot() to record a btree search path traversed from a
>>> + * root down to a leaf.
>>>   */
>>>  enum { READA_NONE = 0, READA_BACK, READA_FORWARD };
>>>  struct btrfs_path {
>>> +	/*
>>> +	 * The nodes[] and slots[] arrays are indexed according to btree
>>> +	 * levels. Index 0 corresponds to the lowest (leaf) level. Increasing
>>> +	 * indexes correspond to interior btree nodes at subsequently higher
>>> +	 * levels.
>>> +	 *
>>> +	 * nodes[0] always points to a leaf. nodes[1] points to a btree node
>>> +	 * which contains a block pointer referencing that leaf. For higher
>>> +	 * indexes, node[n] points to a btree node which contains a block
>>> +	 * pointer referencing node[n-1].
>>> +	 */
>>>  	struct extent_buffer *nodes[BTRFS_MAX_LEVEL];
>>> +
>>> +	/*
>>> +	 * The slots[0] value identifies an item index in the leaf at nodes[0].
>>> +	 * Each slots[n] value identifies a block pointer index in the
>>> +	 * corresponding tree node at nodes[n].
>>> +	 */
>>>  	int slots[BTRFS_MAX_LEVEL];
>>> +
>>>  	/* if there is real range locking, this locks field will change */
>>>  	u8 locks[BTRFS_MAX_LEVEL];
>>> +
>>>  	u8 reada;
>>> +
>>>  	/* keep some upper locks as we walk down */
>>>  	u8 lowest_level;
>>>  
>>> diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
>>> index 64365bb..34ea85c3 100644
>>> --- a/fs/btrfs/extent_map.h
>>> +++ b/fs/btrfs/extent_map.h
>>> @@ -5,35 +5,57 @@
>>>  #include <linux/rbtree.h>
>>>  #include <linux/refcount.h>
>>>  
>>> -#define EXTENT_MAP_LAST_BYTE ((u64)-4)
>>> -#define EXTENT_MAP_HOLE ((u64)-3)
>>> -#define EXTENT_MAP_INLINE ((u64)-2)
>>> -#define EXTENT_MAP_DELALLOC ((u64)-1)
>>> -
>>> -/* bits for the flags field */
>>> -#define EXTENT_FLAG_PINNED 0 /* this entry not yet on disk, don't free it */
>>> -#define EXTENT_FLAG_COMPRESSED 1
>>> -#define EXTENT_FLAG_VACANCY 2 /* no file extent item found */
>>> -#define EXTENT_FLAG_PREALLOC 3 /* pre-allocated extent */
>>> -#define EXTENT_FLAG_LOGGING 4 /* Logging this extent */
>>> -#define EXTENT_FLAG_FILLING 5 /* Filling in a preallocated extent */
>>> -#define EXTENT_FLAG_FS_MAPPING 6 /* filesystem extent mapping type */
>>> +/*
>>> + * Reserve some special-case values for the extent_map .start, .block_start,
>>> + *: and .orig_start fields.
>>      ^ extra :
> 
> Thanks, will fix.
> 
> 
>>> + */
>>> +#define EXTENT_MAP_LAST_BYTE	((u64)-4)	/* Lowest reserved value */
>>> +#define EXTENT_MAP_HOLE		((u64)-3)	/* Unwritten extent */
>>> +#define EXTENT_MAP_INLINE	((u64)-2)	/* Inlined file data */
>>> +#define EXTENT_MAP_DELALLOC	((u64)-1)	/* Delayed block allocation */
>>
>> These values are only set/checked on the .block_start parameter so you
>> can possibly remove the .start/.orig_start from the above comment.
> 
> Seems there still are a few tucked away:
> 
> $ grep -n " = EXTENT_MAP_" *.[ch] | grep -v block_start
> file-item.c:996:		em->orig_start = EXTENT_MAP_HOLE;
> inode.c:6969:	em->start = EXTENT_MAP_HOLE;
> inode.c:6970:	em->orig_start = EXTENT_MAP_HOLE;

As a matter of fact I think those are redundant/buggy since they are
never checked. So I gues you can fold their removal in this patch.

> 
> Thanks,
> Ed
> 
> 
>>>  
>>> +/*
>>> + * Bit flags for the extent_map .flags field.
>>> + */
>>> +#define EXTENT_FLAG_PINNED	0 /* entry not yet on disk, don't free it */
>>> +#define EXTENT_FLAG_COMPRESSED	1
>>> +#define EXTENT_FLAG_VACANCY	2 /* no file extent item found */
>>> +#define EXTENT_FLAG_PREALLOC	3 /* pre-allocated extent */
>>> +#define EXTENT_FLAG_LOGGING	4 /* Logging this extent */
>>> +#define EXTENT_FLAG_FILLING	5 /* Filling in a preallocated extent */
>>> +#define EXTENT_FLAG_FS_MAPPING	6 /* filesystem extent mapping type */
>>> +
>>> +/*
>>> + * In-memory representation of a file extent (regular/prealloc/inline).
>>> + */
>>
>> <rant>
>> I just realise how badly named this struct is. Because we really have
>> on-disk extents and in-memory extents (similar to XFS nomenclature) and
>> the extent_map name doesn't really bring those 2 things together
>> </rant>
>>
>>>  struct extent_map {
>>>  	struct rb_node rb_node;
>>>  
>>>  	/* all of these are in bytes */
>>> -	u64 start;
>>> -	u64 len;
>>> +	u64 start;	/* logical byte offset in the file */
>>> +	u64 len;	/* byte length of extent in the file */
>>>  	u64 mod_start;
>>>  	u64 mod_len;
>>>  	u64 orig_start;
>>>  	u64 orig_block_len;
>>> +
>>> +	/* max. bytes needed to hold the (uncompressed) extent in memory. */
>>>  	u64 ram_bytes;
>>> +
>>> +	/*
>>> +	 * For regular/prealloc, block_start is a logical address denoting the
>>> +	 * start of the extent data, and block_len is the on-disk byte length
>>> +	 * of the extent (which may be comressed). block_start may be
>>> +	 * EXTENT_MAP_HOLE if the extent is unwritten.  For inline, block_start
>>> +	 * is EXTENT_MAP_INLINE and block_len is (u64)-1.  See also
>>> +	 * btrfs_extent_item_to_extent_map() and btrfs_get_extent().
>>> +	 */
>>>  	u64 block_start;
>>>  	u64 block_len;
>>> -	u64 generation;
>>> -	unsigned long flags;
>>> +
>>> +	u64 generation;		/* transaction id */
>>> +	unsigned long flags;	/* EXTENT_FLAG_* bit flags as above */
>>> +
>>>  	union {
>>>  		struct block_device *bdev;
>>>  
>>> @@ -44,7 +66,7 @@ struct extent_map {
>>>  		struct map_lookup *map_lookup;
>>>  	};
>>>  	refcount_t refs;
>>> -	unsigned int compress_type;
>>> +	unsigned int compress_type;	/* BTRFS_COMPRESS_* */
>>>  	struct list_head list;
>>>  };
>>>  
>>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>>> index fdcb410..5d63172 100644
>>> --- a/fs/btrfs/file-item.c
>>> +++ b/fs/btrfs/file-item.c
>>> @@ -929,6 +929,9 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
>>>  	goto out;
>>>  }
>>>  
>>> +/*
>>> + * Populate an extent_map from the btrfs_file_extent_item contents.
>>> + */
>>>  void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>>>  				     const struct btrfs_path *path,
>>>  				     struct btrfs_file_extent_item *fi,
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 72c7b38..b6d5d94 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -6916,17 +6916,28 @@ static noinline int uncompress_inline(struct btrfs_path *path,
>>>  }
>>>  
>>>  /*
>>> - * a bit scary, this does extent mapping from logical file offset to the disk.
>>> - * the ugly parts come from merging extents from the disk with the in-ram
>>> + * btrfs_get_extent - find or create an extent_map for the (@start, @len) range.
>>> + *
>>> + *  @inode - inode containing the desired extent.
>>> + *  @page - page for inlined extent (NULL if none)
>>> + *  @page_offset - byte offset within @page
>>> + *  @start - logical byte offset in the file.
>>> + *  @len - byte length of the range.
>>> + *  @create - for inlined extents: 0 to update @page with extent data from
>>> + *            the item; 1 to bypass the update.
>>> + *
>>> + * A bit scary, this does extent mapping from logical file offset to the disk.
>>> + * The ugly parts come from merging extents from the disk with the in-ram
>>>   * representation.  This gets more complex because of the data=ordered code,
>>>   * where the in-ram extents might be locked pending data=ordered completion.
>>>   *
>>>   * This also copies inline extents directly into the page.
>>> + *
>>> + * Returns the extent_map, or error code.
>>>   */>  struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>>> -		struct page *page,
>>> -	    size_t pg_offset, u64 start, u64 len,
>>> -		int create)
>>> +				    struct page *page, size_t pg_offset,
>>> +				    u64 start, u64 len, int create)
>>>  {
>>>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->vfs_inode.i_sb);
>>>  	int ret;
>>> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
>>> index 6d6e5da..693636a 100644
>>> --- a/include/uapi/linux/btrfs_tree.h
>>> +++ b/include/uapi/linux/btrfs_tree.h
>>> @@ -730,6 +730,7 @@ struct btrfs_balance_item {
>>>  	__le64 unused[4];
>>>  } __attribute__ ((__packed__));
>>>  
>>> +/* Values for .type field in btrfs_file_extent_item */
>>>  #define BTRFS_FILE_EXTENT_INLINE 0
>>>  #define BTRFS_FILE_EXTENT_REG 1
>>>  #define BTRFS_FILE_EXTENT_PREALLOC 2
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH 2/3] btrfs: update some code documentation

[David Sterba]

On Mon, Nov 20, 2017 at 01:24:48PM -0700, Edmund Nadolski wrote:
> +/*
> + * Reserve some special-case values for the extent_map .start, .block_start,
> + *: and .orig_start fields.

I'm used to write the structure members as type::member, which resembles
the C++ syntax. There are already a few recently added examples in the
code documentation, eg. the device mutexes in volume.c. I'd like to
maintain some consistency so that it's clear which syntax we use for
btrfs documentataion. The .member also looks acceptable to me, though
I'd prefer the :: way.

[PATCH 3/3] btrfs: remove dead code from btrfs_get_extent

[Edmund Nadolski]

Due to new_inline logic, the create == 0 is always true at this
point in the code, so the create != 0 branch can be removed.

Signed-off-by: Edmund Nadolski <enadolski@suse.com>
---
 fs/btrfs/inode.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b6d5d94..0d4a066 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6957,7 +6957,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	struct btrfs_trans_handle *trans = NULL;
 	const bool new_inline = !page || create;
 
-again:
 	read_lock(&em_tree->lock);
 	em = lookup_extent_mapping(em_tree, start, len);
 	if (em)
@@ -7098,7 +7097,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 		em->orig_block_len = em->len;
 		em->orig_start = em->start;
 		ptr = btrfs_file_extent_inline_start(item) + extent_offset;
-		if (create == 0 && !PageUptodate(page)) {
+		if (!PageUptodate(page)) {
 			if (btrfs_file_extent_compression(leaf, item) !=
 			    BTRFS_COMPRESS_NONE) {
 				ret = uncompress_inline(path, page, pg_offset,
@@ -7119,25 +7118,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 				kunmap(page);
 			}
 			flush_dcache_page(page);
-		} else if (create && PageUptodate(page)) {
-			BUG();
-			if (!trans) {
-				kunmap(page);
-				free_extent_map(em);
-				em = NULL;
-
-				btrfs_release_path(path);
-				trans = btrfs_join_transaction(root);
-
-				if (IS_ERR(trans))
-					return ERR_CAST(trans);
-				goto again;
-			}
-			map = kmap(page);
-			write_extent_buffer(leaf, map + pg_offset, ptr,
-					    copy_size);
-			kunmap(page);
-			btrfs_mark_buffer_dirty(leaf);
 		}
 		set_extent_uptodate(io_tree, em->start,
 				    extent_map_end(em) - 1, NULL, GFP_NOFS);
-- 
2.10.2

Re: [PATCH 3/3] btrfs: remove dead code from btrfs_get_extent

[Nikolay Borisov]

On 20.11.2017 22:24, Edmund Nadolski wrote:
> Due to new_inline logic, the create == 0 is always true at this
> point in the code, so the create != 0 branch can be removed.
> 
> Signed-off-by: Edmund Nadolski <enadolski@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/inode.c | 22 +---------------------
>  1 file changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b6d5d94..0d4a066 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6957,7 +6957,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  	struct btrfs_trans_handle *trans = NULL;
>  	const bool new_inline = !page || create;
>  
> -again:
>  	read_lock(&em_tree->lock);
>  	em = lookup_extent_mapping(em_tree, start, len);
>  	if (em)
> @@ -7098,7 +7097,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  		em->orig_block_len = em->len;
>  		em->orig_start = em->start;
>  		ptr = btrfs_file_extent_inline_start(item) + extent_offset;
> -		if (create == 0 && !PageUptodate(page)) {
> +		if (!PageUptodate(page)) {
>  			if (btrfs_file_extent_compression(leaf, item) !=
>  			    BTRFS_COMPRESS_NONE) {
>  				ret = uncompress_inline(path, page, pg_offset,
> @@ -7119,25 +7118,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  				kunmap(page);
>  			}
>  			flush_dcache_page(page);
> -		} else if (create && PageUptodate(page)) {
> -			BUG();
> -			if (!trans) {
> -				kunmap(page);
> -				free_extent_map(em);
> -				em = NULL;
> -
> -				btrfs_release_path(path);
> -				trans = btrfs_join_transaction(root);
> -
> -				if (IS_ERR(trans))
> -					return ERR_CAST(trans);
> -				goto again;
> -			}
> -			map = kmap(page);
> -			write_extent_buffer(leaf, map + pg_offset, ptr,
> -					    copy_size);
> -			kunmap(page);
> -			btrfs_mark_buffer_dirty(leaf);
>  		}
>  		set_extent_uptodate(io_tree, em->start,
>  				    extent_map_end(em) - 1, NULL, GFP_NOFS);
> 

Re: [PATCH 0/3] btrfs: some code cleanup and documentation

[David Sterba]

On Mon, Nov 20, 2017 at 01:24:46PM -0700, Edmund Nadolski wrote:
> This series adds a bit of code cleanup and some documentation in the
> form of comments. No operational change.
> 
> Edmund Nadolski (3):
>   btrfs: btrfs_inode_log_parent should use defined inode_only values.
>   btrfs: update some code documentation
>   btrfs: remove dead code from btrfs_get_extent

Patches 1 and 3 added to next, thanks.