Re: Commit 'fs: Do not check if there is a fsnotify watcher on pseudo inodes' breaks chromium here

[Amir Goldstein]

On Mon, Jun 29, 2020 at 4:09 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Sun 28-06-20 15:53:51, Amir Goldstein wrote:
> > On Sun, Jun 28, 2020 at 2:14 PM Maxim Levitsky <mlevitsk@xxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > I just did usual kernel update and now chromium crashes on startup.
> > > It happens both in a KVM's VM (with virtio-gpu if that matters) and natively with amdgpu driver.
> > > Most likely not GPU related although I initially suspected that it is.
> > >
> > > Chromium starts as a white rectangle, shows few white rectangles
> > > that resemble its notifications and then crashes.
> > >
> > > The stdout output from chromium:
> >
> > I guess this answers our question whether we could disable fsnoitfy
> > watches on pseudo inodes....
>
> Right :-|
>
> > From comments like these in chromium code:
> > https://chromium.googlesource.com/chromium/src/+/master/mojo/core/watcher_dispatcher.cc#77
> > https://chromium.googlesource.com/chromium/src/+/master/base/files/file_descriptor_watcher_posix.cc#176
> > https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_mojo.cc#240
> >
> > I am taking a wild guess that the missing FS_CLOSE event on anonymous pipes is
> > the cause for regression.
>
> I was checking the Chromium code for some time. It uses inotify in
> base/files/file_path_watcher_linux.cc and watches IN_CLOSE_WRITE event
> (among other ones) but I was unable to track down how the class gets
> connected to the mojo class that crashes. I'd be somewhat curious how they
> place inotify watches on pipe inodes - probably they have to utilize proc
> magic links but I'd like to be sure. Anyway your guess appears to be
> correct :)

Well, I lost track of the code as well...

>
> > The motivation for the patch "fs: Do not check if there is a fsnotify
> > watcher on pseudo inodes"
> > was performance, but actually, FS_CLOSE and FS_OPEN events probably do
> > not impact performance as FS_MODIFY and FS_ACCESS.
>
> Correct.
>
> > Do you agree that dropping FS_MODIFY/FS_ACCESS events for FMODE_STREAM
> > files as a general rule should be safe?
>
> Hum, so your patch drops FS_MODIFY/FS_ACCESS events also for named pipes
> compared to the original patch AFAIU and for those fsnotify works fine
> so far. So I'm not sure we won't regress someone else with this.
>
> I've also tested inotify on a sample pipe like: cat /dev/stdin | tee
> and watched /proc/<tee pid>/fd/0 and it actually generated IN_MODIFY |
> IN_ACCESS when data arrived to a pipe and tee(1) read it and then
> IN_CLOSE_WRITE | IN_CLOSE_NOWRITE when the pipe got closed (I thought you
> mentioned modify and access events didn't get properly generated?).

I don't think that I did (did I?)

>
> So as much as I agree that some fsnotify events on FMODE_STREAM files are
> dubious, they could get used (possibly accidentally) and so after this
> Chromium experience I think we just have to revert the change and live with
> generating notification events for pipes to avoid userspace regressions.
>
> Thoughts?

I am fine with that.

Before I thought of trying out FMODE_STREAM I was considering to propose
to set the new flag FMODE_NOIONOTIFY in alloc_file_pseudo() to narrow Mel's
patch to dropping FS_MODIFY|FS_ACCESS.

But I guess the burden of proof is back on Mel.
And besides, quoting Mel's patch:
"A patch is pending that reduces, but does not eliminate, the overhead of
    fsnotify but for files that cannot be looked up via a path, even that
    small overhead is unnecessary"

So really, we are not even sacrificing much by reverting this patch.
We down to "nano optimizations".

Thanks,
Amir.