Re: [PATCH 0/5] RCU-walk support for autofs

[NeilBrown]

On Tue, 19 Aug 2014 20:36:55 +0800 Ian Kent <raven@xxxxxxxxxx> wrote:

> On Tue, 2014-08-19 at 21:16 +1000, NeilBrown wrote:
> > On Tue, 19 Aug 2014 18:02:27 +0800 Ian Kent <raven@xxxxxxxxxx> wrote:
> > 
> > > On Mon, 2014-08-18 at 16:25 +0800, Ian Kent wrote:
> > > > On Mon, 2014-08-18 at 16:33 +1000, NeilBrown wrote:
> > > > > Hi Ian,
> > > > >  Have you had a chance to run your tests in these patches yet?
> > > > >  I've done what testing I can think of and cannot fault them.
> > > > 
> > > > I haven't, I've been plagued with illness so I'm not getting nearly
> > > > enough done. I'll try to put a kernel together and run the test in the
> > > > next week or so.
> > > 
> > > Just to let you know that I managed to spend some time on this. I built
> > > a kernel (3.17.0-rc1) with the series and ran a couple of tests.
> > > 
> > > I'm not certain that the patches I used are identical to your posting, I
> > > saw one difference, after the fact, that shouldn't matter, I'll have to
> > > check that.
> > > 
> > > It isn't possible to test expire to mount races because the mounts in
> > > the tree never expire.
> > > 
> > > At first I thought it was because so many processes were accessing the
> > > tree all the time but manually constructing the maps and mounting the
> > > mounts shows that nothing ever expires, at least for this tree.
> > > 
> > > However, issuing a shut down does expire all the mounts and shuts down
> > > autofs cleanly.
> > > 
> > > So there is something not quite right with the expire check or my
> > > patches have mistakes.
> > 
> > Ahh.. I bet I know what it is.
> > autofs4_can_expire() isn't idempotent.
> > Because we call should_expire twice, autofs4_can_expire() is called twice and
> > the second time it failed because  the first time it resets ->last_used.
> > 
> > diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
> > index eb4b770a4bf6..80133a9d9427 100644
> > --- a/fs/autofs4/expire.c
> > +++ b/fs/autofs4/expire.c
> > @@ -26,6 +26,9 @@ static inline int autofs4_can_expire(struct dentry *dentry,
> >  	if (ino == NULL)
> >  		return 0;
> >  
> > +	if (ino->flags & AUTOFS_INF_NO_RCU)
> > +		/* Already performed this test */
> > +		return 1;
> 
> Wouldn't it be better to perform this check, or similar, further down
> where the last_used time is updated? After all it's only updated to
> prevent rapid fire expires on dentrys that refuse to umount for some
> reason.
> 

How about the follow approach instead?
Though I must admit that I find the 'now' global variable feels a bit
clumsy...
Is there a reason for not just using "jiffies" everywhere?

I've created a tag 'autofs4' in git://neil.brown.name/md/ which has this in
with all the others, in case that is useful.

Thanks,
NeilBrown



From 201f75bc25906e8f64e28b37f1bb478958bf2987 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@xxxxxxx>
Date: Wed, 20 Aug 2014 12:40:06 +1000
Subject: [PATCH] autofs4: make "autofs4_can_expire" idempotent.

Have a "test" function change the value it is testing can
be confusing, particularly as a future patch will be calling
this function twice.

So move the update for 'last_used' to avoid repeat expiry
to the place where the final determination on what to expire is known.

Signed-off-by: NeilBrown <neilb@xxxxxxx>

diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index bee939efca2b..af09dada91bc 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -30,12 +30,6 @@ static inline int autofs4_can_expire(struct dentry *dentry,
 		/* Too young to die */
 		if (!timeout || time_after(ino->last_used + timeout, now))
 			return 0;
-
-		/* update last_used here :-
-		   - obviously makes sense if it is in use now
-		   - less obviously, prevents rapid-fire expire
-		     attempts if expire fails the first time */
-		ino->last_used = now;
 	}
 	return 1;
 }
@@ -541,6 +535,8 @@ int autofs4_expire_run(struct super_block *sb,
 
 	spin_lock(&sbi->fs_lock);
 	ino = autofs4_dentry_ino(dentry);
+	/* avoid rapid-fire expire attempts if expiry fails */
+	ino->last_used = now;
 	ino->flags &= ~AUTOFS_INF_EXPIRING;
 	complete_all(&ino->expire_complete);
 	spin_unlock(&sbi->fs_lock);
@@ -567,6 +563,8 @@ int autofs4_do_expire_multi(struct super_block *sb, struct vfsmount *mnt,
 		ret = autofs4_wait(sbi, dentry, NFY_EXPIRE);
 
 		spin_lock(&sbi->fs_lock);
+		/* avoid rapid-fire expire attempts if expiry fails */
+		ino->last_used = now;
 		ino->flags &= ~AUTOFS_INF_EXPIRING;
 		complete_all(&ino->expire_complete);
 		spin_unlock(&sbi->fs_lock);

Attachment: signature.asc
Description: PGP signature