[PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

transport_lookup_cmd_lun() may be called from interrupt context (eg
tcm_loop_allocate_core_cmd() calls it, and it has a comment that says,
"Can be called from interrupt context"), so it needs to use
spin_lock_irqsave() instead of spin_lock_irq() to avoid enabling
interrupts at the wrong time.

(And indeed the last set of lock operations, on lun_cmd_lock, were
already using spin_lock_irqsave(), so we just need to fix the other
two locks we take)

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
Not sure if this is causing anything other than lockdep reports at
the moment, so I don't think it's worth trying to get into 3.0.0-rc...

 drivers/target/target_core_device.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index fcb8161..ca0ba71 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -72,7 +72,7 @@ int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
 		return -ENODEV;
 	}
 
-	spin_lock_irq(&se_sess->se_node_acl->device_list_lock);
+	spin_lock_irqsave(&se_sess->se_node_acl->device_list_lock, flags);
 	se_cmd->se_deve = &se_sess->se_node_acl->device_list[unpacked_lun];
 
 	if (se_cmd->se_deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS) {
@@ -89,7 +89,7 @@ int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
 			       " Access for 0x%08x\n",
 			       se_cmd->se_tfo->get_fabric_name(),
 			       unpacked_lun);
-			spin_unlock_irq(&se_sess->se_node_acl->device_list_lock);
+			spin_unlock_irqrestore(&se_sess->se_node_acl->device_list_lock, flags);
 			return -EACCES;
 		}
 
@@ -107,7 +107,7 @@ int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
 		se_cmd->se_orig_obj_ptr = se_cmd->se_lun->lun_se_dev;
 		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 	}
