RE: [PATCH 2/2] virtio-blk: support BLKSECDISCARD
> The Linux block layer shares the DISCARD queue limits with SECDISCARD.
> That's different from BLKZEROOUT (QEMU's WRITE_ZEROES). This is a Linux
> implementation detail but I guess virtio-blk can share the DISCARD limits with
> SECDISCARD too...
>
> > @@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq
> *req, MultiReqBuffer *mrb)
> > unsigned out_num = req->elem.out_num;
> > VirtIOBlock *s = req->dev;
> > VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > +bool is_secdiscard = false;
> >
> > if (req->elem.out_num < 1 || req->elem.in_num < 1) {
> > virtio_error(vdev, "virtio-blk missing headers"); @@ -722,6
> > +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> > * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch
> > statement,
> > * so we must mask it for these requests, then we will check if it is
> > set.
> > */
> > +case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
> > +is_secdiscard = true;
> > +__attribute__((fallthrough));
>
> The DISCARD case doesn't use __attribute__((fallthrough)) so this is
> inconsistent.
> QEMU doesn't use __attribute__((fallthrough)) so I suggest dropping this.
Sure, will try to drop the fallthrough case.
>
> > diff --git a/include/standard-headers/linux/virtio_blk.h
> > b/include/standard-headers/linux/virtio_blk.h
> > index 2dcc90826a..c55a07840c 100644
> > --- a/include/standard-headers/linux/virtio_blk.h
> > +++ b/include/standard-headers/linux/virtio_blk.h
> > @@ -40,6 +40,7 @@
> > #define VIRTIO_BLK_F_MQ12 /* support more than one vq
> */
> > #define VIRTIO_BLK_F_DISCARD 13 /* DISCARD is supported */
> > #define VIRTIO_BLK_F_WRITE_ZEROES 14 /* WRITE ZEROES is
> supported */
> > +#define VIRTIO_BLK_F_SECDISCARD15 /* WRITE ZEROES is supported
> */
>
> The comment is copy-pasted from WRITE_ZEROES.
Will fix it.
RE: [PATCH 2/2] virtio-blk: support BLKSECDISCARD
> On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong...@intel.com wrote: > > From: Yadong Qi > > > > Add new virtio feature: VIRTIO_BLK_F_SECDISCARD. > > Add new virtio command: VIRTIO_BLK_T_SECDISCARD. > > > > This feature is disabled by default, it will check the backend > > bs->open_flags & BDRV_O_SECDISCARD, enable it if BDRV_O_SECDISCARD > > is supported. > > > > Signed-off-by: Yadong Qi > > --- > > hw/block/virtio-blk.c | 26 + > > include/standard-headers/linux/virtio_blk.h | 4 > > > Any changes to standard headers need to go to linux and virtio TC. Sure. I will draft proposal to virtio-comment for review.
RE: [PATCH 2/2] virtio-blk: support BLKSECDISCARD
> >Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
> >Add new virtio command: VIRTIO_BLK_T_SECDISCARD.
>
> Has a proposal been sent out yet to virtio-comment mailing list for discussing
> these specification changes?
>
Not yet. I will draft a proposal to virtio-comment if no big concern of this
patch
>From maintainer.
> >
> >diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index
> >dbc4c5a3cd..7bc3484521 100644
> >--- a/hw/block/virtio-blk.c
> >+++ b/hw/block/virtio-blk.c
> >@@ -536,7 +536,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock
> >*dev, }
> >
> > static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
> >-struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
> >+struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes,
> >+bool is_secdiscard)
>
> Since the function now handles 3 commands, I'm thinking if it's better to pass
> the command directly and then make a switch instead of using 2 booleans.
>
Make sense.
> > {
> > VirtIOBlock *s = req->dev;
> > VirtIODevice *vdev = VIRTIO_DEVICE(s); @@ -577,8 +578,8 @@ static
> >uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
> > goto err;
> > }
> >
> >+int blk_aio_flags = 0;
>
> Maybe better to move it to the beginning of the function.
Sure.
>
> >
> >-blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
> >+if (is_secdiscard) {
> >+blk_aio_flags |= BDRV_REQ_SECDISCARD;
> >+}
> >+
> >+blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
> >+ blk_aio_flags,
> > virtio_blk_discard_write_zeroes_complete, req);
> > }
> >
> >@@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq
> *req, MultiReqBuffer *mrb)
> > unsigned out_num = req->elem.out_num;
> > VirtIOBlock *s = req->dev;
> > VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >+bool is_secdiscard = false;
> >
> > if (req->elem.out_num < 1 || req->elem.in_num < 1) {
> > virtio_error(vdev, "virtio-blk missing headers"); @@ -722,6
> >+729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> > * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch
> > statement,
> > * so we must mask it for these requests, then we will check if it is
> > set.
> > */
> >+case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
> >+is_secdiscard = true;
> >+__attribute__((fallthrough));
>
> We can use QEMU_FALLTHROUGH here.
Sure.
>
> >
> > err_status = virtio_blk_handle_discard_write_zeroes(req, _hdr,
> >-
> >is_write_zeroes);
> >+is_write_zeroes,
> >+
> >+ is_secdiscard);
> >
> >+if (blk_get_flags(conf->conf.blk) & BDRV_O_SECDISCARD)
> >+virtio_add_feature(>host_features,
> >VIRTIO_BLK_F_SECDISCARD);
> >+else
> >+virtio_clear_feature(>host_features,
> >+ VIRTIO_BLK_F_SECDISCARD);
> >+
>
> IIUC here we set or not the feature if BDRV_O_SECDISCARD is set.
>
> Should we keep it disabled if "secdiscard" is false? (e.g. to avoid migration
> problems)
Yes, BDRV_O_SECDISCARD=(secdiscard=="on") ? 1 : 0;
>
> Otherwise what is the purpose of the "secdiscard" property?
I cannot find a good method to detect whether host device support BLKSECDISCARD.
So I add this "secdiscard" property to explicitly enable this feature.
Best Regard
Yadong
>
> Thanks,
> Stefano
Re: [PATCH 2/2] virtio-blk: support BLKSECDISCARD
On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong...@intel.com wrote:
The Linux block layer shares the DISCARD queue limits with SECDISCARD.
That's different from BLKZEROOUT (QEMU's WRITE_ZEROES). This is a Linux
implementation detail but I guess virtio-blk can share the DISCARD
limits with SECDISCARD too...
> @@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> unsigned out_num = req->elem.out_num;
> VirtIOBlock *s = req->dev;
> VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +bool is_secdiscard = false;
>
> if (req->elem.out_num < 1 || req->elem.in_num < 1) {
> virtio_error(vdev, "virtio-blk missing headers");
> @@ -722,6 +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch
> statement,
> * so we must mask it for these requests, then we will check if it is
> set.
> */
> +case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
> +is_secdiscard = true;
> +__attribute__((fallthrough));
The DISCARD case doesn't use __attribute__((fallthrough)) so this is
inconsistent. QEMU doesn't use __attribute__((fallthrough)) so I suggest
dropping this.
> diff --git a/include/standard-headers/linux/virtio_blk.h
> b/include/standard-headers/linux/virtio_blk.h
> index 2dcc90826a..c55a07840c 100644
> --- a/include/standard-headers/linux/virtio_blk.h
> +++ b/include/standard-headers/linux/virtio_blk.h
> @@ -40,6 +40,7 @@
> #define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
> #define VIRTIO_BLK_F_DISCARD 13 /* DISCARD is supported */
> #define VIRTIO_BLK_F_WRITE_ZEROES14 /* WRITE ZEROES is supported */
> +#define VIRTIO_BLK_F_SECDISCARD 15 /* WRITE ZEROES is supported */
The comment is copy-pasted from WRITE_ZEROES.
signature.asc
Description: PGP signature
Re: [PATCH 2/2] virtio-blk: support BLKSECDISCARD
On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong...@intel.com wrote:
> From: Yadong Qi
>
> Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
> Add new virtio command: VIRTIO_BLK_T_SECDISCARD.
>
> This feature is disabled by default, it will check the backend
> bs->open_flags & BDRV_O_SECDISCARD, enable it if BDRV_O_SECDISCARD
> is supported.
>
> Signed-off-by: Yadong Qi
> ---
> hw/block/virtio-blk.c | 26 +
> include/standard-headers/linux/virtio_blk.h | 4
Any changes to standard headers need to go to linux and virtio TC.
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index dbc4c5a3cd..7bc3484521 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -536,7 +536,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
> }
>
> static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
> -struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
> +struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes,
> +bool is_secdiscard)
> {
> VirtIOBlock *s = req->dev;
> VirtIODevice *vdev = VIRTIO_DEVICE(s);
> @@ -577,8 +578,8 @@ static uint8_t
> virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
> goto err;
> }
>
> +int blk_aio_flags = 0;
> if (is_write_zeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
> -int blk_aio_flags = 0;
>
> if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
> blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
> @@ -600,7 +601,12 @@ static uint8_t
> virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
> goto err;
> }
>
> -blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
> +if (is_secdiscard) {
> +blk_aio_flags |= BDRV_REQ_SECDISCARD;
> +}
> +
> +blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
> + blk_aio_flags,
> virtio_blk_discard_write_zeroes_complete, req);
> }
>
> @@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> unsigned out_num = req->elem.out_num;
> VirtIOBlock *s = req->dev;
> VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +bool is_secdiscard = false;
>
> if (req->elem.out_num < 1 || req->elem.in_num < 1) {
> virtio_error(vdev, "virtio-blk missing headers");
> @@ -722,6 +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch
> statement,
> * so we must mask it for these requests, then we will check if it is
> set.
> */
> +case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
> +is_secdiscard = true;
> +__attribute__((fallthrough));
> case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
> case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
> {
> @@ -752,7 +762,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> }
>
> err_status = virtio_blk_handle_discard_write_zeroes(req, _hdr,
> -is_write_zeroes);
> +is_write_zeroes,
> +is_secdiscard);
> if (err_status != VIRTIO_BLK_S_OK) {
> virtio_blk_req_complete(req, err_status);
> virtio_blk_free_request(req);
> @@ -1201,6 +1212,11 @@ static void virtio_blk_device_realize(DeviceState
> *dev, Error **errp)
> return;
> }
>
> +if (blk_get_flags(conf->conf.blk) & BDRV_O_SECDISCARD)
> +virtio_add_feature(>host_features, VIRTIO_BLK_F_SECDISCARD);
> +else
> +virtio_clear_feature(>host_features, VIRTIO_BLK_F_SECDISCARD);
> +
> if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_WRITE_ZEROES) &&
> (!conf->max_write_zeroes_sectors ||
> conf->max_write_zeroes_sectors > BDRV_REQUEST_MAX_SECTORS)) {
> @@ -1307,6 +1323,8 @@ static Property virtio_blk_properties[] = {
> conf.report_discard_granularity, true),
> DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
>VIRTIO_BLK_F_WRITE_ZEROES, true),
> +DEFINE_PROP_BIT64("secdiscard", VirtIOBlock, host_features,
> + VIRTIO_BLK_F_SECDISCARD, false),
> DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
> conf.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS),
> DEFINE_PROP_UINT32("max-write-zeroes-sectors", VirtIOBlock,
> diff --git a/include/standard-headers/linux/virtio_blk.h
> b/include/standard-headers/linux/virtio_blk.h
> index 2dcc90826a..c55a07840c 100644
> --- a/include/standard-headers/linux/virtio_blk.h
> +++
Re: [PATCH 2/2] virtio-blk: support BLKSECDISCARD
On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong...@intel.com wrote:
From: Yadong Qi
Add new virtio feature: VIRTIO_BLK_F_SECDISCARD.
Add new virtio command: VIRTIO_BLK_T_SECDISCARD.
Has a proposal been sent out yet to virtio-comment mailing list for
discussing these specification changes?
This feature is disabled by default, it will check the backend
bs->open_flags & BDRV_O_SECDISCARD, enable it if BDRV_O_SECDISCARD
is supported.
Signed-off-by: Yadong Qi
---
hw/block/virtio-blk.c | 26 +
include/standard-headers/linux/virtio_blk.h | 4
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index dbc4c5a3cd..7bc3484521 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -536,7 +536,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
}
static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
-struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
+struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes,
+bool is_secdiscard)
Since the function now handles 3 commands, I'm thinking if it's better
to pass the command directly and then make a switch instead of using 2
booleans.
{
VirtIOBlock *s = req->dev;
VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -577,8 +578,8 @@ static uint8_t
virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
goto err;
}
+int blk_aio_flags = 0;
Maybe better to move it to the beginning of the function.
if (is_write_zeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
-int blk_aio_flags = 0;
if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
@@ -600,7 +601,12 @@ static uint8_t
virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
goto err;
}
-blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, 0,
+if (is_secdiscard) {
+blk_aio_flags |= BDRV_REQ_SECDISCARD;
+}
+
+blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
+ blk_aio_flags,
virtio_blk_discard_write_zeroes_complete, req);
}
@@ -622,6 +628,7 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
unsigned out_num = req->elem.out_num;
VirtIOBlock *s = req->dev;
VirtIODevice *vdev = VIRTIO_DEVICE(s);
+bool is_secdiscard = false;
if (req->elem.out_num < 1 || req->elem.in_num < 1) {
virtio_error(vdev, "virtio-blk missing headers");
@@ -722,6 +729,9 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
MultiReqBuffer *mrb)
* VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
* so we must mask it for these requests, then we will check if it is set.
*/
+case VIRTIO_BLK_T_SECDISCARD & ~VIRTIO_BLK_T_OUT:
+is_secdiscard = true;
+__attribute__((fallthrough));
We can use QEMU_FALLTHROUGH here.
case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
{
@@ -752,7 +762,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq
*req, MultiReqBuffer *mrb)
}
err_status = virtio_blk_handle_discard_write_zeroes(req, _hdr,
-is_write_zeroes);
+is_write_zeroes,
+is_secdiscard);
if (err_status != VIRTIO_BLK_S_OK) {
virtio_blk_req_complete(req, err_status);
virtio_blk_free_request(req);
@@ -1201,6 +1212,11 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
return;
}
+if (blk_get_flags(conf->conf.blk) & BDRV_O_SECDISCARD)
+virtio_add_feature(>host_features,
VIRTIO_BLK_F_SECDISCARD);
+else
+virtio_clear_feature(>host_features, VIRTIO_BLK_F_SECDISCARD);
+
IIUC here we set or not the feature if BDRV_O_SECDISCARD is set.
Should we keep it disabled if "secdiscard" is false? (e.g. to avoid
migration problems)
Otherwise what is the purpose of the "secdiscard" property?
Thanks,
Stefano
