Re: [PATCH 17/20] aio: support for IO polling

[Jens Axboe <axboe@kernel.dk>]

On 11/27/18 2:53 AM, Benny Halevy wrote:
>> @@ -818,11 +853,15 @@ static int kill_ioctx(struct mm_struct *mm, struct kioctx *ctx,
>>  {
>>  	struct kioctx_table *table;
>>  
>> +	mutex_lock(&ctx->getevents_lock);
>>  	spin_lock(&mm->ioctx_lock);
>>  	if (atomic_xchg(&ctx->dead, 1)) {
>>  		spin_unlock(&mm->ioctx_lock);
>> +		mutex_unlock(&ctx->getevents_lock);
>>  		return -EINVAL;
>>  	}
>> +	aio_iopoll_reap_events(ctx);
>> +	mutex_unlock(&ctx->getevents_lock);
> 
> Is it worth handling the mutex lock and calling aio_iopoll_reap_events
> only if (ctx->flags & IOCTX_FLAG_IOPOLL)?  If so, testing it can be
> removed from aio_iopoll_reap_events() (and maybe it could even
> be open coded
> here since this is its only call site apparently)

I don't think it really matters, this only happens when you tear down an
io_context. FWIW, I think it's cleaner to retain the test in the
function, not outside it.

>> @@ -1072,6 +1112,15 @@ static inline void iocb_put(struct aio_kiocb *iocb)
>>  	}
>>  }
>>  
>> +static void iocb_put_many(struct kioctx *ctx, void **iocbs, int *nr)
>> +{
>> +	if (nr) {
> 
> How can nr by NULL?
> And what's the point of supporting this case?
> Did you mean: if (*nr)?
> (In this case, if safe to call the functions below with *nr==0,
> I'm not sure it's worth optimizing... especially since this is a static
> function and its callers make sure to call it only when *nr > 0)

Indeed, that should be if (*nr), thanks! The slub implementation of the
bulk free complains if you pass in nr == 0. Outside of that, a single
check should be better than checking in multiple spots.

>> @@ -1261,6 +1310,166 @@ static bool aio_read_events(struct kioctx *ctx, long min_nr, long nr,
>>  	return ret < 0 || *i >= min_nr;
>>  }
>>  
>> +#define AIO_IOPOLL_BATCH	8
>> +
>> +/*
>> + * Process completed iocb iopoll entries, copying the result to userspace.
>> + */
>> +static long aio_iopoll_reap(struct kioctx *ctx, struct io_event __user *evs,
>> +			    unsigned int *nr_events, long max)
>> +{
>> +	void *iocbs[AIO_IOPOLL_BATCH];
>> +	struct aio_kiocb *iocb, *n;
>> +	int to_free = 0, ret = 0;
>> +
>> +	list_for_each_entry_safe(iocb, n, &ctx->poll_completing, ki_list) {
>> +		if (*nr_events == max)
> 
> *nr_events >= max would be safer.

I don't see how we can get there with it being larger than already, that
would be a big bug if we fill more events than userspace asked for.

>> @@ -1570,17 +1829,43 @@ static inline void aio_rw_done(struct kiocb *req, ssize_t ret)
>>  	default:
>>  		req->ki_complete(req, ret, 0);
>>  	}
>> +
> 
> nit: this hunk is probably unintentional

Looks like it, I'll kill it.


-- 
Jens Axboe