Date: Tue, 13 Jun 2017 11:14:12 +0300
From: Cyrill Gorcunov <gorcunov@gmail.com>
To: Kirill Tkhai <ktkhai@virtuozzo.com>
Cc: avagin@openvz.org, linux-kernel@vger.kernel.org, bcrl@kvack.org,
        viro@zeniv.linux.org.uk, akpm@linux-foundation.org,
        xemul@virtuozzo.com
Subject: Re: [PATCH] aio: Add command to wait completion of all requests
Message-ID: <20170613081412.GA23987@uranus.lan>
References: <149700173837.15252.8419518498235874341.stgit@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <149700173837.15252.8419518498235874341.stgit@localhost.localdomain>
User-Agent: Mutt/1.8.0 (2017-02-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1347
Lines: 49

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?

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