[Qemu-devel] [PATCH 0/2] scsi: add block job opblockers for scsi-block

[Qemu-devel] [PATCH 0/2] scsi: add block job opblockers for scsi-block

[Paolo Bonzini]

SCSI passthrough bypasses the block layer and issues SCSI commands
directly to the disk.  This breaks write notifiers and dirty bitmaps,
so that scsi-block devices cannot act as a mirror or backup source
(and commit too, even though that shouldn't be possible at all in the
lack of a backing file).  This series adds op blockers for that purpose.

There is currently a blk_op_unblock but no blk_op_block, so patch 2
adds it.

Paolo

Paolo Bonzini (2):
  scsi: add unrealize method for SCSI devices
  scsi: add block job opblockers for scsi-block

 block/block-backend.c          |  9 ++++++
 hw/scsi/scsi-bus.c             |  4 +++
 hw/scsi/scsi-disk.c            | 62 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/scsi/scsi.h         |  1 +
 include/sysemu/block-backend.h |  1 +
 5 files changed, 77 insertions(+)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] scsi: add unrealize method for SCSI devices

[Paolo Bonzini]

The next patch will introduce a different unrealize implementation
for scsi-block.  Compared to the code before commit fb7b5c0df6
("scsi: devirtualize unrealize of SCSI devices", 2014-10-31), the
common code for all SCSI devices is kept in scsi-bus.c.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c     | 4 ++++
 include/hw/scsi/scsi.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 05e501efd3..a0790438f0 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -212,12 +212,16 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
 static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
 {
     SCSIDevice *dev = SCSI_DEVICE(qdev);
+    SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(dev);
 
     if (dev->vmsentry) {
         qemu_del_vm_change_state_handler(dev->vmsentry);
     }
 
     scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE));
+    if (sc->unrealize) {
+        sc->unrealize(dev, errp);
+    }
     blockdev_mark_auto_del(dev->conf.blk);
 }
 
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 802a647cdc..569a4b9d14 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -59,6 +59,7 @@ struct SCSIRequest {
 typedef struct SCSIDeviceClass {
     DeviceClass parent_class;
     void (*realize)(SCSIDevice *dev, Error **errp);
+    void (*unrealize)(SCSIDevice *dev, Error **errp);
     int (*parse_cdb)(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                      void *hba_private);
     SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/2] scsi: add unrealize method for SCSI devices

[Fam Zheng]

On Wed, 02/07 17:36, Paolo Bonzini wrote:
> The next patch will introduce a different unrealize implementation
> for scsi-block.  Compared to the code before commit fb7b5c0df6
> ("scsi: devirtualize unrealize of SCSI devices", 2014-10-31), the
> common code for all SCSI devices is kept in scsi-bus.c.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Paolo Bonzini]

scsi-block bypasses the dirty bitmaps and pre-write notifiers, so it
cannot be the source of a block job.  The gist of the fix is to add
op-blockers to the BlockBackend, and remove them at "unrealize" time,
but things are a little more complex because quit closes the BlockBackend
without going through unrealize.

So use Notifiers: the remove_bs notifier is called by bdrv_close_all, and
the insert_bs notifier might not be really necessary but make things a
little more symmetric.

Suggested-by: Karen Noel <knoel@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/block-backend.c          |  9 ++++++
 hw/scsi/scsi-disk.c            | 62 ++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/block-backend.h |  1 +
 3 files changed, 72 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index baef8e7abc..1759639a4a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1747,6 +1747,15 @@ bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp)
     return bdrv_op_is_blocked(bs, op, errp);
 }
 
+void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason)
+{
+    BlockDriverState *bs = blk_bs(blk);
+
+    if (bs) {
+        bdrv_op_block(bs, op, reason);
+    }
+}
+
 void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason)
 {
     BlockDriverState *bs = blk_bs(blk);
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 49d2559d93..023673cb04 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2578,9 +2578,39 @@ static int get_device_type(SCSIDiskState *s)
     return 0;
 }
 
