Re: [PATCH v4 0/3] fanotify accounting for fs/splice.c

[Jan Kara]

Hi!

On Wed 28-06-23 20:54:28, Ahelenia Ziemiańska wrote:
> On Wed, Jun 28, 2023 at 01:38:53PM +0200, Jan Kara wrote:
> > On Tue 27-06-23 22:50:46, Ahelenia Ziemiańska wrote:
> > > Always generate modify out, access in for splice;
> > > this gets automatically merged with no ugly special cases.
> > > 
> > > No changes to 2/3 or 3/3.
> > Thanks for the patches Ahelena! The code looks fine to me but to be honest
> > I still have one unresolved question so let me think about it loud here for
> > documentation purposes :). Do we want fsnotify (any filesystem
> > notification framework like inotify or fanotify) to actually generate
> > events on FIFOs? FIFOs are virtual objects and are not part of the
> > filesystem as such (well, the inode itself and the name is), hence
> > *filesystem* notification framework does not seem like a great fit to watch
> > for changes or accesses there. And if we say "yes" for FIFOs, then why not
> > AF_UNIX sockets? Where do we draw the line? And is it all worth the
> > trouble?
> As a relative outsider (I haven't used inotify before this, and have not
>  been subjected to it or its peripheries before),
> I interpreted inotify as being the Correct solution for:
>   1. stuff you can find in a normal
>      (non-/dev, you don't want to touch devices)
>      filesystem traversal
>   2. stuff you can open
> where, going down the list in inode(7):
>   S_IFSOCK   can't open
>   S_IFLNK    can't open
>   S_IFREG    yes!
>   S_IFBLK    it's a device
>   S_IFDIR    yes!
>   S_IFCHR    it's a device
>   S_IFIFO    yes!
> 
> It appears that I'm not the only one who's interpreted it that way,
> especially since neither regular files nor pipes are pollable.
> (Though, under that same categorisation, I wouldn't be surprised
>  if anonymous pipes had been refused, for example, since those are
>  conventionally unnameable.)
> 
> To this end, I'd say we're leaving the line precisely where it was drawn
> before, even if by accident.

I agree, although I'd note that there are S_IFREG inodes under /sys or
/proc where it would be too difficult to provide fsnotify events (exactly
because the file contents is not "data stored somewhere" but rather
something "generated on the fly") so the illusion is not perfect already.

> > I understand the convenience of inotify working on FIFOs for the "tail -f"
> > usecase but then wouldn't this better be fixed in tail(1) itself by using
> > epoll(7) for FIFOs which, as I've noted in my other reply, does not have
> > the problem that poll(2) has when there are no writers?
> Yes, epoll in ET mode returns POLLHUP only once, but you /also/ need the
> inotify anyway for regular files, which epoll refuses
> (and, with -F, you may want both epoll for a pipe and inotify for the
>  directory it's contained in).
> Is it possible to do? yes. Is it more annoying than just having pipes
> report when they were written to? very much so.
> 
> inotify actually working(*) is presumably why coreutils tail doesn't use
> epoll ‒ inotify already provides all required events(*), you can use the
> same code for regular files and fifos, and with one fewer level of
> indirection: there's just no need(*).
> 
> (*: except with a magic syscall only I use apparently)

Yeah, I've slept to this and I still think adding fsnotify events to splice
is a nicer option so feel free to add:

Acked-by: Jan Kara <jack@xxxxxxx>

to all kernel patches in your series. Since the changes are in splice code,
Christian or Al Viro (who you already have on CC list) should be merging
this so please make sure to also include them in the v5 submission.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR