On Tue, Jun 27, 2023 at 09:03:17PM +0300, Amir Goldstein wrote: > 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. The if/else if/else with no goto is better than before; it was made ugly by the special-casing below. > > 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? No, all splice/tee/vmsplice cases need to generate modify events for the output fd. Really, all I/O syscalls do, but those are for today. > So either make the change that you need, or all the changes > that are simple to follow without trying to make the world > consistent Thus I also originally had all the aforementioned generate access/modify for in/out. > - 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. That's how I discovered it, yes. They aren't merged because we'd generate modify out <- from the VFS callback access in <- from do_splice modify out <- ibid. I agree this got very ugly very fast for a weird edge case ‒ maybe I did get a little over-zealous on having a consistent "one syscall↔one event for each affected file" model. OTOH: I've found that just using if (ret > 0) { fsnotify_modify(out); fsnotify_access(in); } does get the events merged from modify out <- from the VFS callback modify out <- from do_splice access in <- ibid. into modify out access in which solves all issues (reliable wake-up regardless of backing file, no spurious wake-ups) at no cost. I would've done this originally, but I hadn't known inotify events get merged :v
Attachment:
signature.asc
Description: PGP signature