>
> 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
