[PATCH v4 0/3] btrfs: more explaination on extent_map members

[PATCH v4 0/3] btrfs: more explaination on extent_map members

[Qu Wenruo]

[REPO]
https://github.com/adam900710/linux.git/ em_cleanup

The first 3 patches only. As later patches are the huge rename part.

[CHANGELOG]
v4:
- Add extra comments for the extent map merging
  Since extent maps can be merged, the members are only matching
  on-disk file extent items before merging.
  This also means users of extent_map should not rely on it for
  a reliable file extent item member.
  (That's also why we use ordered_extent not extent_maps, to update
   file extent items)

v3:
- Rebased to latest for-next branch
- Further comments polishment
- Coding style update to follow the guideline

v2:
- Add Filipe's cleanup on mod_start/mod_len
  These two members are no longer utilized, saving me quite some time on
  digging into their usage.

- Update the comments of the extent_map structure
  To make them more readable and less confusing.

- Further cleanup for inline extent_map reading

- A new patch to do extra sanity checks for create_io_em()
  Firstly pure NOCOW writes should not call create_io_em(), secondly
  with the new knowledge of extent_map, it's easier to do extra sanity
  checks for the already pretty long parameter list.

Btrfs uses extent_map to represent a in-memory file extent.

There are severam members that are 1:1 mappe in on-disk file extent
items and extent maps:

- extent_map::start	==	key.offset
- extent_map::len	==	file_extent_num_bytes
- extent_map::ram_bytes	==	file_extent_ram_bytes

But that's all, the remaining are pretty different:

- Use block_start to indicate holes/inline extents
  Meanwhile btrfs on-disk file extent items go with a dedicated type for
  inline extents, and disk_bytenr 0 for holes.

- Weird block_start/orig_block_len/orig_start
  In theory we can directly go with the same file_extent_disk_bytenr,
  file_extent_disk_num_bytes and file_extent_offset to calculate the
  remaining members (block_start/orig_start/orig_block_len/block_len).

  But for whatever reason, we didn't go that path and have a hell of
  weird and inconsistent calculation for them.

Qu Wenruo (3):
  btrfs: add extra comments on extent_map members
  btrfs: simplify the inline extent map creation
  btrfs: add extra sanity checks for create_io_em()

 fs/btrfs/extent_map.h | 58 ++++++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/file-item.c  | 20 +++++++--------
 fs/btrfs/inode.c      | 40 ++++++++++++++++++++++++++++-
 3 files changed, 106 insertions(+), 12 deletions(-)

-- 
2.44.0

[PATCH v4 1/3] btrfs: add extra comments on extent_map members

[Qu Wenruo]

The extent_map structure is very critical to btrfs, as it is involved
for both read and write paths.

Unfortunately the structure is not properly explained, making it pretty
hard to understand nor to do further improvement.

