Subject: Re: [PATCH] aio: Add command to wait completion of all requests
To: Cyrill Gorcunov <gorcunov@gmail.com>
References: <149700173837.15252.8419518498235874341.stgit@localhost.localdomain>
 <20170613081412.GA23987@uranus.lan>
Cc: avagin@openvz.org, linux-kernel@vger.kernel.org, bcrl@kvack.org,
        viro@zeniv.linux.org.uk, akpm@linux-foundation.org,
        xemul@virtuozzo.com
From: Kirill Tkhai <ktkhai@virtuozzo.com>
Message-ID: <9d133393-1ea7-c370-24b0-7bb565428b7e@virtuozzo.com>
Date: Tue, 13 Jun 2017 12:45:39 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170613081412.GA23987@uranus.lan>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2017 09:45:55.9657 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0801MB1337
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1603
Lines: 54

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