[PATCH] btrfs: Fix bad comment on disk_bytenr of btrfs_file_extent_item

[PATCH] btrfs: Fix bad comment on disk_bytenr of btrfs_file_extent_item

[Qu Wenruo]

All btrfs_file_extent_item members don't take checksum size into
consideration.

This bad comment looks like from early days where inlined data checksum
(checksum is stored along with data) is being considered.
But the reality is, we never support inlined data checksum since btrfs
is mainlined.

Remove this dead comment, add a new comment explaining how data checksum is
stored, and remove the unnecessary data csum reference.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 include/uapi/linux/btrfs_tree.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
index 8e322e2c7e78..bfe6f38031a3 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -776,15 +776,16 @@ struct btrfs_file_extent_item {
 	__u8 type;
 
 	/*
-	 * disk space consumed by the extent, checksum blocks are included
-	 * in these numbers
+	 * disk space consumed by the data extent.
+	 * Checksum is stored in csum tree, thus no bytenr/length takes
+	 * csum into consideration.
 	 *
 	 * 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)
+	 * the logical offset in file blocks
 	 * 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
@@ -792,8 +793,8 @@ struct btrfs_file_extent_item {
 	 */
 	__le64 offset;
 	/*
-	 * the logical number of file blocks (no csums included).  This
-	 * always reflects the size uncompressed and without encoding.
+	 * the logical number of file blocks.  This always reflects the size
+	 * uncompressed and without encoding.
 	 */
 	__le64 num_bytes;
 
-- 
2.24.1

Re: [PATCH] btrfs: Fix bad comment on disk_bytenr of btrfs_file_extent_item

[Su Yue]

On 2019/12/17 2:48 PM, Qu Wenruo wrote:
> All btrfs_file_extent_item members don't take checksum size into
> consideration.
>
> This bad comment looks like from early days where inlined data checksum
> (checksum is stored along with data) is being considered.
> But the reality is, we never support inlined data checksum since btrfs
> is mainlined.
>
> Remove this dead comment, add a new comment explaining how data checksum is
> stored, and remove the unnecessary data csum reference.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

LGTM.

Reviewed-by: Su Yue <Damenly_Su@gmx.com>
> ---
>   include/uapi/linux/btrfs_tree.h | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
> index 8e322e2c7e78..bfe6f38031a3 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -776,15 +776,16 @@ struct btrfs_file_extent_item {
>   	__u8 type;
>
>   	/*
> -	 * disk space consumed by the extent, checksum blocks are included
> -	 * in these numbers
> +	 * disk space consumed by the data extent.
> +	 * Checksum is stored in csum tree, thus no bytenr/length takes
> +	 * csum into consideration.
>   	 *
>   	 * 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)
> +	 * the logical offset in file blocks
>   	 * 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
> @@ -792,8 +793,8 @@ struct btrfs_file_extent_item {
>   	 */
>   	__le64 offset;
>   	/*
> -	 * the logical number of file blocks (no csums included).  This
> -	 * always reflects the size uncompressed and without encoding.
> +	 * the logical number of file blocks.  This always reflects the size
> +	 * uncompressed and without encoding.
>   	 */
>   	__le64 num_bytes;
>
>

Re: [PATCH] btrfs: Fix bad comment on disk_bytenr of btrfs_file_extent_item

[Nikolay Borisov]

On 17.12.19 г. 8:48 ч., Qu Wenruo wrote:
> All btrfs_file_extent_item members don't take checksum size into
> consideration.
> 
> This bad comment looks like from early days where inlined data checksum
> (checksum is stored along with data) is being considered.
> But the reality is, we never support inlined data checksum since btrfs
> is mainlined.
> 
> Remove this dead comment, add a new comment explaining how data checksum is
> stored, and remove the unnecessary data csum reference.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  include/uapi/linux/btrfs_tree.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
> index 8e322e2c7e78..bfe6f38031a3 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -776,15 +776,16 @@ struct btrfs_file_extent_item {
>  	__u8 type;
>  
>  	/*
> -	 * disk space consumed by the extent, checksum blocks are included
> -	 * in these numbers
> +	 * disk space consumed by the data extent.
> +	 * Checksum is stored in csum tree, thus no bytenr/length takes
> +	 * csum into consideration.

This wording is awkward. Simply say that checksum blocks are excluded

>  	 *
>  	 * 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)
> +	 * the logical offset in file blocks
>  	 * 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
> @@ -792,8 +793,8 @@ struct btrfs_file_extent_item {
>  	 */
>  	__le64 offset;
>  	/*
> -	 * the logical number of file blocks (no csums included).  This
> -	 * always reflects the size uncompressed and without encoding.
> +	 * the logical number of file blocks.  This always reflects the size
> +	 * uncompressed and without encoding.

I don't think this is better. Because one has to actually read the
comment about disk_bytenr to understand whether the following members
include exclude checksums. I prefer to explicitly state whether each
member includes/excludes the checksums. As it stands, a sensible
correction is to simply state (checksums excluded).

>  	 */
>  	__le64 num_bytes;
>  
>