Re: [PATCH 2/2] virtio-blk: support BLKSECDISCARD

[Stefano Garzarella]

On Mon, Nov 15, 2021 at 12:52:00PM +0800, yadong.qi@intel.com wrote:
> From: Yadong Qi <yadong.qi@intel.com>
>
> 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 <yadong.qi@intel.com>
> ---
> 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, &dwz_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(&s->host_features, 
> VIRTIO_BLK_F_SECDISCARD);
> +    else
> +        virtio_clear_feature(&s->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