linux-kernel - Re: [PATCH 6/6] autofs4: don't take spinlock when not needed in autofs4_lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 16 Jul 2014 16:10:03 +1000
From:	NeilBrown <neilb@...e.de>
To:	Ian Kent <raven@...maw.net>
Cc:	autofs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] autofs4: don't take spinlock when not needed in
 autofs4_lookup_expiring

On Wed, 16 Jul 2014 11:42:12 +0800 Ian Kent <raven@...maw.net> wrote:

> On Thu, 2014-07-10 at 09:41 +1000, NeilBrown wrote:
> > If the expiring_list is empty, we can avoid a costly spinlock
> > in the rcu-walk path through authfs4_d_manage.
> > 
> > Signed-off-by: NeilBrown <neilb@...e.de>
> 
> I know it should be straight forward to say this is OK but I always
> think twice and again about areas that are subject to race pressure,
> such as the expire to mount pressure of this code.
> 
> After thinking about it for a while now I don't have any reason to think
> this would be a problem. Perhaps later pressure testing will reveal
> something I missed.
> 
> It occurs to me that autofs4_lookup_active() might benefit from a
> similar addition. Multiple calls to ->lookup() made while the dentry is
> still unhashed should have enforced ordering due to the directory
> i_mutex so there shouldn't be a problem adding this. But perhaps you
> haven't seen delays in that function.

Yes I think the same optimisation could apply to autofs4_lookup_active().  It
wouldn't benefit as much though.
autofs4_lookup_active() is only called from autofs4_lookup() which is only
called when the dentry doesn't already exist.  So it isn't called so often
and isn't on the fast path.

However ... maybe "is on the active list" is exactly the "flag" I was wanting
earlier to see if a dentry might be waiting to be mounted on - in which case
d_managed already does the right thing.
I think it is time to read through the code again.  I seem to be
understanding more of it which always helps:-)

Thanks,
NeilBrown


> 
> Acked-by: Ian Kent <raven@...maw.net>
> 
> > ---
> >  fs/autofs4/root.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
> > index 1ad119407e2f..774c2dab331b 100644
> > --- a/fs/autofs4/root.c
> > +++ b/fs/autofs4/root.c
> > @@ -219,6 +219,8 @@ static struct dentry *autofs4_lookup_expiring(struct dentry *dentry,
> >  	const unsigned char *str = name->name;
> >  	struct list_head *p, *head;
> >  
> > +	if (list_empty(&sbi->expiring_list))
> > +		return NULL;
> >  	spin_lock(&sbi->lookup_lock);
> >  	head = &sbi->expiring_list;
> >  	list_for_each(p, head) {
> > 
> > 
> 


Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)