[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v4 21/23] block: Align block status
From: |
John Snow |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v4 21/23] block: Align block status requests |
Date: |
Mon, 2 Oct 2017 16:24:51 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
On 09/13/2017 12:03 PM, Eric Blake wrote:
> Any device that has request_alignment greater than 512 should be
> unable to report status at a finer granularity; it may also be
> simpler for such devices to be guaranteed that the block layer
> has rounded things out to the granularity boundary (the way the
> block layer already rounds all other I/O out). Besides, getting
> the code correct for super-sector alignment also benefits us
> for the fact that our public interface now has byte granularity,
> even though none of our drivers have byte-level callbacks.
>
> Add an assertion in blkdebug that proves that the block layer
> never requests status of unaligned sections, similar to what it
> does on other requests (while still keeping the generic helper
> in place for when future patches add a throttle driver). Note
> that iotest 177 already covers this (it would fail if you use
> just the blkdebug.c hunk without the io.c changes). Meanwhile,
> we can drop assertions in callers that no longer have to pass
> in sector-aligned addresses.
>
> There is a mid-function scope added for 'int count', for a
> couple of reasons: first, an upcoming patch will add an 'if'
> statement that checks whether a driver has an old- or new-style
> callback, and can conveniently use the same scope for less
> indentation churn at that time. Second, since we are trying
> to get rid of sector-based computations, wrapping things in
> a scope makes it easier to group and see what will be deleted
> in a final cleanup patch once all drivers have been converted
> to the new-style callback.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v3: tweak commit message [Fam], rebase to context conflicts, ensure
> we don't exceed 32-bit limit, drop R-b
> v2: new patch
> ---
> include/block/block_int.h | 3 ++-
> block/io.c | 55
> +++++++++++++++++++++++++++++++----------------
> block/blkdebug.c | 13 ++++++++++-
> 3 files changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 7f71c585a0..b1ceffba78 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -207,7 +207,8 @@ struct BlockDriver {
> * according to the current layer, and should not set
> * BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW. See block.h
> * for the meaning of _DATA, _ZERO, and _OFFSET_VALID. The block
> - * layer guarantees non-NULL pnum and file.
> + * layer guarantees input aligned to request_alignment, as well as
> + * non-NULL pnum and file.
> */
> int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
> int64_t sector_num, int nb_sectors, int *pnum,
> diff --git a/block/io.c b/block/io.c
> index ea63d19480..c78201b8eb 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1773,7 +1773,8 @@ static int64_t coroutine_fn
> bdrv_co_block_status(BlockDriverState *bs,
> int64_t n; /* bytes */
> int64_t ret, ret2;
> BlockDriverState *local_file = NULL;
> - int count; /* sectors */
> + int64_t aligned_offset, aligned_bytes;
> + uint32_t align;
>
> assert(pnum);
> total_size = bdrv_getlength(bs);
> @@ -1815,28 +1816,45 @@ static int64_t coroutine_fn
> bdrv_co_block_status(BlockDriverState *bs,
> }
>
> bdrv_inc_in_flight(bs);
> - /*
> - * TODO: Rather than require aligned offsets, we could instead
> - * round to the driver's request_alignment here, then touch up
> - * count afterwards back to the caller's expectations.
> - */
> - assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE));
> - bytes = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
> - ret = bs->drv->bdrv_co_get_block_status(bs, offset >> BDRV_SECTOR_BITS,
> - bytes >> BDRV_SECTOR_BITS,
> &count,
> - &local_file);
> - if (ret < 0) {
> - *pnum = 0;
> - goto out;
> +
> + /* Round out to request_alignment boundaries */
> + align = MAX(bs->bl.request_alignment, BDRV_SECTOR_SIZE);
There's something funny to me about an alignment request getting itself
aligned...
> + aligned_offset = QEMU_ALIGN_DOWN(offset, align);
> + aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset;
> +
> + {
> + int count; /* sectors */
> +
> + assert(QEMU_IS_ALIGNED(aligned_offset | aligned_bytes,
> + BDRV_SECTOR_SIZE));
> + ret = bs->drv->bdrv_co_get_block_status(
> + bs, aligned_offset >> BDRV_SECTOR_BITS,
> + MIN(INT_MAX, aligned_bytes) >> BDRV_SECTOR_BITS, &count,
I guess under the belief that INT_MAX will be strictly less than
BDRV_REQUEST_MAX_BYTES, or some other reason I'm missing for the change?
> + &local_file);
> + if (ret < 0) {
> + *pnum = 0;
> + goto out;
> + }
> + *pnum = count * BDRV_SECTOR_SIZE;
Is it asking for trouble to be updating pnum here before we undo our
alignment corrections? For readability reasons and preventing an
accidental context-based oopsy-daisy.
> + }
> +
> + /* Clamp pnum and ret to original request */
> + assert(QEMU_IS_ALIGNED(*pnum, align));
Oh, do we guarantee this? I guess we do..
> + *pnum -= offset - aligned_offset;
can pnum prior to adjustment ever be less than offset - aligned_offset?
i.e., can this underflow?
(Can we fail to actually inquire about the range the caller was
interested in by aligning down too much and observing a difference in
allocation status between the alignment pre-range and the actual range?)
> + if (aligned_offset >> BDRV_SECTOR_BITS != offset >> BDRV_SECTOR_BITS &&
> + ret & BDRV_BLOCK_OFFSET_VALID) {
> + ret += QEMU_ALIGN_DOWN(offset - aligned_offset, BDRV_SECTOR_SIZE);
> + }
Alright, and if the starting sectors are different (Wait, why is it
sectors now instead of the requested alignment? Is this safe for all
formats?) we adjust the return value forward a little bit to match the
difference.
> + if (*pnum > bytes) {
> + *pnum = bytes;
> }
Assuming this clamps the aligned_bytes range down to the bytes range, in
case it's contiguous beyond what the caller asked for.
> - *pnum = count * BDRV_SECTOR_SIZE;
>
> if (ret & BDRV_BLOCK_RAW) {
> assert(ret & BDRV_BLOCK_OFFSET_VALID && local_file);
> ret = bdrv_co_block_status(local_file, mapping,
> - ret & BDRV_BLOCK_OFFSET_MASK,
> + (ret & BDRV_BLOCK_OFFSET_MASK) |
> + (offset & ~BDRV_BLOCK_OFFSET_MASK),
> *pnum, pnum, &local_file);
> - assert(QEMU_IS_ALIGNED(*pnum, BDRV_SECTOR_SIZE));
> goto out;
> }
>
> @@ -1860,7 +1878,8 @@ static int64_t coroutine_fn
> bdrv_co_block_status(BlockDriverState *bs,
> int64_t file_pnum;
>
> ret2 = bdrv_co_block_status(local_file, mapping,
> - ret & BDRV_BLOCK_OFFSET_MASK,
> + (ret & BDRV_BLOCK_OFFSET_MASK) |
> + (offset & ~BDRV_BLOCK_OFFSET_MASK),
> *pnum, &file_pnum, NULL);
> if (ret2 >= 0) {
> /* Ignore errors. This is just providing extra information, it
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 46e53f2f09..f54fe33cae 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -628,6 +628,17 @@ static int coroutine_fn
> blkdebug_co_pdiscard(BlockDriverState *bs,
> return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
> }
>
> +static int64_t coroutine_fn blkdebug_co_get_block_status(
> + BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
> + BlockDriverState **file)
> +{
> + assert(QEMU_IS_ALIGNED(sector_num | nb_sectors,
> + DIV_ROUND_UP(bs->bl.request_alignment,
> + BDRV_SECTOR_SIZE)));
> + return bdrv_co_get_block_status_from_file(bs, sector_num, nb_sectors,
> + pnum, file);
> +}
> +
> static void blkdebug_close(BlockDriverState *bs)
> {
> BDRVBlkdebugState *s = bs->opaque;
> @@ -897,7 +908,7 @@ static BlockDriver bdrv_blkdebug = {
> .bdrv_co_flush_to_disk = blkdebug_co_flush,
> .bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes,
> .bdrv_co_pdiscard = blkdebug_co_pdiscard,
> - .bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
> + .bdrv_co_get_block_status = blkdebug_co_get_block_status,
>
> .bdrv_debug_event = blkdebug_debug_event,
> .bdrv_debug_breakpoint = blkdebug_debug_breakpoint,
>
Looks good overall but I have some comprehension issues in my own head
about the adjustment math and why the various alignments are safe.
- Re: [Qemu-block] [Qemu-devel] [PATCH v4 21/23] block: Align block status requests,
John Snow <=