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 <enadol...@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 majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html