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;
