Re: [PATCH] btrfs: fix comments in definition of struct btrfs_file_extent_item

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/10/3 02:14, Mark Harmstone 写道:
> The comments in the definition of struct btrfs_file_extent_item were
> written while the FS was still in flux, and are no longer accurate.
>
> The range [disk_bytenr, disk_num_bytes) is the same as the extent in the
> extent tree. There's no difference here between csummed and non-csummed
> extents, as the comments were implying. And the fields offset and
> num_bytes are in bytes, not file blocks.
>
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> ---
>   include/uapi/linux/btrfs_tree.h | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
> index fc29d273845d..5df54a11c74c 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -1094,24 +1094,23 @@ struct btrfs_file_extent_item {
>   	__u8 type;
>
>   	/*
> -	 * disk space consumed by the extent, checksum blocks are included
> -	 * in these numbers
> +	 * The address and size of the referenced extent.  These should exactly
> +	 * match an entry in the extent tree.

Recently I'm also helping Han Yuwei to understand all the ondisk format.
Maybe he can provide a better advice from a new respective.

And he is definitely not happy with the docs and comments on those
structures.

>   	 *
>   	 * At this offset in the structure, the inline extent data start.
>   	 */
>   	__le64 disk_bytenr;
>   	__le64 disk_num_bytes;
>   	/*
> -	 * the logical offset in file blocks (no csums)
> -	 * this extent record is for.  This allows a file extent to point
> -	 * into the middle of an existing extent on disk, sharing it
> -	 * between two snapshots (useful if some bytes in the middle of the
> -	 * extent have changed
> +	 * The logical offset in bytes this extent record is for.
> +	 * This allows a file extent to point into the middle of an existing
> +	 * extent on disk, sharing it between two snapshots (useful if some
> +	 * bytes in the middle of the extent have changed)

Maybe you want to add the offset is for the uncompressed data.

Another thing is, maybe we want to have a more consistent wording.

The word "extent record" may be a little confusing, I guess you mean
"file extent".
Since the structure is called "btrfs_file_extent_item", we may want to
unify to "file extent" when referring to the file extent, and "data
extent" to refer to the data extent.

Thanks,
Qu
>   	 */
>   	__le64 offset;
>   	/*
> -	 * the logical number of file blocks (no csums included).  This
> -	 * always reflects the size uncompressed and without encoding.
> +	 * The logical number of bytes.  This always reflects the size
> +	 * uncompressed and without encoding.
>   	 */
>   	__le64 num_bytes;
>