On Wed, Oct 20, 2021 at 05:35:42PM +0300, Nikolay Borisov wrote:
>
>
> On 20.10.21 г. 17:09, Nikolay Borisov wrote:
> >
> >
> > On 1.09.21 г. 20:01, Omar Sandoval wrote:
> >> From: Boris Burkov <boris@xxxxxx>
> >>
> >> In send stream v2, write commands can now be an arbitrary size. For that
>
> nit: Actually can't commands really be up-to BTRFS_MAX_COMPRESSED + 16K
> really or are we going to leave this as an implementation detail? I'm
> fine either way but looking at the changelog of patch 12 in the kernel
> series doesn't really mention of arbitrary size, instead it explicitly
> is talking about sending the max compressed extent size (128K) + some
> space for metadata (the 16K above).
Patch 10 mentions it in the changelog: "It also documents two changes to the
send stream format in v2: the receiver shouldn't assume a maximum command size,
and the DATA attribute is encoded differently to allow for writes larger than
64k".
And in send.h:
-#define BTRFS_SEND_BUF_SIZE SZ_64K
+/*
+ * In send stream v1, no command is larger than 64k. In send stream v2, no limit
+ * should be assumed.
+ */
+#define BTRFS_SEND_BUF_SIZE_V1 SZ_64K
You're correct that right now the limit is BTRFS_MAX_COMPRESSED + 16k,
but I think it's better if userspace doesn't make any assumptions about
that in case we want to send larger commands in the future.
> >> reason, we can no longer allocate a fixed array in sctx for read_cmd.
> >> Instead, read_cmd dynamically allocates sctx->read_buf. To avoid
> >> needless reallocations, we reuse read_buf between read_cmd calls by also
> >> keeping track of the size of the allocated buffer in sctx->read_buf_sz.
> >>
> >> We do the first allocation of the old default size at the start of
> >> processing the stream, and we only reallocate if we encounter a command
> >> that needs a larger buffer.
> >>
> >> Signed-off-by: Boris Burkov <boris@xxxxxx>
> >> ---
> >> common/send-stream.c | 55 ++++++++++++++++++++++++++++----------------
> >> send.h | 2 +-
> >> 2 files changed, 36 insertions(+), 21 deletions(-)
> >>
> >
> > <snip>
> >
> >> @@ -124,18 +125,22 @@ static int read_cmd(struct btrfs_send_stream *sctx)
> >> goto out;
> >> }
> >>
> >> - sctx->cmd_hdr = (struct btrfs_cmd_header *)sctx->read_buf;
> >> - cmd = le16_to_cpu(sctx->cmd_hdr->cmd);
> >> - cmd_len = le32_to_cpu(sctx->cmd_hdr->len);
> >> -
> >> - if (cmd_len + sizeof(*sctx->cmd_hdr) >= sizeof(sctx->read_buf)) {
> >> - ret = -EINVAL;
> >> - error("command length %u too big for buffer %zu",
> >> - cmd_len, sizeof(sctx->read_buf));
> >> - goto out;
> >> + cmd_hdr = (struct btrfs_cmd_header *)sctx->read_buf;
> >> + cmd_len = le32_to_cpu(cmd_hdr->len);
> >> + cmd = le16_to_cpu(cmd_hdr->cmd);
> >> + buf_len = sizeof(*cmd_hdr) + cmd_len;
> >> + if (sctx->read_buf_sz < buf_len) {
> >> + sctx->read_buf = realloc(sctx->read_buf, buf_len);
> >> + if (!sctx->read_buf) {
> >
> > nit: This is prone to a memory leak, because according to
> > https://en.cppreference.com/w/c/memory/realloc
> >
> > If there is not enough memory, the old memory block is not freed and
> > null pointer is returned.
> >
> >
> > This means if realloc fails it will overwrite sctx->read_buf with NULL,
> > yet the old memory won't be freed which will cause a memory leak. It can
> > be argued that's not critical since we'll very quickly terminate the
> > program afterwards but still.
Good catch, I'll fix that.
