[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 17/17] block: Move request_alignment into Blo
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2 17/17] block: Move request_alignment into BlockLimit |
Date: |
Wed, 22 Jun 2016 12:14:44 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 22.06.2016 um 00:26 hat Eric Blake geschrieben:
> On 06/21/2016 08:16 AM, Kevin Wolf wrote:
> > Am 14.06.2016 um 23:30 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.
> >>
> >> Signed-off-by: Eric Blake <address@hidden>
> >>
> >> ---
>
> >> struct BlockLimits {
> >> + /* Alignment requirement, in bytes, for offset/length of I/O
> >> + * requests. Must be a power of 2 less than INT_MAX. A value of 0
> >> + * defaults to 1 for drivers with modern byte interfaces, and to
> >> + * 512 otherwise. */
> >
> > No, a value of zero probably crashes qemu. The defaults apply when they
> > aren't overridden by the driver, but this field is always non-zero.
> >
>
> Then why does block.c have:
>
> --- a/block.c
> +++ b/block.c
> @@ -1016,7 +1016,7 @@ static int bdrv_open_common(BlockDriverState *bs,
> BdrvChild *file,
>
> assert(bdrv_opt_mem_align(bs) != 0);
> assert(bdrv_min_mem_align(bs) != 0);
> - assert(is_power_of_2(bs->request_alignment) || bdrv_is_sg(bs));
> + assert(is_power_of_2(bs->bl.request_alignment) || bdrv_is_sg(bs));
>
> That says that if bdrv_is_sg(bs), we can indeed have request_alignment
> be 0. Should I track that down and fix it first, so that
> request_alignment is always nonzero? If so, is using '1' for
> bdrv_is_sg(bs) the ideal solution?
Hm, yes, forgot about this. bdrv_is_sg(bs) means that we never use the
I/O functions, so there is no point in specifying any alignment. If we
want to say something about 0 in the comment maybe mention bs->sg
explicitly and that you can't use read/write functions then.
But in fact, I think even sg devices already get a non-zero alignment,
so the second part of the assertion might be redundant. Didn't test it,
though, just had a quick look at the raw-posix code.
Kevin
pgpYWhnAYpyWW.pgp
Description: PGP signature
[Qemu-devel] [PATCH v2 14/17] block: Switch transfer length bounds to byte-based, Eric Blake, 2016/06/14
[Qemu-devel] [PATCH v2 09/17] iscsi: Set request_alignment during .bdrv_refresh_limits(), Eric Blake, 2016/06/14
[Qemu-devel] [PATCH v2 16/17] block: Split bdrv_merge_limits() from bdrv_refresh_limits(), Eric Blake, 2016/06/14