On Tue, Sep 27, 2016 at 06:07:32PM +0800, Ian Kent wrote:
> Original Linus tree commit id: 7cbdb4a286
>
> 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 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()") added a
> might_sleep() to dput() causing a WARN_ONCE() 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) because 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.
>
> Fixes: 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()")
> Link: http://lkml.kernel.org/r/20160912014017.1773.73060.stgit@xxxxxxxxxxxxxxxx
> Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
> Reported-by: Takashi Iwai <tiwai@xxxxxxx>
> Tested-by: Takashi Iwai <tiwai@xxxxxxx>
> Cc: Takashi Iwai <tiwai@xxxxxxx>
> Cc: NeilBrown <neilb@xxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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 4182d3e..5b4c4d4 100644
> --- a/fs/autofs4/expire.c
> +++ b/fs/autofs4/expire.c
> @@ -415,6 +415,7 @@ static struct dentry *should_expire(struct dentry *dentry,
> }
> return NULL;
> }
> +
> /*
> * Find an eligible tree to time-out
> * A tree is eligible if :-
> @@ -430,6 +431,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)
> @@ -440,31 +442,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;
>
> @@ -481,6 +498,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))
> @@ -488,8 +506,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_EXPIRING) {
The upstream patch says:
if (state == AUTOFS_INF_WANT_EXPIRE) {
here, why change it?
> + spin_unlock(&sbi->fs_lock);
> + /*
> + * Possibly being selected for expire, wait until
> + * it's selected or not.
> + */
> + schedule_timeout_uninterruptible(HZ/10);
> + goto retry;
> + }
> + if (state & AUTOFS_INF_EXPIRING) {
Now you are checking the same thing again?
Are you sure this backport is correct?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html