Re: [PATCH 2/6] Replace inode flush semaphore with a completion

[Dave Chinner <david@fromorbit.com>]

On Thu, Jun 26, 2008 at 08:30:11PM -0600, Matthew Wilcox wrote:
> On Thu, Jun 26, 2008 at 02:41:13PM +1000, Dave Chinner wrote:
> > Use the new completion flush code to implement the inode
> > flush lock. Removes one of the final users of semaphores
> > in the XFS code base.
> 
> Let's demonstrate converting this one to completions ...

Great. I need all the help I can get. ;)

> > --- a/fs/xfs/xfs_iget.c
> > +++ b/fs/xfs/xfs_iget.c
> > @@ -216,7 +216,7 @@ finish_inode:
> >  	mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
> >  	init_waitqueue_head(&ip->i_ipin_wait);
> >  	atomic_set(&ip->i_pincount, 0);
> > -	initnsema(&ip->i_flock, 1, "xfsfino");
> > +	init_completion_flush(&ip->i_flush);
> 
> +	init_completion(&ip->i_flush);
> +	complete(&ip->i_flush);

Ok, that would work. Need commenting, though.

> >  int
> >  xfs_iflock_nowait(xfs_inode_t *ip)
> >  {
> > -	return (cpsema(&(ip->i_flock)));
> > +	return completion_flush_start_nowait(&ip->i_flush);
> 
> This is where you need a new function ...
> 
> +	return nowait_for_completion(&ip->i_flush);
> 
> Yes, we probably need a better name for the down_trylock() equivalent.

I suggested try_wait_for_completion() in a different email - it's
a difficult on to say "a wait_for_completion() call that doesn't
wait"....

I think I'll stick with the try_.... version to match other "try"
operations.


> >  int
> > @@ -3070,7 +3069,7 @@ xfs_iflush(
> >  	XFS_STATS_INC(xs_iflush_count);
> >  
> >  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> > -	ASSERT(issemalocked(&(ip->i_flock)));
> > +	ASSERT(completion_flush_inprogress(&ip->i_flush));
> 
> is_complete()?

That's a little to general for my liking - I note that there
are already an existing is_complete() function in a driver.
completion_done() perhaps?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com