Skip to content

Commit 1a46f33

Browse files
committed
CP-53555: Split unplug atomic into deactivate/detach
This consists of two parts, splitting the unplug function into functionally equivalent deactivate and detach functions, and splitting the VBD_unplug atomic itself's behaviour into two new atomics: VBD_deactivate and VBD_detach If the xenopsd_vbd_plug_unplug_legacy flag is true, the only difference will be that VBD_unplug calls deactivate and detach sequentially instead of unplug If xenopsd_vbd_plug_unplug_legacy is false, the VBD_deactivate and VBD_detach atomics will be used in place of VBD_unplug in all places that it is used. This should still be functionally equivalent. The purpose of this change is to allow optimisations in this area as there are some situations where they do not need to be called at the same time. For example we could skip detaching on reboot and only deactivate and activate again reducing reboot time. Signed-off-by: Steven Woods <[email protected]>
1 parent 79c9219 commit 1a46f33

File tree

5 files changed

+99
-20
lines changed

5 files changed

+99
-20
lines changed

ocaml/xenopsd/lib/xenops_server.ml

+33-3
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,8 @@ type atomic =
130130
| VBD_epoch_end of (Vbd.id * disk)
131131
| VBD_set_qos of Vbd.id
132132
| VBD_unplug of Vbd.id * bool
133+
| VBD_deactivate of Vbd.id * bool
134+
| VBD_detach of Vbd.id
133135
| VBD_insert of Vbd.id * disk
134136
| VBD_set_active of Vbd.id * bool
135137
| VM_remove of Vm.id
@@ -211,6 +213,10 @@ let rec name_of_atomic = function
211213
"VBD_set_qos"
212214
| VBD_unplug _ ->
213215
"VBD_unplug"
216+
| VBD_deactivate _ ->
217+
"VBD_deactivate"
218+
| VBD_detach _ ->
219+
"VBD_detach"
214220
| VBD_insert _ ->
215221
"VBD_insert"
216222
| VBD_set_active _ ->
@@ -1600,6 +1606,14 @@ let vbd_plug vbd_id =
16001606
serial_of "VBD.attach_and_activate" ~id:(VBD_DB.vm_of vbd_id)
16011607
(VBD_attach vbd_id) (VBD_activate vbd_id) []
16021608

