From: Jan Kara <jack@suse.cz>
To: Eric Sandeen <sandeen@sandeen.net>
Cc: Jan Kara <jack@suse.cz>, Eric Sandeen <sandeen@redhat.com>,
fsdevel <linux-fsdevel@vger.kernel.org>,
Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH V2] fs: avoid softlockups in s_inodes iterators
Date: Wed, 16 Oct 2019 15:49:45 +0200 [thread overview]
Message-ID: <20191016134945.GD7198@quack2.suse.cz> (raw)
In-Reply-To: <3a175c93-d7b2-5afb-fc2c-69951eb17838@sandeen.net>
On Wed 16-10-19 08:23:51, Eric Sandeen wrote:
> On 10/16/19 4:42 AM, Jan Kara wrote:
> > On Tue 15-10-19 21:36:08, Eric Sandeen wrote:
> >> On 10/15/19 2:37 AM, Jan Kara wrote:
> >>> On Mon 14-10-19 16:30:24, Eric Sandeen wrote:
> >>>> Anything that walks all inodes on sb->s_inodes list without rescheduling
> >>>> risks softlockups.
> >>>>
> >>>> Previous efforts were made in 2 functions, see:
> >>>>
> >>>> c27d82f fs/drop_caches.c: avoid softlockups in drop_pagecache_sb()
> >>>> ac05fbb inode: don't softlockup when evicting inodes
> >>>>
> >>>> but there hasn't been an audit of all walkers, so do that now. This
> >>>> also consistently moves the cond_resched() calls to the bottom of each
> >>>> loop in cases where it already exists.
> >>>>
> >>>> One loop remains: remove_dquot_ref(), because I'm not quite sure how
> >>>> to deal with that one w/o taking the i_lock.
> >>>>
> >>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >>>
> >>> Thanks Eric. The patch looks good to me. You can add:
> >>>
> >>> Reviewed-by: Jan Kara <jack@suse.cz>
> >>
> >> thanks
> >>
> >>> BTW, I suppose you need to add Al to pickup the patch?
> >>
> >> Yeah (cc'd now)
> >>
> >> But it was just pointed out to me that if/when the majority of inodes
> >> at umount time have i_count == 0, we'll never hit the resched in
> >> fsnotify_unmount_inodes() and may still have an issue ...
> >
> > Yeah, that's a good point. So that loop will need some further tweaking
> > (like doing iget-iput dance in need_resched() case like in some other
> > places).
>
> Well, it's already got an iget/iput for anything with i_count > 0. But
> as the comment says (and I think it's right...) doing an iget/iput
> on i_count == 0 inodes at this point would be without SB_ACTIVE and the final
> iput here would actually start evicting inodes in /this/ loop, right?
Yes, it would but since this is just before calling evict_inodes(), I have
currently hard time remembering why evicting inodes like that would be an
issue.
> I think we could (ab)use the lru list to construct a "dispose" list for
> fsnotify processing as was done in evict_inodes...
>
> or maybe the two should be merged, and fsnotify watches could be handled
> directly in evict_inodes. But that doesn't feel quite right.
Merging the two would be possible (and faster!) as well but I agree it
feels a bit dirty :)
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2019-10-16 13:49 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-14 21:30 [PATCH V2] fs: avoid softlockups in s_inodes iterators Eric Sandeen
2019-10-14 21:36 ` Eric Sandeen
2019-10-15 7:37 ` Jan Kara
2019-10-16 2:36 ` Eric Sandeen
2019-10-16 9:42 ` Jan Kara
2019-10-16 13:23 ` Eric Sandeen
2019-10-16 13:49 ` Jan Kara [this message]
2019-10-16 14:39 ` Eric Sandeen
2019-10-16 15:26 ` Eric Sandeen
2019-10-16 15:35 ` Jan Kara
2019-10-16 17:11 ` [PATCH 2/1] fs: call fsnotify_sb_delete after evict_inodes Eric Sandeen
2019-10-17 8:39 ` Jan Kara
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=20191016134945.GD7198@quack2.suse.cz \
--to=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=sandeen@redhat.com \
--cc=sandeen@sandeen.net \
--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).