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

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

[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.
 	 *
 	 * 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)
 	 */
 	__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;
 
-- 
2.44.2

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

[Boris Burkov]

On Wed, Oct 02, 2024 at 05:44:45PM +0100, Mark Harmstone wrote:
> 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>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  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.
>  	 *
>  	 * 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)
>  	 */
>  	__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;
>  
> -- 
> 2.44.2
> 

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

[Qu Wenruo]

在 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;
>

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

[Yuwei Han]

在 2024/10/3 05:21, Qu Wenruo 写道:
> 
> 
> 在 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.
>
I didn't read btrfs code in kernel tree, I just read Qu's 
adam900710/btrfs-fuse for reference, so correct me if I am wrong.
If this is EXTENT_DATA(0x6c) in docs, I think the descriptions in btrfs 
doc is almost ok.
>>        *
>>        * 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;
>>
My description on these field would be this,
disk_bytenr & disk_num_bytes:
logical address and size of extent on disk. Should exactly match an 
EXTENT_ITEM.
offset & num_bytes:
offset and size of file content within the extent after decoding and 
decompressing. (removed "logical" since we don't need to do logical -> 
physical address translation)
And write an e.g as doc did.
> 
>