+typedef struct SCSIBlockState {
+    SCSIDiskState sd;
+    Error *mirror_source;
+    Error *backup_source;
+    Error *commit_source;
+    Notifier insert_bs;
+    Notifier remove_bs;
+} SCSIBlockState;
+
+static void scsi_block_insert_bs(Notifier *n, void *opaque)
+{
+    SCSIBlockState *sb = container_of(n, SCSIBlockState, insert_bs);
+    SCSIDiskState *s = &sb->sd;
+
+    blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, sb->mirror_source);
+    blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, sb->commit_source);
+    blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, sb->backup_source);
+}
+
+static void scsi_block_remove_bs(Notifier *n, void *opaque)
+{
+    SCSIBlockState *sb = container_of(n, SCSIBlockState, remove_bs);
+    SCSIDiskState *s = &sb->sd;
+
+    blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, sb->mirror_source);
+    blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, sb->commit_source);
+    blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, sb->backup_source);
+}
+
 static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s);
     int sg_version;
     int rc;
 
@@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 
     scsi_realize(&s->qdev, errp);
     scsi_generic_read_device_identification(&s->qdev);
+
+    /* For op blockers, due to lack of support for dirty bitmaps.  */
+    error_setg(&sb->mirror_source,
+               "scsi-block does not support acting as a mirroring source");
+    error_setg(&sb->commit_source,
+               "scsi-block does not support acting as an active commit source");
+
+    /* For op blockers, due to lack of support for write notifiers.  */
+    error_setg(&sb->backup_source,
+               "scsi-block does not support acting as a backup source");
+
+    sb->insert_bs.notify = scsi_block_insert_bs;
+    blk_add_insert_bs_notifier(s->qdev.conf.blk, &sb->insert_bs);
+    sb->remove_bs.notify = scsi_block_remove_bs;
+    blk_add_remove_bs_notifier(s->qdev.conf.blk, &sb->remove_bs);
+
+    scsi_block_insert_bs(&sb->insert_bs, s->qdev.conf.blk);
+}
+
+static void scsi_block_unrealize(SCSIDevice *dev, Error **errp)
+{
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s);
+
+    notifier_remove(&sb->insert_bs);
+    notifier_remove(&sb->remove_bs);
+    scsi_block_remove_bs(&sb->insert_bs, s->qdev.conf.blk);
+    error_free(sb->mirror_source);
+    error_free(sb->commit_source);
+    error_free(sb->backup_source);
 }
 
 typedef struct SCSIBlockReq {
@@ -3017,6 +3077,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
     SCSIDiskClass *sdc = SCSI_DISK_BASE_CLASS(klass);
 
     sc->realize      = scsi_block_realize;
+    sc->unrealize    = scsi_block_unrealize;
     sc->alloc_req    = scsi_block_new_request;
     sc->parse_cdb    = scsi_block_parse_cdb;
     sdc->dma_readv   = scsi_block_dma_readv;
@@ -3031,6 +3092,7 @@ static const TypeInfo scsi_block_info = {
     .name          = "scsi-block",
     .parent        = TYPE_SCSI_DISK_BASE,
     .class_init    = scsi_block_class_initfn,
+    .instance_size = sizeof(SCSIBlockState),
 };
 #endif
 
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c4e52a5fa3..a48a49ca79 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -182,6 +182,7 @@ void blk_set_guest_block_size(BlockBackend *blk, int align);
 void *blk_try_blockalign(BlockBackend *blk, size_t size);
 void *blk_blockalign(BlockBackend *blk, size_t size);
 bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp);
+void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason);
 void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason);
 void blk_op_block_all(BlockBackend *blk, Error *reason);
 void blk_op_unblock_all(BlockBackend *blk, Error *reason);
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Fam Zheng]

On Wed, 02/07 17:36, Paolo Bonzini wrote:
> scsi-block bypasses the dirty bitmaps and pre-write notifiers, so it
> cannot be the source of a block job.  The gist of the fix is to add
> op-blockers to the BlockBackend, and remove them at "unrealize" time,
> but things are a little more complex because quit closes the BlockBackend
> without going through unrealize.
> 
> So use Notifiers: the remove_bs notifier is called by bdrv_close_all, and
> the insert_bs notifier might not be really necessary but make things a
> little more symmetric.
> 
> Suggested-by: Karen Noel <knoel@redhat.com>

:)

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Though I have one comment below.

