linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Maxim Levitsky <mlevitsk@redhat.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Mel Gorman <mgorman@techsingularity.net>, Jan Kara <jack@suse.cz>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: Commit 'fs: Do not check if there is a fsnotify watcher on pseudo inodes' breaks chromium here
Date: Mon, 29 Jun 2020 15:09:15 +0200	[thread overview]
Message-ID: <20200629130915.GF26507@quack2.suse.cz> (raw)
In-Reply-To: <CAOQ4uxg8E-im=B6L0PQNaTTKdtxVAO=MSJki7kxq875ME4hOLw@mail.gmail.com>

On Sun 28-06-20 15:53:51, Amir Goldstein wrote:
> On Sun, Jun 28, 2020 at 2:14 PM Maxim Levitsky <mlevitsk@redhat.com> 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 :)

> 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?).

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?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  parent reply	other threads:[~2020-06-29 19:22 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-28 11:13 Commit 'fs: Do not check if there is a fsnotify watcher on pseudo inodes' breaks chromium here Maxim Levitsky
2020-06-28 12:53 ` Amir Goldstein
2020-06-28 13:14   ` Maxim Levitsky
2020-06-28 13:17     ` Maxim Levitsky
2020-06-28 13:34       ` Amir Goldstein
2020-06-29  9:31         ` Mel Gorman
2020-06-29 13:09   ` Jan Kara [this message]
2020-06-29 14:05     ` Amir Goldstein
2020-06-29 14:32       ` Mel Gorman
2020-06-29 14:41       ` [PATCH] Revert "fs: Do not check if there is a fsnotify watcher on pseudo inodes" Mel Gorman
2020-06-29 18:12         ` Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200629130915.GF26507@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=amir73il@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mgorman@techsingularity.net \
    --cc=mlevitsk@redhat.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).