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

[Chrysostomos Nanakos]

On 09/01/2014 01:33 PM, Paolo Bonzini wrote:
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.

Have to check the refactoring code thoroughly but at a first glance 
seems to be fine.

I'll come back with a new patch for block/archipelago.c then.

Thanks very much for your time and effort!

Regards,
Chrysostomos.

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;