[v2,17/17] block: Move request_alignment into BlockLimit

Commit Message

Eric Blake June 21, 2016, 10:26 p.m. UTC

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 <[e-mail]>
>>
>> ---

>>  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:

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?

Comments

Kevin Wolf June 22, 2016, 10:14 a.m. UTC | #1

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 <[e-mail]>
> >>
> >> ---
> 
> >>  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

Patch

--- 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