Re: [PATCH 1/1] null_blk: fix handling of BLKPREP_DEFER case

[Jens Axboe <axboe@kernel.dk>]

[-- 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,