1609+
let vbd_unplug vbd_id force =
1610+
if !xenopsd_vbd_plug_unplug_legacy then
1611+
VBD_unplug (vbd_id, force)
1612+
else
1613+
serial_of "VBD.deactivate_and_detach" ~id:(VBD_DB.vm_of vbd_id)
1614+
(VBD_deactivate (vbd_id, force))
1615+
(VBD_detach vbd_id) []
1616+
16031617
let rec atomics_of_operation = function
16041618
| VM_start (id, force) ->
16051619
let vbds_rw, vbds_ro = VBD_DB.vbds id |> vbd_plug_sets in
@@ -1688,7 +1702,7 @@ let rec atomics_of_operation = function
16881702
]
16891703
; parallel_concat "Devices.unplug" ~id
16901704
[
1691-
List.map (fun vbd -> VBD_unplug (vbd.Vbd.id, true)) vbds
1705+
List.map (fun vbd -> vbd_unplug vbd.Vbd.id true) vbds
16921706
; List.map (fun vif -> VIF_unplug (vif.Vif.id, true)) vifs
16931707
; List.map (fun pci -> PCI_unplug pci.Pci.id) pcis
16941708
]
@@ -1847,7 +1861,7 @@ let rec atomics_of_operation = function
18471861
| VBD_hotplug id ->
18481862
[VBD_set_active (id, true); vbd_plug id]
18491863
| VBD_hotunplug (id, force) ->
1850-
[VBD_unplug (id, force); VBD_set_active (id, false)]
1864+
[vbd_unplug id force; VBD_set_active (id, false)]
18511865
| VIF_hotplug id ->
18521866
[VIF_set_active (id, true); VIF_plug id]
18531867
| VIF_hotunplug (id, force) ->
@@ -2065,8 +2079,22 @@ let rec perform_atomic ~progress_callback ?result (op : atomic)
20652079
| VBD_unplug (id, force) ->
20662080
debug "VBD.unplug %s" (VBD_DB.string_of_id id) ;
20672081
finally
2068-
(fun () -> B.VBD.unplug t (VBD_DB.vm_of id) (VBD_DB.read_exn id) force)
2082+
(fun () ->
2083+
B.VBD.deactivate t (VBD_DB.vm_of id) (VBD_DB.read_exn id) force ;
2084+
B.VBD.detach t (VBD_DB.vm_of id) (VBD_DB.read_exn id)
2085+
)
2086+
(fun () -> VBD_DB.signal id)
2087+
| VBD_deactivate (id, force) ->
2088+
debug "VBD.deactivate %s" (VBD_DB.string_of_id id) ;
2089+
finally
2090+
(fun () ->
2091+
B.VBD.deactivate t (VBD_DB.vm_of id) (VBD_DB.read_exn id) force
2092+
)
20692093
(fun () -> VBD_DB.signal id)
2094+
| VBD_detach id ->
2095+
debug "VBD.detach %s" (VBD_DB.string_of_id id) ;
2096+
B.VBD.detach t (VBD_DB.vm_of id) (VBD_DB.read_exn id) ;
2097+
VBD_DB.signal id
20702098
| VBD_insert (id, disk) -> (
20712099
(* NB this is also used to "refresh" ie signal a qemu that it should
20722100
re-open a device, useful for when a physical CDROM is inserted into the
@@ -2481,6 +2509,8 @@ and trigger_cleanup_after_failure_atom op t =
24812509
| VBD_epoch_end (id, _)
24822510
| VBD_set_qos id
24832511
| VBD_unplug (id, _)
2512+
| VBD_deactivate (id, _)
2513+
| VBD_detach id
24842514
| VBD_insert (id, _) ->
24852515
immediate_operation dbg (fst id) (VBD_check_state id)
24862516
| VIF_plug id

ocaml/xenopsd/lib/xenops_server_plugin.ml

+3-1
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,9 @@ module type S = sig
211211

212212
val activate : Xenops_task.task_handle -> Vm.id -> Vbd.t -> unit
213213

214-
val unplug : Xenops_task.task_handle -> Vm.id -> Vbd.t -> bool -> unit
214+
val deactivate : Xenops_task.task_handle -> Vm.id -> Vbd.t -> bool -> unit
215+
216+
val detach : Xenops_task.task_handle -> Vm.id -> Vbd.t -> unit
215217

216218
val insert : Xenops_task.task_handle -> Vm.id -> Vbd.t -> disk -> unit
217219

ocaml/xenopsd/lib/xenops_server_simulator.ml

+3-1
Original file line numberDiff line numberDiff line change
@@ -677,7 +677,9 @@ module VBD = struct
677677

678678
let activate _ (_vm : Vm.id) (_vbd : Vbd.t) = ()
679679

680-
let unplug _ vm vbd _ = with_lock m (remove_vbd vm vbd)
680+
let deactivate _ vm vbd _ = with_lock m (remove_vbd vm vbd)
681+
682+
let detach _ _vm _vbd = ()
681683

682684
let insert _ _vm _vbd _disk = ()
683685

ocaml/xenopsd/lib/xenops_server_skeleton.ml

+3-1
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,9 @@ module VBD = struct
149149

150150
let activate _ _ _ = unimplemented "VBD.activate"
151151

152-
let unplug _ _ _ _ = unimplemented "VBD.unplug"
152+
let deactivate _ _ _ _ = unimplemented "VBD.deactivate"
153+
154+
let detach _ _ _ = unimplemented "VBD.detach"
153155

154156
let insert _ _ _ _ = unimplemented "VBD.insert"
155157

ocaml/xenopsd/xc/xenops_server_xen.ml

+57-14
Original file line numberDiff line numberDiff line change
@@ -3886,22 +3886,22 @@ module VBD = struct
38863886
)
38873887
(fun () -> cleanup_attached_vdis vm vbd.id)
38883888

3889-
let unplug task vm vbd force =
3889+
let deactivate task vm vbd force =
38903890
with_xc_and_xs (fun xc xs ->
38913891
try
38923892
(* On destroying the datapath
38933893
3894-
1. if the device has already been shutdown and deactivated (as in
3895-
suspend) we must call DP.destroy here to avoid leaks
3894+
1. if the device has already been shutdown and deactivated (as in
3895+
suspend) we must call DP.destroy here to avoid leaks
38963896
3897-
2. if the device is successfully shutdown here then we must call
3898-
DP.destroy because no-one else will
3897+
2. if the device is successfully shutdown here then we must call
3898+
DP.destroy because no-one else will
38993899
3900-
3. if the device shutdown is rejected then we should leave the DP
3901-
alone and rely on the event thread calling us again later. *)
3900+
3. if the device shutdown is rejected then we should leave the DP
3901+
alone and rely on the event thread calling us again later. *)
39023902
let domid = domid_of_uuid ~xs (uuid_of_string vm) in
39033903
(* If the device is gone then we don't need to shut it down but we do
3904-
need to free any storage resources. *)
3904+
need to free any storage resources. *)
39053905
let dev =
39063906
try
39073907
Some (device_by_id xc xs vm (device_kind_of ~xs vbd) (id_of vbd))
@@ -3939,7 +3939,7 @@ module VBD = struct
39393939
vm (id_of vbd) ;
39403940
(* this happens on normal shutdown too *)
39413941
(* Case (1): success; Case (2): success; Case (3): an exception is
3942-
thrown *)
3942+
thrown *)
39433943
with_tracing ~task ~name:"VBD_device_shutdown" @@ fun () ->
39443944
Xenops_task.with_subtask task
39453945
(Printf.sprintf "Vbd.clean_shutdown %s" (id_of vbd))
@@ -3950,7 +3950,7 @@ module VBD = struct
39503950
)
39513951
dev ;
39523952
(* We now have a shutdown device but an active DP: we should destroy
3953-
the DP if the backend is of type VDI *)
3953+
the DP if the backend is of type VDI *)
39543954
finally
39553955
(fun () ->
39563956
with_tracing ~task ~name:"VBD_device_release" (fun () ->
@@ -3994,11 +3994,14 @@ module VBD = struct
39943994
()
39953995
)
39963996
(fun () ->
3997-
with_tracing ~task ~name:"VBD_dp_destroy" @@ fun () ->
3997+
with_tracing ~task ~name:"VBD_deactivate" @@ fun () ->
3998+
let vmid = Storage.vm_of_domid domid in
39983999
match (domid, backend) with
3999-
| Some x, None | Some x, Some (VDI _) ->
4000-
Storage.dp_destroy task
4001-
(Storage.id_of (string_of_int x) vbd.Vbd.id)
4000+
| Some x, Some (VDI path) ->
4001+
let sr, vdi = Storage.get_disk_by_name task path in
4002+
let dp = Storage.id_of (string_of_int x) vbd.id in
4003+
Storage.deactivate task dp sr vdi vmid
4004+
(* We don't need to detach Local or CDROM *)
40024005
| _ ->
40034006
()
40044007
)
@@ -4007,6 +4010,46 @@ module VBD = struct
40074010
raise (Xenopsd_error (Device_detach_rejected ("VBD", id_of vbd, s)))
40084011
)
40094012

4013+
let detach task vm vbd =
4014+
with_xc_and_xs (fun xc xs ->
4015+
let domid = domid_of_uuid ~xs (uuid_of_string vm) in
4016+
let dev =
4017+
try
4018+
Some (device_by_id xc xs vm (device_kind_of ~xs vbd) (id_of vbd))
4019+
with
4020+
| Xenopsd_error (Does_not_exist (_, _)) ->
4021+
debug "VM = %s; VBD = %s; Ignoring missing domain" vm (id_of vbd) ;
4022+
None
4023+
| Xenopsd_error Device_not_connected ->
4024+
debug "VM = %s; VBD = %s; Ignoring missing device" vm (id_of vbd) ;
4025+
None
4026+
in
4027+
let backend =
4028+
match dev with
4029+
| None ->
4030+
None
4031+
| Some dv -> (
4032+
match
4033+
Rpcmarshal.unmarshal typ_of_backend
4034+
(Device.Generic.get_private_key ~xs dv _vdi_id
4035+
|> Jsonrpc.of_string
4036+
)
4037+
with
4038+
| Ok x ->
4039+
x
4040+
| Error (`Msg m) ->
4041+
internal_error "Failed to unmarshal VBD backend: %s" m
4042+
)
4043+
in
4044+
with_tracing ~task ~name:"VBD_dp_destroy" @@ fun () ->
4045+
match (domid, backend) with
4046+
| Some x, None | Some x, Some (VDI _) ->
4047+
Storage.dp_destroy task (Storage.id_of (string_of_int x) vbd.Vbd.id)
4048+
| _ ->
4049+
()
4050+
) ;
4051+
cleanup_attached_vdis vm vbd.id
4052+
40104053
let insert task vm vbd d =
40114054
on_frontend
40124055
(fun xc xs frontend_domid domain_type ->

0 commit comments

Comments
 (0)