diff mbox

Regression: block: Add .bdrv_co_pwrite_zeroes()

Message ID 577BB031.8000902@redhat.com
State New
Headers show

Commit Message

Eric Blake July 5, 2016, 1:03 p.m. UTC 
On 07/05/2016 01:30 AM, Peter Lieven wrote:
> Am 05.07.2016 um 03:53 schrieb Eric Blake:
>> On 07/04/2016 07:49 AM, Peter Lieven wrote:
>>> Hi,
>>>
>>> the above commit:
>>>
>>> commit d05aa8bb4a8b6aa9a915ec5074fb12ae632d2323
>>> Author: Eric Blake <eblake@redhat.com>
>>> Date:   Wed Jun 1 15:10:03 2016 -0600
>>>
>>>      block: Add .bdrv_co_pwrite_zeroes()
>>>
>>> introduces a regression (at least for me).
>>>
>>> The Limits from the iSCSI Block Limits VPD have no requirement of being
>>> a power of two.
>>> We use Dell Equallogic iSCSI SANs for instance. They have an internal
>>> page size of 15MB. And
>>> they advertise this page size as max_ws_len, opt_transfer_len and
>>> opt_discard_alignment.
>> A non-power-of-2 max_ws_len shouldn't be a problem, but opt_transfer_len
>> and opt_discard_alignment not being a power of 2 impacts other code.
>> 15MB is a rather odd page size.
> 
> I know, not my idea ;-) I think at least opt_discard_alignment of 15MB
> used to work
> before.

Does this fix it for you?


 /* Note that this will not re-establish a connection with an iSCSI
target - it


One of those two hunks is covered by my pending 'block: Switch discard
length bounds to byte-based' making its way through the block queue
review, so if it works for you, I just need to formalize the patch for
the pwrite_zeroes_alignment.

Comments

Paolo Bonzini July 5, 2016, 1:39 p.m. UTC | #1 
On 05/07/2016 15:03, Eric Blake wrote:
>          bs->bl.pwrite_zeroes_alignment =
> -            iscsilun->bl.opt_unmap_gran * iscsilun->block_size;
> +            pow2floor(iscsilun->bl.opt_unmap_gran * iscsilun->block_size);
>      } else {
>          bs->bl.pwrite_zeroes_alignment = iscsilun->block_size;
>      }
>      bs->bl.opt_transfer_length =
> -        sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len, iscsilun);
> +        pow2floor(sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len,
> iscsilun));

I see no reason why the alignment needs to be a power of two in
block/io.c, if you use / % * instead of &.

Paolo
diff mbox

Patch

diff --git i/block/iscsi.c w/block/iscsi.c
index 9bb5ff6..556486c 100644
--- i/block/iscsi.c
+++ w/block/iscsi.c
@@ -1732,12 +1732,12 @@  static void
iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
     }
     if (iscsilun->lbp.lbpws) {
         bs->bl.pwrite_zeroes_alignment =
-            iscsilun->bl.opt_unmap_gran * iscsilun->block_size;
+            pow2floor(iscsilun->bl.opt_unmap_gran * iscsilun->block_size);
     } else {
         bs->bl.pwrite_zeroes_alignment = iscsilun->block_size;
     }
     bs->bl.opt_transfer_length =
-        sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len, iscsilun);
+        pow2floor(sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len,
iscsilun));
 }