Re: Full hostlock pushdown available

Jeff Garzik
2010-11-01T18:59:14Z

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