Re: [patch 05/27] fs: brlock vfsmount_lock

[Nick Piggin <npiggin@suse.de>]

On Sat, Apr 25, 2009 at 04:50:40AM +0100, Al Viro wrote:
> On Sat, Apr 25, 2009 at 11:20:25AM +1000, npiggin@suse.de wrote:
> 
> [overall: sane idea, but...]
> 
> > +void vfsmount_read_lock(void)
> > +{
> > +	spinlock_t *lock;
> > +
> > +	lock = &get_cpu_var(vfsmount_lock);
> > +	spin_lock(lock);
> > +}
> > +
> > +void vfsmount_read_unlock(void)
> > +{
> > +	spinlock_t *lock;
> > +
> > +	lock = &__get_cpu_var(vfsmount_lock);
> > +	spin_unlock(lock);
> > +	put_cpu_var(vfsmount_lock);
> > +}
> 
> These might be hot enough to be worth inlining, at least in fs/namei.c
> users.  Or not - really needs testing.

Hmm, no you could be right. Most of the code is still OOL in the
spinlock call, so avoiding one level of call chain is probably
going to be a win. I'll see how much it increases code size.

 
> > @@ -68,9 +113,9 @@ static int mnt_alloc_id(struct vfsmount
> >  
> >  retry:
> >  	ida_pre_get(&mnt_id_ida, GFP_KERNEL);
> > -	spin_lock(&vfsmount_lock);
> > +	vfsmount_write_lock();
> >  	res = ida_get_new(&mnt_id_ida, &mnt->mnt_id);
> > -	spin_unlock(&vfsmount_lock);
> > +	vfsmount_write_unlock();
> 
> Yuck.  _Really_ an overkill here.
> 
> >  static void mnt_free_id(struct vfsmount *mnt)
> >  {
> > -	spin_lock(&vfsmount_lock);
> > +	vfsmount_write_lock();
> >  	ida_remove(&mnt_id_ida, mnt->mnt_id);
> > -	spin_unlock(&vfsmount_lock);
> > +	vfsmount_write_unlock();
> >  }
> 
> Ditto.

Yeah, wanted to try going as simple as possible for the first cut.
Shall I just add another spinlock for it?

> Missing: description of when we need it for read/when we need it for write.

OK, I'll work on the documentation.