Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD

[Kevin Wolf <kwolf@redhat.com>]

Am 15.11.2021 um 05:51 hat yadong.qi@intel.com geschrieben:
> From: Yadong Qi <yadong.qi@intel.com>
> 
> Add a new option "secdiscard" for block drive. Parse it and
> record it in bs->open_flags as bit(BDRV_O_SECDISCARD).
> 
> Add a new BDRV_REQ_SECDISCARD bit for secure discard request
> from virtual device.
> 
> For host_device backend: implement by ioctl(BLKSECDISCARD) on
> real host block device.
> For other backend, no implementation.
> 
> E.g.:
>     qemu-system-x86_64 \
>     ... \
>     -drive file=/dev/mmcblk0p2,if=none,format=raw,discard=on,secdiscard=on,id=sd0 \
>     -device virtio-blk-pci,drive=sd0,id=sd0_vblk \
>     ...
> 
> Signed-off-by: Yadong Qi <yadong.qi@intel.com>
> ---
>  block.c                          | 46 +++++++++++++++++++++++++++++
>  block/blkdebug.c                 |  5 ++--
>  block/blklogwrites.c             |  6 ++--
>  block/blkreplay.c                |  5 ++--
>  block/block-backend.c            | 15 ++++++----
>  block/copy-before-write.c        |  5 ++--
>  block/copy-on-read.c             |  5 ++--
>  block/coroutines.h               |  6 ++--
>  block/file-posix.c               | 50 ++++++++++++++++++++++++++++----
>  block/filter-compress.c          |  5 ++--
>  block/io.c                       |  5 ++--
>  block/mirror.c                   |  5 ++--
>  block/nbd.c                      |  3 +-
>  block/nvme.c                     |  3 +-
>  block/preallocate.c              |  5 ++--
>  block/qcow2-refcount.c           |  4 +--
>  block/qcow2.c                    |  3 +-
>  block/raw-format.c               |  5 ++--
>  block/throttle.c                 |  5 ++--
>  hw/block/virtio-blk.c            |  2 +-
>  hw/ide/core.c                    |  1 +
>  hw/nvme/ctrl.c                   |  3 +-
>  hw/scsi/scsi-disk.c              |  2 +-
>  include/block/block.h            | 13 +++++++--
>  include/block/block_int.h        |  2 +-
>  include/block/raw-aio.h          |  4 ++-
>  include/sysemu/block-backend.h   |  1 +
>  tests/unit/test-block-iothread.c |  9 +++---
>  28 files changed, 171 insertions(+), 52 deletions(-)

Notably absent: qapi/block-core.json. Without changing this, the option
can't be available in -blockdev, which is the primary option to configure
block device backends.

This patch seems to contain multiple logical changes that should be
split into separate patches:

* Adding a flags parameter to .bdrv_co_pdiscard

* Support for the new feature in the core block layer (should be done
  with -blockdev)

* Convenience magic for -drive (BDRV_O_SECDISCARD). It's not clear that
  this should be done at all because the option is really specific to
  one single block driver (file-posix). I think in your patch, all
  other block drivers silently ignore the option, which is not what we
  want.

