[Top] [All Lists]

Re: [PATCH 1/3] fs: get_blocks needs an unaligned mapping flag

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/3] fs: get_blocks needs an unaligned mapping flag
From: Alex Elder <aelder@xxxxxxx>
Date: Fri, 23 Jul 2010 10:29:57 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1279881678-1660-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1279881678-1660-1-git-send-email-david@xxxxxxxxxxxxx> <1279881678-1660-2-git-send-email-david@xxxxxxxxxxxxx>
Reply-to: aelder@xxxxxxx
On Fri, 2010-07-23 at 20:41 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> When issuing concurrent unaligned direct IO to the same filesystem block, the
> direct IO sub-block zeroing code will extend the length of the write being 
> done
> when writing into a hole or unwritten extents. If we are writing into 
> unwritten
> extents, then the two IOs will both see the extent as unwritten at IO issue
> time and attempt to zero the part of the block that they are not writing to.
> The result of this is that whichever IO completes last will win and part of 
> the
> block will be zero instead of containing the correct data. Eric Sandeen has
> demonstrated the problem with xfstest #240. In the case of XFS, we allow
> concurrent direct IO writes to occur, but we cannot allow block zeroing to
> occur concurrently with other IO.
> To allow serialisation of block zeroing across multiple independent IOs, we
> need to know if the region being mapped by the IO is fsb-aligned or not. If it
> is not aligned, then we need to prevent further direct IO writes from being
> executed until the IO that is doing the zeroing completes (i.e. converts the
> extent back to written). Passing the fact that the mapping is for an unaligned
> IO into the get_blocks calback is sufficient to allow us to implement the
> necessary serialisation.
> Change the "create" parameter of the get_blocks callback to a flags field,
> and define the flags to be backwards compatible as such:
> #define GET_BLOCKS_READ               0x00    /* map, no allocation */
> #define GET_BLOCKS_CREATE     0x01    /* map, allocate if hole */
> #define GET_BLOCKS_UNALIGNED  0x02    /* mapping for unaligned IO */

This looks good to me.

Two nits.  You could change the name of the "create" variable
in get_more_blocks() to be consistent with your change.

And I guess I like that the GET_BLOCKS_UNALIGNED is a flag
OR'd rather than a distinct value (i.e., CREATE_UNALIGNED).
You could make the comment at the definition of these
flag values to indicate they're "flag bits" rather than
just "flags" because it could conceivably be misconstrued

In any case:

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

<Prev in Thread] Current Thread [Next in Thread>