[Qemu-devel] [PATCH 0/4] virtio: handle zoned backing devices

[Qemu-devel] [PATCH 0/4] virtio: handle zoned backing devices

[Dmitry Fomichev]

Currently, attaching zoned block devices (i.e. storage devices
compliant to ZAC/ZBC standards) using several virtio methods doesn't
work - the zoned devices appear as regular block devices at the guest.
This may cause unexpected i/o errors and, potentially, some data
corruption.

To be more precise, attaching a zoned device via virtio-pci-blk,
virtio-scsi-pci/scsi-disk or virtio-scsi-pci/scsi-hd demonstrates the
above behavior. A simple fix is needed to make
virtio-scsi-pci/scsi-block work and this is covered by a different
patch. The virtio-scsi-pci/scsi-generic method appears to handle zoned
devices without problems.

This patchset adds code to check if the backing device that is being
opened is zoned. If this is the case, the new code prohibits
virtualization of the device for all use cases lacking proper zoned
support.

Considering that there is still a couple of different working ways
to virtualize a ZBD, this patchset provides a reasonable short-term
solution for this problem. What about long term?

It appears to be beneficial to add proper ZBD support to virtio-blk.
In order to support this use case properly, some virtio-blk protocol
changes will be necessary. They are needed to allow the host code to
propagate some ZBD properties that are required for virtio guest
driver to configure the guest block device as ZBD, such as zoned
device model, zone size and the total number of zones. Further, some
support needs to be added for REPORT ZONES command as well as for zone
operations, such as OPEN ZONE, CLOSE ZONE, FINISH ZONE and RESET ZONE.

These additions to the protocol are relatively straightforward, but
they need to be approved by the virtio TC and the whole process may
take some time.

ZBD support for virtio-scsi-pci/scsi-disk and virtio-scsi-pci/scsi-hd
does not seem as necessary. Users will be expected to attach zoned
block devices via virtio-scsi-pci/scsi-block instead.

This patchset contains some Linux-specific code. This code is
necessary to obtain Zoned Block Device model value from Linux sysfs.
Dmitry Fomichev (3):
  block: Add zoned device model property
  raw: Recognize zoned backing devices
  virtio-blk: Don't realize zoned block devices

Shin'ichiro Kawasaki (1):
  hw/scsi: Don't realize zoned block devices for virtio-scsi legacy
    drivers

 block.c                        | 14 +++++++
 block/block-backend.c          | 20 ++++++++++
 block/file-posix.c             | 69 ++++++++++++++++++++++++++++------
 block/raw-format.c             |  8 ++++
 hw/block/virtio-blk.c          |  5 +++
 hw/scsi/scsi-disk.c            |  5 +++
 include/block/block.h          |  9 +++++
 include/block/block_int.h      |  4 ++
 include/sysemu/block-backend.h |  2 +
 9 files changed, 125 insertions(+), 11 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/4] block: Add zoned device model property

[Dmitry Fomichev]

This commit adds Zoned Device Model as defined in T10 ZBC and
T13 ZAC standards, along with some useful accessor functions.

Since the default value of zero in the zoned model field corresponds
to a regular (non-zoned) block device, there is no functionality
change with this commit.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 block.c                        | 14 ++++++++++++++
 block/block-backend.c          | 20 ++++++++++++++++++++
 include/block/block.h          |  9 +++++++++
 include/block/block_int.h      |  4 ++++
 include/sysemu/block-backend.h |  2 ++
 5 files changed, 49 insertions(+)

diff --git a/block.c b/block.c
index c139540f2b..396d999f00 100644
--- a/block.c
+++ b/block.c
@@ -4649,6 +4649,20 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
     *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
 }
 
+uint8_t bdrv_get_zoned_model(BlockDriverState *bs)
+{
+    if (bs->drv->bdrv_get_zoned_info) {
+        bs->drv->bdrv_get_zoned_info(bs);
+    }
+
+    return bs->bl.zoned_model;
+}
+
+uint8_t bdrv_is_zoned(BlockDriverState *bs)
+{
+    return bdrv_get_zoned_model(bs) != BLK_ZONED_NONE;
+}
+
 bool bdrv_is_sg(BlockDriverState *bs)
 {
     return bs->sg;
diff --git a/block/block-backend.c b/block/block-backend.c
index a8d160fd5d..34723f3655 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1438,6 +1438,15 @@ int64_t blk_nb_sectors(BlockBackend *blk)
     return bdrv_nb_sectors(blk_bs(blk));
 }
 
+uint8_t blk_get_zoned_model(BlockBackend *blk)
+{
+    if (!blk_is_available(blk)) {
+        return BLK_ZONED_NONE;
+    }
+
+    return bdrv_get_zoned_model(blk_bs(blk));
+}
+
 BlockAIOCB *blk_aio_preadv(BlockBackend *blk, int64_t offset,
                            QEMUIOVector *qiov, BdrvRequestFlags flags,
                            BlockCompletionFunc *cb, void *opaque)
@@ -1705,6 +1714,17 @@ bool blk_is_sg(BlockBackend *blk)
     return bdrv_is_sg(bs);
 }
 
+bool blk_is_zoned(BlockBackend *blk)
+{
+    BlockDriverState *bs = blk_bs(blk);
+
+    if (!bs) {
+        return false;
+    }
+
+    return bdrv_is_zoned(bs);
+}
+
 bool blk_enable_write_cache(BlockBackend *blk)
 {
     return blk->enable_write_cache;
diff --git a/include/block/block.h b/include/block/block.h
index 734c9d2f76..7aa096afcc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -278,6 +278,13 @@ enum {
 
 char *bdrv_perm_names(uint64_t perm);
 
+/* Known zoned device models */
+enum blk_zoned_model {
+    BLK_ZONED_NONE, /* Regular block device */
+    BLK_ZONED_HA,   /* Host-aware zoned block device */
+    BLK_ZONED_HM,   /* Host-managed zoned block device */
+};
+
 /* disk I/O throttling */
 void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
@@ -354,6 +361,8 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
 BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
                                BlockDriverState *in_bs, Error **errp);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
+uint8_t bdrv_get_zoned_model(BlockDriverState *bs);
+uint8_t bdrv_is_zoned(BlockDriverState *bs);
 void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
 int bdrv_commit(BlockDriverState *bs);
 int bdrv_change_backing_file(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index d6415b53c1..8f35591803 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -416,6 +416,7 @@ struct BlockDriver {
     bool (*bdrv_debug_is_suspended)(BlockDriverState *bs, const char *tag);
 
     void (*bdrv_refresh_limits)(BlockDriverState *bs, Error **errp);
+    void (*bdrv_get_zoned_info)(BlockDriverState *bs);
 
     /*
      * Returns 1 if newly created images are guaranteed to contain only
@@ -614,6 +615,9 @@ typedef struct BlockLimits {
 
     /* maximum number of iovec elements */
     int max_iov;
+
+    /* Zoned device model. Zero value indicates a regular block device */
+    uint8_t zoned_model;
 } BlockLimits;
 
 typedef struct BdrvOpBlocker BdrvOpBlocker;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 733c4957eb..65b6341218 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -156,6 +156,7 @@ int blk_pwrite(BlockBackend *blk, int64_t offset, const void *buf, int bytes,
 int64_t blk_getlength(BlockBackend *blk);
 void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr);
 int64_t blk_nb_sectors(BlockBackend *blk);
+uint8_t blk_get_zoned_model(BlockBackend *blk);
 BlockAIOCB *blk_aio_preadv(BlockBackend *blk, int64_t offset,
                            QEMUIOVector *qiov, BdrvRequestFlags flags,
                            BlockCompletionFunc *cb, void *opaque);
@@ -189,6 +190,7 @@ void blk_error_action(BlockBackend *blk, BlockErrorAction action,
                       bool is_read, int error);
 bool blk_is_read_only(BlockBackend *blk);
 bool blk_is_sg(BlockBackend *blk);
+bool blk_is_zoned(BlockBackend *blk);
 bool blk_enable_write_cache(BlockBackend *blk);
 void blk_set_enable_write_cache(BlockBackend *blk, bool wce);
 void blk_invalidate_cache(BlockBackend *blk, Error **errp);
-- 
2.21.0

[Qemu-devel] [PATCH 2/4] raw: Recognize zoned backing devices

[Dmitry Fomichev]

The purpose of this patch is to recognize a zoned block device (ZBD)
when it is opened as a raw file. The new code initializes the zoned
model propery introduced by the previous commit.

This commit is Linux-specific and in essence it checks the Zoned
Block Device Model (None/Host-managed/Host-aware) value in sysfs on
the host.

In order to avoid code duplication in file-posix.c, a common helper
function is added to read value of a sysfs entry under
/sys/block/<dev>/queue. Now, the existing function that reads the
"max_segments" value and the the new function that reads "zoned"
value share the same helper code.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 block/file-posix.c | 69 ++++++++++++++++++++++++++++++++++++++--------
 block/raw-format.c |  8 ++++++
 2 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index ab05b51a66..dd81dc3301 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1055,33 +1055,53 @@ static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
 #endif
 }
 
-static int hdev_get_max_segments(const struct stat *st)
+static int hdev_read_blk_queue_entry(const struct stat *st, const char *key,
+    char *buf, int buf_len)
 {
 #ifdef CONFIG_LINUX
-    char buf[32];
-    const char *end;
     char *sysfspath;
     int ret;
     int fd = -1;
-    long max_segments;
 
-    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
-                                major(st->st_rdev), minor(st->st_rdev));
+    sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
+                                major(st->st_rdev), minor(st->st_rdev), key);
     fd = open(sysfspath, O_RDONLY);
     if (fd == -1) {
         ret = -errno;
         goto out;
     }
     do {
-        ret = read(fd, buf, sizeof(buf) - 1);
+        ret = read(fd, buf, buf_len - 1);
     } while (ret == -1 && errno == EINTR);
     if (ret < 0) {
         ret = -errno;
-        goto out;
     } else if (ret == 0) {
         ret = -EIO;
+    }
+out:
+    if (fd != -1) {
+        close(fd);
+    }
+    g_free(sysfspath);
+    return ret;
+#else
+    return -ENOTSUP;
+#endif
+}
+
+static int hdev_get_max_segments(const struct stat *st)
+{
+#ifdef CONFIG_LINUX
+    char buf[32];
+    const char *end;
+    int ret;
+    long max_segments;
+
+    ret = hdev_read_blk_queue_entry(st, "max_segments", buf, sizeof(buf));
+    if (ret < 0) {
         goto out;
     }
+
     buf[ret] = 0;
     /* The file is ended with '\n', pass 'end' to accept that. */
     ret = qemu_strtol(buf, &end, 10, &max_segments);
@@ -1090,10 +1110,33 @@ static int hdev_get_max_segments(const struct stat *st)
     }
 
 out:
