Add VIRTIO_BLK_T_DISCARD (TRIM) support to the bhyve virtio-blk backend

[allanjude]

Summary

This will advertise support for TRIM to the guest virtio-blk driver and
perform the DIOCGDELETE ioctl on the backing storage if it supports it.

[markj]

usr.sbin/bhyve/pci_virtio_block.c

83 ↗

(On Diff #62277)

I would think that we should just be using definitions from a driver header. Not something to fix in this review though.

93 ↗

(On Diff #62277)

Does this mean that a guest will by default send discard requests even when we don't support them at the blockif layer? For instance, we do not handle discards when the device is backed by a regular file.

305 ↗

(On Diff #62277)

In the kernel we are hard-coding 512 instead of DEV_BSIZE. I suspect that the same should be done here (or we should use a separate constant) instead of assuming that DEV_BSIZE is always going to be 512.

[jhb]

usr.sbin/bhyve/pci_virtio_block.c

93 ↗

(On Diff #62277)

Yes, but this can be fixed. It looks like block_if already has a 'bc_candelete' member we can query via blockif_candelete(). All that needs to happen is that 'struct pci_vtblk_softc' needs to grow a new pointer to track an allocated 'struct virtio_consts'. pci_vtblk_init() would then allocate a structure and initialize it by copying 'vtblk_vi_consts' into it, then adding F_DISCARD to the host_caps member optionally. Something like:

sc->vbsc_consts = malloc(sizeof(struct virtio_consts));
sc->vbsc_consts = vtblk_vi_consts;
if (blockif_candelete(bctxt))
    sc->vbsc_consts->vc_host_caps |= VTBLK_F_DISCARD;

/* init virtio softc and virtqueues */
vi_softc_linkup(&sc->vbsc_sc, sc->vbsc_consts, sc, pi, &sc->vbsc_vq);

305 ↗

(On Diff #62277)

If the spec defines it as units of 512 bytes, then I agree with a bare constant and/or having a dedicated constant that is not DEV_BSIZE.

420 ↗

(On Diff #62277)

How did you come up with this number?

[markj]

usr.sbin/bhyve/pci_virtio_block.c

93 ↗

(On Diff #62277)

Ok, that makes sense. At some point it'd make sense to handle discard on regular files by punching a hole, but I don't think we have an interface to do that. (Linux has fallocate(FALLOC_FL_PUNCH_HOLE).)

305 ↗

(On Diff #62277)

Yes, the spec states that discard commands are always expressed with multiples of 512 bytes.

[jhb]

usr.sbin/bhyve/pci_virtio_block.c

93 ↗

(On Diff #62277)

At that point the block_if.c implementation can choose to support can delete. However, sooner to happen is supporting formats like QCOW2 via libvdsk which also supports sparse disks as part of its file format and will thus support trim.

[jhb]

Ping. Can you confirm that this works ok with Linux guests? Also, any sense of making forward progress on using block_if_can_delete()?

[jason.brian.king_gmail.com]

usr.sbin/bhyve/pci_virtio_block.c

301 ↗

(On Diff #62277)

The virtio 1.1 spec states that if the unmap bit is set (in discard) for a DISCARD request, it should return VIRTIO_BLK_S_UNSUPP.

[allanjude]

Add VIRTIO_BLK_T_DISCARD (TRIM) support to the bhyve virtio-blk backend

This will advertise support for TRIM to the guest virtio-blk driver and perform the DIOCGDELETE ioctl on the backing storage if it supports it.

Thanks to Jason King and others at Joyent and illumos for improvements including error handling and following the virtio spec better

Contributed By: Jason King <[e-mail]> (improvements)
Obtained From: illumos-joyent (improvements)

Testing and joyent reviews: https://smartos.org/bugview/OS-8136

[jhb]

usr.sbin/bhyve/block_if.c

32 ↗

(On Diff #70412)

Which license? I'm assuming the BSD2 clause, so this should go up in the previous license block (but after the ARR line)

[jhb]

Looks fine to me modulo the license statement question

[allanjude]

Move copyright message to the correct place