Re: [PATCH v11 13/14] btrfs: send: send compressed extents with encoded writes

[Nikolay Borisov <nborisov@suse.com>]

On 1.09.21 г. 20:01, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Now that all of the pieces are in place, we can use the ENCODED_WRITE
> command to send compressed extents when appropriate.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Overall looks sane but consider some of the nits below.


<snip>

> +static int send_encoded_extent(struct send_ctx *sctx, struct btrfs_path *path,
> +			       u64 offset, u64 len)
> +{
> +	struct btrfs_root *root = sctx->send_root;
> +	struct btrfs_fs_info *fs_info = root->fs_info;
> +	struct inode *inode;
> +	struct fs_path *p;
> +	struct extent_buffer *leaf = path->nodes[0];
> +	struct btrfs_key key;
> +	struct btrfs_file_extent_item *ei;
> +	u64 block_start;
> +	u64 block_len;
> +	u32 data_offset;
> +	struct btrfs_cmd_header *hdr;
> +	u32 crc;
> +	int ret;
> +
> +	inode = btrfs_iget(fs_info->sb, sctx->cur_ino, root);
> +	if (IS_ERR(inode))
> +		return PTR_ERR(inode);
> +
> +	p = fs_path_alloc();
> +	if (!p) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = begin_cmd(sctx, BTRFS_SEND_C_ENCODED_WRITE);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = get_cur_path(sctx, sctx->cur_ino, sctx->cur_inode_gen, p);
> +	if (ret < 0)
> +		goto out;
> +
> +	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
> +	ei = btrfs_item_ptr(leaf, path->slots[0],
> +			    struct btrfs_file_extent_item);
> +	block_start = btrfs_file_extent_disk_bytenr(leaf, ei);

block_start is somewhat ambiguous here, this is just the disk bytenr of
the extent.

> +	block_len = btrfs_file_extent_disk_num_bytes(leaf, ei);

Why is this called block_len when it's just the size in bytes on-disk?

> +
> +	TLV_PUT_PATH(sctx, BTRFS_SEND_A_PATH, p);
> +	TLV_PUT_U64(sctx, BTRFS_SEND_A_FILE_OFFSET, offset);
> +	TLV_PUT_U64(sctx, BTRFS_SEND_A_UNENCODED_FILE_LEN,
> +		    min(key.offset + btrfs_file_extent_num_bytes(leaf, ei) - offset,
> +			len));
> +	TLV_PUT_U64(sctx, BTRFS_SEND_A_UNENCODED_LEN,
> +		    btrfs_file_extent_ram_bytes(leaf, ei));
> +	TLV_PUT_U64(sctx, BTRFS_SEND_A_UNENCODED_OFFSET,
> +		    offset - key.offset + btrfs_file_extent_offset(leaf, ei));
> +	ret = btrfs_encoded_io_compression_from_extent(
> +				btrfs_file_extent_compression(leaf, ei));
> +	if (ret < 0)
> +		goto out;
> +	TLV_PUT_U32(sctx, BTRFS_SEND_A_COMPRESSION, ret);
> +	TLV_PUT_U32(sctx, BTRFS_SEND_A_ENCRYPTION, 0);
> +
> +	ret = put_data_header(sctx, block_len);
> +	if (ret < 0)
> +		goto out;
> +
> +	data_offset = ALIGN(sctx->send_size, PAGE_SIZE);

nit: The whole data_offset warrants a comment here, since send_buf is
now mapped from send_buf_pages, so all the TLV you've put before are
actually stored in the beginning of send_buf_pages, so by doing the
above you ensure the data write begins on a clean page boundary ...

> +	if (data_offset > sctx->send_max_size ||
> +	    sctx->send_max_size - data_offset < block_len) {
> +		ret = -EOVERFLOW;
> +		goto out;
> +	}
> +
> +	ret = btrfs_encoded_read_regular_fill_pages(inode, block_start,
> +						    block_len,
> +						    sctx->send_buf_pages +
> +						    (data_offset >> PAGE_SHIFT));
> +	if (ret)
> +		goto out;
> +
> +	hdr = (struct btrfs_cmd_header *)sctx->send_buf;
> +	hdr->len = cpu_to_le32(sctx->send_size + block_len - sizeof(*hdr));
> +	hdr->crc = 0;
> +	crc = btrfs_crc32c(0, sctx->send_buf, sctx->send_size);
> +	crc = btrfs_crc32c(crc, sctx->send_buf + data_offset, block_len);

... and because of that you can't simply use send_cmd ;(

> +	hdr->crc = cpu_to_le32(crc);
> +
> +	ret = write_buf(sctx->send_filp, sctx->send_buf, sctx->send_size,
> +			&sctx->send_off);
> +	if (!ret) {
> +		ret = write_buf(sctx->send_filp, sctx->send_buf + data_offset,
> +				block_len, &sctx->send_off);
> +	}
> +	sctx->total_send_size += sctx->send_size + block_len;
> +	sctx->cmd_send_size[le16_to_cpu(hdr->cmd)] +=
> +		sctx->send_size + block_len;
> +	sctx->send_size = 0;
> +
> +tlv_put_failure:
> +out:
> +	fs_path_free(p);
> +	iput(inode);
> +	return ret;
> +}
> +
> +static int send_extent_data(struct send_ctx *sctx, struct btrfs_path *path,
> +			    const u64 offset, const u64 len)

nit: Instead of sending around a btrfs_path struct around and
"polluting" callees to deal with the oddities of our btree interface i.e
btrfs_item_ptr et al. Why not refactor the code so that when we know we
are about to send an extent data simply initialize some struct
extent_info with all the necessary data items: extent type, compression
type, based on the extent type properly initialize a size attribute etc
and pass that. Right now you have send_extent_data fiddling with
path->nodes[0], then based on that you either call
send_encoded_inline_extent or send_encoded_extent, instead pass
extent_info to send_extent_data/clone_range and be done with it.


<snip>