-	spin_unlock_irq(&se_sess->se_node_acl->device_list_lock);
+	spin_unlock_irqrestore(&se_sess->se_node_acl->device_list_lock, flags);
 
 	if (!se_lun) {
 		/*
@@ -156,13 +156,13 @@ int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
 
 	/* TODO: get rid of this and use atomics for stats */
 	dev = se_lun->lun_se_dev;
-	spin_lock_irq(&dev->stats_lock);
+	spin_lock_irqsave(&dev->stats_lock, flags);
 	dev->num_cmds++;
 	if (se_cmd->data_direction == DMA_TO_DEVICE)
 		dev->write_bytes += se_cmd->data_length;
 	else if (se_cmd->data_direction == DMA_FROM_DEVICE)
 		dev->read_bytes += se_cmd->data_length;
-	spin_unlock_irq(&dev->stats_lock);
+	spin_unlock_irqrestore(&dev->stats_lock, flags);
 
 	/*
 	 * Add the iscsi_cmd_t to the struct se_lun's cmd list.  This list is used

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Christoph Hellwig]

On Mon, Jun 13, 2011 at 08:44:05PM -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> transport_lookup_cmd_lun() may be called from interrupt context (eg
> tcm_loop_allocate_core_cmd() calls it, and it has a comment that says,
> "Can be called from interrupt context"), so it needs to use
> spin_lock_irqsave() instead of spin_lock_irq() to avoid enabling
> interrupts at the wrong time.
> 
> (And indeed the last set of lock operations, on lun_cmd_lock, were
> already using spin_lock_irqsave(), so we just need to fix the other
> two locks we take)

It currently is, but I think we'd better be off stopping to call it from
IRQ context.  Soon after we offload the work to thread context anyway.
By moving the point of offloading a little earlier we can simply the
calling conventions a lot, and keep the amount of code that needs
IRQ-safe locking minimal.

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Roland Dreier]

On Tue, Jun 14, 2011 at 1:12 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> transport_lookup_cmd_lun() may be called from interrupt context (eg
>> tcm_loop_allocate_core_cmd() calls it, and it has a comment that says,
>> "Can be called from interrupt context"), so it needs to use
>> spin_lock_irqsave() instead of spin_lock_irq() to avoid enabling
>> interrupts at the wrong time.

> It currently is, but I think we'd better be off stopping to call it from
> IRQ context.  Soon after we offload the work to thread context anyway.
> By moving the point of offloading a little earlier we can simply the
> calling conventions a lot, and keep the amount of code that needs
> IRQ-safe locking minimal.

Fair enough... however that seems like a much more major project,
as eg the qla2xxx code does a fair bit from interrupt context.

Also it seems even if we are running from thread context it would
be legitimate for a target driver to do

    spin_lock_irq(&internal_target_lock);
    ...
    transport_lookup_cmd_lun();
    ...
    spin_unlock_irq

and so transport_lookup_cmd_lun() shouldn't be reenabling IRQs
unconditionally.

Thanks,
  Roland
--
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

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Nicholas A. Bellinger]

On Tue, 2011-06-14 at 04:12 -0400, Christoph Hellwig wrote:
> On Mon, Jun 13, 2011 at 08:44:05PM -0700, Roland Dreier wrote:
> > From: Roland Dreier <roland@purestorage.com>
> > 
> > transport_lookup_cmd_lun() may be called from interrupt context (eg
> > tcm_loop_allocate_core_cmd() calls it, and it has a comment that says,
> > "Can be called from interrupt context"), so it needs to use
> > spin_lock_irqsave() instead of spin_lock_irq() to avoid enabling
> > interrupts at the wrong time.
> > 
> > (And indeed the last set of lock operations, on lun_cmd_lock, were
> > already using spin_lock_irqsave(), so we just need to fix the other
> > two locks we take)
> 
> It currently is, but I think we'd better be off stopping to call it from
> IRQ context.  Soon after we offload the work to thread context anyway.
> By moving the point of offloading a little earlier we can simply the
> calling conventions a lot, and keep the amount of code that needs
> IRQ-safe locking minimal.
> 

The thing here is that the new descriptor can't be offloaded into thread
context for TFO->new_cmd_map() processing until the se_cmd->se_dev
assignment has occured, because we don't know which backend it's
destined for.

So, I think we at least need this patch..

--nab

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Nicholas A. Bellinger]

On Mon, 2011-06-13 at 20:44 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> transport_lookup_cmd_lun() may be called from interrupt context (eg
> tcm_loop_allocate_core_cmd() calls it, and it has a comment that says,
> "Can be called from interrupt context"), so it needs to use
> spin_lock_irqsave() instead of spin_lock_irq() to avoid enabling
> interrupts at the wrong time.
> 
> (And indeed the last set of lock operations, on lun_cmd_lock, were
> already using spin_lock_irqsave(), so we just need to fix the other
> two locks we take)
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
> Not sure if this is causing anything other than lockdep reports at
> the moment, so I don't think it's worth trying to get into 3.0.0-rc...
> 

<nod>  Committed as f39cc7c3da62.

Btw, I think the same type of conversion may need to happen for
transport_lookup_tmr_lun() as well, as I believe qla_target.c can call
this directly from interrupt context in certain situations.

--nab

>  drivers/target/target_core_device.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index fcb8161..ca0ba71 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -72,7 +72,7 @@ int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
>  		return -ENODEV;
>  	}
>  
> -	spin_lock_irq(&se_sess->se_node_acl->device_list_lock);
> +	spin_lock_irqsave(&se_sess->se_node_acl->device_list_lock, flags);
>  	se_cmd->se_deve = &se_sess->se_node_acl->device_list[unpacked_lun];
>  
>  	if (se_cmd->se_deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS) {
> @@ -89,7 +89,7 @@ int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
>  			       " Access for 0x%08x\n",
>  			       se_cmd->se_tfo->get_fabric_name(),
>  			       unpacked_lun);
> -			spin_unlock_irq(&se_sess->se_node_acl->device_list_lock);
> +			spin_unlock_irqrestore(&se_sess->se_node_acl->device_list_lock, flags);
>  			return -EACCES;
>  		}
>  
> @@ -107,7 +107,7 @@ int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
>  		se_cmd->se_orig_obj_ptr = se_cmd->se_lun->lun_se_dev;
>  		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
>  	}
> -	spin_unlock_irq(&se_sess->se_node_acl->device_list_lock);
> +	spin_unlock_irqrestore(&se_sess->se_node_acl->device_list_lock, flags);
>  
>  	if (!se_lun) {
>  		/*
> @@ -156,13 +156,13 @@ int transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
>  
>  	/* TODO: get rid of this and use atomics for stats */
>  	dev = se_lun->lun_se_dev;
> -	spin_lock_irq(&dev->stats_lock);
> +	spin_lock_irqsave(&dev->stats_lock, flags);
>  	dev->num_cmds++;
>  	if (se_cmd->data_direction == DMA_TO_DEVICE)
>  		dev->write_bytes += se_cmd->data_length;
>  	else if (se_cmd->data_direction == DMA_FROM_DEVICE)
>  		dev->read_bytes += se_cmd->data_length;
> -	spin_unlock_irq(&dev->stats_lock);
> +	spin_unlock_irqrestore(&dev->stats_lock, flags);
>  
>  	/*
>  	 * Add the iscsi_cmd_t to the struct se_lun's cmd list.  This list is used
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Roland Dreier]

On Wed, Jun 15, 2011 at 10:06 AM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> Btw, I think the same type of conversion may need to happen for
> transport_lookup_tmr_lun() as well, as I believe qla_target.c can call
> this directly from interrupt context in certain situations.

Yes, I think I sent another mail about this... the reason I didn't just
send a patch is that transport_lookup_tmr_lun() ends with:

        spin_lock(&se_tmr->tmr_dev->se_tmr_lock);
        list_add_tail(&se_tmr->tmr_list, &se_tmr->tmr_dev->dev_tmr_list);
        spin_unlock(&se_tmr->tmr_dev->se_tmr_lock);

and indeed se_tmr_lock looks like it is taken with bare spin_lock()
in lots of places.

Presumably se_tmr_lock is taken from process context sometimes?
So we would need to convert all those spin_lock()s to spin_lock_irq()
(or irqsave I guess).

 - R.

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Nicholas A. Bellinger]

On Wed, 2011-06-15 at 10:22 -0700, Roland Dreier wrote:
> On Wed, Jun 15, 2011 at 10:06 AM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > Btw, I think the same type of conversion may need to happen for
> > transport_lookup_tmr_lun() as well, as I believe qla_target.c can call
> > this directly from interrupt context in certain situations.
> 
> Yes, I think I sent another mail about this... the reason I didn't just
> send a patch is that transport_lookup_tmr_lun() ends with:
> 
>         spin_lock(&se_tmr->tmr_dev->se_tmr_lock);
>         list_add_tail(&se_tmr->tmr_list, &se_tmr->tmr_dev->dev_tmr_list);
>         spin_unlock(&se_tmr->tmr_dev->se_tmr_lock);
> 
> and indeed se_tmr_lock looks like it is taken with bare spin_lock()
> in lots of places.
> 

Actually, it's only two places:  core_tmr_release_req() and
core_tmr_lun_reset()

> Presumably se_tmr_lock is taken from process context sometimes?
> So we would need to convert all those spin_lock()s to spin_lock_irq()
> (or irqsave I guess).
> 

Correct, both of the above are only every called from process context,
so a simple conversion to spin_lock_irq() for these two, and
spin_lock_irqsave() in transport_lookup_tmr_lun() should by sufficent..

--nab

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Kiran Patil]

On 6/15/2011 10:25 AM, Nicholas A. Bellinger wrote:
> On Wed, 2011-06-15 at 10:22 -0700, Roland Dreier wrote:
>> On Wed, Jun 15, 2011 at 10:06 AM, Nicholas A. Bellinger
>> <nab@linux-iscsi.org>  wrote:
>>> Btw, I think the same type of conversion may need to happen for
>>> transport_lookup_tmr_lun() as well, as I believe qla_target.c can call
>>> this directly from interrupt context in certain situations.
>> Yes, I think I sent another mail about this... the reason I didn't just
>> send a patch is that transport_lookup_tmr_lun() ends with:
>>
>>          spin_lock(&se_tmr->tmr_dev->se_tmr_lock);
>>          list_add_tail(&se_tmr->tmr_list,&se_tmr->tmr_dev->dev_tmr_list);
>>          spin_unlock(&se_tmr->tmr_dev->se_tmr_lock);
>>
>> and indeed se_tmr_lock looks like it is taken with bare spin_lock()
>> in lots of places.
>>
> Actually, it's only two places:  core_tmr_release_req() and
> core_tmr_lun_reset()
>
>> Presumably se_tmr_lock is taken from process context sometimes?
>> So we would need to convert all those spin_lock()s to spin_lock_irq()
>> (or irqsave I guess).
>>
> Correct, both of the above are only every called from process context,
> so a simple conversion to spin_lock_irq() for these two, and
> spin_lock_irqsave() in transport_lookup_tmr_lun() should by sufficent..
Likewise, we need to do same change in function 
"transport_get_lun_for_tmr", specifically for tmr_lock. Please correct 
me if I am missing anything.
> --nab
>
> --
> 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
Thanks,
-- Kiran P.

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Roland Dreier]

On Wed, Jun 15, 2011 at 10:25 AM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> Actually, it's only two places:  core_tmr_release_req() and
> core_tmr_lun_reset()
>
>> Presumably se_tmr_lock is taken from process context sometimes?
>> So we would need to convert all those spin_lock()s to spin_lock_irq()
>> (or irqsave I guess).
>
> Correct, both of the above are only every called from process context,
> so a simple conversion to spin_lock_irq() for these two, and
> spin_lock_irqsave() in transport_lookup_tmr_lun() should by sufficent..

Looking at this...

core_tmr_lun_reset() uses _irqsave locking for cmd->t_state_lock...
is that not needed, or can it actually be called from a context where
we can't just use spin_lock_irq?  (eg because some caller might have
already disabled interrupts)

 - R.
--
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

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Roland Dreier]

On Wed, Jun 15, 2011 at 10:52 AM, Kiran Patil <kiran.patil@intel.com> wrote:
> Likewise, we need to do same change in function "transport_get_lun_for_tmr",
> specifically for tmr_lock. Please correct me if I am missing anything.

Yes, that's right... in fact that is what we were discussing now, isn't it?
Or is there something different I'm missing?

(BTW transport_get_lun_for_tmr is renamed to transport_lookup_tmr_lun
in the latest tree)

 - R.

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Kiran Patil]

On 6/15/2011 12:41 PM, Roland Dreier wrote:
> On Wed, Jun 15, 2011 at 10:52 AM, Kiran Patil<kiran.patil@intel.com>  wrote:
>> Likewise, we need to do same change in function "transport_get_lun_for_tmr",
>> specifically for tmr_lock. Please correct me if I am missing anything.
> Yes, that's right... in fact that is what we were discussing now, isn't it?
> Or is there something different I'm missing?
>
> (BTW transport_get_lun_for_tmr is renamed to transport_lookup_tmr_lun
> in the latest tree)
>
>   - R.

Cool. Then we are referring to same function. Looks like I am using old 
tree which has function name "transport_get_lun_for_tmr"

Thanks,
-- Kiran P,

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Christoph Hellwig]

On Tue, Jun 14, 2011 at 08:58:57AM -0700, Roland Dreier wrote:
> Fair enough... however that seems like a much more major project,
> as eg the qla2xxx code does a fair bit from interrupt context.

Yes, just pointing out in which direction it should go eventually.

> 
> Also it seems even if we are running from thread context it would
> be legitimate for a target driver to do
> 
>     spin_lock_irq(&internal_target_lock);
>     ...
>     transport_lookup_cmd_lun();
>     ...
>     spin_unlock_irq
> 
> and so transport_lookup_cmd_lun() shouldn't be reenabling IRQs
> unconditionally.

Yes.  At that point we shouldn't require any irq locking, making life
even easier.

Re: [PATCH] target: Make transport_lookup_cmd_lun() locking IRQ-safe

[Christoph Hellwig]

On Wed, Jun 15, 2011 at 09:58:41AM -0700, Nicholas A. Bellinger wrote:
> > It currently is, but I think we'd better be off stopping to call it from
> > IRQ context.  Soon after we offload the work to thread context anyway.
> > By moving the point of offloading a little earlier we can simply the
> > calling conventions a lot, and keep the amount of code that needs
> > IRQ-safe locking minimal.
> > 
> 
> The thing here is that the new descriptor can't be offloaded into thread
> context for TFO->new_cmd_map() processing until the se_cmd->se_dev
> assignment has occured, because we don't know which backend it's
> destined for.

The actual backend is pretty irrelevant.  What matters with the current
code is which se_dev it is for, but once switched to the concurrency
managed workqueues that becomes irrelevant, too.