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

[Nikolay Borisov <nborisov@suse.com>]

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