Re: [PATCH v14 4/7] btrfs: send: write larger chunks when using stream v2

[Sweet Tea Dorminy <sweettea-kernel@dorminy.me>]

> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 1f141de3a7d6..02053fff80ca 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -82,6 +82,7 @@ struct send_ctx {
>   	char *send_buf;
>   	u32 send_size;
>   	u32 send_max_size;
> +	bool put_data;
put_data's use seems to be about making sure put_data_header() isn't 
called more than once, which is not super obvious to me from the name; 
perhaps one of 'data_header_{set,setup,initialized}' might make it 
clearer? Or if it's actually about put_file_data, maybe moving the 
assertion there would make that clearer?
>   static int put_data_header(struct send_ctx *sctx, u32 len)
>   {
> -	struct btrfs_tlv_header *hdr;
> +	if (WARN_ON_ONCE(sctx->put_data))
> +		return -EINVAL;
> +	sctx->put_data = true;
> +	if (sctx->proto >= 2) {
> +		/*
> +		 * In v2, the data attribute header doesn't include a length; it
> +		 * is implicitly to the end of the command.
> +		 */
> +		if (sctx->send_max_size - sctx->send_size < 2 + len)
> +			return -EOVERFLOW;
> +		put_unaligned_le16(BTRFS_SEND_A_DATA,
> +				   sctx->send_buf + sctx->send_size);
> +		sctx->send_size += 2;
> +	} else {
> +		struct btrfs_tlv_header *hdr;
>   
> -	if (sctx->send_max_size - sctx->send_size < sizeof(*hdr) + len)
> -		return -EOVERFLOW;
> -	hdr = (struct btrfs_tlv_header *)(sctx->send_buf + sctx->send_size);
> -	put_unaligned_le16(BTRFS_SEND_A_DATA, &hdr->tlv_type);
> -	put_unaligned_le16(len, &hdr->tlv_len);
> -	sctx->send_size += sizeof(*hdr);
> +		if (sctx->send_max_size - sctx->send_size < sizeof(*hdr) + len)
> +			return -EOVERFLOW;
> +		hdr = (struct btrfs_tlv_header *)(sctx->send_buf +
> +						  sctx->send_size);
> +		put_unaligned_le16(BTRFS_SEND_A_DATA, &hdr->tlv_type);
> +		put_unaligned_le16(len, &hdr->tlv_len);
> +		sctx->send_size += sizeof(*hdr);
> +	}
>   	return 0;
>   }

I wish the 2s were named, and that there were more commonality between 
the two branches... Might I propose this alternative? It doesn't check 
the length's suitability until after adding the two fields, but I don't 
think anything bad happens from delaying the check.

static int put_data_header(struct send_ctx *sctx, u32 len)
{
         struct btrfs_tlv_header *hdr =
                 (struct btrfs_tlv_header *)(sctx->send_buf + sctx->send_size);

         if (WARN_ON_ONCE(sctx->put_data))
                 return -EINVAL;
         sctx->put_data = true;

         put_unaligned_le16(BTRFS_SEND_A_DATA, &hdr->tlv_type);
         sctx->send_size += sizeof(hdr->tlv_type);
                                                                                                                        
         /*
          * In v2+, the data attribute header doesn't include a length; it is
          * implicitly to the end of the command.
          */
         if (sctx->proto == 1) {
                 put_unaligned_le16(len, &hdr->tlv_len);
                 sctx->send_size += sizeof(hdr->tlv_len);
         }

         if (sctx->send_max_size - sctx->send_size < len)
                 return -EOVERFLOW;

         return 0;
}