Re: [RFC] fsnotify: allow sleepable child dentry flag update

[Amir Goldstein]

On Mon, Oct 17, 2022 at 8:00 PM Stephen Brennan
<stephen.s.brennan@xxxxxxxxxx> wrote:
>
> Amir Goldstein <amir73il@xxxxxxxxx> writes:
>
> > On Mon, Oct 17, 2022 at 10:59 AM Stephen Brennan
> > <stephen.s.brennan@xxxxxxxxxx> wrote:
> >>
> >> Amir Goldstein <amir73il@xxxxxxxxx> writes:
> [snip]
> >> > I think that d_find_any_alias() should be used to obtain
> >> > the alias with elevated refcount instead of the awkward
> >> > d_u.d_alias iteration loop.
> >>
> >> D'oh! Much better idea :)
> >> Do you think the BUG_ON would still be worthwhile?
> >>
> >
> > Which BUG_ON()?
> > In general no, if there are ever more multiple aliases for
> > a directory inode, updating dentry flags would be the last
> > of our problems.
>
> Sorry, I meant the one in my patch which asserts that the dentry is the
> only alias for that inode. I suppose you're right about having bigger
> problems in that case -- but the existing code "handles" it by iterating
> over the alias list.
>

It is not important IMO.

> >
> >> > In the context of __fsnotify_parent(), I think the optimization
> >> > should stick with updating the flags for the specific child dentry
> >> > that had the false positive parent_watched indication,
> >> > leaving the rest of
> >>
> >> > WOULD that address the performance issues of your workload?
> >>
> >> I think synchronizing the __fsnotify_update_child_dentry_flags() with a
> >> mutex and getting rid of the call from __fsnotify_parent() would go a
> >> *huge* way (maybe 80%?) towards resolving the performance issues we've
> >> seen. To be clear, I'm representing not one single workload, but a few
> >> different customer workloads which center around this area.
> >>
> >> There are some extreme cases I've seen, where the dentry list is so
> >> huge, that even iterating over it once with the parent dentry spinlock
> >> held is enough to trigger softlockups - no need for several calls to
> >> __fsnotify_update_child_dentry_flags() queueing up as described in the
> >> original mail. So ideally, I'd love to try make *something* work with
> >> the cursor idea as well. But I think the two ideas can be separated
> >> easily, and I can discuss with Al further about if cursors can be
> >> salvaged at all.
> >>
> >
> > Assuming that you take the dir inode_lock() in
> > __fsnotify_update_child_dentry_flags(), then I *think* that children
> > dentries cannot be added to dcache and children dentries cannot
> > turn from positive to negative and vice versa.
> >
> > Probably the only thing that can change d_subdirs is children dentries
> > being evicted from dcache(?), so I *think* that once in N children
> > if you can dget(child), drop alias->d_lock, cond_resched(),
> > and then continue d_subdirs iteration from child->d_child.
>
> This sounds like an excellent idea. I can't think of anything which
> would remove a dentry from d_subdirs without the inode lock held.
> Cursors wouldn't move without the lock held in read mode. Temporary
> dentries from d_alloc_parallel are similar - they need the inode locked
> shared in order to be removed from the parent list.
>
> I'll try implementing it (along with the fsnotify changes we've
> discussed in this thread). I'll add a BUG_ON after we wake up from
> COND_RESCHED() to guarantee that the parent is the same dentry as
> expected - just in case the assumption is wrong.

BUG_ON() is almost never a good idea.
If anything you should use if (WARN_ON_ONCE())
and break out of the loop either returning an error
to fanotify_mark() or not.
I personally think that as an unexpected code assertion
returning an error to the user is not a must in this case.

Thanks,
Amir.

>
> Al - if you've read this far :) - does this approach sound reasonable,
> compared to the cursor? I'll send out some concrete patches as soon as
> I've implemented and done a few tests on them.
>
> Thanks,
> Stephen