> ---
>  block/block-backend.c          |  9 ++++++
>  hw/scsi/scsi-disk.c            | 62 ++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/block-backend.h |  1 +
>  3 files changed, 72 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index baef8e7abc..1759639a4a 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1747,6 +1747,15 @@ bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp)
>      return bdrv_op_is_blocked(bs, op, errp);
>  }
>  
> +void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason)
> +{
> +    BlockDriverState *bs = blk_bs(blk);
> +
> +    if (bs) {
> +        bdrv_op_block(bs, op, reason);
> +    }
> +}
> +
>  void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason)
>  {
>      BlockDriverState *bs = blk_bs(blk);
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 49d2559d93..023673cb04 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2578,9 +2578,39 @@ static int get_device_type(SCSIDiskState *s)
>      return 0;
>  }
>  
> +typedef struct SCSIBlockState {
> +    SCSIDiskState sd;
> +    Error *mirror_source;
> +    Error *backup_source;
> +    Error *commit_source;
> +    Notifier insert_bs;
> +    Notifier remove_bs;
> +} SCSIBlockState;
> +
> +static void scsi_block_insert_bs(Notifier *n, void *opaque)
> +{
> +    SCSIBlockState *sb = container_of(n, SCSIBlockState, insert_bs);
> +    SCSIDiskState *s = &sb->sd;
> +
> +    blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, sb->mirror_source);
> +    blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, sb->commit_source);
> +    blk_op_block(s->qdev.conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, sb->backup_source);
> +}
> +
> +static void scsi_block_remove_bs(Notifier *n, void *opaque)
> +{
> +    SCSIBlockState *sb = container_of(n, SCSIBlockState, remove_bs);
> +    SCSIDiskState *s = &sb->sd;
> +
> +    blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_MIRROR_SOURCE, sb->mirror_source);
> +    blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, sb->commit_source);
> +    blk_op_unblock(s->qdev.conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, sb->backup_source);
> +}
> +
>  static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> +    SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s);
>      int sg_version;
>      int rc;
>  
> @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>  
>      scsi_realize(&s->qdev, errp);
>      scsi_generic_read_device_identification(&s->qdev);
> +
> +    /* For op blockers, due to lack of support for dirty bitmaps.  */
> +    error_setg(&sb->mirror_source,
> +               "scsi-block does not support acting as a mirroring source");
> +    error_setg(&sb->commit_source,
> +               "scsi-block does not support acting as an active commit source");

An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error message
will not be as nice but it can be useful for another (blockjob) operation that
requires dirty bitmap support, or another device that doesn't support dirty
bitmaps. Though there isn't one for now.

> +
> +    /* For op blockers, due to lack of support for write notifiers.  */
> +    error_setg(&sb->backup_source,
> +               "scsi-block does not support acting as a backup source");
> +
> +    sb->insert_bs.notify = scsi_block_insert_bs;
> +    blk_add_insert_bs_notifier(s->qdev.conf.blk, &sb->insert_bs);
> +    sb->remove_bs.notify = scsi_block_remove_bs;
> +    blk_add_remove_bs_notifier(s->qdev.conf.blk, &sb->remove_bs);
> +
> +    scsi_block_insert_bs(&sb->insert_bs, s->qdev.conf.blk);
> +}
> +
> +static void scsi_block_unrealize(SCSIDevice *dev, Error **errp)
> +{
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> +    SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s);
> +
> +    notifier_remove(&sb->insert_bs);
> +    notifier_remove(&sb->remove_bs);
> +    scsi_block_remove_bs(&sb->insert_bs, s->qdev.conf.blk);
> +    error_free(sb->mirror_source);
> +    error_free(sb->commit_source);
> +    error_free(sb->backup_source);
>  }
>  
>  typedef struct SCSIBlockReq {
> @@ -3017,6 +3077,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
>      SCSIDiskClass *sdc = SCSI_DISK_BASE_CLASS(klass);
>  
>      sc->realize      = scsi_block_realize;
> +    sc->unrealize    = scsi_block_unrealize;
>      sc->alloc_req    = scsi_block_new_request;
>      sc->parse_cdb    = scsi_block_parse_cdb;
>      sdc->dma_readv   = scsi_block_dma_readv;
> @@ -3031,6 +3092,7 @@ static const TypeInfo scsi_block_info = {
>      .name          = "scsi-block",
>      .parent        = TYPE_SCSI_DISK_BASE,
>      .class_init    = scsi_block_class_initfn,
> +    .instance_size = sizeof(SCSIBlockState),
>  };
>  #endif
>  
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index c4e52a5fa3..a48a49ca79 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -182,6 +182,7 @@ void blk_set_guest_block_size(BlockBackend *blk, int align);
>  void *blk_try_blockalign(BlockBackend *blk, size_t size);
>  void *blk_blockalign(BlockBackend *blk, size_t size);
>  bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp);
> +void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason);
>  void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason);
>  void blk_op_block_all(BlockBackend *blk, Error *reason);
>  void blk_op_unblock_all(BlockBackend *blk, Error *reason);
> -- 
> 2.14.3
> 
> 

