On Mon, May 04, 2020 at 08:55:05AM +0200, Hannes Reinecke wrote: > On 5/2/20 4:29 PM, Ming Lei wrote: > > On Sat, May 02, 2020 at 10:49:32AM +0200, Hannes Reinecke wrote: > > > On 5/1/20 7:45 PM, Christoph Hellwig wrote: > > > > On Fri, May 01, 2020 at 11:01:29PM +0800, Ming Lei wrote: > > > > > > We cannot increase MAX_QUEUE arbitrarily as this is a compile time variable, > > > > > > which seems to relate to a hardware setting. > > > > > > > > > > > > But I can see to update the reserved command functionality for allowing to > > > > > > fetch commands from the normal I/O tag pool; in the case of LUN reset it > > > > > > shouldn't make much of a difference as the all I/O is quiesced anyway. > > > > > > > > > > It isn't related with reset. > > > > > > > > > > This patch reduces active IO queue depth by 1 anytime no matter there is reset > > > > > or not, and this way may cause performance regression. > > > > > > > > But isn't it the right thing to do? How else do we guarantee that > > > > there always is a tag available for the LU reset? > > > > > > > Precisely. One could argue that this is an issue with the current driver, > > > too; if all tags have timed-out there is no way how we can send a LUN reset > > > even now. Hence we need to reserve a tag for us to reliably send a LUN > > > reset. > > > And this was precisely the problem what sparked off this entire patchset; > > > some drivers require a valid tag to send internal, non SCSI commands to the > > > hardware. > > > > Could you explain a bit how you conclude that csio_scsi reset hander has to > > use one unique tag? At least we don't allocate request from block layer for > > ioctl(SG_SCSI_RESET), see scsi_ioctl_reset(). Also this patch doesn't > > use the reserved rq->tag too. > > > > > And with the current design it requires some really ugly hacks to make this > > > to work. > > > > You also don't explain how csio_eh_lun_reset_handler() is broken and where > > the ugly hack is in csio scsi too, and how this patch fixes the issue, could > > you document the exact reason in the commit log? > > > The problem is the ioctl path. > When issuing TMF commands from the ioctl path we currently do not have a > valid SCSI command to pass to the various SCSI EH functions. > This requires the SCSI LLDDs to check for every EH function whether the > passed in SCSI command is valid (ie coming from SCSI EH), or a made up one > coming from the ioctl path. Could you point out where the check is in csio driver? > And having to code various ways to work around this issue. > > With this patchset I'm implementing a standardized way how these functions > can be coded. By using a reserved command the EH functions and the driver > internal command handling will always have a valid command, so the > workarounds can be removed. It depends if every HBA really requires unique tag for sending reset command. If some HBAs don't require unique tag for resetting device or host, the reserved tag is wasted. thanks, Ming
