Re: [PATCH 12/13] scsi: scsi_dh_alua: Switch to use core support

[John Garry <john.g.garry@oracle.com>]

On 23/03/2026 01:47, Benjamin Marzinski wrote:
>>   static void alua_rtpg_work(struct work_struct *work)
>>   {
>>   	struct alua_dh_data *h =
>> @@ -670,56 +179,41 @@ static void alua_rtpg_work(struct work_struct *work)
>>   	int err = SCSI_DH_OK;
>>   	struct alua_queue_data *qdata, *tmp;
>>   	unsigned long flags;
>> +	int ret;
>>   
>>   	spin_lock_irqsave(&h->lock, flags);
>>   	h->flags |= ALUA_PG_RUNNING;
>>   	if (h->flags & ALUA_PG_RUN_RTPG) {
>> -		int state = h->state;
>>   
>>   		h->flags &= ~ALUA_PG_RUN_RTPG;
>>   		spin_unlock_irqrestore(&h->lock, flags);
>> -		if (state == SCSI_ACCESS_STATE_TRANSITIONING) {
>> -			if (alua_tur(sdev) == SCSI_DH_RETRY) {
>> -				spin_lock_irqsave(&h->lock, flags);
>> -				h->flags &= ~ALUA_PG_RUNNING;
>> -				h->flags |= ALUA_PG_RUN_RTPG;
>> -				if (!h->interval)
>> -					h->interval = ALUA_RTPG_RETRY_DELAY;
>> -				spin_unlock_irqrestore(&h->lock, flags);
>> -				queue_delayed_work(kaluad_wq, &h->rtpg_work,
>> -						   h->interval * HZ);
>> -				return;
>> -			}
>> -			/* Send RTPG on failure or if TUR indicates SUCCESS */
>> -		}
>> -		err = alua_rtpg(sdev);
>> -		spin_lock_irqsave(&h->lock, flags);
>> -
>> -		if (err == SCSI_DH_RETRY || h->flags & ALUA_PG_RUN_RTPG) {
>> +		ret = scsi_alua_rtpg_run(sdev);
>> +		if (ret == -EAGAIN) {
> This no longer handles the case where you want to trigger a new rtpg as
> soon as the running one finishes. I think it should be checking
> (ret == -EAGAIN || h->flags & ALUA_PG_RUN_RTPG)
> with a spinlock held.
> 

Yeah, this is all tricky to handle, as the code in scsi_dh_alua.c was 
handling the error codes with the state machine, and I want to move the 
error handling into the core driver.

As for your specific point, I think that ALUA_PG_RUN_RTPG can only now 
be set from outside this work handler, and that should also trigger the 
work (so the check on ALUA_PG_RUN_RTPG was not really required). But, I 
think that I can just have as before (with the h->flags & 
ALUA_PG_RUN_RTPG check)

>> +			spin_lock_irqsave(&h->lock, flags);
>>   			h->flags &= ~ALUA_PG_RUNNING;
>> -			if (err == SCSI_DH_IMM_RETRY)
>> -				h->interval = 0;
>> -			else if (!h->interval && !(h->flags & ALUA_PG_RUN_RTPG))
>> -				h->interval = ALUA_RTPG_RETRY_DELAY;
>>   			h->flags |= ALUA_PG_RUN_RTPG;
>>   			spin_unlock_irqrestore(&h->lock, flags);
>> -			goto queue_rtpg;
>> +			queue_delayed_work(kaluad_wq, &h->rtpg_work,
>> +							   sdev->alua->interval * HZ);
>> +			return;
>>   		}
>> -		if (err != SCSI_DH_OK)
>> -			h->flags &= ~ALUA_PG_RUN_STPG;
>> +		if (err != 0)
>> +				h->flags &= ~ALUA_PG_RUN_STPG;
>>   	}
>> +	spin_lock_irqsave(&h->lock, flags);
> If h->flags & ALUA_PG_RUN_RTPG is false above, h->lock will already be
> locked.
> 

Right, that is a bug

>>   	if (h->flags & ALUA_PG_RUN_STPG) {
>>   		h->flags &= ~ALUA_PG_RUN_STPG;
>>   		spin_unlock_irqrestore(&h->lock, flags);
>> -		err = alua_stpg(sdev);
>> -		spin_lock_irqsave(&h->lock, flags);
>> -		if (err == SCSI_DH_RETRY || h->flags & ALUA_PG_RUN_RTPG) {
>> +		ret = scsi_alua_stpg_run(sdev, h->flags & ALUA_OPTIMIZE_STPG);
>> +		if (err == -EAGAIN || h->flags & ALUA_PG_RUN_RTPG) {
> To avoid a race with resetting ALUA_PG_RUN_RTPG, this check needs to be
> done with the spinlock held.

Yes,

Thanks,
John