On Tue, 2018-11-27 at 08:24 -0700, Jens Axboe wrote:

On 11/27/18 2:53 AM, Benny Halevy wrote:

quoted

quoted

@@ -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.

quoted

quoted

@@ -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.

Cool. The compiler might also optimize it away when inlining this function
if the caller tests *nr for being non-zero too.

quoted

quoted

@@ -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.

Currently we indeed can't, but if the code changes in the future
and we do, this will reduce the damage - hence being safer (and it costs nothing
in terms of performance).

quoted

quoted

@@ -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.