Fam

Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Paolo Bonzini]

On 08/02/2018 02:35, Fam Zheng wrote:
> On Wed, 02/07 17:36, Paolo Bonzini wrote:
>> @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>>  
>>      scsi_realize(&s->qdev, errp);
>>      scsi_generic_read_device_identification(&s->qdev);
>> +
>> +    /* For op blockers, due to lack of support for dirty bitmaps.  */
>> +    error_setg(&sb->mirror_source,
>> +               "scsi-block does not support acting as a mirroring source");
>> +    error_setg(&sb->commit_source,
>> +               "scsi-block does not support acting as an active commit source");
> 
> An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error message
> will not be as nice but it can be useful for another (blockjob) operation that
> requires dirty bitmap support, or another device that doesn't support dirty
> bitmaps. Though there isn't one for now.

Yeah, I thought about it.  Another possibility is make BLOCK_OP_TYPE_* a
bitmask.  Then you can easily add a single Error * for multiple
blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as
BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for
notifiers below.

Paolo

>> +
>> +    /* For op blockers, due to lack of support for write notifiers.  */
>> +    error_setg(&sb->backup_source,
>> +               "scsi-block does not support acting as a backup source");
>> +
>> +    sb->insert_bs.notify = scsi_block_insert_bs;
>> +    blk_add_insert_bs_notifier(s->qdev.conf.blk, &sb->insert_bs);
>> +    sb->remove_bs.notify = scsi_block_remove_bs;
>> +    blk_add_remove_bs_notifier(s->qdev.conf.blk, &sb->remove_bs);
>> +
>> +    scsi_block_insert_bs(&sb->insert_bs, s->qdev.conf.blk);
>> +}
>> +
>> +static void scsi_block_unrealize(SCSIDevice *dev, Error **errp)
>> +{
>> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
>> +    SCSIBlockState *sb = DO_UPCAST(SCSIBlockState, sd, s);
>> +
>> +    notifier_remove(&sb->insert_bs);
>> +    notifier_remove(&sb->remove_bs);
>> +    scsi_block_remove_bs(&sb->insert_bs, s->qdev.conf.blk);
>> +    error_free(sb->mirror_source);
>> +    error_free(sb->commit_source);
>> +    error_free(sb->backup_source);
>>  }
>>  
>>  typedef struct SCSIBlockReq {
>> @@ -3017,6 +3077,7 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
>>      SCSIDiskClass *sdc = SCSI_DISK_BASE_CLASS(klass);
>>  
>>      sc->realize      = scsi_block_realize;
>> +    sc->unrealize    = scsi_block_unrealize;
>>      sc->alloc_req    = scsi_block_new_request;
>>      sc->parse_cdb    = scsi_block_parse_cdb;
>>      sdc->dma_readv   = scsi_block_dma_readv;
>> @@ -3031,6 +3092,7 @@ static const TypeInfo scsi_block_info = {
>>      .name          = "scsi-block",
>>      .parent        = TYPE_SCSI_DISK_BASE,
>>      .class_init    = scsi_block_class_initfn,
>> +    .instance_size = sizeof(SCSIBlockState),
>>  };
>>  #endif
>>  
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> index c4e52a5fa3..a48a49ca79 100644
>> --- a/include/sysemu/block-backend.h
>> +++ b/include/sysemu/block-backend.h
>> @@ -182,6 +182,7 @@ void blk_set_guest_block_size(BlockBackend *blk, int align);
>>  void *blk_try_blockalign(BlockBackend *blk, size_t size);
>>  void *blk_blockalign(BlockBackend *blk, size_t size);
>>  bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp);
>> +void blk_op_block(BlockBackend *blk, BlockOpType op, Error *reason);
>>  void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason);
>>  void blk_op_block_all(BlockBackend *blk, Error *reason);
>>  void blk_op_unblock_all(BlockBackend *blk, Error *reason);
>> -- 
>> 2.14.3
>>
>>
> 
> Fam
> 

Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Kevin Wolf]

