From: Ian Kent <raven@themaw.net> To: NeilBrown <neilb@suse.com>, Al Viro <viro@ZenIV.linux.org.uk> Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>, Takashi Iwai <tiwai@suse.de>, autofs mailing list <autofs@vger.kernel.org>, Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] autofs - use dentry flags to block walks during expire Date: Fri, 09 Sep 2016 11:52:46 +0800 [thread overview] Message-ID: <1473393166.3097.3.camel@themaw.net> (raw) In-Reply-To: <87vay57sy3.fsf@notabene.neil.brown.name> On Fri, 2016-09-09 at 11:39 +1000, NeilBrown wrote: > On Thu, Sep 01 2016, Ian Kent wrote: > > > Somewhere along the way the autofs expire operation has changed to > > hold a spin lock over expired dentry selection. The autofs indirect > > mount expired dentry selection is complicated and quite lengthy so > > it isn't appropriate to hold a spin lock over the operation. > > > > Commit 47be6184 added a might_sleep() to dput() causing a BUG() > > about this usage to be issued. > > > > But the spin lock doesn't need to be held over this check, the > > autofs dentry info. flags are enough to block walks into dentrys > > during the expire. > > > > I've left the direct mount expire as it is (for now) becuase it > > is much simpler and quicker than the indirect mount expire and > > adding spin lock release and re-aquires would do nothing more > > than add overhead. > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > --- > > fs/autofs4/expire.c | 55 +++++++++++++++++++++++++++++++++++++++--------- > > --- > > 1 file changed, 42 insertions(+), 13 deletions(-) > > > > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c > > index b493909..2d8e176 100644 > > --- a/fs/autofs4/expire.c > > +++ b/fs/autofs4/expire.c > > @@ -417,6 +417,7 @@ static struct dentry *should_expire(struct dentry > > *dentry, > > } > > return NULL; > > } > > + > > /* > > * Find an eligible tree to time-out > > * A tree is eligible if :- > > @@ -432,6 +433,7 @@ struct dentry *autofs4_expire_indirect(struct > > super_block *sb, > > struct dentry *root = sb->s_root; > > struct dentry *dentry; > > struct dentry *expired; > > + struct dentry *found; > > struct autofs_info *ino; > > > > if (!root) > > @@ -442,31 +444,46 @@ struct dentry *autofs4_expire_indirect(struct > > super_block *sb, > > > > dentry = NULL; > > while ((dentry = get_next_positive_subdir(dentry, root))) { > > + int flags = how; > > + > > spin_lock(&sbi->fs_lock); > > ino = autofs4_dentry_ino(dentry); > > - if (ino->flags & AUTOFS_INF_WANT_EXPIRE) > > - expired = NULL; > > - else > > - expired = should_expire(dentry, mnt, timeout, how); > > - if (!expired) { > > + if (ino->flags & AUTOFS_INF_WANT_EXPIRE) { > > spin_unlock(&sbi->fs_lock); > > continue; > > } > > + spin_unlock(&sbi->fs_lock); > > + > > + expired = should_expire(dentry, mnt, timeout, flags); > > + if (!expired) > > + continue; > > + > > + spin_lock(&sbi->fs_lock); > > ino = autofs4_dentry_ino(expired); > > ino->flags |= AUTOFS_INF_WANT_EXPIRE; > > spin_unlock(&sbi->fs_lock); > > synchronize_rcu(); > > - spin_lock(&sbi->fs_lock); > > - if (should_expire(expired, mnt, timeout, how)) { > > - if (expired != dentry) > > - dput(dentry); > > - goto found; > > - } > > > > + /* Make sure a reference is not taken on found if > > + * things have changed. > > + */ > > + flags &= ~AUTOFS_EXP_LEAVES; > > + found = should_expire(expired, mnt, timeout, how); > > + if (!found || found != expired) > > + /* Something has changed, continue */ > > + goto next; > > + > > + if (expired != dentry) > > + dput(dentry); > > + > > + spin_lock(&sbi->fs_lock); > > + goto found; > > +next: > > + spin_lock(&sbi->fs_lock); > > ino->flags &= ~AUTOFS_INF_WANT_EXPIRE; > > + spin_unlock(&sbi->fs_lock); > > if (expired != dentry) > > dput(expired); > > - spin_unlock(&sbi->fs_lock); > > } > > return NULL; > > > > @@ -483,6 +500,7 @@ int autofs4_expire_wait(struct dentry *dentry, int > > rcu_walk) > > struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb); > > struct autofs_info *ino = autofs4_dentry_ino(dentry); > > int status; > > + int state; > > > > /* Block on any pending expire */ > > if (!(ino->flags & AUTOFS_INF_WANT_EXPIRE)) > > @@ -490,8 +508,19 @@ int autofs4_expire_wait(struct dentry *dentry, int > > rcu_walk) > > if (rcu_walk) > > return -ECHILD; > > > > +retry: > > spin_lock(&sbi->fs_lock); > > - if (ino->flags & AUTOFS_INF_EXPIRING) { > > + state = ino->flags & (AUTOFS_INF_WANT_EXPIRE | > > AUTOFS_INF_EXPIRING); > > + if (state == AUTOFS_INF_WANT_EXPIRE) { > > + spin_unlock(&sbi->fs_lock); > > + /* > > + * Possibly being selected for expire, wait until > > + * it's selected or not. > > + */ > > + schedule_timeout(HZ/10); > > Hi Ian, > > I think you want schedule_timeout_uninterruptible(HZ/10) here. > schedule_timeout() only causes a delay if the task state has been > changed from runnable. Right, I'll have another look, I saw that should be used but didn't actually do the change state. I have another location that calls schedule_timeout() which likely needs the same treatment. > > There is a similar bug in fscache_object_sleep_till_congested(). > Nothing changes the task state from TASK_RUNNING in that function > before it calls schedule_timeout(*timeoutp); > > Also should this patch be marked as > > Fixes: 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()") Indeed yes, I'll do that in a re-post, thanks. Ian
WARNING: multiple messages have this Message-ID (diff)
From: Ian Kent <raven@themaw.net> To: NeilBrown <neilb@suse.com>, Al Viro <viro@ZenIV.linux.org.uk> Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>, Takashi Iwai <tiwai@suse.de>, autofs mailing list <autofs@vger.kernel.org>, Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] autofs - use dentry flags to block walks during expire Date: Fri, 09 Sep 2016 11:52:46 +0800 [thread overview] Message-ID: <1473393166.3097.3.camel@themaw.net> (raw) In-Reply-To: <87vay57sy3.fsf@notabene.neil.brown.name> On Fri, 2016-09-09 at 11:39 +1000, NeilBrown wrote: > On Thu, Sep 01 2016, Ian Kent wrote: > > > Somewhere along the way the autofs expire operation has changed to > > hold a spin lock over expired dentry selection. The autofs indirect > > mount expired dentry selection is complicated and quite lengthy so > > it isn't appropriate to hold a spin lock over the operation. > > > > Commit 47be6184 added a might_sleep() to dput() causing a BUG() > > about this usage to be issued. > > > > But the spin lock doesn't need to be held over this check, the > > autofs dentry info. flags are enough to block walks into dentrys > > during the expire. > > > > I've left the direct mount expire as it is (for now) becuase it > > is much simpler and quicker than the indirect mount expire and > > adding spin lock release and re-aquires would do nothing more > > than add overhead. > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > --- > > fs/autofs4/expire.c | 55 +++++++++++++++++++++++++++++++++++++++--------- > > --- > > 1 file changed, 42 insertions(+), 13 deletions(-) > > > > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c > > index b493909..2d8e176 100644 > > --- a/fs/autofs4/expire.c > > +++ b/fs/autofs4/expire.c > > @@ -417,6 +417,7 @@ static struct dentry *should_expire(struct dentry > > *dentry, > > } > > return NULL; > > } > > + > > /* > > * Find an eligible tree to time-out > > * A tree is eligible if :- > > @@ -432,6 +433,7 @@ struct dentry *autofs4_expire_indirect(struct > > super_block *sb, > > struct dentry *root = sb->s_root; > > struct dentry *dentry; > > struct dentry *expired; > > + struct dentry *found; > > struct autofs_info *ino; > > > > if (!root) > > @@ -442,31 +444,46 @@ struct dentry *autofs4_expire_indirect(struct > > super_block *sb, > > > > dentry = NULL; > > while ((dentry = get_next_positive_subdir(dentry, root))) { > > + int flags = how; > > + > > spin_lock(&sbi->fs_lock); > > ino = autofs4_dentry_ino(dentry); > > - if (ino->flags & AUTOFS_INF_WANT_EXPIRE) > > - expired = NULL; > > - else > > - expired = should_expire(dentry, mnt, timeout, how); > > - if (!expired) { > > + if (ino->flags & AUTOFS_INF_WANT_EXPIRE) { > > spin_unlock(&sbi->fs_lock); > > continue; > > } > > + spin_unlock(&sbi->fs_lock); > > + > > + expired = should_expire(dentry, mnt, timeout, flags); > > + if (!expired) > > + continue; > > + > > + spin_lock(&sbi->fs_lock); > > ino = autofs4_dentry_ino(expired); > > ino->flags |= AUTOFS_INF_WANT_EXPIRE; > > spin_unlock(&sbi->fs_lock); > > synchronize_rcu(); > > - spin_lock(&sbi->fs_lock); > > - if (should_expire(expired, mnt, timeout, how)) { > > - if (expired != dentry) > > - dput(dentry); > > - goto found; > > - } > > > > + /* Make sure a reference is not taken on found if > > + * things have changed. > > + */ > > + flags &= ~AUTOFS_EXP_LEAVES; > > + found = should_expire(expired, mnt, timeout, how); > > + if (!found || found != expired) > > + /* Something has changed, continue */ > > + goto next; > > + > > + if (expired != dentry) > > + dput(dentry); > > + > > + spin_lock(&sbi->fs_lock); > > + goto found; > > +next: > > + spin_lock(&sbi->fs_lock); > > ino->flags &= ~AUTOFS_INF_WANT_EXPIRE; > > + spin_unlock(&sbi->fs_lock); > > if (expired != dentry) > > dput(expired); > > - spin_unlock(&sbi->fs_lock); > > } > > return NULL; > > > > @@ -483,6 +500,7 @@ int autofs4_expire_wait(struct dentry *dentry, int > > rcu_walk) > > struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb); > > struct autofs_info *ino = autofs4_dentry_ino(dentry); > > int status; > > + int state; > > > > /* Block on any pending expire */ > > if (!(ino->flags & AUTOFS_INF_WANT_EXPIRE)) > > @@ -490,8 +508,19 @@ int autofs4_expire_wait(struct dentry *dentry, int > > rcu_walk) > > if (rcu_walk) > > return -ECHILD; > > > > +retry: > > spin_lock(&sbi->fs_lock); > > - if (ino->flags & AUTOFS_INF_EXPIRING) { > > + state = ino->flags & (AUTOFS_INF_WANT_EXPIRE | > > AUTOFS_INF_EXPIRING); > > + if (state == AUTOFS_INF_WANT_EXPIRE) { > > + spin_unlock(&sbi->fs_lock); > > + /* > > + * Possibly being selected for expire, wait until > > + * it's selected or not. > > + */ > > + schedule_timeout(HZ/10); > > Hi Ian, > > I think you want schedule_timeout_uninterruptible(HZ/10) here. > schedule_timeout() only causes a delay if the task state has been > changed from runnable. Right, I'll have another look, I saw that should be used but didn't actually do the change state. I have another location that calls schedule_timeout() which likely needs the same treatment. > > There is a similar bug in fscache_object_sleep_till_congested(). > Nothing changes the task state from TASK_RUNNING in that function > before it calls schedule_timeout(*timeoutp); > > Also should this patch be marked as > > Fixes: 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()") Indeed yes, I'll do that in a re-post, thanks. Ian -- To unsubscribe from this list: send the line "unsubscribe autofs" in
next prev parent reply other threads:[~2016-09-09 3:52 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-09-01 1:21 [PATCH] autofs - use dentry flags to block walks during expire Ian Kent 2016-09-01 1:21 ` Ian Kent 2016-09-01 9:13 ` Takashi Iwai 2016-09-01 9:13 ` Takashi Iwai 2016-09-09 1:39 ` NeilBrown 2016-09-09 3:52 ` Ian Kent [this message] 2016-09-09 3:52 ` Ian Kent 2016-09-12 1:40 Ian Kent 2016-09-12 1:40 ` Ian Kent 2016-09-12 10:06 ` Takashi Iwai 2016-09-12 21:07 ` Andrew Morton 2016-09-12 21:07 ` Andrew Morton
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=1473393166.3097.3.camel@themaw.net \ --to=raven@themaw.net \ --cc=autofs@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=neilb@suse.com \ --cc=tiwai@suse.de \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.