Re: Full hostlock pushdown available

[Jeff Garzik <jeff@garzik.org>]

On 11/01/2010 01:57 PM, Andi Kleen wrote:
> @@ -3186,11 +3186,14 @@ int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done
>         struct ata_device *dev;
>         struct scsi_device *scsidev = cmd->device;
>         struct Scsi_Host *shost = scsidev->host;
> +       unsigned long irqflags;
>         int rc = 0;
>
>         ap = ata_shost_to_port(shost);
>
> -       spin_unlock(shost->host_lock);
> +       spin_lock_irqsave(shost->host_lock, irqflags);
> +       scsi_cmd_get_serial(shost, cmd);
> +
>         spin_lock(ap->lock);
>
>         ata_scsi_dump_cdb(ap, cmd);
> @@ -3205,6 +3208,7 @@ int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(
>
>         spin_unlock(ap->lock);
>         spin_lock(shost->host_lock);
> +       spin_unlock_irqrestore(shost->host_lock, irqflags);
>         return rc;
>  }
>

It's a bit disappointing that libata's lock profile in this patch is 
quite different than that of current upstream: with your patch, libata 
holds the scsi host lock for a considerably longer period of time, while 
also holding the ATA port/host spinlock.

IOW, it's doing the exact opposite of what the previous code did 
(release the scsi host lock, before acquiring the ATA port/host 
spinlock), not at all an equivalent transformation.

The following sequence would seem to better preserve the existing lock 
profile, correct?

	local_irq_save(flags)

	spin_lock(shost->host_lock)
	scsi_cmd_get_serial()
	spin_unlock(shost->host_lock)

	spin_lock(ap->lock)
	...
	spin_unlock(ap->lock)

	local_irq_restore(flags)

Regards,

	Jeff