CP-52880: Avoid O(N^2) behaviour in Xapi_vdi.update_allowed_operations

Activate old xapi_vdi.update_allowed_operations optimization: get_internal_records_where does a linear scan currently, so operating on N VDIs is O(N^2). Look at the VBD records directly, like before this 2013 commit which regressed it: 5097475 (We are going to optimize get_record separately so it doesn't go through serialization) Signed-off-by: Edwin Török <[email protected]>
edwintorok · Dec 12, 2024 · a83477f · a83477f
1 parent 0b34302
commit a83477f
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 21 deletions.
diff --git a/ocaml/xapi/cancel_tasks.ml b/ocaml/xapi/cancel_tasks.ml
@@ -83,14 +83,14 @@ let update_all_allowed_operations ~__context =
       in
       let vbd_records =
         List.map
-          (fun vbd -> (vbd, Db.VBD.get_record_internal ~__context ~self:vbd))
+          (fun vbd -> Db.VBD.get_record_internal ~__context ~self:vbd)
           all_vbds
       in
       List.iter
         (safe_wrapper "allowed_ops - VDIs" (fun self ->
              let relevant_vbds =
                List.filter
-                 (fun (_, vbd_record) -> vbd_record.Db_actions.vBD_VDI = self)
+                 (fun vbd_record -> vbd_record.Db_actions.vBD_VDI = self)
                  vbd_records
              in
              Xapi_vdi.update_allowed_operations_internal ~__context ~self

diff --git a/ocaml/xapi/xapi_vdi.ml b/ocaml/xapi/xapi_vdi.ml
@@ -155,16 +155,14 @@ let check_operation_error ~__context ?sr_records:_ ?(pbd_records = [])
                )
           )
     | Some records ->
-        List.map snd
-          (List.filter
-             (fun (_, vbd_record) ->
-               vbd_record.Db_actions.vBD_VDI = _ref'
-               && (vbd_record.Db_actions.vBD_currently_attached
-                  || vbd_record.Db_actions.vBD_reserved
-                  )
-             )
-             records
+        List.filter
+          (fun vbd_record ->
+            vbd_record.Db_actions.vBD_VDI = _ref'
+            && (vbd_record.Db_actions.vBD_currently_attached
+               || vbd_record.Db_actions.vBD_reserved
+               )
           )
+          records
   in
   let my_active_rw_vbd_records =
     List.filter (fun vbd -> vbd.Db_actions.vBD_mode = `RW) my_active_vbd_records
@@ -183,14 +181,12 @@ let check_operation_error ~__context ?sr_records:_ ?(pbd_records = [])
                )
           )
     | Some records ->
-        List.map snd
-          (List.filter
-             (fun (_, vbd_record) ->
-               vbd_record.Db_actions.vBD_VDI = _ref'
-               && vbd_record.Db_actions.vBD_current_operations <> []
-             )
-             records
+        List.filter
+          (fun vbd_record ->
+            vbd_record.Db_actions.vBD_VDI = _ref'
+            && vbd_record.Db_actions.vBD_current_operations <> []
           )
+          records
   in
   (* If the VBD is currently_attached then some operations can still be
      performed ie: VDI.clone (if the VM is suspended we have to have the
@@ -477,10 +473,18 @@ let update_allowed_operations_internal ~__context ~self ~sr_records ~pbd_records
          )
   in
   let all = Db.VDI.get_record_internal ~__context ~self in
+  let vbd_records =
+    match vbd_records with
+    | None ->
+        all.Db_actions.vDI_VBDs
+        |> List.rev_map (fun self -> Db.VBD.get_record_internal ~__context ~self)
+    | Some cached ->
+        cached (* currently unused *)
+  in
   let allowed =
     let check x =
       match
-        check_operation_error ~__context ~sr_records ~pbd_records ?vbd_records
+        check_operation_error ~__context ~sr_records ~pbd_records ~vbd_records
           ha_enabled all self x
       with
       | Ok () ->

diff --git a/ocaml/xapi/xapi_vdi.mli b/ocaml/xapi/xapi_vdi.mli
@@ -23,7 +23,7 @@ val check_operation_error :
      __context:Context.t
   -> ?sr_records:'a list
   -> ?pbd_records:(API.ref_PBD * API.pBD_t) list
-  -> ?vbd_records:(API.ref_VBD * Db_actions.vBD_t) list
+  -> ?vbd_records:Db_actions.vBD_t list
   -> bool
   -> Db_actions.vDI_t
   -> API.ref_VDI
@@ -40,7 +40,7 @@ val update_allowed_operations_internal :
   -> self:[`VDI] API.Ref.t
   -> sr_records:'a list
   -> pbd_records:(API.ref_PBD * API.pBD_t) list
-  -> ?vbd_records:(API.ref_VBD * Db_actions.vBD_t) list
+  -> ?vbd_records:Db_actions.vBD_t list
   -> unit
   -> unit