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. Patch 4 is a fix that this
series depends on; it can be merged independently. Patches 5-8 are Btrfs
prep patches. Patch 9 adds Btrfs encoded read support and patch 10 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.12-rc3.
Changes since v7 [3]:
- Rebased on current kdave/misc-next (v5.12-rc3).
Omar Sandoval (10):
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 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
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.1611346706.git.osandov@fb.com/
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 | 46 +-
fs/btrfs/inode.c | 936 ++++++++++++++++++++---
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/read_write.c | 168 +++-
include/linux/encoded_io.h | 17 +
include/linux/fcntl.h | 2 +-
include/linux/fs.h | 13 +
include/linux/uio.h | 2 +
include/uapi/asm-generic/fcntl.h | 4 +
include/uapi/linux/encoded_io.h | 30 +
include/uapi/linux/fs.h | 5 +-
lib/iov_iter.c | 82 ++
26 files changed, 1378 insertions(+), 201 deletions(-)
create mode 100644 Documentation/filesystems/encoded_io.rst
create mode 100644 include/linux/encoded_io.h
create mode 100644 include/uapi/linux/encoded_io.h
--
2.30.2
@@ -217,6 +217,7 @@ in .Iarg are ignored. On Linux, this command can change only the+.BRO_ALLOW_ENCODED, .BRO_APPEND, .BRO_ASYNC, .BRO_DIRECT,
@@ -1815,6 +1816,13 @@ and the soft or hard user pipe limit has been reached; see .BRpipe(7). .TP .BEPERM+Attempted to set the+.BO_ALLOW_ENCODED+flag and the calling process did not have the+.BCAP_SYS_ADMIN+capability.+.TP+.BEPERM Attempted to clear the .BO_APPEND flag on a file that has the append-only attribute set.
@@ -180,6 +180,14 @@ for details. .PP The full list of file creation flags and file status flags is as follows: .TP+.BO_ALLOW_ENCODED+Open the file with encoded I/O permissions;+see+.BRencoded_io(7).+The caller must have the+.BCAP_SYS_ADMIN+capability.+.TP .BO_APPEND The file is opened in append mode. Before each
@@ -1232,6 +1240,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
@@ -263,6 +263,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.13)"+Read or write encoded (e.g., compressed) data.+See+.BRencoded_io(7). .SHRETURNVALUE On success, .BRreadv(),
@@ -282,6 +287,12 @@ than requested (see and .BRwrite(2)). .PP+If+.BRWF_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 to indicate the error. .SHERRORS The errors are as given for
@@ -312,6 +323,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 additionally 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 additionally 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+.Iencoded_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+.Isizeof(structencoded_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+.B"Filesystem support"+below.+.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 from the first byte of the unencoded data+to the first byte of logical data in the file+(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.+.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(2)+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(2)+is -1, then the file offset is incremented by+.IRlen.+If+.Iiov[0].iov_len+is less than+.Isizeof(structencoded_iov)+in the kernel,+then any fields unknown to user space 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(2)+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(2)+is -1, then the file offset is incremented by+.IRlen.+If+.Iiov[0].iov_len+is less than+.Isizeof(structencoded_iov)+in the kernel and any fields unknown to user space are non-zero,+then+.BRpreadv2(2)+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(2)+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+.Iunencoded_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 if the file description has the+.BO_ALLOW_ENCODED+file status flag set,+and the+.BO_ALLOW_ENCODED+flag may only be set by a thread with the+.BCAP_SYS_ADMIN+capability.+The+.BO_ALLOW_ENCODED+flag can be set by+.BRopen(2)+or+.BRfcntl(2).+It can also be cleared by+.BRfcntl(2);+clearing it does not require+.BCAP_SYS_ADMIN.+Note that it is not cleared on+.BRfork(2)+or+.BRexecve(2).+One may wish to use+.BO_CLOEXEC+with+.BRO_ALLOW_ENCODED.+.SSFilesystemsupport+Encoded I/O is supported on the following filesystems:+.TP+Btrfs (since Linux 5.13)+.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(2):+.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+.SHSEEALSO+.BRopen(2),+.BRpreadv2(2)
Following the semantics of copy_struct_from_user() is certainly a good
idea but can this in any way be rewritten to not look like this; at
least not as crammed. It's a bit painful to follow here what's going.
Following the semantics of copy_struct_from_user() is certainly a good
idea but can this in any way be rewritten to not look like this; at
least not as crammed. It's a bit painful to follow here what's going.
I think that's just the nature of the iov_iter code :) I'm just
following the rest of this file, which uses some mind-expanding macros.
Do you have any suggestions for how to clean this function up?
Following the semantics of copy_struct_from_user() is certainly a good
idea but can this in any way be rewritten to not look like this; at
least not as crammed. It's a bit painful to follow here what's going.
I think that's just the nature of the iov_iter code :) I'm just
following the rest of this file, which uses some mind-expanding macros.
Do you have any suggestions for how to clean this function up?
I think the follow-up discussion this triggered caused an improvement now. :)
Christian
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.
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 ++++
include/linux/fcntl.h | 2 +-
include/uapi/asm-generic/fcntl.h | 4 ++++
7 files changed, 20 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)
@@ -1035,7 +1041,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));
@@ -2892,6 +2892,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>
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 | 168 +++++++++++++++++++++--
include/linux/encoded_io.h | 17 +++
include/linux/fs.h | 13 ++
include/uapi/linux/encoded_io.h | 30 ++++
include/uapi/linux/fs.h | 5 +-
7 files changed, 294 insertions(+), 14 deletions(-)
create mode 100644 Documentation/filesystems/encoded_io.rst
create mode 100644 include/linux/encoded_io.h
create mode 100644 include/uapi/linux/encoded_io.h
@@ -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 +1626,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>
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: 265d4ac03fdf ("btrfs: sink parameter start and len to check_data_csum")
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/inode.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
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.
So direct IO pages doesn't have page::index set at all?
Any reproducer? I'd like to try to reproduce it first.
quoted hunk
Fixes: 265d4ac03fdf ("btrfs: sink parameter start and len to check_data_csum")
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/inode.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
On Wed, Mar 17, 2021 at 07:21:46PM +0800, Qu Wenruo wrote:
On 2021/3/17 上午3:43, 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.
So direct IO pages doesn't have page::index set at all?
No, they don't. Usually you do direct I/O into an anonymous page, but I
suppose you could even do direct I/O into a page mmap'd from another
file or filesystem. In either case, the index isn't meaningful for the
file you're doing direct I/O from.
Any reproducer? I'd like to try to reproduce it first.
The easiest way to see this issue is to apply this patch:
Please add some comment if only for direct IO we need that @start parameter.
Won't that add more confusion? Someone might read that and assume that
they don't need to pass start for a page cache page. In my opinion,
having this change in the git log is enough.
On Wed, Mar 17, 2021 at 07:21:46PM +0800, Qu Wenruo wrote:
quoted
On 2021/3/17 上午3:43, 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.
So direct IO pages doesn't have page::index set at all?
No, they don't. Usually you do direct I/O into an anonymous page, but I
suppose you could even do direct I/O into a page mmap'd from another
file or filesystem. In either case, the index isn't meaningful for the
file you're doing direct I/O from.
quoted
Any reproducer? I'd like to try to reproduce it first.
The easiest way to see this issue is to apply this patch:
Please add some comment if only for direct IO we need that @start parameter.
Won't that add more confusion? Someone might read that and assume that
they don't need to pass start for a page cache page. In my opinion,
having this change in the git log is enough.
That's fine.
Then this patch looks fine to me.
Reviewed-by: Qu Wenruo <redacted>
Thanks,
Qu
From: David Sterba <hidden> Date: 2021-03-18 20:28:30
On Thu, Mar 18, 2021 at 07:47:29AM +0800, Qu Wenruo wrote:
On 2021/3/18 上午2:33, Omar Sandoval wrote:
quoted
On Wed, Mar 17, 2021 at 07:21:46PM +0800, Qu Wenruo wrote:
quoted
On 2021/3/17 上午3:43, 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.
So direct IO pages doesn't have page::index set at all?
No, they don't. Usually you do direct I/O into an anonymous page, but I
suppose you could even do direct I/O into a page mmap'd from another
file or filesystem. In either case, the index isn't meaningful for the
file you're doing direct I/O from.
quoted
Any reproducer? I'd like to try to reproduce it first.
The easiest way to see this issue is to apply this patch:
Please add some comment if only for direct IO we need that @start parameter.
Won't that add more confusion? Someone might read that and assume that
they don't need to pass start for a page cache page. In my opinion,
having this change in the git log is enough.
That's fine.
Then this patch looks fine to me.
Reviewed-by: Qu Wenruo <redacted>
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 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(-)
@@ -1126,8 +1125,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;
@@ -1768,10 +1768,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,
@@ -1780,9 +1781,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;}
@@ -2586,6 +2589,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};
@@ -2660,7 +2664,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);
@@ -2686,20 +2691,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;
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(-)
@@ -661,14 +653,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|
@@ -1056,9 +1049,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>
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.
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Omar Sandoval <redacted>
---
fs/btrfs/ctree.h | 2 +
fs/btrfs/file.c | 5 +
fs/btrfs/inode.c | 497 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 504 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().
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
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 | 38 +++++-
fs/btrfs/inode.c | 259 +++++++++++++++++++++++++++++++++++++++-
fs/btrfs/ordered-data.c | 12 +-
fs/btrfs/ordered-data.h | 2 +
7 files changed, 314 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;
@@ -2712,6 +2712,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),
@@ -2746,7 +2747,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);
@@ -10478,6 +10480,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: Josef Bacik <josef@toxicpanda.com> Date: 2021-03-19 18:21:48
On 3/16/21 3:42 PM, Omar Sandoval wrote:
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. Patch 4 is a fix that this
series depends on; it can be merged independently. Patches 5-8 are Btrfs
prep patches. Patch 9 adds Btrfs encoded read support and patch 10 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.12-rc3.
Al,
Can we get some movement on this? Omar is sort of spinning his wheels here
trying to get this stuff merged, no major changes have been done in a few
postings. Thanks,
Josef
On Fri, Mar 19, 2021 at 11:21 AM Josef Bacik [off-list ref] wrote:
Can we get some movement on this? Omar is sort of spinning his wheels here
trying to get this stuff merged, no major changes have been done in a few
postings.
I'm not Al, and I absolutely detest the IOCB_ENCODED thing, and want
more explanations of why this should be done that way, and pollute our
iov_iter handling EVEN MORE.
Our iov_iter stuff isn't the most legible, and I don't understand why
anybody would ever think it's a good idea to spread what is clearly a
"struct" inside multiple different iov extents.
Honestly, this sounds way more like an ioctl interface than
read/write. We've done that before.
But if it has to be done with an iov_iter, is there *any* reason to
not make it be a hard rule that iov[0] should not be the entirely of
the struct, and the code shouldn't ever need to iterate?
Also I see references to the man-page, but honestly, that's not how
the kernel UAPI should be defined ("just read the man-page"), plus I
refuse to live in the 70's, and consider troff to be an atrocious
format.
So make the UAPI explanation for this horror be in a legible format
that is actually part of the kernel so that I can read what the intent
is, instead of having to decode hieroglypics.
Linus
From: Josef Bacik <josef@toxicpanda.com> Date: 2021-03-19 20:13:49
On 3/19/21 4:08 PM, Linus Torvalds wrote:
On Fri, Mar 19, 2021 at 11:21 AM Josef Bacik [off-list ref] wrote:
quoted
Can we get some movement on this? Omar is sort of spinning his wheels here
trying to get this stuff merged, no major changes have been done in a few
postings.
I'm not Al, and I absolutely detest the IOCB_ENCODED thing, and want
more explanations of why this should be done that way, and pollute our
iov_iter handling EVEN MORE.
Our iov_iter stuff isn't the most legible, and I don't understand why
anybody would ever think it's a good idea to spread what is clearly a
"struct" inside multiple different iov extents.
Honestly, this sounds way more like an ioctl interface than
read/write. We've done that before.
On Fri, Mar 19, 2021 at 01:08:05PM -0700, Linus Torvalds wrote:
On Fri, Mar 19, 2021 at 11:21 AM Josef Bacik [off-list ref] wrote:
quoted
Can we get some movement on this? Omar is sort of spinning his wheels here
trying to get this stuff merged, no major changes have been done in a few
postings.
I'm not Al, and I absolutely detest the IOCB_ENCODED thing, and want
more explanations of why this should be done that way, and pollute our
iov_iter handling EVEN MORE.
Our iov_iter stuff isn't the most legible, and I don't understand why
anybody would ever think it's a good idea to spread what is clearly a
"struct" inside multiple different iov extents.
Honestly, this sounds way more like an ioctl interface than
read/write. We've done that before.
As Josef just mentioned, I started with an ioctl, and Dave Chinner
suggested doing it this way:
https://lore.kernel.org/linux-fsdevel/20190905021012.GL7777@dread.disaster.area/
The nice thing about it is that it sidesteps all of the issues we had
with the dedupe/reflink ioctls over the years where permissions checks
and the like were slightly different from read/write.
But if it has to be done with an iov_iter, is there *any* reason to
not make it be a hard rule that iov[0] should not be the entirely of
the struct, and the code shouldn't ever need to iterate?
For RWF_ENCODED, iov[0] is always used as the entirety of the struct. I
made the helper more generic to support other use cases, but if that's
the main objection I can easily make it specifically use iov[0].
Also I see references to the man-page, but honestly, that's not how
the kernel UAPI should be defined ("just read the man-page"), plus I
refuse to live in the 70's, and consider troff to be an atrocious
format.
No disagreement here, troff is horrible to read.
So make the UAPI explanation for this horror be in a legible format
that is actually part of the kernel so that I can read what the intent
is, instead of having to decode hieroglypics.
I didn't want to document the UAPI in two places that would need to be
kept in sync, but this is fair enough. I'll add UAPI documentation to
the kernel. In the meantime, for anyone else following along, here's the
formatted man page:
ENCODED_IO(7) Linux Programmer's Manual ENCODED_IO(7)
NAME
encoded_io - overview of encoded I/O
DESCRIPTION
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".
Encoded I/O API
Encoded I/O is specified with the RWF_ENCODED flag to
preadv2(2) and pwritev2(2). If RWF_ENCODED is specified, then
iov[0].iov_base points to an encoded_iov structure, defined in
<linux/fs.h> as:
struct encoded_iov {
__aligned_u64 len;
__aligned_u64 unencoded_len;
__aligned_u64 unencoded_offset;
__u32 compression;
__u32 encryption;
};
This may be extended in the future, so iov[0].iov_len must be
set to sizeof(struct encoded_iov) for forward/backward compati‐
bility. The remaining buffers contain the encoded data.
compression and encryption are the encoding fields. compres‐
sion is ENCODED_IOV_COMPRESSION_NONE (zero) or a filesystem-
specific ENCODED_IOV_COMPRESSION_* constant; see Filesystem
support below. encryption is currently always ENCODED_IOV_EN‐
CRYPTION_NONE (zero).
unencoded_len is the length of the unencoded (i.e., decrypted
and decompressed) data. unencoded_offset is the offset from
the first byte of the unencoded data to the first byte of logi‐
cal data in the file (less than or equal to unencoded_len).
len is the length of the data in the file (less than or equal
to unencoded_len - unencoded_offset). See Extent layout below
for some examples.
If the unencoded data is actually longer than unencoded_len,
then it is truncated; if it is shorter, then it is extended
with zeroes.
pwritev2(2) uses the metadata specified in iov[0], writes the
encoded data from the remaining buffers, and returns the number
of encoded bytes written (that is, the sum of iov[n].iov_len
for 1 <= n < 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 offset argument to pwritev2(2) is -1,
then the file offset is incremented by len. If iov[0].iov_len
is less than sizeof(struct encoded_iov) in the kernel, then any
fields unknown to user space 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 errno to E2BIG.
preadv2(2) populates the metadata in 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 offset argument to
preadv2(2) is -1, then the file offset is incremented by len.
If iov[0].iov_len is less than sizeof(struct encoded_iov) in
the kernel and any fields unknown to user space are non-zero,
then preadv2(2) returns -1 and sets errno to 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, then preadv2(2) returns -1 and sets
errno to ENOBUFS.
As the filesystem page cache typically contains decoded data,
encoded I/O bypasses the page cache.
Extent layout
By using len, unencoded_len, and unencoded_offset, it is possi‐
ble to refer to a subset of an unencoded extent.
In the simplest case, len is equal to unencoded_len and unen‐
coded_offset is zero. This means that the entire unencoded ex‐
tent is used.
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
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, unencoded_offset would
be 50, and len would accordingly be unencoded_len - 50.
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 in‐
clude the appropriate padding, and unencoded_len would be 4096.
However, to represent the logical size of the file, len would
be 500 (and unencoded_offset would be 0).
Similar situations can arise in other cases:
* If the filesystem pads data to the filesystem block size be‐
fore compressing, then compressed files with a size un‐
aligned to the filesystem block size will end with an extent
with len < unencoded_len.
* Extents cloned from the middle of a larger encoded extent
with FICLONERANGE may have a non-zero unencoded_offset
and/or len < unencoded_len.
* If the middle of an encoded extent is overwritten, the
filesystem may create extents with a non-zero unencoded_off‐
set and/or len < unencoded_len for the parts that were not
overwritten.
Security
Encoded I/O creates the potential for some security issues:
* 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 ma‐
liciously crafted data.
* Encoded reads may return data which is not logically present
in the file (see the discussion of len vs unencoded_len
above). It may not be intended for this data to be read‐
able.
Therefore, encoded I/O requires privilege. Namely, the RWF_EN‐
CODED flag may only be used if the file description has the
O_ALLOW_ENCODED file status flag set, and the O_ALLOW_ENCODED
flag may only be set by a thread with the CAP_SYS_ADMIN capa‐
bility. The O_ALLOW_ENCODED flag can be set by open(2) or fc‐
ntl(2). It can also be cleared by fcntl(2); clearing it does
not require CAP_SYS_ADMIN. Note that it is not cleared on
fork(2) or execve(2). One may wish to use O_CLOEXEC with O_AL‐
LOW_ENCODED.
Filesystem support
Encoded I/O is supported on the following filesystems:
Btrfs (since Linux 5.13)
Btrfs supports encoded reads and writes of compressed
data. The data is encoded as follows:
* If compression is ENCODED_IOV_COMPRESSION_BTRFS_ZLIB,
then the encoded data is a single zlib stream.
* If compression is ENCODED_IOV_COMPRESSION_BTRFS_ZSTD,
then the encoded data is a single zstd frame com‐
pressed with the windowLog compression parameter set
to no more than 17.
* If compression is one of ENCODED_IOV_COMPRES‐
SION_BTRFS_LZO_4K, ENCODED_IOV_COMPRES‐
SION_BTRFS_LZO_8K, ENCODED_IOV_COMPRES‐
SION_BTRFS_LZO_16K, ENCODED_IOV_COMPRES‐
SION_BTRFS_LZO_32K, or ENCODED_IOV_COMPRES‐
SION_BTRFS_LZO_64K, then the encoded data is com‐
pressed 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 fs/btrfs/lzo.c.
Additionally, there are some restrictions on
pwritev2(2):
* offset (or the current file offset if offset is -1)
must be aligned to the sector size of the filesystem.
* len must be aligned to the sector size of the
filesystem unless the data ends at or beyond the cur‐
rent end of the file.
* unencoded_len and the length of the encoded data must
each be no more than 128 KiB. This limit may in‐
crease in the future.
* The length of the encoded data must be less than or
equal to unencoded_len.
* If using LZO, the filesystem's page size must match
the compression page size.
SEE ALSO
open(2), preadv2(2)
Linux 2020-11-11 ENCODED_IO(7)
From: Christian Brauner <hidden> Date: 2021-03-19 20:44:56
On Fri, Mar 19, 2021 at 01:27:25PM -0700, Omar Sandoval wrote:
On Fri, Mar 19, 2021 at 01:08:05PM -0700, Linus Torvalds wrote:
quoted
On Fri, Mar 19, 2021 at 11:21 AM Josef Bacik [off-list ref] wrote:
quoted
Can we get some movement on this? Omar is sort of spinning his wheels here
trying to get this stuff merged, no major changes have been done in a few
postings.
I'm not Al, and I absolutely detest the IOCB_ENCODED thing, and want
more explanations of why this should be done that way, and pollute our
iov_iter handling EVEN MORE.
Our iov_iter stuff isn't the most legible, and I don't understand why
anybody would ever think it's a good idea to spread what is clearly a
"struct" inside multiple different iov extents.
Honestly, this sounds way more like an ioctl interface than
read/write. We've done that before.
As Josef just mentioned, I started with an ioctl, and Dave Chinner
suggested doing it this way:
https://lore.kernel.org/linux-fsdevel/20190905021012.GL7777@dread.disaster.area/
The nice thing about it is that it sidesteps all of the issues we had
with the dedupe/reflink ioctls over the years where permissions checks
and the like were slightly different from read/write.
quoted
But if it has to be done with an iov_iter, is there *any* reason to
not make it be a hard rule that iov[0] should not be the entirely of
the struct, and the code shouldn't ever need to iterate?
For RWF_ENCODED, iov[0] is always used as the entirety of the struct. I
made the helper more generic to support other use cases, but if that's
the main objection I can easily make it specifically use iov[0].
quoted
Also I see references to the man-page, but honestly, that's not how
the kernel UAPI should be defined ("just read the man-page"), plus I
refuse to live in the 70's, and consider troff to be an atrocious
format.
No disagreement here, troff is horrible to read.
Not a fan of troff either: One thing that might help you a little bit is
to use pandoc to convert to troff. You can write your manpage in
markdown or rst and then convert it into troff via pandoc. Still might
require some massaging but it makes it considerably more pleasant to
write a manpage.
Christian
On Fri, Mar 19, 2021 at 1:27 PM Omar Sandoval [off-list ref] wrote:
For RWF_ENCODED, iov[0] is always used as the entirety of the struct. I
made the helper more generic to support other use cases, but if that's
the main objection I can easily make it specifically use iov[0].
Honestly, with new interfaces, I'd prefer to always start off as
limited as possible.
And read/write is not very limited (but O_ALLOW_ENCODED and
RWF_ENCODED at least helps with the "fool suid program to do it"). But
at least we could make sure that the structure then has to be as
strict as humanly possible.
So it's not so much a "main objection" as more about trying to make
the rules stricter in the hope that that at least makes only one very
particular way of doing things valid. I'd hate for user space to start
'streaming" struct data.
quoted
Also I see references to the man-page, but honestly, that's not how
the kernel UAPI should be defined ("just read the man-page"), plus I
refuse to live in the 70's, and consider troff to be an atrocious
format.
No disagreement here, troff is horrible to read.
quoted
So make the UAPI explanation for this horror be in a legible format
that is actually part of the kernel so that I can read what the intent
is, instead of having to decode hieroglypics.
I didn't want to document the UAPI in two places that would need to be
kept in sync
Honestly, I would suggest that nobody ever write troff format stuff.
I don't think it supports UTF-8 properly, for example, which means
that you can't even give credit to people properly etc.
RST isn't perfect, but at least it's human-legible, and it's obviously
what we're encouraging for kernel use. You can use rst2man to convert
to bad formats (and yes, you might lose something in the translation,
eg proper names etc).
Almost anything else(*) is better than troff. But at least I can read
the formatted version.
Linus
(*) With the possible exception of "info" files. Now *there* is a
truly pointless format maximally designed to make it inconvenient for
users.
On Fri, Mar 19, 2021 at 01:55:18PM -0700, Linus Torvalds wrote:
On Fri, Mar 19, 2021 at 1:27 PM Omar Sandoval [off-list ref] wrote:
quoted
For RWF_ENCODED, iov[0] is always used as the entirety of the struct. I
made the helper more generic to support other use cases, but if that's
the main objection I can easily make it specifically use iov[0].
Honestly, with new interfaces, I'd prefer to always start off as
limited as possible.
And read/write is not very limited (but O_ALLOW_ENCODED and
RWF_ENCODED at least helps with the "fool suid program to do it"). But
at least we could make sure that the structure then has to be as
strict as humanly possible.
So it's not so much a "main objection" as more about trying to make
the rules stricter in the hope that that at least makes only one very
particular way of doing things valid. I'd hate for user space to start
'streaming" struct data.
After spending a few minutes trying to simplify copy_struct_from_iter(),
it's honestly easier to just use the iterate_all_kinds() craziness than
open coding it to only operate on iov[0]. But that's an implementation
detail, and we can trivially make the interface stricter:
I.e., the size of the userspace structure is always the remaining size
of the current segment. Maybe we can even throw in a check that we're
either at the beginning of the current segment or the very beginning of
the whole iter, what do you think?
(Again, this is what RWF_ENCODED already does, it was just easier to
write copy_struct_from_iter() more generically).
quoted
quoted
Also I see references to the man-page, but honestly, that's not how
the kernel UAPI should be defined ("just read the man-page"), plus I
refuse to live in the 70's, and consider troff to be an atrocious
format.
No disagreement here, troff is horrible to read.
quoted
So make the UAPI explanation for this horror be in a legible format
that is actually part of the kernel so that I can read what the intent
is, instead of having to decode hieroglypics.
I didn't want to document the UAPI in two places that would need to be
kept in sync
Honestly, I would suggest that nobody ever write troff format stuff.
I don't think it supports UTF-8 properly, for example, which means
that you can't even give credit to people properly etc.
RST isn't perfect, but at least it's human-legible, and it's obviously
what we're encouraging for kernel use. You can use rst2man to convert
to bad formats (and yes, you might lose something in the translation,
eg proper names etc).
Almost anything else(*) is better than troff. But at least I can read
the formatted version.
Linus
(*) With the possible exception of "info" files. Now *there* is a
truly pointless format maximally designed to make it inconvenient for
users.
On Fri, Mar 19, 2021 at 2:12 PM Omar Sandoval [off-list ref] wrote:
After spending a few minutes trying to simplify copy_struct_from_iter(),
it's honestly easier to just use the iterate_all_kinds() craziness than
open coding it to only operate on iov[0]. But that's an implementation
detail, and we can trivially make the interface stricter:
This is an improvement, but talking about the iterate_all_kinds()
craziness, I think your existing code is broken.
That third case (kernel pointer source):
+ copy = min(ksize - copied, v.iov_len);
+ memcpy(dst + copied, v.iov_base, copy);
+ if (memchr_inv(v.iov_base, 0, v.iov_len))
+ return -E2BIG;
can't be right. Aren't you checking that it's *all* zero, even the
part you copied?
Our iov_iter stuff is really complicated already, this is part of why
I'm not a huge fan of using it.
I still suspect you'd be better off not using the iterate_all_kinds()
thing at all, and just explicitly checking ITER_BVEC/ITER_KVEC
manually.
Because you can play games like fooling your "copy_struct_from_iter()"
to not copy anything at all with ITER_DISCARD, can't you?
Which then sounds like it might end up being useful as a kernel data
leak, because it will use some random uninitialized kernel memory for
the structure.
Now, I don't think you can actually get that ITER_DISCARD case, so
this is not *really* a problem, but it's another example of how that
iterate_all_kinds() thing has these subtle cases embedded into it.
The whole point of copy_struct_from_iter() is presumably to be the
same kind of "obviously safe" interface as copy_struct_from_user() is
meant to be, so these subtle cases just then make me go "Hmm".
I think just open-coding this when you know there is no actual
looping going on, and the data has to be at the *beginning*, should be
fairly simple. What makes iterate_all_kinds() complicated is that
iteration, the fact that there can be empty entries in there, but it's
also that "iov_offset" thing etc.
For the case where you just (a) require that iov_offset is zero, and
(b) everything has to fit into the very first iov entry (regardless of
what type that iov entry is), I think you actually end up with a much
simpler model.
I do realize that I am perhaps concentrating a bit too much on this
one part of the patch series, but the iov_iter thing has bitten us
before. And it has bitten really core developers and then Al has had
to fix up mistakes.
In fact, it wasn't that long ago that I asked Al to verify code I
wrote, because I was worried about having missed something subtle. So
now when I see these iov_iter users, it just makes me go all nervous.
Linus
On Fri, Mar 19, 2021 at 02:47:03PM -0700, Linus Torvalds wrote:
On Fri, Mar 19, 2021 at 2:12 PM Omar Sandoval [off-list ref] wrote:
quoted
After spending a few minutes trying to simplify copy_struct_from_iter(),
it's honestly easier to just use the iterate_all_kinds() craziness than
open coding it to only operate on iov[0]. But that's an implementation
detail, and we can trivially make the interface stricter:
This is an improvement, but talking about the iterate_all_kinds()
craziness, I think your existing code is broken.
That third case (kernel pointer source):
+ copy = min(ksize - copied, v.iov_len);
+ memcpy(dst + copied, v.iov_base, copy);
+ if (memchr_inv(v.iov_base, 0, v.iov_len))
+ return -E2BIG;
can't be right. Aren't you checking that it's *all* zero, even the
part you copied?
Oops, that should of course be
if (memchr_inv(v.iov_base + copy, 0, v.iov_len - copy))
return -E2BIG;
like the other cases. Point taken, though.
Our iov_iter stuff is really complicated already, this is part of why
I'm not a huge fan of using it.
I still suspect you'd be better off not using the iterate_all_kinds()
thing at all, and just explicitly checking ITER_BVEC/ITER_KVEC
manually.
Because you can play games like fooling your "copy_struct_from_iter()"
to not copy anything at all with ITER_DISCARD, can't you?
Which then sounds like it might end up being useful as a kernel data
leak, because it will use some random uninitialized kernel memory for
the structure.
Now, I don't think you can actually get that ITER_DISCARD case, so
this is not *really* a problem, but it's another example of how that
iterate_all_kinds() thing has these subtle cases embedded into it.
Right, that would probably be better off returning EFAULT or something
for ITER_DISCARD.
The whole point of copy_struct_from_iter() is presumably to be the
same kind of "obviously safe" interface as copy_struct_from_user() is
meant to be, so these subtle cases just then make me go "Hmm".
I think just open-coding this when you know there is no actual
looping going on, and the data has to be at the *beginning*, should be
fairly simple. What makes iterate_all_kinds() complicated is that
iteration, the fact that there can be empty entries in there, but it's
also that "iov_offset" thing etc.
For the case where you just (a) require that iov_offset is zero, and
(b) everything has to fit into the very first iov entry (regardless of
what type that iov entry is), I think you actually end up with a much
simpler model.
I do realize that I am perhaps concentrating a bit too much on this
one part of the patch series, but the iov_iter thing has bitten us
before. And it has bitten really core developers and then Al has had
to fix up mistakes.
In fact, it wasn't that long ago that I asked Al to verify code I
wrote, because I was worried about having missed something subtle. So
now when I see these iov_iter users, it just makes me go all nervous.
So here's what it looks like with these restrictions (chances are
there's a bug or two in here):
int copy_struct_from_iter(void *dst, size_t ksize, struct iov_iter *i)
{
size_t usize;
int ret;
if (i->iov_offset != 0)
return -EINVAL;
if (iter_is_iovec(i)) {
usize = i->iov->iov_len;
might_fault();
if (copyin(dst, i->iov->iov_base, min(ksize, usize)))
return -EFAULT;
if (usize > ksize) {
ret = check_zeroed_user(i->iov->iov_base + ksize,
usize - ksize);
if (ret < 0)
return ret;
else if (ret == 0)
return -E2BIG;
}
} else if (iov_iter_is_kvec(i)) {
usize = i->kvec->iov_len;
memcpy(dst, i->kvec->iov_base, min(ksize, usize));
if (usize > ksize &&
memchr_inv(i->kvec->iov_base + ksize, 0, usize - ksize))
return -E2BIG;
} else if (iov_iter_is_bvec(i)) {
char *p;
usize = i->bvec->bv_len;
p = kmap_atomic(i->bvec->bv_page);
memcpy(dst, p + i->bvec->bv_offset, min(ksize, usize));
if (usize > ksize &&
memchr_inv(p + i->bvec->bv_offset + ksize, 0,
usize - ksize)) {
kunmap_atomic(p);
return -E2BIG;
}
kunmap_atomic(p);
} else {
return -EFAULT;
}
if (usize < ksize)
memset(dst + usize, 0, ksize - usize);
iov_iter_advance(i, usize);
return 0;
}
Not much shorter, but it is easier to follow.
On Fri, Mar 19, 2021 at 3:46 PM Omar Sandoval [off-list ref] wrote:
Not much shorter, but it is easier to follow.
Yeah, that looks about right to me.
You should probably use kmap_local_page() rather than kmap_atomic()
these days, but other than that this looks fairly straightforward, and
I much prefer the model where we very much force that "must be the
first iovec entry".
As you say, maybe not shorter, but a lot more straightforward.
That said, looking through the patch series, I see at least one other
issue. Look at parisc:
+#define O_ALLOW_ENCODED 100000000
yeah, that's completely wrong. I see how it happened, but that's _really_ wrong.
I would want others to take a look in case there's something else. I'm
not qualified to comment about (nor do I deeply care) about the btrfs
parts, but the generic interface parts should most definitely get more
attention.
By Al, if possible, but other fs people too..
Linus
On Fri, Mar 19, 2021 at 05:31:18PM -0700, Linus Torvalds wrote:
On Fri, Mar 19, 2021 at 3:46 PM Omar Sandoval [off-list ref] wrote:
quoted
Not much shorter, but it is easier to follow.
Yeah, that looks about right to me.
You should probably use kmap_local_page() rather than kmap_atomic()
these days, but other than that this looks fairly straightforward, and
I much prefer the model where we very much force that "must be the
first iovec entry".
To be exact, this code only enforces that the iov_iter is at the
beginning of the current entry. As far as I can tell, iov_iter doesn't
track its position overall, so there's no way to tell whether the
current entry is the first one.
As you say, maybe not shorter, but a lot more straightforward.
That said, looking through the patch series, I see at least one other
issue. Look at parisc:
+#define O_ALLOW_ENCODED 100000000
yeah, that's completely wrong. I see how it happened, but that's _really_ wrong.
Ugh, that's embarrassing. It also happens to add exactly one new bit to
VALID_OPEN_FLAGS, so the BUILD_BUG_ON() in fcntl_init() didn't catch it.
I would want others to take a look in case there's something else. I'm
not qualified to comment about (nor do I deeply care) about the btrfs
parts, but the generic interface parts should most definitely get more
attention.
By Al, if possible, but other fs people too..
That's all I ask. I'll fix your comments and wait a few days to get some
more feedback on the fs side before I resend the patch bomb.
Thanks,
Omar