Check that there are no changes during SR.scan

Currently, we are only checking that no VDIs have been removed during the
SR scan performed by the SM plugin. However, there are situations where
a VDI has been added, and if this VDI is not present in the list obtained
from SR.scan, it will be forgotten. The checks only prevent this in the
case where the VDI was added during the scan. There is still a TOCTOU
situation if the issue happens after the scan, and there is room for that.

Signed-off-by: Guillaume <[email protected]>

Author: gthvn1

ocaml/xapi/xapi_sr.ml

@@ -757,6 +757,11 @@ let update_vdis ~__context ~sr db_vdis vdi_infos =
 
 (* Perform a scan of this locally-attached SR *)
 let scan ~__context ~sr =
+  let module RefSet = Set.Make (struct
+    type t = [`VDI] Ref.t
+
+    let compare = Ref.compare
+  end) in
   let open Storage_access in
   let task = Context.get_task_id __context in
   let module C = Storage_interface.StorageAPI (Idl.Exn.GenClient (struct
@@ -781,9 +786,21 @@ let scan ~__context ~sr =
             (* It is sufficient to just compare the refs in two db_vdis, as this
                is what update_vdis uses to determine what to delete *)
             let vdis_ref_equal db_vdi1 db_vdi2 =
-              Listext.List.set_difference (List.map fst db_vdi1)
-                (List.map fst db_vdi2)
-              = []
+              let refs1 = RefSet.of_list (List.map fst db_vdi1) in
+              let refs2 = RefSet.of_list (List.map fst db_vdi2) in
+              if RefSet.equal refs1 refs2 then
+                true
+              else
+                let log_diff label a b =
+                  RefSet.diff a b
+                  |> RefSet.elements
+                  |> List.map Ref.string_of
+                  |> String.concat " "
+                  |> debug "%s: VDIs %s during scan: %s" __FUNCTION__ label
+                in
+                log_diff "removed" refs1 refs2 ;
+                log_diff "added" refs2 refs1 ;
+                false
             in
             let db_vdis_before = find_vdis () in
             let vs, sr_info =
@@ -793,24 +810,16 @@ let scan ~__context ~sr =
             let db_vdis_after = find_vdis () in
             if limit > 0 && not (vdis_ref_equal db_vdis_before db_vdis_after)
             then (
-              debug
-                "%s detected db change while scanning, before scan vdis %s, \
-                 after scan vdis %s, retry limit left %d"
-                __FUNCTION__
-                (List.map (fun (_, v) -> v.vDI_uuid) db_vdis_before
-                |> String.concat ","
-                )
-                (List.map (fun (_, v) -> v.vDI_uuid) db_vdis_after
-                |> String.concat ","
-                )
-                limit ;
+              debug "%s detected db change while scanning, retry limit left %d"
+                __FUNCTION__ limit ;
               (scan_rec [@tailcall]) (limit - 1)
             ) else if limit = 0 then
               raise
                 (Api_errors.Server_error
                    (Api_errors.internal_error, ["SR.scan retry limit exceeded"])
                 )
             else (
+              debug "%s no change detected, updating VDIs" __FUNCTION__ ;
               update_vdis ~__context ~sr db_vdis_after vs ;
               let virtual_allocation =
                 List.fold_left