3.16.57-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Oleg Nesterov oleg@redhat.com
commit 4b70ac5fd9b58bfaa5f25b4ea48f528aefbf3308 upstream.
On 04/30, Benjamin LaHaise wrote:
ctx->mmap_size = 0;kill_ioctx(mm, ctx, NULL);
if (ctx) {ctx->mmap_size = 0;kill_ioctx(mm, ctx, NULL);}Rather than indenting and moving the two lines changing mmap_size and the kill_ioctx() call, why not just do "if (!ctx) ... continue;"? That reduces the number of lines changed and avoid excessive indentation.
OK. To me the code looks better/simpler with "if (ctx)", but this is subjective of course, I won't argue.
The patch still removes the empty line between mmap_size = 0 and kill_ioctx(), we reset mmap_size only for kill_ioctx(). But feel free to remove this change.
------------------------------------------------------------------------------- Subject: [PATCH v3 1/2] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()
1. We can read ->ioctx_table only once and we do not read rcu_read_lock() or even rcu_dereference().
This mm has no users, nobody else can play with ->ioctx_table. Otherwise the code is buggy anyway, if we need rcu_read_lock() in a loop because ->ioctx_table can be updated then kfree(table) is obviously wrong.
2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid munmap(), but another reason is that we simply can't do vm_munmap() unless current->mm == mm and this is not true in general, the caller is mmput().
3. We do not really need to nullify mm->ioctx_table before return, probably the current code does this to catch the potential problems. But in this case RCU_INIT_POINTER(NULL) looks better.
Signed-off-by: Oleg Nesterov oleg@redhat.com Signed-off-by: Benjamin LaHaise bcrl@kvack.org [bwh: Backported to 3.16: Adjust context to apply after backport of commit 6098b45b32e6 "aio: block exit_aio() until all context requests are completed"] Signed-off-by: Ben Hutchings ben@decadent.org.uk --- --- a/fs/aio.c +++ b/fs/aio.c @@ -803,46 +803,35 @@ EXPORT_SYMBOL(wait_on_sync_kiocb); */ void exit_aio(struct mm_struct *mm) { - struct kioctx_table *table; - struct kioctx *ctx; - unsigned i = 0; + struct kioctx_table *table = rcu_dereference_raw(mm->ioctx_table); + int i;
- while (1) { + if (!table) + return; + + for (i = 0; i < table->nr; ++i) { + struct kioctx *ctx = table->table[i]; struct completion requests_done = COMPLETION_INITIALIZER_ONSTACK(requests_done);
- rcu_read_lock(); - table = rcu_dereference(mm->ioctx_table); - - do { - if (!table || i >= table->nr) { - rcu_read_unlock(); - rcu_assign_pointer(mm->ioctx_table, NULL); - if (table) - kfree(table); - return; - } - - ctx = table->table[i++]; - } while (!ctx); - - rcu_read_unlock(); - + if (!ctx) + continue; /* - * We don't need to bother with munmap() here - - * exit_mmap(mm) is coming and it'll unmap everything. - * Since aio_free_ring() uses non-zero ->mmap_size - * as indicator that it needs to unmap the area, - * just set it to 0; aio_free_ring() is the only - * place that uses ->mmap_size, so it's safe. + * We don't need to bother with munmap() here - exit_mmap(mm) + * is coming and it'll unmap everything. And we simply can't, + * this is not necessarily our ->mm. + * Since kill_ioctx() uses non-zero ->mmap_size as indicator + * that it needs to unmap the area, just set it to 0. */ ctx->mmap_size = 0; - kill_ioctx(mm, ctx, &requests_done);
/* Wait until all IO for the context are done. */ wait_for_completion(&requests_done); } + + RCU_INIT_POINTER(mm->ioctx_table, NULL); + kfree(table); }
static void put_reqs_available(struct kioctx *ctx, unsigned nr)