sendfile regression detected in ZFS 2.0 (tip) with 5.10-rc2 (and not 5.9) #11151
Comments
ColinIanKing
added
Status: Triage Needed
|
This looks to be because of the removal of the |
|
Proper fix will probably require extending UIO with pipes; if I remember correctly, that proper support would need at least some bits from the direct IO PR (#10018). |
Right, registering the generic splice_read / splice_write helpers would get us most of the way there. However it looks like we'll also need to update the diff --git a/module/os/linux/zfs/zpl_file.c b/module/os/linux/zfs/zpl_file.c
index 90d9737f0..d8ea78aaf 100644
--- a/module/os/linux/zfs/zpl_file.c
+++ b/module/os/linux/zfs/zpl_file.c
@@ -1035,6 +1036,8 @@ const struct file_operations zpl_file_operations = {
#endif
.read_iter = zpl_iter_read,
.write_iter = zpl_iter_write,
+ .splice_read = generic_file_splice_read,
+ .splice_write = iter_file_splice_write,
#else
.read = do_sync_read,
.write = do_sync_write, |
behlendorf
added
the
Type: Building
|
For now, I have this patch to work around the lack of support for pipes down the stack - it seems to work, but I probably wouldn't want to merge this 😄 it's based on the generic helpers (which might be an issue for merging I guess, even some comments stayed the same 🙂...). diff --git a/module/os/linux/zfs/zpl_file.c b/module/os/linux/zfs/zpl_file.c
index 90d9737f0..e3d82f330 100644
--- a/module/os/linux/zfs/zpl_file.c
+++ b/module/os/linux/zfs/zpl_file.c
@@ -27,6 +27,7 @@
#ifdef CONFIG_COMPAT
#include <linux/compat.h>
#endif
+#include <linux/splice.h>
#include <sys/file.h>
#include <sys/dmu_objset.h>
#include <sys/zfs_znode.h>
@@ -1016,6 +1017,101 @@ zpl_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
#endif /* CONFIG_COMPAT */
+static ssize_t zpl_file_splice_read(struct file *in, loff_t *ppos,
+ struct pipe_inode_info *pipe, size_t len,
+ unsigned int flags)
+{
+ cred_t *cr = CRED();
+ struct inode *ip = in->f_mapping->host;
+ struct iov_iter to;
+ struct page **pages;
+ unsigned int nr_pages;
+ unsigned int mask;
+ size_t offset, base, copied = 0;
+ ssize_t res;
+ int i;
+
+ if (pipe_full(pipe->head, pipe->tail, pipe->max_usage))
+ return -EAGAIN;
+
+ /*
+ * Try to keep page boundaries matching to source pagecache ones -
+ * it probably won't be much help, but...
+ */
+ offset = *ppos & ~PAGE_MASK;
+
+ iov_iter_pipe(&to, READ, pipe, len + offset);
+
+ res = iov_iter_get_pages_alloc(&to, &pages, len + offset, &base);
+ if (res <= 0)
+ return -ENOMEM;
+
+ nr_pages = DIV_ROUND_UP(res + base, PAGE_SIZE);
+
+ mask = pipe->ring_size - 1;
+ pipe->bufs[to.head & mask].offset = offset;
+ pipe->bufs[to.head & mask].len -= offset;
+
+ for (i = 0; i < nr_pages; i++) {
+ struct iovec vec;
+ size_t this_len = min_t(size_t, len, PAGE_SIZE - offset);
+ vec.iov_base = page_address(pages[i]) + offset;
+ vec.iov_len = this_len;
+ len -= this_len;
+ res = zpl_read_common_iovec(ip, &vec, this_len, 1, ppos,
+ UIO_SYSSPACE, flags, cr, offset);
+ if (res < 0)
+ goto out;
+ else
+ copied += res;
+ offset = 0;
+ }
+
+ return copied;
+out:
+ for (i = 0; i < nr_pages; i++)
+ put_page(pages[i]);
+ kvfree(pages);
+ iov_iter_advance(&to, copied); /* truncates and discards */
+ return res;
+}
+
+static int zpl_write_pipe_buf(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
+ struct splice_desc *sd)
+{
+ int ret;
+ void *data;
+ loff_t tmp = sd->pos;
+
+ data = kmap(buf->page);
+ ret = __kernel_write(sd->u.file, data + buf->offset, sd->len, &tmp);
+ kunmap(buf->page);
+
+ return ret;
+}
+
+static ssize_t zpl_file_splice_write(struct pipe_inode_info *pipe,
+ struct file *out, loff_t *ppos,
+ size_t len, unsigned int flags)
+{
+ ssize_t ret;
+
+ struct splice_desc sd = {
+ .total_len = len,
+ .flags = flags,
+ .pos = *ppos,
+ .u.file = out,
+ };
+
+ pipe_lock(pipe);
+ ret = __splice_from_pipe(pipe, &sd, zpl_write_pipe_buf);
+ pipe_unlock(pipe);
+ if (ret > 0)
+ *ppos += ret;
+
+ return ret;
+}
+
const struct address_space_operations zpl_address_space_operations = {
.readpages = zpl_readpages,
.readpage = zpl_readpage,
@@ -1051,6 +1147,8 @@ const struct file_operations zpl_file_operations = {
#ifdef CONFIG_COMPAT
.compat_ioctl = zpl_compat_ioctl,
#endif
+ .splice_write = zpl_file_splice_write,
+ .splice_read = zpl_file_splice_read,
};
const struct file_operations zpl_dir_file_operations = { |
|
@snajpa there are only so many ways to copy from A to B, some version of what you're suggesting should be fine if we need a quick fix. That said, it's really pretty sub-optimal because we're calling |
|
The suggested workaround does have some issues of it's own: [ 116.832238] run fstests generic/013 at 2020-11-25 19:41:25 |
|
Given that 5.10 will be complete in a few weeks, is there a planned fix for this regression for 5.10+ |
|
Not yet. I was planning to look in to putting together a fix for this next week. |
|
Thanks for the update - much appreciated! |
|
@snajpa @ColinIanKing I've opened PR #11351 with a proposed fix. Any feedback or testing would be welcome. The change may look large but it ends up simplifying quite a few areas for 4.10 and newer kernels which is nice. |
|
@behlendorf - thanks, I've tested this against 5.10 and the Ubuntu 5.10 kernel against all our internal ZFS regression tests and it works perfectly. Code is solid and 100% pass. Much appreciated for the speedy fix! |
|
Is it possible to patch the kernel to restore what was removed to cause this breakage? I am looking for a short term fix until the PR makes its way into master. Portage is completely broken in 5.10 right now. |
|
You may be able to revert torvalds/linux@36e2c74 from your kernel to restore the 5.9 kernel behavior. I expect this will be merged tomorrow. If possible it would great if you could verify this change does fully resolve the portage breakage. |
|
Also, should we add a kernel log message in this code path for the "Invalid Argument" that portage sees during copyfile()? I was floored why portage was throwing this error out of the blue doing just copy of the file (manual 'cp' worked). I had no clue as to what happened. |
|
Closed by #11351 . |
|
Did not even let me login to the desktop: |
|
Updated to master as of yesterday night. |
|
@sunilvaltix there was a small follow up fix merged yesterday in #11378 for this. |
|
I get something like this too with zfs master, but on both 5.9 and 5.8, with 5.10 configure bails out with an error saying that up to 5.9 is supported. |
|
@rkitover can you specify what version of ZFS were you using? Was this the 2.0 tag or a recent commit form the master branch? |
|
Not sure how to find that in gentoo right now, but I tried a few hours ago, I am going to try again right now. |
|
But more importantly, I am using sys-fs/zfs-kmod-9999 which is git master. |
|
Found it, this is the last commit I tried: |
|
OK, that makes sense. The fix for this went is as the next commit to master 9ac535e which appears not to be included sys-fs/zfs-kmod-9999 quite yet. |
|
Just built 39372fa and that seems to work fine with 5.9, still can't build with 5.10 but I suppose that's expected, although it's hard to find information on this. Maybe we could have a wiki page that tracks issues for major kernel versions and what work is happening wrt. that. Yeah the -9999 ebuilds in gentoo generally build whatever is the tip of master at the time you call |
|
@rkitover could you open a new issue with the 5.10 build failure on Gentoo. I've been able to build the master branch just fine with the Fedora 5.10 kernel and I'm not aware of any additional patches that are needed. |
|
I was incorrect in saying this is 5.10, this is actually git master, but anyway opened #11387. |
Running xfs test 249 I'm seeing a regression just on the 5.10-rc2 kernel (and not on the 5.9 stable kernel) with ZFS. The failure is in sendfile returning -EINVAL with the 5.10-rc2 kernel where as in 5.9 it works fine.
Just a side note, I back-ported the ZFS 5.10/5.9 compat changes to an earlier 0.8.4 ZFS and I see the same issue, so I'm not sure if this a 5.10-rc2 issue or a ZFS compat issue or a combo of both.
Describe how to reproduce the problem
Reproducer:
With ZFS 2.0 + 5.10-rc2 we see the sendfile return an unexpected -EINVAL as follows:
This works fine on 5.9 with ZFS 2.0.
The text was updated successfully, but these errors were encountered: