Re: [PATCH RFC] nfsd: avoid recursive locking through fsnotify

[Jan Kara <jack@suse.cz>]

On Thu 24-03-22 21:17:09, Amir Goldstein wrote:
> On Wed, Mar 23, 2022 at 5:46 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Mar 23, 2022 at 4:28 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 23-03-22 16:00:30, Amir Goldstein wrote:
> > > > > Well, but reclaim from kswapd is always the main and preferred source of
> > > > > memory reclaim. And we will kick kswapd to do work if we are running out of
> > > > > memory. Doing direct filesystem slab reclaim from mark allocation is useful
> > > > > only to throttle possibly aggressive mark allocations to the speed of
> > > > > reclaim (instead of getting ENOMEM). So I'm still not convinced this is a
> > > > > big issue but I certainly won't stop you from implementing more fine
> > > > > grained GFP mode selection and lockdep annotations if you want to go that
> > > > > way :).
> > > >
> > > > Well it was just two lines of code to annotate the fanotify mutex as its own
> > > > class, so I just did that:
> > > >
> > > > https://github.com/amir73il/linux/commit/7b4b6e2c0bd1942cd130e9202c4b187a8fb468c6
> > >
> > > But this implicitely assumes there isn't any allocation under mark_mutex
> > > anywhere else where it is held. Which is likely true (I didn't check) but
> > > it is kind of fragile. So I was rather imagining we would have per-group
> > > "NOFS" flag and fsnotify_group_lock/unlock() would call
> > > memalloc_nofs_save() based on the flag. And we would use
> > > fsnotify_group_lock/unlock() uniformly across the whole fsnotify codebase.
> > >
> >
> > I see what you mean, but looking at the code it seems quite a bit of churn to go
> > over all the old backends and convert the locks to use wrappers where we "know"
> > those backends are fs reclaim safe (because we did not get reports of deadlocks
> > over the decades they existed).
> >
> > I think I will sleep better with a conversion to three flavors:
> >
> > 1. pflags = fsnotify_group_nofs_lock(fanotify_group);
> > 2. fsnotify_group_lock(dnotify_group) =>
> >     WARN_ON_ONCE(group->flags & FSNOTIFY_NOFS)
> >     mutex_lock(&group->mark_mutex)
> > 3. fsnotify_group_lock_nested(group) =>
> >     mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING)
> >
> 
> I think I might have misunderstood you and you meant that the
> SINGLE_DEPTH_NESTING subcalls should be eliminated and then
> we are left with two lock classes.
> Correct?

Yeah, at least at this point I don't see a good reason for using
SINGLE_DEPTH_NESTING lockdep annotation. In my opinion it has just a
potential of silencing reports of real locking problems. So removing it and
seeing what complains would be IMO a way to go.

								Honza

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