Re: [PATCH 1/5] [XFS] Use xfs_sync_inodes() for device flushing

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 17, 2009 at 09:08:36AM -0400, Mikulas Patocka wrote:
> On Sun, 15 Mar 2009, Dave Chinner wrote:
> 
> > From: Dave Chinner <dgc@evostor.com>
> > 
> > Currently xfs_device_flush calls sync_blockdev() which is
> > a no-op for XFS as all it's metadata is held in a different
> > address to the one sync_blockdev() works on.
> > 
> > Call xfs_sync_inodes() instead to flush all the delayed
> > allocation blocks out. To do this as efficiently as possible,
> > do it via two passes - one to do an async flush of all the
> > dirty blocks and a second to wait for all the IO to complete.
> > This requires some modification to the xfs-sync_inodes_ag()
> > flush code to do efficiently.

.....

> I tested the code and it works fine.
> 
> But I'm not somehow convinced that that TRYLOCK is right ... that it will 
> avoid that pagelock-deadlock that I found before.

Care to expand on why?

> What is the ordering of pagelock, ILOCK and IOLOCK?

iolock, pagelock, and ilock is always the innermost of the three.

> If you keep the 
> ordering right, you don't need trylock.

In this case, we are doing:

	iolock exclusive
	pagelock
	<flush>

Clearly we can't take the iolock during <flush> (nor any other
several other places in XFS that might try to take the iolock), so
trylock-and-fail is the usual method used in XFS for avoiding a
deadlock in this scenario.

> And if the ordering is wrong, 
> trylock will only lower the probability of a deadlock, not avoid it 
> completely.

Details of why you think this? XFS uses trylocks extensively to
avoid deadlocks in these sorts of situations, so I'm curious to
know why you think this technique provide an absolute guarantee in
this case....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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