[05/17] target/core: Make sure that target_wait_for_sess_cmds() waits long enough
diff mbox series

Message ID 20180917213554.987-6-bvanassche@acm.org
State New, archived
Headers show
Series
  • Make ABORT and LUN RESET handling synchronous
Related show

Commit Message

Bart Van Assche Sept. 17, 2018, 9:35 p.m. UTC
A session must only be released after all code that accesses the session
structure has finished. Make sure that this is the case by introducing a
new command counter per session that is only decremented after the
.release_cmd() callback has finished.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_transport.c | 27 +++++++++++++++++---------
 include/target/target_core_base.h      |  1 +
 2 files changed, 19 insertions(+), 9 deletions(-)

Comments

Christoph Hellwig Oct. 6, 2018, 12:05 p.m. UTC | #1
On Mon, Sep 17, 2018 at 02:35:42PM -0700, Bart Van Assche wrote:
> A session must only be released after all code that accesses the session
> structure has finished. Make sure that this is the case by introducing a
> new command counter per session that is only decremented after the
> .release_cmd() callback has finished.

Can you explain what problems we are running into right now due to the
lack of a refcount?

> @@ -2897,6 +2907,8 @@ void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
>  	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>  	se_sess->sess_tearing_down = 1;
>  	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +
> +	percpu_ref_kill_and_confirm(&se_sess->cmd_count, NULL);

This is equivalent ot a plain percpu_ref_kill call.

