Bug 225934

Comment 1 Mark Johnston 2020-04-28 14:03:09 UTC

Indeed, at the end of fifo_open() we set the file's ops table to pipeops.  pipe_kqfilter() has no way of attaching a knote to the fifo vnode.  We could perhaps handle it with a new file ops table for fifos: every operation except kqfilter would just be the corresponding operation in pipeops.  fifo_kqfilter() would attempt to handle EVFILT_VNODE by calling vfs_kqfilter() and otherwise would call pipe_kqfilter().

Another option might be to maintain a backpointer from the pipe structure to the vnode for named pipes.

BTW there is some dead code here, I can't see how ufsfifo_kqfilter() can ever get called.

I've seen some places in sys_pipe.c where operations are delegated to vnode operations (in the fifo case). Could something like the following work?


--- a/sys/kern/sys_pipe.c
+++ b/sys/kern/sys_pipe.c
@@ -1707,6 +1707,10 @@ pipe_kqfilter(struct file *fp, struct knote *kn)
                cpipe = PIPE_PEER(cpipe);
                break;
        default:
+               if (cpipe->pipe_state & PIPE_NAMED) {
+                       PIPE_UNLOCK(cpipe);
+                       return (vnops.fo_kqfilter(fp, kn));
+               }
                PIPE_UNLOCK(cpipe);
                return (EINVAL);
        }


Maybe vop_kqfilter/VOP_KQFILTER could be removed completely in favor of vfs_kqfilter() as the only implementation? It looks like this mechanism was only needed to forward *_kqfilter calls to fifo's vop_kqfilter. But now it's done the other way around in a sense.

Comment 3 Mark Johnston 2020-04-28 15:36:00 UTC

(In reply to Jan Kokemüller from comment #2)
I think that would work, and is better than what I had in mind.

I think we should at least remove the VOP_KQFILTER implementations for fifos across the various filesystems, they're just dead code.  I know of at least one out-of-tree filesystem that implements VOP_KQFILTER though, so I'd be inclined to keep the VOP itself.

Comment 4 commit-hook 2021-10-03 06:03:46 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6b88668f0bfcca09035549e25d0f3c181cd537c8

commit 6b88668f0bfcca09035549e25d0f3c181cd537c8
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2021-10-02 05:17:57 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2021-10-03 06:02:51 +0000

    vfs: remove dead fifoop VOP_KQFILTER implementations

    These began to become obsolete in d6d64f0f2c26 (r137739) and the deal
    was later sealed in 003e18aef4c4 (r137801) when vfs.fifofs.fops was
    dropped and vop-bypass for pipes became mandatory.

    PR:             225934
    Suggested by:   markj
    Reviewe by:     kib, markj
    Differential Revision:  https://reviews.freebsd.org/D32270

 sys/fs/ext2fs/ext2_vnops.c | 18 ------------------
 sys/ufs/ufs/ufs_vnops.c    | 19 -------------------
 2 files changed, 37 deletions(-)

Comment 5 commit-hook 2021-10-09 00:57:59 UTC

A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=48e109cf31e1e710328f5750fc683daf058ccc19

commit 48e109cf31e1e710328f5750fc683daf058ccc19
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2021-10-02 05:17:57 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2021-10-09 00:56:46 +0000

    vfs: remove dead fifoop VOP_KQFILTER implementations

    These began to become obsolete in d6d64f0f2c26 (r137739) and the deal
    was later sealed in 003e18aef4c4 (r137801) when vfs.fifofs.fops was
    dropped and vop-bypass for pipes became mandatory.

    PR:             225934

    (cherry picked from commit 6b88668f0bfcca09035549e25d0f3c181cd537c8)

 sys/fs/ext2fs/ext2_vnops.c | 18 ------------------
 sys/ufs/ufs/ufs_vnops.c    | 19 -------------------
 2 files changed, 37 deletions(-)

Comment 6 commit-hook 2021-10-12 07:44:50 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=7259ca31048e5ced8e7f90657a3d7084aeafdf51

commit 7259ca31048e5ced8e7f90657a3d7084aeafdf51
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2021-10-02 05:23:03 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2021-10-12 07:43:07 +0000

    fifos: delegate unhandled kqueue filters to underlying filesystem

    This gives the vfs layer a chance to provide handling for EVFILT_VNODE,
    for instance.  Change pipe_specops to use the default vop_kqfilter to
    accommodate fifoops that don't specify the method (i.e. all in-tree).

    Based on a patch by Jan Kokemüller.

    PR:             225934
    Reviewed by:    kib, markj (both pre-KASSERT)
    Differential Revision:  https://reviews.freebsd.org/D32271

 sys/fs/fifofs/fifo_vnops.c          |  1 -
 sys/kern/sys_pipe.c                 |  4 +++
 sys/kern/vfs_subr.c                 |  3 ++
 tests/sys/kqueue/libkqueue/common.h |  2 ++
 tests/sys/kqueue/libkqueue/vnode.c  | 59 +++++++++++++++++++++++++++++++++++++
 5 files changed, 68 insertions(+), 1 deletion(-)

Comment 7 Graham Perrin 2022-10-23 03:10:40 UTC

^Triage: assignment to a committer of related commits. 

(In reply to commit-hook from comment #6)

For 7259ca31048e5ced8e7f90657a3d7084aeafdf51 (D32271), are merges from current applicable?

Comment 8 Mark Linimon 2024-01-02 03:42:35 UTC

^Triage: the 12 branch is now out of support.