linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fs: 9p: add generic splice_read file operations
@ 2020-12-01 13:54 Toke Høiland-Jørgensen
  2020-12-01 14:57 ` Dominique Martinet
  0 siblings, 1 reply; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-12-01 13:54 UTC (permalink / raw)
  To: Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet,
	v9fs-developer, linux-kernel
  Cc: Toke Høiland-Jørgensen

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 <toke@redhat.com>
---
 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs: 9p: add generic splice_read file operations
  2020-12-01 13:54 [PATCH] fs: 9p: add generic splice_read file operations Toke Høiland-Jørgensen
@ 2020-12-01 14:57 ` Dominique Martinet
  2020-12-01 15:16   ` [V9fs-developer] " Dominique Martinet
  0 siblings, 1 reply; 9+ messages in thread
From: Dominique Martinet @ 2020-12-01 14:57 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Eric Van Hensbergen, Latchesar Ionkov, v9fs-developer, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [V9fs-developer] [PATCH] fs: 9p: add generic splice_read file operations
  2020-12-01 14:57 ` Dominique Martinet
@ 2020-12-01 15:16   ` Dominique Martinet
  2020-12-01 15:23     ` Toke Høiland-Jørgensen
  2020-12-01 15:44     ` [PATCH] fs: 9p: add generic splice_write file operation Dominique Martinet
  0 siblings, 2 replies; 9+ messages in thread
From: Dominique Martinet @ 2020-12-01 15:16 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Eric Van Hensbergen, Latchesar Ionkov, linux-kernel, v9fs-developer

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [V9fs-developer] [PATCH] fs: 9p: add generic splice_read file operations
  2020-12-01 15:16   ` [V9fs-developer] " Dominique Martinet
@ 2020-12-01 15:23     ` Toke Høiland-Jørgensen
  2020-12-01 15:39       ` Dominique Martinet
  2020-12-01 15:44     ` [PATCH] fs: 9p: add generic splice_write file operation Dominique Martinet
  1 sibling, 1 reply; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-12-01 15:23 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Eric Van Hensbergen, Latchesar Ionkov, linux-kernel, v9fs-developer

Dominique Martinet <asmadeus@codewreck.org> writes:

> 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)

Yeah, makes sense; I was only testing with a very specific use case
where a file is being passed to the kernel with sendfile().

> 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...

Huh, no idea; this is my first time digging into filesystem code, I
normally do networking and BPF :)

> I'll add an extra patch with the second and take your patch.
> Thanks!

Awesome, thanks!

-Toke


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [V9fs-developer] [PATCH] fs: 9p: add generic splice_read file operations
  2020-12-01 15:23     ` Toke Høiland-Jørgensen
@ 2020-12-01 15:39       ` Dominique Martinet
  2020-12-01 20:37         ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 9+ messages in thread
From: Dominique Martinet @ 2020-12-01 15:39 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Eric Van Hensbergen, Latchesar Ionkov, linux-kernel, v9fs-developer

Toke Høiland-Jørgensen wrote on Tue, Dec 01, 2020:
> > 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...
> 
> Huh, no idea; this is my first time digging into filesystem code, I
> normally do networking and BPF :)

In case anyone else wants to know, this broke in 5.10-rc1 with
36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")

So really a recent regression, good catch :)

-- 
Dominique

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] fs: 9p: add generic splice_write file operation
  2020-12-01 15:16   ` [V9fs-developer] " Dominique Martinet
  2020-12-01 15:23     ` Toke Høiland-Jørgensen
@ 2020-12-01 15:44     ` Dominique Martinet
  2020-12-01 19:25       ` Christoph Hellwig
  2020-12-01 20:37       ` Toke Høiland-Jørgensen
  1 sibling, 2 replies; 9+ messages in thread
From: Dominique Martinet @ 2020-12-01 15:44 UTC (permalink / raw)
  To: asmadeus
  Cc: linux-kernel, v9fs-developer, linux-fsdevel,
	Toke Høiland-Jørgensen

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 <toke@redhat.com>
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs: 9p: add generic splice_write file operation
  2020-12-01 15:44     ` [PATCH] fs: 9p: add generic splice_write file operation Dominique Martinet
@ 2020-12-01 19:25       ` Christoph Hellwig
  2020-12-01 20:37       ` Toke Høiland-Jørgensen
  1 sibling, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2020-12-01 19:25 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: linux-kernel, v9fs-developer, linux-fsdevel, Toke H??iland-J??rgensen

On Tue, Dec 01, 2020 at 04:44:56PM +0100, Dominique Martinet wrote:
> 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 <toke@redhat.com>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [V9fs-developer] [PATCH] fs: 9p: add generic splice_read file operations
  2020-12-01 15:39       ` Dominique Martinet
@ 2020-12-01 20:37         ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-12-01 20:37 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Eric Van Hensbergen, Latchesar Ionkov, linux-kernel, v9fs-developer

Dominique Martinet <asmadeus@codewreck.org> writes:

> Toke Høiland-Jørgensen wrote on Tue, Dec 01, 2020:
>> > 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...
>> 
>> Huh, no idea; this is my first time digging into filesystem code, I
>> normally do networking and BPF :)
>
> In case anyone else wants to know, this broke in 5.10-rc1 with
> 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops")
>
> So really a recent regression, good catch :)

Thanks - and what a lucky coincidence that I happened upon this now so
it can be fixed before 5.10-final :)

-Toke


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs: 9p: add generic splice_write file operation
  2020-12-01 15:44     ` [PATCH] fs: 9p: add generic splice_write file operation Dominique Martinet
  2020-12-01 19:25       ` Christoph Hellwig
@ 2020-12-01 20:37       ` Toke Høiland-Jørgensen
  1 sibling, 0 replies; 9+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-12-01 20:37 UTC (permalink / raw)
  To: Dominique Martinet, asmadeus; +Cc: linux-kernel, v9fs-developer, linux-fsdevel

Dominique Martinet <asmadeus@codewreck.org> writes:

> 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 <toke@redhat.com>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>

FWIW:

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-12-01 20:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-01 13:54 [PATCH] fs: 9p: add generic splice_read file operations Toke Høiland-Jørgensen
2020-12-01 14:57 ` Dominique Martinet
2020-12-01 15:16   ` [V9fs-developer] " Dominique Martinet
2020-12-01 15:23     ` Toke Høiland-Jørgensen
2020-12-01 15:39       ` Dominique Martinet
2020-12-01 20:37         ` Toke Høiland-Jørgensen
2020-12-01 15:44     ` [PATCH] fs: 9p: add generic splice_write file operation Dominique Martinet
2020-12-01 19:25       ` Christoph Hellwig
2020-12-01 20:37       ` Toke Høiland-Jørgensen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).