Add VIRTIO_BLK_T_DISCARD support to the virtio-blk driver

[allanjude]

Summary

If the hypervisor advertises support for the DISCARD command then the
guest can perform TRIM commands, freeing space on the backing store.

If VIRTIO_BLK_F_DISCARD is enabled, advertise DISKFLAG_CANDELETE

[bryanv]

sys/dev/virtio/block/virtio_blk.c

731 ↗

(On Diff #62278)

Spec says this is in 512-byte units.

...max_discard_sectors discard_sector_alignment are expressed in 512-byte units if the VIRTIO_BLK_F_DISCARD feature bit...

732 ↗

(On Diff #62278)

Is there a corresponding disk field for discard_sector_alignment?

948 ↗

(On Diff #62278)

This will go out of scope

sys/dev/virtio/block/virtio_blk.h

37 ↗

(On Diff #62278)

I'd prefer to you keep the existing style (and it is more or less consistent with the other VirtIO drivers defs)

[allanjude]

Update to fix some issues and catch up with HEAD

[cem]

sys/dev/virtio/block/virtio_blk.c

557–560 ↗

(On Diff #70437)

You should also check for VIRTIO_BLK_F_DISCARD before submitting the DELETE bio to the queue.

696 ↗

(On Diff #70437)

This refactoring (512 -> VTBLK_BSIZE) can go in as a separate non-functional change.

889–894 ↗

(On Diff #70437)

Should these still be in units of 512 if VIRTIO_BLK_F_BLK_SIZE? (Orthogonal to the TRIM change, I know.)

959–960 ↗

(On Diff #70437)

We've already done this math once in req->vbr_hdr.type, etc, so it's a bit odd to do it again. I'd use the req values here, since that seems to be the primary interface.

1127–1128 ↗

(On Diff #70437)

I'd suggest assigning NULL to the pointer after free as well.

sys/dev/virtio/block/virtio_blk.h

47–48 ↗

(On Diff #70437)

These comments seem backwards? Also are the bits really both 0x200, or should one be 0x100?

76–83 ↗

(On Diff #70437)

style nit: Capitalize the first letter of each sentence.

[allanjude]

Address most of cem's feedback

sys/dev/virtio/block/virtio_blk.c

889–894 ↗

(On Diff #70437)

I think so, everything in the virtio protocol is specifically defined in 512 bytes

959–960 ↗

(On Diff #70437)

The req object only knows the sector number, it does not know the length (num_sectors)

sys/dev/virtio/block/virtio_blk.h

47–48 ↗

(On Diff #70437)

I have improved the comment. They are both 0x200, the flag was renamed from WCE to FLUSH in virtio 1.0

[allanjude]

Fix typo

[jhb]

Please do test against a non-bhyve hypervisor (KVM or QEMU)

[allanjude]

I've created some images for people to test in other hypervisors. A build of -current from this morning, with this patch applied:

http://trooper.hml3.scaleengine.net/freebsd_virtio_trim_r362891.vmdk

http://trooper.hml3.scaleengine.net/freebsd_virtio_trim_r362891.raw

[freqlabs]

I haven't reviewed the code, but I did a basic test in bhyve and it put zeros on the disk in an instant :)