This patch adds extra comments explaining the major members based on
my code reading.
Hopefully we can find more members to cleanup in the future.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_map.h | 58 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
index 10e9491865c9..1b8a55e4b678 100644
--- a/fs/btrfs/extent_map.h
+++ b/fs/btrfs/extent_map.h
@@ -35,19 +35,75 @@ enum {
 };
 
 /*
+ * This structure represents file extents and holes.
+ *
+ * Unlike on-disk file extent items, extent maps can be merged to
+ * save memory.
+ * This means members only match file extent items before any merging.
+ * And users of extent_map should never assume the members are always
+ * matching an on-disk file extent item.
+ *
  * Keep this structure as compact as possible, as we can have really large
  * amounts of allocated extent maps at any time.
  */
 struct extent_map {
 	struct rb_node rb_node;
 
-	/* all of these are in bytes */
+	/* All of these are in bytes. */
+
+	/* File offset matching the offset of a BTRFS_EXTENT_ITEM_KEY key. */
 	u64 start;
+
+	/*
+	 * Length of the file extent.
+	 *
+	 * For non-inlined file extents it's btrfs_file_extent_item::num_bytes.
+	 * For inline extents it's sectorsize, since inline data starts at
+	 * offsetof(struct btrfs_file_extent_item, disk_bytenr) thus
+	 * btrfs_file_extent_item::num_bytes is not valid.
+	 */
 	u64 len;
+
+	/*
+	 * The file offset of the original file extent before splitting.
+	 *
+	 * This is an in-memory only member, matching
+	 * extent_map::start - btrfs_file_extent_item::offset for
+	 * regular/preallocated extents. EXTENT_MAP_HOLE otherwise.
+	 */
 	u64 orig_start;
+
+	/*
+	 * The full on-disk extent length, matching
+	 * btrfs_file_extent_item::disk_num_bytes.
+	 */
 	u64 orig_block_len;
+
+	/*
+	 * The decompressed size of the whole on-disk extent, matching
+	 * btrfs_file_extent_item::ram_bytes.
+	 */
 	u64 ram_bytes;
+
+	/*
+	 * The on-disk logical bytenr for the file extent.
+	 *
+	 * For compressed extents it matches btrfs_file_extent_item::disk_bytenr.
+	 * For uncompressed extents it matches
+	 * btrfs_file_extent_item::disk_bytenr + btrfs_file_extent_item::offset
+	 *
+	 * For holes it is EXTENT_MAP_HOLE and for inline extents it is
+	 * EXTENT_MAP_INLINE.
+	 */
 	u64 block_start;
+
+	/*
+	 * The on-disk length for the file extent.
+	 *
+	 * For compressed extents it matches btrfs_file_extent_item::disk_num_bytes.
+	 * For uncompressed extents it matches extent_map::len.
+	 * For holes and inline extents it's -1 and shouldn't be used.
+	 */
 	u64 block_len;
 
 	/*
-- 
2.44.0

Re: [PATCH v4 1/3] btrfs: add extra comments on extent_map members

[Filipe Manana]

On Thu, Apr 11, 2024 at 11:48 PM Qu Wenruo <wqu@suse.com> wrote:
>
> The extent_map structure is very critical to btrfs, as it is involved
> for both read and write paths.
>
> Unfortunately the structure is not properly explained, making it pretty
> hard to understand nor to do further improvement.
>
> This patch adds extra comments explaining the major members based on
> my code reading.
> Hopefully we can find more members to cleanup in the future.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_map.h | 58 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
> index 10e9491865c9..1b8a55e4b678 100644
> --- a/fs/btrfs/extent_map.h
> +++ b/fs/btrfs/extent_map.h
> @@ -35,19 +35,75 @@ enum {
>  };
>
>  /*
> + * This structure represents file extents and holes.
> + *
> + * Unlike on-disk file extent items, extent maps can be merged to
> + * save memory.
> + * This means members only match file extent items before any merging.
> + * And users of extent_map should never assume the members are always
> + * matching an on-disk file extent item.

They can safely assume they match one single file extent item if the
extent map is pinned or it's in the list of modified extents of the
extent tree.
This is very important and the fast fsync path relies on this for correctness.

There's also the case of compressed extents, which are never merged.

So that part of saying "never assume ..." is misleading and doesn't
match reality.

With that corrected:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> + *
>   * Keep this structure as compact as possible, as we can have really large
>   * amounts of allocated extent maps at any time.
>   */
>  struct extent_map {
>         struct rb_node rb_node;
>
> -       /* all of these are in bytes */
> +       /* All of these are in bytes. */
> +
> +       /* File offset matching the offset of a BTRFS_EXTENT_ITEM_KEY key. */
>         u64 start;
> +
> +       /*
> +        * Length of the file extent.
> +        *
> +        * For non-inlined file extents it's btrfs_file_extent_item::num_bytes.
> +        * For inline extents it's sectorsize, since inline data starts at
> +        * offsetof(struct btrfs_file_extent_item, disk_bytenr) thus
> +        * btrfs_file_extent_item::num_bytes is not valid.
> +        */
>         u64 len;
> +
> +       /*
> +        * The file offset of the original file extent before splitting.
> +        *
> +        * This is an in-memory only member, matching
> +        * extent_map::start - btrfs_file_extent_item::offset for
> +        * regular/preallocated extents. EXTENT_MAP_HOLE otherwise.
> +        */
>         u64 orig_start;
> +
> +       /*
> +        * The full on-disk extent length, matching
> +        * btrfs_file_extent_item::disk_num_bytes.
> +        */
>         u64 orig_block_len;
> +
> +       /*
> +        * The decompressed size of the whole on-disk extent, matching
> +        * btrfs_file_extent_item::ram_bytes.
> +        */
>         u64 ram_bytes;
> +
> +       /*
> +        * The on-disk logical bytenr for the file extent.
> +        *
> +        * For compressed extents it matches btrfs_file_extent_item::disk_bytenr.
> +        * For uncompressed extents it matches
> +        * btrfs_file_extent_item::disk_bytenr + btrfs_file_extent_item::offset
> +        *
> +        * For holes it is EXTENT_MAP_HOLE and for inline extents it is
> +        * EXTENT_MAP_INLINE.
> +        */
>         u64 block_start;
> +
> +       /*
> +        * The on-disk length for the file extent.
> +        *
> +        * For compressed extents it matches btrfs_file_extent_item::disk_num_bytes.
> +        * For uncompressed extents it matches extent_map::len.
> +        * For holes and inline extents it's -1 and shouldn't be used.
> +        */
>         u64 block_len;
>
>         /*
> --
> 2.44.0
>
>

[PATCH v4 2/3] btrfs: simplify the inline extent map creation

[Qu Wenruo]

With the tree-checker ensuring all inline file extents starts at file
offset 0 and has a length no larger than sectorsize, we can simplify the
calculation to assigned those fixes values directly.

Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file-item.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index e58fb5347e65..844439f19949 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -1265,20 +1265,19 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
 	struct extent_buffer *leaf = path->nodes[0];
 	const int slot = path->slots[0];
 	struct btrfs_key key;
-	u64 extent_start, extent_end;
+	u64 extent_start;
 	u64 bytenr;
 	u8 type = btrfs_file_extent_type(leaf, fi);
 	int compress_type = btrfs_file_extent_compression(leaf, fi);
 
 	btrfs_item_key_to_cpu(leaf, &key, slot);
 	extent_start = key.offset;
-	extent_end = btrfs_file_extent_end(path);
 	em->ram_bytes = btrfs_file_extent_ram_bytes(leaf, fi);
 	em->generation = btrfs_file_extent_generation(leaf, fi);
 	if (type == BTRFS_FILE_EXTENT_REG ||
 	    type == BTRFS_FILE_EXTENT_PREALLOC) {
 		em->start = extent_start;
-		em->len = extent_end - extent_start;
+		em->len = btrfs_file_extent_end(path) - extent_start;
 		em->orig_start = extent_start -
 			btrfs_file_extent_offset(leaf, fi);
 		em->orig_block_len = btrfs_file_extent_disk_num_bytes(leaf, fi);
@@ -1299,9 +1298,12 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
 				em->flags |= EXTENT_FLAG_PREALLOC;
 		}
 	} else if (type == BTRFS_FILE_EXTENT_INLINE) {
+		/* Tree-checker has ensured this. */
+		ASSERT(extent_start == 0);
+
 		em->block_start = EXTENT_MAP_INLINE;
-		em->start = extent_start;
-		em->len = extent_end - extent_start;
+		em->start = 0;
+		em->len = fs_info->sectorsize;
 		/*
 		 * Initialize orig_start and block_len with the same values
 		 * as in inode.c:btrfs_get_extent().
@@ -1334,12 +1336,10 @@ u64 btrfs_file_extent_end(const struct btrfs_path *path)
 	ASSERT(key.type == BTRFS_EXTENT_DATA_KEY);
 	fi = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item);
 
-	if (btrfs_file_extent_type(leaf, fi) == BTRFS_FILE_EXTENT_INLINE) {
-		end = btrfs_file_extent_ram_bytes(leaf, fi);
-		end = ALIGN(key.offset + end, leaf->fs_info->sectorsize);
-	} else {
+	if (btrfs_file_extent_type(leaf, fi) == BTRFS_FILE_EXTENT_INLINE)
+		end = leaf->fs_info->sectorsize;
+	else
 		end = key.offset + btrfs_file_extent_num_bytes(leaf, fi);
-	}
 
 	return end;
 }
-- 
2.44.0

[PATCH v4 3/3] btrfs: add extra sanity checks for create_io_em()

[Qu Wenruo]

The function create_io_em() is called before we submit an IO, to update
the in-memory extent map for the involved range.

This patch changes the following aspects:

- Does not allow BTRFS_ORDERED_NOCOW type
  For real NOCOW (excluding NOCOW writes into preallocated ranges)
  writes, we never call create_io_em(), as we does not need to update
  the extent map at all.

  So remove the sanity check allowing BTRFS_ORDERED_NOCOW type.

- Add extra sanity checks
  * PREALLOC
    - @block_len == len
      For uncompressed writes.

  * REGULAR
    - @block_len == @orig_block_len == @ram_bytes == @len
      We're creating a new uncompressed extent, and referring all of it.

    - @orig_start == @start
      We haven no offset inside the extent.

  * COMPRESSED
    - valid @compress_type
    - @len <= @ram_bytes
      This is to co-operate with encoded writes, which can cause a new
      file extent referring only part of a uncompressed extent.

Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c6f2b5d1dee1..ced916f42bab 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7320,11 +7320,49 @@ static struct extent_map *create_io_em(struct btrfs_inode *inode, u64 start,
 	struct extent_map *em;
 	int ret;
 
+	/*
+	 * Note the missing of NOCOW type.
+	 *
+	 * For pure NOCOW writes, we should not create an io extent map,
+	 * but just reusing the existing one.
+	 * Only PREALLOC writes (NOCOW write into preallocated range) can
+	 * create io extent map.
+	 */
 	ASSERT(type == BTRFS_ORDERED_PREALLOC ||
 	       type == BTRFS_ORDERED_COMPRESSED ||
-	       type == BTRFS_ORDERED_NOCOW ||
 	       type == BTRFS_ORDERED_REGULAR);
 
+	switch (type) {
+	case BTRFS_ORDERED_PREALLOC:
+		/* Uncompressed extents. */
+		ASSERT(block_len == len);
+
+		/* We're only referring part of a larger preallocated extent. */
+		ASSERT(block_len <= ram_bytes);
+		break;
+	case BTRFS_ORDERED_REGULAR:
+		/* Uncompressed extents. */
+		ASSERT(block_len == len);
+
+		/* COW results a new extent matching our file extent size. */
+		ASSERT(orig_block_len == len);
+		ASSERT(ram_bytes == len);
+
+		/* Since it's a new extent, we should not have any offset. */
+		ASSERT(orig_start == start);
+		break;
+	case BTRFS_ORDERED_COMPRESSED:
+		/* Must be compressed. */
+		ASSERT(compress_type != BTRFS_COMPRESS_NONE);
+
+		/*
+		 * Encoded write can make us to refer to part of the
+		 * uncompressed extent.
+		 */
+		ASSERT(len <= ram_bytes);
+		break;
+	}
+
 	em = alloc_extent_map();
 	if (!em)
 		return ERR_PTR(-ENOMEM);
-- 
2.44.0

Re: [PATCH v4 0/3] btrfs: more explaination on extent_map members

[Filipe Manana]

On Thu, Apr 11, 2024 at 11:48 PM Qu Wenruo <wqu@suse.com> wrote:
>
> [REPO]
> https://github.com/adam900710/linux.git/ em_cleanup
>
> The first 3 patches only. As later patches are the huge rename part.
>
> [CHANGELOG]
> v4:
> - Add extra comments for the extent map merging
>   Since extent maps can be merged, the members are only matching
>   on-disk file extent items before merging.
>   This also means users of extent_map should not rely on it for
>   a reliable file extent item member.
>   (That's also why we use ordered_extent not extent_maps, to update
>    file extent items)

So this isn't true.
We use information from the ordered extent to create the file extent
item, when finishing the ordered extent, because the ordered extent is
immediately accessible.
We could also use the extent map, we would just have to do a tree search for it.
The extent map created when starting delalloc or at the beginning of a
DIO write is pinned and in the list of modified extents, so it can't
be merged and it represents exactly one file extent item - the one
going to be inserted the inode's subvolume tree when finishing the
ordered extent.

>
> v3:
> - Rebased to latest for-next branch
> - Further comments polishment
> - Coding style update to follow the guideline
>
> v2:
> - Add Filipe's cleanup on mod_start/mod_len
>   These two members are no longer utilized, saving me quite some time on
>   digging into their usage.
>
> - Update the comments of the extent_map structure
>   To make them more readable and less confusing.
>
> - Further cleanup for inline extent_map reading
>
> - A new patch to do extra sanity checks for create_io_em()
>   Firstly pure NOCOW writes should not call create_io_em(), secondly
>   with the new knowledge of extent_map, it's easier to do extra sanity
>   checks for the already pretty long parameter list.
>
> Btrfs uses extent_map to represent a in-memory file extent.
>
> There are severam members that are 1:1 mappe in on-disk file extent
> items and extent maps:
>
> - extent_map::start     ==      key.offset
> - extent_map::len       ==      file_extent_num_bytes
> - extent_map::ram_bytes ==      file_extent_ram_bytes
>
> But that's all, the remaining are pretty different:
>
> - Use block_start to indicate holes/inline extents
>   Meanwhile btrfs on-disk file extent items go with a dedicated type for
>   inline extents, and disk_bytenr 0 for holes.
>
> - Weird block_start/orig_block_len/orig_start
>   In theory we can directly go with the same file_extent_disk_bytenr,
>   file_extent_disk_num_bytes and file_extent_offset to calculate the
>   remaining members (block_start/orig_start/orig_block_len/block_len).
>
>   But for whatever reason, we didn't go that path and have a hell of
>   weird and inconsistent calculation for them.
>
> Qu Wenruo (3):
>   btrfs: add extra comments on extent_map members
>   btrfs: simplify the inline extent map creation
>   btrfs: add extra sanity checks for create_io_em()
>
>  fs/btrfs/extent_map.h | 58 ++++++++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/file-item.c  | 20 +++++++--------
>  fs/btrfs/inode.c      | 40 ++++++++++++++++++++++++++++-
>  3 files changed, 106 insertions(+), 12 deletions(-)
>
> --
> 2.44.0
>
>

Re: [PATCH v4 0/3] btrfs: more explaination on extent_map members

[Qu Wenruo]

在 2024/4/12 20:01, Filipe Manana 写道:
> On Thu, Apr 11, 2024 at 11:48 PM Qu Wenruo <wqu@suse.com> wrote:
>>
>> [REPO]
>> https://github.com/adam900710/linux.git/ em_cleanup
>>
>> The first 3 patches only. As later patches are the huge rename part.
>>
>> [CHANGELOG]
>> v4:
>> - Add extra comments for the extent map merging
>>    Since extent maps can be merged, the members are only matching
>>    on-disk file extent items before merging.
>>    This also means users of extent_map should not rely on it for
>>    a reliable file extent item member.
>>    (That's also why we use ordered_extent not extent_maps, to update
>>     file extent items)
> 
> So this isn't true.
> We use information from the ordered extent to create the file extent
> item, when finishing the ordered extent, because the ordered extent is
> immediately accessible.
> We could also use the extent map, we would just have to do a tree search for it.
> The extent map created when starting delalloc or at the beginning of a
> DIO write is pinned and in the list of modified extents, so it can't
> be merged and it represents exactly one file extent item - the one
> going to be inserted the inode's subvolume tree when finishing the
> ordered extent.

If you're doing COW writes, extent map is fine, as it's always new 
extents and they are all pinned thus won't be merged.

For pure NOCOW, it's also fine because there would be no extent map 
inserted, nor updating file extent items.

But for PREALLOC writes (NOCOW into preallocated writes), it's not the 
case as the target extent maps can be merged already.

Thus that's why for NOCOW we go with a subvolume tree search to grab the 
correct original disk_bytenr/disk_num_bytes/offset to go.

If you use extent map instead of search subvolume tree, the following 
layout can screw up everything:

Filepos 0:
disk_bytenr X, disk_num_bytes 64K
num_bytes 64K, offset 0, ram_bytes 64K

Filepos 64K
disk_bytenr X+64K, disk_num_bytes 64K
num_bytes 64K, offset 0, ram_bytes 64K.

The extent map would be (after merge):

filepos 0, len 128K block_start X, block len 128K, orig_start 0.

Even with my new disk_bytenr it would be no difference, the disk_bytenr 
would still be X, and disk_num_bytes would be merged 128K.

So the merging behavior is affecting PREALLOC writes, and we're already 
avoiding the only pitfall.

Thanks,
Qu

> 
>>
>> v3:
>> - Rebased to latest for-next branch
>> - Further comments polishment
>> - Coding style update to follow the guideline
>>
>> v2:
>> - Add Filipe's cleanup on mod_start/mod_len
>>    These two members are no longer utilized, saving me quite some time on
>>    digging into their usage.
>>
>> - Update the comments of the extent_map structure
>>    To make them more readable and less confusing.
>>
>> - Further cleanup for inline extent_map reading
>>
>> - A new patch to do extra sanity checks for create_io_em()
>>    Firstly pure NOCOW writes should not call create_io_em(), secondly
>>    with the new knowledge of extent_map, it's easier to do extra sanity
>>    checks for the already pretty long parameter list.
>>
>> Btrfs uses extent_map to represent a in-memory file extent.
>>
>> There are severam members that are 1:1 mappe in on-disk file extent
>> items and extent maps:
>>
>> - extent_map::start     ==      key.offset
>> - extent_map::len       ==      file_extent_num_bytes
>> - extent_map::ram_bytes ==      file_extent_ram_bytes
>>
>> But that's all, the remaining are pretty different:
>>
>> - Use block_start to indicate holes/inline extents
>>    Meanwhile btrfs on-disk file extent items go with a dedicated type for
>>    inline extents, and disk_bytenr 0 for holes.
>>
>> - Weird block_start/orig_block_len/orig_start
>>    In theory we can directly go with the same file_extent_disk_bytenr,
>>    file_extent_disk_num_bytes and file_extent_offset to calculate the
>>    remaining members (block_start/orig_start/orig_block_len/block_len).
>>
>>    But for whatever reason, we didn't go that path and have a hell of
>>    weird and inconsistent calculation for them.
>>
>> Qu Wenruo (3):
>>    btrfs: add extra comments on extent_map members
>>    btrfs: simplify the inline extent map creation
>>    btrfs: add extra sanity checks for create_io_em()
>>
>>   fs/btrfs/extent_map.h | 58 ++++++++++++++++++++++++++++++++++++++++++-
>>   fs/btrfs/file-item.c  | 20 +++++++--------
>>   fs/btrfs/inode.c      | 40 ++++++++++++++++++++++++++++-
>>   3 files changed, 106 insertions(+), 12 deletions(-)
>>
>> --
>> 2.44.0
>>
>>

Re: [PATCH v4 0/3] btrfs: more explaination on extent_map members

[Filipe Manana]

On Fri, Apr 12, 2024 at 11:45 AM Qu Wenruo <wqu@suse.com> wrote:
>
>
>
> 在 2024/4/12 20:01, Filipe Manana 写道:
> > On Thu, Apr 11, 2024 at 11:48 PM Qu Wenruo <wqu@suse.com> wrote:
> >>
> >> [REPO]
> >> https://github.com/adam900710/linux.git/ em_cleanup
> >>
> >> The first 3 patches only. As later patches are the huge rename part.
> >>
> >> [CHANGELOG]
> >> v4:
> >> - Add extra comments for the extent map merging
> >>    Since extent maps can be merged, the members are only matching
> >>    on-disk file extent items before merging.
> >>    This also means users of extent_map should not rely on it for
> >>    a reliable file extent item member.
> >>    (That's also why we use ordered_extent not extent_maps, to update
> >>     file extent items)
> >
> > So this isn't true.
> > We use information from the ordered extent to create the file extent
> > item, when finishing the ordered extent, because the ordered extent is
> > immediately accessible.
> > We could also use the extent map, we would just have to do a tree search for it.
> > The extent map created when starting delalloc or at the beginning of a
> > DIO write is pinned and in the list of modified extents, so it can't
> > be merged and it represents exactly one file extent item - the one
> > going to be inserted the inode's subvolume tree when finishing the
> > ordered extent.
>
> If you're doing COW writes, extent map is fine, as it's always new
> extents and they are all pinned thus won't be merged.
>
> For pure NOCOW, it's also fine because there would be no extent map
> inserted, nor updating file extent items.
>
> But for PREALLOC writes (NOCOW into preallocated writes), it's not the
> case as the target extent maps can be merged already.

Yes, but prealloc is an exception, as we don't insert a new file
extent item, just update an existing one or split and change the type
from prealloc to regular, etc.
My comment was obviously for COW, which is the only case that needs to
insert a new file extent item.

>
> Thus that's why for NOCOW we go with a subvolume tree search to grab the
> correct original disk_bytenr/disk_num_bytes/offset to go.
>
> If you use extent map instead of search subvolume tree, the following
> layout can screw up everything:
>
> Filepos 0:
> disk_bytenr X, disk_num_bytes 64K
> num_bytes 64K, offset 0, ram_bytes 64K
>
> Filepos 64K
> disk_bytenr X+64K, disk_num_bytes 64K
> num_bytes 64K, offset 0, ram_bytes 64K.
>
> The extent map would be (after merge):
>
> filepos 0, len 128K block_start X, block len 128K, orig_start 0.
>
> Even with my new disk_bytenr it would be no difference, the disk_bytenr
> would still be X, and disk_num_bytes would be merged 128K.
>
> So the merging behavior is affecting PREALLOC writes, and we're already
> avoiding the only pitfall.
>
> Thanks,
> Qu
>
> >
> >>
> >> v3:
> >> - Rebased to latest for-next branch
> >> - Further comments polishment
> >> - Coding style update to follow the guideline
> >>
> >> v2:
> >> - Add Filipe's cleanup on mod_start/mod_len
> >>    These two members are no longer utilized, saving me quite some time on
> >>    digging into their usage.
> >>
> >> - Update the comments of the extent_map structure
> >>    To make them more readable and less confusing.
> >>
> >> - Further cleanup for inline extent_map reading
> >>
> >> - A new patch to do extra sanity checks for create_io_em()
> >>    Firstly pure NOCOW writes should not call create_io_em(), secondly
> >>    with the new knowledge of extent_map, it's easier to do extra sanity
> >>    checks for the already pretty long parameter list.
> >>
> >> Btrfs uses extent_map to represent a in-memory file extent.
> >>
> >> There are severam members that are 1:1 mappe in on-disk file extent
> >> items and extent maps:
> >>
> >> - extent_map::start     ==      key.offset
> >> - extent_map::len       ==      file_extent_num_bytes
> >> - extent_map::ram_bytes ==      file_extent_ram_bytes
> >>
> >> But that's all, the remaining are pretty different:
> >>
> >> - Use block_start to indicate holes/inline extents
> >>    Meanwhile btrfs on-disk file extent items go with a dedicated type for
> >>    inline extents, and disk_bytenr 0 for holes.
> >>
> >> - Weird block_start/orig_block_len/orig_start
> >>    In theory we can directly go with the same file_extent_disk_bytenr,
> >>    file_extent_disk_num_bytes and file_extent_offset to calculate the
> >>    remaining members (block_start/orig_start/orig_block_len/block_len).
> >>
> >>    But for whatever reason, we didn't go that path and have a hell of
> >>    weird and inconsistent calculation for them.
> >>
> >> Qu Wenruo (3):
> >>    btrfs: add extra comments on extent_map members
> >>    btrfs: simplify the inline extent map creation
> >>    btrfs: add extra sanity checks for create_io_em()
> >>
> >>   fs/btrfs/extent_map.h | 58 ++++++++++++++++++++++++++++++++++++++++++-
> >>   fs/btrfs/file-item.c  | 20 +++++++--------
> >>   fs/btrfs/inode.c      | 40 ++++++++++++++++++++++++++++-
> >>   3 files changed, 106 insertions(+), 12 deletions(-)
> >>
> >> --
> >> 2.44.0
> >>
> >>