Re: [PATCH 3/4] scsi: improved eh timeout handler

["Jörn Engel" <joern@logfs.org>]

On Thu, 6 June 2013 11:43:54 +0200, Hannes Reinecke wrote:
> 
> +	spin_lock_irqsave(&sdev->list_lock, flags);
> +	list_for_each_entry_safe(scmd, tmp, &sdev->eh_abort_list, eh_entry) {
> +		list_del_init(&scmd->eh_entry);
> +		spin_unlock_irqrestore(&sdev->list_lock, flags);

I never liked the list_for_each_entry_safe() loop but until this
morning couldn't really say why.  The problem is that it can finish
with commands left on the list.  And since your kick_worker checks for
list_empty, there wouldn't be any more aborts.

Thread1		list state		Thread2
		head->1->2->head
spin_lock_irqsave();
scmd = 1; tmp = 2;
list_del_init(scmd);
spin_unlock_irqrestore();
		head->2->head
					spin_lock_irqsave();
					kick_worker = 0;
					list_add();
					spin_unlock_irqrestore();
		head->3->2->head	
spin_lock_irqsave();
scmd = 2; tmp = head;
list_del_init(scmd);
spin_unlock_irqrestore();
		head->3->head

And now that I think about nasty races, you could also do
schedule_work() when the list is empty, but the worker thread is still
running.  I've had to debug similar cases and the approach is to waste
a day or five, despair and eventually get lucky half a year later when
you happen to notice the race.  Rare random crashes in the kworker
code a not my favorite sight.

> +void
> +scsi_abort_command(struct scsi_cmnd *scmd)
> +{
> +	unsigned long flags;
> +	int kick_worker = 0;
> +	struct scsi_device *sdev = scmd->device;
> +
> +	spin_lock_irqsave(&sdev->list_lock, flags);
> +	if (list_empty(&sdev->eh_abort_list))
> +		kick_worker = 1;
> +	list_add(&scmd->eh_entry, &sdev->eh_abort_list);
> +	SCSI_LOG_ERROR_RECOVERY(3,
> +		scmd_printk(KERN_INFO, scmd, "adding to eh_abort_list\n"));
> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +	if (kick_worker)
> +		schedule_work(&sdev->abort_work);
> +}
> +EXPORT_SYMBOL_GPL(scsi_abort_command);

Jörn

--
Time? What's that? Time is only worth what you do with it.
-- Theo de Raadt
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html