Page MenuHomeFreeBSD

Add VIRTIO_BLK_T_DISCARD support to the virtio-blk driver
ClosedPublic

Authored by allanjude on Sep 18 2019, 7:24 PM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bryanv requested changes to this revision.Sep 27 2019, 5:18 PM
bryanv added a subscriber: bryanv.
bryanv added inline comments.
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)

This revision now requires changes to proceed.Sep 27 2019, 5:18 PM
allanjude marked 2 inline comments as done.

Update to fix some issues and catch up with HEAD

cem added inline comments.
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 marked 5 inline comments as done.

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

jhb added a subscriber: jhb.

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

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

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

This revision was not accepted when it landed; it landed in state Needs Review.Jul 16 2020, 4:32 PM
This revision was automatically updated to reflect the committed changes.