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

[Omar Sandoval <osandov@osandov.com>]

On Thu, Mar 24, 2022 at 01:52:53PM -0400, Sweet Tea Dorminy wrote:
> 
> > 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?

The intention is to prevent adding another attribute after a data
attribute, since that's impossible with v2. Notice that it's also
checked in tlv_put(). So "put_data" means "was a data attribute already
added to this command?"; it's not specifically about the data header or
data itself. I'll add a comment to that effect.

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

We need to check that writing the header itself won't write past the end
of send_buf, so it'd have to look more like:

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;

	if (sctx->send_max_size - sctx->send_size < sizeof(hdr->tlv_type))
		return -EOVERFLOW;
	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 < 2) {
		if (sctx->send_max_size - sctx->send_size <
		    sizeof(hdr->tlv_len))
			return -EOVERFLOW;
		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;
}

Now we're checking for overflow in three places, shrug.

One thing that I like about the two separate cases is that it makes it
more clear that v2+ doesn't actually have a btrfs_tlv_header; it's just
a single __le16. If it's alright with you, I'll stick with my original
version, but I will replaced the hard-coded 2s with sizeof(__le16).