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
