Re: Issues with delalloc->real extent allocation

[Dave Chinner <david@fromorbit.com>]

On Sun, Jan 16, 2011 at 07:28:27PM -0500, Lachlan McIlroy wrote:
> ----- Original Message -----
> > Hi Dave,
> > 
> > On Fri, Jan 14, 2011 at 11:29:00AM +1100, Dave Chinner wrote:
> > > I've noticed a few suspicious things trying to reproduce the
> > > allocate-in-the-middle-of-a-delalloc-extent,
> > ...
> > > Secondly, I think we have the same expose-the-entire-delalloc-extent
> > > -to-stale-data-exposure problem in ->writepage. This onnne, however,
> > > is due to using BMAPI_ENTIRE to allocate the entire delalloc extent
> > > the first time any part of it is written to. Even if we are only
> > > writing a single page (i.e. wbc->nr_to_write = 1) and the delalloc
> > > extent covers gigabytes. So, same problem when we crash.
> > >
> > > Finally, I think the extsize based problem exposed by test 229 is a
> > > also a result of allocating space we have no pages covering in the
> > > page cache (triggered by BMAPI_ENTIRE allocation) so the allocated
> > > space is never zeroed and hence exposes stale data.
> > 
> > This is precisely the bug I was going after when I hit the
> > allocate-in-the-middle-of-a-delalloc-extent bug. This is a race
> > between
> > block_prepare_write/__xfs_get_blocks and writepage/xfs_page_state
> > convert. When xfs_page_state_convert allocates a real extent for a
> > page
> > toward the beginning of a delalloc extent, XFS_BMAPI converts the
> > entire
> > delalloc extent. Any subsequent writes into the page cache toward the
> > end of this freshly allocated extent will see a written extent instead
> > of delalloc and read the block from disk into the page before writing
> > over it. If the write does not cover the entire page garbage from disk
> > will be exposed into the page cache.
> 
> Ben, take a look at the XFS gap list code in IRIX - this code was
> designed specifically to handle this problem.  It's also implemented in
> several of the cxfs clients too.  On entry to a write() it will create a
> list of all the holes that exist in the write range before any delayed
> allocations are created.  The first call to xfs_bmapi() sets up the delayed
> allocation for the entire write (even if the bmaps returned don't cover the
> full write range).  If the write results in a fault from disk then it checks
> the gap list to see if any sections of the buffer used to cover a hole and
> if so it ignores the state of the extent and zeroes those region(s) in the
> buffer that match the pre-existing holes.  If the buffer has multiple
> non-hole sections that need to be read from disk the whole buffer will be
> read from disk and the zeroing of the holes is done post I/O - this reduces
> the number of I/Os to be done.  The whole delayed allocation can be safely
> converted at any time without risk of reading exposed data (assuming no
> crash that is). 

IMO, that caveat exposes the problem with this approach - it is not
persistent. It adds a lot of complexity but doesn't solve the
underlying problem: that we are exposing real extents via allocation
without having written any data to them.

> As the write progresses through the range it removes
> sections from the front of the gap list so by the time the write is complete
> the gap list is empty.  The gap list does not have a dedicated lock to
> protect it but instead relies on the iolock to ensure that only one write
> operation occurs at once (so it's not appropriate for direct I/O).
> 
> > 
> > <snip>
> > 
> > > I'm sure there are other ways to solve these problems, but these two
> > > are the ones that come to mind for me. I'm open to other solutions
> > > or ways to improve on these ones, especially if they are simpler. ;)
> > > Anyone got any ideas or improvements?
> > 
> > The direction I've been taking is to use XFS_BMAPI_EXACT in
> > *xfs_iomap_write_allocate to ensure that an extent covering exactly
> > the
> > pages we're prepared to write out immediately is allocated and the
> > rest
> > of the delalloc extent is left as is. This exercises some of the btree
> > code more heavily and led to the discovery of the
> > allocate-in-the-middle-of-a-delalloc-extent bug. It also presents a
> > performance issue which I've tried to resolve by extending
> > xfs_probe_cluster to probe delalloc extents-- lock up all of the pages
> > to be converted before performing the allocation and hold those locks
> > until they are submitted for writeback. It's not very pretty but it
> > resolves the corruption.
> > 
> > There is still the issue of crashes... This could be solved by
> > converting from delalloc to unwritten in xfs_page_state_convert in
> > this
> > very exact way and then to written in the io completion handler. Never
> > go delalloc->written directly.
> 
> That solution would probably make the gap list redundant too.

Yes, I think you are right. The "gaps" would get mapped as unwritten
rather than as normal extents and hence get zereod appropriately. It
would also preventing exposure after a crash due to being
persistent.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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