Allow clients to request different flush behaviour #52

djs55 · 2016-09-19T20:09:21Z

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 call flush because the disk is ephemeral and will be destroyed; this PR allows these clients to replace flush with a no-op. For example, creating a DB2 database on an ephemeral disk takes 10x longer with flush than without, see [docker/for-mac#668]

There is no change to the default (safe, conservative) behaviour.

coveralls · 2016-09-19T20:31:48Z

Coverage increased (+2.2%) to 55.734% when pulling 55165d2 on djs55:optional-sync into cc511fc on mirage:master.

coveralls · 2016-09-20T08:28:28Z

Coverage increased (+2.2%) to 55.734% when pulling f36d4d4 on djs55:optional-sync into cc511fc on mirage:master.

coveralls · 2016-09-20T09:48:58Z

Coverage increased (+2.2%) to 55.734% when pulling 6e36685 on djs55:optional-sync into cc511fc on mirage:master.

talex5 · 2016-09-20T09:50:30Z

lib/block.mli

+}
+(** Configuration of a device *)
+
+val connect_uri : Uri.t -> [`Ok of t | `Error of error] io


Why does this take a Uri.t rather than a config?

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.

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.

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 <dave@recoil.org>

Signed-off-by: David Scott <dave@recoil.org>

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 <dave@recoil.org>

These use the `get_config` function to check the configuration is as expected. Signed-off-by: David Scott <dave@recoil.org>

This is docker/vpnkit#1669eb1925ee43afd5a46188a43bf030bf34e01d Signed-off-by: David Scott <dave@recoil.org> Signed-off-by: Magnus Skjegstad <magnus@skjegstad.com>

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 <dave@recoil.org>

Signed-off-by: David Scott <dave@recoil.org>

This is in preparation for some `to_string` `of_string` functions. Signed-off-by: David Scott <dave@recoil.org>

Signed-off-by: David Scott <dave@recoil.org>

This will allow new fields to be added without modifying all users provided the fields have sensible default values. Signed-off-by: David Scott <dave@recoil.org>

Signed-off-by: David Scott <dave@recoil.org>

coveralls · 2016-09-20T12:28:15Z

Coverage increased (+1.5%) to 54.977% when pulling c61e37c on djs55:optional-sync into cc511fc on mirage:master.

samoht · 2016-09-20T12:46:12Z

Do we still need the initial connect function then? Why not renaming connect_from_config to config?

djs55 · 2016-09-20T12:55:52Z

(Do you mean to rename connect_from_config to connect?)

I agree it would be nice to replace the existing connect function but it's used in lots of other repos (for example it's commonly-used in unit tests of libraries which are functorised over BLOCK). I'd prefer to move in 2 stages:

