| Message ID | 1519631932-1730-1-git-send-email-jianchao.w.wang@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, 2018-02-26 at 15:58 +0800, Jianchao Wang wrote: > In scsi core, __scsi_queue_insert should just put request back on > the queue and retry using the same command as before. However, for > blk-mq, scsi_mq_requeue_cmd is employed here which will unprepare > the request. To align with the semantics of __scsi_queue_insert, > just use blk_mq_requeue_request with kick_requeue_list == true. > > Cc: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> > --- > drivers/scsi/scsi_lib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index a86df9c..06d8110 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -191,7 +191,7 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy) > */ > cmd->result = 0; > if (q->mq_ops) { > - scsi_mq_requeue_cmd(cmd); > + blk_mq_requeue_request(cmd->request, true); > return; > } > spin_lock_irqsave(q->queue_lock, flags); I think this patch will break the code in the aacraid driver that iterates over sdev->cmd_list because commands are added to and removed from that list in the prep / unprep code. Bart.
Hi Bart Thanks for your kindly response. On 02/27/2018 11:08 AM, Bart Van Assche wrote: > On Mon, 2018-02-26 at 15:58 +0800, Jianchao Wang wrote: >> In scsi core, __scsi_queue_insert should just put request back on >> the queue and retry using the same command as before. However, for >> blk-mq, scsi_mq_requeue_cmd is employed here which will unprepare >> the request. To align with the semantics of __scsi_queue_insert, >> just use blk_mq_requeue_request with kick_requeue_list == true. >> >> Cc: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> >> --- >> drivers/scsi/scsi_lib.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index a86df9c..06d8110 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -191,7 +191,7 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy) >> */ >> cmd->result = 0; >> if (q->mq_ops) { >> - scsi_mq_requeue_cmd(cmd); >> + blk_mq_requeue_request(cmd->request, true); >> return; >> } >> spin_lock_irqsave(q->queue_lock, flags); > > I think this patch will break the code in the aacraid driver that iterates > over sdev->cmd_list because commands are added to and removed from that > list in the prep / unprep code. If that is true, what if aacraid driver uses block legacy instead of blk-mq ? w/ blk-mq disabled, __scsi_queue_insert just requeue the request with blk_requeue_request. __scsi_queue_insert ... if (q->mq_ops) { scsi_mq_requeue_cmd(cmd); return; } spin_lock_irqsave(q->queue_lock, flags); blk_requeue_request(q, cmd->request); kblockd_schedule_work(&device->requeue_work); spin_unlock_irqrestore(q->queue_lock, flags); ... no prep/unprep code there for block legacy code. Thanks Jianchao > > Bart. >
On Tue, 2018-02-27 at 11:28 +0800, jianchao.wang wrote: > If that is true, what if aacraid driver uses block legacy instead of blk-mq ? > w/ blk-mq disabled, __scsi_queue_insert just requeue the request with blk_requeue_request. > > __scsi_queue_insert > ... > if (q->mq_ops) { > scsi_mq_requeue_cmd(cmd); > return; > } > spin_lock_irqsave(q->queue_lock, flags); > blk_requeue_request(q, cmd->request); > kblockd_schedule_work(&device->requeue_work); > spin_unlock_irqrestore(q->queue_lock, flags); > ... > > no prep/unprep code there for block legacy code. Hello Jianchao, For the legacy block layer preparing and unpreparing a request happens from inside the block layer core. Please have a look at block/blk-core.c and the code in that file that handles the request flag RQF_DONTPREP. Thanks, Bart.
Hi Bart Thanks for your kindly response. On 02/27/2018 11:41 AM, Bart Van Assche wrote: > On Tue, 2018-02-27 at 11:28 +0800, jianchao.wang wrote: >> If that is true, what if aacraid driver uses block legacy instead of blk-mq ? >> w/ blk-mq disabled, __scsi_queue_insert just requeue the request with blk_requeue_request. >> >> __scsi_queue_insert >> ... >> if (q->mq_ops) { >> scsi_mq_requeue_cmd(cmd); >> return; >> } >> spin_lock_irqsave(q->queue_lock, flags); >> blk_requeue_request(q, cmd->request); >> kblockd_schedule_work(&device->requeue_work); >> spin_unlock_irqrestore(q->queue_lock, flags); >> ... >> >> no prep/unprep code there for block legacy code. > > Hello Jianchao, > > For the legacy block layer preparing and unpreparing a request happens from > inside the block layer core. Please have a look at block/blk-core.c and the > code in that file that handles the request flag RQF_DONTPREP. Yes, thanks for your directive. On the other hand, this patch is to align the actions between blk-mq and block legacy code in __scsi_queue_insert. As we know, __scsi_queue_insert is just to requeue the request back to queue, as the block legacy code segment above: for block legacy, it just blk_requeue_request for the request and kick the queue run. However, for the blk-mq, scsi_mq_requeue_cmd will be invoked, it not only requeue the request, but also unprep request. This is not what __scsi_queue_insert should do, but scsi_io_completion. When the request is not finished completely, and scsi_io_completion finds it need a ACTION_REPREP, at the moment, we need requeue and unprep there. If I missed something, please feel free to point out. :) Thanks Jianchao > > Thanks, > > Bart. > > >
On Tue, 2018-02-27 at 12:00 +0800, jianchao.wang wrote: > On the other hand, this patch is to align the actions between blk-mq and block > legacy code in __scsi_queue_insert. Hello Jianchao, Since the legacy SCSI core unpreps an reprepares a request when requeuing it I think your patch does not align the blk-mq and legacy block layer actions but instead makes the behavior of the two code paths different. Thanks, Bart.
Hi Bart Thanks for your kindly response. On 02/27/2018 01:12 PM, Bart Van Assche wrote: > On Tue, 2018-02-27 at 12:00 +0800, jianchao.wang wrote: >> On the other hand, this patch is to align the actions between blk-mq and block >> legacy code in __scsi_queue_insert. > > Hello Jianchao, > > Since the legacy SCSI core unpreps an reprepares a request when requeuing it I > think your patch does not align the blk-mq and legacy block layer actions but > instead makes the behavior of the two code paths different. > Can you share more details about this ? Thanks Jianchao
On Tue, 2018-02-27 at 13:15 +0800, jianchao.wang wrote: > Can you share more details about this ? After having had another look, I think your patch is fine. So if you want you can add: Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
On Tue, 2018-02-27 at 17:06 +0000, Bart Van Assche wrote: > On Tue, 2018-02-27 at 13:15 +0800, jianchao.wang wrote: > > Can you share more details about this ? > > After having had another look, I think your patch is fine. (replying to my own e-mail) What I think is fine in your patch is that it skips the unprep and reprep when requeueing. However, there is a put_device(&sdev->sdev_gendev) call in scsi_mq_requeue_cmd() and your patch causes that put_device() call to be skipped when requeueing. An explanation is needed in the commit message why you think that removing that put_device() call is fine. Bart.
Hi Bart Thanks for your kindly response and precious time to review this. On 02/28/2018 01:18 AM, Bart Van Assche wrote: > On Tue, 2018-02-27 at 17:06 +0000, Bart Van Assche wrote: >> On Tue, 2018-02-27 at 13:15 +0800, jianchao.wang wrote: >>> Can you share more details about this ? >> >> After having had another look, I think your patch is fine. > > (replying to my own e-mail) > > What I think is fine in your patch is that it skips the unprep and reprep > when requeueing. However, there is a put_device(&sdev->sdev_gendev) call > in scsi_mq_requeue_cmd() and your patch causes that put_device() call to > be skipped when requeueing. An explanation is needed in the commit message > why you think that removing that put_device() call is fine. Your concern is right. For the block legacy path in scsi core, the get_device(&sdev->sdev_gendev) is in prep. So when it requeue the request w/ RQF_DONTPREP, the reference will not be got again. However, for blk-mq patch in scsi core, the get_device(&sdev->sdev_gendev) in .get_budget, so put_device is still needed here. Thanks for your directive. Jianchao
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index a86df9c..06d8110 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -191,7 +191,7 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy) */ cmd->result = 0; if (q->mq_ops) { - scsi_mq_requeue_cmd(cmd); + blk_mq_requeue_request(cmd->request, true); return; } spin_lock_irqsave(q->queue_lock, flags);
In scsi core, __scsi_queue_insert should just put request back on the queue and retry using the same command as before. However, for blk-mq, scsi_mq_requeue_cmd is employed here which will unprepare the request. To align with the semantics of __scsi_queue_insert, just use blk_mq_requeue_request with kick_requeue_list == true. Cc: Christoph Hellwig <hch@lst.de> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> --- drivers/scsi/scsi_lib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)