Re: [PATCH 2/2] aio: make aio_read_events_ring be aware of aio_complete

[Gu Zheng <guz.fnst@cn.fujitsu.com>]

Hi Kent,
Sorry for late replay.
On 03/01/2014 04:52 AM, Kent Overstreet wrote:

> On Fri, Feb 28, 2014 at 06:27:18PM +0800, Gu Zheng wrote:
>> Commit 5ffac122dbda8(aio: Don't use ctx->tail unnecessarily) uses
>> ring->tail rather than the ctx->tail, but with this change, we fetch 'tail'
>> only once at the start, so that we can not be aware of adding event by aio_complete
>> when reading events. It seems a regression.
> 
> "Seems a regression" is not good enough. What breaks? 

Previously without the commit 5ffac122dbda8, we used tail from aio context to judge
whether the ring buffer has events(have not been read) each time, as following:
avail = (head <= ctx->tail ? ctx->tail : ctx->nr_events) - head;
so that we can be aware of new events that added by aio_complete when we are reading the
current ones, and we can read the new added events together.
E.g. we want to read 10 events, only 8 events now, add 2 events added while we copying
current 8 events to userspace, and we can read the 2 new events in the next circle.
But with the commit 5ffac122dbda8, we fetch tail from ring buffer only once when we start
to read events, so that we can not be aware of events that added by aio_complete while we
are reading current events(e.g. copy currents events to user space). Such as the case
mentioned above can only read 8 events, though these are 2 new events in the ring buffer.
Is not it a regression, or am I missing something?

Regards,
Gu

> 
>> So here we fetch the ring->tail in start of the loop each time to make it be
>> aware of adding event from aio_complete.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  fs/aio.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/aio.c b/fs/aio.c
>> index 7eaa631..f5b8551 100644
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -1029,10 +1029,14 @@ static long aio_read_events_ring(struct kioctx *ctx,
>>  		struct io_event *ev;
>>  		struct page *page;
>>  
>> -		avail = (head <= tail ?  tail : ctx->nr_events) - head;
>> +		ring = kmap_atomic(ctx->ring_pages[0]);
>> +		tail = ring->tail;
>> +		kunmap_atomic(ring);
>> +
>>  		if (head == tail)
>>  			break;
>>  
>> +		avail = (head <= tail ?  tail : ctx->nr_events) - head;
>>  		avail = min(avail, nr - ret);
>>  		avail = min_t(long, avail, AIO_EVENTS_PER_PAGE -
>>  			    ((head + AIO_EVENTS_OFFSET) % AIO_EVENTS_PER_PAGE));
>> -- 
>> 1.7.7
>>
>