From: Stephen Brennan <stephen.s.brennan@oracle.com>
To: Jan Kara <jack@suse.cz>
Cc: Jan Kara <jack@suse.cz>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
Amir Goldstein <amir73il@gmail.com>,
Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v3 0/3] fsnotify: fix softlockups iterating over d_subdirs
Date: Fri, 04 Nov 2022 16:33:18 -0700 [thread overview]
Message-ID: <87y1sqfg75.fsf@oracle.com> (raw)
In-Reply-To: <20221102175224.iacden3lq4oksmof@quack3>
Jan Kara <jack@suse.cz> writes:
> On Tue 01-11-22 13:48:54, Stephen Brennan wrote:
>> Jan Kara <jack@suse.cz> writes:
>> > Hi Stephen!
>> >
>> > On Thu 27-10-22 17:10:13, Stephen Brennan wrote:
>> >> Here is v3 of the patch series. I've taken all of the feedback,
>> >> thanks Amir, Christian, Hilf, et al. Differences are noted in each
>> >> patch.
>> >>
>> >> I caught an obvious and silly dentry reference leak: d_find_any_alias()
>> >> returns a reference, which I never called dput() on. With that change, I
>> >> no longer see the rpc_pipefs issue, but I do think I need more testing
>> >> and thinking through the third patch. Al, I'd love your feedback on that
>> >> one especially.
>> >>
>> >> Thanks,
>> >> Stephen
>> >>
>> >> Stephen Brennan (3):
>> >> fsnotify: Use d_find_any_alias to get dentry associated with inode
>> >> fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem
>> >> fsnotify: allow sleepable child flag update
>> >
>> > Thanks for the patches Stephen and I'm sorry for replying somewhat late.
>>
>> Absolutely no worries, these things take time. Thanks for taking a look!
>>
>> > The first patch is a nobrainer. The other two patches ... complicate things
>> > somewhat more complicated than I'd like. I guess I can live with them if we
>> > don't find a better solution but I'd like to discuss a bit more about
>> > alternatives.
>>
>> Understood!
>>
>> > So what would happen if we just clear DCACHE_FSNOTIFY_PARENT_WATCHED in
>> > __fsnotify_parent() for the dentry which triggered the event and does not
>> > have watched parent anymore and never bother with full children walk? I
>> > suppose your contention problems will be gone, we'll just pay the price of
>> > dget_parent() + fsnotify_inode_watches_children() for each child that
>> > falsely triggers instead of for only one. Maybe that's not too bad? After
>> > all any event upto this moment triggered this overhead as well...
>>
>> This is an interesting idea. It came across my mind but I don't think I
>> considered it seriously because I assumed that it was too big a change.
>> But I suppose in the process I created an even bigger change :P
>>
>> The false positive dget_parent() + fsnotify_inode_watches_children()
>> shouldn't be too bad. I could see a situation where there's a lot of
>> random accesses within a directory, where the dget_parent() could cause
>> some contention over the parent dentry. But to be fair, the performance
>> would have been the same or worse while fsnotify was active in that
>> case, and the contention would go away as most of the dentries get their
>> flags cleared. So I don't think this is a problem.
>>
>> > Am I missing something?
>>
>> I think there's one thing missed here. I understand you'd like to get
>> rid of the extra flag in the connector. But the advantage of the flag is
>> avoiding duplicate work by saving a bit of state. Suppose that a mark is
>> added to a connector, which causes fsnotify_inode_watches_children() to
>> become true. Then, any subsequent call to fsnotify_recalc_mask() must
>> call __fsnotify_update_child_dentry_flags(), even though the child
>> dentry flags don't need to be updated: they're already set. For (very)
>> large directories, this can take a few seconds, which means that we're
>> doing a few extra seconds of work each time a new mark is added to or
>> removed from a connector in that case. I can't imagine that's a super
>> common workload though, and I don't know if my customers do that (my
>> guess would be no).
>
> I understand. This basically matters for fsnotify_recalc_mask(). As a side
> note I've realized that your changes to fsnotify_recalc_mask() acquiring
> inode->i_rwsem for updating dentry flags in patch 2/3 are problematic for
> dnotify because that calls fsnotify_recalc_mask() under a spinlock.
> Furthermore it is somewhat worrying also for inotify & fanotify because it
> nests inode->i_rwsem inside fsnotify_group->lock however I'm not 100% sure
> something doesn't force the ordering the other way around (e.g. the removal
> of oneshot mark during modify event generation). Did you run tests with
> lockdep enabled?
No I didn't. I'll be sure to get the LTP tests running with lockdep
early next week and try this series out, we'll probably get an error
like you say.
I'll also take a look at the dnotify use case and see if there's
anything to do there. Hopefully there's something we can do to salvage
it :D
Thanks,
Stephen
> Anyway, if the lock ordering issues can be solved, I suppose we can
> optimize fsnotify_recalc_mask() like:
>
> inode_lock(inode);
> spin_lock(&conn->lock);
> oldmask = inode->i_fsnotify_mask;
> __fsnotify_recalc_mask(conn);
> newmask = inode->i_fsnotify_mask;
> spin_unlock(&conn->lock);
> if (watching children changed(oldmask, newmask))
> __fsnotify_update_child_dentry_flags(...)
> inode_unlock(inode);
>
> And because everything is serialized by inode_lock, we don't have to worry
> about inode->i_fsnotify_mask and dentry flags getting out of sync or some
> mark addition returning before all children are marked for reporting
> events. No need for the connector flag AFAICT.
>
> But the locking issue needs to be resolved first in any case. I need to
> think some more...
>
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
next prev parent reply other threads:[~2022-11-04 23:33 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-13 22:27 [RFC] fsnotify: allow sleepable child dentry flag update Stephen Brennan
2022-10-13 23:51 ` Al Viro
2022-11-01 21:47 ` Stephen Brennan
2022-10-14 8:01 ` Amir Goldstein
2022-10-17 7:59 ` Stephen Brennan
2022-10-17 11:44 ` Amir Goldstein
2022-10-17 16:59 ` Stephen Brennan
2022-10-17 17:42 ` Amir Goldstein
2022-10-17 9:09 ` Jan Kara
2022-10-18 4:12 ` [PATCH 0/2] fsnotify: fix softlockups iterating over d_subdirs Stephen Brennan
2022-10-18 4:12 ` [PATCH 1/2] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem Stephen Brennan
2022-10-18 7:39 ` Amir Goldstein
2022-10-21 0:33 ` Stephen Brennan
2022-10-21 7:22 ` Amir Goldstein
2022-10-18 4:12 ` [PATCH 2/2] fsnotify: allow sleepable child flag update Stephen Brennan
2022-10-18 5:36 ` Amir Goldstein
2022-10-27 7:50 ` kernel test robot
2022-10-27 8:44 ` Yujie Liu
2022-10-27 22:12 ` Stephen Brennan
2022-10-18 8:07 ` [PATCH 0/2] fsnotify: fix softlockups iterating over d_subdirs Amir Goldstein
2022-10-18 23:52 ` Stephen Brennan
2022-10-19 5:33 ` Amir Goldstein
2022-10-27 22:06 ` Stephen Brennan
2022-10-28 8:58 ` Amir Goldstein
2022-10-21 1:03 ` [PATCH v2 0/3] " Stephen Brennan
2022-10-21 1:03 ` [PATCH v2 1/3] fsnotify: Use d_find_any_alias to get dentry associated with inode Stephen Brennan
2022-10-21 9:25 ` Amir Goldstein
2022-10-21 1:03 ` [PATCH v2 2/3] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem Stephen Brennan
2022-10-21 4:01 ` kernel test robot
2022-10-21 8:22 ` Amir Goldstein
2022-10-21 9:18 ` Amir Goldstein
2022-10-25 18:02 ` Stephen Brennan
2022-10-26 5:41 ` Amir Goldstein
2022-10-21 9:17 ` Christian Brauner
2022-10-21 9:21 ` Amir Goldstein
2022-10-21 1:03 ` [PATCH v2 3/3] fsnotify: allow sleepable child flag update Stephen Brennan
2022-10-28 0:10 ` [PATCH v3 0/3] fsnotify: fix softlockups iterating over d_subdirs Stephen Brennan
2022-10-28 0:10 ` [PATCH v3 1/3] fsnotify: Use d_find_any_alias to get dentry associated with inode Stephen Brennan
2022-11-10 1:12 ` Stephen Brennan
2022-10-28 0:10 ` [PATCH v3 2/3] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem Stephen Brennan
2022-10-28 9:11 ` Amir Goldstein
2022-11-10 0:03 ` kernel test robot
2022-11-10 1:06 ` Stephen Brennan
2022-10-28 0:10 ` [PATCH v3 3/3] fsnotify: allow sleepable child flag update Stephen Brennan
2022-10-28 9:32 ` Amir Goldstein
2022-11-01 21:25 ` Stephen Brennan
2022-11-01 17:51 ` [PATCH v3 0/3] fsnotify: fix softlockups iterating over d_subdirs Jan Kara
2022-11-01 20:48 ` Stephen Brennan
2022-11-02 8:55 ` Amir Goldstein
2022-11-10 20:04 ` Stephen Brennan
2022-11-02 17:52 ` Jan Kara
2022-11-04 23:33 ` Stephen Brennan [this message]
2022-11-07 11:56 ` Jan Kara
2022-11-11 22:06 ` [PATCH v4 0/5] " Stephen Brennan
2022-11-11 22:06 ` [PATCH v4 1/5] fsnotify: clear PARENT_WATCHED flags lazily Stephen Brennan
2022-11-11 22:06 ` [PATCH v4 2/5] fsnotify: Use d_find_any_alias to get dentry associated with inode Stephen Brennan
2022-11-12 8:53 ` Amir Goldstein
2022-11-11 22:06 ` [PATCH v4 3/5] dnotify: move fsnotify_recalc_mask() outside spinlock Stephen Brennan
2022-11-12 9:06 ` Amir Goldstein
2022-11-11 22:06 ` [PATCH v4 4/5] fsnotify: allow sleepable child flag update Stephen Brennan
2022-11-12 10:00 ` Amir Goldstein
2022-11-15 7:10 ` kernel test robot
2022-11-11 22:06 ` [PATCH v4 5/5] fsnotify: require inode lock held during " Stephen Brennan
2022-11-12 9:42 ` Amir Goldstein
2022-11-11 22:08 ` [PATCH v4 0/5] fsnotify: fix softlockups iterating over d_subdirs Stephen Brennan
2022-11-22 11:50 ` Jan Kara
2022-11-22 14:03 ` Amir Goldstein
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y1sqfg75.fsf@oracle.com \
--to=stephen.s.brennan@oracle.com \
--cc=amir73il@gmail.com \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).