lkml.org 
[lkml]   [2010]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 01/12] libiscsi: Convert to host_lock less w/ interrupts disabled internally
    On Sun, Dec 19, 2010 at 01:21:56PM -0800, Nicholas A. Bellinger wrote:
    > This patch changes iscsi_queuecommand_lck() to a host_lock less
    > iscsi_queuecommand() that will internally disable interrupts using
    > session->lock and drop the now legacy host_lock unlock.

    I think this patch is buggy. Before, iscsi_queuecommand_lck is called with
    interrupts disabled. Now, it's called with interrupts disabled. Elsewhere,
    session->lock is acquired with bh's disabled:

    void iscsi_put_task(struct iscsi_task *task)
    {
    struct iscsi_session *session = task->conn->session;

    spin_lock_bh(&session->lock);

    So I think you need to convert the
    spin_lock(&session->lock);
    in iscsi_queuecommand_lck to at least a spin_lock_bh, and possibly
    a spin_lock_irq -- it's not clear to me whether it needs to exclude
    against interrupt context, or only BH context. In some places, it's
    taken with spin_lock_irqsave().

    I'd play it safe and use _irq, but someone more confident with this code
    might choose to only use _bh.

    > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
    > ---
    > drivers/scsi/libiscsi.c | 10 ++--------
    > 1 files changed, 2 insertions(+), 8 deletions(-)
    >
    > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
    > index c15fde8..5535782 100644
    > --- a/drivers/scsi/libiscsi.c
    > +++ b/drivers/scsi/libiscsi.c
    > @@ -1599,7 +1599,7 @@ enum {
    > FAILURE_SESSION_NOT_READY,
    > };
    >
    > -static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
    > +int iscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
    > {
    > struct iscsi_cls_session *cls_session;
    > struct Scsi_Host *host;
    > @@ -1609,13 +1609,11 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
    > struct iscsi_conn *conn;
    > struct iscsi_task *task = NULL;
    >
    > - sc->scsi_done = done;
    > sc->result = 0;
    > sc->SCp.ptr = NULL;
    >
    > host = sc->device->host;
    > ihost = shost_priv(host);
    > - spin_unlock(host->host_lock);
    >
    > cls_session = starget_to_session(scsi_target(sc->device));
    > session = cls_session->dd_data;
    > @@ -1706,7 +1704,6 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
    >
    > session->queued_cmdsn++;
    > spin_unlock(&session->lock);
    > - spin_lock(host->host_lock);
    > return 0;
    >
    > prepd_reject:
    > @@ -1716,7 +1713,6 @@ reject:
    > spin_unlock(&session->lock);
    > ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
    > sc->cmnd[0], reason);
    > - spin_lock(host->host_lock);
    > return SCSI_MLQUEUE_TARGET_BUSY;
    >
    > prepd_fault:
    > @@ -1732,12 +1728,10 @@ fault:
    > scsi_out(sc)->resid = scsi_out(sc)->length;
    > scsi_in(sc)->resid = scsi_in(sc)->length;
    > }
    > - done(sc);
    > - spin_lock(host->host_lock);
    > + sc->scsi_done(sc);
    > return 0;
    > }
    >
    > -DEF_SCSI_QCMD(iscsi_queuecommand)
    > EXPORT_SYMBOL_GPL(iscsi_queuecommand);
    >
    > int iscsi_change_queue_depth(struct scsi_device *sdev, int depth, int reason)
    > --
    > 1.7.3.4
    >
    > --
    > 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

    --
    Matthew Wilcox Intel Open Source Technology Centre
    "Bill, look, we understand that you're interested in selling us this
    operating system, but compare it to ours. We can't possibly take such
    a retrograde step."


    \
     
     \ /
      Last update: 2010-12-20 00:41    [W:0.027 / U:0.788 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site