Re: [PATCH v11 01/10] btrfs-progs: receive: support v2 send stream larger tlv_len

Nikolay Borisov
2021-10-20T13:49:42Z

On 1.09.21 г. 20:01, Omar Sandoval wrote:
> From: Boris Burkov <borisb@fb.com>
>
> An encoded extent can be up to 128K in length, which exceeds the largest
> value expressible by the current send stream format's 16 bit tlv_len
> field. Since encoded writes cannot be split into multiple writes by
> btrfs send, the send stream format must change to accommodate encoded
> writes.
>
> Supporting this changed format requires retooling how we store the
> commands we have processed. Since we can no longer use btrfs_tlv_header
> to describe every attribute, we define a new struct btrfs_send_attribute
> which has a 32 bit length field, and use that to store the attribute
> information needed for receive processing. This is transparent to users
> of the various TLV_GET macros.
>
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> common/send-stream.c | 34 +++++++++++++++++++++++++---------
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/common/send-stream.c b/common/send-stream.c
> index a0c52f79..cd5aa311 100644
> --- a/common/send-stream.c
> +++ b/common/send-stream.c
> @@ -24,13 +24,23 @@
> #include "crypto/crc32c.h"
> #include "common/utils.h"
>
> +struct btrfs_send_attribute {
> + u16 tlv_type;
> + /*
> + * Note: in btrfs_tlv_header, this is __le16, but we need 32 bits for
> + * attributes with file data as of version 2 of the send stream format
> + */
> + u32 tlv_len;
> + char *data;
> +};
> +
> struct btrfs_send_stream {
> char read_buf[BTRFS_SEND_BUF_SIZE];
> int fd;
>
> int cmd;
> struct btrfs_cmd_header *cmd_hdr;
> - struct btrfs_tlv_header *cmd_attrs[BTRFS_SEND_A_MAX + 1];
> + struct btrfs_send_attribute cmd_attrs[BTRFS_SEND_A_MAX + 1];

This is subtle and it took me a couple of minutes to get it at first.
Currently cmds_attrs holds an array of pointers into the command buffer,
with every pointer being the beginning of the tlv_header, whilst with
your change cmd_attr now holds actual btrfs_send_attribute structures
(52 bytes vs sizeof(uintptr_t) bytes before). So this increases the
overall size of btrfs_send_stream because with your version of the code
you parse the type/length fields and store them directly in the send
attribute structure at command parse time rather than just referring to
the raw command buffer during read_cmd and referring to them during
attribute parsing.

This might seem superficial but this kind of change should really be
mentioned explicitly in the changelog to better prepare reviewers what
to expect.

OTOH the code LGTM and actually now it seems less tricky than before so:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

David if you deem it necessary adjust the commit message appropriately.