The v9fs file operations were missing the splice_read operations, which
breaks sendfile() of files on such a filesystem. I discovered this while
trying to load an eBPF program using iproute2 inside a 'virtme' environment
which uses 9pfs for the virtual file system. iproute2 relies on sendfile()
with an AF_ALG socket to hash files, which was erroring out in the virtual
environment.
Since generic_file_splice_read() seems to just implement splice_read in
terms of the read_iter operation, I simply added the generic implementation
to the file operations, which fixed the error I was seeing. A quick grep
indicates that this is what most other file systems do as well.
The only caveat is that my test case was only hitting the
v9fs_file_operations_dotl implementation. I added it to the other file
operations structs as well because it seemed like the sensible thing to do
given that they all implement read_iter, but those are only compile tested.
Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
---
fs/9p/vfs_file.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index b177fd3b1eb3..01026b47018c 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -655,6 +655,7 @@ const struct file_operations v9fs_cached_file_operations = {
.release = v9fs_dir_release,
.lock = v9fs_file_lock,
.mmap = v9fs_file_mmap,
+ .splice_read = generic_file_splice_read,
.fsync = v9fs_file_fsync,
};
@@ -667,6 +668,7 @@ const struct file_operations v9fs_cached_file_operations_dotl = {
.lock = v9fs_file_lock_dotl,
.flock = v9fs_file_flock_dotl,
.mmap = v9fs_file_mmap,
+ .splice_read = generic_file_splice_read,
.fsync = v9fs_file_fsync_dotl,
};
@@ -678,6 +680,7 @@ const struct file_operations v9fs_file_operations = {
.release = v9fs_dir_release,
.lock = v9fs_file_lock,
.mmap = generic_file_readonly_mmap,
+ .splice_read = generic_file_splice_read,
.fsync = v9fs_file_fsync,
};
@@ -690,6 +693,7 @@ const struct file_operations v9fs_file_operations_dotl = {
.lock = v9fs_file_lock_dotl,
.flock = v9fs_file_flock_dotl,
.mmap = generic_file_readonly_mmap,
+ .splice_read = generic_file_splice_read,
.fsync = v9fs_file_fsync_dotl,
};
@@ -701,6 +705,7 @@ const struct file_operations v9fs_mmap_file_operations = {
.release = v9fs_dir_release,
.lock = v9fs_file_lock,
.mmap = v9fs_mmap_file_mmap,
+ .splice_read = generic_file_splice_read,
.fsync = v9fs_file_fsync,
};
@@ -713,5 +718,6 @@ const struct file_operations v9fs_mmap_file_operations_dotl = {
.lock = v9fs_file_lock_dotl,
.flock = v9fs_file_flock_dotl,
.mmap = v9fs_mmap_file_mmap,
+ .splice_read = generic_file_splice_read,
.fsync = v9fs_file_fsync_dotl,
};
--
2.29.2
Toke Høiland-Jørgensen wrote on Tue, Dec 01, 2020:
> The v9fs file operations were missing the splice_read operations, which
> breaks sendfile() of files on such a filesystem. I discovered this while
> trying to load an eBPF program using iproute2 inside a 'virtme' environment
> which uses 9pfs for the virtual file system. iproute2 relies on sendfile()
> with an AF_ALG socket to hash files, which was erroring out in the virtual
> environment.
>
> Since generic_file_splice_read() seems to just implement splice_read in
> terms of the read_iter operation, I simply added the generic implementation
> to the file operations, which fixed the error I was seeing. A quick grep
> indicates that this is what most other file systems do as well.
Good catch, might as well do that.
I'm surprised you didn't hit the same problem with splice_write?
I see iter_file_splice_write being used for it on many filesystems,
it's probably better to add both?
> The only caveat is that my test case was only hitting the
> v9fs_file_operations_dotl implementation. I added it to the other file
> operations structs as well because it seemed like the sensible thing to do
> given that they all implement read_iter, but those are only compile tested.
The logic is close enough that it should work, I'll run it through in
cached mode at least first though (just mount with cache=loose or
cache=fscache to hit v9fs_cached_file_operations_dotl yourself if you
want to)
non-dotl operations are harder to test, I don't have any server
compatible either so we'll have to trust it works close enough...
(note you can write comments such as this one after the three dashes
line before the diff chunk so maintainers can reply without having it in
commit message itself)
--
Dominique
Dominique Martinet wrote on Tue, Dec 01, 2020:
> > Since generic_file_splice_read() seems to just implement splice_read in
> > terms of the read_iter operation, I simply added the generic implementation
> > to the file operations, which fixed the error I was seeing. A quick grep
> > indicates that this is what most other file systems do as well.
>
> Good catch, might as well do that.
> I'm surprised you didn't hit the same problem with splice_write?
>
> I see iter_file_splice_write being used for it on many filesystems,
> it's probably better to add both?
Yeah, I confirm both are needed (the second for the pipe -> fs side)
This made me test copy_file_range, and it works with both as well (used
not to)
interestingly on older kernels this came as default somehow? I have
splice working on 5.4.67 :/ so this broke somewhat recently...
I'll add an extra patch with the second and take your patch.
Thanks!
--
Dominique
The default splice operations got removed recently, add it back to 9p
with iter_file_splice_write like many other filesystems do.
Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
Cc: Toke Høiland-Jørgensen <[email protected]>
Signed-off-by: Dominique Martinet <[email protected]>
---
fs/9p/vfs_file.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index 145f6f83aa9a..5f9c0c796a37 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -668,6 +668,7 @@ const struct file_operations v9fs_cached_file_operations = {
.lock = v9fs_file_lock,
.mmap = v9fs_file_mmap,
.splice_read = generic_file_splice_read,
+ .splice_write = iter_file_splice_write,
.fsync = v9fs_file_fsync,
};
@@ -681,6 +682,7 @@ const struct file_operations v9fs_cached_file_operations_dotl = {
.flock = v9fs_file_flock_dotl,
.mmap = v9fs_file_mmap,
.splice_read = generic_file_splice_read,
+ .splice_write = iter_file_splice_write,
.fsync = v9fs_file_fsync_dotl,
};
@@ -693,6 +695,7 @@ const struct file_operations v9fs_file_operations = {
.lock = v9fs_file_lock,
.mmap = generic_file_readonly_mmap,
.splice_read = generic_file_splice_read,
+ .splice_write = iter_file_splice_write,
.fsync = v9fs_file_fsync,
};
@@ -706,6 +709,7 @@ const struct file_operations v9fs_file_operations_dotl = {
.flock = v9fs_file_flock_dotl,
.mmap = generic_file_readonly_mmap,
.splice_read = generic_file_splice_read,
+ .splice_write = iter_file_splice_write,
.fsync = v9fs_file_fsync_dotl,
};
@@ -718,6 +722,7 @@ const struct file_operations v9fs_mmap_file_operations = {
.lock = v9fs_file_lock,
.mmap = v9fs_mmap_file_mmap,
.splice_read = generic_file_splice_read,
+ .splice_write = iter_file_splice_write,
.fsync = v9fs_file_fsync,
};
@@ -731,5 +736,6 @@ const struct file_operations v9fs_mmap_file_operations_dotl = {
.flock = v9fs_file_flock_dotl,
.mmap = v9fs_mmap_file_mmap,
.splice_read = generic_file_splice_read,
+ .splice_write = iter_file_splice_write,
.fsync = v9fs_file_fsync_dotl,
};
--
2.28.0