Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[h...@lst.de]

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.

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Hannes Reinecke]

On 05/22/2017 02:48 PM, h...@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
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[h...@lst.de]

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.

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Hannes Reinecke]

On 05/22/2017 09:54 AM, h...@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
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[h...@lst.de]

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..

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Hannes Reinecke]

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
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Bart Van Assche]

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.

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig 

Although we really need to stop abusing requests/cmds for EH..
(Hannes, time to dust off your old patches!)