On Thu, Sep 11, 2025 at 12:12 PM Jan Kara <jack@xxxxxxx> wrote:
> > + scoped_guard(spinlock, &group->notification_lock) {
>
> Frankly, I don't see the scoped guard bringing benefit here. It just shifts
> indentation level by 1 which makes some of the lines below longer than I
> like :)
No strong arguments. The scoped guard is safe against goto/return,
but there are none here, so...
> > + pr_warn_ratelimited("PID %u (%s) failed to respond to fanotify queue for more than %i seconds\n",
>
> Use %d instead of %i here? IMHO we use %d everywhere in the kernel. I had
> to look up whether %i is really signed int.
Fine. I haven't noticed that convention yet, will keep it in mind.
> > + event->recv_pid, task ? task->comm : NULL, perm_group_timeout);
> > + rcu_read_unlock();
> > + }
> > + }
>
> I'm wondering if we should cond_resched() somewhere in these loops. There
> could be *many* events pending... OTOH continuing the iteration afterwards
> would be non-trivial so probably let's keep our fingers crossed that
> softlockups won't trigger...
Yeah, I think this won't be an issue in practice because
a) the loop is very tight for the first iteration case (which is the likely one)
b) my gut feel about the number of perf events generated and being
simultaneously processed by the server should normally be less than a
hundred.
> > +static void fanotify_perm_watchdog_group_remove(struct fsnotify_group *group)
> > +{
> > + if (!list_empty(&group->fanotify_data.perm_group)) {
> > + /* Perm event watchdog can no longer scan this group. */
> > + spin_lock(&perm_group_lock);
> > + list_del(&group->fanotify_data.perm_group);
>
> list_del_init() here would give me a better peace of mind... It's not like
> the performance matters here.
Agreed.
> > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > index d4034ddaf392..7f7fe4f3aa34 100644
> > --- a/include/linux/fsnotify_backend.h
> > +++ b/include/linux/fsnotify_backend.h
> > @@ -273,6 +273,8 @@ struct fsnotify_group {
> > int f_flags; /* event_f_flags from fanotify_init() */
> > struct ucounts *ucounts;
> > mempool_t error_events_pool;
> > + /* chained on perm_group_list */
> > + struct list_head perm_group;
>
> Can we call this perm_group_list, perm_list or simply something with 'list'
> in the name, please? We follow this naming convention throughout the
> fsnotify subsystem.
Okay.
Thanks,
Miklos
