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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
