-
Notifications
You must be signed in to change notification settings - Fork 23
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.
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
Allow clients to request different flush behaviour #52
Conversation
f36d4d4
to
962c29d
Compare
} | ||
(** Configuration of a device *) | ||
|
||
val connect_uri : Uri.t -> [`Ok of t | `Error of error] io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this take a Uri.t
rather than a config
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add another function (connect_config
?) but in my current use-case (within https://github.com/docker/hyperkit) the main process is written in C and accepts a string as a command-line argument of the form:
%d,virtio-blk,file://%s,format=qcow
where the file://%s
names the file to be opened.
I'd like to avoid changing the C code whenever the OCaml API is expanded here -- if I keep it as something generic I can get the C code to pass it down into the OCaml code unmodified, so a further expansion or change would only involve changes here.
Another option would be to change connect
which currently accepts strings of the form:
/foo/bar
buffered:/foo/bar
and add support for
file:///foo/bar?sync=1&buffered=0...
WDYT?
In an ideal world (where hypervisors are written in OCaml :) I'd probably just change connect
to have a proper typed interface, although that would require changes in many more places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a separate function (e.g. parse_config : string -> config or_error
) either here or in the code that uses this. We shouldn't force other users of the API to turn bools into URIs with query strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I've moved the type config
into type t
inside a module Config
and added to_string
and of_string
as well as a convenient make
function. I've removed connect_uri
and added connect_from_config
instead.
The `connect_uri` function accepts URIs of the form file:///var/tmp/foo.qcow2?buffered=(0|1)&sync=(0|1) where `buffered` means use regular open rather than O_DIRECT and `sync` means faithfully persist writes to disk after a `flush` (rather than potentially leave them in hardware caches which could cause data loss or corruption over power loss) Signed-off-by: David Scott <[email protected]>
Signed-off-by: David Scott <[email protected]>
This allows clients to determine - whether the device has buffering enabled - whether `flush` will perform a full expensive flush - the path of the underlying file Signed-off-by: David Scott <[email protected]>
These use the `get_config` function to check the configuration is as expected. Signed-off-by: David Scott <[email protected]>
This is docker/vpnkit#1669eb1925ee43afd5a46188a43bf030bf34e01d Signed-off-by: David Scott <[email protected]> Signed-off-by: Magnus Skjegstad <[email protected]>
On Windows for example: path (make ~path:"C:\\cygwin" ())) -> "C:%5Ccygwin" pct_decode @@ path @@ make ~path:"C:\\cygwin" () -> "C:\\cygwin" Signed-off-by: David Scott <[email protected]>
Signed-off-by: David Scott <[email protected]>
This is in preparation for some `to_string` `of_string` functions. Signed-off-by: David Scott <[email protected]>
Signed-off-by: David Scott <[email protected]>
This will allow new fields to be added without modifying all users provided the fields have sensible default values. Signed-off-by: David Scott <[email protected]>
Signed-off-by: David Scott <[email protected]>
6e36685
to
c61e37c
Compare
Do we still need the initial |
(Do you mean to rename I agree it would be nice to replace the existing
Does that seem reasonable? |
If we make |
Do you mean something like
I think |
@djs55 you're right - I didn't look carefully enough. |
Existing code which calls `Block.connect path` will still work. New code can now request different behaviour by supplying the optional arguments ?buffered and ?sync. This patch also renames `get_config` to `to_config` so we also have: val to_config: t -> Config.t (** [to_config t] returns the configuration of a device *) val of_config: Config.t -> [ `Ok of t | `Error of error ] io (** [of_config config] creates a fresh device from [config] *) Signed-off-by: David Scott <[email protected]>
b89887b
to
c145b50
Compare
OK we've now got a nice |
Hm, not all users are unaffected by the modified
resulting in
This is probably not very common practice so we can probably go ahead. IIRC the same thing happened when an optional argument was added to a function in the standard library and some "standard libraries++" had to also be updated. |
See [mirage/mirage-block-unix#52] Signed-off-by: David Scott <[email protected]>
The implementation of
flush
is correct but very slow: on my laptop each call can take 10ms as the cache on the disk itself must be flushed. Sometimes a client knows that it's not necessary to really callflush
because the disk is ephemeral and will be destroyed; this PR allows these clients to replaceflush
with a no-op. For example, creating a DB2 database on an ephemeral disk takes 10x longer withflush
than without, see [docker/for-mac#668]There is no change to the default (safe, conservative) behaviour.