[PATCH v2 08/30] cxlflash: Fix to avoid CXL services during EEH

[Matthew R. Ochs]

> On Sep 18, 2015, at 8:37 AM, Brian King <brking at linux.vnet.ibm.com> wrote:
> On 09/16/2015 04:27 PM, Matthew R. Ochs wrote:
>> 
>> /**
>> + * drain_ioctls() - wait until all currently executing ioctls have completed
>> + * @cfg:	Internal structure associated with the host.
>> + *
>> + * Obtain write access to read/write semaphore that wraps ioctl
>> + * handling to 'drain' ioctls currently executing.
>> + */
>> +static void drain_ioctls(struct cxlflash_cfg *cfg)
>> +{
>> +	down_write(&cfg->ioctl_rwsem);
>> +	up_write(&cfg->ioctl_rwsem);
>> +}
>> +
>> +/**
>>  * cxlflash_pci_error_detected() - called when a PCI error is detected
>>  * @pdev:	PCI device struct.
>>  * @state:	PCI channel state.
>> @@ -2383,16 +2397,14 @@ static pci_ers_result_t cxlflash_pci_error_detected(struct pci_dev *pdev,
>> 	switch (state) {
>> 	case pci_channel_io_frozen:
>> 		cfg->state = STATE_LIMBO;
>> -
>> -		/* Turn off legacy I/O */
>> 		scsi_block_requests(cfg->host);
>> +		drain_ioctls(cfg);
> 
> So, what kicks any outstanding ioctls back? Let's assume you are in the middle of disk_attach
> and you've sent the READ_CAP16 to the device. It appears as if what would happen here is we'd
> sit here in cxlflash_pci_error_detected. Eventually, the READ_CAP16 would timeout. This would
> wake the SCSI error handler, and end up calling your eh_device_reset handler, which would see that
> we are in STATE_LIMBO, where it would then do a wait_event, waiting for us to get out of STATE_LIMBO,
> and we would end up in a deadlock.
> 
> Rather than implementing a rw semaphore, would it be better to simply make the ioctls check the
> state we are in and either wait to get out of EEH state or fail themselves?

We do have the ioctls check the state and wait for EEH to complete
or fail completely in the event that the device is terminating (see
ioctl_common()). The drain exists to create a wait point for ioctls that
have already passed the state check and are active. The CXL services
cannot be called during the recovery window (maybe this requirement
will go away in a future release?), thus the reason for this 'drain'.

To handle it I considered 3 options:

 - add state check wraps to all CXL service calls
 - create a "running ioctls" count that could be evaluated
 - wrap the ioctl in read semaphore and obtain write access to 'wait'

I started with the first option and it quickly made the code very nasty. I then
began implementing the second option and as I was writing the code to wrap
the ioctl with increment/decrement statements, the third option entered my
mind and seemed like a much cleaner solution. Therefore I went with that
approach and did not look back.

With regard to your example, you bring up a good point and we'll need to
do something about that. One thought that comes to mind would be for us
to drop the semaphore before making this type of call (I believe there are
only 2 places like this), reacquiring it when we return, and then checking
the state to make sure we're not in a reset situation.

> 
>> 		rc = cxlflash_mark_contexts_error(cfg);
>> 		if (unlikely(rc))
>> 			dev_err(dev, "%s: Failed to mark user contexts!(%d)\n",
>> 				__func__, rc);
>> 		term_mc(cfg, UNDO_START);
>> 		stop_afu(cfg);
>> -
>> 		return PCI_ERS_RESULT_NEED_RESET;
>> 	case pci_channel_io_perm_failure:
>> 		cfg->state = STATE_FAILTERM;
>> diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
>> index cf2a85d..8a18230 100644
>> --- a/drivers/scsi/cxlflash/superpipe.c
>> +++ b/drivers/scsi/cxlflash/superpipe.c
>> @@ -1214,6 +1214,48 @@ static const struct file_operations null_fops = {
>> };
>> 
>> /**
>> + * check_state() - checks and responds to the current adapter state
>> + * @cfg:	Internal structure associated with the host.
>> + * @ioctl:	Indicates if on an ioctl thread.
>> + *
>> + * This routine can block and should only be used on process context.
>> + * When blocking on an ioctl thread, the ioctl read semaphore should be
>> + * let up to allow for draining actively running ioctls. Also note that
>> + * when waking up from waiting in reset, the state is unknown and must
>> + * be checked again before proceeding.
>> + *
>> + * Return: 0 on success, -errno on failure
>> + */
>> +static int check_state(struct cxlflash_cfg *cfg, bool ioctl)
> 
> All your callers appear to set the second parameter to true, so why bother having it?

That's a good point. I originally had a case where there was a need for this
but have since removed it. I can fix that in v3.