> @@ -2911,17 +2923,14 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
>  
>  	WARN_ON_ONCE(!se_sess->sess_tearing_down);
>  
> -	spin_lock_irq(&se_sess->sess_cmd_lock);
>  	do {
> -		ret = wait_event_interruptible_lock_irq_timeout(
> -				se_sess->cmd_list_wq,
> -				list_empty(&se_sess->sess_cmd_list),
> -				se_sess->sess_cmd_lock, 180 * HZ);
> +		ret = wait_event_timeout(se_sess->cmd_list_wq,
> +				percpu_ref_is_zero(&se_sess->cmd_count),
> +				180 * HZ);

So this is basically the big change - check for a zero reference instead
of the list_empty.

I fail to see how this makes a difference, and also why we even need a
percpu ref as the get/pull calls are under, or right next to
sess_cmd_lock critical sections.
Nicholas A. Bellinger Oct. 7, 2018, 3:28 a.m. UTC | #2
On Mon, 2018-09-17 at 14:35 -0700, Bart Van Assche wrote:
> A session must only be released after all code that accesses the session
> structure has finished. Make sure that this is the case by introducing a
> new command counter per session that is only decremented after the
> .release_cmd() callback has finished.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/target/target_core_transport.c | 27 +++++++++++++++++---------
>  include/target/target_core_base.h      |  1 +
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 

Per HCH's earlier comment.  Is this required to fix a regression
introduced by commit 00d909a107 in mainline..?  Is it SRPT specific..?

Btw, I'm not completely against moving to a se_sess percpu_ref for
tracking se_cmd association to a session for the purposes of active I/O
shutdown.  However, inter-mixing it with existing se_sess->sess_cmd_lock
+ se_sess->sess_cmd_list for the single benefit of dropping
->sess_cmd_lock during non fast-path code in target_wait_for_sess_cmd()
doesn't really make sense.

That said, what would be better is utilize a new se_sess percpu_ref that
eliminates se_sess->sess_cmd_lock + se_sess->sess_cmd_list all-together!

How..?  Well, what about using the newly sbitmap converted (thanks
willy) se_sess->sess_tag_pool instead..?  Similar to how
blk_mq_queue_tag_busy_iter() uses sbitmap_for_each_set(), the same could
be done to eliminate se_sess->sess_cmd_list all together.

However, that would depend on all upstream fabric drivers using
se_sess->sess_tag_pool.  This is true in all but one case since 4.6.x,
the SRPT driver.

In addition, there would likely be some hairy TMR ABORT_TASK issues to
resolve no doubt, but killing se_sess->sess_cmd_[lock,list] and moving
to per se_session percpu_ref counting is a good goal.

Anyways, I'll review this patch as a starting point to remove
se_sess->sess_cmd_[lock,list] all-together.  Comments inline below.

> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 94e9d03af99d..54ccd8f56a57 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -224,6 +224,13 @@ void transport_subsystem_check_init(void)
>  	sub_api_initialized = 1;
>  }
>  
> +static void target_release_sess_cmd_refcnt(struct percpu_ref *ref)
> +{
> +	struct se_session *sess = container_of(ref, typeof(*sess), cmd_count);
> +
> +	wake_up(&sess->cmd_list_wq);
> +}
> +
>  /**
>   * transport_init_session - initialize a session object
>   * @se_sess: Session object pointer.
> @@ -237,7 +244,8 @@ int transport_init_session(struct se_session *se_sess)
>  	INIT_LIST_HEAD(&se_sess->sess_cmd_list);
>  	spin_lock_init(&se_sess->sess_cmd_lock);
>  	init_waitqueue_head(&se_sess->cmd_list_wq);
> -	return 0;
> +	return percpu_ref_init(&se_sess->cmd_count,
> +			       target_release_sess_cmd_refcnt, 0, GFP_KERNEL);
>  }
>  EXPORT_SYMBOL(transport_init_session);
>  
> @@ -587,6 +595,7 @@ void transport_free_session(struct se_session *se_sess)
>  		sbitmap_queue_free(&se_sess->sess_tag_pool);
>  		kvfree(se_sess->sess_cmd_map);
>  	}
> +	percpu_ref_exit(&se_sess->cmd_count);
>  	kmem_cache_free(se_sess_cache, se_sess);
>  }
>  EXPORT_SYMBOL(transport_free_session);
> @@ -2730,6 +2739,7 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref)
>  	}
>  	se_cmd->transport_state |= CMD_T_PRE_EXECUTE;
>  	list_add_tail(&se_cmd->se_cmd_list, &se_sess->sess_cmd_list);
> +	percpu_ref_get(&se_sess->cmd_count);
>  out:
>  	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
>  

This would need to be a percpu_ref_tryget_live() before
CMD_T_PRE_EXECUTE is assigned, replacing the sess->sess_tearing_down
check.

> @@ -2760,8 +2770,6 @@ static void target_release_cmd_kref(struct kref *kref)
>  	if (se_sess) {
>  		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>  		list_del_init(&se_cmd->se_cmd_list);
> -		if (list_empty(&se_sess->sess_cmd_list))
> -			wake_up(&se_sess->cmd_list_wq);
>  		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
>  	}
>  

Btw, I noticed this small regression as part of your commit 00d909a1 in
mainline, seperate from this patch.

That is, for a iodepth=1 workload the new wake_up() above in
target_release_cmd_kref() is getting called every se_cmd->cmd_kref
release, even during the normal case when a session is not actually
being shutdown..

To address this for <= v4.19 separate from further changes:

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 049a966..0473bf5 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2760,7 +2760,7 @@ static void target_release_cmd_kref(struct kref *kref)
        if (se_sess) {
                spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
                list_del_init(&se_cmd->se_cmd_list);
-               if (list_empty(&se_sess->sess_cmd_list))
+               if (se_sess->sess_tearing_down && list_empty(&se_sess->sess_cmd_list))
                        wake_up(&se_sess->cmd_list_wq);
                spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
        }

I'll send it out separately for MKP to pick up.

> @@ -2769,6 +2777,8 @@ static void target_release_cmd_kref(struct kref *kref)
>  	se_cmd->se_tfo->release_cmd(se_cmd);
>  	if (compl)
>  		complete(compl);
> +
> +	percpu_ref_put(&se_sess->cmd_count);
>  }
>  
>  /**
> @@ -2897,6 +2907,8 @@ void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
>  	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>  	se_sess->sess_tearing_down = 1;
>  	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +
> +	percpu_ref_kill_and_confirm(&se_sess->cmd_count, NULL);
>  }
>  EXPORT_SYMBOL(target_sess_cmd_list_set_waiting);
>  

Here is where percpu-ref and RCU grace-period magic comes in..

As a (future) consequence of relying solely upon se_sess->cmd_count, the
percpu_ref_kill_and_confirm() needs a percpu_ref->confirm_switch()
callback to work correctly, along with a matching local
wait_for_completion() within target_sess_cmd_list_set_waiting().

That is, the completion waits for percpu_ref_switch_to_atomic_rcu()
to call percpu_ref->confirm_switch() after a RCU grade-period has
elapsed so se_sess->cmd_count is seen as __PERCPU_REF_DEAD on all CPUs.

From there, se_sess->cmd_count is switched to atomic_t mode so that
percpu_ref_tryget_live() lookup of se_sess->cmd_count fails for all new
incoming I/O.

This is exactly how se_lun->lun_ref works today. See commit bd4e2d2907f
for more information.

> @@ -2911,17 +2923,14 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
>  
>  	WARN_ON_ONCE(!se_sess->sess_tearing_down);
>  
> -	spin_lock_irq(&se_sess->sess_cmd_lock);
>  	do {
> -		ret = wait_event_interruptible_lock_irq_timeout(
> -				se_sess->cmd_list_wq,
> -				list_empty(&se_sess->sess_cmd_list),
> -				se_sess->sess_cmd_lock, 180 * HZ);
> +		ret = wait_event_timeout(se_sess->cmd_list_wq,
> +				percpu_ref_is_zero(&se_sess->cmd_count),
> +				180 * HZ);
>  		list_for_each_entry(cmd, &se_sess->sess_cmd_list, se_cmd_list)
>  			target_show_cmd("session shutdown: still waiting for ",
>  					cmd);
>  	} while (ret <= 0);
> -	spin_unlock_irq(&se_sess->sess_cmd_lock);
>  }
>  EXPORT_SYMBOL(target_wait_for_sess_cmds);


It would be useful to move the hardcoded '180' into a tunable at some
point.
Bart Van Assche Oct. 8, 2018, 4:14 p.m. UTC | #3
On Sat, 2018-10-06 at 14:05 +0200, Christoph Hellwig wrote:
> On Mon, Sep 17, 2018 at 02:35:42PM -0700, Bart Van Assche wrote:
> > A session must only be released after all code that accesses the session
> > structure has finished. Make sure that this is the case by introducing a
> > new command counter per session that is only decremented after the
> > .release_cmd() callback has finished.
> 
> Can you explain what problems we are running into right now due to the
> lack of a refcount?

I will explain that further down in this e-mail.

> > @@ -2897,6 +2907,8 @@ void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
> >  	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> >  	se_sess->sess_tearing_down = 1;
> >  	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > +
> > +	percpu_ref_kill_and_confirm(&se_sess->cmd_count, NULL);
> 
> This is equivalent ot a plain percpu_ref_kill call.

OK, I will change this into percpu_ref_kill().

> > @@ -2911,17 +2923,14 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
> >  
> >  	WARN_ON_ONCE(!se_sess->sess_tearing_down);
> >  
> > -	spin_lock_irq(&se_sess->sess_cmd_lock);
> >  	do {
> > -		ret = wait_event_interruptible_lock_irq_timeout(
> > -				se_sess->cmd_list_wq,
> > -				list_empty(&se_sess->sess_cmd_list),
> > -				se_sess->sess_cmd_lock, 180 * HZ);
> > +		ret = wait_event_timeout(se_sess->cmd_list_wq,
> > +				percpu_ref_is_zero(&se_sess->cmd_count),
> > +				180 * HZ);
> 
> So this is basically the big change - check for a zero reference instead
> of the list_empty.
> 
> I fail to see how this makes a difference, and also why we even need a
> percpu ref as the get/pull calls are under, or right next to
> sess_cmd_lock critical sections.

Hi Christoph,

After having called target_wait_for_sess_cmds(), several target drivers call
target_remove_session(). That last function frees the session object
synchronously. In other words, it is not safe to use the session pointer after
target_wait_for_sess_cmds() has returned. That means that
target_wait_for_sess_cmds() must wait until all code that can dereference the
session pointer has finished, including target_release_cmd_kref(). That
function executes the following code after having removed a command from the
command list:

	target_free_cmd_mem(se_cmd);
	se_cmd->se_tfo->release_cmd(se_cmd);
	if (compl)
		complete(compl);

Hence this patch that makes target_wait_for_sess_cmds() wait until
target_release_cmd_kref() has called .release_cmd(). Since I applied this patch
I have not hit the following crash anymore:

BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c/0x130
Read of size 4 at addr ffff8801534b16e4 by task rmdir/14805
CPU: 16 PID: 14805 Comm: rmdir Not tainted 4.18.0-rc2-dbg+ #5
Call Trace:
dump_stack+0xa4/0xf5
print_address_description+0x6f/0x270
kasan_report+0x241/0x360
__asan_load4+0x78/0x80
do_raw_spin_lock+0x1c/0x130
_raw_spin_lock_irqsave+0x52/0x60
srpt_set_ch_state+0x27/0x70 [ib_srpt]
srpt_disconnect_ch+0x1b/0xc0 [ib_srpt]
srpt_close_session+0xa8/0x260 [ib_srpt]
target_shutdown_sessions+0x170/0x180 [target_core_mod]
core_tpg_del_initiator_node_acl+0xf3/0x200 [target_core_mod]
target_fabric_nacl_base_release+0x25/0x30 [target_core_mod]
config_item_release+0x9c/0x110 [configfs]
config_item_put+0x26/0x30 [configfs]
configfs_rmdir+0x3b8/0x510 [configfs]
vfs_rmdir+0xb3/0x1e0
do_rmdir+0x262/0x2c0
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe

Please let me know if you need more information.

Bart.
Bart Van Assche Oct. 8, 2018, 4:32 p.m. UTC | #4
On Sat, 2018-10-06 at 20:28 -0700, Nicholas A. Bellinger wrote:
> On Mon, 2018-09-17 at 14:35 -0700, Bart Van Assche wrote:
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 94e9d03af99d..54ccd8f56a57 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -224,6 +224,13 @@ void transport_subsystem_check_init(void)
> >  	sub_api_initialized = 1;
> >  }
> >  
> > +static void target_release_sess_cmd_refcnt(struct percpu_ref *ref)
> > +{
> > +	struct se_session *sess = container_of(ref, typeof(*sess), cmd_count);
> > +
> > +	wake_up(&sess->cmd_list_wq);
> > +}
> > +
> >  /**
> >   * transport_init_session - initialize a session object
> >   * @se_sess: Session object pointer.
> > @@ -237,7 +244,8 @@ int transport_init_session(struct se_session *se_sess)
> >  	INIT_LIST_HEAD(&se_sess->sess_cmd_list);
> >  	spin_lock_init(&se_sess->sess_cmd_lock);
> >  	init_waitqueue_head(&se_sess->cmd_list_wq);
> > -	return 0;
> > +	return percpu_ref_init(&se_sess->cmd_count,
> > +			       target_release_sess_cmd_refcnt, 0, GFP_KERNEL);
> >  }
> >  EXPORT_SYMBOL(transport_init_session);
> >  
> > @@ -587,6 +595,7 @@ void transport_free_session(struct se_session *se_sess)
> >  		sbitmap_queue_free(&se_sess->sess_tag_pool);
> >  		kvfree(se_sess->sess_cmd_map);
> >  	}
> > +	percpu_ref_exit(&se_sess->cmd_count);
> >  	kmem_cache_free(se_sess_cache, se_sess);
> >  }
> >  EXPORT_SYMBOL(transport_free_session);
> > @@ -2730,6 +2739,7 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref)
> >  	}
> >  	se_cmd->transport_state |= CMD_T_PRE_EXECUTE;
> >  	list_add_tail(&se_cmd->se_cmd_list, &se_sess->sess_cmd_list);
> > +	percpu_ref_get(&se_sess->cmd_count);
> >  out:
> >  	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> >  
> 
> This would need to be a percpu_ref_tryget_live() before
> CMD_T_PRE_EXECUTE is assigned, replacing the sess->sess_tearing_down
> check.

I think the current code is fine since the percpu_ref_get() call happens
with the sess_cmd_lock held and after the sess_tearing_down flag has been
checked. The spinlock is needed anyway to protect the list_add_tail() call.
Let's keep the discussion about dropping se_cmd_list and switching to
sbitmap instead for later because this patch series is already complicated
enough and also because I'm not convinced that that switching to an sbitmap
would be an improvement.

> > @@ -2769,6 +2777,8 @@ static void target_release_cmd_kref(struct kref *kref)
> >  	se_cmd->se_tfo->release_cmd(se_cmd);
> >  	if (compl)
> >  		complete(compl);
> > +
> > +	percpu_ref_put(&se_sess->cmd_count);
> >  }
> >  
> >  /**
> > @@ -2897,6 +2907,8 @@ void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
> >  	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> >  	se_sess->sess_tearing_down = 1;
> >  	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > +
> > +	percpu_ref_kill_and_confirm(&se_sess->cmd_count, NULL);
> >  }
> >  EXPORT_SYMBOL(target_sess_cmd_list_set_waiting);
> >  
> 
> Here is where percpu-ref and RCU grace-period magic comes in..
> 
> As a (future) consequence of relying solely upon se_sess->cmd_count, the
> percpu_ref_kill_and_confirm() needs a percpu_ref->confirm_switch()
> callback to work correctly, along with a matching local
> wait_for_completion() within target_sess_cmd_list_set_waiting().

I do not agree. A session is only freed after target_wait_for_sess_cmds()
has returned so it is sufficient if that function waits until the switch
to atomic mode of the percpu-refcount has finished. I don't see why
target_sess_cmd_list_set_waiting() should wait until the switch to atomic
mode has finished.

Bart.

Patch
diff mbox series

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 94e9d03af99d..54ccd8f56a57 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -224,6 +224,13 @@  void transport_subsystem_check_init(void)
 	sub_api_initialized = 1;
 }
 
+static void target_release_sess_cmd_refcnt(struct percpu_ref *ref)
+{
+	struct se_session *sess = container_of(ref, typeof(*sess), cmd_count);
+
+	wake_up(&sess->cmd_list_wq);
+}
+
 /**
  * transport_init_session - initialize a session object
  * @se_sess: Session object pointer.
@@ -237,7 +244,8 @@  int transport_init_session(struct se_session *se_sess)
 	INIT_LIST_HEAD(&se_sess->sess_cmd_list);
 	spin_lock_init(&se_sess->sess_cmd_lock);
 	init_waitqueue_head(&se_sess->cmd_list_wq);
-	return 0;
+	return percpu_ref_init(&se_sess->cmd_count,
+			       target_release_sess_cmd_refcnt, 0, GFP_KERNEL);
 }
 EXPORT_SYMBOL(transport_init_session);
 
@@ -587,6 +595,7 @@  void transport_free_session(struct se_session *se_sess)
 		sbitmap_queue_free(&se_sess->sess_tag_pool);
 		kvfree(se_sess->sess_cmd_map);
 	}
+	percpu_ref_exit(&se_sess->cmd_count);
 	kmem_cache_free(se_sess_cache, se_sess);
 }
 EXPORT_SYMBOL(transport_free_session);
@@ -2730,6 +2739,7 @@  int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref)
 	}
 	se_cmd->transport_state |= CMD_T_PRE_EXECUTE;
 	list_add_tail(&se_cmd->se_cmd_list, &se_sess->sess_cmd_list);
+	percpu_ref_get(&se_sess->cmd_count);
 out:
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 
@@ -2760,8 +2770,6 @@  static void target_release_cmd_kref(struct kref *kref)
 	if (se_sess) {
 		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 		list_del_init(&se_cmd->se_cmd_list);
-		if (list_empty(&se_sess->sess_cmd_list))
-			wake_up(&se_sess->cmd_list_wq);
 		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 	}
 
@@ -2769,6 +2777,8 @@  static void target_release_cmd_kref(struct kref *kref)
 	se_cmd->se_tfo->release_cmd(se_cmd);
 	if (compl)
 		complete(compl);
+
+	percpu_ref_put(&se_sess->cmd_count);
 }
 
 /**
@@ -2897,6 +2907,8 @@  void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
 	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	se_sess->sess_tearing_down = 1;
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+
+	percpu_ref_kill_and_confirm(&se_sess->cmd_count, NULL);
 }
 EXPORT_SYMBOL(target_sess_cmd_list_set_waiting);
 
@@ -2911,17 +2923,14 @@  void target_wait_for_sess_cmds(struct se_session *se_sess)
 
 	WARN_ON_ONCE(!se_sess->sess_tearing_down);
 
-	spin_lock_irq(&se_sess->sess_cmd_lock);
 	do {
-		ret = wait_event_interruptible_lock_irq_timeout(
-				se_sess->cmd_list_wq,
-				list_empty(&se_sess->sess_cmd_list),
-				se_sess->sess_cmd_lock, 180 * HZ);
+		ret = wait_event_timeout(se_sess->cmd_list_wq,
+				percpu_ref_is_zero(&se_sess->cmd_count),
+				180 * HZ);
 		list_for_each_entry(cmd, &se_sess->sess_cmd_list, se_cmd_list)
 			target_show_cmd("session shutdown: still waiting for ",
 					cmd);
 	} while (ret <= 0);
-	spin_unlock_irq(&se_sess->sess_cmd_lock);
 }
 EXPORT_SYMBOL(target_wait_for_sess_cmds);
 
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 7a4ee7852ca4..2cfd3b4573b0 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -602,6 +602,7 @@  struct se_session {
 	struct se_node_acl	*se_node_acl;
 	struct se_portal_group *se_tpg;
 	void			*fabric_sess_ptr;
+	struct percpu_ref	cmd_count;
 	struct list_head	sess_list;
 	struct list_head	sess_acl_list;
 	struct list_head	sess_cmd_list;