On Tue, Jan 18, 2011 at 03:47:52PM -0500, Christoph Hellwig wrote:
> On Sat, Jan 15, 2011 at 10:55:49AM +1100, Dave Chinner wrote:
> > I see from later on you are getting this state fom using a large
> > extsize. Perhaps this comes back to my comment about extsize
> > alignment might be better handled at the .aio_write level rather
> > than hiding inside the get_block() callback and attempting to handle
> > the mismatch at the .writepage level.
> > Worth noting, though, is that DIO handles extsize unaligned writes
> > by allocating unwritten extsize sized/aligned extents an then doing
> > conversion at IO completion time. So perhaps we should be following
> > this example for delalloc conversion....
> That's the other option for zeroing. In many ways this makes a lot
> more sense, also for the thin provisioned virtualization image file
> use case I'm looking into right now.
> > I think, however, if we use delalloc->unwritten allocation, we will
> > need to stop trusting the state of buffer heads in .writepage. That
> > is because we'd then have blocks marked as buffer_delay() that
> > really cover unwritten extents and would need remapping. We're
> > already moving in the direction of not using the state in buffer
> > heads in ->writepage, so perhaps we need to speed up that
> > conversion as the first. Christoph, what are you plans here?
> We really don't do much with the flags anymore. We already treat
> overwritten (just buffer_uptodate) and delayed buffers are already
> exactly the same.
Except for the fact we use the delalloc state from the buffer to
trigger allocation after mapping. We could probably just use
isnullstartblock() for this - only a mapping from a buffer over a
delalloc range should return a null startblock.
> Unwritten buffers are still slightly different,
> in that we add XFS_BMAPI_IGSTATE to the bmapi flags. This is just
> a leftover from the old code, and finding out why exactly we add
> it is still on my todo list.
XFS_BMAPI_IGSTATE does a couple of things. Firstly, It prevents
xfs_bmapi() from allocating new extents (turns off write mode).
This isn't an issue where it is used because neither of the call
sites set XFS_BMAPI_WRITE.
Secondly, it allows xfs_bmapi() to consider adjacent written
and unwritten extents as contiguous but has no effect on delalloc
extents. That is, if we have:
A B C
And we ask to map the range A-C with a single map, we'll get back
A-B. If we add XFS_BMAPI_IGSTATE, we'll get back A-C instead. This
means that when we map the range A-C for IO, we'll do it as a single
IO. The unwritten extent conversion code handles ranges like this
correctly - it will only convert the unwritten part of the range to
written. It is arguable that this is unnecessary because the
elevator will merge the two adjacent IOs anyway, but it does reduce
the number of mapping calls and IOs issued...
Hence it is effectively mechanism for optimising IO mappings where
we have contiguous extents with mixed written/unwritten state.
In fact, we probably should be setting this for normal written
extents as well, so that the case:
A B C
is also handled with the same optimisation. That makes handling
unwritten and overwrites identical, with only delalloc being
different. If we assume delalloc when we get a null startblock,
then we don't need to look at the buffer state at all for the
> The page clustering code checks if
> the buffer still matches the type in xfs_convert_page, though.
> And I'm not quite sure yet how we can remove that - the easiest
> option would be to keep holding the ilock until the whole clustered
> writeout is done, but we'd need to investigate what effect that
> has on performance.
Holding the ilock doesn't protect against page cache truncation, but
we can still check for that via page->mapping. Otherwise this seems
like it would work to me - I trust the extent list to reflect the
correct state a lot more than I do the buffer heads.
It seems to me that with such modifications, the only thing that we
are using the bufferhead for is the buffer_uptodate() flag to
determine if we should write the block or not. If we can find some
other way of holding this state then we don't need bufferheads in
the write path at all, right?