Am 08.02.2018 um 11:42 hat Paolo Bonzini geschrieben:
> On 08/02/2018 02:35, Fam Zheng wrote:
> > On Wed, 02/07 17:36, Paolo Bonzini wrote:
> >> @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
> >>  
> >>      scsi_realize(&s->qdev, errp);
> >>      scsi_generic_read_device_identification(&s->qdev);
> >> +
> >> +    /* For op blockers, due to lack of support for dirty bitmaps.  */
> >> +    error_setg(&sb->mirror_source,
> >> +               "scsi-block does not support acting as a mirroring source");
> >> +    error_setg(&sb->commit_source,
> >> +               "scsi-block does not support acting as an active commit source");
> > 
> > An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error message
> > will not be as nice but it can be useful for another (blockjob) operation that
> > requires dirty bitmap support, or another device that doesn't support dirty
> > bitmaps. Though there isn't one for now.
> 
> Yeah, I thought about it.  Another possibility is make BLOCK_OP_TYPE_* a
> bitmask.  Then you can easily add a single Error * for multiple
> blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as
> BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for
> notifiers below.

We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
find the time yet to remove the existing ones, but any new protections
should be using the permission system.

I propose a new BLK_PERM_BYPASS that allows its users to bypass the
block layer I/O functions. In other words, bdrv_aio_ioctl() would
require that you got this permission. A dirty bitmap would keep a
BdrvChild with perm=0, shared=BLK_PERM_ALL & ~BLK_PERM_BYPASS, so you
can never have a dirty bitmap and a device using ioctls attached to the
BDS at the same time.

Kevin

Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Paolo Bonzini]

On 12/02/2018 14:52, Kevin Wolf wrote:
> Am 08.02.2018 um 11:42 hat Paolo Bonzini geschrieben:
>> On 08/02/2018 02:35, Fam Zheng wrote:
>>> On Wed, 02/07 17:36, Paolo Bonzini wrote:
>>>> @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>>>>  
>>>>      scsi_realize(&s->qdev, errp);
>>>>      scsi_generic_read_device_identification(&s->qdev);
>>>> +
>>>> +    /* For op blockers, due to lack of support for dirty bitmaps.  */
>>>> +    error_setg(&sb->mirror_source,
>>>> +               "scsi-block does not support acting as a mirroring source");
>>>> +    error_setg(&sb->commit_source,
>>>> +               "scsi-block does not support acting as an active commit source");
>>>
>>> An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error message
>>> will not be as nice but it can be useful for another (blockjob) operation that
>>> requires dirty bitmap support, or another device that doesn't support dirty
>>> bitmaps. Though there isn't one for now.
>>
>> Yeah, I thought about it.  Another possibility is make BLOCK_OP_TYPE_* a
>> bitmask.  Then you can easily add a single Error * for multiple
>> blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as
>> BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for
>> notifiers below.
> 
> We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
> find the time yet to remove the existing ones, but any new protections
> should be using the permission system.

I agree.  But does this include not fixing bugs wherever clients are
using the old op blockers?

Paolo

> I propose a new BLK_PERM_BYPASS that allows its users to bypass the
> block layer I/O functions. In other words, bdrv_aio_ioctl() would
> require that you got this permission. A dirty bitmap would keep a
> BdrvChild with perm=0, shared=BLK_PERM_ALL & ~BLK_PERM_BYPASS, so you
> can never have a dirty bitmap and a device using ioctls attached to the
> BDS at the same time.

Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Kevin Wolf]

