On Mon, May 04, 2020 at 12:24:41PM +0200, Hannes Reinecke wrote: > On 5/4/20 10:47 AM, Ming Lei wrote: > > 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? > > > Okok, I see your point. > > Indeed the csiostor driver doesn't use the 'tag' per se for submitting > commands; it's just using the scsi command pointer as a tag to figure out if > a completion has been send from the hw. > > I'll be giving it a bit more thought, and will be dropping it for the next > round (which will contain only the minimal changes to get the > 'reserved_cmds' interface in). IMO, the 'reserved_cmds' interface is only needed in case that RESET command is transported from IO channel. Any HBA which has dedicated channel for sending RESET doesn't need this interface. Thanks, Ming
