Re: [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins

[Paolo Bonzini]

Il 01/09/2014 12:13, Chrysostomos Nanakos ha scritto:
>>>
>>> -        if ((__sync_add_and_fetch(&segreq->ref, -segments_nr + i))
>>> == 0) {
>>> +        if ((atomic_add_fetch(&segreq->ref, -segments_nr + i)) == 0) {
> 
> 
> What about this one? It seems easier to me to read the above and
> understand what is going on.
> 
> By any means, it's up to you to accept patch 1 :)

Yeah, you win on this one. :)  But this code could use some
refactoring...

Also, there is also a missing memory barrier before the
first archipelago_submit_request call, and setting "failed" does
not need an atomic operation.

Something like this (untested):

diff --git a/block/archipelago.c b/block/archipelago.c
index 34f72dc..c71898a 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -824,76 +824,47 @@ static int 
archipelago_aio_segmented_rw(BDRVArchipelagoState *s,
                                         ArchipelagoAIOCB *aio_cb,
                                         int op)
 {
-    int i, ret, segments_nr, last_segment_size;
+    int i, ret, segments_nr;
+    size_t pos;
     ArchipelagoSegmentedRequest *segreq;
 
-    segreq = g_new(ArchipelagoSegmentedRequest, 1);
+    segreq = g_new0(ArchipelagoSegmentedRequest, 1);
 
     if (op == ARCHIP_OP_FLUSH) {
         segments_nr = 1;
-        segreq->ref = segments_nr;
-        segreq->total = count;
-        segreq->count = 0;
-        segreq->failed = 0;
-        ret = archipelago_submit_request(s, 0, count, offset, aio_cb,
-                                           segreq, ARCHIP_OP_FLUSH);
-        if (ret < 0) {
-            goto err_exit;
-        }
-        return 0;
+    } else {
+        segments_nr = (int)(count / MAX_REQUEST_SIZE) + \
+                      ((count % MAX_REQUEST_SIZE) ? 1 : 0);
     }
 
-    segments_nr = (int)(count / MAX_REQUEST_SIZE) + \
-                  ((count % MAX_REQUEST_SIZE) ? 1 : 0);
-    last_segment_size = (int)(count % MAX_REQUEST_SIZE);
-
-    segreq->ref = segments_nr;
     segreq->total = count;
-    segreq->count = 0;
-    segreq->failed = 0;
-
-    for (i = 0; i < segments_nr - 1; i++) {
-        ret = archipelago_submit_request(s, i * MAX_REQUEST_SIZE,
-                                           MAX_REQUEST_SIZE,
-                                           offset + i * MAX_REQUEST_SIZE,
-                                           aio_cb, segreq, op);
-
+    atomic_mb_set(&segreq->ref, segments_nr);
+
+    pos = 0;
+    for (; segments_nr > 1; segments_nr--) {
+        ret = archipelago_submit_request(s, pos,
+                                         MAX_REQUEST_SIZE,
+                                         offset + pos,
+                                         aio_cb, segreq, op);
         if (ret < 0) {
             goto err_exit;
         }
+        count -= MAX_REQUEST_SIZE;
+        pos += MAX_REQUEST_SIZE;
     }
 
-    if ((segments_nr > 1) && last_segment_size) {
-        ret = archipelago_submit_request(s, i * MAX_REQUEST_SIZE,
-                                           last_segment_size,
-                                           offset + i * MAX_REQUEST_SIZE,
-                                           aio_cb, segreq, op);
-    } else if ((segments_nr > 1) && !last_segment_size) {
-        ret = archipelago_submit_request(s, i * MAX_REQUEST_SIZE,
-                                           MAX_REQUEST_SIZE,
-                                           offset + i * MAX_REQUEST_SIZE,
-                                           aio_cb, segreq, op);
-    } else if (segments_nr == 1) {
-            ret = archipelago_submit_request(s, 0, count, offset, aio_cb,
-                                               segreq, op);
-    }
-
+    ret = archipelago_submit_request(s, pos, count,
+                                     offset + pos,
+                                     aio_cb, segreq, op);
     if (ret < 0) {
         goto err_exit;
     }
-
     return 0;
 
 err_exit:
-    __sync_add_and_fetch(&segreq->failed, 1);
-    if (segments_nr == 1) {
-        if (__sync_add_and_fetch(&segreq->ref, -1) == 0) {
-            g_free(segreq);
-        }
-    } else {
-        if ((__sync_add_and_fetch(&segreq->ref, -segments_nr + i)) == 0) {
-            g_free(segreq);
-        }
+    segreq->failed = 1;
+    if (atomic_fetch_sub(&segreq->ref, segments_nr) == segments_nr) {
+        g_free(segreq);
     }
 
     return ret;