-    if (fd != -1) {
-        close(fd);
+    return ret;
+#else
+    return -ENOTSUP;
+#endif
+}
+
+static int hdev_get_zoned_model(const struct stat *st)
+{
+#ifdef CONFIG_LINUX
+    char buf[32];
+    int ret;
+
+    ret = hdev_read_blk_queue_entry(st, "zoned", buf, sizeof(buf));
+    if (ret < 0) {
+        ret = BLK_ZONED_NONE;
+        goto out;
     }
-    g_free(sysfspath);
+
+    buf[ret - 1] = 0;
+    ret = BLK_ZONED_NONE;
+    if (strcmp(buf, "host-managed") == 0) {
+        ret = BLK_ZONED_HM;
+    } else if (strcmp(buf, "host-aware") == 0) {
+        ret = BLK_ZONED_HA;
+    }
+
+out:
     return ret;
 #else
     return -ENOTSUP;
@@ -1116,6 +1159,10 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
                 bs->bl.max_transfer = MIN(bs->bl.max_transfer,
                                           ret * getpagesize());
             }
+            ret = hdev_get_zoned_model(&st);
+            if (ret >= 0) {
+                bs->bl.zoned_model = ret;
+            }
         }
     }
 
diff --git a/block/raw-format.c b/block/raw-format.c
index bffd424dd0..12c2a3f95d 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -369,6 +369,13 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
+static void raw_get_zoned_info(BlockDriverState *bs)
+{
+    if (!bs->probed) {
+        bs->bl.zoned_model = bs->file->bs->bl.zoned_model;
+    }
+}
+
 static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
                                         PreallocMode prealloc, Error **errp)
 {
@@ -572,6 +579,7 @@ BlockDriver bdrv_raw = {
     .bdrv_co_ioctl        = &raw_co_ioctl,
     .create_opts          = &raw_create_opts,
     .bdrv_has_zero_init   = &raw_has_zero_init,
+    .bdrv_get_zoned_info  = &raw_get_zoned_info,
     .strong_runtime_opts  = raw_strong_runtime_opts,
     .mutable_opts         = mutable_opts,
 };
-- 
2.21.0

[Qemu-devel] [PATCH 3/4] virtio-blk: Don't realize zoned block devices

[Dmitry Fomichev]

Prevent virtio-blk code from attaching a zoned block device because
it will otherwise appear as a reqular block device at the guest and
that will most certainly cause problems.

The functionality to support ZBDs via virtio-blk should be pretty
useful and there are some attempts underway to get it implemented,
but such work will inevitably lead to some modifications in virtio
protocol spec. Therefore, this activity is considered a more
long-term effort.

So for now, we just don't allow zoned block devices to work via
virtio-blk.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 hw/block/virtio-blk.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cbb3729158..c11e028308 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1140,6 +1140,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (blk_is_zoned(conf->conf.blk)) {
+        error_setg(errp, "zoned block devices are not supported");
+        return;
+    }
+
     if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD) &&
         (!conf->max_discard_sectors ||
          conf->max_discard_sectors > BDRV_REQUEST_MAX_SECTORS)) {
-- 
2.21.0

[Qemu-devel] [PATCH 4/4] hw/scsi: Don't realize zoned block devices for virtio-scsi legacy drivers

[Dmitry Fomichev]

From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Prevent scsi-hd and scsi-disk drivers from attaching a zoned block device
because it will appear as a regular block device at the guest and will
most certainly cause problems.

The functionality to support ZBDs is not planned for scsi-hd and
scsi-disk legacy drivers. It is supported via scsi-generic driver already
and discussion is ongoing for scsi-block driver.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 hw/scsi/scsi-disk.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index ed7295bfd7..80682a61fb 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2401,6 +2401,11 @@ static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
      * backend will be issued in scsi_realize
      */
     if (s->qdev.conf.blk) {
+        if (blk_is_zoned(s->qdev.conf.blk)) {
+            error_setg(errp, "zoned block devices are not supported");
+            return;
+        }
+
         ctx = blk_get_aio_context(s->qdev.conf.blk);
         aio_context_acquire(ctx);
         blkconf_blocksizes(&s->qdev.conf);
-- 
2.21.0

Re: [Qemu-devel] [PATCH 0/4] virtio: handle zoned backing devices

[Paolo Bonzini]

On 09/07/19 22:38, Dmitry Fomichev wrote:
> Currently, attaching zoned block devices (i.e. storage devices
> compliant to ZAC/ZBC standards) using several virtio methods doesn't
> work - the zoned devices appear as regular block devices at the guest.
> This may cause unexpected i/o errors and, potentially, some data
> corruption.
> 
> To be more precise, attaching a zoned device via virtio-pci-blk,
> virtio-scsi-pci/scsi-disk or virtio-scsi-pci/scsi-hd demonstrates the
> above behavior. A simple fix is needed to make
> virtio-scsi-pci/scsi-block work and this is covered by a different
> patch. The virtio-scsi-pci/scsi-generic method appears to handle zoned
> devices without problems.

The problem with this approach is that other devices (e.g. ide-hd or sd
card) also break with zoned devices and the only way to fix it would be
to add code denying zoned block devices to all of them.

The question then becomes how to define a whitelist.  One possiblity is
to add a QOM interface (for example TYPE_ZONED_BLOCK_SUPPORT) to
scsi-block and scsi-generic.  In do_parse_drive you can query the
BlockBackend with bdrv_get_zoned_info, and return an error if the
backend is a zoned block device and the device does not implement
TYPE_ZONED_BLOCK_SUPPORT.  (Commit 6b1566c is an example of adding a new
QOM interface; in your case, it would be simpler as the interface would
not have any method).  Kevin, what do you think?

Also, why deny passing Host Aware devices?

Paolo

Re: [Qemu-devel] [PATCH 0/4] virtio: handle zoned backing devices

[Kevin Wolf]

Am 10.07.2019 um 12:09 hat Paolo Bonzini geschrieben:
> On 09/07/19 22:38, Dmitry Fomichev wrote:
> > Currently, attaching zoned block devices (i.e. storage devices
> > compliant to ZAC/ZBC standards) using several virtio methods doesn't
> > work - the zoned devices appear as regular block devices at the guest.
> > This may cause unexpected i/o errors and, potentially, some data
> > corruption.
> > 
> > To be more precise, attaching a zoned device via virtio-pci-blk,
> > virtio-scsi-pci/scsi-disk or virtio-scsi-pci/scsi-hd demonstrates the
> > above behavior. A simple fix is needed to make
> > virtio-scsi-pci/scsi-block work and this is covered by a different
> > patch. The virtio-scsi-pci/scsi-generic method appears to handle zoned
> > devices without problems.
> 
> The problem with this approach is that other devices (e.g. ide-hd or sd
> card) also break with zoned devices and the only way to fix it would be
> to add code denying zoned block devices to all of them.
> 
> The question then becomes how to define a whitelist.  One possiblity is
> to add a QOM interface (for example TYPE_ZONED_BLOCK_SUPPORT) to
> scsi-block and scsi-generic.  In do_parse_drive you can query the
> BlockBackend with bdrv_get_zoned_info, and return an error if the
> backend is a zoned block device and the device does not implement
> TYPE_ZONED_BLOCK_SUPPORT.  (Commit 6b1566c is an example of adding a new
> QOM interface; in your case, it would be simpler as the interface would
> not have any method).  Kevin, what do you think?

What about non-device users such as block jobs or (NBD) exports? Won't
they have to special-case such devices, too? In fact, what about image
format drivers or even filters?

I feel that this needs to be managed at the BDS level somehow. Not sure
which mechanism to use, though. Permissions would be suitable for a
blacklist approach, but I agree with you that we need a whitelist
instead.

Hm... Actually, file-posix implements .bdrv_check_perm and could just
refuse attaching a parent there if it doesn't request a specific
permission like BLK_PERM_SUPPORT_ZONED. That should give us the
whitelist semantics through existing infrastructure.

Kevin

Re: [Qemu-devel] [PATCH 0/4] virtio: handle zoned backing devices

[Paolo Bonzini]

On 10/07/19 13:02, Kevin Wolf wrote:
> Hm... Actually, file-posix implements .bdrv_check_perm and could just
> refuse attaching a parent there if it doesn't request a specific
> permission like BLK_PERM_SUPPORT_ZONED. That should give us the
> whitelist semantics through existing infrastructure.

I'd like Dmitry to have something more precise to base his work on.  The
permissions system is really complicated and I never really wrapped my
head around it, so I need your help.

IIUC, blkconf_apply_backend_options would grow a new argument (like
"resizable") and that argument would add BLK_PERM_SUPPORT_ZONED to the
perm that blkconf_apply_backend_options passes to blk_set_perm.  On the
other side raw_check_perm would say something like

    if (is_zoned(s) && !(perm & BLK_PERM_SUPPORT_ZONED)) {
        error_setg(....);
        return -ENOTSUP;
    }

Is this correct?

In addition, BLK_PERM_SUPPORT_ZONED would have to be a shared
permission, since it's possible to assign the same block device to
multiple scsi-block devices.  So BLK_PERM_SUPPORT_ZONED would be added
unconditionally to shared_perm.

Paolo

ps: I have always thought that shared_perm is expressed the wrong way
and should have been "denied_perm".  How hard would it be to change that
now?

Re: [Qemu-devel] [PATCH 0/4] virtio: handle zoned backing devices

[Kevin Wolf]

Am 10.07.2019 um 13:33 hat Paolo Bonzini geschrieben:
> On 10/07/19 13:02, Kevin Wolf wrote:
> > Hm... Actually, file-posix implements .bdrv_check_perm and could just
> > refuse attaching a parent there if it doesn't request a specific
> > permission like BLK_PERM_SUPPORT_ZONED. That should give us the
> > whitelist semantics through existing infrastructure.
> 
> I'd like Dmitry to have something more precise to base his work on.  The
> permissions system is really complicated and I never really wrapped my
> head around it, so I need your help.
> 
> IIUC, blkconf_apply_backend_options would grow a new argument (like
> "resizable") and that argument would add BLK_PERM_SUPPORT_ZONED to the
> perm that blkconf_apply_backend_options passes to blk_set_perm.  On the
> other side raw_check_perm would say something like
> 
>     if (is_zoned(s) && !(perm & BLK_PERM_SUPPORT_ZONED)) {
>         error_setg(....);
>         return -ENOTSUP;
>     }
> 
> Is this correct?

Yes, I think this is how you'd best implement it.

> In addition, BLK_PERM_SUPPORT_ZONED would have to be a shared
> permission, since it's possible to assign the same block device to
> multiple scsi-block devices.  So BLK_PERM_SUPPORT_ZONED would be added
> unconditionally to shared_perm.

Right, this part shows that we're kind of abusing the permission system
here. I think unconditionally adding BLK_PERM_SUPPORT_ZONED to the set
of shared permissions could probably happen centrally in
bdrv_child_perm().

> ps: I have always thought that shared_perm is expressed the wrong way
> and should have been "denied_perm".  How hard would it be to change that
> now?

I'm not sure it would be better. It is shared_perm because that means
that the default is restrictive (error mode: operation refused, clearly
reported, easy to fix) rather than permissive (error mode: image
corruption, hard to figure out where we were too permissive). Basically,
whitelist instead of blacklist, once again.

But if we did indeed decide to change it, the only way to find out how
hard it is would be doing it. I suspect not too hard in principle, but
making sure that we converted all callers and don't introduce wrong
callers later through (silent) merge conflicts is the more concerning
part.

Kevin

Re: [Qemu-devel] [PATCH 0/4] virtio: handle zoned backing devices

[Dmitry Fomichev]

On Wed, 2019-07-10 at 23:09 +0200, Kevin Wolf wrote:
> Am 10.07.2019 um 13:33 hat Paolo Bonzini geschrieben:
> > On 10/07/19 13:02, Kevin Wolf wrote:
> > > Hm... Actually, file-posix implements .bdrv_check_perm and could just
> > > refuse attaching a parent there if it doesn't request a specific
> > > permission like BLK_PERM_SUPPORT_ZONED. That should give us the
> > > whitelist semantics through existing infrastructure.
> > 
> > I'd like Dmitry to have something more precise to base his work on.  The
> > permissions system is really complicated and I never really wrapped my
> > head around it, so I need your help.
> > 
> > IIUC, blkconf_apply_backend_options would grow a new argument (like
> > "resizable") and that argument would add BLK_PERM_SUPPORT_ZONED to the
> > perm that blkconf_apply_backend_options passes to blk_set_perm.  On the
> > other side raw_check_perm would say something like
> > 
> >     if (is_zoned(s) && !(perm & BLK_PERM_SUPPORT_ZONED)) {
> >         error_setg(....);
> >         return -ENOTSUP;
> >     }
> > 
> > Is this correct?
> 
> Yes, I think this is how you'd best implement it.
> 
> > In addition, BLK_PERM_SUPPORT_ZONED would have to be a shared
> > permission, since it's possible to assign the same block device to
> > multiple scsi-block devices.  So BLK_PERM_SUPPORT_ZONED would be added
> > unconditionally to shared_perm.
> 
> Right, this part shows that we're kind of abusing the permission system
> here. I think unconditionally adding BLK_PERM_SUPPORT_ZONED to the set
> of shared permissions could probably happen centrally in
> bdrv_child_perm().
> 
> > ps: I have always thought that shared_perm is expressed the wrong way
> > and should have been "denied_perm".  How hard would it be to change that
> > now?
> 
> I'm not sure it would be better. It is shared_perm because that means
> that the default is restrictive (error mode: operation refused, clearly
> reported, easy to fix) rather than permissive (error mode: image
> corruption, hard to figure out where we were too permissive). Basically,
> whitelist instead of blacklist, once again.
> 
> But if we did indeed decide to change it, the only way to find out how
> hard it is would be doing it. I suspect not too hard in principle, but
> making sure that we converted all callers and don't introduce wrong
> callers later through (silent) merge conflicts is the more concerning
> part.
> 
> Kevin

Thanks for the feedback, the permissions based approach indeed looks
cleaner. I am looking into modifying the patchset to do the check in
bdrv_check_perm and will send a V2.

Paolo,
WRT to Host Aware drives, these MAY work, but we don't have any of these
available for testing and are not able to verify which drivers do work
with them and which do not. This is the reason for not letting them pass
thru. If you prefer, I can enable passing HA drives in V2.

Dmitry

Re: [Qemu-devel] [PATCH 0/4] virtio: handle zoned backing devices

[Paolo Bonzini]

On 11/07/19 02:52, Dmitry Fomichev wrote:
> Paolo,
> WRT to Host Aware drives, these MAY work, but we don't have any of these
> available for testing and are not able to verify which drivers do work
> with them and which do not. This is the reason for not letting them pass
> thru. If you prefer, I can enable passing HA drives in V2.

In theory host aware should work the same as drive managed if the host
is oblivious of zones, so I prefer enabling them indeed.

Thanks,

Paolo