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.
