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

[Omar Sandoval <osandov@osandov.com>]

On Mon, Oct 18, 2021 at 02:59:08PM +0300, Nikolay Borisov wrote:
> 
> 
> 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?

I copied this naming from extent_map since btrfs_encoded_read() was the
reference for this code, but I'll change the naming here.

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

Yup, I'll add a comment.

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

I don't like this for a few reasons:

* An extra "struct extent_info" layer of abstraction would just be extra
  cognitive overhead. I hate having to trace back where the fields in
  some struct came from when it's information that's readily available
  in more well-known data structures.
* send_encoded_inline_extent() (called by send_extent_data()) needs the
  btrfs_path in order to get the inline data anyways.
* clone_range() also already deals with btrfs_paths, so it's not new.