On 8/9/17 13:15, Bart Van Assche wrote: > On Wed, 2017-08-09 at 13:07 +0900, Damien Le Moal wrote: >> >> On 8/9/17 12:57, Bart Van Assche wrote: >>> On Wed, 2017-08-09 at 11:47 +0900, Damien Le Moal wrote: >>>> On 8/9/17 11:19, Damien Le Moal wrote: >>>>> On 8/9/17 00:07, Bart Van Assche wrote: >>>>>> On Tue, 2017-08-08 at 13:17 +0900, Damien Le Moal wrote: >>>>>>> acquired the lock completes can cause deadlocks with scsi-mq due to >>>>>>> potential queue reordering if the lock owning request is requeued and >>>>>>> not executed. >>>>>> >>>>>> Please note that even with scsi-sq requeueing can cause request reordering. >>>>> >>>>> I am painfully aware of this fact... I will update the commit message to >>>>> reflect this. >>>> >>>> Correction: checking the code again, I just realized that the prep_fn >>>> queue function, which calls sd_init_cmnd() and causes a zone to be >>>> write-locked if the prepared command is a write, is called within >>>> blk_peek_request(). That function is called in scsi_request_fn() with >>>> the queue lock held. So for the legacy scsi path, this means that for a >>>> write command to a zone, the operation "lock zone and dequeue command" >>>> is atomic. Thanks to the zone lock, further dequeue of write commands >>>> for the same (locked) zone cannot happen and so there is no reordering >>>> possible for write commands directed at the same zone. Reordering for >>>> other commands is possible, but similarly to regular disks, this is not >>>> a problem with ZBC/ZAC drives. >>> >>> Hello Damien, >>> >>> The locking approach in the upstream code indeed prevents reordering of >>> write requests that apply to the same zone. However, this patch modifies >>> the locking approach. Are you sure that with this patch applied no request >>> reordering will occur if requests are requeued since this patch causes the >>> zone lock to be released before requeuing occurs? >> >> scsi_requeue_command() calls blk_unprep_request() and >> blk_requeue_request under the queue lock. So atomically. >> I think that suppresses any possibility of write requests to the same >> zone to be reordered. > > The queue lock is released after the request is requeued until the queue is > rerun. Another request can get executed before the requeued request gets > reexecuted. Or in other words, requeuing can change the order in which > requests are executed. Overall, yes, that is possible. However, considering only write commands to a single zone, since at any time only one command is dequeued (the one holding the zone lock), having the "lock+dequeue" and "unlock+requeue" both atomic prevent any reordering of write commands directed to that zone. The first command in the queue for the zone will either (1) get the zone lock and be dequeued OR (2) stay in the queue. In case (1), the next write for the zone will hit case (2) until the dispatch command completes. If the dispatch command is requeued (at the dispatch queue head), we hit back again the (1) or (2) cases, without any change in the order. Isnt't it ? -- Damien Le Moal, Western Digital
