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

[Qi, Yadong]

> 
> 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.
Sorry for the absent for -blockdev. Will try add that.
Also I will try to split the patches.
And for the BDRV_O_SECDISCARD, it is specific for file-posix.c(host_device). 
Maybe
I need to add the option only for file-posix.c.

> 
> > 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".
Sure. Will use qemu_opt_get_bool() instead.

> 
> > +
> > +        *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.
Will fix it.

> 
> > --- 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.
Will remove it.

> 
> > @@ -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.
Will fix it.

> 
> > @@ -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?
OK. I will try to find another way to record whether backend driver would 
support
SECDISCARD.

Best Regard
Yadong