> diff --git a/block.c b/block.c
> index 580cb77a70..4f05e96d12 100644
> --- a/block.c
> +++ b/block.c
> @@ -1128,6 +1128,32 @@ int bdrv_parse_discard_flags(const char *mode, int *flags)
>      return 0;
>  }
>  
> +/**
> + * Set open flags for a given secdiscard mode
> + *
> + * Return 0 on success, -1 if the secdiscard mode was invalid.
> + */
> +int bdrv_parse_secdiscard_flags(const char *mode, int *flags, Error **errp)
> +{
> +    *flags &= ~BDRV_O_SECDISCARD;
> +
> +    if (!strcmp(mode, "off")) {
> +        /* do nothing */
> +    } else if (!strcmp(mode, "on")) {
> +        if (!(*flags & BDRV_O_UNMAP)) {
> +            error_setg(errp, "cannot enable secdiscard when discard is "
> +                             "disabled!");
> +            return -1;
> +        }

This check has nothing to do with parsing the option, it's validating
its value.

You don't even need a new function to parse it, because there is already
qemu_opt_get_bool(). Duplicating it means only that you're inconsistent
with other boolean options, which alternatively accept "yes"/"no",
"true"/"false", "y/n".

> +
> +        *flags |= BDRV_O_SECDISCARD;
> +    } else {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  /**
>   * Set open flags for a given cache mode
>   *
> @@ -1695,6 +1721,11 @@ QemuOptsList bdrv_runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "discard operation (ignore/off, unmap/on)",
>          },
> +        {
> +            .name = BDRV_OPT_SECDISCARD,
> +            .type = QEMU_OPT_STRING,
> +            .help = "secure discard operation (off, on)",
> +        },
>          {
>              .name = BDRV_OPT_FORCE_SHARE,
>              .type = QEMU_OPT_BOOL,
> @@ -1735,6 +1766,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
>      const char *driver_name = NULL;
>      const char *node_name = NULL;
>      const char *discard;
> +    const char *secdiscard;
>      QemuOpts *opts;
>      BlockDriver *drv;
>      Error *local_err = NULL;
> @@ -1829,6 +1861,16 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
>          }
>      }
>  
> +
> +    secdiscard = qemu_opt_get(opts, BDRV_OPT_SECDISCARD);
> +    if (secdiscard != NULL) {
> +        if (bdrv_parse_secdiscard_flags(secdiscard, &bs->open_flags,
> +                                        errp) != 0) {
> +            ret = -EINVAL;
> +            goto fail_opts;
> +        }
> +    }
> +
>      bs->detect_zeroes =
>          bdrv_parse_detect_zeroes(opts, bs->open_flags, &local_err);
>      if (local_err) {
> @@ -3685,6 +3727,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>                                 &flags, options, flags, options);
>      }
>  
> +    if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_SECDISCARD), "on")) {
> +            flags |= BDRV_O_SECDISCARD;

Indentation is off.

> +    }
> +
>      bs->open_flags = flags;
>      bs->options = options;
>      options = qdict_clone_shallow(options);
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index bbf2948703..b49bb6a3e9 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -717,7 +717,8 @@ static int coroutine_fn blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
>  }
>  
>  static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
> -                                             int64_t offset, int64_t bytes)
> +                                             int64_t offset, int64_t bytes,
> +                                             BdrvRequestFlags flags)
>  {
>      uint32_t align = bs->bl.pdiscard_alignment;
>      int err;
> @@ -747,7 +748,7 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
>          return err;
>      }
>  
> -    return bdrv_co_pdiscard(bs->file, offset, bytes);
> +    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
>  }
>  
>  static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index f7a251e91f..d8d81a40ae 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -456,7 +456,8 @@ static int coroutine_fn blk_log_writes_co_do_file_flush(BlkLogWritesFileReq *fr)
>  static int coroutine_fn
>  blk_log_writes_co_do_file_pdiscard(BlkLogWritesFileReq *fr)
>  {
> -    return bdrv_co_pdiscard(fr->bs->file, fr->offset, fr->bytes);
> +    return bdrv_co_pdiscard(fr->bs->file, fr->offset, fr->bytes,
> +                            fr->file_flags);
>  }
>  
>  static int coroutine_fn
> @@ -484,7 +485,8 @@ static int coroutine_fn blk_log_writes_co_flush_to_disk(BlockDriverState *bs)
>  }
>  
>  static int coroutine_fn
> -blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
> +blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes,
> +                                       BdrvRequestFlags flags)
>  {
>      return blk_log_writes_co_log(bs, offset, bytes, NULL, 0,
>                                   blk_log_writes_co_do_file_pdiscard,
> diff --git a/block/blkreplay.c b/block/blkreplay.c
> index dcbe780ddb..65e66d0766 100644
> --- a/block/blkreplay.c
> +++ b/block/blkreplay.c
> @@ -105,10 +105,11 @@ static int coroutine_fn blkreplay_co_pwrite_zeroes(BlockDriverState *bs,
>  }
>  
>  static int coroutine_fn blkreplay_co_pdiscard(BlockDriverState *bs,
> -                                              int64_t offset, int64_t bytes)
> +                                              int64_t offset, int64_t bytes,
> +                                              BdrvRequestFlags flags)
>  {
>      uint64_t reqid = blkreplay_next_id();
> -    int ret = bdrv_co_pdiscard(bs->file, offset, bytes);
> +    int ret = bdrv_co_pdiscard(bs->file, offset, bytes, flags);
>      block_request_create(reqid, bs, qemu_coroutine_self());
>      qemu_coroutine_yield();
>  
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 12ef80ea17..f2c5776172 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1597,7 +1597,8 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
>  
>  /* To be called between exactly one pair of blk_inc/dec_in_flight() */
>  int coroutine_fn
> -blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
> +blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
> +                   BdrvRequestFlags flags)
>  {
>      int ret;
>  
> @@ -1608,7 +1609,7 @@ blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
>          return ret;
>      }
>  
> -    return bdrv_co_pdiscard(blk->root, offset, bytes);
> +    return bdrv_co_pdiscard(blk->root, offset, bytes, flags);
>  }
>  
>  static void blk_aio_pdiscard_entry(void *opaque)
> @@ -1616,15 +1617,17 @@ static void blk_aio_pdiscard_entry(void *opaque)
>      BlkAioEmAIOCB *acb = opaque;
>      BlkRwCo *rwco = &acb->rwco;
>  
> -    rwco->ret = blk_co_do_pdiscard(rwco->blk, rwco->offset, acb->bytes);
> +    rwco->ret = blk_co_do_pdiscard(rwco->blk, rwco->offset, acb->bytes,
> +                                   rwco->flags);
>      blk_aio_complete(acb);
>  }
>  
>  BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
>                               int64_t offset, int64_t bytes,
> +                             BdrvRequestFlags flags,
>                               BlockCompletionFunc *cb, void *opaque)
>  {
> -    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_pdiscard_entry, 0,
> +    return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_pdiscard_entry, flags,
>                          cb, opaque);
>  }
>  
> @@ -1634,7 +1637,7 @@ int coroutine_fn blk_co_pdiscard(BlockBackend *blk, int64_t offset,
>      int ret;
>  
>      blk_inc_in_flight(blk);
> -    ret = blk_co_do_pdiscard(blk, offset, bytes);
> +    ret = blk_co_do_pdiscard(blk, offset, bytes, 0);
>      blk_dec_in_flight(blk);
>  
>      return ret;
> @@ -1645,7 +1648,7 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes)
>      int ret;
>  
>      blk_inc_in_flight(blk);
> -    ret = blk_do_pdiscard(blk, offset, bytes);
> +    ret = blk_do_pdiscard(blk, offset, bytes, 0);
>      blk_dec_in_flight(blk);
>  
>      return ret;
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index c30a5ff8de..8d60a3028f 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -64,14 +64,15 @@ static coroutine_fn int cbw_do_copy_before_write(BlockDriverState *bs,
>  }
>  
>  static int coroutine_fn cbw_co_pdiscard(BlockDriverState *bs,
> -                                        int64_t offset, int64_t bytes)
> +                                        int64_t offset, int64_t bytes,
> +                                       BdrvRequestFlags flags)
>  {
>      int ret = cbw_do_copy_before_write(bs, offset, bytes, 0);
>      if (ret < 0) {
>          return ret;
>      }
>  
> -    return bdrv_co_pdiscard(bs->file, offset, bytes);
> +    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
>  }
>  
>  static int coroutine_fn cbw_co_pwrite_zeroes(BlockDriverState *bs,
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 1fc7fb3333..52183cc9a2 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -201,9 +201,10 @@ static int coroutine_fn cor_co_pwrite_zeroes(BlockDriverState *bs,
>  
>  
>  static int coroutine_fn cor_co_pdiscard(BlockDriverState *bs,
> -                                        int64_t offset, int64_t bytes)
> +                                        int64_t offset, int64_t bytes,
> +                                       BdrvRequestFlags flags)
>  {
> -    return bdrv_co_pdiscard(bs->file, offset, bytes);
> +    return bdrv_co_pdiscard(bs->file, offset, bytes, 0);
>  }
>  
>  
> diff --git a/block/coroutines.h b/block/coroutines.h
> index c8c14a29c8..b0ba771bef 100644
> --- a/block/coroutines.h
> +++ b/block/coroutines.h
> @@ -98,9 +98,11 @@ int coroutine_fn
>  blk_co_do_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
>  
>  int generated_co_wrapper
> -blk_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
> +blk_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
> +                BdrvRequestFlags flags);
>  int coroutine_fn
> -blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes);
> +blk_co_do_pdiscard(BlockBackend *blk, int64_t offset, int64_t bytes,
> +                   BdrvRequestFlags flags);
>  
>  int generated_co_wrapper blk_do_flush(BlockBackend *blk);
>  int coroutine_fn blk_co_do_flush(BlockBackend *blk);
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 7a27c83060..caa406e429 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -160,6 +160,7 @@ typedef struct BDRVRawState {
>      bool is_xfs:1;
>  #endif
>      bool has_discard:1;
> +    bool has_secdiscard:1;
>      bool has_write_zeroes:1;
>      bool discard_zeroes:1;
>      bool use_linux_aio:1;

has_secdiscard is only set to false in raw_open_common() and never
changed or used.

> @@ -727,6 +728,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>  #endif /* !defined(CONFIG_LINUX_IO_URING) */
>  
>      s->has_discard = true;
> +    s->has_secdiscard = false;
>      s->has_write_zeroes = true;
>      if ((bs->open_flags & BDRV_O_NOCACHE) != 0 && !dio_byte_aligned(s->fd)) {
>          s->needs_alignment = true;
> @@ -765,6 +767,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>              s->discard_zeroes = true;
>          }
>  #endif
> +
>  #ifdef __linux__
>          /* On Linux 3.10, BLKDISCARD leaves stale data in the page cache.  Do
>           * not rely on the contents of discarded blocks unless using O_DIRECT.

Unrelated hunk.

> @@ -1859,6 +1862,35 @@ static int handle_aiocb_discard(void *opaque)
>      return ret;
>  }
>  
> +static int handle_aiocb_secdiscard(void *opaque)
> +{
> +    RawPosixAIOData *aiocb = opaque;
> +    int ret = -ENOTSUP;
> +    BlockDriverState *bs = aiocb->bs;
> +
> +    if (!(bs->open_flags & BDRV_O_SECDISCARD)) {
> +        return -ENOTSUP;
> +    }
> +
> +    if (aiocb->aio_type & QEMU_AIO_BLKDEV) {
> +#ifdef BLKSECDISCARD
> +        do {
> +            uint64_t range[2] = { aiocb->aio_offset, aiocb->aio_nbytes };
> +            if (ioctl(aiocb->aio_fildes, BLKSECDISCARD, range) == 0) {
> +                return 0;
> +            }
> +        } while (errno == EINTR);
> +
> +        ret = translate_err(-errno);
> +#endif
> +    }
> +
> +    if (ret == -ENOTSUP) {
> +        bs->open_flags &= ~BDRV_O_SECDISCARD;

I'd rather avoid changing bs->open_flags. This is user input and I would
preserve it in its original state.

We already know when opening the image whether it is a block device. Why
do we even open the image instead of erroring out there?

> +    }
> +    return ret;
> +}
> +
>  /*
>   * Help alignment probing by allocating the first block.
>   *

Kevin