On Tue, Jun 27, 2023 at 7:55 PM Ahelenia Ziemiańska
<nabijaczleweli@xxxxxxxxxxxxxxxxxx> wrote:
>
> In 1/3 I've applied if/else if/else tree like you said,
> and expounded a bit in the message.
>
> This is less pretty now, however, since it turns out that
If my advice turns out to be bad, then please drop it.
> iter_file_splice_write() already marks the out fd as written because it
> writes to it via vfs_iter_write(), and that sent a double notification.
>
> $ git grep -F .splice_write | grep -v iter_file_splice_write
> drivers/char/mem.c: .splice_write = splice_write_null,
> drivers/char/virtio_console.c: .splice_write = port_fops_splice_write,
> fs/fuse/dev.c: .splice_write = fuse_dev_splice_write,
> fs/gfs2/file.c: .splice_write = gfs2_file_splice_write,
> fs/gfs2/file.c: .splice_write = gfs2_file_splice_write,
> fs/overlayfs/file.c: .splice_write = ovl_splice_write,
> net/socket.c: .splice_write = generic_splice_sendpage,
> scripts/coccinelle/api/stream_open.cocci: .splice_write = splice_write_f,
>
> Of these, splice_write_null() doesn't mark out as written
> (but it's for /dev/null so I think this is expected),
> and I haven't been able to visually confirm whether
> port_fops_splice_write() and generic_splice_sendpage() do.
>
> All the others delegate to iter_file_splice_write().
>
All this is very troubling to me.
It translates to a mental model that I cannot remember and
cannot maintain for fixes whose value are still questionable.
IIUC, the only thing you need to change in do_splice() for
making your use case work is to add fsnotify_modify()
for the splice_pipe_to_pipe() case. Right?
So either make the change that you need, or all the changes
that are simple to follow without trying to make the world
consistent - these pipe iterators business is really messy.
I don't know if avoiding a double event (which is likely not visible)
is worth the complicated code that is hard to understand.
> In 2/3 I fixed the vmsplice notification placement
> (access from pipe, modify to pipe).
>
> I'm following this up with an LTP patch, where only sendfile_file_to_pipe
> passes on 6.1.27-1 and all tests pass on v6.4 + this patchset.
>
Were these tests able to detect the double event?
Maybe it's not visible because double consequent events get merged.
> Ahelenia Ziemiańska (3):
> splice: always fsnotify_access(in), fsnotify_modify(out) on success
> splice: fsnotify_access(fd)/fsnotify_modify(fd) in vmsplice
> splice: fsnotify_access(in), fsnotify_modify(out) on success in tee
>
> fs/splice.c | 43 +++++++++++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 18 deletions(-)
>
>
> Interdiff against v2:
> diff --git a/fs/splice.c b/fs/splice.c
> index 3234aaa6e957..0427f0a91c7d 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1155,10 +1155,7 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
> flags |= SPLICE_F_NONBLOCK;
>
> ret = splice_pipe_to_pipe(ipipe, opipe, len, flags);
> - goto notify;
> - }
> -
> - if (ipipe) {
> + } else if (ipipe) {
> if (off_in)
> return -ESPIPE;
> if (off_out) {
> @@ -1188,10 +1185,10 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
> else
> *off_out = offset;
>
> - goto notify;
> - }
> -
> - if (opipe) {
> + // ->splice_write already marked out
> + // as modified via vfs_iter_write()
> + goto noaccessout;
That's too ugly IMO.
Are you claiming that the code in master is wrong?
Because in master there is fsnotify_modify(out) for (ipipe) case.
> + } else if (opipe) {
> if (off_out)
> return -ESPIPE;
> if (off_in) {
> @@ -1211,17 +1208,14 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
> in->f_pos = offset;
> else
> *off_in = offset;
> + } else
> + return -EINVAL;
>
> - goto notify;
> - }
> -
> - return -EINVAL;
> -
> -notify:
> - if (ret > 0) {
> - fsnotify_access(in);
> + if (ret > 0)
> fsnotify_modify(out);
> - }
> +noaccessout:
> + if (ret > 0)
> + fsnotify_access(in);
>
Not to mention that it should be nomodifyout, but I dislike this
"common" code that it not common at all, so either just handle
the pipe_to_pipe case to fix your use case or leave this code
completely common ignoring the possible double events.
Thanks,
Amir.
