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
Details
Details
- Reviewers
bryanv jhb - Group Reviewers
bhyve - Commits
- rS363255: Add VIRTIO_BLK_T_DISCARD support to the virtio-blk driver
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 30428 Build 28187: arc lint + arc unit
Event Timeline
| sys/dev/virtio/block/virtio_blk.c | ||
|---|---|---|
| 744 | Spec says this is in 512-byte units.
| |
| 745 | Is there a corresponding disk field for discard_sector_alignment? | |
| 961 | This will go out of scope | |
| sys/dev/virtio/block/virtio_blk.h | ||
| 38–39 | I'd prefer to you keep the existing style (and it is more or less consistent with the other VirtIO drivers defs) | |
| sys/dev/virtio/block/virtio_blk.c | ||
|---|---|---|
| 557–560 | You should also check for VIRTIO_BLK_F_DISCARD before submitting the DELETE bio to the queue. | |
| 703 | This refactoring (512 -> VTBLK_BSIZE) can go in as a separate non-functional change. | |
| 896–905 | Should these still be in units of 512 if VIRTIO_BLK_F_BLK_SIZE? (Orthogonal to the TRIM change, I know.) | |
| 966–967 | 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. | |
| 1134–1135 | I'd suggest assigning NULL to the pointer after free as well. | |
| sys/dev/virtio/block/virtio_blk.h | ||
| 47–48 | These comments seem backwards? Also are the bits really both 0x200, or should one be 0x100? | |
| 76–83 | style nit: Capitalize the first letter of each sentence. | |
Comment Actions
Address most of cem's feedback
| sys/dev/virtio/block/virtio_blk.c | ||
|---|---|---|
| 896–905 | I think so, everything in the virtio protocol is specifically defined in 512 bytes | |
| 966–967 | 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 | I have improved the comment. They are both 0x200, the flag was renamed from WCE to FLUSH in virtio 1.0 | |
Comment Actions
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
Comment Actions
I haven't reviewed the code, but I did a basic test in bhyve and it put zeros on the disk in an instant :)