All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] autofs - use dentry flags to block walks during expire
@ 2016-09-01  1:21 ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2016-09-01  1:21 UTC (permalink / raw)
  To: Al Viro
  Cc: linux-fsdevel, Takashi Iwai, autofs mailing list, Kernel Mailing List

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);
+		goto retry;
+	}
+	if (state & AUTOFS_INF_EXPIRING) {
 		spin_unlock(&sbi->fs_lock);
 
 		pr_debug("waiting for expire %p name=%pd\n", dentry, dentry);

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] autofs - use dentry flags to block walks during expire
@ 2016-09-01  1:21 ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2016-09-01  1:21 UTC (permalink / raw)
  To: Al Viro
  Cc: linux-fsdevel, Takashi Iwai, autofs mailing list, Kernel Mailing List

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);
+		goto retry;
+	}
+	if (state & AUTOFS_INF_EXPIRING) {
 		spin_unlock(&sbi->fs_lock);
 
 		pr_debug("waiting for expire %p name=%pd\n", dentry, dentry);

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs - use dentry flags to block walks during expire
  2016-09-01  1:21 ` Ian Kent
@ 2016-09-01  9:13   ` Takashi Iwai
  -1 siblings, 0 replies; 12+ messages in thread
From: Takashi Iwai @ 2016-09-01  9:13 UTC (permalink / raw)
  To: Ian Kent; +Cc: Al Viro, linux-fsdevel, autofs mailing list, Kernel Mailing List

On Thu, 01 Sep 2016 03:21:14 +0200,
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>

Looks working fine, so far.

Reported-and-tested-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs - use dentry flags to block walks during expire
@ 2016-09-01  9:13   ` Takashi Iwai
  0 siblings, 0 replies; 12+ messages in thread
From: Takashi Iwai @ 2016-09-01  9:13 UTC (permalink / raw)
  To: Ian Kent; +Cc: Al Viro, linux-fsdevel, autofs mailing list, Kernel Mailing List

On Thu, 01 Sep 2016 03:21:14 +0200,
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>

Looks working fine, so far.

Reported-and-tested-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs - use dentry flags to block walks during expire
  2016-09-01  1:21 ` Ian Kent
  (?)
  (?)
@ 2016-09-09  1:39 ` NeilBrown
  2016-09-09  3:52     ` Ian Kent
  -1 siblings, 1 reply; 12+ messages in thread
From: NeilBrown @ 2016-09-09  1:39 UTC (permalink / raw)
  To: Ian Kent, Al Viro
  Cc: linux-fsdevel, Takashi Iwai, autofs mailing list, Kernel Mailing List

[-- Attachment #1: Type: text/plain, Size: 4640 bytes --]

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.

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()")

so it follows that patch in -stable backport?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs - use dentry flags to block walks during expire
  2016-09-09  1:39 ` NeilBrown
@ 2016-09-09  3:52     ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2016-09-09  3:52 UTC (permalink / raw)
  To: NeilBrown, Al Viro
  Cc: linux-fsdevel, Takashi Iwai, autofs mailing list, Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs - use dentry flags to block walks during expire
@ 2016-09-09  3:52     ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2016-09-09  3:52 UTC (permalink / raw)
  To: NeilBrown, Al Viro
  Cc: linux-fsdevel, Takashi Iwai, autofs mailing list, Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs - use dentry flags to block walks during expire
  2016-09-12  1:40 ` Ian Kent
@ 2016-09-12 21:07   ` Andrew Morton
  -1 siblings, 0 replies; 12+ messages in thread
From: Andrew Morton @ 2016-09-12 21:07 UTC (permalink / raw)
  To: Ian Kent
  Cc: Al Viro, Takashi Iwai, autofs mailing list, NeilBrown,
	Kernel Mailing List, linux-fsdevel

On Mon, 12 Sep 2016 09:40:17 +0800 Ian Kent <raven@themaw.net> 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.

It's a WARN_ONCE(), not a BUG()?

> 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.
> 
> Fixes: 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()")
> Signed-off-by: Ian Kent <raven@themaw.net>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: NeilBrown <neilb@suse.com>

47be61845c77 is cc:stable, so this should be likewise.

I've made those two changes to my copy of this changelog.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs - use dentry flags to block walks during expire
@ 2016-09-12 21:07   ` Andrew Morton
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Morton @ 2016-09-12 21:07 UTC (permalink / raw)
  To: Ian Kent
  Cc: Al Viro, Takashi Iwai, autofs mailing list, NeilBrown,
	Kernel Mailing List, linux-fsdevel

On Mon, 12 Sep 2016 09:40:17 +0800 Ian Kent <raven@themaw.net> 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.

It's a WARN_ONCE(), not a BUG()?

> 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.
> 
> Fixes: 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()")
> Signed-off-by: Ian Kent <raven@themaw.net>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: NeilBrown <neilb@suse.com>

47be61845c77 is cc:stable, so this should be likewise.

I've made those two changes to my copy of this changelog.

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs - use dentry flags to block walks during expire
  2016-09-12  1:40 ` Ian Kent
  (?)
@ 2016-09-12 10:06 ` Takashi Iwai
  -1 siblings, 0 replies; 12+ messages in thread
From: Takashi Iwai @ 2016-09-12 10:06 UTC (permalink / raw)
  To: Ian Kent
  Cc: Al Viro, autofs mailing list, NeilBrown, Kernel Mailing List,
	linux-fsdevel, Andrew Morton

On Mon, 12 Sep 2016 03:40:17 +0200,
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.
> 
> Fixes: 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()")
> Signed-off-by: Ian Kent <raven@themaw.net>
> Cc: Takashi Iwai <tiwai@suse.de>

Reported-and-tested-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: NeilBrown <neilb@suse.com>
> ---
>  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..d8e6d42 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_uninterruptible(HZ/10);
> +		goto retry;
> +	}
> +	if (state & AUTOFS_INF_EXPIRING) {
>  		spin_unlock(&sbi->fs_lock);
>  
>  		pr_debug("waiting for expire %p name=%pd\n", dentry, dentry);
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] autofs - use dentry flags to block walks during expire
@ 2016-09-12  1:40 ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2016-09-12  1:40 UTC (permalink / raw)
  To: Al Viro
  Cc: Takashi Iwai, autofs mailing list, NeilBrown,
	Kernel Mailing List, linux-fsdevel, Andrew Morton

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.

Fixes: 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()")
Signed-off-by: Ian Kent <raven@themaw.net>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: NeilBrown <neilb@suse.com>
---
 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..d8e6d42 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_uninterruptible(HZ/10);
+		goto retry;
+	}
+	if (state & AUTOFS_INF_EXPIRING) {
 		spin_unlock(&sbi->fs_lock);
 
 		pr_debug("waiting for expire %p name=%pd\n", dentry, dentry);

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] autofs - use dentry flags to block walks during expire
@ 2016-09-12  1:40 ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2016-09-12  1:40 UTC (permalink / raw)
  To: Al Viro
  Cc: Takashi Iwai, autofs mailing list, NeilBrown,
	Kernel Mailing List, linux-fsdevel, Andrew Morton

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.

Fixes: 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()")
Signed-off-by: Ian Kent <raven@themaw.net>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: NeilBrown <neilb@suse.com>
---
 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..d8e6d42 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_uninterruptible(HZ/10);
+		goto retry;
+	}
+	if (state & AUTOFS_INF_EXPIRING) {
 		spin_unlock(&sbi->fs_lock);
 
 		pr_debug("waiting for expire %p name=%pd\n", dentry, dentry);

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-09-12 21:07 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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.