On Mon, Jul 17, 2017 at 05:02:27PM +0100, Roger Pau Monné wrote: > 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? Yes. > > > > > > 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? http://marc.info/?l=linux-block&m=150007418321196&w=2 -- Ming
