Re: [Qemu-block] [PATCH 5/5] block: Move request_alignment into BlockLimit

[Kevin Wolf]

Am 03.06.2016 um 19:03 hat Eric Blake geschrieben:
> It makes more sense to have ALL block size limit constraints
> in the same struct.  Improve the documentation while at it.
> 
> Note that bdrv_refresh_limits() has to keep things alive across
> a memset() of BlockLimits.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  include/block/block_int.h | 12 ++++++++----
>  block.c                   |  4 ++--
>  block/blkdebug.c          |  4 ++--
>  block/bochs.c             |  2 +-
>  block/cloop.c             |  2 +-
>  block/dmg.c               |  2 +-
>  block/io.c                | 12 +++++++-----
>  block/iscsi.c             |  2 +-
>  block/raw-posix.c         | 16 ++++++++--------
>  block/raw-win32.c         |  6 +++---
>  block/vvfat.c             |  2 +-
>  11 files changed, 35 insertions(+), 29 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9dc7af4..0bc7ff4 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -322,6 +322,12 @@ struct BlockDriver {
>  };
> 
>  typedef struct BlockLimits {
> +    /* Alignment requirement, in bytes, for offset/length of I/O
> +     * requests. Must be a power of 2, and should not exceed
> +     * INT_MAX. For now, a value of 0 defaults to 512, but may change
> +     * to default to 1. */
> +    uint32_t request_alignment;
> +
>      /* maximum number of bytes that can be discarded at once (since it
>       * is signed, it must be < 2G, if set), should be multiple of
>       * pdiscard_alignment, but need not be power of 2. May be 0 if no
> @@ -353,10 +359,10 @@ typedef struct BlockLimits {
>       * should be multiple of opt_transfer), or 0 for no 32-bit limit */
>      uint32_t max_transfer;
> 
> -    /* memory alignment so that no bounce buffer is needed */
> +    /* memory alignment, in bytes so that no bounce buffer is needed */
>      size_t min_mem_alignment;
> 
> -    /* memory alignment for bounce buffer */
> +    /* memory alignment, in bytes, for bounce buffer */
>      size_t opt_mem_alignment;
> 
>      /* maximum number of iovec elements */
> @@ -463,8 +469,6 @@ struct BlockDriverState {
>      /* Whether produces zeros when read beyond eof */
>      bool zero_beyond_eof;
> 
> -    /* Alignment requirement for offset/length of I/O requests */
> -    unsigned int request_alignment;
>      /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
>      unsigned int supported_write_flags;
>      /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA,
> diff --git a/block.c b/block.c
> index f54bc25..1b56161 100644
> --- a/block.c
> +++ b/block.c
> @@ -937,7 +937,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BdrvChild *file,
>          goto fail_opts;
>      }
> 
> -    bs->request_alignment = 512;
> +    bs->bl.request_alignment = 512;
>      bs->zero_beyond_eof = true;
>      bs->read_only = !(bs->open_flags & BDRV_O_RDWR);

We don't explicitly initialise any other BlockLimits here, the function
calls bdrv_refresh_limits() to do that.

> diff --git a/block/io.c b/block/io.c
> index e845ef9..aaeedaa 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -71,8 +71,10 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
> **errp)
>  {
>      BlockDriver *drv = bs->drv;
>      Error *local_err = NULL;
> +    uint32_t request_alignment = bs->bl.request_alignment;
> 
>      memset(&bs->bl, 0, sizeof(bs->bl));
> +    bs->bl.request_alignment = request_alignment;

This looks fishy, and it's because you didn't fully convert
request_alignment to work like all the other limits.

You should set the default of 512 here in bdrv_refresh_limits() before
calling into drv->bdrv_refresh_limits() where it can be overridden. No
other fields are set earlier and then preserved here, we start over with
everything.

Of course, this means that drivers that set request_alignment must be
changed so that they don't set it in .bdrv_open(), but only in their
.bdrv_refresh_limits() callback.

Kevin