* [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case @ 2015-04-19 18:19 Dmitry Krivenok 2015-04-27 16:52 ` Dmitry Krivenok 2015-04-27 17:25 ` Jens Axboe 0 siblings, 2 replies; 5+ messages in thread From: Dmitry Krivenok @ 2015-04-19 18:19 UTC (permalink / raw) To: linux-fsdevel When we fail to allocate new cmd in null_rq_prep_fn we return BLKPREP_DEFER which is not handled properly. In single-queue mode of null_blk the following command hangs forever in io_schedule(): $ dd if=/dev/nullb0 of=/dev/null bs=8M count=5000 iflag=direct The reason is that when 64 commands have been allocated, the 65th command allocation will fail due to missing free tag. The request, however, will be kept in the queue which will never be started again (unless you run another command that does I/O to /dev/nullb0). This small patch tries to solve the issue by stopping the queue when we detect that all tags were exhausted and starting it again when we free the tag. I've verified that the command mentioned above doesn't hang anymore and also made sure that null_blk with my change survives fio-based stress tests. Signed-off-by: Dmitry V. Krivenok <krivenok.dmitry@gmail.com> --- drivers/block/null_blk.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c index 65cd61a..4ac684b 100644 --- a/drivers/block/null_blk.c +++ b/drivers/block/null_blk.c @@ -25,6 +25,7 @@ struct nullb_queue { unsigned int queue_depth; struct nullb_cmd *cmds; + bool no_cmds; }; struct nullb { @@ -171,6 +172,13 @@ static unsigned int get_tag(struct nullb_queue *nq) static void free_cmd(struct nullb_cmd *cmd) { put_tag(cmd->nq, cmd->tag); + if(cmd->nq->no_cmds) { + unsigned long flags; + cmd->nq->no_cmds = false; + spin_lock_irqsave(cmd->rq->q->queue_lock, flags); + blk_start_queue(cmd->rq->q); + spin_unlock_irqrestore(cmd->rq->q->queue_lock, flags); + } } static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq) @@ -195,6 +203,9 @@ static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, int can_wait) DEFINE_WAIT(wait); cmd = __alloc_cmd(nq); + if (!cmd && !can_wait) { + nq->no_cmds = true; + } if (cmd || !can_wait) return cmd; @@ -341,6 +352,7 @@ static int null_rq_prep_fn(struct request_queue *q, struct request *req) static void null_request_fn(struct request_queue *q) { struct request *rq; + struct nullb_queue *nq = nullb_to_queue(q->queuedata); while ((rq = blk_fetch_request(q)) != NULL) { struct nullb_cmd *cmd = rq->special; @@ -349,6 +361,9 @@ static void null_request_fn(struct request_queue *q) null_handle_cmd(cmd); spin_lock_irq(q->queue_lock); } + if(nq->no_cmds) { + blk_stop_queue(q); + } } static int null_queue_rq(struct blk_mq_hw_ctx *hctx, @@ -430,6 +445,7 @@ static int setup_commands(struct nullb_queue *nq) if (!nq->cmds) return -ENOMEM; + nq->no_cmds = false; tag_size = ALIGN(nq->queue_depth, BITS_PER_LONG) / BITS_PER_LONG; nq->tag_map = kzalloc(tag_size * sizeof(unsigned long), GFP_KERNEL); if (!nq->tag_map) { -- 2.3.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case 2015-04-19 18:19 [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case Dmitry Krivenok @ 2015-04-27 16:52 ` Dmitry Krivenok 2015-04-27 17:25 ` Jens Axboe 1 sibling, 0 replies; 5+ messages in thread From: Dmitry Krivenok @ 2015-04-27 16:52 UTC (permalink / raw) To: linux-fsdevel; +Cc: Jens Axboe Folks, any comments on this patch? Thanks, Dmitry On Sun, Apr 19, 2015 at 9:19 PM, Dmitry Krivenok <krivenok.dmitry@gmail.com> wrote: > When we fail to allocate new cmd in null_rq_prep_fn we return BLKPREP_DEFER > which is not handled properly. In single-queue mode of null_blk the following > command hangs forever in io_schedule(): > $ dd if=/dev/nullb0 of=/dev/null bs=8M count=5000 iflag=direct > > The reason is that when 64 commands have been allocated, the 65th command > allocation will fail due to missing free tag. The request, however, will be > kept in the queue which will never be started again (unless you run another > command that does I/O to /dev/nullb0). > > This small patch tries to solve the issue by stopping the queue when we > detect that all tags were exhausted and starting it again when we free the tag. > > I've verified that the command mentioned above doesn't hang anymore and also > made sure that null_blk with my change survives fio-based stress tests. > > Signed-off-by: Dmitry V. Krivenok <krivenok.dmitry@gmail.com> > --- > drivers/block/null_blk.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c > index 65cd61a..4ac684b 100644 > --- a/drivers/block/null_blk.c > +++ b/drivers/block/null_blk.c > @@ -25,6 +25,7 @@ struct nullb_queue { > unsigned int queue_depth; > > struct nullb_cmd *cmds; > + bool no_cmds; > }; > > struct nullb { > @@ -171,6 +172,13 @@ static unsigned int get_tag(struct nullb_queue *nq) > static void free_cmd(struct nullb_cmd *cmd) > { > put_tag(cmd->nq, cmd->tag); > + if(cmd->nq->no_cmds) { > + unsigned long flags; > + cmd->nq->no_cmds = false; > + spin_lock_irqsave(cmd->rq->q->queue_lock, flags); > + blk_start_queue(cmd->rq->q); > + spin_unlock_irqrestore(cmd->rq->q->queue_lock, flags); > + } > } > > static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq) > @@ -195,6 +203,9 @@ static struct nullb_cmd *alloc_cmd(struct > nullb_queue *nq, int can_wait) > DEFINE_WAIT(wait); > > cmd = __alloc_cmd(nq); > + if (!cmd && !can_wait) { > + nq->no_cmds = true; > + } > if (cmd || !can_wait) > return cmd; > > @@ -341,6 +352,7 @@ static int null_rq_prep_fn(struct request_queue > *q, struct request *req) > static void null_request_fn(struct request_queue *q) > { > struct request *rq; > + struct nullb_queue *nq = nullb_to_queue(q->queuedata); > > while ((rq = blk_fetch_request(q)) != NULL) { > struct nullb_cmd *cmd = rq->special; > @@ -349,6 +361,9 @@ static void null_request_fn(struct request_queue *q) > null_handle_cmd(cmd); > spin_lock_irq(q->queue_lock); > } > + if(nq->no_cmds) { > + blk_stop_queue(q); > + } > } > > static int null_queue_rq(struct blk_mq_hw_ctx *hctx, > @@ -430,6 +445,7 @@ static int setup_commands(struct nullb_queue *nq) > if (!nq->cmds) > return -ENOMEM; > > + nq->no_cmds = false; > tag_size = ALIGN(nq->queue_depth, BITS_PER_LONG) / BITS_PER_LONG; > nq->tag_map = kzalloc(tag_size * sizeof(unsigned long), GFP_KERNEL); > if (!nq->tag_map) { > -- > 2.3.5 -- Sincerely yours, Dmitry V. Krivenok e-mail: krivenok.dmitry@gmail.com skype: krivenok_dmitry jabber: krivenok_dmitry@jabber.ru icq: 242-526-443 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case 2015-04-19 18:19 [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case Dmitry Krivenok 2015-04-27 16:52 ` Dmitry Krivenok @ 2015-04-27 17:25 ` Jens Axboe 2015-04-27 17:36 ` Jens Axboe 1 sibling, 1 reply; 5+ messages in thread From: Jens Axboe @ 2015-04-27 17:25 UTC (permalink / raw) To: Dmitry Krivenok, linux-fsdevel On 04/19/2015 12:19 PM, Dmitry Krivenok wrote: > When we fail to allocate new cmd in null_rq_prep_fn we return BLKPREP_DEFER > which is not handled properly. In single-queue mode of null_blk the following > command hangs forever in io_schedule(): > $ dd if=/dev/nullb0 of=/dev/null bs=8M count=5000 iflag=direct > > The reason is that when 64 commands have been allocated, the 65th command > allocation will fail due to missing free tag. The request, however, will be > kept in the queue which will never be started again (unless you run another > command that does I/O to /dev/nullb0). > > This small patch tries to solve the issue by stopping the queue when we > detect that all tags were exhausted and starting it again when we free the tag. > > I've verified that the command mentioned above doesn't hang anymore and also > made sure that null_blk with my change survives fio-based stress tests. You are right, legacy request_fn mode has a bug there. I'd get rid of the no_cmds bool, though. If we fail allocating a command in alloc_cmd() and we're in NULL_Q_RQ mode, stop the queue. In free_cmd(), again if we're in NULL_Q_RQ_MODE, check the stopped flag and start the queue if it is set. -- Jens Axboe ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case 2015-04-27 17:25 ` Jens Axboe @ 2015-04-27 17:36 ` Jens Axboe 2015-04-27 21:47 ` Dmitry Krivenok 0 siblings, 1 reply; 5+ messages in thread From: Jens Axboe @ 2015-04-27 17:36 UTC (permalink / raw) To: Dmitry Krivenok, linux-fsdevel [-- Attachment #1: Type: text/plain, Size: 1348 bytes --] On 04/27/2015 11:25 AM, Jens Axboe wrote: > On 04/19/2015 12:19 PM, Dmitry Krivenok wrote: >> When we fail to allocate new cmd in null_rq_prep_fn we return >> BLKPREP_DEFER >> which is not handled properly. In single-queue mode of null_blk the >> following >> command hangs forever in io_schedule(): >> $ dd if=/dev/nullb0 of=/dev/null bs=8M count=5000 iflag=direct >> >> The reason is that when 64 commands have been allocated, the 65th command >> allocation will fail due to missing free tag. The request, however, >> will be >> kept in the queue which will never be started again (unless you run >> another >> command that does I/O to /dev/nullb0). >> >> This small patch tries to solve the issue by stopping the queue when we >> detect that all tags were exhausted and starting it again when we free >> the tag. >> >> I've verified that the command mentioned above doesn't hang anymore >> and also >> made sure that null_blk with my change survives fio-based stress tests. > > You are right, legacy request_fn mode has a bug there. I'd get rid of > the no_cmds bool, though. If we fail allocating a command in alloc_cmd() > and we're in NULL_Q_RQ mode, stop the queue. In free_cmd(), again if > we're in NULL_Q_RQ_MODE, check the stopped flag and start the queue if > it is set. Something like the attached, totally untested. -- Jens Axboe [-- Attachment #2: nullb-defer.patch --] [-- Type: text/x-patch, Size: 1413 bytes --] diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c index 65cd61a4145e..d7141d5ef4ff 100644 --- a/drivers/block/null_blk.c +++ b/drivers/block/null_blk.c @@ -24,6 +24,7 @@ struct nullb_queue { wait_queue_head_t wait; unsigned int queue_depth; + struct nullb *dev; struct nullb_cmd *cmds; }; @@ -153,6 +154,13 @@ static void put_tag(struct nullb_queue *nq, unsigned int tag) if (waitqueue_active(&nq->wait)) wake_up(&nq->wait); + + if (queue_mode == NULL_Q_RQ) { + struct nullb *nullb = nq->dev; + + if (blk_queue_stopped(nullb->q)) + blk_start_queue(nullb->q); + } } static unsigned int get_tag(struct nullb_queue *nq) @@ -196,7 +204,7 @@ static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, int can_wait) cmd = __alloc_cmd(nq); if (cmd || !can_wait) - return cmd; + goto out; do { prepare_to_wait(&nq->wait, &wait, TASK_UNINTERRUPTIBLE); @@ -208,6 +216,11 @@ static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, int can_wait) } while (1); finish_wait(&nq->wait, &wait); +out: + if (!cmd) { + BUG_ON(queue_mode != NULL_Q_RQ); + blk_stop_queue(nq->dev->q); + } return cmd; } @@ -372,6 +385,7 @@ static void null_init_queue(struct nullb *nullb, struct nullb_queue *nq) init_waitqueue_head(&nq->wait); nq->queue_depth = nullb->queue_depth; + nq->dev = nullb; } static int null_init_hctx(struct blk_mq_hw_ctx *hctx, void *data, ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case 2015-04-27 17:36 ` Jens Axboe @ 2015-04-27 21:47 ` Dmitry Krivenok 0 siblings, 0 replies; 5+ messages in thread From: Dmitry Krivenok @ 2015-04-27 21:47 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-fsdevel [-- Attachment #1: Type: text/plain, Size: 1836 bytes --] I think we need to surround blk_start_queue() call with queue lock/unlock as it expects that lock should be held. Slightly modified patch is attached. I've tested it using the same commands as before. Thanks, Dmitry On Mon, Apr 27, 2015 at 8:36 PM, Jens Axboe <axboe@kernel.dk> wrote: > On 04/27/2015 11:25 AM, Jens Axboe wrote: >> >> On 04/19/2015 12:19 PM, Dmitry Krivenok wrote: >>> >>> When we fail to allocate new cmd in null_rq_prep_fn we return >>> BLKPREP_DEFER >>> which is not handled properly. In single-queue mode of null_blk the >>> following >>> command hangs forever in io_schedule(): >>> $ dd if=/dev/nullb0 of=/dev/null bs=8M count=5000 iflag=direct >>> >>> The reason is that when 64 commands have been allocated, the 65th command >>> allocation will fail due to missing free tag. The request, however, >>> will be >>> kept in the queue which will never be started again (unless you run >>> another >>> command that does I/O to /dev/nullb0). >>> >>> This small patch tries to solve the issue by stopping the queue when we >>> detect that all tags were exhausted and starting it again when we free >>> the tag. >>> >>> I've verified that the command mentioned above doesn't hang anymore >>> and also >>> made sure that null_blk with my change survives fio-based stress tests. >> >> >> You are right, legacy request_fn mode has a bug there. I'd get rid of >> the no_cmds bool, though. If we fail allocating a command in alloc_cmd() >> and we're in NULL_Q_RQ mode, stop the queue. In free_cmd(), again if >> we're in NULL_Q_RQ_MODE, check the stopped flag and start the queue if >> it is set. > > > Something like the attached, totally untested. > > -- > Jens Axboe > -- Sincerely yours, Dmitry V. Krivenok e-mail: krivenok.dmitry@gmail.com skype: krivenok_dmitry jabber: krivenok_dmitry@jabber.ru icq: 242-526-443 [-- Attachment #2: 0001-null_blk-fix-handling-of-BLKPREP_DEFER-case.patch --] [-- Type: text/x-patch, Size: 2072 bytes --] From c5dfcc7f3d3f79688dbf06aeb099f1a036f42bb9 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Krivenok" <krivenok.dmitry@gmail.com> Date: Tue, 28 Apr 2015 00:39:21 +0300 Subject: [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case This is a fix for BLKPREP_DEFER case in single queue mode. It's rework of original implementation based on feedback from Jens (basically his version plus locking fix). Signed-off-by: Dmitry V. Krivenok <krivenok.dmitry@gmail.com> --- drivers/block/null_blk.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c index 65cd61a..a26b57f 100644 --- a/drivers/block/null_blk.c +++ b/drivers/block/null_blk.c @@ -24,6 +24,7 @@ struct nullb_queue { wait_queue_head_t wait; unsigned int queue_depth; + struct nullb *dev; struct nullb_cmd *cmds; }; @@ -153,6 +154,16 @@ static void put_tag(struct nullb_queue *nq, unsigned int tag) if (waitqueue_active(&nq->wait)) wake_up(&nq->wait); + + if (queue_mode == NULL_Q_RQ) { + struct nullb *nullb = nq->dev; + + if (blk_queue_stopped(nullb->q)) { + spin_lock_irq(nullb->q->queue_lock); + blk_start_queue(nullb->q); + spin_unlock_irq(nullb->q->queue_lock); + } + } } static unsigned int get_tag(struct nullb_queue *nq) @@ -196,7 +207,7 @@ static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, int can_wait) cmd = __alloc_cmd(nq); if (cmd || !can_wait) - return cmd; + goto out; do { prepare_to_wait(&nq->wait, &wait, TASK_UNINTERRUPTIBLE); @@ -208,6 +219,11 @@ static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, int can_wait) } while (1); finish_wait(&nq->wait, &wait); +out: + if (!cmd) { + BUG_ON(queue_mode != NULL_Q_RQ); + blk_stop_queue(nq->dev->q); + } return cmd; } @@ -372,6 +388,7 @@ static void null_init_queue(struct nullb *nullb, struct nullb_queue *nq) init_waitqueue_head(&nq->wait); nq->queue_depth = nullb->queue_depth; + nq->dev = nullb; } static int null_init_hctx(struct blk_mq_hw_ctx *hctx, void *data, -- 2.3.6 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-04-27 21:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-04-19 18:19 [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case Dmitry Krivenok 2015-04-27 16:52 ` Dmitry Krivenok 2015-04-27 17:25 ` Jens Axboe 2015-04-27 17:36 ` Jens Axboe 2015-04-27 21:47 ` Dmitry Krivenok
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.