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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