Am 12.02.2018 um 15:00 hat Paolo Bonzini geschrieben:
> On 12/02/2018 14:52, Kevin Wolf wrote:
> > Am 08.02.2018 um 11:42 hat Paolo Bonzini geschrieben:
> >> On 08/02/2018 02:35, Fam Zheng wrote:
> >>> On Wed, 02/07 17:36, Paolo Bonzini wrote:
> >>>> @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
> >>>>  
> >>>>      scsi_realize(&s->qdev, errp);
> >>>>      scsi_generic_read_device_identification(&s->qdev);
> >>>> +
> >>>> +    /* For op blockers, due to lack of support for dirty bitmaps.  */
> >>>> +    error_setg(&sb->mirror_source,
> >>>> +               "scsi-block does not support acting as a mirroring source");
> >>>> +    error_setg(&sb->commit_source,
> >>>> +               "scsi-block does not support acting as an active commit source");
> >>>
> >>> An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error message
> >>> will not be as nice but it can be useful for another (blockjob) operation that
> >>> requires dirty bitmap support, or another device that doesn't support dirty
> >>> bitmaps. Though there isn't one for now.
> >>
> >> Yeah, I thought about it.  Another possibility is make BLOCK_OP_TYPE_* a
> >> bitmask.  Then you can easily add a single Error * for multiple
> >> blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as
> >> BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for
> >> notifiers below.
> > 
> > We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
> > find the time yet to remove the existing ones, but any new protections
> > should be using the permission system.
> 
> I agree.  But does this include not fixing bugs wherever clients are
> using the old op blockers?

I'm not saying that we shouldn't fix the bug, just that we should fix it
properly with the best infrastructure we have.

The old op blockers are "fixing" the problem at the symptom level, and
you have to check for each high-level operation if it does something
problematic internally. You have to repeat this analysis every time you
add a new operation or modifiy an existing one (which noone ever does).
The risk that this breaks sooner or later is pretty high.

The new permission system, on the other hand, directly addresses the
root cause, and any new feature that uses dirty bitmaps will then
automatically get the protection, too.

So in fact, I would say that the bug isn't really fixed (but at best
papered over) until we add a proper fix on the permission level.

Kevin

Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Paolo Bonzini]

On 12/02/2018 15:30, Kevin Wolf wrote:
>>> We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
>>> find the time yet to remove the existing ones, but any new protections
>>> should be using the permission system.
>> I agree.  But does this include not fixing bugs wherever clients are
>> using the old op blockers?
> I'm not saying that we shouldn't fix the bug, just that we should fix it
> properly with the best infrastructure we have.
> 
> The old op blockers are "fixing" the problem at the symptom level, and
> you have to check for each high-level operation if it does something
> problematic internally. You have to repeat this analysis every time you
> add a new operation or modifiy an existing one (which noone ever does).
> The risk that this breaks sooner or later is pretty high.
> 
> The new permission system, on the other hand, directly addresses the
> root cause, and any new feature that uses dirty bitmaps will then
> automatically get the protection, too.
> 
> So in fact, I would say that the bug isn't really fixed (but at best
> papered over) until we add a proper fix on the permission level.

Okay, we are in agreement about this and you expressed very well why I
(at the gut feeling level) didn't like the old op blockers.  But you
bypassed the real question, which is: should I send a pull request for
these two patches or not? :)

Paolo

Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Kevin Wolf]

Am 12.02.2018 um 15:32 hat Paolo Bonzini geschrieben:
> On 12/02/2018 15:30, Kevin Wolf wrote:
> >>> We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
> >>> find the time yet to remove the existing ones, but any new protections
> >>> should be using the permission system.
> >> I agree.  But does this include not fixing bugs wherever clients are
> >> using the old op blockers?
> > I'm not saying that we shouldn't fix the bug, just that we should fix it
> > properly with the best infrastructure we have.
> > 
> > The old op blockers are "fixing" the problem at the symptom level, and
> > you have to check for each high-level operation if it does something
> > problematic internally. You have to repeat this analysis every time you
> > add a new operation or modifiy an existing one (which noone ever does).
> > The risk that this breaks sooner or later is pretty high.
> > 
> > The new permission system, on the other hand, directly addresses the
> > root cause, and any new feature that uses dirty bitmaps will then
> > automatically get the protection, too.
> > 
> > So in fact, I would say that the bug isn't really fixed (but at best
> > papered over) until we add a proper fix on the permission level.
> 
> Okay, we are in agreement about this and you expressed very well why I
> (at the gut feeling level) didn't like the old op blockers.  But you
> bypassed the real question, which is: should I send a pull request for
> these two patches or not? :)

I didn't spell it out that explicitly, but this is essentially a NACK.
I'd very much prefer if you could replace it with the proper solution.
Of course, we can always make exceptions when there is a good reason,
but with 2.12 still two months away, I doubt we have one.

Kevin

Re: [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Paolo Bonzini]

