From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from verein.lst.de ([213.95.11.211]:58435 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730504AbeLUL2u (ORCPT ); Fri, 21 Dec 2018 06:28:50 -0500 Date: Fri, 21 Dec 2018 12:28:49 +0100 From: Christoph Hellwig To: Jens Axboe Cc: linux-fsdevel@vger.kernel.org, linux-aio@kvack.org, hch@lst.de, viro@zeniv.linux.org.uk Subject: Re: [PATCH 08/22] aio: add submission side request cache Message-ID: <20181221112849.GD7319@lst.de> References: <20181218154230.3120-1-axboe@kernel.dk> <20181218154230.3120-9-axboe@kernel.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181218154230.3120-9-axboe@kernel.dk> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Dec 18, 2018 at 08:42:16AM -0700, Jens Axboe wrote: > We have to add each submitted polled request to the io_context > poll_submitted list, which means we have to grab the poll_lock. We > already use the block plug to batch submissions if we're doing a batch > of IO submissions, extend that to cover the poll requests internally as > well. The way we make this conditional looks rather odd. What about something like the untested patch below to clean this up a bit? I also think that we don't need the explicit aio_flush_state_reqs call in aio_submit_state_end given that blk_finish_plug already calls the supplied callback. index d22913b7de74..ebf5d787d03b 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -235,18 +235,18 @@ struct aio_kiocb { }; struct aio_submit_state { +#ifdef CONFIG_BLOCK struct kioctx *ctx; struct blk_plug plug; -#ifdef CONFIG_BLOCK struct blk_plug_cb plug_cb; -#endif /* * Polled iocbs that have been submitted, but not added to the ctx yet */ struct list_head req_list; unsigned int req_count; +#endif }; /*------ sysctl variables----*/ @@ -263,15 +263,6 @@ static struct vfsmount *aio_mnt; static const struct file_operations aio_ring_fops; static const struct address_space_operations aio_ctx_aops; -/* - * We rely on block level unplugs to flush pending requests, if we schedule - */ -#ifdef CONFIG_BLOCK -static const bool aio_use_state_req_list = true; -#else -static const bool aio_use_state_req_list = false; -#endif - static void aio_iopoll_reap_events(struct kioctx *); static struct file *aio_private_file(struct kioctx *ctx, loff_t nr_pages) @@ -1849,52 +1840,55 @@ static inline void aio_rw_done(struct kiocb *req, ssize_t ret) } } +#ifdef CONFIG_BLOCK /* - * Called either at the end of IO submission, or through a plug callback - * because we're going to schedule. Moves out local batch of requests to - * the ctx poll list, so they can be found for polling + reaping. + * Called either at the end of IO submission, or through a plug callback because + * we're going to schedule. Moves out local batch of requests to the ctx poll + * list, so they can be found for polling + reaping. */ -static void aio_flush_state_reqs(struct kioctx *ctx, - struct aio_submit_state *state) +static void aio_flush_state_reqs(struct aio_submit_state *state) { - spin_lock(&ctx->poll_lock); - list_splice_tail_init(&state->req_list, &ctx->poll_submitted); - spin_unlock(&ctx->poll_lock); + spin_lock(&state->ctx->poll_lock); + list_splice_tail_init(&state->req_list, &state->ctx->poll_submitted); + spin_unlock(&state->ctx->poll_lock); state->req_count = 0; } +static void aio_iopoll_add_iocb_to_state(struct aio_submit_state *state, + struct aio_kiocb *kiocb) +{ + /* + * For fast devices, IO may have already completed. If it has, add it to + * the front so we find it first. We can't add to the poll_done list as + * that is used without locking from the completion side. + */ + if (test_bit(KIOCB_F_POLL_COMPLETED, &kiocb->ki_flags)) + list_add(&kiocb->ki_list, &state->req_list); + else + list_add_tail(&kiocb->ki_list, &state->req_list); + if (++state->req_count >= AIO_IOPOLL_BATCH) + aio_flush_state_reqs(state); +} +#else +#define aio_iopoll_add_iocb_to_state(state, kiocb) do { } while (0) +#endif + /* * After the iocb has been issued, it's safe to be found on the poll list. * Adding the kiocb to the list AFTER submission ensures that we don't * find it from a io_getevents() thread before the issuer is done accessing * the kiocb cookie. */ -static void aio_iopoll_iocb_issued(struct aio_submit_state *state, - struct aio_kiocb *kiocb) +static void aio_iopoll_add_iocb_to_list(struct aio_kiocb *kiocb) { - /* - * For fast devices, IO may have already completed. If it has, add - * it to the front so we find it first. We can't add to the poll_done - * list as that's unlocked from the completion side. - */ - const int front = test_bit(KIOCB_F_POLL_COMPLETED, &kiocb->ki_flags); struct kioctx *ctx = kiocb->ki_ctx; - if (!state || !aio_use_state_req_list) { - spin_lock(&ctx->poll_lock); - if (front) - list_add(&kiocb->ki_list, &ctx->poll_submitted); - else - list_add_tail(&kiocb->ki_list, &ctx->poll_submitted); - spin_unlock(&ctx->poll_lock); - } else { - if (front) - list_add(&kiocb->ki_list, &state->req_list); - else - list_add_tail(&kiocb->ki_list, &state->req_list); - if (++state->req_count >= AIO_IOPOLL_BATCH) - aio_flush_state_reqs(ctx, state); - } + spin_lock(&ctx->poll_lock); + if (test_bit(KIOCB_F_POLL_COMPLETED, &kiocb->ki_flags)) + list_add(&kiocb->ki_list, &ctx->poll_submitted); + else + list_add_tail(&kiocb->ki_list, &ctx->poll_submitted); + spin_unlock(&ctx->poll_lock); } static ssize_t aio_read(struct aio_kiocb *kiocb, const struct iocb *iocb, @@ -2294,7 +2288,11 @@ static int __io_submit_one(struct kioctx *ctx, const struct iocb *iocb, ret = -EAGAIN; goto out_put_req; } - aio_iopoll_iocb_issued(state, req); + + if (state) + aio_iopoll_add_iocb_to_state(state, req); + else + aio_iopoll_add_iocb_to_list(req); } return 0; out_put_req: @@ -2325,9 +2323,8 @@ static void aio_state_unplug(struct blk_plug_cb *cb, bool from_schedule) state = container_of(cb, struct aio_submit_state, plug_cb); if (!list_empty(&state->req_list)) - aio_flush_state_reqs(state->ctx, state); + aio_flush_state_reqs(state); } -#endif /* * Batched submission is done, ensure local IO is flushed out. @@ -2336,24 +2333,27 @@ static void aio_submit_state_end(struct aio_submit_state *state) { blk_finish_plug(&state->plug); if (!list_empty(&state->req_list)) - aio_flush_state_reqs(state->ctx, state); + aio_flush_state_reqs(state); } /* * Start submission side cache. */ -static void aio_submit_state_start(struct aio_submit_state *state, - struct kioctx *ctx) +static struct aio_submit_state * +aio_submit_state_start(struct aio_submit_state *state, struct kioctx *ctx) { state->ctx = ctx; INIT_LIST_HEAD(&state->req_list); state->req_count = 0; -#ifdef CONFIG_BLOCK state->plug_cb.callback = aio_state_unplug; blk_start_plug(&state->plug); list_add(&state->plug_cb.list, &state->plug.cb_list); -#endif + return state; } +#else +#define aio_submit_state_end(state) do { } while (0) +#define aio_submit_state_start(state, ctx) (state) +#endif /* CONFIG_BLOCK */ /* sys_io_submit: * Queue the nr iocbs pointed to by iocbpp for processing. Returns @@ -2387,10 +2387,9 @@ SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr, if (nr > ctx->nr_events) nr = ctx->nr_events; - if (nr > AIO_PLUG_THRESHOLD) { - aio_submit_state_start(&state, ctx); - statep = &state; - } + if (IS_ENABLED(CONFIG_BLOCK) && nr > AIO_PLUG_THRESHOLD) + statep = aio_submit_state_start(&state, ctx); + for (i = 0; i < nr; i++) { struct iocb __user *user_iocb;