Re: [PATCH 2/2] btrfs: add ioctl for directly writing compressed data

[Omar Sandoval <osandov@osandov.com>]

On Thu, Sep 05, 2019 at 12:10:12PM +1000, Dave Chinner wrote:
> On Wed, Sep 04, 2019 at 12:13:26PM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > This adds an API for writing compressed data directly to the filesystem.
> > The use case that I have in mind is send/receive: currently, when
> > sending data from one compressed filesystem to another, the sending side
> > decompresses the data and the receiving side recompresses it before
> > writing it out. This is wasteful and can be avoided if we can just send
> > and write compressed extents. The send part will be implemented in a
> > separate series, as this ioctl can stand alone.
> > 
> > The interface is essentially pwrite(2) with some extra information:
> > 
> > - The input buffer contains the compressed data.
> > - Both the compressed and decompressed sizes of the data are given.
> > - The compression type (zlib, lzo, or zstd) is given.

Hi, Dave,

> So why can't you do this with pwritev2()? Heaps of flags, and
> use a second iovec to hold the decompressed size of the previous
> iovec. i.e.
> 
> 	iov[0].iov_base = compressed_data;
> 	iov[0].iov_len = compressed_size;
> 	iov[1].iov_base = NULL;
> 	iov[1].iov_len = uncompressed_size;
> 	pwritev2(fd, iov, 2, offset, RWF_COMPRESSED_ZLIB);
> 
> And you don't need to reinvent pwritev() with some whacky ioctl that
> is bound to be completely screwed up is ways not noticed until
> someone else tries to use it...

This is a good suggestion, thanks. I hadn't considered (ab?)using iovecs
in this way.

One modification I'd make would be to put the encoding into the second
iovec and use a single RWF_ENCODED flag so that we don't have to keep
stealing from RWF_* every time we add a new compression
algorithm/encryption type/whatever:

 	iov[0].iov_base = compressed_data;
 	iov[0].iov_len = compressed_size;
 	iov[1].iov_base = (void *)IOV_ENCODING_ZLIB;
 	iov[1].iov_len = uncompressed_size;
 	pwritev2(fd, iov, 2, offset, RWF_ENCODED);

Making every other iovec a metadata iovec in this way would be a major
pain to plumb through the iov_iter and VFS code, though. Instead, we
could put the metadata in iov[0] and the encoded data in iov[1..iovcnt -
1]:

	iov[0].iov_base = (void *)IOV_ENCODING_ZLIB;
	iov[0].iov_len = unencoded_len;
	iov[1].iov_base = encoded_data1;
	iov[1].iov_len = encoded_size1;
	iov[2].iov_base = encoded_data2;
	iov[2].iov_len = encoded_size2;
 	pwritev2(fd, iov, 3, offset, RWF_ENCODED);

In my opinion, these are both reasonable interfaces. The former allows
the user to write multiple encoded "extents" at once, while the latter
allows writing a single encoded extent from scattered buffers. The
latter is much simpler to implement ;) Thoughts?

> I'd also suggest atht if we are going to be able to write compressed
> data directly, then we should be able to read them as well directly
> via preadv2()....
> 
> > The interface is general enough that it can be extended to encrypted or
> > otherwise encoded extents in the future. A more detailed description,
> > including restrictions and edge cases, is included in
> > include/uapi/linux/btrfs.h.
> 
> No thanks, that bit us on the arse -hard- with the clone interfaces
> we lifted to the VFS from btrfs. Let's do it through the existing IO
> paths and write a bunch of fstests to exercise it and verify the
> interface's utility and the filesystem implementation correctness
> before anything is merged.
> 
> > The implementation is similar to direct I/O: we have to flush any
> > ordered extents, invalidate the page cache, and do the io
> > tree/delalloc/extent map/ordered extent dance.
> 
> Which, to me, says that this should be a small bit of extra code
> in the direct IO path that skips the compression/decompression code
> and sets a few extra flags in the iocb that is passed down to the
> direct IO code.
> 
> We don't need a whole new IO path just to skip a data transformation
> step in the direct IO path....

Eh, at least for Btrfs, it's much hairier to retrofit this onto the mess
of callbacks that is __blockdev_direct_IO() than it is to have a
separate path. But that doesn't affect the interface, and other
filesystems may be able to share more code with the direct IO path.