From: Omar Sandoval <redacted>
This series adds an API for reading compressed data on a filesystem
without decompressing it as well as support for writing compressed data
directly to the filesystem. As with the previous submissions, I've
included a man page patch describing the API. I have test cases
(including fsstress support) and example programs which I'll send up
[1].
The main use-case is Btrfs 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 patches implementing the send/receive support
will be sent shortly.
Patches 1-3 add the VFS support and UAPI. Patches 4 and 5 are fixes for
patches in the Btrfs misc-next branch that conflicted with this series;
they can go into misc-next or be folded into the original patches
independently. Patches 6-9 are Btrfs prep patches. Patch 10 adds Btrfs
encoded read support and patch 11 adds Btrfs encoded write support.
These patches are based on Dave Sterba's Btrfs misc-next branch [2],
which is in turn currently based on v5.10-rc4.
Changes since v5 [3]:
- Made O_CLOEXEC mandatory in conjuction with O_ALLOW_ENCODED.
- Added _BTRFS to the ENCODED_IOV_COMPRESSION names (e.g.,
ENCODED_IOV_COMPRESSION_ZSTD -> ENCODED_IOV_COMPRESSION_BTRFS_ZSTD).
- Split ENCODED_IOV_COMPRESSION_LZO compression mode into separate modes
per page size. I missed that the ill-conceived Btrfs LZO format
depends on PAGE_SIZE. Having a separate compression mode for each
supported page size at least lets us detect mismatches.
- Fixed up other minor comments from v5.
- Added reviewed-bys.
1: https://github.com/osandov/xfstests/tree/rwf-encoded
2: https://github.com/kdave/btrfs-devel/tree/misc-next
3: https://lore.kernel.org/linux-btrfs/cover.1597993855.git.osandov@osandov.com/
Omar Sandoval (11):
iov_iter: add copy_struct_from_iter()
fs: add O_ALLOW_ENCODED open flag
fs: add RWF_ENCODED for reading/writing compressed data
btrfs: fix btrfs_write_check()
btrfs: fix check_data_csum() error message for direct I/O
btrfs: don't advance offset for compressed bios in
btrfs_csum_one_bio()
btrfs: add ram_bytes and offset to btrfs_ordered_extent
btrfs: support different disk extent size for delalloc
btrfs: optionally extend i_size in cow_file_range_inline()
btrfs: implement RWF_ENCODED reads
btrfs: implement RWF_ENCODED writes
Documentation/filesystems/encoded_io.rst | 74 ++
Documentation/filesystems/index.rst | 1 +
arch/alpha/include/uapi/asm/fcntl.h | 1 +
arch/parisc/include/uapi/asm/fcntl.h | 1 +
arch/sparc/include/uapi/asm/fcntl.h | 1 +
fs/btrfs/compression.c | 12 +-
fs/btrfs/compression.h | 6 +-
fs/btrfs/ctree.h | 9 +-
fs/btrfs/delalloc-space.c | 18 +-
fs/btrfs/file-item.c | 35 +-
fs/btrfs/file.c | 73 +-
fs/btrfs/inode.c | 933 ++++++++++++++++++++---
fs/btrfs/ordered-data.c | 80 +-
fs/btrfs/ordered-data.h | 18 +-
fs/btrfs/relocation.c | 4 +-
fs/fcntl.c | 10 +-
fs/namei.c | 4 +
fs/open.c | 7 +
fs/read_write.c | 167 +++-
include/linux/fcntl.h | 2 +-
include/linux/fs.h | 11 +
include/linux/uio.h | 2 +
include/uapi/asm-generic/fcntl.h | 4 +
include/uapi/linux/fs.h | 41 +-
lib/iov_iter.c | 82 ++
25 files changed, 1384 insertions(+), 212 deletions(-)
create mode 100644 Documentation/filesystems/encoded_io.rst
--
2.29.2
@@ -221,8 +221,9 @@ On Linux, this command can change only the .BRO_ASYNC, .BRO_DIRECT, .BRO_NOATIME,+.BRO_NONBLOCK, and-.BO_NONBLOCK+.BO_ALLOW_ENCODED flags. It is not possible to change the .BRO_DSYNC
@@ -1820,6 +1821,13 @@ Attempted to clear the flag on a file that has the append-only attribute set. .TP .BEPERM+Attempted to set the+.BO_ALLOW_ENCODED+flag and the calling process did not have the+.BCAP_SYS_ADMIN+capability.+.TP+.BEPERM .Icmd was .BRF_ADD_SEALS,
@@ -437,6 +437,16 @@ was followed by a call to .BRfdatasync(2)). .IR"See NOTES below". .TP+.BO_ALLOW_ENCODED+Open the file with encoded I/O permissions;+see+.BRencoded_io(7).+.BO_CLOEXEC+must be specified in conjuction with this flag.+The caller must have the+.BCAP_SYS_ADMIN+capability.+.TP .BO_EXCL Ensure that this call creates the file: if this flag is specified in conjunction with
@@ -1082,6 +1092,14 @@ is invalid (e.g., it contains characters not permitted by the underlying filesystem). .TP .BEINVAL+.BO_ALLOW_ENCODED+was specified in+.IRflags,+but+.BO_CLOEXEC+was not specified.+.TP+.BEINVAL The final component ("basename") of .Ipathname is invalid
@@ -1238,6 +1256,11 @@ did not match the owner of the file and the caller was not privileged. The operation was prevented by a file seal; see .BRfcntl(2). .TP+.BEPERM+The+.BO_ALLOW_ENCODED+flag was specified, but the caller was not privileged.+.TP .BEROFS .Ipathname refers to a file on a read-only filesystem and write access was
@@ -264,6 +264,11 @@ the data is always appended to the end of the file. However, if the .Ioffset argument is \-1, the current file offset is updated.+.TP+.BRRWF_ENCODED" (since Linux 5.12)"+Read or write encoded (e.g., compressed) data.+See+.BRencoded_io(7). .SHRETURNVALUE On success, .BRreadv(),
@@ -283,6 +288,13 @@ than requested (see and .BRwrite(2)). .PP+If+.B+RWF_ENCODED+was specified in+.IRflags,+then the return value is the number of encoded bytes.+.PP On error, \-1 is returned, and \fIerrno\fP is set appropriately. .SHERRORS The errors are as given for
@@ -313,6 +325,64 @@ is less than zero or greater than the permitted maximum. .TP .BEOPNOTSUPP An unknown flag is specified in \fIflags\fP.+.TP+.BEOPNOTSUPP+.BRWF_ENCODED+is specified in+.Iflags+and the filesystem does not implement encoded I/O.+.TP+.BEPERM+.BRWF_ENCODED+is specified in+.Iflags+and the file was not opened with the+.BO_ALLOW_ENCODED+flag.+.PP+.BRpreadv2()+can fail for the following reasons:+.TP+.BE2BIG+.BRWF_ENCODED+is specified in+.Iflags+and+.Iiov[0]+is not large enough to return the encoding metadata.+.TP+.BENOBUFS+.BRWF_ENCODED+is specified in+.Iflags+and the buffers in+.Iiov+are not big enough to return the encoded data.+.PP+.BRpwritev2()+can fail for the following reasons:+.TP+.BE2BIG+.BRWF_ENCODED+is specified in+.Iflags+and+.Iiov[0]+contains non-zero fields+after the kernel's+.IR"sizeof(struct\ encoded_iov)".+.TP+.BEINVAL+.BRWF_ENCODED+is specified in+.Iflags+and the encoding is unknown or not supported by the filesystem.+.TP+.BEINVAL+.BRWF_ENCODED+is specified in+.Iflags+and the alignment and/or size requirements are not met. .SHVERSIONS .BRpreadv() and
@@ -0,0 +1,369 @@+.\" Copyright (c) 2020 by Omar Sandoval <osandov@fb.com>+.\"+.\" %%%LICENSE_START(VERBATIM)+.\" Permission is granted to make and distribute verbatim copies of this+.\" manual provided the copyright notice and this permission notice are+.\" preserved on all copies.+.\"+.\" Permission is granted to copy and distribute modified versions of this+.\" manual under the conditions for verbatim copying, provided that the+.\" entire resulting derived work is distributed under the terms of a+.\" permission notice identical to this one.+.\"+.\" Since the Linux kernel and libraries are constantly changing, this+.\" manual page may be incorrect or out-of-date. The author(s) assume no+.\" responsibility for errors or omissions, or for damages resulting from+.\" the use of the information contained herein. The author(s) may not+.\" have taken the same level of care in the production of this manual,+.\" which is licensed free of charge, as they might when working+.\" professionally.+.\"+.\" Formatted or processed versions of this manual, if unaccompanied by+.\" the source, must acknowledge the copyright and authors of this work.+.\" %%%LICENSE_END+.\"+.\"+.THENCODED_IO72020-11-11"Linux""Linux Programmer's Manual"+.SHNAME+encoded_io \- overview of encoded I/O+.SHDESCRIPTION+Several filesystems (e.g., Btrfs) support transparent encoding+(e.g., compression, encryption) of data on disk:+written data is encoded by the kernel before it is written to disk,+and read data is decoded before being returned to the user.+In some cases, it is useful to skip this encoding step.+For example, the user may want to read the compressed contents of a file+or write pre-compressed data directly to a file.+This is referred to as "encoded I/O".+.SSEncodedI/OAPI+Encoded I/O is specified with the+.BRWF_ENCODED+flag to+.BRpreadv2(2)+and+.BRpwritev2(2).+If+.BRWF_ENCODED+is specified, then+.Iiov[0].iov_base+points to an+.I+encoded_iov+structure, defined in+.I<linux/fs.h>+as:+.PP+.in+4n+.EX+struct encoded_iov {+ __aligned_u64 len;+ __aligned_u64 unencoded_len;+ __aligned_u64 unencoded_offset;+ __u32 compression;+ __u32 encryption;+};+.EE+.in+.PP+This may be extended in the future, so+.Iiov[0].iov_len+must be set to+.I"sizeof(struct\ encoded_iov)"+for forward/backward compatibility.+The remaining buffers contain the encoded data.+.PP+.Icompression+and+.Iencryption+are the encoding fields.+.Icompression+is+.BENCODED_IOV_COMPRESSION_NONE+(zero)+or a filesystem-specific+.BENCODED_IOV_COMPRESSION+constant;+see+.BRFilesystem\support.+.Iencryption+is currently always+.BENCODED_IOV_ENCRYPTION_NONE+(zero).+.PP+.Iunencoded_len+is the length of the unencoded (i.e., decrypted and decompressed) data.+.Iunencoded_offset+is the offset into the unencoded data where the data in the file begins+(less than or equal to+.IRunencoded_len).+.Ilen+is the length of the data in the file+(less than or equal to+.Iunencoded_len+-+.IRunencoded_offset).+See+.BExtentlayout+below for some examples.+.I+.PP+If the unencoded data is actually longer than+.IRunencoded_len,+then it is truncated;+if it is shorter, then it is extended with zeroes.+.PP++.BRpwritev2()+uses the metadata specified in+.IRiov[0],+writes the encoded data from the remaining buffers,+and returns the number of encoded bytes written+(that is, the sum of+.Iiov[n].iov_len+for 1 <=+.In+<+.IRiovcnt;+partial writes will not occur).+At least one encoding field must be non-zero.+Note that the encoded data is not validated when it is written;+if it is not valid (e.g., it cannot be decompressed),+then a subsequent read may return an error.+If the+.Ioffset+argument to+.BRpwritev2()+is -1, then the file offset is incremented by+.IRlen.+If+.Iiov[0].iov_len+is less than+.I"sizeof(struct\ encoded_iov)"+in the kernel,+then any fields unknown to userspace are treated as if they were zero;+if it is greater and any fields unknown to the kernel are non-zero,+then this returns -1 and sets+.Ierrno+to+.BRE2BIG.+.PP+.BRpreadv2()+populates the metadata in+.IRiov[0],+the encoded data in the remaining buffers,+and returns the number of encoded bytes read.+This will only return one extent per call.+This can also read data which is not encoded;+all encoding fields will be zero in that case.+If the+.Ioffset+argument to+.BRpreadv2()+is -1, then the file offset is incremented by+.IRlen.+If+.Iiov[0].iov_len+is less than+.I"sizeof(struct\ encoded_iov)"+in the kernel and any fields unknown to userspace are non-zero,+then+.BRpreadv2()+returns -1 and sets+.Ierrno+to+.BRE2BIG;+if it is greater,+then any fields unknown to the kernel are returned as zero.+If the provided buffers are not large enough to return an entire encoded+extent,+then+.BRpreadv2()+returns -1 and sets+.Ierrno+to+.BRENOBUFS.+.PP+As the filesystem page cache typically contains decoded data,+encoded I/O bypasses the page cache.+.SSExtentlayout+By using+.IRlen,+.IRunencoded_len,+and+.IRunencoded_offset,+it is possible to refer to a subset of an unencoded extent.+.PP+In the simplest case,+.Ilen+is equal to+.Iunencoded_len+and+.Iunencoded_offset+is zero.+This means that the entire unencoded extent is used.+.PP+However, suppose we read 50 bytes into a file+which contains a single compressed extent.+The filesystem must still return the entire compressed extent+for us to be able to decompress it,+so+.Iunencoded_len+would be the length of the entire decompressed extent.+However, because the read was at offset 50,+the first 50 bytes should be ignored.+Therefore,+.Iunencoded_offset+would be 50,+and+.Ilen+would accordingly be+.IRunencoded_len\-\50.+.PP+Additionally, suppose we want to create an encrypted file with length 500,+but the file is encrypted with a block cipher using a block size of 4096.+The unencoded data would therefore include the appropriate padding,+and+.Iunencoded_len+would be 4096.+However, to represent the logical size of the file,+.Ilen+would be 500+(and+.Iunencoded_offset+would be 0).+.PP+Similar situations can arise in other cases:+.IP*3+If the filesystem pads data to the filesystem block size before compressing,+then compressed files with a size unaligned to the filesystem block size will+end with an extent with+.Ilen+<+.IRunencoded_len.+.IP*+Extents cloned from the middle of a larger encoded extent with+.BFICLONERANGE+may have a non-zero+.Iunencoded_offset+and/or+.Ilen+<+.IRunencoded_len.+.IP*+If the middle of an encoded extent is overwritten,+the filesystem may create extents with a non-zero+.Iunencoded_offset+and/or+.Ilen+<+.Iunencoded_len+for the parts that were not overwritten.+.SSSecurity+Encoded I/O creates the potential for some security issues:+.IP*3+Encoded writes allow writing arbitrary data which the kernel will decode on+a subsequent read. Decompression algorithms are complex and may have bugs+which can be exploited by maliciously crafted data.+.IP*+Encoded reads may return data which is not logically present in the file+(see the discussion of+.Ilen+vs.+.Iunencoded_len+above).+It may not be intended for this data to be readable.+.PP+Therefore, encoded I/O requires privilege.+Namely, the+.BRWF_ENCODED+flag may only be used when the file was opened with the+.BO_ALLOW_ENCODED+flag to+.BRopen(2),+which requires the+.BCAP_SYS_ADMIN+capability.+The+.BO_CLOEXEC+flag must be specified in conjunction with+.BRO_ALLOW_ENCODED.+This avoids accidentally leaking the encoded I/O privilege+(it is not cleared on+.BRfork(2)+or+.BRexecve(2)+otherwise).+If+.BO_ALLOW_ENCODED+without+.BO_CLOEXEC+is desired,+.BO_CLOEXEC+can be cleared afterwards with+.BRfnctl(2).+.BRfcntl(2)+can also clear or set+.BO_ALLOW_ENCODED+(including without+.BRO_CLOEXEC).+.SSFilesystemsupport+Encoded I/O is supported on the following filesystems:+.TP+Btrfs (since Linux 5.12)+.IP+Btrfs supports encoded reads and writes of compressed data.+The data is encoded as follows:+.RS+.IP*3+If+.Icompression+is+.BRENCODED_IOV_COMPRESSION_BTRFS_ZLIB,+then the encoded data is a single zlib stream.+.IP*+If+.Icompression+is+.BRENCODED_IOV_COMPRESSION_BTRFS_ZSTD,+then the encoded data is a single zstd frame compressed with the+.IwindowLog+compression parameter set to no more than 17.+.IP*+If+.Icompression+is one of+.BRENCODED_IOV_COMPRESSION_BTRFS_LZO_4K,+.BRENCODED_IOV_COMPRESSION_BTRFS_LZO_8K,+.BRENCODED_IOV_COMPRESSION_BTRFS_LZO_16K,+.BRENCODED_IOV_COMPRESSION_BTRFS_LZO_32K,+or+.BRENCODED_IOV_COMPRESSION_BTRFS_LZO_64K,+then the encoded data is compressed page by page+(using the page size indicated by the name of the constant)+with LZO1X+and wrapped in the format documented in the Linux kernel source file+.IRfs/btrfs/lzo.c.+.RE+.IP+Additionally, there are some restrictions on+.BRpwritev2():+.RS+.IP*3+.Ioffset+(or the current file offset if+.Ioffset+is -1) must be aligned to the sector size of the filesystem.+.IP*+.Ilen+must be aligned to the sector size of the filesystem+unless the data ends at or beyond the current end of the file.+.IP*+.Iunencoded_len+and the length of the encoded data must each be no more than 128 KiB.+This limit may increase in the future.+.IP*+The length of the encoded data must be less than or equal to+.IRunencoded_len.+.IP*+If using LZO, the filesystem's page size must match the compression page size.+.RE
From: Omar Sandoval <redacted>
Btrfs supports transparent compression: data written by the user can be
compressed when written to disk and decompressed when read back.
However, we'd like to add an interface to write pre-compressed data
directly to the filesystem, and the matching interface to read
compressed data without decompressing it. This adds support for
so-called "encoded I/O" via preadv2() and pwritev2().
A new RWF_ENCODED flags indicates that a read or write is "encoded". If
this flag is set, iov[0].iov_base points to a struct encoded_iov which
is used for metadata: namely, the compression algorithm, unencoded
(i.e., decompressed) length, and what subrange of the unencoded data
should be used (needed for truncated or hole-punched extents and when
reading in the middle of an extent). For reads, the filesystem returns
this information; for writes, the caller provides it to the filesystem.
iov[0].iov_len must be set to sizeof(struct encoded_iov), which can be
used to extend the interface in the future a la copy_struct_from_user().
The remaining iovecs contain the encoded extent.
This adds the VFS helpers for supporting encoded I/O and documentation
for filesystem support.
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <redacted>
---
Documentation/filesystems/encoded_io.rst | 74 ++++++++++
Documentation/filesystems/index.rst | 1 +
fs/read_write.c | 167 +++++++++++++++++++++--
include/linux/fs.h | 11 ++
include/uapi/linux/fs.h | 41 +++++-
5 files changed, 280 insertions(+), 14 deletions(-)
create mode 100644 Documentation/filesystems/encoded_io.rst
@@ -0,0 +1,74 @@+===========+Encoded I/O+===========++Encoded I/O is a mechanism for reading and writing encoded (e.g., compressed+and/or encrypted) data directly from/to the filesystem. The userspace interface+is thoroughly described in the :manpage:`encoded_io(7)` man page; this document+describes the requirements for filesystem support.++First of all, a filesystem supporting encoded I/O must indicate this by setting+the ``FMODE_ENCODED_IO`` flag in its ``file_open`` file operation::++ static int foo_file_open(struct inode *inode, struct file *filp)+ {+ ...+ filep->f_mode |= FMODE_ENCODED_IO;+ ...+ }++Encoded I/O goes through ``read_iter`` and ``write_iter``, designated by the+``IOCB_ENCODED`` flag in ``kiocb->ki_flags``.++Reads+=====++Encoded ``read_iter`` should:++1. Call ``generic_encoded_read_checks()`` to validate the file and buffers+ provided by userspace.+2. Initialize the ``encoded_iov`` appropriately.+3. Copy it to the user with ``copy_encoded_iov_to_iter()``.+4. Copy the encoded data to the user.+5. Advance ``kiocb->ki_pos`` by ``encoded_iov->len``.+6. Return the size of the encoded data read, not including the ``encoded_iov``.++There are a few details to be aware of:++* Encoded ``read_iter`` should support reading unencoded data if the extent is+ not encoded.+* If the buffers provided by the user are not large enough to contain an entire+ encoded extent, then ``read_iter`` should return ``-ENOBUFS``. This is to+ avoid confusing userspace with truncated data that cannot be properly+ decoded.+* Reads in the middle of an encoded extent can be returned by setting+``encoded_iov->unencoded_offset`` to non-zero.+* Truncated unencoded data (e.g., because the file does not end on a block+ boundary) may be returned by setting ``encoded_iov->len`` to a value smaller+ value than ``encoded_iov->unencoded_len - encoded_iov->unencoded_offset``.++Writes+======++Encoded ``write_iter`` should (in addition to the usual accounting/checks done+by ``write_iter``):++1. Call ``copy_encoded_iov_from_iter()`` to get and validate the+``encoded_iov``.+2. Call ``generic_encoded_write_checks()`` instead of+``generic_write_checks()``.+3. Check that the provided encoding in ``encoded_iov`` is supported.+4. Advance ``kiocb->ki_pos`` by ``encoded_iov->len``.+5. Return the size of the encoded data written.++Again, there are a few details:++* Encoded ``write_iter`` doesn't need to support writing unencoded data.+*``write_iter`` should either write all of the encoded data or none of it; it+ must not do partial writes.+*``write_iter`` doesn't need to validate the encoded data; a subsequent read+ may return, e.g., ``-EIO`` if the data is not valid.+* The user may lie about the unencoded size of the data; a subsequent read+ should truncate or zero-extend the unencoded data rather than returning an+ error.+* Be careful of page cache coherency.
@@ -1625,24 +1625,15 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)return0;}-/*-*Performsnecessarychecksbeforedoingawrite-*-*Canadjustwritingpositionoramountofbytestowrite.-*Returnsappropriateerrorcodethatcallershouldreturnor-*zeroincasethatwriteshouldbeallowed.-*/-ssize_tgeneric_write_checks(structkiocb*iocb,structiov_iter*from)+staticintgeneric_write_checks_common(structkiocb*iocb,loff_t*count){structfile*file=iocb->ki_filp;structinode*inode=file->f_mapping->host;-loff_tcount;-intret;if(IS_SWAPFILE(inode))return-ETXTBSY;-if(!iov_iter_count(from))+if(!*count)return0;/* FIXME: this is for backwards compatibility with 2.4 */
From: Omar Sandoval <redacted>
btrfs_write_check() has two related bugs:
1. It gets the iov_iter count before calling generic_write_checks(), but
generic_write_checks() may truncate the iov_iter.
2. It returns the count or negative errno as a size_t, which the callers
cast to an int. If the count is greater than INT_MAX, this overflows.
To fix both of these, pull the call to generic_write_checks() out of
btrfs_write_check(), use the new iov_iter count returned from
generic_write_checks(), and have btrfs_write_check() return 0 or a
negative errno as an int instead of the count. This rearrangement also
paves the way for RWF_ENCODED write support.
Fixes: f945968ff64c ("btrfs: introduce btrfs_write_check()")
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/file.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
From: Omar Sandoval <redacted>
The upcoming RWF_ENCODED operation introduces some security concerns:
1. Compressed writes will pass arbitrary data to decompression
algorithms in the kernel.
2. Compressed reads can leak truncated/hole punched data.
Therefore, we need to require privilege for RWF_ENCODED. It's not
possible to do the permissions checks at the time of the read or write
because, e.g., io_uring submits IO from a worker thread. So, add an open
flag which requires CAP_SYS_ADMIN. It can also be set and cleared with
fcntl(). The flag is not cleared in any way on fork or exec. It must be
combined with O_CLOEXEC when opening to avoid accidental leaks (if
needed, it may be set without O_CLOEXEC by using fnctl()).
Note that the usual issue that unknown open flags are ignored doesn't
really matter for O_ALLOW_ENCODED; if the kernel doesn't support
O_ALLOW_ENCODED, then it doesn't support RWF_ENCODED, either.
Signed-off-by: Omar Sandoval <redacted>
---
arch/alpha/include/uapi/asm/fcntl.h | 1 +
arch/parisc/include/uapi/asm/fcntl.h | 1 +
arch/sparc/include/uapi/asm/fcntl.h | 1 +
fs/fcntl.c | 10 ++++++++--
fs/namei.c | 4 ++++
fs/open.c | 7 +++++++
include/linux/fcntl.h | 2 +-
include/uapi/asm-generic/fcntl.h | 4 ++++
8 files changed, 27 insertions(+), 3 deletions(-)
@@ -49,6 +50,11 @@ static int setfl(int fd, struct file * filp, unsigned long arg)if(!inode_owner_or_capable(inode))return-EPERM;+/* O_ALLOW_ENCODED can only be set by superuser */+if((arg&O_ALLOW_ENCODED)&&!(filp->f_flags&O_ALLOW_ENCODED)&&+!capable(CAP_SYS_ADMIN))+return-EPERM;+/* required for strict SunOS emulation */if(O_NONBLOCK!=O_NDELAY)if(arg&O_NDELAY)
@@ -1033,7 +1039,7 @@ static int __init fcntl_init(void)*Exceptions:O_NONBLOCKisatwobitdefineonparisc;O_NDELAY*isdefinedasO_NONBLOCKonsomeplatformsandnotonothers.*/-BUILD_BUG_ON(21-1/* for O_RDONLY being 0 */!=+BUILD_BUG_ON(22-1/* for O_RDONLY being 0 */!=HWEIGHT32((VALID_OPEN_FLAGS&~(O_NONBLOCK|O_NDELAY))|__FMODE_EXEC|__FMODE_NONOTIFY));
@@ -2890,6 +2890,10 @@ static int may_open(const struct path *path, int acc_mode, int flag)if(flag&O_NOATIME&&!inode_owner_or_capable(inode))return-EPERM;+/* O_ALLOW_ENCODED can only be set by superuser */+if((flag&O_ALLOW_ENCODED)&&!capable(CAP_SYS_ADMIN))+return-EPERM;+return0;}
@@ -10,7 +10,7 @@(O_RDONLY|O_WRONLY|O_RDWR|O_CREAT|O_EXCL|O_NOCTTY|O_TRUNC|\O_APPEND|O_NDELAY|O_NONBLOCK|__O_SYNC|O_DSYNC|\FASYNC|O_DIRECT|O_LARGEFILE|O_DIRECTORY|O_NOFOLLOW|\-O_NOATIME|O_CLOEXEC|O_PATH|__O_TMPFILE)+O_NOATIME|O_CLOEXEC|O_PATH|__O_TMPFILE|O_ALLOW_ENCODED)/* List of all valid flags for the how->upgrade_mask argument: */#define VALID_UPGRADE_FLAGS \
@@ -89,6 +89,10 @@#define __O_TMPFILE 020000000#endif+#ifndef O_ALLOW_ENCODED+#define O_ALLOW_ENCODED 040000000+#endif+/* a horrid kludge trying to make sure that this will fail on old kernels */#define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)#define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)
From: Omar Sandoval <redacted>
Commit 1dae796aabf6 ("btrfs: inode: sink parameter start and len to
check_data_csum()") replaced the start parameter to check_data_csum()
with page_offset(), but page_offset() is not meaningful for direct I/O
pages. Bring back the start parameter.
Fixes: 1dae796aabf6 ("btrfs: inode: sink parameter start and len to check_data_csum()")
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/inode.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
From: Omar Sandoval <redacted>
btrfs_csum_one_bio() loops over each filesystem block in the bio while
keeping a cursor of its current logical position in the file in order to
look up the ordered extent to add the checksums to. However, this
doesn't make much sense for compressed extents, as a sector on disk does
not correspond to a sector of decompressed file data. It happens to work
because 1) the compressed bio always covers one ordered extent and 2)
the size of the bio is always less than the size of the ordered extent.
However, the second point will not always be true for encoded writes.
Let's add a boolean parameter to btrfs_csum_one_bio() to indicate that
it can assume that the bio only covers one ordered extent. Since we're
already changing the signature, let's get rid of the contig parameter
and make it implied by the offset parameter, similar to the change we
recently made to btrfs_lookup_bio_sums(). Additionally, let's rename
nr_sectors to blockcount to make it clear that it's the number of
filesystem blocks, not the number of 512-byte sectors.
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/compression.c | 5 +++--
fs/btrfs/ctree.h | 2 +-
fs/btrfs/file-item.c | 35 ++++++++++++++++-------------------
fs/btrfs/inode.c | 8 ++++----
4 files changed, 24 insertions(+), 26 deletions(-)
From: Omar Sandoval <redacted>
Currently, we always reserve the same extent size in the file and extent
size on disk for delalloc because the former is the worst case for the
latter. For RWF_ENCODED writes, we know the exact size of the extent on
disk, which may be less than or greater than (for bookends) the size in
the file. Add a disk_num_bytes parameter to
btrfs_delalloc_reserve_metadata() so that we can reserve the correct
amount of csum bytes. No functional change.
Reviewed-by: Nikolay Borisov <redacted>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/ctree.h | 3 ++-
fs/btrfs/delalloc-space.c | 18 ++++++++++--------
fs/btrfs/file.c | 3 ++-
fs/btrfs/inode.c | 2 +-
fs/btrfs/relocation.c | 4 ++--
5 files changed, 17 insertions(+), 13 deletions(-)
From: Omar Sandoval <redacted>
Currently, an inline extent is always created after i_size is extended
from btrfs_dirty_pages(). However, for encoded writes, we only want to
update i_size after we successfully created the inline extent. Add an
update_i_size parameter to cow_file_range_inline() and
insert_inline_extent() and pass in the size of the extent rather than
determining it from i_size. Since the start parameter is always passed
as 0, get rid of it and simplify the logic in these two functions. While
we're here, let's document the requirements for creating an inline
extent.
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/inode.c | 100 +++++++++++++++++++++++------------------------
1 file changed, 48 insertions(+), 52 deletions(-)
@@ -662,14 +654,15 @@ static noinline int compress_file_range(struct async_chunk *async_chunk)/* we didn't compress the entire range, try*tomakeanuncompressedinlineextent.*/-ret=cow_file_range_inline(BTRFS_I(inode),start,end,+ret=cow_file_range_inline(BTRFS_I(inode),actual_end,0,BTRFS_COMPRESS_NONE,-NULL);+NULL,false);}else{/* try making a compressed inline extent */-ret=cow_file_range_inline(BTRFS_I(inode),start,end,+ret=cow_file_range_inline(BTRFS_I(inode),actual_end,total_compressed,-compress_type,pages);+compress_type,pages,+false);}if(ret<=0){unsignedlongclear_flags=EXTENT_DELALLOC|
@@ -1057,9 +1050,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode,inode_should_defrag(inode,start,end,num_bytes,SZ_64K);if(start==0){+u64actual_end=min_t(u64,i_size_read(&inode->vfs_inode),+end+1);+/* lets try to make an inline extent */-ret=cow_file_range_inline(inode,start,end,0,-BTRFS_COMPRESS_NONE,NULL);+ret=cow_file_range_inline(inode,actual_end,0,+BTRFS_COMPRESS_NONE,NULL,false);if(ret==0){/**WeuseDO_ACCOUNTINGherebecauseweneedthe
From: Omar Sandoval <redacted>
Currently, we only create ordered extents when ram_bytes == num_bytes
and offset == 0. However, RWF_ENCODED writes may create extents which
only refer to a subset of the full unencoded extent, so we need to plumb
these fields through the ordered extent infrastructure and pass them
down to insert_reserved_file_extent().
Since we're changing the btrfs_add_ordered_extent* signature, let's get
rid of the trivial wrappers and add a kernel-doc.
Reviewed-by: Nikolay Borisov <redacted>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/inode.c | 56 ++++++++++++++++++---------------
fs/btrfs/ordered-data.c | 68 ++++++++++++++++-------------------------
fs/btrfs/ordered-data.h | 16 ++++------
3 files changed, 64 insertions(+), 76 deletions(-)
@@ -1127,8 +1126,9 @@ static noinline int cow_file_range(struct btrfs_inode *inode,}free_extent_map(em);-ret=btrfs_add_ordered_extent(inode,start,ins.objectid,-ram_size,cur_alloc_size,0);+ret=btrfs_add_ordered_extent(inode,start,ram_size,ram_size,+ins.objectid,cur_alloc_size,0,+0,BTRFS_COMPRESS_NONE);if(ret)gotoout_drop_extent_cache;
@@ -1760,10 +1760,11 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,gotoerror;}free_extent_map(em);-ret=btrfs_add_ordered_extent(inode,cur_offset,-disk_bytenr,num_bytes,-num_bytes,-BTRFS_ORDERED_PREALLOC);+ret=btrfs_add_ordered_extent(inode,+cur_offset,num_bytes,num_bytes,+disk_bytenr,num_bytes,0,+1<<BTRFS_ORDERED_PREALLOC,+BTRFS_COMPRESS_NONE);if(ret){btrfs_drop_extent_cache(inode,cur_offset,cur_offset+num_bytes-1,
@@ -1772,9 +1773,11 @@ static noinline int run_delalloc_nocow(struct btrfs_inode *inode,}}else{ret=btrfs_add_ordered_extent(inode,cur_offset,+num_bytes,num_bytes,disk_bytenr,num_bytes,-num_bytes,-BTRFS_ORDERED_NOCOW);+0,+1<<BTRFS_ORDERED_NOCOW,+BTRFS_COMPRESS_NONE);if(ret)gotoerror;}
@@ -2578,6 +2581,7 @@ static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,structbtrfs_keyins;u64disk_num_bytes=btrfs_stack_file_extent_disk_num_bytes(stack_fi);u64disk_bytenr=btrfs_stack_file_extent_disk_bytenr(stack_fi);+u64offset=btrfs_stack_file_extent_offset(stack_fi);u64num_bytes=btrfs_stack_file_extent_num_bytes(stack_fi);u64ram_bytes=btrfs_stack_file_extent_ram_bytes(stack_fi);structbtrfs_drop_extents_argsdrop_args={0};
@@ -2652,7 +2656,8 @@ static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,gotoout;ret=btrfs_alloc_reserved_file_extent(trans,root,btrfs_ino(inode),-file_pos,qgroup_reserved,&ins);+file_pos-offset,+qgroup_reserved,&ins);out:btrfs_free_path(path);
@@ -2678,20 +2683,20 @@ static int insert_ordered_extent_file_extent(struct btrfs_trans_handle *trans,structbtrfs_ordered_extent*oe){structbtrfs_file_extent_itemstack_fi;-u64logical_len;boolupdate_inode_bytes;+u64num_bytes=oe->num_bytes;+u64ram_bytes=oe->ram_bytes;memset(&stack_fi,0,sizeof(stack_fi));btrfs_set_stack_file_extent_type(&stack_fi,BTRFS_FILE_EXTENT_REG);btrfs_set_stack_file_extent_disk_bytenr(&stack_fi,oe->disk_bytenr);btrfs_set_stack_file_extent_disk_num_bytes(&stack_fi,oe->disk_num_bytes);+btrfs_set_stack_file_extent_offset(&stack_fi,oe->offset);if(test_bit(BTRFS_ORDERED_TRUNCATED,&oe->flags))-logical_len=oe->truncated_len;-else-logical_len=oe->num_bytes;-btrfs_set_stack_file_extent_num_bytes(&stack_fi,logical_len);-btrfs_set_stack_file_extent_ram_bytes(&stack_fi,logical_len);+num_bytes=ram_bytes=oe->truncated_len;+btrfs_set_stack_file_extent_num_bytes(&stack_fi,num_bytes);+btrfs_set_stack_file_extent_ram_bytes(&stack_fi,ram_bytes);btrfs_set_stack_file_extent_compression(&stack_fi,oe->compress_type);/* Encryption and other encoding is reserved and all 0 */
@@ -171,7 +182,8 @@ static int __btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offsetstructbtrfs_ordered_extent*entry;intret;-if(type==BTRFS_ORDERED_NOCOW||type==BTRFS_ORDERED_PREALLOC){+if(flags&+((1<<BTRFS_ORDERED_NOCOW)|(1<<BTRFS_ORDERED_PREALLOC))){/* For nocow write, we can release the qgroup rsv right now */ret=btrfs_qgroup_free_data(inode,NULL,file_offset,num_bytes);if(ret<0)
@@ -191,21 +203,21 @@ static int __btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offsetreturn-ENOMEM;entry->file_offset=file_offset;-entry->disk_bytenr=disk_bytenr;entry->num_bytes=num_bytes;+entry->ram_bytes=ram_bytes;+entry->disk_bytenr=disk_bytenr;entry->disk_num_bytes=disk_num_bytes;+entry->offset=offset;entry->bytes_left=num_bytes;entry->inode=igrab(&inode->vfs_inode);entry->compress_type=compress_type;entry->truncated_len=(u64)-1;entry->qgroup_rsv=ret;-if(type!=BTRFS_ORDERED_IO_DONE&&type!=BTRFS_ORDERED_COMPLETE)-set_bit(type,&entry->flags);-if(dio){+entry->flags=flags;+if(flags&(1<<BTRFS_ORDERED_DIRECT)){percpu_counter_add_batch(&fs_info->dio_bytes,num_bytes,fs_info->delalloc_batch);-set_bit(BTRFS_ORDERED_DIRECT,&entry->flags);}/* one ref for the tree */
@@ -72,9 +72,11 @@ struct btrfs_ordered_extent {*Thesefieldsdirectlycorrespondtothesamefieldsin*btrfs_file_extent_item.*/-u64disk_bytenr;u64num_bytes;+u64ram_bytes;+u64disk_bytenr;u64disk_num_bytes;+u64offset;/* number of bytes that still need writing */u64bytes_left;
@@ -160,15 +162,9 @@ int btrfs_dec_test_first_ordered_pending(struct btrfs_inode *inode,u64*file_offset,u64io_size,intuptodate);intbtrfs_add_ordered_extent(structbtrfs_inode*inode,u64file_offset,-u64disk_bytenr,u64num_bytes,u64disk_num_bytes,-inttype);-intbtrfs_add_ordered_extent_dio(structbtrfs_inode*inode,u64file_offset,-u64disk_bytenr,u64num_bytes,-u64disk_num_bytes,inttype);-intbtrfs_add_ordered_extent_compress(structbtrfs_inode*inode,u64file_offset,-u64disk_bytenr,u64num_bytes,-u64disk_num_bytes,inttype,-intcompress_type);+u64num_bytes,u64ram_bytes,u64disk_bytenr,+u64disk_num_bytes,u64offset,intflags,+intcompress_type);voidbtrfs_add_ordered_sum(structbtrfs_ordered_extent*entry,structbtrfs_ordered_sum*sum);structbtrfs_ordered_extent*btrfs_lookup_ordered_extent(structbtrfs_inode*inode,
From: Omar Sandoval <redacted>
There are 4 main cases:
1. Inline extents: we copy the data straight out of the extent buffer.
2. Hole/preallocated extents: we fill in zeroes.
3. Regular, uncompressed extents: we read the sectors we need directly
from disk.
4. Regular, compressed extents: we read the entire compressed extent
from disk and indicate what subset of the decompressed extent is in
the file.
This initial implementation simplifies a few things that can be improved
in the future:
- We hold the inode lock during the operation.
- Cases 1, 3, and 4 allocate temporary memory to read into before
copying out to userspace.
- We don't do read repair, because it turns out that read repair is
currently broken for compressed data.
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/ctree.h | 2 +
fs/btrfs/file.c | 5 +
fs/btrfs/inode.c | 496 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 503 insertions(+)
From: Omar Sandoval <redacted>
The implementation resembles 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. From there, we can reuse the compression code
with a minor modification to distinguish the write from writeback. This
also creates inline extents when possible.
Now that read and write are implemented, this also sets the
FMODE_ENCODED_IO flag in btrfs_file_open().
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/compression.c | 7 +-
fs/btrfs/compression.h | 6 +-
fs/btrfs/ctree.h | 2 +
fs/btrfs/file.c | 37 +++++-
fs/btrfs/inode.c | 259 +++++++++++++++++++++++++++++++++++++++-
fs/btrfs/ordered-data.c | 12 +-
fs/btrfs/ordered-data.h | 2 +
7 files changed, 313 insertions(+), 12 deletions(-)
@@ -336,7 +336,8 @@ static void end_compressed_bio_write(struct bio *bio)bio->bi_status==BLK_STS_OK);cb->compressed_pages[0]->mapping=NULL;-end_compressed_writeback(inode,cb);+if(cb->writeback)+end_compressed_writeback(inode,cb);/* note, our inode could be gone now *//*
@@ -49,6 +49,9 @@ struct compressed_bio {/* the compression algorithm for this bio */intcompress_type;+/* Whether this is a write for writeback. */+boolwriteback;+/* number of compressed pages in the array */unsignedlongnr_pages;
@@ -2703,6 +2703,7 @@ static int insert_ordered_extent_file_extent(struct btrfs_trans_handle *trans,*exceptiftheorderedextentwastruncated.*/update_inode_bytes=test_bit(BTRFS_ORDERED_DIRECT,&oe->flags)||+test_bit(BTRFS_ORDERED_ENCODED,&oe->flags)||test_bit(BTRFS_ORDERED_TRUNCATED,&oe->flags);returninsert_reserved_file_extent(trans,BTRFS_I(oe->inode),
@@ -2737,7 +2738,8 @@ static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent)if(!test_bit(BTRFS_ORDERED_NOCOW,&ordered_extent->flags)&&!test_bit(BTRFS_ORDERED_PREALLOC,&ordered_extent->flags)&&-!test_bit(BTRFS_ORDERED_DIRECT,&ordered_extent->flags))+!test_bit(BTRFS_ORDERED_DIRECT,&ordered_extent->flags)&&+!test_bit(BTRFS_ORDERED_ENCODED,&ordered_extent->flags))clear_bits|=EXTENT_DELALLOC_NEW;freespace_inode=btrfs_is_free_space_inode(inode);
@@ -10432,6 +10434,259 @@ ssize_t btrfs_encoded_read(struct kiocb *iocb, struct iov_iter *iter)returnret;}+ssize_tbtrfs_do_encoded_write(structkiocb*iocb,structiov_iter*from,+structencoded_iov*encoded)+{+structinode*inode=file_inode(iocb->ki_filp);+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+structbtrfs_root*root=BTRFS_I(inode)->root;+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+structextent_changeset*data_reserved=NULL;+structextent_state*cached_state=NULL;+intcompression;+size_torig_count;+u64start,end;+u64num_bytes,ram_bytes,disk_num_bytes;+unsignedlongnr_pages,i;+structpage**pages;+structbtrfs_keyins;+boolextent_reserved=false;+structextent_map*em;+ssize_tret;++switch(encoded->compression){+caseENCODED_IOV_COMPRESSION_BTRFS_ZLIB:+compression=BTRFS_COMPRESS_ZLIB;+break;+caseENCODED_IOV_COMPRESSION_BTRFS_ZSTD:+compression=BTRFS_COMPRESS_ZSTD;+break;+caseENCODED_IOV_COMPRESSION_BTRFS_LZO_4K:+caseENCODED_IOV_COMPRESSION_BTRFS_LZO_8K:+caseENCODED_IOV_COMPRESSION_BTRFS_LZO_16K:+caseENCODED_IOV_COMPRESSION_BTRFS_LZO_32K:+caseENCODED_IOV_COMPRESSION_BTRFS_LZO_64K:+/* The page size must match for LZO. */+if(encoded->compression-+ENCODED_IOV_COMPRESSION_BTRFS_LZO_4K+12!=PAGE_SHIFT)+return-EINVAL;+compression=BTRFS_COMPRESS_LZO;+break;+default:+return-EINVAL;+}+if(encoded->encryption!=ENCODED_IOV_ENCRYPTION_NONE)+return-EINVAL;++orig_count=iov_iter_count(from);++/* The extent size must be sane. */+if(encoded->unencoded_len>BTRFS_MAX_UNCOMPRESSED||+orig_count>BTRFS_MAX_COMPRESSED||orig_count==0)+return-EINVAL;++/*+*Thecompresseddatamustbesmallerthanthedecompresseddata.+*+*It'sofcoursepossiblefordatatocompresstolargerorthesame+*size,butthebufferedI/Opathfallsbacktonocompressionforsuch+*data,andwedon'twanttobreakanyassumptionsbycreatingthese+*extents.+*+*Notethatthisislessstrictthanthecurrentcheckwehavethatthe+*compresseddatamustbeatleastonesectorsmallerthanthe+*decompresseddata.Weonlywanttoenforcetheweakerrequirement+*fromoldkernelsthatitisatleastonebytesmaller.+*/+if(orig_count>=encoded->unencoded_len)+return-EINVAL;++/* The extent must start on a sector boundary. */+start=iocb->ki_pos;+if(!IS_ALIGNED(start,fs_info->sectorsize))+return-EINVAL;++/*+*Theextentmustendonasectorboundary.However,weallowawrite+*whichendsatorextendsi_sizetohaveanunalignedlength;weround+*uptheextentsizeandseti_sizetotheunalignedend.+*/+if(start+encoded->len<inode->i_size&&+!IS_ALIGNED(start+encoded->len,fs_info->sectorsize))+return-EINVAL;++/* Finally, the offset in the unencoded data must be sector-aligned. */+if(!IS_ALIGNED(encoded->unencoded_offset,fs_info->sectorsize))+return-EINVAL;++num_bytes=ALIGN(encoded->len,fs_info->sectorsize);+ram_bytes=ALIGN(encoded->unencoded_len,fs_info->sectorsize);+end=start+num_bytes-1;++/*+*Iftheextentcannotbeinline,thecompresseddataondiskmustbe+*sector-aligned.Forconvenience,weextenditwithzeroesifit+*isn't.+*/+disk_num_bytes=ALIGN(orig_count,fs_info->sectorsize);+nr_pages=DIV_ROUND_UP(disk_num_bytes,PAGE_SIZE);+pages=kvcalloc(nr_pages,sizeof(structpage*),GFP_KERNEL_ACCOUNT);+if(!pages)+return-ENOMEM;+for(i=0;i<nr_pages;i++){+size_tbytes=min_t(size_t,PAGE_SIZE,iov_iter_count(from));+char*kaddr;++pages[i]=alloc_page(GFP_KERNEL_ACCOUNT|__GFP_HIGHMEM);+if(!pages[i]){+ret=-ENOMEM;+gotoout_pages;+}+kaddr=kmap(pages[i]);+if(copy_from_iter(kaddr,bytes,from)!=bytes){+kunmap(pages[i]);+ret=-EFAULT;+gotoout_pages;+}+if(bytes<PAGE_SIZE)+memset(kaddr+bytes,0,PAGE_SIZE-bytes);+kunmap(pages[i]);+}++for(;;){+structbtrfs_ordered_extent*ordered;++ret=btrfs_wait_ordered_range(inode,start,num_bytes);+if(ret)+gotoout_pages;+ret=invalidate_inode_pages2_range(inode->i_mapping,+start>>PAGE_SHIFT,+end>>PAGE_SHIFT);+if(ret)+gotoout_pages;+lock_extent_bits(io_tree,start,end,&cached_state);+ordered=btrfs_lookup_ordered_range(BTRFS_I(inode),start,+num_bytes);+if(!ordered&&+!filemap_range_has_page(inode->i_mapping,start,end))+break;+if(ordered)+btrfs_put_ordered_extent(ordered);+unlock_extent_cached(io_tree,start,end,&cached_state);+cond_resched();+}++/*+*Wedon'tusethehigher-leveldelallocspacefunctionsbecauseour+*num_bytesanddisk_num_bytesaredifferent.+*/+ret=btrfs_alloc_data_chunk_ondemand(BTRFS_I(inode),disk_num_bytes);+if(ret)+gotoout_unlock;+ret=btrfs_qgroup_reserve_data(BTRFS_I(inode),&data_reserved,start,+num_bytes);+if(ret)+gotoout_free_data_space;+ret=btrfs_delalloc_reserve_metadata(BTRFS_I(inode),num_bytes,+disk_num_bytes);+if(ret)+gotoout_qgroup_free_data;++/* Try an inline extent first. */+if(start==0&&encoded->unencoded_len==encoded->len&&+encoded->unencoded_offset==0){+ret=cow_file_range_inline(BTRFS_I(inode),encoded->len,+orig_count,compression,pages,+true);+if(ret<=0){+if(ret==0)+ret=orig_count;+gotoout_delalloc_release;+}+}++ret=btrfs_reserve_extent(root,disk_num_bytes,disk_num_bytes,+disk_num_bytes,0,0,&ins,1,1);+if(ret)+gotoout_delalloc_release;+extent_reserved=true;++em=create_io_em(BTRFS_I(inode),start,num_bytes,+start-encoded->unencoded_offset,ins.objectid,+ins.offset,ins.offset,ram_bytes,compression,+BTRFS_ORDERED_COMPRESSED);+if(IS_ERR(em)){+ret=PTR_ERR(em);+gotoout_free_reserved;+}+free_extent_map(em);++ret=btrfs_add_ordered_extent(BTRFS_I(inode),start,num_bytes,+ram_bytes,ins.objectid,ins.offset,+encoded->unencoded_offset,+(1<<BTRFS_ORDERED_ENCODED)|+(1<<BTRFS_ORDERED_COMPRESSED),+compression);+if(ret){+btrfs_drop_extent_cache(BTRFS_I(inode),start,end,0);+gotoout_free_reserved;+}+btrfs_dec_block_group_reservations(fs_info,ins.objectid);++if(start+encoded->len>inode->i_size)+i_size_write(inode,start+encoded->len);++unlock_extent_cached(io_tree,start,end,&cached_state);++btrfs_delalloc_release_extents(BTRFS_I(inode),num_bytes);++if(btrfs_submit_compressed_write(BTRFS_I(inode),start,num_bytes,+ins.objectid,ins.offset,pages,+nr_pages,0,NULL,false)){+structpage*page=pages[0];++page->mapping=inode->i_mapping;+btrfs_writepage_endio_finish_ordered(page,start,end,0);+page->mapping=NULL;+ret=-EIO;+gotoout_pages;+}+ret=orig_count;+gotoout;++out_free_reserved:+btrfs_dec_block_group_reservations(fs_info,ins.objectid);+btrfs_free_reserved_extent(fs_info,ins.objectid,ins.offset,1);+out_delalloc_release:+btrfs_delalloc_release_extents(BTRFS_I(inode),num_bytes);+btrfs_delalloc_release_metadata(BTRFS_I(inode),disk_num_bytes,+ret<0);+out_qgroup_free_data:+if(ret<0){+btrfs_qgroup_free_data(BTRFS_I(inode),data_reserved,start,+num_bytes);+}+out_free_data_space:+/*+*Ifbtrfs_reserve_extent()succeeded,thenwealreadydecremented+*bytes_may_use.+*/+if(!extent_reserved)+btrfs_free_reserved_data_space_noquota(fs_info,disk_num_bytes);+out_unlock:+unlock_extent_cached(io_tree,start,end,&cached_state);+out_pages:+for(i=0;i<nr_pages;i++){+if(pages[i])+__free_page(pages[i]);+}+kvfree(pages);+out:+if(ret>=0)+iocb->ki_pos+=encoded->len;+returnret;+}+#ifdef CONFIG_SWAP/**Addanentryindicatingablockgroupordevicewhichispinnedbya
@@ -62,6 +62,8 @@ enum {BTRFS_ORDERED_LOGGED_CSUM,/* We wait for this extent to complete in the current transaction */BTRFS_ORDERED_PENDING,+/* RWF_ENCODED I/O */+BTRFS_ORDERED_ENCODED,};structbtrfs_ordered_extent{
From: Amir Goldstein <amir73il@gmail.com> Date: 2020-11-19 07:03:19
On Wed, Nov 18, 2020 at 9:18 PM Omar Sandoval [off-list ref] wrote:
quoted hunk
From: Omar Sandoval <redacted>
The upcoming RWF_ENCODED operation introduces some security concerns:
1. Compressed writes will pass arbitrary data to decompression
algorithms in the kernel.
2. Compressed reads can leak truncated/hole punched data.
Therefore, we need to require privilege for RWF_ENCODED. It's not
possible to do the permissions checks at the time of the read or write
because, e.g., io_uring submits IO from a worker thread. So, add an open
flag which requires CAP_SYS_ADMIN. It can also be set and cleared with
fcntl(). The flag is not cleared in any way on fork or exec. It must be
combined with O_CLOEXEC when opening to avoid accidental leaks (if
needed, it may be set without O_CLOEXEC by using fnctl()).
Note that the usual issue that unknown open flags are ignored doesn't
really matter for O_ALLOW_ENCODED; if the kernel doesn't support
O_ALLOW_ENCODED, then it doesn't support RWF_ENCODED, either.
Signed-off-by: Omar Sandoval <redacted>
---
arch/alpha/include/uapi/asm/fcntl.h | 1 +
arch/parisc/include/uapi/asm/fcntl.h | 1 +
arch/sparc/include/uapi/asm/fcntl.h | 1 +
fs/fcntl.c | 10 ++++++++--
fs/namei.c | 4 ++++
fs/open.c | 7 +++++++
include/linux/fcntl.h | 2 +-
include/uapi/asm-generic/fcntl.h | 4 ++++
8 files changed, 27 insertions(+), 3 deletions(-)
@@ -49,6 +50,11 @@ static int setfl(int fd, struct file * filp, unsigned long arg)if(!inode_owner_or_capable(inode))return-EPERM;+/* O_ALLOW_ENCODED can only be set by superuser */+if((arg&O_ALLOW_ENCODED)&&!(filp->f_flags&O_ALLOW_ENCODED)&&+!capable(CAP_SYS_ADMIN))+return-EPERM;+/* required for strict SunOS emulation */if(O_NONBLOCK!=O_NDELAY)if(arg&O_NDELAY)
@@ -1033,7 +1039,7 @@ static int __init fcntl_init(void)*Exceptions:O_NONBLOCKisatwobitdefineonparisc;O_NDELAY*isdefinedasO_NONBLOCKonsomeplatformsandnotonothers.*/-BUILD_BUG_ON(21-1/* for O_RDONLY being 0 */!=+BUILD_BUG_ON(22-1/* for O_RDONLY being 0 */!=HWEIGHT32((VALID_OPEN_FLAGS&~(O_NONBLOCK|O_NDELAY))|__FMODE_EXEC|__FMODE_NONOTIFY));
@@ -2890,6 +2890,10 @@ static int may_open(const struct path *path, int acc_mode, int flag)if(flag&O_NOATIME&&!inode_owner_or_capable(inode))return-EPERM;+/* O_ALLOW_ENCODED can only be set by superuser */+if((flag&O_ALLOW_ENCODED)&&!capable(CAP_SYS_ADMIN))+return-EPERM;+return0;}
dup() can also result in accidental leak.
We could fail dup() of fd without O_CLOEXEC. Should we?
If we should than what error code should it be? We could return EPERM,
but since we do allow to clear O_CLOEXEC or set O_ALLOW_ENCODED
after open, EPERM seems a tad harsh.
EINVAL seems inappropriate because the error has nothing to do with
input args of dup() and EBADF would also be confusing.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2020-11-19 07:38:43
On Wed, Nov 18, 2020 at 9:18 PM Omar Sandoval [off-list ref] wrote:
quoted hunk
From: Omar Sandoval <redacted>
Btrfs supports transparent compression: data written by the user can be
compressed when written to disk and decompressed when read back.
However, we'd like to add an interface to write pre-compressed data
directly to the filesystem, and the matching interface to read
compressed data without decompressing it. This adds support for
so-called "encoded I/O" via preadv2() and pwritev2().
A new RWF_ENCODED flags indicates that a read or write is "encoded". If
this flag is set, iov[0].iov_base points to a struct encoded_iov which
is used for metadata: namely, the compression algorithm, unencoded
(i.e., decompressed) length, and what subrange of the unencoded data
should be used (needed for truncated or hole-punched extents and when
reading in the middle of an extent). For reads, the filesystem returns
this information; for writes, the caller provides it to the filesystem.
iov[0].iov_len must be set to sizeof(struct encoded_iov), which can be
used to extend the interface in the future a la copy_struct_from_user().
The remaining iovecs contain the encoded extent.
This adds the VFS helpers for supporting encoded I/O and documentation
for filesystem support.
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <redacted>
---
Documentation/filesystems/encoded_io.rst | 74 ++++++++++
Documentation/filesystems/index.rst | 1 +
fs/read_write.c | 167 +++++++++++++++++++++--
include/linux/fs.h | 11 ++
include/uapi/linux/fs.h | 41 +++++-
5 files changed, 280 insertions(+), 14 deletions(-)
create mode 100644 Documentation/filesystems/encoded_io.rst
@@ -0,0 +1,74 @@+===========+Encoded I/O+===========++Encoded I/O is a mechanism for reading and writing encoded (e.g., compressed+and/or encrypted) data directly from/to the filesystem. The userspace interface+is thoroughly described in the :manpage:`encoded_io(7)` man page; this document+describes the requirements for filesystem support.++First of all, a filesystem supporting encoded I/O must indicate this by setting+the ``FMODE_ENCODED_IO`` flag in its ``file_open`` file operation::+
Should this be FMODE_ALLOW_ENCODED_IO?
How come I see no checks for this flag in vfs code?
You seem to only be checking the O_ flag.
Do we really want to allow setting the O_ flag after open or should we
deny that?
quoted hunk
+ static int foo_file_open(struct inode *inode, struct file *filp)
+ {
+ ...
+ filep->f_mode |= FMODE_ENCODED_IO;
+ ...
+ }
+
+Encoded I/O goes through ``read_iter`` and ``write_iter``, designated by the
+``IOCB_ENCODED`` flag in ``kiocb->ki_flags``.
+
+Reads
+=====
+
+Encoded ``read_iter`` should:
+
+1. Call ``generic_encoded_read_checks()`` to validate the file and buffers
+ provided by userspace.
+2. Initialize the ``encoded_iov`` appropriately.
+3. Copy it to the user with ``copy_encoded_iov_to_iter()``.
+4. Copy the encoded data to the user.
+5. Advance ``kiocb->ki_pos`` by ``encoded_iov->len``.
+6. Return the size of the encoded data read, not including the ``encoded_iov``.
+
+There are a few details to be aware of:
+
+* Encoded ``read_iter`` should support reading unencoded data if the extent is
+ not encoded.
+* If the buffers provided by the user are not large enough to contain an entire
+ encoded extent, then ``read_iter`` should return ``-ENOBUFS``. This is to
+ avoid confusing userspace with truncated data that cannot be properly
+ decoded.
+* Reads in the middle of an encoded extent can be returned by setting
+ ``encoded_iov->unencoded_offset`` to non-zero.
+* Truncated unencoded data (e.g., because the file does not end on a block
+ boundary) may be returned by setting ``encoded_iov->len`` to a value smaller
+ value than ``encoded_iov->unencoded_len - encoded_iov->unencoded_offset``.
+
+Writes
+======
+
+Encoded ``write_iter`` should (in addition to the usual accounting/checks done
+by ``write_iter``):
+
+1. Call ``copy_encoded_iov_from_iter()`` to get and validate the
+ ``encoded_iov``.
+2. Call ``generic_encoded_write_checks()`` instead of
+ ``generic_write_checks()``.
+3. Check that the provided encoding in ``encoded_iov`` is supported.
+4. Advance ``kiocb->ki_pos`` by ``encoded_iov->len``.
+5. Return the size of the encoded data written.
+
+Again, there are a few details:
+
+* Encoded ``write_iter`` doesn't need to support writing unencoded data.
+* ``write_iter`` should either write all of the encoded data or none of it; it
+ must not do partial writes.
+* ``write_iter`` doesn't need to validate the encoded data; a subsequent read
+ may return, e.g., ``-EIO`` if the data is not valid.
+* The user may lie about the unencoded size of the data; a subsequent read
+ should truncate or zero-extend the unencoded data rather than returning an
+ error.
+* Be careful of page cache coherency.
@@ -1625,24 +1625,15 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)return0;}-/*-*Performsnecessarychecksbeforedoingawrite-*-*Canadjustwritingpositionoramountofbytestowrite.-*Returnsappropriateerrorcodethatcallershouldreturnor-*zeroincasethatwriteshouldbeallowed.-*/-ssize_tgeneric_write_checks(structkiocb*iocb,structiov_iter*from)+staticintgeneric_write_checks_common(structkiocb*iocb,loff_t*count){structfile*file=iocb->ki_filp;structinode*inode=file->f_mapping->host;-loff_tcount;-intret;if(IS_SWAPFILE(inode))return-ETXTBSY;-if(!iov_iter_count(from))+if(!*count)return0;/* FIXME: this is for backwards compatibility with 2.4 */
I am not a fan of this trick.
There is no shortage of enums in uapi headers, but I think that if we want
to set values in stone, the values should be set explicitly and not
auto assigned
by compiler.
If anybody ever adds a line, say ENCODED_IOV_COMPRESSION_BTRFS_ZLIB_V2
in the middle of the enum list, it won't be obvious that it's a uapi breakage.
In principle, we could have partitioned the encoding types by domains
(e.g. btrfs),
and the btrfs specific encodings would have been a part of a btrfs
header, but it's
not that important.
However, please move all encoded_io stuff to a new uapi header and do
not include it
from fs.h to avoid having to compile most filesystems every time a new
btrfs private encoding
type is added.
Thanks,
Amir.
Hi Omar,
Please, see some fixes below:
Michael, I've also some questions for you below
(you can grep for mtk to find those).
Thanks,
Alex
On 11/18/20 8:18 PM, Omar Sandoval wrote:
quoted hunk
From: Omar Sandoval <redacted>
This adds a new page, encoded_io(7), providing an overview of encoded
I/O and updates fcntl(2), open(2), and preadv2(2)/pwritev2(2) to
reference it.
Cc: Michael Kerrisk <redacted>
Cc: linux-man <redacted>
Signed-off-by: Omar Sandoval <redacted>
---
This feature is not yet upstream.
man2/fcntl.2 | 10 +-
man2/open.2 | 23 +++
man2/readv.2 | 70 +++++++++
man7/encoded_io.7 | 369 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 471 insertions(+), 1 deletion(-)
create mode 100644 man7/encoded_io.7
@@ -221,8 +221,9 @@ On Linux, this command can change only the .BRO_ASYNC, .BRO_DIRECT, .BRO_NOATIME,+.BRO_NONBLOCK, and-.BO_NONBLOCK+.BO_ALLOW_ENCODED flags. It is not possible to change the .BRO_DSYNC
@@ -1820,6 +1821,13 @@ Attempted to clear the flag on a file that has the append-only attribute set. .TP .BEPERM+Attempted to set the+.BO_ALLOW_ENCODED+flag and the calling process did not have the+.BCAP_SYS_ADMIN+capability.+.TP+.BEPERM .Icmd was .BRF_ADD_SEALS,
@@ -437,6 +437,16 @@ was followed by a call to .BRfdatasync(2)). .IR"See NOTES below". .TP+.BO_ALLOW_ENCODED
The list is alphabetically sorted;
please, follow that
(O_ALLOW_ENCODED should be the first one).
quoted hunk
+Open the file with encoded I/O permissions;
+see
+.BR encoded_io (7).
+.B O_CLOEXEC
+must be specified in conjuction with this flag.
+The caller must have the
+.B CAP_SYS_ADMIN
+capability.
+.TP
.B O_EXCL
Ensure that this call creates the file:
if this flag is specified in conjunction with
@@ -1082,6 +1092,14 @@ is invalid (e.g., it contains characters not permitted by the underlying filesystem). .TP .B EINVAL+.B O_ALLOW_ENCODED+was specified in+.IR flags ,+but+.B O_CLOEXEC+was not specified.+.TP+.B EINVAL The final component ("basename") of .I pathname is invalid
@@ -1238,6 +1256,11 @@ did not match the owner of the file and the caller was not privileged. The operation was prevented by a file seal; see .BR fcntl (2). .TP+.B EPERM+The+.B O_ALLOW_ENCODED+flag was specified, but the caller was not privileged.+.TP .B EROFS .I pathname refers to a file on a read-only filesystem and write access was
@@ -264,6 +264,11 @@ the data is always appended to the end of the file. However, if the .Ioffset argument is \-1, the current file offset is updated.+.TP+.BRRWF_ENCODED" (since Linux 5.12)"+Read or write encoded (e.g., compressed) data.+See+.BRencoded_io(7). .SHRETURNVALUE On success, .BRreadv(),
@@ -283,6 +288,13 @@ than requested (see and .BRwrite(2)). .PP+If+.B+RWF_ENCODED
RWF_ENCODED should go in the same line as .B:
[
.B RWF_ENCODED
]
quoted hunk
+was specified in
+.IR flags ,
+then the return value is the number of encoded bytes.
+.PP
On error, \-1 is returned, and \fIerrno\fP is set appropriately.
.SH ERRORS
The errors are as given for
@@ -313,6 +325,64 @@ is less than zero or greater than the permitted maximum. .TP .B EOPNOTSUPP An unknown flag is specified in \fIflags\fP.+.TP+.B EOPNOTSUPP+.B RWF_ENCODED+is specified in+.I flags+and the filesystem does not implement encoded I/O.+.TP+.B EPERM+.B RWF_ENCODED+is specified in+.I flags+and the file was not opened with the+.B O_ALLOW_ENCODED+flag.+.PP+.BR preadv2 ()+can fail for the following reasons:
The wording is a bit unclear:
Above your additions (old text, not yours),
it says that some errors apply to preadv2
(as well as to other functions):
[
ERRORS
The errors are as given for read(2) and write(2). Furthermore,
preadv(), preadv2(), pwritev(), and pwritev2() can also fail
for the same reasons as lseek(2). Additionally, the following
errors are defined:
EINVAL The sum of the iov_len values overflows an ssize_t
value.
EINVAL The vector count, iovcnt, is less than zero or greater
than the permitted maximum.
EOPNOTSUPP
An unknown flag is specified in flags.
EOPNOTSUPP
RWF_ENCODED is specified in flags and the filesystem
does not implement encoded I/O.
EPERM RWF_ENCODED is specified in flags and the file was not
opened with the O_ALLOW_ENCODED flag.
]
And then you added a line that says:
[
preadv2() can fail for the following reasons:
]
Which if read strictly, it says that [only] the following errors apply.
Did you mean that
"preadv3() can _additionally_ fail for the following reasons"?
Could you please be a bit more specific?
The same applies for pwritev2() below.
+.TP
+.B E2BIG
+.B RWF_ENCODED
+is specified in
+.I flags
+and
+.I iov[0]
+is not large enough to return the encoding metadata.
+.TP
+.B ENOBUFS
+.B RWF_ENCODED
+is specified in
+.I flags
+and the buffers in
+.I iov
+are not big enough to return the encoded data.
+.PP
+.BR pwritev2 ()
+can fail for the following reasons:
+.TP
+.B E2BIG
+.B RWF_ENCODED
+is specified in
+.I flags
+and
+.I iov[0]
+contains non-zero fields
+after the kernel's
+.IR "sizeof(struct\ encoded_iov)" .
Don't escape the space, if the string is already in "".
quoted hunk
+.TP
+.B EINVAL
+.B RWF_ENCODED
+is specified in
+.I flags
+and the encoding is unknown or not supported by the filesystem.
+.TP
+.B EINVAL
+.B RWF_ENCODED
+is specified in
+.I flags
+and the alignment and/or size requirements are not met.
.SH VERSIONS
.BR preadv ()
and
@@ -0,0 +1,369 @@+.\" Copyright (c) 2020 by Omar Sandoval <osandov@fb.com>+.\"+.\" %%%LICENSE_START(VERBATIM)+.\" Permission is granted to make and distribute verbatim copies of this+.\" manual provided the copyright notice and this permission notice are+.\" preserved on all copies.+.\"+.\" Permission is granted to copy and distribute modified versions of this+.\" manual under the conditions for verbatim copying, provided that the+.\" entire resulting derived work is distributed under the terms of a+.\" permission notice identical to this one.+.\"+.\" Since the Linux kernel and libraries are constantly changing, this+.\" manual page may be incorrect or out-of-date. The author(s) assume no+.\" responsibility for errors or omissions, or for damages resulting from+.\" the use of the information contained herein. The author(s) may not+.\" have taken the same level of care in the production of this manual,+.\" which is licensed free of charge, as they might when working+.\" professionally.+.\"+.\" Formatted or processed versions of this manual, if unaccompanied by+.\" the source, must acknowledge the copyright and authors of this work.+.\" %%%LICENSE_END+.\"+.\"+.THENCODED_IO72020-11-11"Linux""Linux Programmer's Manual"+.SHNAME+encoded_io \- overview of encoded I/O+.SHDESCRIPTION+Several filesystems (e.g., Btrfs) support transparent encoding+(e.g., compression, encryption) of data on disk:+written data is encoded by the kernel before it is written to disk,+and read data is decoded before being returned to the user.+In some cases, it is useful to skip this encoding step.
Here I would use ';' instead of '.'
(and next letter would be lowercase, then).
+For example, the user may want to read the compressed contents of a file
+or write pre-compressed data directly to a file.
+This is referred to as "encoded I/O".
+.SS Encoded I/O API
+Encoded I/O is specified with the
+.B RWF_ENCODED
+flag to
+.BR preadv2 (2)
+and
+.BR pwritev2 (2).
+If
+.B RWF_ENCODED
+is specified, then
+.I iov[0].iov_base
+points to an
+.I
+encoded_iov
On the same line, please.
+structure, defined in
+.I <linux/fs.h>
+as:
+.PP
+.in +4n
+.EX
+struct encoded_iov {
+ __aligned_u64 len;
+ __aligned_u64 unencoded_len;
+ __aligned_u64 unencoded_offset;
+ __u32 compression;
+ __u32 encryption;
+};
+.EE
+.in
+.PP
+This may be extended in the future, so
+.I iov[0].iov_len
+must be set to
+.I "sizeof(struct\ encoded_iov)"
+for forward/backward compatibility.
+The remaining buffers contain the encoded data.
+.PP
+.I compression
+and
+.I encryption
+are the encoding fields.
+.I compression
+is
+.B ENCODED_IOV_COMPRESSION_NONE
+(zero)
+or a filesystem-specific
+.B ENCODED_IOV_COMPRESSION
Please, write it as [.BR "Filesystem support" .]
and maybe I would change it, to be more specific, to the following:
[
see
.B Filesystem support
below.
]
So that the reader clearly understands it's on the same page.
+.I encryption
+is currently always
+.B ENCODED_IOV_ENCRYPTION_NONE
+(zero).
+.PP
+.I unencoded_len
+is the length of the unencoded (i.e., decrypted and decompressed) data.
+.I unencoded_offset
+is the offset into the unencoded data where the data in the file begins
The above wording is a bit unclear to me.
I suggest the following:
[
.I unencoded_offset
is the offset from the begining of the file
to the first byte of the unencoded data
]
+(less than or equal to
+.IR unencoded_len ).
+.I len
+is the length of the data in the file
+(less than or equal to
+.I unencoded_len
+-
Here's a question for Michael (mtk):
I've seen (many) cases where these math operations
are written without spaces,
and in the same line (e.g., [.IR a + b]).
I'd like to know your preferences on this,
or what is actually more extended in the manual pages,
to stick with only one of them.
+.IR unencoded_offset ).
+See
+.B Extent layout
+below for some examples.
+.I
Were you maybe going to add something there?
If not, please remove that [.I].
+.PP
+If the unencoded data is actually longer than
+.IR unencoded_len ,
+then it is truncated;
+if it is shorter, then it is extended with zeroes.
+.PP
+
Please, remove that blank line.
+.BR pwritev2 ()
Should be [.BR pwritev2 (2)]
Michael (mtk),
Am I right in that? Please, confirm.
+uses the metadata specified in
+.IR iov[0] ,
+writes the encoded data from the remaining buffers,
+and returns the number of encoded bytes written
+(that is, the sum of
+.I iov[n].iov_len
+for 1 <=
+.I n
+<
+.IR iovcnt ;
+partial writes will not occur).
+At least one encoding field must be non-zero.
+Note that the encoded data is not validated when it is written;
+if it is not valid (e.g., it cannot be decompressed),
+then a subsequent read may return an error.
+If the
+.I offset
+argument to
+.BR pwritev2 ()
Same as above: specify (2).
+is -1, then the file offset is incremented by
+.IR len .
+If
+.I iov[0].iov_len
+is less than
+.I "sizeof(struct\ encoded_iov)"
[.I] allows spaces, so it should be:
[
.I sizeof(struct encoded_iov)
]
+in the kernel,
+then any fields unknown to userspace are treated as if they were zero;
s/userspace/user space/
See man-pages(7)::STYLE GUIDE::Preferred terms
+if it is greater and any fields unknown to the kernel are non-zero,
+then this returns -1 and sets
+.I errno
+to
+.BR E2BIG .
+.PP
+.BR preadv2 ()
Same as above: specify (2).
+populates the metadata in
+.IR iov[0] ,
+the encoded data in the remaining buffers,
+and returns the number of encoded bytes read.
+This will only return one extent per call.
+This can also read data which is not encoded;
+all encoding fields will be zero in that case.
+If the
+.I offset
+argument to
+.BR preadv2 ()
Smae as above: specify (2).
+is -1, then the file offset is incremented by
+.IR len .
+If
+.I iov[0].iov_len
+is less than
+.I "sizeof(struct\ encoded_iov)"
Don't need '"' nor '\', as above.
+in the kernel and any fields unknown to userspace are non-zero,
s/userspace/user space/
+then
+.BR preadv2 ()
(2)
+returns -1 and sets
+.I errno
+to
+.BR E2BIG ;
+if it is greater,
+then any fields unknown to the kernel are returned as zero.
+If the provided buffers are not large enough to return an entire encoded
+extent,
Please use semantic newlines.
I haven't checked that in the text above,
so if you happen to find that there's any other line
that should also be fixed in that sense, please do so.
To understand 'semantic newlines',
please have a look at
man-pages(7)::STYLE GUIDE::Use semantic newlines
Basically, split lines at the most natural separation point,
instead of just when the line gets over the margin.
+then
+.BR preadv2 ()
(2)
+returns -1 and sets
+.I errno
+to
+.BR ENOBUFS .
+.PP
+As the filesystem page cache typically contains decoded data,
+encoded I/O bypasses the page cache.
+.SS Extent layout
+By using
+.IR len ,
+.IR unencoded_len ,
+and
+.IR unencoded_offset ,
+it is possible to refer to a subset of an unencoded extent.
+.PP
+In the simplest case,
+.I len
+is equal to
+.I unencoded_len
+and
+.I unencoded_offset
+is zero.
+This means that the entire unencoded extent is used.
+.PP
+However, suppose we read 50 bytes into a file
+which contains a single compressed extent.
+The filesystem must still return the entire compressed extent
+for us to be able to decompress it,
+so
+.I unencoded_len
+would be the length of the entire decompressed extent.
+However, because the read was at offset 50,
+the first 50 bytes should be ignored.
+Therefore,
+.I unencoded_offset
+would be 50,
+and
+.I len
+would accordingly be
+.IR unencoded_len\ -\ 50 .
This formats everything as I, except for the last dot.
Replace by:
[
.I unencoded
- 50.
]
Michael (mtk), same as above:
to space, or not to space? That is the question :p
Personally, I find spaces more clear.
+.PP
+Additionally, suppose we want to create an encrypted file with length 500,
+but the file is encrypted with a block cipher using a block size of 4096.
+The unencoded data would therefore include the appropriate padding,
+and
+.I unencoded_len
+would be 4096.
+However, to represent the logical size of the file,
+.I len
+would be 500
+(and
+.I unencoded_offset
+would be 0).
+.PP
+Similar situations can arise in other cases:
+.IP * 3
+If the filesystem pads data to the filesystem block size before compressing,
+then compressed files with a size unaligned to the filesystem block size will
+end with an extent with
+.I len
+<
+.IR unencoded_len .
+.IP *
+Extents cloned from the middle of a larger encoded extent with
+.B FICLONERANGE
+may have a non-zero
+.I unencoded_offset
+and/or
+.I len
+<
+.IR unencoded_len .
+.IP *
+If the middle of an encoded extent is overwritten,
+the filesystem may create extents with a non-zero
+.I unencoded_offset
+and/or
+.I len
+<
+.I unencoded_len
+for the parts that were not overwritten.
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
I haven't found any other "\<vs\>\.".
However, I've found a few "\<vs\>[^\.]".
+.I unencoded_len
+above).
+It may not be intended for this data to be readable.
+.PP
+Therefore, encoded I/O requires privilege.
+Namely, the
+.B RWF_ENCODED
+flag may only be used when the file was opened with the
+.B O_ALLOW_ENCODED
+flag to
+.BR open (2),
+which requires the
+.B CAP_SYS_ADMIN
+capability.
+The
+.B O_CLOEXEC
+flag must be specified in conjunction with
+.BR O_ALLOW_ENCODED .
+This avoids accidentally leaking the encoded I/O privilege
+(it is not cleared on
+.BR fork (2)
+or
+.BR execve (2)
+otherwise).
+If
+.B O_ALLOW_ENCODED
+without
+.B O_CLOEXEC
+is desired,
+.B O_CLOEXEC
+can be cleared afterwards with
+.BR fnctl (2).
+.BR fcntl (2)
+can also clear or set
+.B O_ALLOW_ENCODED
+(including without
+.BR O_CLOEXEC ).
+.SS Filesystem support
+Encoded I/O is supported on the following filesystems:
+.TP
+Btrfs (since Linux 5.12)
+.IP
+Btrfs supports encoded reads and writes of compressed data.
+The data is encoded as follows:
+.RS
+.IP * 3
+If
+.I compression
+is
+.BR ENCODED_IOV_COMPRESSION_BTRFS_ZLIB ,
+then the encoded data is a single zlib stream.
+.IP *
+If
+.I compression
+is
+.BR ENCODED_IOV_COMPRESSION_BTRFS_ZSTD ,
+then the encoded data is a single zstd frame compressed with the
+.I windowLog
+compression parameter set to no more than 17.
+.IP *
+If
+.I compression
+is one of
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_4K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_8K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_16K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_32K ,
+or
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_64K ,
+then the encoded data is compressed page by page
+(using the page size indicated by the name of the constant)
+with LZO1X
+and wrapped in the format documented in the Linux kernel source file
+.IR fs/btrfs/lzo.c .
+.RE
+.IP
+Additionally, there are some restrictions on
+.BR pwritev2 ():
(2)
+.RS
+.IP * 3
+.I offset
+(or the current file offset if
+.I offset
+is -1) must be aligned to the sector size of the filesystem.
+.IP *
+.I len
+must be aligned to the sector size of the filesystem
+unless the data ends at or beyond the current end of the file.
+.IP *
+.I unencoded_len
+and the length of the encoded data must each be no more than 128 KiB.
+This limit may increase in the future.
+.IP *
+The length of the encoded data must be less than or equal to
+.IR unencoded_len .
+.IP *
+If using LZO, the filesystem's page size must match the compression page size.
+.RE
Please, add a SEE ALSO section, which should at least point to
preadv2(2) (or pwritev2(2), if you prefer):
[
.SH SEE ALSO
.BR preadv2 (2)
]
Hi Omar and Michael,
please, see below.
Thanks,
Alex
On 11/20/20 12:29 AM, Alejandro Colomar (mailing lists; readonly) wrote:
Hi Omar,
Please, see some fixes below:
Michael, I've also some questions for you below
(you can grep for mtk to find those).
Thanks,
Alex
On 11/18/20 8:18 PM, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
This adds a new page, encoded_io(7), providing an overview of encoded
I/O and updates fcntl(2), open(2), and preadv2(2)/pwritev2(2) to
reference it.
Cc: Michael Kerrisk <redacted>
Cc: linux-man <redacted>
Signed-off-by: Omar Sandoval <redacted>
---
This feature is not yet upstream.
man2/fcntl.2 | 10 +-
man2/open.2 | 23 +++
man2/readv.2 | 70 +++++++++
man7/encoded_io.7 | 369 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 471 insertions(+), 1 deletion(-)
create mode 100644 man7/encoded_io.7
@@ -221,8 +221,9 @@ On Linux, this command can change only the .BRO_ASYNC, .BRO_DIRECT, .BRO_NOATIME,+.BRO_NONBLOCK, and-.BO_NONBLOCK+.BO_ALLOW_ENCODED flags. It is not possible to change the .BRO_DSYNC
@@ -1820,6 +1821,13 @@ Attempted to clear the flag on a file that has the append-only attribute set. .TP .BEPERM+Attempted to set the+.BO_ALLOW_ENCODED+flag and the calling process did not have the+.BCAP_SYS_ADMIN+capability.+.TP+.BEPERM .Icmd was .BRF_ADD_SEALS,
@@ -437,6 +437,16 @@ was followed by a call to .BRfdatasync(2)). .IR"See NOTES below". .TP+.BO_ALLOW_ENCODED
The list is alphabetically sorted;
please, follow that
(O_ALLOW_ENCODED should be the first one).
quoted
+Open the file with encoded I/O permissions;
+see
+.BR encoded_io (7).
+.B O_CLOEXEC
+must be specified in conjuction with this flag.
+The caller must have the
+.B CAP_SYS_ADMIN
+capability.
+.TP
.B O_EXCL
Ensure that this call creates the file:
if this flag is specified in conjunction with
@@ -1082,6 +1092,14 @@ is invalid (e.g., it contains characters not permitted by the underlying filesystem). .TP .B EINVAL+.B O_ALLOW_ENCODED+was specified in+.IR flags ,+but+.B O_CLOEXEC+was not specified.+.TP+.B EINVAL The final component ("basename") of .I pathname is invalid
@@ -1238,6 +1256,11 @@ did not match the owner of the file and the caller was not privileged. The operation was prevented by a file seal; see .BR fcntl (2). .TP+.B EPERM+The+.B O_ALLOW_ENCODED+flag was specified, but the caller was not privileged.+.TP .B EROFS .I pathname refers to a file on a read-only filesystem and write access was
@@ -264,6 +264,11 @@ the data is always appended to the end of the file. However, if the .Ioffset argument is \-1, the current file offset is updated.+.TP+.BRRWF_ENCODED" (since Linux 5.12)"+Read or write encoded (e.g., compressed) data.+See+.BRencoded_io(7). .SHRETURNVALUE On success, .BRreadv(),
@@ -283,6 +288,13 @@ than requested (see and .BRwrite(2)). .PP+If+.B+RWF_ENCODED
RWF_ENCODED should go in the same line as .B:
[
.B RWF_ENCODED
]
quoted
+was specified in
+.IR flags ,
+then the return value is the number of encoded bytes.
+.PP
On error, \-1 is returned, and \fIerrno\fP is set appropriately.
.SH ERRORS
The errors are as given for
@@ -313,6 +325,64 @@ is less than zero or greater than the permitted maximum. .TP .B EOPNOTSUPP An unknown flag is specified in \fIflags\fP.+.TP+.B EOPNOTSUPP+.B RWF_ENCODED+is specified in+.I flags+and the filesystem does not implement encoded I/O.+.TP+.B EPERM+.B RWF_ENCODED+is specified in+.I flags+and the file was not opened with the+.B O_ALLOW_ENCODED+flag.+.PP+.BR preadv2 ()+can fail for the following reasons:
The wording is a bit unclear:
Above your additions (old text, not yours),
it says that some errors apply to preadv2
(as well as to other functions):
[
ERRORS
The errors are as given for read(2) and write(2). Furthermore,
preadv(), preadv2(), pwritev(), and pwritev2() can also fail
for the same reasons as lseek(2). Additionally, the following
errors are defined:
EINVAL The sum of the iov_len values overflows an ssize_t
value.
EINVAL The vector count, iovcnt, is less than zero or greater
than the permitted maximum.
EOPNOTSUPP
An unknown flag is specified in flags.
EOPNOTSUPP
RWF_ENCODED is specified in flags and the filesystem
does not implement encoded I/O.
EPERM RWF_ENCODED is specified in flags and the file was not
opened with the O_ALLOW_ENCODED flag.
]
And then you added a line that says:
[
preadv2() can fail for the following reasons:
]
Which if read strictly, it says that [only] the following errors apply.
Did you mean that
"preadv3() can _additionally_ fail for the following reasons"?
Could you please be a bit more specific?
The same applies for pwritev2() below.
quoted
+.TP
+.B E2BIG
+.B RWF_ENCODED
+is specified in
+.I flags
+and
+.I iov[0]
+is not large enough to return the encoding metadata.
+.TP
+.B ENOBUFS
+.B RWF_ENCODED
+is specified in
+.I flags
+and the buffers in
+.I iov
+are not big enough to return the encoded data.
+.PP
+.BR pwritev2 ()
+can fail for the following reasons:
+.TP
+.B E2BIG
+.B RWF_ENCODED
+is specified in
+.I flags
+and
+.I iov[0]
+contains non-zero fields
+after the kernel's
+.IR "sizeof(struct\ encoded_iov)" .
Don't escape the space, if the string is already in "".
quoted
+.TP
+.B EINVAL
+.B RWF_ENCODED
+is specified in
+.I flags
+and the encoding is unknown or not supported by the filesystem.
+.TP
+.B EINVAL
+.B RWF_ENCODED
+is specified in
+.I flags
+and the alignment and/or size requirements are not met.
.SH VERSIONS
.BR preadv ()
and
@@ -0,0 +1,369 @@+.\" Copyright (c) 2020 by Omar Sandoval <osandov@fb.com>+.\"+.\" %%%LICENSE_START(VERBATIM)+.\" Permission is granted to make and distribute verbatim copies of this+.\" manual provided the copyright notice and this permission notice are+.\" preserved on all copies.+.\"+.\" Permission is granted to copy and distribute modified versions of this+.\" manual under the conditions for verbatim copying, provided that the+.\" entire resulting derived work is distributed under the terms of a+.\" permission notice identical to this one.+.\"+.\" Since the Linux kernel and libraries are constantly changing, this+.\" manual page may be incorrect or out-of-date. The author(s) assume no+.\" responsibility for errors or omissions, or for damages resulting from+.\" the use of the information contained herein. The author(s) may not+.\" have taken the same level of care in the production of this manual,+.\" which is licensed free of charge, as they might when working+.\" professionally.+.\"+.\" Formatted or processed versions of this manual, if unaccompanied by+.\" the source, must acknowledge the copyright and authors of this work.+.\" %%%LICENSE_END+.\"+.\"+.THENCODED_IO72020-11-11"Linux""Linux Programmer's Manual"+.SHNAME+encoded_io \- overview of encoded I/O+.SHDESCRIPTION+Several filesystems (e.g., Btrfs) support transparent encoding+(e.g., compression, encryption) of data on disk:+written data is encoded by the kernel before it is written to disk,+and read data is decoded before being returned to the user.+In some cases, it is useful to skip this encoding step.
Here I would use ';' instead of '.'
(and next letter would be lowercase, then).
quoted
+For example, the user may want to read the compressed contents of a file
+or write pre-compressed data directly to a file.
+This is referred to as "encoded I/O".
+.SS Encoded I/O API
+Encoded I/O is specified with the
+.B RWF_ENCODED
+flag to
+.BR preadv2 (2)
+and
+.BR pwritev2 (2).
+If
+.B RWF_ENCODED
+is specified, then
+.I iov[0].iov_base
+points to an
+.I
+encoded_iov
On the same line, please.
quoted
+structure, defined in
+.I <linux/fs.h>
+as:
+.PP
+.in +4n
+.EX
+struct encoded_iov {
+ __aligned_u64 len;
+ __aligned_u64 unencoded_len;
+ __aligned_u64 unencoded_offset;
+ __u32 compression;
+ __u32 encryption;
+};
+.EE
+.in
+.PP
+This may be extended in the future, so
+.I iov[0].iov_len
+must be set to
+.I "sizeof(struct\ encoded_iov)"
+for forward/backward compatibility.
+The remaining buffers contain the encoded data.
+.PP
+.I compression
+and
+.I encryption
+are the encoding fields.
+.I compression
+is
+.B ENCODED_IOV_COMPRESSION_NONE
+(zero)
+or a filesystem-specific
+.B ENCODED_IOV_COMPRESSION
Or s/ENCODED_IOV_COMPRESSION/ENCODED_IOV_COMPRESSION_/
I'm not sure about existing practice.
Michael (mtk), what would you do here?
quoted
+constant;
+see
+.BR Filesystem\ support .
Please, write it as [.BR "Filesystem support" .]
and maybe I would change it, to be more specific, to the following:
[
see
.B Filesystem support
below.
]
So that the reader clearly understands it's on the same page.
quoted
+.I encryption
+is currently always
+.B ENCODED_IOV_ENCRYPTION_NONE
+(zero).
+.PP
+.I unencoded_len
+is the length of the unencoded (i.e., decrypted and decompressed) data.
+.I unencoded_offset
+is the offset into the unencoded data where the data in the file begins
The above wording is a bit unclear to me.
I suggest the following:
[
.I unencoded_offset
is the offset from the begining of the file
to the first byte of the unencoded data
]
quoted
+(less than or equal to
+.IR unencoded_len ).
+.I len
+is the length of the data in the file
+(less than or equal to
+.I unencoded_len
+-
Here's a question for Michael (mtk):
I've seen (many) cases where these math operations
are written without spaces,
and in the same line (e.g., [.IR a + b]).
I'd like to know your preferences on this,
or what is actually more extended in the manual pages,
to stick with only one of them.
quoted
+.IR unencoded_offset ).
+See
+.B Extent layout
+below for some examples.
+.I
Were you maybe going to add something there?
If not, please remove that [.I].
quoted
+.PP
+If the unencoded data is actually longer than
+.IR unencoded_len ,
+then it is truncated;
+if it is shorter, then it is extended with zeroes.
+.PP
+
Please, remove that blank line.
quoted
+.BR pwritev2 ()
Should be [.BR pwritev2 (2)]
Michael (mtk),
Am I right in that? Please, confirm.
quoted
+uses the metadata specified in
+.IR iov[0] ,
+writes the encoded data from the remaining buffers,
+and returns the number of encoded bytes written
+(that is, the sum of
+.I iov[n].iov_len
+for 1 <=
+.I n
+<
+.IR iovcnt ;
+partial writes will not occur).
+At least one encoding field must be non-zero.
+Note that the encoded data is not validated when it is written;
+if it is not valid (e.g., it cannot be decompressed),
+then a subsequent read may return an error.
+If the
+.I offset
+argument to
+.BR pwritev2 ()
Same as above: specify (2).
quoted
+is -1, then the file offset is incremented by
+.IR len .
+If
+.I iov[0].iov_len
+is less than
+.I "sizeof(struct\ encoded_iov)"
[.I] allows spaces, so it should be:
[
.I sizeof(struct encoded_iov)
]
quoted
+in the kernel,
+then any fields unknown to userspace are treated as if they were zero;
s/userspace/user space/
See man-pages(7)::STYLE GUIDE::Preferred terms
quoted
+if it is greater and any fields unknown to the kernel are non-zero,
+then this returns -1 and sets
+.I errno
+to
+.BR E2BIG .
+.PP
+.BR preadv2 ()
Same as above: specify (2).
quoted
+populates the metadata in
+.IR iov[0] ,
+the encoded data in the remaining buffers,
+and returns the number of encoded bytes read.
+This will only return one extent per call.
+This can also read data which is not encoded;
+all encoding fields will be zero in that case.
+If the
+.I offset
+argument to
+.BR preadv2 ()
Smae as above: specify (2).
quoted
+is -1, then the file offset is incremented by
+.IR len .
+If
+.I iov[0].iov_len
+is less than
+.I "sizeof(struct\ encoded_iov)"
Don't need '"' nor '\', as above.
quoted
+in the kernel and any fields unknown to userspace are non-zero,
s/userspace/user space/
quoted
+then
+.BR preadv2 ()
(2)
quoted
+returns -1 and sets
+.I errno
+to
+.BR E2BIG ;
+if it is greater,
+then any fields unknown to the kernel are returned as zero.
+If the provided buffers are not large enough to return an entire encoded
+extent,
Please use semantic newlines.
I haven't checked that in the text above,
so if you happen to find that there's any other line
that should also be fixed in that sense, please do so.
To understand 'semantic newlines',
please have a look at
man-pages(7)::STYLE GUIDE::Use semantic newlines
Basically, split lines at the most natural separation point,
instead of just when the line gets over the margin.
quoted
+then
+.BR preadv2 ()
(2)
quoted
+returns -1 and sets
+.I errno
+to
+.BR ENOBUFS .
+.PP
+As the filesystem page cache typically contains decoded data,
+encoded I/O bypasses the page cache.
+.SS Extent layout
+By using
+.IR len ,
+.IR unencoded_len ,
+and
+.IR unencoded_offset ,
+it is possible to refer to a subset of an unencoded extent.
+.PP
+In the simplest case,
+.I len
+is equal to
+.I unencoded_len
+and
+.I unencoded_offset
+is zero.
+This means that the entire unencoded extent is used.
+.PP
+However, suppose we read 50 bytes into a file
+which contains a single compressed extent.
+The filesystem must still return the entire compressed extent
+for us to be able to decompress it,
+so
+.I unencoded_len
+would be the length of the entire decompressed extent.
+However, because the read was at offset 50,
+the first 50 bytes should be ignored.
+Therefore,
+.I unencoded_offset
+would be 50,
+and
+.I len
+would accordingly be
+.IR unencoded_len\ -\ 50 .
This formats everything as I, except for the last dot.
Replace by:
[
.I unencoded
- 50.
]
Michael (mtk), same as above:
to space, or not to space? That is the question :p
Personally, I find spaces more clear.
quoted
+.PP
+Additionally, suppose we want to create an encrypted file with length 500,
+but the file is encrypted with a block cipher using a block size of 4096.
+The unencoded data would therefore include the appropriate padding,
+and
+.I unencoded_len
+would be 4096.
+However, to represent the logical size of the file,
+.I len
+would be 500
+(and
+.I unencoded_offset
+would be 0).
+.PP
+Similar situations can arise in other cases:
+.IP * 3
+If the filesystem pads data to the filesystem block size before compressing,
+then compressed files with a size unaligned to the filesystem block size will
+end with an extent with
+.I len
+<
+.IR unencoded_len .
+.IP *
+Extents cloned from the middle of a larger encoded extent with
+.B FICLONERANGE
+may have a non-zero
+.I unencoded_offset
+and/or
+.I len
+<
+.IR unencoded_len .
+.IP *
+If the middle of an encoded extent is overwritten,
+the filesystem may create extents with a non-zero
+.I unencoded_offset
+and/or
+.I len
+<
+.I unencoded_len
+for the parts that were not overwritten.
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
I haven't found any other "\<vs\>\.".
However, I've found a few "\<vs\>[^\.]".
quoted
+.I unencoded_len
+above).
+It may not be intended for this data to be readable.
+.PP
+Therefore, encoded I/O requires privilege.
+Namely, the
+.B RWF_ENCODED
+flag may only be used when the file was opened with the
+.B O_ALLOW_ENCODED
+flag to
+.BR open (2),
+which requires the
+.B CAP_SYS_ADMIN
+capability.
+The
+.B O_CLOEXEC
+flag must be specified in conjunction with
+.BR O_ALLOW_ENCODED .
+This avoids accidentally leaking the encoded I/O privilege
+(it is not cleared on
+.BR fork (2)
+or
+.BR execve (2)
+otherwise).
+If
+.B O_ALLOW_ENCODED
+without
+.B O_CLOEXEC
+is desired,
+.B O_CLOEXEC
+can be cleared afterwards with
+.BR fnctl (2).
+.BR fcntl (2)
+can also clear or set
+.B O_ALLOW_ENCODED
+(including without
+.BR O_CLOEXEC ).
+.SS Filesystem support
+Encoded I/O is supported on the following filesystems:
+.TP
+Btrfs (since Linux 5.12)
+.IP
+Btrfs supports encoded reads and writes of compressed data.
+The data is encoded as follows:
+.RS
+.IP * 3
+If
+.I compression
+is
+.BR ENCODED_IOV_COMPRESSION_BTRFS_ZLIB ,
+then the encoded data is a single zlib stream.
+.IP *
+If
+.I compression
+is
+.BR ENCODED_IOV_COMPRESSION_BTRFS_ZSTD ,
+then the encoded data is a single zstd frame compressed with the
+.I windowLog
+compression parameter set to no more than 17.
+.IP *
+If
+.I compression
+is one of
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_4K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_8K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_16K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_32K ,
+or
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_64K ,
+then the encoded data is compressed page by page
+(using the page size indicated by the name of the constant)
+with LZO1X
+and wrapped in the format documented in the Linux kernel source file
+.IR fs/btrfs/lzo.c .
+.RE
+.IP
+Additionally, there are some restrictions on
+.BR pwritev2 ():
(2)
quoted
+.RS
+.IP * 3
+.I offset
+(or the current file offset if
+.I offset
+is -1) must be aligned to the sector size of the filesystem.
+.IP *
+.I len
+must be aligned to the sector size of the filesystem
+unless the data ends at or beyond the current end of the file.
+.IP *
+.I unencoded_len
+and the length of the encoded data must each be no more than 128 KiB.
+This limit may increase in the future.
+.IP *
+The length of the encoded data must be less than or equal to
+.IR unencoded_len .
+.IP *
+If using LZO, the filesystem's page size must match the compression page size.
+.RE
Please, add a SEE ALSO section, which should at least point to
preadv2(2) (or pwritev2(2), if you prefer):
[
.SH SEE ALSO
.BR preadv2 (2)
]
Hi Omar,
I found a wording of mine to be a bit confusing.
Please see below.
Thanks,
Alex
On 11/20/20 3:06 PM, Alejandro Colomar (man-pages) wrote:
Hi Omar and Michael,
please, see below.
Thanks,
Alex
On 11/20/20 12:29 AM, Alejandro Colomar (mailing lists; readonly) wrote:
quoted
Hi Omar,
Please, see some fixes below:
Michael, I've also some questions for you below
(you can grep for mtk to find those).
Thanks,
Alex
On 11/18/20 8:18 PM, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
This adds a new page, encoded_io(7), providing an overview of encoded
I/O and updates fcntl(2), open(2), and preadv2(2)/pwritev2(2) to
reference it.
Cc: Michael Kerrisk <redacted>
Cc: linux-man <redacted>
Signed-off-by: Omar Sandoval <redacted>
---
This feature is not yet upstream.
man2/fcntl.2 | 10 +-
man2/open.2 | 23 +++
man2/readv.2 | 70 +++++++++
man7/encoded_io.7 | 369 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 471 insertions(+), 1 deletion(-)
create mode 100644 man7/encoded_io.7
@@ -221,8 +221,9 @@ On Linux, this command can change only the .BRO_ASYNC, .BRO_DIRECT, .BRO_NOATIME,+.BRO_NONBLOCK, and-.BO_NONBLOCK+.BO_ALLOW_ENCODED flags. It is not possible to change the .BRO_DSYNC
@@ -1820,6 +1821,13 @@ Attempted to clear the flag on a file that has the append-only attribute set. .TP .BEPERM+Attempted to set the+.BO_ALLOW_ENCODED+flag and the calling process did not have the+.BCAP_SYS_ADMIN+capability.+.TP+.BEPERM .Icmd was .BRF_ADD_SEALS,
@@ -437,6 +437,16 @@ was followed by a call to .BRfdatasync(2)). .IR"See NOTES below". .TP+.BO_ALLOW_ENCODED
The list is alphabetically sorted;
please, follow that
(O_ALLOW_ENCODED should be the first one).
quoted
+Open the file with encoded I/O permissions;
+see
+.BR encoded_io (7).
+.B O_CLOEXEC
+must be specified in conjuction with this flag.
+The caller must have the
+.B CAP_SYS_ADMIN
+capability.
+.TP
.B O_EXCL
Ensure that this call creates the file:
if this flag is specified in conjunction with
@@ -1082,6 +1092,14 @@ is invalid (e.g., it contains characters not permitted by the underlying filesystem). .TP .B EINVAL+.B O_ALLOW_ENCODED+was specified in+.IR flags ,+but+.B O_CLOEXEC+was not specified.+.TP+.B EINVAL The final component ("basename") of .I pathname is invalid
@@ -1238,6 +1256,11 @@ did not match the owner of the file and the caller was not privileged. The operation was prevented by a file seal; see .BR fcntl (2). .TP+.B EPERM+The+.B O_ALLOW_ENCODED+flag was specified, but the caller was not privileged.+.TP .B EROFS .I pathname refers to a file on a read-only filesystem and write access was
@@ -264,6 +264,11 @@ the data is always appended to the end of the file. However, if the .Ioffset argument is \-1, the current file offset is updated.+.TP+.BRRWF_ENCODED" (since Linux 5.12)"+Read or write encoded (e.g., compressed) data.+See+.BRencoded_io(7). .SHRETURNVALUE On success, .BRreadv(),
@@ -283,6 +288,13 @@ than requested (see and .BRwrite(2)). .PP+If+.B+RWF_ENCODED
RWF_ENCODED should go in the same line as .B:
[
.B RWF_ENCODED
]
quoted
+was specified in
+.IR flags ,
+then the return value is the number of encoded bytes.
+.PP
On error, \-1 is returned, and \fIerrno\fP is set appropriately.
.SH ERRORS
The errors are as given for
@@ -313,6 +325,64 @@ is less than zero or greater than the permitted maximum. .TP .B EOPNOTSUPP An unknown flag is specified in \fIflags\fP.+.TP+.B EOPNOTSUPP+.B RWF_ENCODED+is specified in+.I flags+and the filesystem does not implement encoded I/O.+.TP+.B EPERM+.B RWF_ENCODED+is specified in+.I flags+and the file was not opened with the+.B O_ALLOW_ENCODED+flag.+.PP+.BR preadv2 ()+can fail for the following reasons:
The wording is a bit unclear:
Above your additions (old text, not yours),
it says that some errors apply to preadv2
(as well as to other functions):
[
ERRORS
The errors are as given for read(2) and write(2). Furthermore,
preadv(), preadv2(), pwritev(), and pwritev2() can also fail
for the same reasons as lseek(2). Additionally, the following
errors are defined:
EINVAL The sum of the iov_len values overflows an ssize_t
value.
EINVAL The vector count, iovcnt, is less than zero or greater
than the permitted maximum.
EOPNOTSUPP
An unknown flag is specified in flags.
EOPNOTSUPP
RWF_ENCODED is specified in flags and the filesystem
does not implement encoded I/O.
EPERM RWF_ENCODED is specified in flags and the file was not
opened with the O_ALLOW_ENCODED flag.
]
And then you added a line that says:
[
preadv2() can fail for the following reasons:
]
Which if read strictly, it says that [only] the following errors apply.
Did you mean that
"preadv3() can _additionally_ fail for the following reasons"?
Could you please be a bit more specific?
The same applies for pwritev2() below.
quoted
+.TP
+.B E2BIG
+.B RWF_ENCODED
+is specified in
+.I flags
+and
+.I iov[0]
+is not large enough to return the encoding metadata.
+.TP
+.B ENOBUFS
+.B RWF_ENCODED
+is specified in
+.I flags
+and the buffers in
+.I iov
+are not big enough to return the encoded data.
+.PP
+.BR pwritev2 ()
+can fail for the following reasons:
+.TP
+.B E2BIG
+.B RWF_ENCODED
+is specified in
+.I flags
+and
+.I iov[0]
+contains non-zero fields
+after the kernel's
+.IR "sizeof(struct\ encoded_iov)" .
Don't escape the space, if the string is already in "".
quoted
+.TP
+.B EINVAL
+.B RWF_ENCODED
+is specified in
+.I flags
+and the encoding is unknown or not supported by the filesystem.
+.TP
+.B EINVAL
+.B RWF_ENCODED
+is specified in
+.I flags
+and the alignment and/or size requirements are not met.
.SH VERSIONS
.BR preadv ()
and
@@ -0,0 +1,369 @@+.\" Copyright (c) 2020 by Omar Sandoval <osandov@fb.com>+.\"+.\" %%%LICENSE_START(VERBATIM)+.\" Permission is granted to make and distribute verbatim copies of this+.\" manual provided the copyright notice and this permission notice are+.\" preserved on all copies.+.\"+.\" Permission is granted to copy and distribute modified versions of this+.\" manual under the conditions for verbatim copying, provided that the+.\" entire resulting derived work is distributed under the terms of a+.\" permission notice identical to this one.+.\"+.\" Since the Linux kernel and libraries are constantly changing, this+.\" manual page may be incorrect or out-of-date. The author(s) assume no+.\" responsibility for errors or omissions, or for damages resulting from+.\" the use of the information contained herein. The author(s) may not+.\" have taken the same level of care in the production of this manual,+.\" which is licensed free of charge, as they might when working+.\" professionally.+.\"+.\" Formatted or processed versions of this manual, if unaccompanied by+.\" the source, must acknowledge the copyright and authors of this work.+.\" %%%LICENSE_END+.\"+.\"+.THENCODED_IO72020-11-11"Linux""Linux Programmer's Manual"+.SHNAME+encoded_io \- overview of encoded I/O+.SHDESCRIPTION+Several filesystems (e.g., Btrfs) support transparent encoding+(e.g., compression, encryption) of data on disk:+written data is encoded by the kernel before it is written to disk,+and read data is decoded before being returned to the user.+In some cases, it is useful to skip this encoding step.
Here I would use ';' instead of '.'
(and next letter would be lowercase, then).
quoted
+For example, the user may want to read the compressed contents of a file
+or write pre-compressed data directly to a file.
+This is referred to as "encoded I/O".
+.SS Encoded I/O API
+Encoded I/O is specified with the
+.B RWF_ENCODED
+flag to
+.BR preadv2 (2)
+and
+.BR pwritev2 (2).
+If
+.B RWF_ENCODED
+is specified, then
+.I iov[0].iov_base
+points to an
+.I
+encoded_iov
On the same line, please.
quoted
+structure, defined in
+.I <linux/fs.h>
+as:
+.PP
+.in +4n
+.EX
+struct encoded_iov {
+ __aligned_u64 len;
+ __aligned_u64 unencoded_len;
+ __aligned_u64 unencoded_offset;
+ __u32 compression;
+ __u32 encryption;
+};
+.EE
+.in
+.PP
+This may be extended in the future, so
+.I iov[0].iov_len
+must be set to
+.I "sizeof(struct\ encoded_iov)"
+for forward/backward compatibility.
+The remaining buffers contain the encoded data.
+.PP
+.I compression
+and
+.I encryption
+are the encoding fields.
+.I compression
+is
+.B ENCODED_IOV_COMPRESSION_NONE
+(zero)
+or a filesystem-specific
+.B ENCODED_IOV_COMPRESSION
Or s/ENCODED_IOV_COMPRESSION/ENCODED_IOV_COMPRESSION_/
I'm not sure about existing practice.
Michael (mtk), what would you do here?
quoted
quoted
+constant;
+see
+.BR Filesystem\ support .
Please, write it as [.BR "Filesystem support" .]
and maybe I would change it, to be more specific, to the following:
[
see
.B Filesystem support
below.
]
So that the reader clearly understands it's on the same page.
quoted
+.I encryption
+is currently always
+.B ENCODED_IOV_ENCRYPTION_NONE
+(zero).
+.PP
+.I unencoded_len
+is the length of the unencoded (i.e., decrypted and decompressed) data.
+.I unencoded_offset
+is the offset into the unencoded data where the data in the file begins
The above wording is a bit unclear to me.
I suggest the following:
[
.I unencoded_offset
is the offset from the begining of the file
to the first byte of the unencoded data
]
Now I've read it again, and my wording was even worse than yours.
I think yours can be understood after a few reads.
However, I'll still try to reword mine to see if I add some value:
[
.I unencoded_offset
is the offset from the first byte of the unencoded data
to the first byte of logical data.
]
If you prefer yours, or a mix, that's fine.
quoted
quoted
+(less than or equal to
+.IR unencoded_len ).
+.I len
+is the length of the data in the file
+(less than or equal to
+.I unencoded_len
+-
Here's a question for Michael (mtk):
I've seen (many) cases where these math operations
are written without spaces,
and in the same line (e.g., [.IR a + b]).
I'd like to know your preferences on this,
or what is actually more extended in the manual pages,
to stick with only one of them.
quoted
+.IR unencoded_offset ).
+See
+.B Extent layout
+below for some examples.
+.I
Were you maybe going to add something there?
If not, please remove that [.I].
quoted
+.PP
+If the unencoded data is actually longer than
+.IR unencoded_len ,
+then it is truncated;
+if it is shorter, then it is extended with zeroes.
+.PP
+
Please, remove that blank line.
quoted
+.BR pwritev2 ()
Should be [.BR pwritev2 (2)]
Michael (mtk),
Am I right in that? Please, confirm.
quoted
+uses the metadata specified in
+.IR iov[0] ,
+writes the encoded data from the remaining buffers,
+and returns the number of encoded bytes written
+(that is, the sum of
+.I iov[n].iov_len
+for 1 <=
+.I n
+<
+.IR iovcnt ;
+partial writes will not occur).
+At least one encoding field must be non-zero.
+Note that the encoded data is not validated when it is written;
+if it is not valid (e.g., it cannot be decompressed),
+then a subsequent read may return an error.
+If the
+.I offset
+argument to
+.BR pwritev2 ()
Same as above: specify (2).
quoted
+is -1, then the file offset is incremented by
+.IR len .
+If
+.I iov[0].iov_len
+is less than
+.I "sizeof(struct\ encoded_iov)"
[.I] allows spaces, so it should be:
[
.I sizeof(struct encoded_iov)
]
quoted
+in the kernel,
+then any fields unknown to userspace are treated as if they were zero;
s/userspace/user space/
See man-pages(7)::STYLE GUIDE::Preferred terms
quoted
+if it is greater and any fields unknown to the kernel are non-zero,
+then this returns -1 and sets
+.I errno
+to
+.BR E2BIG .
+.PP
+.BR preadv2 ()
Same as above: specify (2).
quoted
+populates the metadata in
+.IR iov[0] ,
+the encoded data in the remaining buffers,
+and returns the number of encoded bytes read.
+This will only return one extent per call.
+This can also read data which is not encoded;
+all encoding fields will be zero in that case.
+If the
+.I offset
+argument to
+.BR preadv2 ()
Smae as above: specify (2).
quoted
+is -1, then the file offset is incremented by
+.IR len .
+If
+.I iov[0].iov_len
+is less than
+.I "sizeof(struct\ encoded_iov)"
Don't need '"' nor '\', as above.
quoted
+in the kernel and any fields unknown to userspace are non-zero,
s/userspace/user space/
quoted
+then
+.BR preadv2 ()
(2)
quoted
+returns -1 and sets
+.I errno
+to
+.BR E2BIG ;
+if it is greater,
+then any fields unknown to the kernel are returned as zero.
+If the provided buffers are not large enough to return an entire encoded
+extent,
Please use semantic newlines.
I haven't checked that in the text above,
so if you happen to find that there's any other line
that should also be fixed in that sense, please do so.
To understand 'semantic newlines',
please have a look at
man-pages(7)::STYLE GUIDE::Use semantic newlines
Basically, split lines at the most natural separation point,
instead of just when the line gets over the margin.
quoted
+then
+.BR preadv2 ()
(2)
quoted
+returns -1 and sets
+.I errno
+to
+.BR ENOBUFS .
+.PP
+As the filesystem page cache typically contains decoded data,
+encoded I/O bypasses the page cache.
+.SS Extent layout
+By using
+.IR len ,
+.IR unencoded_len ,
+and
+.IR unencoded_offset ,
+it is possible to refer to a subset of an unencoded extent.
+.PP
+In the simplest case,
+.I len
+is equal to
+.I unencoded_len
+and
+.I unencoded_offset
+is zero.
+This means that the entire unencoded extent is used.
+.PP
+However, suppose we read 50 bytes into a file
+which contains a single compressed extent.
+The filesystem must still return the entire compressed extent
+for us to be able to decompress it,
+so
+.I unencoded_len
+would be the length of the entire decompressed extent.
+However, because the read was at offset 50,
+the first 50 bytes should be ignored.
+Therefore,
+.I unencoded_offset
+would be 50,
+and
+.I len
+would accordingly be
+.IR unencoded_len\ -\ 50 .
This formats everything as I, except for the last dot.
Replace by:
[
.I unencoded
- 50.
]
Michael (mtk), same as above:
to space, or not to space? That is the question :p
Personally, I find spaces more clear.
quoted
+.PP
+Additionally, suppose we want to create an encrypted file with length 500,
+but the file is encrypted with a block cipher using a block size of 4096.
+The unencoded data would therefore include the appropriate padding,
+and
+.I unencoded_len
+would be 4096.
+However, to represent the logical size of the file,
+.I len
+would be 500
+(and
+.I unencoded_offset
+would be 0).
+.PP
+Similar situations can arise in other cases:
+.IP * 3
+If the filesystem pads data to the filesystem block size before compressing,
+then compressed files with a size unaligned to the filesystem block size will
+end with an extent with
+.I len
+<
+.IR unencoded_len .
+.IP *
+Extents cloned from the middle of a larger encoded extent with
+.B FICLONERANGE
+may have a non-zero
+.I unencoded_offset
+and/or
+.I len
+<
+.IR unencoded_len .
+.IP *
+If the middle of an encoded extent is overwritten,
+the filesystem may create extents with a non-zero
+.I unencoded_offset
+and/or
+.I len
+<
+.I unencoded_len
+for the parts that were not overwritten.
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
I haven't found any other "\<vs\>\.".
However, I've found a few "\<vs\>[^\.]".
quoted
+.I unencoded_len
+above).
+It may not be intended for this data to be readable.
+.PP
+Therefore, encoded I/O requires privilege.
+Namely, the
+.B RWF_ENCODED
+flag may only be used when the file was opened with the
+.B O_ALLOW_ENCODED
+flag to
+.BR open (2),
+which requires the
+.B CAP_SYS_ADMIN
+capability.
+The
+.B O_CLOEXEC
+flag must be specified in conjunction with
+.BR O_ALLOW_ENCODED .
+This avoids accidentally leaking the encoded I/O privilege
+(it is not cleared on
+.BR fork (2)
+or
+.BR execve (2)
+otherwise).
+If
+.B O_ALLOW_ENCODED
+without
+.B O_CLOEXEC
+is desired,
+.B O_CLOEXEC
+can be cleared afterwards with
+.BR fnctl (2).
+.BR fcntl (2)
+can also clear or set
+.B O_ALLOW_ENCODED
+(including without
+.BR O_CLOEXEC ).
+.SS Filesystem support
+Encoded I/O is supported on the following filesystems:
+.TP
+Btrfs (since Linux 5.12)
+.IP
+Btrfs supports encoded reads and writes of compressed data.
+The data is encoded as follows:
+.RS
+.IP * 3
+If
+.I compression
+is
+.BR ENCODED_IOV_COMPRESSION_BTRFS_ZLIB ,
+then the encoded data is a single zlib stream.
+.IP *
+If
+.I compression
+is
+.BR ENCODED_IOV_COMPRESSION_BTRFS_ZSTD ,
+then the encoded data is a single zstd frame compressed with the
+.I windowLog
+compression parameter set to no more than 17.
+.IP *
+If
+.I compression
+is one of
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_4K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_8K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_16K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_32K ,
+or
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_64K ,
+then the encoded data is compressed page by page
+(using the page size indicated by the name of the constant)
+with LZO1X
+and wrapped in the format documented in the Linux kernel source file
+.IR fs/btrfs/lzo.c .
+.RE
+.IP
+Additionally, there are some restrictions on
+.BR pwritev2 ():
(2)
quoted
+.RS
+.IP * 3
+.I offset
+(or the current file offset if
+.I offset
+is -1) must be aligned to the sector size of the filesystem.
+.IP *
+.I len
+must be aligned to the sector size of the filesystem
+unless the data ends at or beyond the current end of the file.
+.IP *
+.I unencoded_len
+and the length of the encoded data must each be no more than 128 KiB.
+This limit may increase in the future.
+.IP *
+The length of the encoded data must be less than or equal to
+.IR unencoded_len .
+.IP *
+If using LZO, the filesystem's page size must match the compression page size.
+.RE
Please, add a SEE ALSO section, which should at least point to
preadv2(2) (or pwritev2(2), if you prefer):
[
.SH SEE ALSO
.BR preadv2 (2)
]
On Thu, Nov 19, 2020 at 8:03 AM Amir Goldstein [off-list ref] wrote:
On Wed, Nov 18, 2020 at 9:18 PM Omar Sandoval [off-list ref] wrote:
quoted
The upcoming RWF_ENCODED operation introduces some security concerns:
1. Compressed writes will pass arbitrary data to decompression
algorithms in the kernel.
2. Compressed reads can leak truncated/hole punched data.
Therefore, we need to require privilege for RWF_ENCODED. It's not
possible to do the permissions checks at the time of the read or write
because, e.g., io_uring submits IO from a worker thread. So, add an open
flag which requires CAP_SYS_ADMIN. It can also be set and cleared with
fcntl(). The flag is not cleared in any way on fork or exec. It must be
combined with O_CLOEXEC when opening to avoid accidental leaks (if
needed, it may be set without O_CLOEXEC by using fnctl()).
Note that the usual issue that unknown open flags are ignored doesn't
really matter for O_ALLOW_ENCODED; if the kernel doesn't support
O_ALLOW_ENCODED, then it doesn't support RWF_ENCODED, either.
dup() can also result in accidental leak.
We could fail dup() of fd without O_CLOEXEC. Should we?
If we should than what error code should it be? We could return EPERM,
but since we do allow to clear O_CLOEXEC or set O_ALLOW_ENCODED
after open, EPERM seems a tad harsh.
EINVAL seems inappropriate because the error has nothing to do with
input args of dup() and EBADF would also be confusing.
This seems very arbitrary to me. Sure, leaking these file descriptors
wouldn't be great, but there are plenty of other types of file
descriptors that are probably more sensitive. (Writable file
descriptors to databases, to important configuration files, to
io_uring instances, and so on.) So I don't see why this specific
feature should impose such special rules on it.
From: David Sterba <hidden> Date: 2020-11-23 17:10:43
On Wed, Nov 18, 2020 at 11:18:11AM -0800, Omar Sandoval wrote:
From: Omar Sandoval <redacted>
btrfs_write_check() has two related bugs:
1. It gets the iov_iter count before calling generic_write_checks(), but
generic_write_checks() may truncate the iov_iter.
2. It returns the count or negative errno as a size_t, which the callers
cast to an int. If the count is greater than INT_MAX, this overflows.
To fix both of these, pull the call to generic_write_checks() out of
btrfs_write_check(), use the new iov_iter count returned from
generic_write_checks(), and have btrfs_write_check() return 0 or a
negative errno as an int instead of the count. This rearrangement also
paves the way for RWF_ENCODED write support.
Fixes: f945968ff64c ("btrfs: introduce btrfs_write_check()")
This patch is still in misc-next and the commit id is unstable, so this
would rather be folded to the patch.
From: David Sterba <hidden> Date: 2020-11-23 17:11:48
On Wed, Nov 18, 2020 at 11:18:12AM -0800, Omar Sandoval wrote:
From: Omar Sandoval <redacted>
Commit 1dae796aabf6 ("btrfs: inode: sink parameter start and len to
check_data_csum()") replaced the start parameter to check_data_csum()
with page_offset(), but page_offset() is not meaningful for direct I/O
pages. Bring back the start parameter.
Fixes: 1dae796aabf6 ("btrfs: inode: sink parameter start and len to check_data_csum()")
This is part of the subpage preparatory patches still in misc-next , I
can drop the part that removes the start parameter if you're going to
use it.
On Mon, Nov 23, 2020 at 06:08:31PM +0100, David Sterba wrote:
On Wed, Nov 18, 2020 at 11:18:11AM -0800, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
btrfs_write_check() has two related bugs:
1. It gets the iov_iter count before calling generic_write_checks(), but
generic_write_checks() may truncate the iov_iter.
2. It returns the count or negative errno as a size_t, which the callers
cast to an int. If the count is greater than INT_MAX, this overflows.
To fix both of these, pull the call to generic_write_checks() out of
btrfs_write_check(), use the new iov_iter count returned from
generic_write_checks(), and have btrfs_write_check() return 0 or a
negative errno as an int instead of the count. This rearrangement also
paves the way for RWF_ENCODED write support.
Fixes: f945968ff64c ("btrfs: introduce btrfs_write_check()")
This patch is still in misc-next and the commit id is unstable, so this
would rather be folded to the patch.
Looks like you folded this in on misc-next, thanks!
On Mon, Nov 23, 2020 at 06:09:56PM +0100, David Sterba wrote:
On Wed, Nov 18, 2020 at 11:18:12AM -0800, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
Commit 1dae796aabf6 ("btrfs: inode: sink parameter start and len to
check_data_csum()") replaced the start parameter to check_data_csum()
with page_offset(), but page_offset() is not meaningful for direct I/O
pages. Bring back the start parameter.
Fixes: 1dae796aabf6 ("btrfs: inode: sink parameter start and len to check_data_csum()")
This is part of the subpage preparatory patches still in misc-next , I
can drop the part that removes the start parameter if you're going to
use it.
To be clear, the original patch is buggy. It causes check_data_csum() to
print nonsense for checksum errors encountered during direct I/O. So,
this should be probably be folded in to the original patch.
On Sat, Nov 21, 2020 at 12:41:23AM +0100, Jann Horn wrote:
On Thu, Nov 19, 2020 at 8:03 AM Amir Goldstein [off-list ref] wrote:
quoted
On Wed, Nov 18, 2020 at 9:18 PM Omar Sandoval [off-list ref] wrote:
quoted
The upcoming RWF_ENCODED operation introduces some security concerns:
1. Compressed writes will pass arbitrary data to decompression
algorithms in the kernel.
2. Compressed reads can leak truncated/hole punched data.
Therefore, we need to require privilege for RWF_ENCODED. It's not
possible to do the permissions checks at the time of the read or write
because, e.g., io_uring submits IO from a worker thread. So, add an open
flag which requires CAP_SYS_ADMIN. It can also be set and cleared with
fcntl(). The flag is not cleared in any way on fork or exec. It must be
combined with O_CLOEXEC when opening to avoid accidental leaks (if
needed, it may be set without O_CLOEXEC by using fnctl()).
Note that the usual issue that unknown open flags are ignored doesn't
really matter for O_ALLOW_ENCODED; if the kernel doesn't support
O_ALLOW_ENCODED, then it doesn't support RWF_ENCODED, either.
dup() can also result in accidental leak.
We could fail dup() of fd without O_CLOEXEC. Should we?
If we should than what error code should it be? We could return EPERM,
but since we do allow to clear O_CLOEXEC or set O_ALLOW_ENCODED
after open, EPERM seems a tad harsh.
EINVAL seems inappropriate because the error has nothing to do with
input args of dup() and EBADF would also be confusing.
This seems very arbitrary to me. Sure, leaking these file descriptors
wouldn't be great, but there are plenty of other types of file
descriptors that are probably more sensitive. (Writable file
descriptors to databases, to important configuration files, to
io_uring instances, and so on.) So I don't see why this specific
feature should impose such special rules on it.
I agree with Jann. I'm okay with the O_CLOEXEC-on-open requirement if it
makes people more comfortable, but I don't think we should be bending
over backwards to block it anywhere else.
From: Amir Goldstein <amir73il@gmail.com> Date: 2020-12-01 08:17:12
On Mon, Nov 30, 2020 at 9:26 PM Omar Sandoval [off-list ref] wrote:
On Sat, Nov 21, 2020 at 12:41:23AM +0100, Jann Horn wrote:
quoted
On Thu, Nov 19, 2020 at 8:03 AM Amir Goldstein [off-list ref] wrote:
quoted
On Wed, Nov 18, 2020 at 9:18 PM Omar Sandoval [off-list ref] wrote:
quoted
The upcoming RWF_ENCODED operation introduces some security concerns:
1. Compressed writes will pass arbitrary data to decompression
algorithms in the kernel.
2. Compressed reads can leak truncated/hole punched data.
Therefore, we need to require privilege for RWF_ENCODED. It's not
possible to do the permissions checks at the time of the read or write
because, e.g., io_uring submits IO from a worker thread. So, add an open
flag which requires CAP_SYS_ADMIN. It can also be set and cleared with
fcntl(). The flag is not cleared in any way on fork or exec. It must be
combined with O_CLOEXEC when opening to avoid accidental leaks (if
needed, it may be set without O_CLOEXEC by using fnctl()).
Note that the usual issue that unknown open flags are ignored doesn't
really matter for O_ALLOW_ENCODED; if the kernel doesn't support
O_ALLOW_ENCODED, then it doesn't support RWF_ENCODED, either.
dup() can also result in accidental leak.
We could fail dup() of fd without O_CLOEXEC. Should we?
If we should than what error code should it be? We could return EPERM,
but since we do allow to clear O_CLOEXEC or set O_ALLOW_ENCODED
after open, EPERM seems a tad harsh.
EINVAL seems inappropriate because the error has nothing to do with
input args of dup() and EBADF would also be confusing.
This seems very arbitrary to me. Sure, leaking these file descriptors
wouldn't be great, but there are plenty of other types of file
descriptors that are probably more sensitive. (Writable file
descriptors to databases, to important configuration files, to
io_uring instances, and so on.) So I don't see why this specific
feature should impose such special rules on it.
I agree with Jann. I'm okay with the O_CLOEXEC-on-open requirement if it
makes people more comfortable, but I don't think we should be bending
over backwards to block it anywhere else.
I'm fine with or without the O_CLOEXEC-on-open requirement.
Just pointing out the weirdness.
Thanks,
Amir.
Hello Michael,
Could you please have a look at a few doubts down there?
Just grep 'mtk' and you'll find them ;)
Thanks,
Alex
On 11/20/20 4:03 PM, Alejandro Colomar (man-pages) wrote:
Hi Omar,
I found a wording of mine to be a bit confusing.
Please see below.
Thanks,
Alex
On 11/20/20 3:06 PM, Alejandro Colomar (man-pages) wrote:
quoted
Hi Omar and Michael,
please, see below.
Thanks,
Alex
On 11/20/20 12:29 AM, Alejandro Colomar (mailing lists; readonly) wrote:
quoted
Hi Omar,
Please, see some fixes below:
Michael, I've also some questions for you below
(you can grep for mtk to find those).
Thanks,
Alex
On 11/18/20 8:18 PM, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
This adds a new page, encoded_io(7), providing an overview of encoded
I/O and updates fcntl(2), open(2), and preadv2(2)/pwritev2(2) to
reference it.
Cc: Michael Kerrisk <redacted>
Cc: linux-man <redacted>
Signed-off-by: Omar Sandoval <redacted>
---
This feature is not yet upstream.
man2/fcntl.2 | 10 +-
man2/open.2 | 23 +++
man2/readv.2 | 70 +++++++++
man7/encoded_io.7 | 369 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 471 insertions(+), 1 deletion(-)
create mode 100644 man7/encoded_io.7
@@ -221,8 +221,9 @@ On Linux, this command can change only the .BRO_ASYNC, .BRO_DIRECT, .BRO_NOATIME,+.BRO_NONBLOCK, and-.BO_NONBLOCK+.BO_ALLOW_ENCODED flags. It is not possible to change the .BRO_DSYNC
@@ -1820,6 +1821,13 @@ Attempted to clear the flag on a file that has the append-only attribute set. .TP .BEPERM+Attempted to set the+.BO_ALLOW_ENCODED+flag and the calling process did not have the+.BCAP_SYS_ADMIN+capability.+.TP+.BEPERM .Icmd was .BRF_ADD_SEALS,
@@ -437,6 +437,16 @@ was followed by a call to .BRfdatasync(2)). .IR"See NOTES below". .TP+.BO_ALLOW_ENCODED
The list is alphabetically sorted;
please, follow that
(O_ALLOW_ENCODED should be the first one).
quoted
+Open the file with encoded I/O permissions;
+see
+.BR encoded_io (7).
+.B O_CLOEXEC
+must be specified in conjuction with this flag.
+The caller must have the
+.B CAP_SYS_ADMIN
+capability.
+.TP
.B O_EXCL
Ensure that this call creates the file:
if this flag is specified in conjunction with
@@ -1082,6 +1092,14 @@ is invalid (e.g., it contains characters not permitted by the underlying filesystem). .TP .B EINVAL+.B O_ALLOW_ENCODED+was specified in+.IR flags ,+but+.B O_CLOEXEC+was not specified.+.TP+.B EINVAL The final component ("basename") of .I pathname is invalid
@@ -1238,6 +1256,11 @@ did not match the owner of the file and the caller was not privileged. The operation was prevented by a file seal; see .BR fcntl (2). .TP+.B EPERM+The+.B O_ALLOW_ENCODED+flag was specified, but the caller was not privileged.+.TP .B EROFS .I pathname refers to a file on a read-only filesystem and write access was
@@ -264,6 +264,11 @@ the data is always appended to the end of the file. However, if the .Ioffset argument is \-1, the current file offset is updated.+.TP+.BRRWF_ENCODED" (since Linux 5.12)"+Read or write encoded (e.g., compressed) data.+See+.BRencoded_io(7). .SHRETURNVALUE On success, .BRreadv(),
@@ -283,6 +288,13 @@ than requested (see and .BRwrite(2)). .PP+If+.B+RWF_ENCODED
RWF_ENCODED should go in the same line as .B:
[
.B RWF_ENCODED
]
quoted
+was specified in
+.IR flags ,
+then the return value is the number of encoded bytes.
+.PP
On error, \-1 is returned, and \fIerrno\fP is set appropriately.
.SH ERRORS
The errors are as given for
@@ -313,6 +325,64 @@ is less than zero or greater than the permitted maximum. .TP .B EOPNOTSUPP An unknown flag is specified in \fIflags\fP.+.TP+.B EOPNOTSUPP+.B RWF_ENCODED+is specified in+.I flags+and the filesystem does not implement encoded I/O.+.TP+.B EPERM+.B RWF_ENCODED+is specified in+.I flags+and the file was not opened with the+.B O_ALLOW_ENCODED+flag.+.PP+.BR preadv2 ()+can fail for the following reasons:
The wording is a bit unclear:
Above your additions (old text, not yours),
it says that some errors apply to preadv2
(as well as to other functions):
[
ERRORS
The errors are as given for read(2) and write(2). Furthermore,
preadv(), preadv2(), pwritev(), and pwritev2() can also fail
for the same reasons as lseek(2). Additionally, the following
errors are defined:
EINVAL The sum of the iov_len values overflows an ssize_t
value.
EINVAL The vector count, iovcnt, is less than zero or greater
than the permitted maximum.
EOPNOTSUPP
An unknown flag is specified in flags.
EOPNOTSUPP
RWF_ENCODED is specified in flags and the filesystem
does not implement encoded I/O.
EPERM RWF_ENCODED is specified in flags and the file was not
opened with the O_ALLOW_ENCODED flag.
]
And then you added a line that says:
[
preadv2() can fail for the following reasons:
]
Which if read strictly, it says that [only] the following errors apply.
Did you mean that
"preadv3() can _additionally_ fail for the following reasons"?
Could you please be a bit more specific?
The same applies for pwritev2() below.
quoted
+.TP
+.B E2BIG
+.B RWF_ENCODED
+is specified in
+.I flags
+and
+.I iov[0]
+is not large enough to return the encoding metadata.
+.TP
+.B ENOBUFS
+.B RWF_ENCODED
+is specified in
+.I flags
+and the buffers in
+.I iov
+are not big enough to return the encoded data.
+.PP
+.BR pwritev2 ()
+can fail for the following reasons:
+.TP
+.B E2BIG
+.B RWF_ENCODED
+is specified in
+.I flags
+and
+.I iov[0]
+contains non-zero fields
+after the kernel's
+.IR "sizeof(struct\ encoded_iov)" .
Don't escape the space, if the string is already in "".
quoted
+.TP
+.B EINVAL
+.B RWF_ENCODED
+is specified in
+.I flags
+and the encoding is unknown or not supported by the filesystem.
+.TP
+.B EINVAL
+.B RWF_ENCODED
+is specified in
+.I flags
+and the alignment and/or size requirements are not met.
.SH VERSIONS
.BR preadv ()
and
@@ -0,0 +1,369 @@+.\" Copyright (c) 2020 by Omar Sandoval <osandov@fb.com>+.\"+.\" %%%LICENSE_START(VERBATIM)+.\" Permission is granted to make and distribute verbatim copies of this+.\" manual provided the copyright notice and this permission notice are+.\" preserved on all copies.+.\"+.\" Permission is granted to copy and distribute modified versions of this+.\" manual under the conditions for verbatim copying, provided that the+.\" entire resulting derived work is distributed under the terms of a+.\" permission notice identical to this one.+.\"+.\" Since the Linux kernel and libraries are constantly changing, this+.\" manual page may be incorrect or out-of-date. The author(s) assume no+.\" responsibility for errors or omissions, or for damages resulting from+.\" the use of the information contained herein. The author(s) may not+.\" have taken the same level of care in the production of this manual,+.\" which is licensed free of charge, as they might when working+.\" professionally.+.\"+.\" Formatted or processed versions of this manual, if unaccompanied by+.\" the source, must acknowledge the copyright and authors of this work.+.\" %%%LICENSE_END+.\"+.\"+.THENCODED_IO72020-11-11"Linux""Linux Programmer's Manual"+.SHNAME+encoded_io \- overview of encoded I/O+.SHDESCRIPTION+Several filesystems (e.g., Btrfs) support transparent encoding+(e.g., compression, encryption) of data on disk:+written data is encoded by the kernel before it is written to disk,+and read data is decoded before being returned to the user.+In some cases, it is useful to skip this encoding step.
Here I would use ';' instead of '.'
(and next letter would be lowercase, then).
quoted
+For example, the user may want to read the compressed contents of a file
+or write pre-compressed data directly to a file.
+This is referred to as "encoded I/O".
+.SS Encoded I/O API
+Encoded I/O is specified with the
+.B RWF_ENCODED
+flag to
+.BR preadv2 (2)
+and
+.BR pwritev2 (2).
+If
+.B RWF_ENCODED
+is specified, then
+.I iov[0].iov_base
+points to an
+.I
+encoded_iov
On the same line, please.
quoted
+structure, defined in
+.I <linux/fs.h>
+as:
+.PP
+.in +4n
+.EX
+struct encoded_iov {
+ __aligned_u64 len;
+ __aligned_u64 unencoded_len;
+ __aligned_u64 unencoded_offset;
+ __u32 compression;
+ __u32 encryption;
+};
+.EE
+.in
+.PP
+This may be extended in the future, so
+.I iov[0].iov_len
+must be set to
+.I "sizeof(struct\ encoded_iov)"
+for forward/backward compatibility.
+The remaining buffers contain the encoded data.
+.PP
+.I compression
+and
+.I encryption
+are the encoding fields.
+.I compression
+is
+.B ENCODED_IOV_COMPRESSION_NONE
+(zero)
+or a filesystem-specific
+.B ENCODED_IOV_COMPRESSION
Or s/ENCODED_IOV_COMPRESSION/ENCODED_IOV_COMPRESSION_/
I'm not sure about existing practice.
Michael (mtk), what would you do here?
quoted
quoted
+constant;
+see
+.BR Filesystem\ support .
Please, write it as [.BR "Filesystem support" .]
and maybe I would change it, to be more specific, to the following:
[
see
.B Filesystem support
below.
]
So that the reader clearly understands it's on the same page.
quoted
+.I encryption
+is currently always
+.B ENCODED_IOV_ENCRYPTION_NONE
+(zero).
+.PP
+.I unencoded_len
+is the length of the unencoded (i.e., decrypted and decompressed) data.
+.I unencoded_offset
+is the offset into the unencoded data where the data in the file begins
The above wording is a bit unclear to me.
I suggest the following:
[
.I unencoded_offset
is the offset from the begining of the file
to the first byte of the unencoded data
]
Now I've read it again, and my wording was even worse than yours.
I think yours can be understood after a few reads.
However, I'll still try to reword mine to see if I add some value:
[
.I unencoded_offset
is the offset from the first byte of the unencoded data
to the first byte of logical data.
]
If you prefer yours, or a mix, that's fine.
quoted
quoted
quoted
+(less than or equal to
+.IR unencoded_len ).
+.I len
+is the length of the data in the file
+(less than or equal to
+.I unencoded_len
+-
Here's a question for Michael (mtk):
I've seen (many) cases where these math operations
are written without spaces,
and in the same line (e.g., [.IR a + b]).
I'd like to know your preferences on this,
or what is actually more extended in the manual pages,
to stick with only one of them.
quoted
+.IR unencoded_offset ).
+See
+.B Extent layout
+below for some examples.
+.I
Were you maybe going to add something there?
If not, please remove that [.I].
quoted
+.PP
+If the unencoded data is actually longer than
+.IR unencoded_len ,
+then it is truncated;
+if it is shorter, then it is extended with zeroes.
+.PP
+
Please, remove that blank line.
quoted
+.BR pwritev2 ()
Should be [.BR pwritev2 (2)]
Michael (mtk),
Am I right in that? Please, confirm.
quoted
+uses the metadata specified in
+.IR iov[0] ,
+writes the encoded data from the remaining buffers,
+and returns the number of encoded bytes written
+(that is, the sum of
+.I iov[n].iov_len
+for 1 <=
+.I n
+<
+.IR iovcnt ;
+partial writes will not occur).
+At least one encoding field must be non-zero.
+Note that the encoded data is not validated when it is written;
+if it is not valid (e.g., it cannot be decompressed),
+then a subsequent read may return an error.
+If the
+.I offset
+argument to
+.BR pwritev2 ()
Same as above: specify (2).
quoted
+is -1, then the file offset is incremented by
+.IR len .
+If
+.I iov[0].iov_len
+is less than
+.I "sizeof(struct\ encoded_iov)"
[.I] allows spaces, so it should be:
[
.I sizeof(struct encoded_iov)
]
quoted
+in the kernel,
+then any fields unknown to userspace are treated as if they were zero;
s/userspace/user space/
See man-pages(7)::STYLE GUIDE::Preferred terms
quoted
+if it is greater and any fields unknown to the kernel are non-zero,
+then this returns -1 and sets
+.I errno
+to
+.BR E2BIG .
+.PP
+.BR preadv2 ()
Same as above: specify (2).
quoted
+populates the metadata in
+.IR iov[0] ,
+the encoded data in the remaining buffers,
+and returns the number of encoded bytes read.
+This will only return one extent per call.
+This can also read data which is not encoded;
+all encoding fields will be zero in that case.
+If the
+.I offset
+argument to
+.BR preadv2 ()
Smae as above: specify (2).
quoted
+is -1, then the file offset is incremented by
+.IR len .
+If
+.I iov[0].iov_len
+is less than
+.I "sizeof(struct\ encoded_iov)"
Don't need '"' nor '\', as above.
quoted
+in the kernel and any fields unknown to userspace are non-zero,
s/userspace/user space/
quoted
+then
+.BR preadv2 ()
(2)
quoted
+returns -1 and sets
+.I errno
+to
+.BR E2BIG ;
+if it is greater,
+then any fields unknown to the kernel are returned as zero.
+If the provided buffers are not large enough to return an entire encoded
+extent,
Please use semantic newlines.
I haven't checked that in the text above,
so if you happen to find that there's any other line
that should also be fixed in that sense, please do so.
To understand 'semantic newlines',
please have a look at
man-pages(7)::STYLE GUIDE::Use semantic newlines
Basically, split lines at the most natural separation point,
instead of just when the line gets over the margin.
quoted
+then
+.BR preadv2 ()
(2)
quoted
+returns -1 and sets
+.I errno
+to
+.BR ENOBUFS .
+.PP
+As the filesystem page cache typically contains decoded data,
+encoded I/O bypasses the page cache.
+.SS Extent layout
+By using
+.IR len ,
+.IR unencoded_len ,
+and
+.IR unencoded_offset ,
+it is possible to refer to a subset of an unencoded extent.
+.PP
+In the simplest case,
+.I len
+is equal to
+.I unencoded_len
+and
+.I unencoded_offset
+is zero.
+This means that the entire unencoded extent is used.
+.PP
+However, suppose we read 50 bytes into a file
+which contains a single compressed extent.
+The filesystem must still return the entire compressed extent
+for us to be able to decompress it,
+so
+.I unencoded_len
+would be the length of the entire decompressed extent.
+However, because the read was at offset 50,
+the first 50 bytes should be ignored.
+Therefore,
+.I unencoded_offset
+would be 50,
+and
+.I len
+would accordingly be
+.IR unencoded_len\ -\ 50 .
This formats everything as I, except for the last dot.
Replace by:
[
.I unencoded
- 50.
]
Michael (mtk), same as above:
to space, or not to space? That is the question :p
Personally, I find spaces more clear.
quoted
+.PP
+Additionally, suppose we want to create an encrypted file with length 500,
+but the file is encrypted with a block cipher using a block size of 4096.
+The unencoded data would therefore include the appropriate padding,
+and
+.I unencoded_len
+would be 4096.
+However, to represent the logical size of the file,
+.I len
+would be 500
+(and
+.I unencoded_offset
+would be 0).
+.PP
+Similar situations can arise in other cases:
+.IP * 3
+If the filesystem pads data to the filesystem block size before compressing,
+then compressed files with a size unaligned to the filesystem block size will
+end with an extent with
+.I len
+<
+.IR unencoded_len .
+.IP *
+Extents cloned from the middle of a larger encoded extent with
+.B FICLONERANGE
+may have a non-zero
+.I unencoded_offset
+and/or
+.I len
+<
+.IR unencoded_len .
+.IP *
+If the middle of an encoded extent is overwritten,
+the filesystem may create extents with a non-zero
+.I unencoded_offset
+and/or
+.I len
+<
+.I unencoded_len
+for the parts that were not overwritten.
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
I haven't found any other "\<vs\>\.".
However, I've found a few "\<vs\>[^\.]".
quoted
+.I unencoded_len
+above).
+It may not be intended for this data to be readable.
+.PP
+Therefore, encoded I/O requires privilege.
+Namely, the
+.B RWF_ENCODED
+flag may only be used when the file was opened with the
+.B O_ALLOW_ENCODED
+flag to
+.BR open (2),
+which requires the
+.B CAP_SYS_ADMIN
+capability.
+The
+.B O_CLOEXEC
+flag must be specified in conjunction with
+.BR O_ALLOW_ENCODED .
+This avoids accidentally leaking the encoded I/O privilege
+(it is not cleared on
+.BR fork (2)
+or
+.BR execve (2)
+otherwise).
+If
+.B O_ALLOW_ENCODED
+without
+.B O_CLOEXEC
+is desired,
+.B O_CLOEXEC
+can be cleared afterwards with
+.BR fnctl (2).
+.BR fcntl (2)
+can also clear or set
+.B O_ALLOW_ENCODED
+(including without
+.BR O_CLOEXEC ).
+.SS Filesystem support
+Encoded I/O is supported on the following filesystems:
+.TP
+Btrfs (since Linux 5.12)
+.IP
+Btrfs supports encoded reads and writes of compressed data.
+The data is encoded as follows:
+.RS
+.IP * 3
+If
+.I compression
+is
+.BR ENCODED_IOV_COMPRESSION_BTRFS_ZLIB ,
+then the encoded data is a single zlib stream.
+.IP *
+If
+.I compression
+is
+.BR ENCODED_IOV_COMPRESSION_BTRFS_ZSTD ,
+then the encoded data is a single zstd frame compressed with the
+.I windowLog
+compression parameter set to no more than 17.
+.IP *
+If
+.I compression
+is one of
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_4K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_8K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_16K ,
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_32K ,
+or
+.BR ENCODED_IOV_COMPRESSION_BTRFS_LZO_64K ,
+then the encoded data is compressed page by page
+(using the page size indicated by the name of the constant)
+with LZO1X
+and wrapped in the format documented in the Linux kernel source file
+.IR fs/btrfs/lzo.c .
+.RE
+.IP
+Additionally, there are some restrictions on
+.BR pwritev2 ():
(2)
quoted
+.RS
+.IP * 3
+.I offset
+(or the current file offset if
+.I offset
+is -1) must be aligned to the sector size of the filesystem.
+.IP *
+.I len
+must be aligned to the sector size of the filesystem
+unless the data ends at or beyond the current end of the file.
+.IP *
+.I unencoded_len
+and the length of the encoded data must each be no more than 128 KiB.
+This limit may increase in the future.
+.IP *
+The length of the encoded data must be less than or equal to
+.IR unencoded_len .
+.IP *
+If using LZO, the filesystem's page size must match the compression page size.
+.RE
Please, add a SEE ALSO section, which should at least point to
preadv2(2) (or pwritev2(2), if you prefer):
[
.SH SEE ALSO
.BR preadv2 (2)
]
From: Michael Kerrisk (man-pages) <hidden> Date: 2020-12-01 20:13:58
Hello Alex,
On 11/20/20 4:03 PM, Alejandro Colomar (man-pages) wrote:
Hi Omar,
I found a wording of mine to be a bit confusing.
Please see below.
Thanks,
Alex
On 11/20/20 3:06 PM, Alejandro Colomar (man-pages) wrote:
quoted
Hi Omar and Michael,
please, see below.
Thanks,
Alex
On 11/20/20 12:29 AM, Alejandro Colomar (mailing lists; readonly) wrote:
quoted
Hi Omar,
Please, see some fixes below:
Michael, I've also some questions for you below
(you can grep for mtk to find those).
Thanks,
Alex
On 11/18/20 8:18 PM, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
This adds a new page, encoded_io(7), providing an overview of encoded
I/O and updates fcntl(2), open(2), and preadv2(2)/pwritev2(2) to
reference it.
Cc: Michael Kerrisk <redacted>
Cc: linux-man <redacted>
Signed-off-by: Omar Sandoval <redacted>
---
This feature is not yet upstream.
man2/fcntl.2 | 10 +-
man2/open.2 | 23 +++
man2/readv.2 | 70 +++++++++
man7/encoded_io.7 | 369 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 471 insertions(+), 1 deletion(-)
create mode 100644 man7/encoded_io.7
@@ -221,8 +221,9 @@ On Linux, this command can change only the
[...]
quoted
quoted
quoted
+.PP
+This may be extended in the future, so
+.I iov[0].iov_len
+must be set to
+.I "sizeof(struct\ encoded_iov)"
+for forward/backward compatibility.
+The remaining buffers contain the encoded data.
+.PP
+.I compression
+and
+.I encryption
+are the encoding fields.
+.I compression
+is
+.B ENCODED_IOV_COMPRESSION_NONE
+(zero)
+or a filesystem-specific
+.B ENCODED_IOV_COMPRESSION
Or s/ENCODED_IOV_COMPRESSION/ENCODED_IOV_COMPRESSION_/
I'm not sure about existing practice.
Michael (mtk), what would you do here?
I think I've tended towards the former
(ENCODED_IOV_COMPRESSION_*) in the past.
quoted
quoted
quoted
+constant;
+see
+.BR Filesystem\ support .
Please, write it as [.BR "Filesystem support" .]
and maybe I would change it, to be more specific, to the following:
[
see
.B Filesystem support
below.
]
So that the reader clearly understands it's on the same page.
quoted
+.I encryption
+is currently always
+.B ENCODED_IOV_ENCRYPTION_NONE
+(zero).
+.PP
+.I unencoded_len
+is the length of the unencoded (i.e., decrypted and decompressed) data.
+.I unencoded_offset
+is the offset into the unencoded data where the data in the file begins
The above wording is a bit unclear to me.
I suggest the following:
[
.I unencoded_offset
is the offset from the begining of the file
to the first byte of the unencoded data
]
Now I've read it again, and my wording was even worse than yours.
I think yours can be understood after a few reads.
However, I'll still try to reword mine to see if I add some value:
[
.I unencoded_offset
is the offset from the first byte of the unencoded data
to the first byte of logical data.
]
If you prefer yours, or a mix, that's fine.
quoted
quoted
quoted
+(less than or equal to
+.IR unencoded_len ).
+.I len
+is the length of the data in the file
+(less than or equal to
+.I unencoded_len
+-
Here's a question for Michael (mtk):
I've seen (many) cases where these math operations
are written without spaces,
and in the same line (e.g., [.IR a + b]).
I'd like to know your preferences on this,
or what is actually more extended in the manual pages,
to stick with only one of them.
I suspect there's a lot of inconsistency across pages. For simple
cases like this, I think writing it without spaces is fine, and
perhaps even preferable.
quoted
quoted
quoted
+.IR unencoded_offset ).
+See
+.B Extent layout
+below for some examples.
+.I
Were you maybe going to add something there?
If not, please remove that [.I].
quoted
+.PP
+If the unencoded data is actually longer than
+.IR unencoded_len ,
+then it is truncated;
+if it is shorter, then it is extended with zeroes.
+.PP
+
Please, remove that blank line.
quoted
+.BR pwritev2 ()
Should be [.BR pwritev2 (2)]
Michael (mtk),
Am I right in that? Please, confirm.
Yes. References to functions documented in other pages should
include the section number in parentheses.
[...]
quoted
quoted
quoted
+.PP
+However, suppose we read 50 bytes into a file
+which contains a single compressed extent.
+The filesystem must still return the entire compressed extent
+for us to be able to decompress it,
+so
+.I unencoded_len
+would be the length of the entire decompressed extent.
+However, because the read was at offset 50,
+the first 50 bytes should be ignored.
+Therefore,
+.I unencoded_offset
+would be 50,
+and
+.I len
+would accordingly be
+.IR unencoded_len\ -\ 50 .
This formats everything as I, except for the last dot.
Replace by:
[
.I unencoded
- 50.
]
Michael (mtk), same as above:
to space, or not to space? That is the question :p
In this case, perhaps
.IR unencoded \-1
[...]
quoted
quoted
quoted
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
Yes, that should probably be avoided. I'm not sure what the
correct way is to prevent that in groff though. I mean, one
could write
.RI "vs.\ " unencoded_len
but I think that simply creates a nonbreaking space,
which is not exactly what is desired.
[....]
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
From: Michael Kerrisk (man-pages) <hidden> Date: 2020-12-01 20:21:12
quoted
quoted
quoted
quoted
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
Yes, that should probably be avoided. I'm not sure what the
correct way is to prevent that in groff though. I mean, one
could write
.RI "vs.\ " unencoded_len
but I think that simply creates a nonbreaking space,
which is not exactly what is desired.
On Tue, Dec 01, 2020 at 10:15:58AM +0200, Amir Goldstein wrote:
On Mon, Nov 30, 2020 at 9:26 PM Omar Sandoval [off-list ref] wrote:
quoted
On Sat, Nov 21, 2020 at 12:41:23AM +0100, Jann Horn wrote:
quoted
On Thu, Nov 19, 2020 at 8:03 AM Amir Goldstein [off-list ref] wrote:
quoted
On Wed, Nov 18, 2020 at 9:18 PM Omar Sandoval [off-list ref] wrote:
quoted
The upcoming RWF_ENCODED operation introduces some security concerns:
1. Compressed writes will pass arbitrary data to decompression
algorithms in the kernel.
2. Compressed reads can leak truncated/hole punched data.
Therefore, we need to require privilege for RWF_ENCODED. It's not
possible to do the permissions checks at the time of the read or write
because, e.g., io_uring submits IO from a worker thread. So, add an open
flag which requires CAP_SYS_ADMIN. It can also be set and cleared with
fcntl(). The flag is not cleared in any way on fork or exec. It must be
combined with O_CLOEXEC when opening to avoid accidental leaks (if
needed, it may be set without O_CLOEXEC by using fnctl()).
Note that the usual issue that unknown open flags are ignored doesn't
really matter for O_ALLOW_ENCODED; if the kernel doesn't support
O_ALLOW_ENCODED, then it doesn't support RWF_ENCODED, either.
dup() can also result in accidental leak.
We could fail dup() of fd without O_CLOEXEC. Should we?
If we should than what error code should it be? We could return EPERM,
but since we do allow to clear O_CLOEXEC or set O_ALLOW_ENCODED
after open, EPERM seems a tad harsh.
EINVAL seems inappropriate because the error has nothing to do with
input args of dup() and EBADF would also be confusing.
This seems very arbitrary to me. Sure, leaking these file descriptors
wouldn't be great, but there are plenty of other types of file
descriptors that are probably more sensitive. (Writable file
descriptors to databases, to important configuration files, to
io_uring instances, and so on.) So I don't see why this specific
feature should impose such special rules on it.
I agree with Jann. I'm okay with the O_CLOEXEC-on-open requirement if it
makes people more comfortable, but I don't think we should be bending
over backwards to block it anywhere else.
I'm fine with or without the O_CLOEXEC-on-open requirement.
Just pointing out the weirdness.
I agree, it's weird to enforce it in one place but not in others, so I
think I might as well drop the O_CLOEXEC requirement altogether.
Hi Michael,
On 12/1/20 9:20 PM, Michael Kerrisk (man-pages) wrote:
quoted
quoted
quoted
quoted
quoted
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
Yes, that should probably be avoided. I'm not sure what the
correct way is to prevent that in groff though. I mean, one
could write
.RI "vs.\ " unencoded_len
but I think that simply creates a nonbreaking space,
which is not exactly what is desired.
From: Michael Kerrisk (man-pages) <hidden> Date: 2020-12-01 21:57:01
Hi Alex,
On 12/1/20 10:35 PM, Alejandro Colomar (man-pages) wrote:
Hi Michael,
On 12/1/20 9:20 PM, Michael Kerrisk (man-pages) wrote:
quoted
quoted
quoted
quoted
quoted
quoted
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
Yes, that should probably be avoided. I'm not sure what the
correct way is to prevent that in groff though. I mean, one
could write
.RI "vs.\ " unencoded_len
but I think that simply creates a nonbreaking space,
which is not exactly what is desired.
From: Josef Bacik <josef@toxicpanda.com> Date: 2020-12-02 22:03:59
On 11/18/20 2:18 PM, Omar Sandoval wrote:
quoted hunk
From: Omar Sandoval <redacted>
The implementation resembles 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. From there, we can reuse the compression code
with a minor modification to distinguish the write from writeback. This
also creates inline extents when possible.
Now that read and write are implemented, this also sets the
FMODE_ENCODED_IO flag in btrfs_file_open().
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/compression.c | 7 +-
fs/btrfs/compression.h | 6 +-
fs/btrfs/ctree.h | 2 +
fs/btrfs/file.c | 37 +++++-
fs/btrfs/inode.c | 259 +++++++++++++++++++++++++++++++++++++++-
fs/btrfs/ordered-data.c | 12 +-
fs/btrfs/ordered-data.h | 2 +
7 files changed, 313 insertions(+), 12 deletions(-)
@@ -336,7 +336,8 @@ static void end_compressed_bio_write(struct bio *bio)bio->bi_status==BLK_STS_OK);cb->compressed_pages[0]->mapping=NULL;-end_compressed_writeback(inode,cb);+if(cb->writeback)+end_compressed_writeback(inode,cb);/* note, our inode could be gone now *//*
@@ -49,6 +49,9 @@ struct compressed_bio {/* the compression algorithm for this bio */intcompress_type;+/* Whether this is a write for writeback. */+boolwriteback;+/* number of compressed pages in the array */unsignedlongnr_pages;
From: Josef Bacik <josef@toxicpanda.com> Date: 2020-12-03 14:46:32
On 11/18/20 2:18 PM, Omar Sandoval wrote:
From: Omar Sandoval <redacted>
The implementation resembles 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. From there, we can reuse the compression code
with a minor modification to distinguish the write from writeback. This
also creates inline extents when possible.
Now that read and write are implemented, this also sets the
FMODE_ENCODED_IO flag in btrfs_file_open().
Signed-off-by: Omar Sandoval <redacted>
Fix up the spacing thing and then you can add
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks,
Josef
From: Josef Bacik <josef@toxicpanda.com> Date: 2020-12-03 14:51:34
On 11/18/20 2:18 PM, Omar Sandoval wrote:
quoted hunk
From: Omar Sandoval <redacted>
There are 4 main cases:
1. Inline extents: we copy the data straight out of the extent buffer.
2. Hole/preallocated extents: we fill in zeroes.
3. Regular, uncompressed extents: we read the sectors we need directly
from disk.
4. Regular, compressed extents: we read the entire compressed extent
from disk and indicate what subset of the decompressed extent is in
the file.
This initial implementation simplifies a few things that can be improved
in the future:
- We hold the inode lock during the operation.
- Cases 1, 3, and 4 allocate temporary memory to read into before
copying out to userspace.
- We don't do read repair, because it turns out that read repair is
currently broken for compressed data.
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/ctree.h | 2 +
fs/btrfs/file.c | 5 +
fs/btrfs/inode.c | 496 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 503 insertions(+)
Hi Omar,
Linux 5.10 has been recently released.
Do you have any updates for this patch?
Thanks,
Alex
On 12/1/20 10:56 PM, Michael Kerrisk (man-pages) wrote:
Hi Alex,
On 12/1/20 10:35 PM, Alejandro Colomar (man-pages) wrote:
quoted
Hi Michael,
On 12/1/20 9:20 PM, Michael Kerrisk (man-pages) wrote:
quoted
quoted
quoted
quoted
quoted
quoted
+.SS Security
+Encoded I/O creates the potential for some security issues:
+.IP * 3
+Encoded writes allow writing arbitrary data which the kernel will decode on
+a subsequent read. Decompression algorithms are complex and may have bugs
+which can be exploited by maliciously crafted data.
+.IP *
+Encoded reads may return data which is not logically present in the file
+(see the discussion of
+.I len
+vs.
Please, s/vs./vs/
See the reasons below:
Michael (mtk),
Here the renderer outputs a double space
(as for separating two sentences).
Are you okay with that?
Yes, that should probably be avoided. I'm not sure what the
correct way is to prevent that in groff though. I mean, one
could write
.RI "vs.\ " unencoded_len
but I think that simply creates a nonbreaking space,
which is not exactly what is desired.
On Thu, Dec 03, 2020 at 09:32:37AM -0500, Josef Bacik wrote:
On 11/18/20 2:18 PM, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
There are 4 main cases:
1. Inline extents: we copy the data straight out of the extent buffer.
2. Hole/preallocated extents: we fill in zeroes.
3. Regular, uncompressed extents: we read the sectors we need directly
from disk.
4. Regular, compressed extents: we read the entire compressed extent
from disk and indicate what subset of the decompressed extent is in
the file.
This initial implementation simplifies a few things that can be improved
in the future:
- We hold the inode lock during the operation.
- Cases 1, 3, and 4 allocate temporary memory to read into before
copying out to userspace.
- We don't do read repair, because it turns out that read repair is
currently broken for compressed data.
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/ctree.h | 2 +
fs/btrfs/file.c | 5 +
fs/btrfs/inode.c | 496 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 503 insertions(+)
@@ -9936,6 +9936,502 @@ void btrfs_set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end)}}+staticintencoded_iov_compression_from_btrfs(unsignedintcompress_type)+{+switch(compress_type){+caseBTRFS_COMPRESS_NONE:+returnENCODED_IOV_COMPRESSION_NONE;+caseBTRFS_COMPRESS_ZLIB:+returnENCODED_IOV_COMPRESSION_BTRFS_ZLIB;+caseBTRFS_COMPRESS_LZO:+/*+*TheLZOformatdependsonthepagesize.64kisthemaximum+*sectorsize(andthuspagesize)thatwesupport.+*/+if(PAGE_SIZE<SZ_4K||PAGE_SIZE>SZ_64K)+return-EINVAL;+returnENCODED_IOV_COMPRESSION_BTRFS_LZO_4K+(PAGE_SHIFT-12);+caseBTRFS_COMPRESS_ZSTD:+returnENCODED_IOV_COMPRESSION_BTRFS_ZSTD;+default:+return-EUCLEAN;+}+}++staticssize_tbtrfs_encoded_read_inline(structkiocb*iocb,+structiov_iter*iter,u64start,+u64lockend,+structextent_state**cached_state,+u64extent_start,size_tcount,+structencoded_iov*encoded,+bool*unlocked)+{+structinode*inode=file_inode(iocb->ki_filp);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+structbtrfs_path*path;+structextent_buffer*leaf;+structbtrfs_file_extent_item*item;+u64ram_bytes;+unsignedlongptr;+void*tmp;+ssize_tret;++path=btrfs_alloc_path();+if(!path){+ret=-ENOMEM;+gotoout;+}+ret=btrfs_lookup_file_extent(NULL,BTRFS_I(inode)->root,path,+btrfs_ino(BTRFS_I(inode)),extent_start,+0);+if(ret){+if(ret>0){+/* The extent item disappeared? */+ret=-EIO;+}+gotoout;+}+leaf=path->nodes[0];+item=btrfs_item_ptr(leaf,path->slots[0],+structbtrfs_file_extent_item);++ram_bytes=btrfs_file_extent_ram_bytes(leaf,item);+ptr=btrfs_file_extent_inline_start(item);++encoded->len=(min_t(u64,extent_start+ram_bytes,inode->i_size)-+iocb->ki_pos);+ret=encoded_iov_compression_from_btrfs(+btrfs_file_extent_compression(leaf,item));+if(ret<0)+gotoout;+encoded->compression=ret;+if(encoded->compression){+size_tinline_size;++inline_size=btrfs_file_extent_inline_item_len(leaf,+btrfs_item_nr(path->slots[0]));+if(inline_size>count){+ret=-ENOBUFS;+gotoout;+}+count=inline_size;+encoded->unencoded_len=ram_bytes;+encoded->unencoded_offset=iocb->ki_pos-extent_start;+}else{+encoded->len=encoded->unencoded_len=count=+min_t(u64,count,encoded->len);+ptr+=iocb->ki_pos-extent_start;+}++tmp=kmalloc(count,GFP_NOFS);+if(!tmp){+ret=-ENOMEM;+gotoout;+}+read_extent_buffer(leaf,tmp,ptr,count);+btrfs_release_path(path);+unlock_extent_cached(io_tree,start,lockend,cached_state);+inode_unlock_shared(inode);+*unlocked=true;++ret=copy_encoded_iov_to_iter(encoded,iter);+if(ret)+gotoout_free;+ret=copy_to_iter(tmp,count,iter);+if(ret!=count)+ret=-EFAULT;+out_free:+kfree(tmp);+out:+btrfs_free_path(path);+returnret;+}++structbtrfs_encoded_read_private{+structinode*inode;+wait_queue_head_twait;+atomic_tpending;+blk_status_tstatus;+boolskip_csum;+};++staticblk_status_tsubmit_encoded_read_bio(structinode*inode,+structbio*bio,intmirror_num,+unsignedlongbio_flags)+{+structbtrfs_encoded_read_private*priv=bio->bi_private;+structbtrfs_io_bio*io_bio=btrfs_io_bio(bio);+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+blk_status_tret;++if(!priv->skip_csum){+ret=btrfs_lookup_bio_sums(inode,bio,io_bio->logical,NULL);+if(ret)+returnret;+}++ret=btrfs_bio_wq_end_io(fs_info,bio,BTRFS_WQ_ENDIO_DATA);+if(ret){+btrfs_io_bio_free_csum(io_bio);+returnret;+}++atomic_inc(&priv->pending);+ret=btrfs_map_bio(fs_info,bio,mirror_num);+if(ret){+atomic_dec(&priv->pending);+btrfs_io_bio_free_csum(io_bio);+}+returnret;+}++staticblk_status_tbtrfs_encoded_read_check_bio(structbtrfs_io_bio*io_bio)+{+constbooluptodate=io_bio->bio.bi_status==BLK_STS_OK;+structbtrfs_encoded_read_private*priv=io_bio->bio.bi_private;+structinode*inode=priv->inode;+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+u32sectorsize=fs_info->sectorsize;+structbio_vec*bvec;+structbvec_iter_alliter_all;+u64start=io_bio->logical;+inticsum=0;++if(priv->skip_csum||!uptodate)+returnio_bio->bio.bi_status;++bio_for_each_segment_all(bvec,&io_bio->bio,iter_all){+unsignedinti,nr_sectors,pgoff;++nr_sectors=BTRFS_BYTES_TO_BLKS(fs_info,bvec->bv_len);+pgoff=bvec->bv_offset;+for(i=0;i<nr_sectors;i++){+ASSERT(pgoff<PAGE_SIZE);+if(check_data_csum(inode,io_bio,icsum,bvec->bv_page,+pgoff,start))+returnBLK_STS_IOERR;+start+=sectorsize;+icsum++;+pgoff+=sectorsize;+}+}+returnBLK_STS_OK;+}++staticvoidbtrfs_encoded_read_endio(structbio*bio)+{+structbtrfs_encoded_read_private*priv=bio->bi_private;+structbtrfs_io_bio*io_bio=btrfs_io_bio(bio);+blk_status_tstatus;++status=btrfs_encoded_read_check_bio(io_bio);+if(status){+/*+*Thememorybarrierimpliedbytheatomic_dec_return()here+*pairswiththememorybarrierimpliedbythe+*atomic_dec_return()orio_wait_event()in+*btrfs_encoded_read_regular_fill_pages()toensurethatthis+*writeisobservedbeforetheloadofstatusin+*btrfs_encoded_read_regular_fill_pages().+*/+WRITE_ONCE(priv->status,status);+}+if(!atomic_dec_return(&priv->pending))+wake_up(&priv->wait);+btrfs_io_bio_free_csum(io_bio);+bio_put(bio);+}++staticintbtrfs_encoded_read_regular_fill_pages(structinode*inode,u64offset,+u64disk_io_size,structpage**pages)+{+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+structbtrfs_encoded_read_privatepriv={+.inode=inode,+.pending=ATOMIC_INIT(1),+.skip_csum=BTRFS_I(inode)->flags&BTRFS_INODE_NODATASUM,+};+unsignedlongi=0;+u64cur=0;+intret;++init_waitqueue_head(&priv.wait);+/*+*Submitbiosfortheextent,splittingduetobioorstripelimitsas+*necessary.+*/+while(cur<disk_io_size){+structbtrfs_io_geometrygeom;+structbio*bio=NULL;+u64remaining;++ret=btrfs_get_io_geometry(fs_info,BTRFS_MAP_READ,+offset+cur,disk_io_size-cur,+&geom);+if(ret){+WRITE_ONCE(priv.status,errno_to_blk_status(ret));+break;+}+remaining=min(geom.len,disk_io_size-cur);+while(bio||remaining){+size_tbytes=min_t(u64,remaining,PAGE_SIZE);++if(!bio){+bio=btrfs_bio_alloc(offset+cur);+bio->bi_end_io=btrfs_encoded_read_endio;+bio->bi_private=&priv;+bio->bi_opf=REQ_OP_READ;+}++if(!bytes||+bio_add_page(bio,pages[i],bytes,0)<bytes){+blk_status_tstatus;++status=submit_encoded_read_bio(inode,bio,0,+0);+if(status){+WRITE_ONCE(priv.status,status);+bio_put(bio);+gotoout;+}+bio=NULL;+continue;+}++i++;+cur+=bytes;+remaining-=bytes;+}+}++out:+if(atomic_dec_return(&priv.pending))+io_wait_event(priv.wait,!atomic_read(&priv.pending));+/* See btrfs_encoded_read_endio() for ordering. */+returnblk_status_to_errno(READ_ONCE(priv.status));+}++staticssize_tbtrfs_encoded_read_regular(structkiocb*iocb,+structiov_iter*iter,+u64start,u64lockend,+structextent_state**cached_state,+u64offset,u64disk_io_size,+size_tcount,+conststructencoded_iov*encoded,+bool*unlocked)+{+structinode*inode=file_inode(iocb->ki_filp);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+structpage**pages;+unsignedlongnr_pages,i;+u64cur;+size_tpage_offset;+ssize_tret;++nr_pages=DIV_ROUND_UP(disk_io_size,PAGE_SIZE);+pages=kcalloc(nr_pages,sizeof(structpage*),GFP_NOFS);+if(!pages)+return-ENOMEM;+for(i=0;i<nr_pages;i++){+pages[i]=alloc_page(GFP_NOFS|__GFP_HIGHMEM);+if(!pages[i]){+ret=-ENOMEM;+gotoout;+}+}++ret=btrfs_encoded_read_regular_fill_pages(inode,offset,disk_io_size,+pages);+if(ret)+gotoout;++unlock_extent_cached(io_tree,start,lockend,cached_state);+inode_unlock_shared(inode);+*unlocked=true;++ret=copy_encoded_iov_to_iter(encoded,iter);+if(ret)+gotoout;+if(encoded->compression){+i=0;+page_offset=0;+}else{+i=(iocb->ki_pos-start)>>PAGE_SHIFT;+page_offset=(iocb->ki_pos-start)&(PAGE_SIZE-1);+}+cur=0;+while(cur<count){+size_tbytes=min_t(size_t,count-cur,+PAGE_SIZE-page_offset);++if(copy_page_to_iter(pages[i],page_offset,bytes,+iter)!=bytes){+ret=-EFAULT;+gotoout;+}+i++;+cur+=bytes;+page_offset=0;+}+ret=count;+out:+for(i=0;i<nr_pages;i++){+if(pages[i])+__free_page(pages[i]);+}+kfree(pages);+returnret;+}++ssize_tbtrfs_encoded_read(structkiocb*iocb,structiov_iter*iter)+{+structinode*inode=file_inode(iocb->ki_filp);+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+ssize_tret;+size_tcount;+u64start,lockend,offset,disk_io_size;+structextent_state*cached_state=NULL;+structextent_map*em;+structencoded_iovencoded={};+boolunlocked=false;++ret=generic_encoded_read_checks(iocb,iter);+if(ret<0)+returnret;+if(ret==0)+returncopy_encoded_iov_to_iter(&encoded,iter);+count=ret;++file_accessed(iocb->ki_filp);++inode_lock_shared(inode);++if(iocb->ki_pos>=inode->i_size){+inode_unlock_shared(inode);+returncopy_encoded_iov_to_iter(&encoded,iter);+}+start=ALIGN_DOWN(iocb->ki_pos,fs_info->sectorsize);+/*+*Wedon'tknowhowlongtheextentcontainingiocb->ki_posis,butif+*it'scompressedweknowthatitwon'tbelongerthanthis.+*/+lockend=start+BTRFS_MAX_UNCOMPRESSED-1;++for(;;){+structbtrfs_ordered_extent*ordered;++ret=btrfs_wait_ordered_range(inode,start,+lockend-start+1);+if(ret)+gotoout_unlock_inode;+lock_extent_bits(io_tree,start,lockend,&cached_state);+ordered=btrfs_lookup_ordered_range(BTRFS_I(inode),start,+lockend-start+1);+if(!ordered)+break;+btrfs_put_ordered_extent(ordered);+unlock_extent_cached(io_tree,start,lockend,&cached_state);+cond_resched();+}
This can be replaced with btrfs_lock_and_flush_ordered_range(). Then you can add
Sorry, finally getting back to this after the break. Please correct me
if I'm wrong, but I don't think btrfs_lock_and_flush_ordered_range() is
strong enough here.
An encoded read needs to make sure that any buffered writes are on disk
(since it's basically direct I/O). btrfs_lock_and_flush_ordered_range()
bails immediately if there aren't any ordered extents. As far as I can
tell, ordered extents aren't created until writepage, so if I do some
buffered writes and call btrfs_lock_and_flush_ordered_range() before
writepage creates the ordered extents, it won't flush the buffered
writes like I need it to. This loop with btrfs_wait_ordered_range()
does.
From: Josef Bacik <josef@toxicpanda.com> Date: 2021-01-11 20:36:09
On 1/11/21 3:21 PM, Omar Sandoval wrote:
On Thu, Dec 03, 2020 at 09:32:37AM -0500, Josef Bacik wrote:
quoted
On 11/18/20 2:18 PM, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
There are 4 main cases:
1. Inline extents: we copy the data straight out of the extent buffer.
2. Hole/preallocated extents: we fill in zeroes.
3. Regular, uncompressed extents: we read the sectors we need directly
from disk.
4. Regular, compressed extents: we read the entire compressed extent
from disk and indicate what subset of the decompressed extent is in
the file.
This initial implementation simplifies a few things that can be improved
in the future:
- We hold the inode lock during the operation.
- Cases 1, 3, and 4 allocate temporary memory to read into before
copying out to userspace.
- We don't do read repair, because it turns out that read repair is
currently broken for compressed data.
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/ctree.h | 2 +
fs/btrfs/file.c | 5 +
fs/btrfs/inode.c | 496 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 503 insertions(+)
@@ -9936,6 +9936,502 @@ void btrfs_set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end)}}+staticintencoded_iov_compression_from_btrfs(unsignedintcompress_type)+{+switch(compress_type){+caseBTRFS_COMPRESS_NONE:+returnENCODED_IOV_COMPRESSION_NONE;+caseBTRFS_COMPRESS_ZLIB:+returnENCODED_IOV_COMPRESSION_BTRFS_ZLIB;+caseBTRFS_COMPRESS_LZO:+/*+*TheLZOformatdependsonthepagesize.64kisthemaximum+*sectorsize(andthuspagesize)thatwesupport.+*/+if(PAGE_SIZE<SZ_4K||PAGE_SIZE>SZ_64K)+return-EINVAL;+returnENCODED_IOV_COMPRESSION_BTRFS_LZO_4K+(PAGE_SHIFT-12);+caseBTRFS_COMPRESS_ZSTD:+returnENCODED_IOV_COMPRESSION_BTRFS_ZSTD;+default:+return-EUCLEAN;+}+}++staticssize_tbtrfs_encoded_read_inline(structkiocb*iocb,+structiov_iter*iter,u64start,+u64lockend,+structextent_state**cached_state,+u64extent_start,size_tcount,+structencoded_iov*encoded,+bool*unlocked)+{+structinode*inode=file_inode(iocb->ki_filp);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+structbtrfs_path*path;+structextent_buffer*leaf;+structbtrfs_file_extent_item*item;+u64ram_bytes;+unsignedlongptr;+void*tmp;+ssize_tret;++path=btrfs_alloc_path();+if(!path){+ret=-ENOMEM;+gotoout;+}+ret=btrfs_lookup_file_extent(NULL,BTRFS_I(inode)->root,path,+btrfs_ino(BTRFS_I(inode)),extent_start,+0);+if(ret){+if(ret>0){+/* The extent item disappeared? */+ret=-EIO;+}+gotoout;+}+leaf=path->nodes[0];+item=btrfs_item_ptr(leaf,path->slots[0],+structbtrfs_file_extent_item);++ram_bytes=btrfs_file_extent_ram_bytes(leaf,item);+ptr=btrfs_file_extent_inline_start(item);++encoded->len=(min_t(u64,extent_start+ram_bytes,inode->i_size)-+iocb->ki_pos);+ret=encoded_iov_compression_from_btrfs(+btrfs_file_extent_compression(leaf,item));+if(ret<0)+gotoout;+encoded->compression=ret;+if(encoded->compression){+size_tinline_size;++inline_size=btrfs_file_extent_inline_item_len(leaf,+btrfs_item_nr(path->slots[0]));+if(inline_size>count){+ret=-ENOBUFS;+gotoout;+}+count=inline_size;+encoded->unencoded_len=ram_bytes;+encoded->unencoded_offset=iocb->ki_pos-extent_start;+}else{+encoded->len=encoded->unencoded_len=count=+min_t(u64,count,encoded->len);+ptr+=iocb->ki_pos-extent_start;+}++tmp=kmalloc(count,GFP_NOFS);+if(!tmp){+ret=-ENOMEM;+gotoout;+}+read_extent_buffer(leaf,tmp,ptr,count);+btrfs_release_path(path);+unlock_extent_cached(io_tree,start,lockend,cached_state);+inode_unlock_shared(inode);+*unlocked=true;++ret=copy_encoded_iov_to_iter(encoded,iter);+if(ret)+gotoout_free;+ret=copy_to_iter(tmp,count,iter);+if(ret!=count)+ret=-EFAULT;+out_free:+kfree(tmp);+out:+btrfs_free_path(path);+returnret;+}++structbtrfs_encoded_read_private{+structinode*inode;+wait_queue_head_twait;+atomic_tpending;+blk_status_tstatus;+boolskip_csum;+};++staticblk_status_tsubmit_encoded_read_bio(structinode*inode,+structbio*bio,intmirror_num,+unsignedlongbio_flags)+{+structbtrfs_encoded_read_private*priv=bio->bi_private;+structbtrfs_io_bio*io_bio=btrfs_io_bio(bio);+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+blk_status_tret;++if(!priv->skip_csum){+ret=btrfs_lookup_bio_sums(inode,bio,io_bio->logical,NULL);+if(ret)+returnret;+}++ret=btrfs_bio_wq_end_io(fs_info,bio,BTRFS_WQ_ENDIO_DATA);+if(ret){+btrfs_io_bio_free_csum(io_bio);+returnret;+}++atomic_inc(&priv->pending);+ret=btrfs_map_bio(fs_info,bio,mirror_num);+if(ret){+atomic_dec(&priv->pending);+btrfs_io_bio_free_csum(io_bio);+}+returnret;+}++staticblk_status_tbtrfs_encoded_read_check_bio(structbtrfs_io_bio*io_bio)+{+constbooluptodate=io_bio->bio.bi_status==BLK_STS_OK;+structbtrfs_encoded_read_private*priv=io_bio->bio.bi_private;+structinode*inode=priv->inode;+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+u32sectorsize=fs_info->sectorsize;+structbio_vec*bvec;+structbvec_iter_alliter_all;+u64start=io_bio->logical;+inticsum=0;++if(priv->skip_csum||!uptodate)+returnio_bio->bio.bi_status;++bio_for_each_segment_all(bvec,&io_bio->bio,iter_all){+unsignedinti,nr_sectors,pgoff;++nr_sectors=BTRFS_BYTES_TO_BLKS(fs_info,bvec->bv_len);+pgoff=bvec->bv_offset;+for(i=0;i<nr_sectors;i++){+ASSERT(pgoff<PAGE_SIZE);+if(check_data_csum(inode,io_bio,icsum,bvec->bv_page,+pgoff,start))+returnBLK_STS_IOERR;+start+=sectorsize;+icsum++;+pgoff+=sectorsize;+}+}+returnBLK_STS_OK;+}++staticvoidbtrfs_encoded_read_endio(structbio*bio)+{+structbtrfs_encoded_read_private*priv=bio->bi_private;+structbtrfs_io_bio*io_bio=btrfs_io_bio(bio);+blk_status_tstatus;++status=btrfs_encoded_read_check_bio(io_bio);+if(status){+/*+*Thememorybarrierimpliedbytheatomic_dec_return()here+*pairswiththememorybarrierimpliedbythe+*atomic_dec_return()orio_wait_event()in+*btrfs_encoded_read_regular_fill_pages()toensurethatthis+*writeisobservedbeforetheloadofstatusin+*btrfs_encoded_read_regular_fill_pages().+*/+WRITE_ONCE(priv->status,status);+}+if(!atomic_dec_return(&priv->pending))+wake_up(&priv->wait);+btrfs_io_bio_free_csum(io_bio);+bio_put(bio);+}++staticintbtrfs_encoded_read_regular_fill_pages(structinode*inode,u64offset,+u64disk_io_size,structpage**pages)+{+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+structbtrfs_encoded_read_privatepriv={+.inode=inode,+.pending=ATOMIC_INIT(1),+.skip_csum=BTRFS_I(inode)->flags&BTRFS_INODE_NODATASUM,+};+unsignedlongi=0;+u64cur=0;+intret;++init_waitqueue_head(&priv.wait);+/*+*Submitbiosfortheextent,splittingduetobioorstripelimitsas+*necessary.+*/+while(cur<disk_io_size){+structbtrfs_io_geometrygeom;+structbio*bio=NULL;+u64remaining;++ret=btrfs_get_io_geometry(fs_info,BTRFS_MAP_READ,+offset+cur,disk_io_size-cur,+&geom);+if(ret){+WRITE_ONCE(priv.status,errno_to_blk_status(ret));+break;+}+remaining=min(geom.len,disk_io_size-cur);+while(bio||remaining){+size_tbytes=min_t(u64,remaining,PAGE_SIZE);++if(!bio){+bio=btrfs_bio_alloc(offset+cur);+bio->bi_end_io=btrfs_encoded_read_endio;+bio->bi_private=&priv;+bio->bi_opf=REQ_OP_READ;+}++if(!bytes||+bio_add_page(bio,pages[i],bytes,0)<bytes){+blk_status_tstatus;++status=submit_encoded_read_bio(inode,bio,0,+0);+if(status){+WRITE_ONCE(priv.status,status);+bio_put(bio);+gotoout;+}+bio=NULL;+continue;+}++i++;+cur+=bytes;+remaining-=bytes;+}+}++out:+if(atomic_dec_return(&priv.pending))+io_wait_event(priv.wait,!atomic_read(&priv.pending));+/* See btrfs_encoded_read_endio() for ordering. */+returnblk_status_to_errno(READ_ONCE(priv.status));+}++staticssize_tbtrfs_encoded_read_regular(structkiocb*iocb,+structiov_iter*iter,+u64start,u64lockend,+structextent_state**cached_state,+u64offset,u64disk_io_size,+size_tcount,+conststructencoded_iov*encoded,+bool*unlocked)+{+structinode*inode=file_inode(iocb->ki_filp);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+structpage**pages;+unsignedlongnr_pages,i;+u64cur;+size_tpage_offset;+ssize_tret;++nr_pages=DIV_ROUND_UP(disk_io_size,PAGE_SIZE);+pages=kcalloc(nr_pages,sizeof(structpage*),GFP_NOFS);+if(!pages)+return-ENOMEM;+for(i=0;i<nr_pages;i++){+pages[i]=alloc_page(GFP_NOFS|__GFP_HIGHMEM);+if(!pages[i]){+ret=-ENOMEM;+gotoout;+}+}++ret=btrfs_encoded_read_regular_fill_pages(inode,offset,disk_io_size,+pages);+if(ret)+gotoout;++unlock_extent_cached(io_tree,start,lockend,cached_state);+inode_unlock_shared(inode);+*unlocked=true;++ret=copy_encoded_iov_to_iter(encoded,iter);+if(ret)+gotoout;+if(encoded->compression){+i=0;+page_offset=0;+}else{+i=(iocb->ki_pos-start)>>PAGE_SHIFT;+page_offset=(iocb->ki_pos-start)&(PAGE_SIZE-1);+}+cur=0;+while(cur<count){+size_tbytes=min_t(size_t,count-cur,+PAGE_SIZE-page_offset);++if(copy_page_to_iter(pages[i],page_offset,bytes,+iter)!=bytes){+ret=-EFAULT;+gotoout;+}+i++;+cur+=bytes;+page_offset=0;+}+ret=count;+out:+for(i=0;i<nr_pages;i++){+if(pages[i])+__free_page(pages[i]);+}+kfree(pages);+returnret;+}++ssize_tbtrfs_encoded_read(structkiocb*iocb,structiov_iter*iter)+{+structinode*inode=file_inode(iocb->ki_filp);+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+ssize_tret;+size_tcount;+u64start,lockend,offset,disk_io_size;+structextent_state*cached_state=NULL;+structextent_map*em;+structencoded_iovencoded={};+boolunlocked=false;++ret=generic_encoded_read_checks(iocb,iter);+if(ret<0)+returnret;+if(ret==0)+returncopy_encoded_iov_to_iter(&encoded,iter);+count=ret;++file_accessed(iocb->ki_filp);++inode_lock_shared(inode);++if(iocb->ki_pos>=inode->i_size){+inode_unlock_shared(inode);+returncopy_encoded_iov_to_iter(&encoded,iter);+}+start=ALIGN_DOWN(iocb->ki_pos,fs_info->sectorsize);+/*+*Wedon'tknowhowlongtheextentcontainingiocb->ki_posis,butif+*it'scompressedweknowthatitwon'tbelongerthanthis.+*/+lockend=start+BTRFS_MAX_UNCOMPRESSED-1;++for(;;){+structbtrfs_ordered_extent*ordered;++ret=btrfs_wait_ordered_range(inode,start,+lockend-start+1);+if(ret)+gotoout_unlock_inode;+lock_extent_bits(io_tree,start,lockend,&cached_state);+ordered=btrfs_lookup_ordered_range(BTRFS_I(inode),start,+lockend-start+1);+if(!ordered)+break;+btrfs_put_ordered_extent(ordered);+unlock_extent_cached(io_tree,start,lockend,&cached_state);+cond_resched();+}
This can be replaced with btrfs_lock_and_flush_ordered_range(). Then you can add
Sorry, finally getting back to this after the break. Please correct me
if I'm wrong, but I don't think btrfs_lock_and_flush_ordered_range() is
strong enough here.
An encoded read needs to make sure that any buffered writes are on disk
(since it's basically direct I/O). btrfs_lock_and_flush_ordered_range()
bails immediately if there aren't any ordered extents. As far as I can
tell, ordered extents aren't created until writepage, so if I do some
buffered writes and call btrfs_lock_and_flush_ordered_range() before
writepage creates the ordered extents, it won't flush the buffered
writes like I need it to. This loop with btrfs_wait_ordered_range()
does.
I didn't realize that btrfs_wait_ordered_range() does the fdatawrite_range,
awesome. You can leave it then and add my reviewed-by. Thanks,
Josef
On Mon, Jan 11, 2021 at 03:35:24PM -0500, Josef Bacik wrote:
On 1/11/21 3:21 PM, Omar Sandoval wrote:
quoted
On Thu, Dec 03, 2020 at 09:32:37AM -0500, Josef Bacik wrote:
quoted
On 11/18/20 2:18 PM, Omar Sandoval wrote:
quoted
From: Omar Sandoval <redacted>
There are 4 main cases:
1. Inline extents: we copy the data straight out of the extent buffer.
2. Hole/preallocated extents: we fill in zeroes.
3. Regular, uncompressed extents: we read the sectors we need directly
from disk.
4. Regular, compressed extents: we read the entire compressed extent
from disk and indicate what subset of the decompressed extent is in
the file.
This initial implementation simplifies a few things that can be improved
in the future:
- We hold the inode lock during the operation.
- Cases 1, 3, and 4 allocate temporary memory to read into before
copying out to userspace.
- We don't do read repair, because it turns out that read repair is
currently broken for compressed data.
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/ctree.h | 2 +
fs/btrfs/file.c | 5 +
fs/btrfs/inode.c | 496 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 503 insertions(+)
@@ -9936,6 +9936,502 @@ void btrfs_set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end)}}+staticintencoded_iov_compression_from_btrfs(unsignedintcompress_type)+{+switch(compress_type){+caseBTRFS_COMPRESS_NONE:+returnENCODED_IOV_COMPRESSION_NONE;+caseBTRFS_COMPRESS_ZLIB:+returnENCODED_IOV_COMPRESSION_BTRFS_ZLIB;+caseBTRFS_COMPRESS_LZO:+/*+*TheLZOformatdependsonthepagesize.64kisthemaximum+*sectorsize(andthuspagesize)thatwesupport.+*/+if(PAGE_SIZE<SZ_4K||PAGE_SIZE>SZ_64K)+return-EINVAL;+returnENCODED_IOV_COMPRESSION_BTRFS_LZO_4K+(PAGE_SHIFT-12);+caseBTRFS_COMPRESS_ZSTD:+returnENCODED_IOV_COMPRESSION_BTRFS_ZSTD;+default:+return-EUCLEAN;+}+}++staticssize_tbtrfs_encoded_read_inline(structkiocb*iocb,+structiov_iter*iter,u64start,+u64lockend,+structextent_state**cached_state,+u64extent_start,size_tcount,+structencoded_iov*encoded,+bool*unlocked)+{+structinode*inode=file_inode(iocb->ki_filp);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+structbtrfs_path*path;+structextent_buffer*leaf;+structbtrfs_file_extent_item*item;+u64ram_bytes;+unsignedlongptr;+void*tmp;+ssize_tret;++path=btrfs_alloc_path();+if(!path){+ret=-ENOMEM;+gotoout;+}+ret=btrfs_lookup_file_extent(NULL,BTRFS_I(inode)->root,path,+btrfs_ino(BTRFS_I(inode)),extent_start,+0);+if(ret){+if(ret>0){+/* The extent item disappeared? */+ret=-EIO;+}+gotoout;+}+leaf=path->nodes[0];+item=btrfs_item_ptr(leaf,path->slots[0],+structbtrfs_file_extent_item);++ram_bytes=btrfs_file_extent_ram_bytes(leaf,item);+ptr=btrfs_file_extent_inline_start(item);++encoded->len=(min_t(u64,extent_start+ram_bytes,inode->i_size)-+iocb->ki_pos);+ret=encoded_iov_compression_from_btrfs(+btrfs_file_extent_compression(leaf,item));+if(ret<0)+gotoout;+encoded->compression=ret;+if(encoded->compression){+size_tinline_size;++inline_size=btrfs_file_extent_inline_item_len(leaf,+btrfs_item_nr(path->slots[0]));+if(inline_size>count){+ret=-ENOBUFS;+gotoout;+}+count=inline_size;+encoded->unencoded_len=ram_bytes;+encoded->unencoded_offset=iocb->ki_pos-extent_start;+}else{+encoded->len=encoded->unencoded_len=count=+min_t(u64,count,encoded->len);+ptr+=iocb->ki_pos-extent_start;+}++tmp=kmalloc(count,GFP_NOFS);+if(!tmp){+ret=-ENOMEM;+gotoout;+}+read_extent_buffer(leaf,tmp,ptr,count);+btrfs_release_path(path);+unlock_extent_cached(io_tree,start,lockend,cached_state);+inode_unlock_shared(inode);+*unlocked=true;++ret=copy_encoded_iov_to_iter(encoded,iter);+if(ret)+gotoout_free;+ret=copy_to_iter(tmp,count,iter);+if(ret!=count)+ret=-EFAULT;+out_free:+kfree(tmp);+out:+btrfs_free_path(path);+returnret;+}++structbtrfs_encoded_read_private{+structinode*inode;+wait_queue_head_twait;+atomic_tpending;+blk_status_tstatus;+boolskip_csum;+};++staticblk_status_tsubmit_encoded_read_bio(structinode*inode,+structbio*bio,intmirror_num,+unsignedlongbio_flags)+{+structbtrfs_encoded_read_private*priv=bio->bi_private;+structbtrfs_io_bio*io_bio=btrfs_io_bio(bio);+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+blk_status_tret;++if(!priv->skip_csum){+ret=btrfs_lookup_bio_sums(inode,bio,io_bio->logical,NULL);+if(ret)+returnret;+}++ret=btrfs_bio_wq_end_io(fs_info,bio,BTRFS_WQ_ENDIO_DATA);+if(ret){+btrfs_io_bio_free_csum(io_bio);+returnret;+}++atomic_inc(&priv->pending);+ret=btrfs_map_bio(fs_info,bio,mirror_num);+if(ret){+atomic_dec(&priv->pending);+btrfs_io_bio_free_csum(io_bio);+}+returnret;+}++staticblk_status_tbtrfs_encoded_read_check_bio(structbtrfs_io_bio*io_bio)+{+constbooluptodate=io_bio->bio.bi_status==BLK_STS_OK;+structbtrfs_encoded_read_private*priv=io_bio->bio.bi_private;+structinode*inode=priv->inode;+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+u32sectorsize=fs_info->sectorsize;+structbio_vec*bvec;+structbvec_iter_alliter_all;+u64start=io_bio->logical;+inticsum=0;++if(priv->skip_csum||!uptodate)+returnio_bio->bio.bi_status;++bio_for_each_segment_all(bvec,&io_bio->bio,iter_all){+unsignedinti,nr_sectors,pgoff;++nr_sectors=BTRFS_BYTES_TO_BLKS(fs_info,bvec->bv_len);+pgoff=bvec->bv_offset;+for(i=0;i<nr_sectors;i++){+ASSERT(pgoff<PAGE_SIZE);+if(check_data_csum(inode,io_bio,icsum,bvec->bv_page,+pgoff,start))+returnBLK_STS_IOERR;+start+=sectorsize;+icsum++;+pgoff+=sectorsize;+}+}+returnBLK_STS_OK;+}++staticvoidbtrfs_encoded_read_endio(structbio*bio)+{+structbtrfs_encoded_read_private*priv=bio->bi_private;+structbtrfs_io_bio*io_bio=btrfs_io_bio(bio);+blk_status_tstatus;++status=btrfs_encoded_read_check_bio(io_bio);+if(status){+/*+*Thememorybarrierimpliedbytheatomic_dec_return()here+*pairswiththememorybarrierimpliedbythe+*atomic_dec_return()orio_wait_event()in+*btrfs_encoded_read_regular_fill_pages()toensurethatthis+*writeisobservedbeforetheloadofstatusin+*btrfs_encoded_read_regular_fill_pages().+*/+WRITE_ONCE(priv->status,status);+}+if(!atomic_dec_return(&priv->pending))+wake_up(&priv->wait);+btrfs_io_bio_free_csum(io_bio);+bio_put(bio);+}++staticintbtrfs_encoded_read_regular_fill_pages(structinode*inode,u64offset,+u64disk_io_size,structpage**pages)+{+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+structbtrfs_encoded_read_privatepriv={+.inode=inode,+.pending=ATOMIC_INIT(1),+.skip_csum=BTRFS_I(inode)->flags&BTRFS_INODE_NODATASUM,+};+unsignedlongi=0;+u64cur=0;+intret;++init_waitqueue_head(&priv.wait);+/*+*Submitbiosfortheextent,splittingduetobioorstripelimitsas+*necessary.+*/+while(cur<disk_io_size){+structbtrfs_io_geometrygeom;+structbio*bio=NULL;+u64remaining;++ret=btrfs_get_io_geometry(fs_info,BTRFS_MAP_READ,+offset+cur,disk_io_size-cur,+&geom);+if(ret){+WRITE_ONCE(priv.status,errno_to_blk_status(ret));+break;+}+remaining=min(geom.len,disk_io_size-cur);+while(bio||remaining){+size_tbytes=min_t(u64,remaining,PAGE_SIZE);++if(!bio){+bio=btrfs_bio_alloc(offset+cur);+bio->bi_end_io=btrfs_encoded_read_endio;+bio->bi_private=&priv;+bio->bi_opf=REQ_OP_READ;+}++if(!bytes||+bio_add_page(bio,pages[i],bytes,0)<bytes){+blk_status_tstatus;++status=submit_encoded_read_bio(inode,bio,0,+0);+if(status){+WRITE_ONCE(priv.status,status);+bio_put(bio);+gotoout;+}+bio=NULL;+continue;+}++i++;+cur+=bytes;+remaining-=bytes;+}+}++out:+if(atomic_dec_return(&priv.pending))+io_wait_event(priv.wait,!atomic_read(&priv.pending));+/* See btrfs_encoded_read_endio() for ordering. */+returnblk_status_to_errno(READ_ONCE(priv.status));+}++staticssize_tbtrfs_encoded_read_regular(structkiocb*iocb,+structiov_iter*iter,+u64start,u64lockend,+structextent_state**cached_state,+u64offset,u64disk_io_size,+size_tcount,+conststructencoded_iov*encoded,+bool*unlocked)+{+structinode*inode=file_inode(iocb->ki_filp);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+structpage**pages;+unsignedlongnr_pages,i;+u64cur;+size_tpage_offset;+ssize_tret;++nr_pages=DIV_ROUND_UP(disk_io_size,PAGE_SIZE);+pages=kcalloc(nr_pages,sizeof(structpage*),GFP_NOFS);+if(!pages)+return-ENOMEM;+for(i=0;i<nr_pages;i++){+pages[i]=alloc_page(GFP_NOFS|__GFP_HIGHMEM);+if(!pages[i]){+ret=-ENOMEM;+gotoout;+}+}++ret=btrfs_encoded_read_regular_fill_pages(inode,offset,disk_io_size,+pages);+if(ret)+gotoout;++unlock_extent_cached(io_tree,start,lockend,cached_state);+inode_unlock_shared(inode);+*unlocked=true;++ret=copy_encoded_iov_to_iter(encoded,iter);+if(ret)+gotoout;+if(encoded->compression){+i=0;+page_offset=0;+}else{+i=(iocb->ki_pos-start)>>PAGE_SHIFT;+page_offset=(iocb->ki_pos-start)&(PAGE_SIZE-1);+}+cur=0;+while(cur<count){+size_tbytes=min_t(size_t,count-cur,+PAGE_SIZE-page_offset);++if(copy_page_to_iter(pages[i],page_offset,bytes,+iter)!=bytes){+ret=-EFAULT;+gotoout;+}+i++;+cur+=bytes;+page_offset=0;+}+ret=count;+out:+for(i=0;i<nr_pages;i++){+if(pages[i])+__free_page(pages[i]);+}+kfree(pages);+returnret;+}++ssize_tbtrfs_encoded_read(structkiocb*iocb,structiov_iter*iter)+{+structinode*inode=file_inode(iocb->ki_filp);+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);+structextent_io_tree*io_tree=&BTRFS_I(inode)->io_tree;+ssize_tret;+size_tcount;+u64start,lockend,offset,disk_io_size;+structextent_state*cached_state=NULL;+structextent_map*em;+structencoded_iovencoded={};+boolunlocked=false;++ret=generic_encoded_read_checks(iocb,iter);+if(ret<0)+returnret;+if(ret==0)+returncopy_encoded_iov_to_iter(&encoded,iter);+count=ret;++file_accessed(iocb->ki_filp);++inode_lock_shared(inode);++if(iocb->ki_pos>=inode->i_size){+inode_unlock_shared(inode);+returncopy_encoded_iov_to_iter(&encoded,iter);+}+start=ALIGN_DOWN(iocb->ki_pos,fs_info->sectorsize);+/*+*Wedon'tknowhowlongtheextentcontainingiocb->ki_posis,butif+*it'scompressedweknowthatitwon'tbelongerthanthis.+*/+lockend=start+BTRFS_MAX_UNCOMPRESSED-1;++for(;;){+structbtrfs_ordered_extent*ordered;++ret=btrfs_wait_ordered_range(inode,start,+lockend-start+1);+if(ret)+gotoout_unlock_inode;+lock_extent_bits(io_tree,start,lockend,&cached_state);+ordered=btrfs_lookup_ordered_range(BTRFS_I(inode),start,+lockend-start+1);+if(!ordered)+break;+btrfs_put_ordered_extent(ordered);+unlock_extent_cached(io_tree,start,lockend,&cached_state);+cond_resched();+}
This can be replaced with btrfs_lock_and_flush_ordered_range(). Then you can add
Sorry, finally getting back to this after the break. Please correct me
if I'm wrong, but I don't think btrfs_lock_and_flush_ordered_range() is
strong enough here.
An encoded read needs to make sure that any buffered writes are on disk
(since it's basically direct I/O). btrfs_lock_and_flush_ordered_range()
bails immediately if there aren't any ordered extents. As far as I can
tell, ordered extents aren't created until writepage, so if I do some
buffered writes and call btrfs_lock_and_flush_ordered_range() before
writepage creates the ordered extents, it won't flush the buffered
writes like I need it to. This loop with btrfs_wait_ordered_range()
does.
I didn't realize that btrfs_wait_ordered_range() does the fdatawrite_range,
awesome. You can leave it then and add my reviewed-by. Thanks,
Yeah btrfs_wait_ordered_range() leaves something to be desired in
naming. Thanks!
On Thu, Nov 19, 2020 at 09:38:17AM +0200, Amir Goldstein wrote:
On Wed, Nov 18, 2020 at 9:18 PM Omar Sandoval [off-list ref] wrote:
quoted
From: Omar Sandoval <redacted>
Btrfs supports transparent compression: data written by the user can be
compressed when written to disk and decompressed when read back.
However, we'd like to add an interface to write pre-compressed data
directly to the filesystem, and the matching interface to read
compressed data without decompressing it. This adds support for
so-called "encoded I/O" via preadv2() and pwritev2().
A new RWF_ENCODED flags indicates that a read or write is "encoded". If
this flag is set, iov[0].iov_base points to a struct encoded_iov which
is used for metadata: namely, the compression algorithm, unencoded
(i.e., decompressed) length, and what subrange of the unencoded data
should be used (needed for truncated or hole-punched extents and when
reading in the middle of an extent). For reads, the filesystem returns
this information; for writes, the caller provides it to the filesystem.
iov[0].iov_len must be set to sizeof(struct encoded_iov), which can be
used to extend the interface in the future a la copy_struct_from_user().
The remaining iovecs contain the encoded extent.
This adds the VFS helpers for supporting encoded I/O and documentation
for filesystem support.
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <redacted>
---
Documentation/filesystems/encoded_io.rst | 74 ++++++++++
Documentation/filesystems/index.rst | 1 +
fs/read_write.c | 167 +++++++++++++++++++++--
include/linux/fs.h | 11 ++
include/uapi/linux/fs.h | 41 +++++-
5 files changed, 280 insertions(+), 14 deletions(-)
create mode 100644 Documentation/filesystems/encoded_io.rst
@@ -0,0 +1,74 @@+===========+Encoded I/O+===========++Encoded I/O is a mechanism for reading and writing encoded (e.g., compressed+and/or encrypted) data directly from/to the filesystem. The userspace interface+is thoroughly described in the :manpage:`encoded_io(7)` man page; this document+describes the requirements for filesystem support.++First of all, a filesystem supporting encoded I/O must indicate this by setting+the ``FMODE_ENCODED_IO`` flag in its ``file_open`` file operation::+
Hi, Amir, I'm getting back to this now after the holidays.
Should this be FMODE_ALLOW_ENCODED_IO?
How come I see no checks for this flag in vfs code?
Thanks for catching that, apparently I dropped the check between v5 and
v6 when I was resolving a conflict with commit ce71bfea207b ("fs: align
IOCB_* flags with RWF_* flags"). I'll add it back for v7. (The flag
indicates support for encoded I/O, and it's checked at read/write time,
so I think FMODE_ENCODED_IO is still the best name for it.)
You seem to only be checking the O_ flag.
Do we really want to allow setting the O_ flag after open or should we
deny that?
I believe the conclusion after the other thread was to give
O_ALLOW_ENCODED no special treatment, so yes, we should allow it.
quoted
+ static int foo_file_open(struct inode *inode, struct file *filp)
+ {
+ ...
+ filep->f_mode |= FMODE_ENCODED_IO;
+ ...
+ }
+
+Encoded I/O goes through ``read_iter`` and ``write_iter``, designated by the
+``IOCB_ENCODED`` flag in ``kiocb->ki_flags``.
+
+Reads
+=====
+
+Encoded ``read_iter`` should:
+
+1. Call ``generic_encoded_read_checks()`` to validate the file and buffers
+ provided by userspace.
+2. Initialize the ``encoded_iov`` appropriately.
+3. Copy it to the user with ``copy_encoded_iov_to_iter()``.
+4. Copy the encoded data to the user.
+5. Advance ``kiocb->ki_pos`` by ``encoded_iov->len``.
+6. Return the size of the encoded data read, not including the ``encoded_iov``.
+
+There are a few details to be aware of:
+
+* Encoded ``read_iter`` should support reading unencoded data if the extent is
+ not encoded.
+* If the buffers provided by the user are not large enough to contain an entire
+ encoded extent, then ``read_iter`` should return ``-ENOBUFS``. This is to
+ avoid confusing userspace with truncated data that cannot be properly
+ decoded.
+* Reads in the middle of an encoded extent can be returned by setting
+ ``encoded_iov->unencoded_offset`` to non-zero.
+* Truncated unencoded data (e.g., because the file does not end on a block
+ boundary) may be returned by setting ``encoded_iov->len`` to a value smaller
+ value than ``encoded_iov->unencoded_len - encoded_iov->unencoded_offset``.
+
+Writes
+======
+
+Encoded ``write_iter`` should (in addition to the usual accounting/checks done
+by ``write_iter``):
+
+1. Call ``copy_encoded_iov_from_iter()`` to get and validate the
+ ``encoded_iov``.
+2. Call ``generic_encoded_write_checks()`` instead of
+ ``generic_write_checks()``.
+3. Check that the provided encoding in ``encoded_iov`` is supported.
+4. Advance ``kiocb->ki_pos`` by ``encoded_iov->len``.
+5. Return the size of the encoded data written.
+
+Again, there are a few details:
+
+* Encoded ``write_iter`` doesn't need to support writing unencoded data.
+* ``write_iter`` should either write all of the encoded data or none of it; it
+ must not do partial writes.
+* ``write_iter`` doesn't need to validate the encoded data; a subsequent read
+ may return, e.g., ``-EIO`` if the data is not valid.
+* The user may lie about the unencoded size of the data; a subsequent read
+ should truncate or zero-extend the unencoded data rather than returning an
+ error.
+* Be careful of page cache coherency.
@@ -1625,24 +1625,15 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count)return0;}-/*-*Performsnecessarychecksbeforedoingawrite-*-*Canadjustwritingpositionoramountofbytestowrite.-*Returnsappropriateerrorcodethatcallershouldreturnor-*zeroincasethatwriteshouldbeallowed.-*/-ssize_tgeneric_write_checks(structkiocb*iocb,structiov_iter*from)+staticintgeneric_write_checks_common(structkiocb*iocb,loff_t*count){structfile*file=iocb->ki_filp;structinode*inode=file->f_mapping->host;-loff_tcount;-intret;if(IS_SWAPFILE(inode))return-ETXTBSY;-if(!iov_iter_count(from))+if(!*count)return0;/* FIXME: this is for backwards compatibility with 2.4 */
I am not a fan of this trick.
There is no shortage of enums in uapi headers, but I think that if we want
to set values in stone, the values should be set explicitly and not
auto assigned
by compiler.
If anybody ever adds a line, say ENCODED_IOV_COMPRESSION_BTRFS_ZLIB_V2
in the middle of the enum list, it won't be obvious that it's a uapi breakage.
In principle, we could have partitioned the encoding types by domains
(e.g. btrfs),
and the btrfs specific encodings would have been a part of a btrfs
header, but it's
not that important.
However, please move all encoded_io stuff to a new uapi header and do
not include it
from fs.h to avoid having to compile most filesystems every time a new
btrfs private encoding
type is added.
Fine with me, I'll make these #define's and move them to their own
header.
Thanks,
Omar