On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote: > On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monné wrote: > > On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote: > > > stopping queue may cause race and may not stop the queue really > > > after the API returns, and we have improved quiescing > > > interface and it really can block dispatching once it returns. > > > > > > So switch to quiesce/unquiece like what we did on other drivers > > > (NVMe, NBD, mtip32xx, ...) > > > > > > The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues() > > > used in blkif_queue_rq() and blkif_interrupt() are for congestion > > > control, we leave it as it is since it is safe for this usage. > > > > Again I yet don't understand the difference between those two, neither > > why start/stop is not fixed instead of introducing quiesce/unquiece. > > There are two usages covered by start/stop now: > > - congestion control for handling queue busy(BLK_STS_RESOURCE), now > only xen-blkfront and virtio-blk use that > > - other usages, such as in xlvbd_release_gendisk(), for blocking > IO to driver/device > > For the 1st case, it is usually fine to use stop/start > > For the 2nd case, stop queue isn't enough, and we can't guarantee > no IO is dispatched to device/driver after returning from stop queue, > for details. Most of this usage have been fixed by Sagi Grimberg: OK, so basically after calling stop the queue might still be running. > We can't use quiesce/unquiesce for replacing stop/start in the > case of BLK_STS_RESOURCE, because quiesce may sleep, and we needn't > block IO for this usage actually. Do you mean that quiesce/unquiesce cannot be used while holding a spinlock? > > > Not to mention that start/stop is not documented, which makes all this > > even more fun. > > Did you read comment of blk_mq_stop_hw_queue() and > blk_mq_stop_hw_queues() in linus tree? OK, this has been added very recently. > > > > Anyway I would like to ask, is the way to re-start a stopped queue the > > same way to unquiece? > > I don't know what your exact question, but it is definitely that > unquiesce is counter part of quiesce, and quiesce/unquiesce doesn't > depend on 'stopped' state any more if you take a look at the code. > > > > > If not I would rather prefer that start/stop or quiece/unquiece is > > used exclusively, in order to not make the code even more complex. It > > I do not think the code becomes more complex, please see the line change > of this patch: Before this patch blkfront used: blk_mq_stop_hw_queues blk_mq_start_stopped_hw_queues blk_mq_kick_requeue_list After the patch it uses: blk_mq_quiesce_queue blk_mq_unquiesce_queue blk_mq_stop_hw_queues blk_mq_start_stopped_hw_queues blk_mq_kick_requeue_list blk_mq_run_hw_queues It's not about line changes, but the amount of interfaces blkfront has to use. Apart from introducing the quiesce/unquiesce, you also introduce a call to blk_mq_run_hw_queues, which is not documented in the commit message. > > seems fairly easy to mess up and call "start" on a "quiesced" queue > > (or the other way around). > > Definitely it shouldn't be worried because start/stop is removed > in this patchset. Hm, how is that? I haven't seen any patch to blkfront to remove the usage of start/stop, am I missing something? Roger.
