Re: [PATCH] aio: Add command to wait completion of all requests

[Kirill Tkhai]

On 13.06.2017 11:14, Cyrill Gorcunov wrote:
> On Fri, Jun 09, 2017 at 12:49:34PM +0300, Kirill Tkhai wrote:
> ...
>> +static int aio_wait_all(struct kioctx *ctx)
>> +{
>> +    unsigned users, reqs = 0;
>> +    struct kioctx_cpu *kcpu;
>> +    int cpu, ret;
>> +
>> +    if (atomic_xchg(&ctx->dead, 1))
>> +            return -EBUSY;
>> +
>> +    users = atomic_read(&current->mm->mm_users);
>> +    if (users > 1) {
>> +            /*
>> +             * Wait till concurrent threads and aio_complete() see
>> +             * dead flag. Implies full memory barrier on all cpus.
>> +             */
>> +            synchronize_sched();
>> +    } else {
>> +            /*
>> +             * Sync with aio_complete() to be sure it puts reqs_available,
>> +             * when dead flag is already seen.
>> +             */
>> +            spin_lock_irq(&ctx->completion_lock);
>> +    }
>> +
>> +    for_each_possible_cpu(cpu) {
>> +            kcpu = per_cpu_ptr(ctx->cpu, cpu);
>> +            reqs += kcpu->reqs_available;
> 
> I'm not that familiar with AIO internals but this snippet worries me:
> the reqs_available is unsigned int, reqs is unsigned it as well but
> used as an accumulator over ALL cpus, can't it get overflow and
> gives modulo result, should not it be unsigned long or something?

All available reqs are initially contain in kioctx::reqs_available,
which is atomic_t, and then they are distributed over percpu counters.
So, this is OK.
 
>> +            kcpu->reqs_available = 0;
>> +    }
>> +
>> +    if (users == 1)
>> +            spin_unlock_irq(&ctx->completion_lock);
>> +
>> +    atomic_add(reqs, &ctx->reqs_available);
>> +
>> +    ret = wait_event_interruptible(ctx->wait, reqs_completed(ctx));
>> +
>> +    atomic_set(&ctx->dead, 0);
>> +
>> +    return ret;
>> +}