From: Kent Overstreet <koverstreet@google.com>
To: "Theodore Ts'o" <tytso@mit.edu>,
linux-kernel@vger.kernel.org, linux-aio@kvack.org,
akpm@linux-foundation.org, Benjamin LaHaise <bcrl@kvack.org>,
Josh Boyer <jwboyer@redhat.com>, Zach Brown <zab@redhat.com>
Subject: Re: [PATCH 33/33] aio: fix kioctx not being freed after cancellation at exit time
Date: Tue, 9 Apr 2013 14:15:10 -0700 [thread overview]
Message-ID: <20130409211510.GL15749@google.com> (raw)
In-Reply-To: <20130402213550.GE5703@thunk.org>
On Tue, Apr 02, 2013 at 05:35:50PM -0400, Theodore Ts'o wrote:
> On Thu, Mar 21, 2013 at 09:35:54AM -0700, Kent Overstreet wrote:
> > From: Benjamin LaHaise <bcrl@kvack.org>
> >
> > The recent changes overhauling fs/aio.c introduced a bug that results in the
> > kioctx not being freed when outstanding kiocbs are cancelled at exit_aio()
> > time. Specifically, a kiocb that is cancelled has its completion events
> > discarded by batch_complete_aio(), which then fails to wake up the process
> > stuck in free_ioctx(). Fix this by removing the event suppression in
> > batch_complete_aio() and modify the wait_event() condition in free_ioctx()
> > appropriately.
>
> Once you remove the event suppression, then it means that every single
> cancelled AIO will result in ki_ctx->reqs_available getting double
> incremented, right?
I'm not sure where you're seeing the double increment...
Previously, when we were supressing the events we needed to increment
reqs_available to account for the fact that we wouldn't be doing a
put_reqs_available() when reaping the io_event.
I think the commit description could've been a bit better - this patch
is changing the behaviour of cancellation, and it makes more sense in
context with some of the other cancellation patches - instead of
returning the io_event via io_cancel(), we're returning it via
io_getevents() as it would be normally.
So all removing the event supression is doing is causing the io_events
from cancelled kiocbs to be handled just like any other io_event.
> But reqs_available gets used in more places than
> just free_ioctx(). It also gets used (for example) by
> get_reqs_available(), which in turn gets used by aio_get_req() to
> decide whether or not it's safe to allocate another aio_request.
> Since reqs_available is getting double allocated, won't we end up
> allowing more AIO requests to be issued --- more than we would have
> room in the ring?
>
> Am I missing something?
You're right about how reqs_available is used, but unless I'm missing
something the accounting is correct. Maybe we should go over it
together?
prev parent reply other threads:[~2013-04-09 21:15 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-21 16:35 [PATCH 00/33] AIO cleanups/performance improvements Kent Overstreet
2013-03-21 16:35 ` [PATCH 01/33] mm: remove old aio use_mm() comment Kent Overstreet
2013-03-28 14:13 ` Theodore Ts'o
2013-04-12 15:42 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 02/33] aio: remove dead code from aio.h Kent Overstreet
2013-03-28 14:13 ` Theodore Ts'o
2013-04-12 15:42 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 03/33] gadget: remove only user of aio retry Kent Overstreet
2013-03-21 16:42 ` Felipe Balbi
2013-04-12 15:42 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 04/33] aio: remove retry-based AIO Kent Overstreet
2013-03-28 14:20 ` Theodore Ts'o
2013-04-12 15:43 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 05/33] char: add aio_{read,write} to /dev/{null,zero} Kent Overstreet
2013-03-28 14:20 ` Theodore Ts'o
2013-04-12 15:43 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 06/33] aio: kill return value of aio_complete() Kent Overstreet
2013-03-28 14:22 ` Theodore Ts'o
2013-04-12 15:44 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 07/33] aio: add kiocb_cancel() Kent Overstreet
2013-03-28 14:54 ` Theodore Ts'o
2013-04-12 15:58 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 08/33] aio: move private stuff out of aio.h Kent Overstreet
2013-03-29 16:07 ` Theodore Ts'o
2013-04-12 15:59 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 09/33] aio: dprintk() -> pr_debug() Kent Overstreet
2013-03-21 17:38 ` Joe Perches
2013-03-29 16:17 ` Theodore Ts'o
2013-04-12 16:01 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 10/33] aio: do fget() after aio_get_req() Kent Overstreet
2013-03-29 16:48 ` Theodore Ts'o
2013-04-12 16:51 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 11/33] aio: make aio_put_req() lockless Kent Overstreet
2013-03-29 17:14 ` Theodore Ts'o
2013-04-12 21:01 ` Jeff Moyer
2013-03-21 16:35 ` [PATCH 12/33] aio: refcounting cleanup Kent Overstreet
2013-04-02 1:32 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 13/33] wait: add wait_event_hrtimeout() Kent Overstreet
2013-04-02 1:43 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 14/33] aio: make aio_read_evt() more efficient, convert to hrtimers Kent Overstreet
2013-04-02 1:58 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 15/33] aio: use flush_dcache_page() Kent Overstreet
2013-04-02 2:12 ` Theodore Ts'o
2013-04-09 21:07 ` Kent Overstreet
2013-03-21 16:35 ` [PATCH 16/33] aio: use cancellation list lazily Kent Overstreet
2013-04-02 2:36 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 17/33] aio: change reqs_active to include unreaped completions Kent Overstreet
2013-04-02 2:53 ` Theodore Ts'o
2013-04-02 15:47 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 18/33] aio: kill batch allocation Kent Overstreet
2013-04-02 3:03 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 19/33] aio: kill struct aio_ring_info Kent Overstreet
2013-04-02 3:26 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 20/33] aio: give shared kioctx fields their own cachelines Kent Overstreet
2013-04-02 3:27 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 21/33] aio: reqs_active -> reqs_available Kent Overstreet
2013-04-02 15:48 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 22/33] aio: percpu reqs_available Kent Overstreet
2013-04-02 16:03 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 23/33] generic dynamic per cpu refcounting Kent Overstreet
2013-04-02 16:27 ` Theodore Ts'o
2013-04-12 19:36 ` Kent Overstreet
2013-04-16 1:41 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 24/33] aio: percpu ioctx refcount Kent Overstreet
2013-04-02 16:28 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 25/33] aio: use xchg() instead of completion_lock Kent Overstreet
2013-04-02 16:35 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 26/33] aio: don't include aio.h in sched.h Kent Overstreet
2013-04-02 16:35 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 27/33] aio: kill ki_key Kent Overstreet
2013-04-02 16:36 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 28/33] aio: kill ki_retry Kent Overstreet
2013-04-02 18:46 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 29/33] block: Prep work for batch completion Kent Overstreet
2013-04-02 18:48 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 30/33] block, aio: batch completion for bios/kiocbs Kent Overstreet
2013-04-02 19:48 ` Theodore Ts'o
2013-04-10 21:59 ` Kent Overstreet
2013-04-02 19:53 ` Theodore Ts'o
2013-04-10 22:09 ` Kent Overstreet
2013-03-21 16:35 ` [PATCH 31/33] virtio-blk: convert to batch completion Kent Overstreet
2013-04-02 19:53 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 32/33] mtip32xx: " Kent Overstreet
2013-04-02 19:54 ` Theodore Ts'o
2013-03-21 16:35 ` [PATCH 33/33] aio: fix kioctx not being freed after cancellation at exit time Kent Overstreet
2013-04-02 21:35 ` Theodore Ts'o
2013-04-09 21:15 ` Kent Overstreet [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130409211510.GL15749@google.com \
--to=koverstreet@google.com \
--cc=akpm@linux-foundation.org \
--cc=bcrl@kvack.org \
--cc=jwboyer@redhat.com \
--cc=linux-aio@kvack.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=zab@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox