Re: [Qemu-devel] [PATCH v2 17/17] block: Move request_alignment into BlockLimit

2016-06-22 Thread Kevin Wolf 
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 
> >>
> >> ---
> 
> >>  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


pgp5N4U19TZJ7.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 17/17] block: Move request_alignment into BlockLimit

2016-06-21 Thread Eric Blake 
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 
>>
>> ---

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

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 17/17] block: Move request_alignment into BlockLimit

2016-06-21 Thread Kevin Wolf 
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 
> 
> ---
> v2: drop hacks for save/restore of alignment, now that earlier
> patches handled setting up BlockLimits more sanely
> ---
>  include/block/block_int.h | 22 +-
>  block.c   |  2 +-
>  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/qcow2.c |  2 +-
>  block/raw-posix.c | 16 
>  block/raw-win32.c |  6 +++---
>  block/vvfat.c |  2 +-
>  12 files changed, 39 insertions(+), 35 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 88ef826..77f47d9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -324,6 +324,12 @@ struct BlockDriver {
>  };
> 
>  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.

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

Kevin

Re: [Qemu-devel] [PATCH v2 17/17] block: Move request_alignment into BlockLimit

2016-06-15 Thread Fam Zheng 
On Tue, 06/14 15:30, Eric Blake wrote:
> 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 

Reviewed-by: Fam Zheng

[Qemu-devel] [PATCH v2 17/17] block: Move request_alignment into BlockLimit

2016-06-14 Thread Eric Blake 
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 

---
v2: drop hacks for save/restore of alignment, now that earlier
patches handled setting up BlockLimits more sanely
---
 include/block/block_int.h | 22 +-
 block.c   |  2 +-
 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/qcow2.c |  2 +-
 block/raw-posix.c | 16 
 block/raw-win32.c |  6 +++---
 block/vvfat.c |  2 +-
 12 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 88ef826..77f47d9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -324,6 +324,12 @@ struct BlockDriver {
 };

 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. */
+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
@@ -332,8 +338,8 @@ struct BlockLimits {

 /* optimal alignment for discard requests in bytes, must be power
  * of 2, less than max_discard if that is set, and multiple of
- * bs->request_alignment. May be 0 if bs->request_alignment is
- * good enough */
+ * bl.request_alignment. May be 0 if bl.request_alignment is good
+ * enough */
 uint32_t pdiscard_alignment;

 /* maximum number of bytes that can zeroized at once (since it is
@@ -343,12 +349,12 @@ struct BlockLimits {

 /* optimal alignment for write zeroes requests in bytes, must be
  * power of 2, less than max_pwrite_zeroes if that is set, and
- * multiple of bs->request_alignment. May be 0 if
- * bs->request_alignment is good enough */
+ * multiple of bl.request_alignment. May be 0 if
+ * bl.request_alignment is good enough */
 uint32_t pwrite_zeroes_alignment;

 /* optimal transfer length in bytes (must be power of 2, and
- * multiple of bs->request_alignment), or 0 if no preferred size */
+ * multiple of bl.request_alignment), or 0 if no preferred size */
 uint32_t opt_transfer;

 /* maximal transfer length in bytes (need not be power of 2, but
@@ -356,10 +362,10 @@ struct BlockLimits {
  * For now, anything larger than INT_MAX is clamped down. */
 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 {
 /* I/O Limits */
 BlockLimits bl;

-/* 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 61fe1df..d578df8 100644
--- 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));

 qemu_opts_del(opts);
 return 0;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 1589fa7..5e32643 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -384,7 +384,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,

 /* Set request alignment */
 align = qemu_opt_get_size(opts, "align",
-  bs->request_alignment ?: BDRV_SECTOR_SIZE);
+  bs->bl.request_alignment ?: BDRV_SECTOR_SIZE);
 if (align > 0 && align < INT_MAX && is_power_of_2(align)) {
 s->align = align;
 } else {
@@ -727,7 +727,7 @@ static void blkdebug_refresh_limits(BlockDriverState *bs, 
Error **errp)
 BDRVBlkdebugState *s = bs->opaque;

 if (s->align) {
-bs->request_alignment = s->align;
+bs->bl.request_alignment = s->align;
 }
 }

diff --git a/block/bochs.c b/block/bochs.c
index 182c50b..4194f1d 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -190,7 +190,7 @@ fail:

 static void