From b5313ae7c5e155b7c2f9d5b7a59db88a7b327ab8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edwin.torok@cloud.com>
Date: Fri, 19 Apr 2024 00:12:15 +0100
Subject: [PATCH] CP-52880: Avoid O(N^2) behaviour in
 Xapi_vdi.update_allowed_operations
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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:
5097475ded2cdd90d879833ad9efea33e1bc29eb

(We are going to optimize get_record separately so it doesn't go through serialization)

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
---
 ocaml/xapi/cancel_tasks.ml |  4 ++--
 ocaml/xapi/xapi_vdi.ml     | 38 +++++++++++++++++++++-----------------
 ocaml/xapi/xapi_vdi.mli    |  4 ++--
 3 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/ocaml/xapi/cancel_tasks.ml b/ocaml/xapi/cancel_tasks.ml
index 690cd1026b1..acdcd2fe015 100644
--- 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
index a2978de0b7f..5fc934d7afe 100644
--- 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
index 45569a12fde..3d60ad31ff1 100644
--- 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