Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for init disk with cache and sync mode #117

Merged
merged 1 commit into from
Dec 23, 2022
Merged

Support for init disk with cache and sync mode #117

merged 1 commit into from
Dec 23, 2022

Conversation

balajiv113
Copy link
Contributor

Which issue(s) this PR fixes:

Fixes #106

Additional documentation

@balajiv113 balajiv113 mentioned this pull request Dec 21, 2022
Closed
4 tasks
@rfay
Copy link

rfay commented Dec 21, 2022 •
edited
Loading

I confim that the workaround in

makes a massive positive performance difference for mounting/importing a database, ddev import-db

Code-Hex
Code-Hex reviewed Dec 22, 2022
View reviewed changes
Copy link
Owner

@Code-Hex Code-Hex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. almost LGTM

storage.go Outdated
Comment on lines 62 to 64
DiskImageSynchronizationModeFull DiskImageSynchronizationMode = 1
DiskImageSynchronizationModeFsync DiskImageSynchronizationMode = 2
DiskImageSynchronizationModeNone DiskImageSynchronizationMode = 3
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a matter of taste. I like

Suggested change
DiskImageSynchronizationModeFull DiskImageSynchronizationMode = 1
DiskImageSynchronizationModeFsync DiskImageSynchronizationMode = 2
DiskImageSynchronizationModeNone DiskImageSynchronizationMode = 3
DiskImageSynchronizationModeFull DiskImageSynchronizationMode = 1 + iota
DiskImageSynchronizationModeFsync DiskImageSynchronizationMode
DiskImageSynchronizationModeNone DiskImageSynchronizationMode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

storage.go
//
// This is only supported on macOS 12 and newer, error will
// be returned on older versions.
func NewDiskImageStorageDeviceAttachmentWithCacheAndSync(diskPath string, readOnly bool, cachingMode DiskImageCachingMode, syncMode DiskImageSynchronizationMode) (*DiskImageStorageDeviceAttachment, error) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add tests here?

This is to check availability.

vz/osversion_test.go

Line 109 in 23eeede

t.Run("macOS 12", func(t *testing.T) {

This one is for checking actually working or not. (It's OK only check exists disk path)
https://github.com/Code-Hex/vz/blob/main/storage_test.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done the first test, about second test we don't have disk path from configuration i believe.
I can add a test to create disk (with cache and sync mode) and start vm and see if its running. Is that okay ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@balajiv113 Sure! Thanks!

@Code-Hex
Copy link
Owner

@rfay If you have any information on what to enable in the Linux Kernel Config (like CONFIG_*) to use this feature, I would be very grateful if you could share it with me.

I'm writing wiki that consolidates all this information.
https://github.com/Code-Hex/vz/wiki

@rfay
Copy link

rfay commented Dec 22, 2022

I'm afraid I'm pretty ignorant; just tried out @balajiv113 's comment/workaround and was super happy with the result.

@balajiv113
Copy link
Contributor Author

Linux Kernel Config

@Code-Hex
On this unfortunately I couldn't find a equivalent kernel config. I thought using DiskImageSynchronizationModeNone should be equivalent to this workaround. But on testing looks like its not.

DiskImageSynchronizationModeFsync and DiskImageSynchronizationModeNone performing more or less same.

@Code-Hex
Copy link
Owner

@balajiv113 I see. Thank you for your comments!

Code-Hex
Code-Hex approved these changes Dec 22, 2022
View reviewed changes
Copy link
Owner

@Code-Hex Code-Hex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@balajiv113
Support for init disk with cache and sync mode
a275b82 
Signed-off-by: Balaji Vijayakumar <kuttibalaji.v6@gmail.com>
@balajiv113
Copy link
Contributor Author

Updated PR with verified commit as well :)

@Code-Hex Code-Hex merged commit 957dfd2 into Code-Hex:main Dec 23, 2022
@balajiv113 balajiv113 deleted the disk branch December 23, 2022 14:12
@rfay rfay mentioned this pull request Dec 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Code-Hex Code-Hex Code-Hex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement DiskImageStorageDeviceAttachment with mode
3 participants
@balajiv113 @rfay @Code-Hex