Page MenuHomeFreeBSD

bhyve: implement NVMe deallocate command
AcceptedPublic

Authored by chuck on Sep 29 2019, 3:29 PM.

Details

Reviewers
imp
ken
jhb
araujo
Group Reviewers
bhyve
Summary

This adds support for the Dataset Management (DSM) command to the NVMe
emulation in general, and more specifically, for the deallocate
attribute (a.k.a. trim in the ATA protocol). If the backing storage for
the namespace supports delete (i.e. deallocate), setting the deallocate
attribute in a DSM will trim/delete the requested LBA ranges in the
underlying storage.

Stacked with changes in D21837 and D21838

Test Plan

This has been lightly exercised and needs wider testing

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 26765

Event Timeline

chuck created this revision.Sep 29 2019, 3:29 PM
jhb accepted this revision.Oct 10 2019, 4:35 PM

I can't speak to the NVMe bits, but from what I can understand on the bhyve side it looks ok to me.

usr.sbin/bhyve/pci_nvme.c
1401

So this uses a length of 4096, would it make sense to static_assert(256 * sizeof(struct nvme_dsm_range) == 4096)? It looks kind of odd to use two different constants for the length.

1420

You can just free() unconditionally as free(NULL) is well-defined to be a nop.

This revision is now accepted and ready to land.Oct 10 2019, 4:35 PM
araujo accepted this revision.Oct 10 2019, 11:43 PM

Lgtm

chuck marked an inline comment as done.Oct 19 2019, 4:40 PM
chuck added inline comments.
usr.sbin/bhyve/pci_nvme.c
1401

You're right, that does look weird. After poking around a bit, I see that the NVMe driver defines NVME_MAX_DSM_TRIM as 4096. So I'll change the code to:

/* copy locally because a range entry could straddle PRPs */
data = calloc(1, NVME_MAX_DSM_TRIM);
if (data == NULL) {
	pci_nvme_status_genc(&status, NVME_SC_INTERNAL_DEVICE_ERROR);
	goto out;
}
nvme_prp_memcpy(sc->nsc_pi->pi_vmctx, cmd->prp1, cmd->prp2,
    data, NVME_MAX_DSM_TRIM, NVME_COPY_FROM_PRP);
1420

Oops. Forgot that FreeBSD is sane. Fixed