On 12/02/2018 15:48, Kevin Wolf wrote:
> Am 12.02.2018 um 15:32 hat Paolo Bonzini geschrieben:
>> Okay, we are in agreement about this and you expressed very well why I
>> (at the gut feeling level) didn't like the old op blockers.  But you
>> bypassed the real question, which is: should I send a pull request for
>> these two patches or not? :)
> 
> I didn't spell it out that explicitly, but this is essentially a NACK.
> I'd very much prefer if you could replace it with the proper solution.
> Of course, we can always make exceptions when there is a good reason,
> but with 2.12 still two months away, I doubt we have one.

Ok, I don't mind explicitness.  I'll keep these two patches in the queue
for now.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Paolo Bonzini]

On 12/02/2018 15:50, Paolo Bonzini wrote:
> On 12/02/2018 15:48, Kevin Wolf wrote:
>> Am 12.02.2018 um 15:32 hat Paolo Bonzini geschrieben:
>>> Okay, we are in agreement about this and you expressed very well why I
>>> (at the gut feeling level) didn't like the old op blockers.  But you
>>> bypassed the real question, which is: should I send a pull request for
>>> these two patches or not? :)
>> I didn't spell it out that explicitly, but this is essentially a NACK.
>> I'd very much prefer if you could replace it with the proper solution.
>> Of course, we can always make exceptions when there is a good reason,
>> but with 2.12 still two months away, I doubt we have one.
> Ok, I don't mind explicitness.  I'll keep these two patches in the queue
> for now.

It's now one month away.  Regarding the solution below:

> I propose a new BLK_PERM_BYPASS that allows its users to bypass the
> block layer I/O functions. In other words, bdrv_aio_ioctl() would
> require that you got this permission. A dirty bitmap would keep a
> BdrvChild with perm=0, shared=BLK_PERM_ALL & ~BLK_PERM_BYPASS, so you
> can never have a dirty bitmap and a device using ioctls attached to the
> BDS at the same time.

I suppose it would be like:

- scsi-block/scsi-generic call blk_set_perm with perm == shared ==
BLK_PERM_BYPASS

- users of dirty bitmaps would call use perm/shared_perm as in your
message above

- dirty bitmaps creation calls bdrv_get_cumulative_perm (which should
now become public) and checks that it doesn't have BLK_PERM_BYPASS in
shared_perm

Anything I'm missing?

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Kevin Wolf]

Am 12.03.2018 um 12:10 hat Paolo Bonzini geschrieben:
> On 12/02/2018 15:50, Paolo Bonzini wrote:
> > On 12/02/2018 15:48, Kevin Wolf wrote:
> >> Am 12.02.2018 um 15:32 hat Paolo Bonzini geschrieben:
> >>> Okay, we are in agreement about this and you expressed very well why I
> >>> (at the gut feeling level) didn't like the old op blockers.  But you
> >>> bypassed the real question, which is: should I send a pull request for
> >>> these two patches or not? :)
> >> I didn't spell it out that explicitly, but this is essentially a NACK.
> >> I'd very much prefer if you could replace it with the proper solution.
> >> Of course, we can always make exceptions when there is a good reason,
> >> but with 2.12 still two months away, I doubt we have one.
> > Ok, I don't mind explicitness.  I'll keep these two patches in the queue
> > for now.
> 
> It's now one month away.  Regarding the solution below:
> 
> > I propose a new BLK_PERM_BYPASS that allows its users to bypass the
> > block layer I/O functions. In other words, bdrv_aio_ioctl() would
> > require that you got this permission. A dirty bitmap would keep a
> > BdrvChild with perm=0, shared=BLK_PERM_ALL & ~BLK_PERM_BYPASS, so you
> > can never have a dirty bitmap and a device using ioctls attached to the
> > BDS at the same time.
> 
> I suppose it would be like:
> 
> - scsi-block/scsi-generic call blk_set_perm with perm == shared ==
> BLK_PERM_BYPASS

perm = BLK_PERM_BYPASS is fine, but for shared it seems overly
restrictive. I don't think the device minds another user accessing the
device.

Other block devices do this in blkconf_apply_backend_options():

    shared_perm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
                  BLK_PERM_GRAPH_MOD;
    if (resizable) {
        shared_perm |= BLK_PERM_RESIZE;
    }
    if (conf->share_rw) {
        shared_perm |= BLK_PERM_WRITE;
    }

