Re: [PATCH v11 06/10] btrfs-progs: receive: encoded_write fallback to explicit decode and write

[Omar Sandoval <osandov@osandov.com>]

On Thu, Oct 21, 2021 at 04:55:19PM +0300, Nikolay Borisov wrote:
> 
> 
> On 1.09.21 г. 20:01, Omar Sandoval wrote:
> > From: Boris Burkov <boris@bur.io>
> > 
> > +static int decompress_lzo(const char *encoded_data, u64 encoded_len,
> > +			  char *unencoded_data, u64 unencoded_len,
> > +			  unsigned int page_size)
> > +{
> > +	uint32_t total_len;
> > +	size_t in_pos, out_pos;
> > +
> > +	if (encoded_len < 4) {
> > +		error("lzo header is truncated");
> > +		return -EIO;
> > +	}
> > +	memcpy(&total_len, encoded_data, 4);
> > +	total_len = le32toh(total_len);
> > +	if (total_len > encoded_len) {
> > +		error("lzo header is invalid");
> > +		return -EIO;
> > +	}
> > +
> > +	in_pos = 4;
> > +	out_pos = 0;
> > +	while (in_pos < total_len && out_pos < unencoded_len) {
> > +		size_t page_remaining;
> > +		uint32_t src_len;
> > +		lzo_uint dst_len;
> > +		int ret;
> > +
> > +		page_remaining = -in_pos % page_size;
> 
> Why the -in_pos?

in_pos is our position in the encoded data. This calculates how many
bytes are remaining in the page that in_pos current points to.

> > +		if (page_remaining < 4) {
> > +			if (total_len - in_pos <= page_remaining)
> > +				break;
> > +			in_pos += page_remaining;
> > +		}
> > +
> > +		if (total_len - in_pos < 4) {
> > +			error("lzo segment header is truncated");
> > +			return -EIO;
> > +		}
> > +
> > +		memcpy(&src_len, encoded_data + in_pos, 4);
> > +		src_len = le32toh(src_len);
> > +		in_pos += 4;
> > +		if (src_len > total_len - in_pos) {
> > +			error("lzo segment header is invalid");
> > +			return -EIO;
> > +		}
> > +
> > +		dst_len = page_size;
> > +		ret = lzo1x_decompress_safe((void *)(encoded_data + in_pos),
> > +					    src_len,
> > +					    (void *)(unencoded_data + out_pos),
> > +					    &dst_len, NULL);
> > +		if (ret != LZO_E_OK) {
> > +			error("lzo1x_decompress_safe failed: %d", ret);
> > +			return -EIO;
> > +		}
> > +
> > +		in_pos += src_len;
> > +		out_pos += dst_len;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int decompress_and_write(struct btrfs_receive *rctx,
> > +				const char *encoded_data, u64 offset,
> > +				u64 encoded_len, u64 unencoded_file_len,
> > +				u64 unencoded_len, u64 unencoded_offset,
> > +				u32 compression)
> > +{
> > +	int ret = 0;
> > +	size_t pos;
> > +	ssize_t w;
> > +	char *unencoded_data;
> > +	int page_shift;
> > +
> > +	unencoded_data = calloc(unencoded_len, 1);
> > +	if (!unencoded_data) {
> > +		error("allocating space for unencoded data failed: %m");
> > +		return -errno;
> > +	}
> > +
> > +	switch (compression) {
> > +	case BTRFS_ENCODED_IO_COMPRESSION_ZLIB:
> > +		ret = decompress_zlib(rctx, encoded_data, encoded_len,
> > +				      unencoded_data, unencoded_len);
> > +		if (ret)
> > +			goto out;
> > +		break;
> > +	case BTRFS_ENCODED_IO_COMPRESSION_ZSTD:
> > +		ret = decompress_zstd(rctx, encoded_data, encoded_len,
> > +				      unencoded_data, unencoded_len);
> > +		if (ret)
> > +			goto out;
> > +		break;
> > +	case BTRFS_ENCODED_IO_COMPRESSION_LZO_4K:
> > +	case BTRFS_ENCODED_IO_COMPRESSION_LZO_8K:
> > +	case BTRFS_ENCODED_IO_COMPRESSION_LZO_16K:
> > +	case BTRFS_ENCODED_IO_COMPRESSION_LZO_32K:
> > +	case BTRFS_ENCODED_IO_COMPRESSION_LZO_64K:
> > +		page_shift = compression - BTRFS_ENCODED_IO_COMPRESSION_LZO_4K + 12;
> 
> Doesn't this calculation assume page size is 4k, what about arches with
> larger page size (ppc/aarch64), shouldn't that '12' be adjusted?

This is unrelated to the machine page size. It is translating the
BTRFS_ENCODED_IO_COMPRESSION_LZO_* value to the page size used for
compressing the data:

compression | - LZO_4K | + 12 | 1 <<
=====================================
LZO_4K = 3  | 0        | 12   | 4096
LZO_8K = 4  | 1        | 13   | 8192
LZO_16K = 5 | 2        | 14   | 16384
LZO_32K = 6 | 3        | 15   | 32768
LZO_64K = 7 | 4        | 16   | 65536

I'll fix the other comments, thanks.