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