Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 2/2] xfs: do not call xfs_bdstrat_cb in xfs_buf_iodone_callbacks
Date: Wed, 4 Jul 2012 18:32:34 +1000	[thread overview]
Message-ID: <20120704083234.GG19223@dastard> (raw)
In-Reply-To: <20120704055723.GB27500@infradead.org>

On Wed, Jul 04, 2012 at 01:57:23AM -0400, Christoph Hellwig wrote:
> On Wed, Jul 04, 2012 at 09:29:23AM +1000, Dave Chinner wrote:
> > It's not working yet - I found an issue with logging and writeback
> > of uncached buffers through the AIL (i.e. the superblock). This only
> > works by good fortune right now and requires uncached buffers to
> > carry their block number internally, so I need to rethink and rework
> > the patch.
> 
> What is the problem with uncached buffers?  I'd hate having to move
> the superblock buffer away from the uncached scheme.

I'm not exactly sure yet - I last looked at it before I went on
holidays last week. I was getting random ASSERT failures, always on
the superblock buffer and always due ot trying to IO to
XFS_BUF_DADDR_NULL because it was an uncached buffer. The path to it
was always through the AIL flushing, so it was using
xfs_buf_iorequest(bp) and getting the null blkno rather than
xfs_buf_uncached_iorequest(bp, blkno, len) which supplies the blkno.
Like I said, I need to rework is so that either
xfs_buf_iorequest(bp) works with uncached buffers, or we don't use
uncached buffers in transactions.

IMO, using uncached buffers in transactions is dangerous because
concurrent transactions can't find buffers uncached buffers at the
same address and so such buffers need higher level synchronisation
interfaces (as the superblock has).

I'm happy to leave the superblock as uncached and work around it in
some way, but it just seems wrong to me to have one buffer behaves
differently to all the cached and other uncached buffers in the
system. It doesn't sit well in my mind to do that when the problem
simply goes away if we make the superblock a cached buffer again....

> > How much does it change? I'm also trying to get all the read verify
> > callback infrastructure changes done for 3.6, and i suspect these may
> > step on each other. I've just about got those patches done - testing
> > and bug fixing is happening at the moment....
> 
> Basically a lot of impact around all callers of xfsbdstrat (which is
> going away) and some impact in xfs_buf.c around the higher-level
> read/write code.  I'm not entirely done with the plane I have so
> other things might get in the way and make it more complicated in
> the end.
> 
> Another thing it depends on is to only start the sync work item later
> during mount so that the re-read of the superblock after recovery can
> use normal buffer cache interfaces instead of xfsbdstrat.

Well, we've already got conflicts there, because all of the uncached
IO changes I've made remove xfsbdstrat() from those paths....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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