Page MenuHomeFreeBSD

Add VIRTIO_BLK_T_DISCARD support to the virtio-blk driver

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



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


Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
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.
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

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.
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.

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

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)

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:

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.