add the backwards-compatible feature as soon as possible (to help fix a bug in a downstream program)
Start planning a v3.x with the interface-breaking change in it (including finding all the users, merging version constraints, making PRs to all of them etc -- this could take a while and I'd like to do it without time pressure)

Does that seem reasonable?

talex5 · 2016-09-20T15:05:01Z

If we make ?config an optional argument then nothing should break, I think.

djs55 · 2016-09-20T15:13:18Z

Do you mean something like

let connect ?(buffered=false) ?(sync=true) path =

I think connect ?(config=...) path would be a bit odd because the path is already in the config so it would be specified twice. I think we would need to keep the path argument to prevent old code from breaking. Or have I misunderstood?

talex5 · 2016-09-20T15:14:51Z

@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 <dave@recoil.org>

djs55 · 2016-09-20T16:56:22Z

OK we've now got a nice connect function with default arguments which should be backwards compatible (phew!)

coveralls · 2016-09-20T17:37:43Z

Coverage increased (+1.8%) to 55.305% when pulling c145b50 on djs55:optional-sync into cc511fc on mirage:master.

coveralls · 2016-09-20T17:44:15Z

Coverage increased (+1.8%) to 55.305% when pulling c145b50 on djs55:optional-sync into cc511fc on mirage:master.

djs55 · 2016-09-21T10:46:57Z

Hm, not all users are unaffected by the modified connect signature -- this breaks some code in ocaml-qcow which does (equivalent of):

module type BLOCK = sig

  include V1_LWT.BLOCK

  val connect: string -> [ `Ok of t | `Error of error ] Lwt.t
end

module B = module Block: BLOCK

resulting in

Error: Signature mismatch:
       ...
       Values do not match:
         val connect :
           ?buffered:bool ->
           ?sync:bool -> string -> [ `Error of error | `Ok of t ] io
       is not included in
         val connect : string -> [ `Error of error | `Ok of t ] Lwt.t
       File "cli/impl.ml", line 73, characters 2-61: Expected declaration

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 <dave@recoil.org>

djs55 force-pushed the optional-sync branch from f36d4d4 to 962c29d Compare September 20, 2016 09:00

talex5 reviewed Sep 20, 2016

View reviewed changes

djs55 added 11 commits September 20, 2016 12:22

Regenerate OASIS

13a712b

Signed-off-by: David Scott <dave@recoil.org>

Add a type config and allow it to be queried

10bb182

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 <dave@recoil.org>

test: add cases for the new connect_uri functions

084115f

These use the `get_config` function to check the configuration is as expected. Signed-off-by: David Scott <dave@recoil.org>

Include appveyor fix from vpnkit

992e0e4

This is docker/vpnkit#1669eb1925ee43afd5a46188a43bf030bf34e01d Signed-off-by: David Scott <dave@recoil.org> Signed-off-by: Magnus Skjegstad <magnus@skjegstad.com>

connect_uri: pct_decode the path

e078766

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 <dave@recoil.org>

Remove unused id function

446eba6

Signed-off-by: David Scott <dave@recoil.org>

Move type config into type t in a new module Config

f13ada7

This is in preparation for some `to_string` `of_string` functions. Signed-off-by: David Scott <dave@recoil.org>

Add Block.Config.{to_string, of_string} with unit tests

a378405

Signed-off-by: David Scott <dave@recoil.org>

Add Block.Config.create with default arguments

db5a604

This will allow new fields to be added without modifying all users provided the fields have sensible default values. Signed-off-by: David Scott <dave@recoil.org>

Replace connect_uri with connect_from_config

c61e37c

Signed-off-by: David Scott <dave@recoil.org>

djs55 force-pushed the optional-sync branch from 6e36685 to c61e37c Compare September 20, 2016 11:23

djs55 force-pushed the optional-sync branch from b89887b to c145b50 Compare September 20, 2016 16:54

djs55 added a commit to djs55/ocaml-qcow that referenced this pull request Sep 21, 2016

CLI: don't break the build if Block.connect has optional arguments

5ba9c65

See [mirage/mirage-block-unix#52] Signed-off-by: David Scott <dave@recoil.org>

djs55 mentioned this pull request Sep 21, 2016

CLI: don't break the build if Block.connect has optional arguments mirage/ocaml-qcow#25

Merged

djs55 merged commit 7e9cb06 into mirage:master Sep 21, 2016

djs55 deleted the optional-sync branch September 21, 2016 11:27

samoht mentioned this pull request Oct 10, 2016

Very slow fsync() on shared container volume docker/for-mac#560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow clients to request different flush behaviour #52

Allow clients to request different flush behaviour #52

djs55 commented Sep 19, 2016

coveralls commented Sep 19, 2016

coveralls commented Sep 20, 2016

coveralls commented Sep 20, 2016

talex5 Sep 20, 2016

djs55 Sep 20, 2016

talex5 Sep 20, 2016

djs55 Sep 20, 2016

coveralls commented Sep 20, 2016

samoht commented Sep 20, 2016

djs55 commented Sep 20, 2016

talex5 commented Sep 20, 2016

djs55 commented Sep 20, 2016

talex5 commented Sep 20, 2016

djs55 commented Sep 20, 2016

coveralls commented Sep 20, 2016

coveralls commented Sep 20, 2016

djs55 commented Sep 21, 2016

Allow clients to request different flush behaviour #52

Allow clients to request different flush behaviour #52

Conversation

djs55 commented Sep 19, 2016

coveralls commented Sep 19, 2016

coveralls commented Sep 20, 2016

coveralls commented Sep 20, 2016

talex5 Sep 20, 2016

Choose a reason for hiding this comment

djs55 Sep 20, 2016

Choose a reason for hiding this comment

talex5 Sep 20, 2016

Choose a reason for hiding this comment

djs55 Sep 20, 2016

Choose a reason for hiding this comment

coveralls commented Sep 20, 2016

samoht commented Sep 20, 2016

djs55 commented Sep 20, 2016

talex5 commented Sep 20, 2016

djs55 commented Sep 20, 2016

talex5 commented Sep 20, 2016

djs55 commented Sep 20, 2016

coveralls commented Sep 20, 2016

coveralls commented Sep 20, 2016

djs55 commented Sep 21, 2016