Re: [PATCH] blk-mq: Avoid that I/O hangs in bt_get()

[Jens Axboe <axboe@kernel.dk>]

On 10/07/2014 02:46 AM, Bart Van Assche wrote:
> On 10/06/14 20:53, Jens Axboe wrote:
>> On 10/06/2014 11:40 AM, Jens Axboe wrote:
>>> I've been able to reproduce this this morning, and your patch does seem
>>> to fix it. The inc/add logic is making my head spin a bit. And we now
>>> end up banging a lot more on the waitqueue lock through
>>> prepare_to_wait(), so there's a substantial performance regression to go
>>> with the change.
>>>
>>> I'll fiddle with this a bit and see if we can't retain existing
>>> performance properties under tag contention, while still fixing the hang.
>>
>> So I think your patch fixes the issue because it just keeps decrementing
>> the wait counts, hence waking up a lot more than it should. This is also
>> why I see a huge increase in wait queue spinlock time.
>>
>> Does this work for you? I think the issue is plainly that we end up
>> setting the batch counts too high. But tell me more about the number of
>> queues, the depth (total or per queue?), and the fio job you are running.
> 
> Hello Jens,
> 
> Thanks for looking into this. I can't reproduce the I/O lockup after 
> having reverted my patch and after having applied your patch. In the 
> test I ran fio was started with the following command-line options:
>  
> fio --bs=512 --ioengine=libaio --rw=randread --buffered=0 --numjobs=12 
> --iodepth=128 --iodepth_batch=64 --iodepth_batch_complete=64 --thread 
> --norandommap --loops=2147483648 --runtime=3600 --group_reporting 
> --gtod_reduce=1 --name=/dev/sdo --filename=/dev/sdo --invalidate=1
> 
> This job was run on a system with 12 CPU threads and against a SCSI 
> initiator driver for which the number of hardware contexts had been set 
> to 6. Queue depth per hardware queue was set to 127:
> $ cat /sys/class/scsi_host/host10/can_queue
> 127
> 
> This is what fio reports about the average queue depth:
> 
> IOdepths: 1=0.0%, 2=0.0%, 4=0.0%, 8=0.0%, 16=0.0%, 32=0.0%, >=64=100.0%
>   submit: 0=0.0%, 4=0.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=100.0%, >=64=0.0%
> complete: 0=0.0%, 4=0.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=100.0%, >=64=0.0%

Great, so that makes sense to me. I'll get the patch applied and marked
for stable. I'll mark it as fixes commit 4bb659b156996.

> 
> While we are at it, how about the patch below ? That patch shouldn't
> change any functionality but should make bt_clear_tag() slightly easier
> to read.

Agree, that looks cleaner and is more readable.

-- 
Jens Axboe