Re: [PATCH 2/2] xfs: push stale, pinned buffers on trylock failures

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 24, 2010 at 12:20:51PM -0500, Christoph Hellwig wrote:
> > -	/* Attempt to get the semaphore without sleeping,
> > -	 * if this does not work then we need to drop the
> > -	 * spinlock and do a hard attempt on the semaphore.
> > +	/*
> > +	 * Attempt to get the semaphore without sleeping first. if we fail then
> > +	 * do a blocking lock if requested.
> >  	 */
> 
> You might as well remove the comment entirely, as it's utterly
> pointless.

Ok.

> > -	if (down_trylock(&bp->b_sema)) {
> > +	if (xfs_buf_cond_lock(bp)) {
> >  		if (!(flags & XBF_TRYLOCK)) {
> >  			/* wait for buffer ownership */
> >  			xfs_buf_lock(bp);
> >  			XFS_STATS_INC(xb_get_locked_waited);
> >  		} else {
> > -			/* We asked for a trylock and failed, no need
> > -			 * to look at file offset and length here, we
> > -			 * know that this buffer at least overlaps our
> > -			 * buffer and is locked, therefore our buffer
> > -			 * either does not exist, or is this buffer.
> > -			 */
> > +			/* We asked for a trylock and failed.  */
> >  			xfs_buf_rele(bp);
> >  			XFS_STATS_INC(xb_busy_locked);
> >  			return NULL;
> >  		}
> 
> In case we wait for the lock we now do the log force twice, but that
> should be fine.

Yeah, I considered ways of avoiding this, but when i actually
measured how often it occurs, it is quite rare because the log force
clears the entire backlog of pinned, stale buffers and so the log
force is lost in the noise of other IO. Hence I thought the more
robust method of fixing and calling xfs_buf_cond_lock() was a better
tradeoff.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs