On Wed, Oct 20, 2021 at 04:49:38PM +0300, Nikolay Borisov wrote:
>
>
> On 1.09.21 г. 20:01, Omar Sandoval wrote:
> > From: Boris Burkov <borisb@xxxxxx>
> >
> > 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@xxxxxx>
> > ---
> > 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@xxxxxxxx>
>
>
> David if you deem it necessary adjust the commit message appropriately.
I clarified the second paragraph to:
Supporting this changed format requires retooling how we store the
commands we have processed. We currently store pointers to the struct
btrfs_tlv_headers in the command buffer. This is not sufficient to
represent the new BTRFS_SEND_A_DATA format. Instead, parse the attribute
headers and store them in a new struct btrfs_send_attribute which has a
32-bit length field. This is transparent to users of the various TLV_GET
macros.
