Message ID | 54044B85.7040608@redhat.com |
---|---|
State | New |
Headers | show |
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; >
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;