OS-8136 Add DISCARD/TRIM support to bhyve #268
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Portions contributed by: Jason King <jason.king@joyent.com>
|
This will allow freeing of space on the zvol's backing the bhyve disks right? Edit: If they are virtio-blk I'll see if I can pull this in on a local PI somewhere today, with bhyve_bar, bhyve_fbuf and this patch, building now so should take a few hours. |
|
You will need the second PR I mentioned to get the functionality working on illumos. It's separate since this change is almost all Allan's work (so easier to preserve proper attribution), and would be what is upstreamed, while the blockif and bhyve zone changes (which is what will be in the second PR) are all illumos specific. |
|
All of this work combined is on the 'bhyve-unmap' branch if you want to see/try that. |
mgerdts
suggested changes
Mar 17, 2020
papertigers
reviewed
Mar 18, 2020
| * An arbitrary limit to prevent excessive latency due to large | ||
| * delete requests. | ||
| */ | ||
| #define VTBLK_MAX_DISCARD_SECT ((16 << 20) / VTBLK_BSIZE) /* 16 MiB */ |
There was a problem hiding this comment.
Do we know how Allan ever arrived at 16MiB? I know the comment says arbitrary, but might be something we want to experiment with down the road.
There was a problem hiding this comment.
No -- he never responded on that. You have to advertise some limit, so it's as good as any I suppose. IIRC, one of the reasons zfs trim support took so long was issuing too many/too large TRIM requests would cause pretty noticeable drops in performance while it was freeing the space, which is why a more elaborate mechanism was required, so it does seem prudent to start out relatively conservatively. No guest should be making assumptions on the sizes advertised by the host, so we should be able to change this value as needed, or maybe even turn it into a tunable (though if we decided to do that, I'd rather do it as a separate change).
|
I did some testing with this (well the whole patch set for illumos) The only VM I got working was Windows 10... You an see the space after creating a post driver update snapshot, then again after trimming. Note I had to use https://fedorapeople.org/groups/virt/virtio-win/direct-downloads/latest-virtio/virtio-win.iso From the changelog |
|
As per IRC the output of dtrace |
|
Note that @sjorge tested with the second part of this PR (which includes the illumos-specific bits) -- unfortunately unlike Gerrit, with the repo config, you can't do a series of dependent changes (e.g. 'review+1' depends on changes in 'review') to review at the same time and then have them separate commits (at least with how illumos-joyent appears to be configured), so the illumos bits will follow after this one is integrated. |
Can you point to a branch where the follow-on work is at? |
|
The bhyve-unmap branch https://github.com/joyent/illumos-joyent/tree/bhyve-unmap |
mgerdts
approved these changes
Mar 18, 2020
papertigers
approved these changes
Mar 18, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first of two PRs to implement the DISCARD/TRIM functionality. This one is largely Allan Jude's original work that we're picking up. I made a few tweaks to his code based on the last bits of FreeBSD phrabicator feedback, as well as added a 'nodelete' option for blockif to be able to explicitly disable the feature for a device.
A second PR w/ the necessary blockif changes for illumos as well as the needed bhyve zone updates (to wire up the 'nodelete' feature), will follow this PR.