| Message ID | 20170519183016.12646-7-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Although we really need to stop abusing requests/cmds for EH..
(Hannes, time to dust off your old patches!)
On Sun, 2017-05-21 at 08:50 +0200, Christoph Hellwig wrote: > Although we really need to stop abusing requests/cmds for EH.. > (Hannes, time to dust off your old patches!) Hello Christoph and Hannes, How about passing a struct scsi_device pointer to the eh_*_reset_handler callbacks instead of a struct scsi_cmnd pointer? Most SCSI LLD eh_*_reset_handler implementations don't do anything with the information passed through the struct scsi_cmnd pointer except reading the SCSI device pointer and logging the SCSI command CDB. Hannes, is this the same as your proposal? Do you want to work on this or do you perhaps expect me to prepare patches to implement that change? Bart.
On 05/21/2017 06:41 PM, Bart Van Assche wrote: > On Sun, 2017-05-21 at 08:50 +0200, Christoph Hellwig wrote: >> Although we really need to stop abusing requests/cmds for EH.. >> (Hannes, time to dust off your old patches!) > > Hello Christoph and Hannes, > > How about passing a struct scsi_device pointer to the eh_*_reset_handler > callbacks instead of a struct scsi_cmnd pointer? Most SCSI LLD > eh_*_reset_handler implementations don't do anything with the information > passed through the struct scsi_cmnd pointer except reading the SCSI device > pointer and logging the SCSI command CDB. Hannes, is this the same as your > proposal? Do you want to work on this or do you perhaps expect me to prepare > patches to implement that change? > If it were so easy. Problem is that quite some LLDDs require a hardware tag for sending TMFs, and currently the re-use the tag from the passed in scmd for that. So first we need to move those to a sane interface, and having them requesting a new tag for TMFs. Should be made easier with Christophs rework, but we still don't have any defined way how TMFs should request their tag; should they use private tags? Should they use 'normal' tags? Should the driver implement their own tag pool, seeing that these commands will never ever making use of the associated request? Hence I'm looking into implementing a REQ_RESET block request operation, which then could be used to facilitate all of this (the request would be allocated from the private tag pool if present). It would also neatly solve the scsi_ioctl_reset() problem, as we then could just issue a REQ_RESET and would avoid having to call into the eh_* function directly. Cheers, Hannes
On Mon, May 22, 2017 at 08:06:46AM +0200, Hannes Reinecke wrote: > Problem is that quite some LLDDs require a hardware tag for sending > TMFs, and currently the re-use the tag from the passed in scmd for that. > So first we need to move those to a sane interface, and having them > requesting a new tag for TMFs. Some do indeed. But these seems to be the exception and not the rule. > Should be made easier with Christophs rework, but we still don't have > any defined way how TMFs should request their tag; should they use > private tags? Should they use 'normal' tags? Should the driver implement > their own tag pool, seeing that these commands will never ever making > use of the associated request? I think the right way to go is that every driver that needs tags for TMFs should use private tags internally without the core knowing about it. > Hence I'm looking into implementing a REQ_RESET block request operation, > which then could be used to facilitate all of this (the request would be > allocated from the private tag pool if present). That seems to be overkill to me for the few drivers. And I suspect most of them would be better off now even using blk-mq private tags (which we'd have to implement for the legacy path first or just kill it off) but just not expose a tag per host to the scsi and block layers and set that aside. > It would also neatly solve the scsi_ioctl_reset() problem, as we then > could just issue a REQ_RESET and would avoid having to call into the > eh_* function directly. I don't think calling the eh_* methods is a problem per see. It's just their stupid calling conventions..
On 05/22/2017 09:54 AM, hch@lst.de wrote: > On Mon, May 22, 2017 at 08:06:46AM +0200, Hannes Reinecke wrote: >> Problem is that quite some LLDDs require a hardware tag for sending >> TMFs, and currently the re-use the tag from the passed in scmd for that. >> So first we need to move those to a sane interface, and having them >> requesting a new tag for TMFs. > > Some do indeed. But these seems to be the exception and not the rule. > Still need to be handled, though ... >> Should be made easier with Christophs rework, but we still don't have >> any defined way how TMFs should request their tag; should they use >> private tags? Should they use 'normal' tags? Should the driver implement >> their own tag pool, seeing that these commands will never ever making >> use of the associated request? > > I think the right way to go is that every driver that needs tags for > TMFs should use private tags internally without the core knowing > about it. > That was my idea, too. >> Hence I'm looking into implementing a REQ_RESET block request operation, >> which then could be used to facilitate all of this (the request would be >> allocated from the private tag pool if present). > > That seems to be overkill to me for the few drivers. And I suspect > most of them would be better off now even using blk-mq private tags > (which we'd have to implement for the legacy path first or just > kill it off) but just not expose a tag per host to the scsi and block > layers and set that aside. > IE not using blk-mq private tags for EH? Hmm. But then we'd need a SCSI-internal mechanism to get one of them. I really would want to avoid having each driver to implement it's own mechanism on how to get a TMF tag; that sort of thing only leads to copy-and-paste errors. Ok; will be looking into it. >> It would also neatly solve the scsi_ioctl_reset() problem, as we then >> could just issue a REQ_RESET and would avoid having to call into the >> eh_* function directly. > > I don't think calling the eh_* methods is a problem per see. It's just > their stupid calling conventions.. > I know. I've been working on a patchset to move at least eh_host_reset() to take the scsi_host as argument. But even that requires some preliminary patches to some LLDDs :-( Right. Dusting off those patches, then. Cheers, Hannes
On Mon, May 22, 2017 at 10:46:10AM +0200, Hannes Reinecke wrote: > > That seems to be overkill to me for the few drivers. And I suspect > > most of them would be better off now even using blk-mq private tags > > (which we'd have to implement for the legacy path first or just > > kill it off) but just not expose a tag per host to the scsi and block > > layers and set that aside. > > > IE not using blk-mq private tags for EH? Hmm. > But then we'd need a SCSI-internal mechanism to get one of them. I > really would want to avoid having each driver to implement it's own > mechanism on how to get a TMF tag; that sort of thing only leads to > copy-and-paste errors. > Ok; will be looking into it. No, we don't. The driver simply sets a tag aside and doesn't expose it to the block layer. Similar to what smartpqi already does for LUN resets and AENs, mpt3sas does for the ioctl tags and NVMe does for AERs.
On 05/22/2017 02:48 PM, hch@lst.de wrote: > On Mon, May 22, 2017 at 10:46:10AM +0200, Hannes Reinecke wrote: >>> That seems to be overkill to me for the few drivers. And I suspect >>> most of them would be better off now even using blk-mq private tags >>> (which we'd have to implement for the legacy path first or just >>> kill it off) but just not expose a tag per host to the scsi and block >>> layers and set that aside. >>> >> IE not using blk-mq private tags for EH? Hmm. >> But then we'd need a SCSI-internal mechanism to get one of them. I >> really would want to avoid having each driver to implement it's own >> mechanism on how to get a TMF tag; that sort of thing only leads to >> copy-and-paste errors. >> Ok; will be looking into it. > > No, we don't. The driver simply sets a tag aside and doesn't expose > it to the block layer. Similar to what smartpqi already does for LUN > resets and AENs, mpt3sas does for the ioctl tags and NVMe does for AERs. > Personally I feel a bit uncomfortable by setting aside just one tag for TMFs; this assumes we'll never be sending LUN resets to devices in parallel. But maybe this is a discussion for another time, if and when we finally move to that. Cheers, Hannes
On Mon, May 22, 2017 at 02:56:18PM +0200, Hannes Reinecke wrote: > > No, we don't. The driver simply sets a tag aside and doesn't expose > > it to the block layer. Similar to what smartpqi already does for LUN > > resets and AENs, mpt3sas does for the ioctl tags and NVMe does for AERs. > > Personally I feel a bit uncomfortable by setting aside just one tag for > TMFs; this assumes we'll never be sending LUN resets to devices in parallel. > > But maybe this is a discussion for another time, if and when we finally > move to that. For midlayer initiated EH that's the case - they are serialized by being issued from the eh thread. For the ioctl we have the scsi_block_when_processing_errors / scsi_host_in_recovery magic, although that hand crafted primitive is a bit of a mess, we'd be much better off with a real EH mutex.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index ce654e35b060..23d6f225c671 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -2286,7 +2286,12 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg) shost->hostt->cmd_size, GFP_KERNEL); if (!rq) goto out_put_autopm_host; - blk_rq_init(NULL, rq); + /* + * Although blk_rq_init() is intended for single queue block + * drivers, this code path even uses blk_rq_init() when @dev is + * a scsi-mq device. + */ + blk_rq_init(dev->request_queue, rq); scmd = (struct scsi_cmnd *)(rq + 1); scsi_init_command(dev, scmd);
A later patch will add a call to a request initialization function into blk_rq_init(). Hence make sure that all blk_rq_init() calls specify the request queue pointer. Since TMF callback functions in SCSI LLD drivers do not use request.q, this patch does not change the behavior of any SCSI driver. Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Hannes Reinecke <hare@suse.com> --- drivers/scsi/scsi_error.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)