Re: [PATCH/RFC] libata-dev: update timer for PIO polling - revised

[Tejun Heo <htejun@gmail.com>]

Albert Lee wrote:
> Jeff,
> 
> Revised per your advice to use the eh_timed_out() handler.
> 
> Description:
>   In the bug reported by Ralf Müller:
> (http://marc.theaimsgroup.com/?l=linux-ide&m=113629906809278&w=2)
> 
> When the scsi command times out, the qc is completed by the time
> out handler. This might cause null pointer dereference if
> the qc is running in the PIO polling thread.
> 
> Changes:
>   Add ata_scsi_timed_out() to libata-scsi.
>   If the qc times out when running in PIO mode, EH_RESET_TIMER is returned.
>   Otherwise, EH_NOT_HANDLED is returned and the eh_strategy_handler()
>   is triggered as before.
> 
> (Will add ata_scsi_timed_out() to LLDs later if this is ok.)
> 
> Patch against the libata-dev irq-pio branch 
> (000080c3499cd5037e60c08a8053efb9e48aa9c0).
> 
> For your review, thanks.
> 
> Albert
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> =====
> 
> --- irq-pio/include/linux/libata.h	2006-01-20 10:35:11.000000000 +0800
> +++ eh_timed_out/include/linux/libata.h	2006-01-20 10:48:29.000000000 +0800
> @@ -449,6 +449,7 @@ extern void ata_host_set_remove(struct a
>  extern int ata_scsi_detect(struct scsi_host_template *sht);
>  extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
>  extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
> +extern enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *);
>  extern int ata_scsi_error(struct Scsi_Host *host);
>  extern int ata_scsi_release(struct Scsi_Host *host);
>  extern unsigned int ata_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
> --- irq-pio/drivers/scsi/libata-scsi.c	2006-01-20 10:35:05.000000000 +0800
> +++ eh_timed_out/drivers/scsi/libata-scsi.c	2006-01-20 22:13:14.000000000 +0800
> @@ -717,6 +717,59 @@ int ata_scsi_slave_config(struct scsi_de
>  }
>  
>  /**
> + *	ata_scsi_timed_out - SCSI layer time out handler callback
> + *	@cmd: SCSI cmd on which time out occurred
> + *
> + *	Handles SCSI-layer-thrown time out events.
> + *
> + *	RETURNS:
> + *	EH_HANDLED if qc was completed
> + *	EH_RESET_TIMER for polling case
> + *	EH_NOT_HANDLED otherwise.
> + */
> +enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd)
> +{
> +	struct scsi_device *scsidev = cmd->device;
> +	struct ata_port *ap = (struct ata_port *) &scsidev->host->hostdata[0];
> +	struct ata_host_set *host_set = ap->host_set;
> +	struct ata_queued_cmd *qc;
> +	unsigned long flags;
> +	int rc = EH_NOT_HANDLED;
> +
> +	DPRINTK("ENTER\n");
> +	spin_lock_irqsave(&host_set->lock, flags);
> +
> +	qc = ata_qc_from_tag(ap, ap->active_tag);
> +	if (!qc) {
> +		/* ata_poll_qc_complete() or ata_interrupt() win the race.
> +		 */
> +		printk(KERN_ERR "ata%u: timeout without qc\n", ap->id);
> +		rc = EH_HANDLED;
> +		goto out;
> +	}
> +
> +	/* check if we are handling the right qc */
> +	assert(qc->scsicmd == cmd);
> +	assert(qc->flags & ATA_QCFLAG_ACTIVE);
> +
> +	if (qc->tf.flags & ATA_TFLAG_POLLING) {
> +		/* The PIO polling thread has its own timeout handling.
> +		 * It won't loop forever, even if it takes a long time.
> +		 * We can wait the PIO polling thread to finish its work.
> +		 * No need to stop the polling thread from the timeout handler.
> +		 * Just reset the timer here.
> +		 */
> +		printk(KERN_WARNING "ata%u: reset scsi timer for PIO polling\n", ap->id);
> +		cmd->retries = 0;
> +		rc = EH_RESET_TIMER;
> +	}
> +out:
> +	spin_unlock_irqrestore(&host_set->lock, flags);
> +	DPRINTK("EXIT rc %d\n", rc);
> +	return rc;
> +}
> +
> +/**
>   *	ata_scsi_error - SCSI layer error handler callback
>   *	@host: SCSI host on which error occurred
>   *
> --- irq-pio/drivers/scsi/libata-core.c	2006-01-20 10:35:05.000000000 +0800
> +++ eh_timed_out/drivers/scsi/libata-core.c	2006-01-20 10:48:44.000000000 +0800
> @@ -5434,6 +5434,7 @@ EXPORT_SYMBOL_GPL(ata_port_disable);
>  EXPORT_SYMBOL_GPL(ata_ratelimit);
>  EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
>  EXPORT_SYMBOL_GPL(ata_scsi_queuecmd);
> +EXPORT_SYMBOL_GPL(ata_scsi_timed_out);
>  EXPORT_SYMBOL_GPL(ata_scsi_error);
>  EXPORT_SYMBOL_GPL(ata_scsi_slave_config);
>  EXPORT_SYMBOL_GPL(ata_scsi_release);
> 

Albert, Jeff, can you please hold off this for a while?  I currently 
have a lot of pending EH changes (reset, revalidation, reconfiguration, 
etc...).  I'm in the process of splitting the changes and throwing out 
unnecessary ones.

One of the changes is ata_flush_pio_task() which waits for the pio tasks 
to finish.  This function is called by error handler.  This achieves 
about the same thing, but in sligthly different manner.

* Doesn't use eh_timed_out.  I think this callback interface is somewhat 
weird and it's also slightly broken in that commands can be completed 
before timer is re-added.  It's not fatal but the command will have to 
wait until the next timeout.

* ata_flush_pio_task() uses a flag to tell pio tasks to exit early.  All 
pio tasks have to do is check whether a flag is set before rescheduling 
itself.

Sorry about asking to hold off after a lot is done.  I've been lazy with 
mailing list for a week or so and am catching up now.

-- 
tejun