I suppose scsi-generic is never resizable, so that part can go away, but
we do have a share-rw qdev property that can be used.

> - users of dirty bitmaps would call use perm/shared_perm as in your
> message above
> 
> - dirty bitmaps creation calls bdrv_get_cumulative_perm (which should
> now become public) and checks that it doesn't have BLK_PERM_BYPASS in
> shared_perm

My proposal was really that users of dirty bitmaps don't change
anything, but we do everything in the dirty bitmap implementation. Dirty
bitmap creation would add a BdrvChild with the above permissions.
Deleting a dirty bitmap would remove the BdrvChild again.

Then you don't need to manually call bdrv_get_cumulative_perm(), because
the permission check is included when you attach the BdrvChild.

> Anything I'm missing?

Ideally, bdrv_co_ioctl() should take a BdrvChild instead of a BDS and
assert that the caller correctly requested the permission:

    assert(child->perm & BLK_PERM_BYPASS);

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Paolo Bonzini]

On 12/03/2018 12:58, Kevin Wolf wrote:
>> - users of dirty bitmaps would call use perm/shared_perm as in your
>> message above
>>
>> - dirty bitmaps creation calls bdrv_get_cumulative_perm (which should
>> now become public) and checks that it doesn't have BLK_PERM_BYPASS in
>> shared_perm
> 
> My proposal was really that users of dirty bitmaps don't change
> anything, but we do everything in the dirty bitmap implementation. Dirty
> bitmap creation would add a BdrvChild with the above permissions.
> Deleting a dirty bitmap would remove the BdrvChild again.

This is also better because it works if somebody requests
BLK_PERM_BYPASS after dirty bitmap creation.

However, is there any better way than also creating a dummy BlockDriver
and BlockDriverState?  I first thought of a root role similar to
BlockBackend's, but BdrvChildRole doesn't have a way to inject its own
permissions.  I then tried moving bdrv_child_perm to BdrvChildRole, and
that almost works except that child_backing has special requirements
(mostly due to commit_top and mirror_top's special block drivers).

Perhaps child_perm should be in BdrvChildRole and BlockDriverState
should only have a bdrv_get_backing_perm (called by
child_backing.child_perm).  This makes sense to me since those
permissions are specific to the driver, e.g. whether it has metadata at
all.  But this becomes 2.13 material.

Do you still object to the two opblocker patches?

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] scsi: add block job opblockers for scsi-block

[Kevin Wolf]

Am 05.04.2018 um 13:59 hat Paolo Bonzini geschrieben:
> On 12/03/2018 12:58, Kevin Wolf wrote:
> >> - users of dirty bitmaps would call use perm/shared_perm as in your
> >> message above
> >>
> >> - dirty bitmaps creation calls bdrv_get_cumulative_perm (which should
> >> now become public) and checks that it doesn't have BLK_PERM_BYPASS in
> >> shared_perm
> > 
> > My proposal was really that users of dirty bitmaps don't change
> > anything, but we do everything in the dirty bitmap implementation. Dirty
> > bitmap creation would add a BdrvChild with the above permissions.
> > Deleting a dirty bitmap would remove the BdrvChild again.
> 
> This is also better because it works if somebody requests
> BLK_PERM_BYPASS after dirty bitmap creation.
> 
> However, is there any better way than also creating a dummy BlockDriver
> and BlockDriverState?  I first thought of a root role similar to
> BlockBackend's, but BdrvChildRole doesn't have a way to inject its own
> permissions.  I then tried moving bdrv_child_perm to BdrvChildRole, and
> that almost works except that child_backing has special requirements
> (mostly due to commit_top and mirror_top's special block drivers).

Have a look at block_job_add_bdrv(), which does the same thing to
restrict permissions while a block job is working on the subchain.

Essentially, yes, you'll probably have a new BdrvChildRole (I suppose
with .stay_at_node = true and .get_parent_desc only), but the place
where you specify the permissions is the bdrv_root_attach_child() call.
When you're done, you call bdrv_root_unref_child().

Kevin

> Perhaps child_perm should be in BdrvChildRole and BlockDriverState
> should only have a bdrv_get_backing_perm (called by
> child_backing.child_perm).  This makes sense to me since those
> permissions are specific to the driver, e.g. whether it has metadata at
> all.  But this becomes 2.13 material.
> 
> Do you still object to the two opblocker patches?
> 
> Paolo