Re: [PATCH v2 0/2] fsnotify: drop SRCU across inotify event delivery

[Jan Kara]

Hello!

On Thu 08-10-26 20:02:37, Jia Zhu wrote:
> We observed fsnotify teardown stalls in production. NMI backtraces
> showed inotify_handle_inode_event() in memcg reclaim, and the mark
> reaper, inotifywait and a systemd process were reported blocked for more
> than 327 seconds.
> 
> We checked upstream and found the same issue: inotify allocates events
> under fsnotify_mark_srcu, and the global mark reaper waits in
> synchronize_srcu().

Thanks for the details. Well, it isn't only inotify. fanotify also
allocates event structure for each notification group when the event is
generated. But when you run setup that can stall for over 5 minutes in
direct reclaim I wouldn't really say it's fsnotify that's broken :) In
general this seems like a classical case of priority inversion where some
heavily throttled task holds a resource other tasks need and so they block
behind. MM guys are actually looking into these issues (e.g. it was
discussed at LPC MM microconference last week [1]). Adding Shakeel to CC.

> The production report shows multiple inotify release paths blocked
> behind the global mark reaper. Because SRCU and the reaper are shared
> across groups, a stalled delivery can delay other groups' close() and
> exit paths.
> 
> Patch 1 pins the iterator marks and drops SRCU around inotify delivery.
> It retains fanotify's pinning semantics and preserves event delivery to
> a surviving inotify watch when another watch is removed. This leaves the
> reclaim stall itself unchanged.
 
This is going to noticeably slow down event delivery so it isn't really
acceptable in this form. I'd actually prefer if these cgroup priority
inversion issues got somehow dealt with in MM or with its better support so
that we don't have to add complexity to fsnotify for this. Also note that
even through we can drop SRCU like you do, the task that is doing the
operation on the filesystem will be still blocked until the allocation
succeeds and will be holding filesystem locks. So although your change will
address the particular stalls you observe, other types of stalls remain
unaddressed (which is another reason why I dislike your solution).

> Patch 2 skips the global reaper flush when only the closing group
> reference remains. As Jan noted on the earlier patch [1], this only
> helps groups with no outstanding marks. It is optional and can be
> dropped.
> 
> The teardown fast path was checked in QEMU with a test-only SRCU holder.
> 
> [1] https://lore.kernel.org/all/4467cbj7eivubtdwcawvr7mhrb7armgoc4xfqjjnncrbkxxag6@3mmou6nhviao/
> 
> Jia Zhu (2):
>   inotify: drop fsnotify SRCU protection during event delivery
>   fsnotify: avoid unrelated mark reaper waits during group teardown
> 
>  fs/notify/fsnotify.c             | 15 ++++++++++++---
>  fs/notify/group.c                | 26 +++++++++++++++-----------
>  fs/notify/inotify/inotify_user.c |  3 ++-
>  fs/notify/mark.c                 | 26 ++++++++++++++++++++++----
>  include/linux/fsnotify_backend.h |  2 ++
>  5 files changed, 53 insertions(+), 19 deletions(-)
> 
> 
> base-commit: df2908090cda368b01ff43709f51890076c56157

								Honza

[1] https://lpc.events/event/20/contributions/2418/
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR