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

[Chrysostomos Nanakos <cnanakos@grnet.gr>]

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;
>