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

[Omar Sandoval <osandov@osandov.com>]

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@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.

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.