Re: Full hostlock pushdown available

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux