On 1.09.21 г. 20:01, Omar Sandoval wrote:
From: Omar Sandoval <redacted>
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 <redacted>
Overall looks sane but consider some of the nits below.
<snip>
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+
+ 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 ...
quoted hunk ↗ jump to hunk
+ 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 ;(
quoted hunk ↗ jump to hunk
+ 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>