lkml.org 
[lkml]   [2018]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[PATCH 3.16 354/410] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()
    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)
    \
     
     \ /
      Last update: 2018-06-07 16:17    [W:4.118 / U:0.408 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site