[PATCH 0/6] target: Update UA handling

[PATCH 0/6] target: Update UA handling

[Hannes Reinecke]

Hi Nic,

lio-target is very minimalistic when it comes to generate UAs;
primarily they are generated for persistent reservations, but
generic changes tend to be ignored.

This patchset updates the UA handling and generates UA for internal
state changes (REPORTED LUNS DATA CHANGED, INQUIRY DATA CHANGED,
and LUN RESET OCCURRED).

Funnily enough this triggers some issues with the SCSI stack;
I'll be sending out patches for that, too.

Hannes Reinecke (6):
  target_core_alua: Correct UA handling when switching states
  target: Remove 'ua_nacl' pointer from se_ua structure
  target: use 'se_dev_entry' when allocating UAs
  target: Send UA on ALUA target port group change
  target: Send UA upon LUN RESET tmr completion
  target: Send UA when changing LUN inventory

 drivers/target/target_core_alua.c      | 56 +++++++++++++++++++++++++---------
 drivers/target/target_core_device.c    | 26 +++++++++++++++-
 drivers/target/target_core_pr.c        | 31 +++++++++++++++----
 drivers/target/target_core_transport.c | 29 ++++++++++++++----
 drivers/target/target_core_ua.c        | 24 ++-------------
 drivers/target/target_core_ua.h        |  5 ++-
 include/target/target_core_base.h      |  1 -
 7 files changed, 121 insertions(+), 51 deletions(-)

-- 
1.8.5.2

[PATCH 1/6] target_core_alua: Correct UA handling when switching states

[Hannes Reinecke]

When switching target port group ALUA states we need to send
one UA when setting the ALUA state to 'transitioning', and
another one once the final state has been set.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_alua.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 2318e6e..228a3c7 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -941,16 +941,11 @@ static int core_alua_update_tpg_primary_metadata(
 	return rc;
 }
 
-static void core_alua_do_transition_tg_pt_work(struct work_struct *work)
+static void core_alua_queue_state_change_ua(struct t10_alua_tg_pt_gp *tg_pt_gp)
 {
-	struct t10_alua_tg_pt_gp *tg_pt_gp = container_of(work,
-		struct t10_alua_tg_pt_gp, tg_pt_gp_transition_work.work);
-	struct se_device *dev = tg_pt_gp->tg_pt_gp_dev;
 	struct se_dev_entry *se_deve;
 	struct se_lun *lun;
 	struct se_lun_acl *lacl;
-	bool explicit = (tg_pt_gp->tg_pt_gp_alua_access_status ==
-			 ALUA_STATUS_ALTERED_BY_EXPLICIT_STPG);
 
 	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
 	list_for_each_entry(lun, &tg_pt_gp->tg_pt_gp_lun_list,
@@ -1002,6 +997,16 @@ static void core_alua_do_transition_tg_pt_work(struct work_struct *work)
 		percpu_ref_put(&lun->lun_ref);
 	}
 	spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
+}
+
+static void core_alua_do_transition_tg_pt_work(struct work_struct *work)
+{
+	struct t10_alua_tg_pt_gp *tg_pt_gp = container_of(work,
+		struct t10_alua_tg_pt_gp, tg_pt_gp_transition_work.work);
+	struct se_device *dev = tg_pt_gp->tg_pt_gp_dev;
+	bool explicit = (tg_pt_gp->tg_pt_gp_alua_access_status ==
+			 ALUA_STATUS_ALTERED_BY_EXPLICIT_STPG);
+
 	/*
 	 * Update the ALUA metadata buf that has been allocated in
 	 * core_alua_do_port_transition(), this metadata will be written
@@ -1031,6 +1036,9 @@ static void core_alua_do_transition_tg_pt_work(struct work_struct *work)
 		tg_pt_gp->tg_pt_gp_id,
 		core_alua_dump_state(tg_pt_gp->tg_pt_gp_alua_previous_state),
 		core_alua_dump_state(tg_pt_gp->tg_pt_gp_alua_pending_state));
+
+	core_alua_queue_state_change_ua(tg_pt_gp);
+
 	spin_lock(&dev->t10_alua.tg_pt_gps_lock);
 	atomic_dec(&tg_pt_gp->tg_pt_gp_ref_cnt);
 	spin_unlock(&dev->t10_alua.tg_pt_gps_lock);
@@ -1083,6 +1091,8 @@ static int core_alua_do_transition_tg_pt(
 				ALUA_STATUS_ALTERED_BY_EXPLICIT_STPG :
 				ALUA_STATUS_ALTERED_BY_IMPLICIT_ALUA;
 
+	core_alua_queue_state_change_ua(tg_pt_gp);
+
 	/*
 	 * Check for the optional ALUA primary state transition delay
 	 */
-- 
1.8.5.2

[PATCH 2/6] target: Remove 'ua_nacl' pointer from se_ua structure

[Hannes Reinecke]

Unused.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_ua.c   | 1 -
 include/target/target_core_base.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index e53d4ee..e506224 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -107,7 +107,6 @@ int core_scsi3_ua_allocate(
 	}
 	INIT_LIST_HEAD(&ua->ua_nacl_list);
 
-	ua->ua_nacl = nacl;
 	ua->ua_asc = asc;
 	ua->ua_ascq = ascq;
 
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index d30271e..4055cbd 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -549,7 +549,6 @@ struct se_cmd {
 struct se_ua {
 	u8			ua_asc;
 	u8			ua_ascq;
-	struct se_node_acl	*ua_nacl;
 	struct list_head	ua_nacl_list;
 };
 
-- 
1.8.5.2

[PATCH 3/6] target: use 'se_dev_entry' when allocating UAs

[Hannes Reinecke]

We need to use 'se_dev_entry' as argument when allocating
UAs, otherwise we'll never see any UAs for an implicit
ALUA state transition triggered from userspace.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_alua.c      | 27 ++++++++++++++++++---------
 drivers/target/target_core_pr.c        | 31 +++++++++++++++++++++++++------
 drivers/target/target_core_transport.c | 18 ++++++++++++------
 drivers/target/target_core_ua.c        | 23 +++--------------------
 drivers/target/target_core_ua.h        |  2 +-
 5 files changed, 59 insertions(+), 42 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 228a3c7..aa2e4b1 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -972,23 +972,32 @@ static void core_alua_queue_state_change_ua(struct t10_alua_tg_pt_gp *tg_pt_gp)
 		list_for_each_entry(se_deve, &lun->lun_deve_list, lun_link) {
 			lacl = rcu_dereference_check(se_deve->se_lun_acl,
 					lockdep_is_held(&lun->lun_deve_lock));
+
 			/*
-			 * se_deve->se_lun_acl pointer may be NULL for a
-			 * entry created without explicit Node+MappedLUN ACLs
+			 * spc4r37 p.242:
+			 * After an explicit target port asymmetric access
+			 * state change, a device server shall establish a
+			 * unit attention condition with the additional sense
+			 * code set to ASYMMETRIC ACCESS STATE CHANGED for
+			 * the initiator port associated with every I_T nexus
+			 * other than the I_T nexus on which the SET TARGET
+			 * PORT GROUPS command was received.
 			 */
-			if (!lacl)
-				continue;
-
 			if ((tg_pt_gp->tg_pt_gp_alua_access_status ==
 			     ALUA_STATUS_ALTERED_BY_EXPLICIT_STPG) &&
-			   (tg_pt_gp->tg_pt_gp_alua_nacl != NULL) &&
-			    (tg_pt_gp->tg_pt_gp_alua_nacl == lacl->se_lun_nacl) &&
 			   (tg_pt_gp->tg_pt_gp_alua_lun != NULL) &&
 			    (tg_pt_gp->tg_pt_gp_alua_lun == lun))
 				continue;
 
-			core_scsi3_ua_allocate(lacl->se_lun_nacl,
-				se_deve->mapped_lun, 0x2A,
+			/*
+			 * se_deve->se_lun_acl pointer may be NULL for a
+			 * entry created without explicit Node+MappedLUN ACLs
+			 */
+			if (lacl && (tg_pt_gp->tg_pt_gp_alua_nacl != NULL) &&
+			    (tg_pt_gp->tg_pt_gp_alua_nacl == lacl->se_lun_nacl))
+				continue;
+
+			core_scsi3_ua_allocate(se_deve, 0x2A,
 				ASCQ_2AH_ASYMMETRIC_ACCESS_STATE_CHANGED);
 		}
 		spin_unlock_bh(&lun->lun_deve_lock);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 436e30b..bb28a97 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -125,6 +125,25 @@ static struct t10_pr_registration *core_scsi3_locate_pr_reg(struct se_device *,
 					struct se_node_acl *, struct se_session *);
 static void core_scsi3_put_pr_reg(struct t10_pr_registration *);
 
+static void core_scsi3_pr_ua_allocate(struct se_node_acl *nacl,
+				      u32 unpacked_lun, u8 asc, u8 ascq)
+{
+	struct se_dev_entry *deve;
+
+	if (!nacl)
+		return;
+
+	rcu_read_lock();
+	deve = target_nacl_find_deve(nacl, unpacked_lun);
+	if (!deve) {
+		rcu_read_unlock();
+		return;
+	}
+
+	core_scsi3_ua_allocate(deve, asc, ascq);
+	rcu_read_unlock();
+}
+
 static int target_check_scsi2_reservation_conflict(struct se_cmd *cmd)
 {
 	struct se_session *se_sess = cmd->se_sess;
@@ -2197,7 +2216,7 @@ core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64 res_key, u64 sa_res_key,
 					&pr_tmpl->registration_list,
 					pr_reg_list) {
 
-				core_scsi3_ua_allocate(
+				core_scsi3_pr_ua_allocate(
 					pr_reg_p->pr_reg_nacl,
 					pr_reg_p->pr_res_mapped_lun,
 					0x2A,
@@ -2624,7 +2643,7 @@ core_scsi3_emulate_pro_release(struct se_cmd *cmd, int type, int scope,
 		if (pr_reg_p == pr_reg)
 			continue;
 
-		core_scsi3_ua_allocate(pr_reg_p->pr_reg_nacl,
+		core_scsi3_pr_ua_allocate(pr_reg_p->pr_reg_nacl,
 				pr_reg_p->pr_res_mapped_lun,
 				0x2A, ASCQ_2AH_RESERVATIONS_RELEASED);
 	}
@@ -2709,7 +2728,7 @@ core_scsi3_emulate_pro_clear(struct se_cmd *cmd, u64 res_key)
 		 *    additional sense code set to RESERVATIONS PREEMPTED.
 		 */
 		if (!calling_it_nexus)
-			core_scsi3_ua_allocate(pr_reg_nacl, pr_res_mapped_lun,
+			core_scsi3_pr_ua_allocate(pr_reg_nacl, pr_res_mapped_lun,
 				0x2A, ASCQ_2AH_RESERVATIONS_PREEMPTED);
 	}
 	spin_unlock(&pr_tmpl->registration_lock);
@@ -2918,7 +2937,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 						NULL, 0);
 			}
 			if (!calling_it_nexus)
-				core_scsi3_ua_allocate(pr_reg_nacl,
+				core_scsi3_pr_ua_allocate(pr_reg_nacl,
 					pr_res_mapped_lun, 0x2A,
 					ASCQ_2AH_REGISTRATIONS_PREEMPTED);
 		}
@@ -3024,7 +3043,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 		 *    persistent reservation and/or registration, with the
 		 *    additional sense code set to REGISTRATIONS PREEMPTED;
 		 */
-		core_scsi3_ua_allocate(pr_reg_nacl, pr_res_mapped_lun, 0x2A,
+		core_scsi3_pr_ua_allocate(pr_reg_nacl, pr_res_mapped_lun, 0x2A,
 				ASCQ_2AH_REGISTRATIONS_PREEMPTED);
 	}
 	spin_unlock(&pr_tmpl->registration_lock);
@@ -3057,7 +3076,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 			if (calling_it_nexus)
 				continue;
 
-			core_scsi3_ua_allocate(pr_reg->pr_reg_nacl,
+			core_scsi3_pr_ua_allocate(pr_reg->pr_reg_nacl,
 					pr_reg->pr_res_mapped_lun, 0x2A,
 					ASCQ_2AH_RESERVATIONS_RELEASED);
 		}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 3da2386..a0e0d3a 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1700,13 +1700,19 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 		 * See spc4r17, section 7.4.6 Control Mode Page, Table 349
 		 */
 		if (cmd->se_sess &&
-		    cmd->se_dev->dev_attrib.emulate_ua_intlck_ctrl == 2)
-			core_scsi3_ua_allocate(cmd->se_sess->se_node_acl,
-				cmd->orig_fe_lun, 0x2C,
-				ASCQ_2CH_PREVIOUS_RESERVATION_CONFLICT_STATUS);
-
+		    cmd->se_dev->dev_attrib.emulate_ua_intlck_ctrl == 2) {
+			struct se_dev_entry *deve;
+
+			rcu_read_lock();
+			deve = target_nacl_find_deve(cmd->se_sess->se_node_acl,
+						     cmd->orig_fe_lun);
+			if (deve)
+				core_scsi3_ua_allocate(deve, 0x2C,
+					ASCQ_2CH_PREVIOUS_RESERVATION_CONFLICT_STATUS);
+			rcu_read_unlock();
+		}
 		trace_target_cmd_complete(cmd);
-		ret = cmd->se_tfo-> queue_status(cmd);
+		ret = cmd->se_tfo->queue_status(cmd);
 		if (ret == -EAGAIN || ret == -ENOMEM)
 			goto queue_full;
 		goto check_stop;
diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index e506224..e97a708 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -87,18 +87,11 @@ target_scsi3_ua_check(struct se_cmd *cmd)
 }
 
 int core_scsi3_ua_allocate(
-	struct se_node_acl *nacl,
-	u64 unpacked_lun,
+	struct se_dev_entry *deve,
 	u8 asc,
 	u8 ascq)
 {
-	struct se_dev_entry *deve;
 	struct se_ua *ua, *ua_p, *ua_tmp;
-	/*
-	 * PASSTHROUGH OPS
-	 */
-	if (!nacl)
-		return -EINVAL;
 
 	ua = kmem_cache_zalloc(se_ua_cache, GFP_ATOMIC);
 	if (!ua) {
@@ -110,12 +103,6 @@ int core_scsi3_ua_allocate(
 	ua->ua_asc = asc;
 	ua->ua_ascq = ascq;
 
-	rcu_read_lock();
-	deve = target_nacl_find_deve(nacl, unpacked_lun);
-	if (!deve) {
-		rcu_read_unlock();
-		return -EINVAL;
-	}
 	spin_lock(&deve->ua_lock);
 	list_for_each_entry_safe(ua_p, ua_tmp, &deve->ua_list, ua_nacl_list) {
 		/*
@@ -123,7 +110,6 @@ int core_scsi3_ua_allocate(
 		 */
 		if ((ua_p->ua_asc == asc) && (ua_p->ua_ascq == ascq)) {
 			spin_unlock(&deve->ua_lock);
-			rcu_read_unlock();
 			kmem_cache_free(se_ua_cache, ua);
 			return 0;
 		}
@@ -170,19 +156,16 @@ int core_scsi3_ua_allocate(
 		spin_unlock(&deve->ua_lock);
 
 		atomic_inc_mb(&deve->ua_count);
-		rcu_read_unlock();
 		return 0;
 	}
 	list_add_tail(&ua->ua_nacl_list, &deve->ua_list);
 	spin_unlock(&deve->ua_lock);
 
-	pr_debug("[%s]: Allocated UNIT ATTENTION, mapped LUN: %llu, ASC:"
-		" 0x%02x, ASCQ: 0x%02x\n",
-		nacl->se_tpg->se_tpg_tfo->get_fabric_name(), unpacked_lun,
+	pr_debug("Allocated UNIT ATTENTION, mapped LUN: %llu, ASC:"
+		" 0x%02x, ASCQ: 0x%02x\n", deve->mapped_lun,
 		asc, ascq);
 
 	atomic_inc_mb(&deve->ua_count);
-	rcu_read_unlock();
 	return 0;
 }
 
diff --git a/drivers/target/target_core_ua.h b/drivers/target/target_core_ua.h
index 6e592b1..a9c4693 100644
--- a/drivers/target/target_core_ua.h
+++ b/drivers/target/target_core_ua.h
@@ -28,7 +28,7 @@
 extern struct kmem_cache *se_ua_cache;
 
 extern sense_reason_t target_scsi3_ua_check(struct se_cmd *);
-extern int core_scsi3_ua_allocate(struct se_node_acl *, u64, u8, u8);
+extern int core_scsi3_ua_allocate(struct se_dev_entry *, u8, u8);
 extern void core_scsi3_ua_release_all(struct se_dev_entry *);
 extern void core_scsi3_ua_for_check_condition(struct se_cmd *, u8 *, u8 *);
 extern int core_scsi3_ua_clear_for_request_sense(struct se_cmd *,
-- 
1.8.5.2

Re: [PATCH 3/6] target: use 'se_dev_entry' when allocating UAs

[Nicholas A. Bellinger]

Hey Hannes,

Apologies for the delayed follow-up on these, one comment below.

On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote:
> We need to use 'se_dev_entry' as argument when allocating
> UAs, otherwise we'll never see any UAs for an implicit
> ALUA state transition triggered from userspace.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/target/target_core_alua.c      | 27 ++++++++++++++++++---------
>  drivers/target/target_core_pr.c        | 31 +++++++++++++++++++++++++------
>  drivers/target/target_core_transport.c | 18 ++++++++++++------
>  drivers/target/target_core_ua.c        | 23 +++--------------------
>  drivers/target/target_core_ua.h        |  2 +-
>  5 files changed, 59 insertions(+), 42 deletions(-)
> 

<SNIP>

> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 436e30b..bb28a97 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -125,6 +125,25 @@ static struct t10_pr_registration *core_scsi3_locate_pr_reg(struct se_device *,
>  					struct se_node_acl *, struct se_session *);
>  static void core_scsi3_put_pr_reg(struct t10_pr_registration *);
>  
> +static void core_scsi3_pr_ua_allocate(struct se_node_acl *nacl,
> +				      u32 unpacked_lun, u8 asc, u8 ascq)
> +{
> +	struct se_dev_entry *deve;
> +
> +	if (!nacl)
> +		return;
> +
> +	rcu_read_lock();
> +	deve = target_nacl_find_deve(nacl, unpacked_lun);
> +	if (!deve) {
> +		rcu_read_unlock();
> +		return;
> +	}
> +
> +	core_scsi3_ua_allocate(deve, asc, ascq);
> +	rcu_read_unlock();
> +}
> +

This should be common for TCM_RESERVATION_CONFLICT case outside of PR
code too.

Any objections for squashing the following into your original patch..?

Thank you,

--nab

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index bb28a97..0bb3292 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -125,25 +125,6 @@ static struct t10_pr_registration *core_scsi3_locate_pr_reg(struct se_device *,
 					struct se_node_acl *, struct se_session *);
 static void core_scsi3_put_pr_reg(struct t10_pr_registration *);
 
-static void core_scsi3_pr_ua_allocate(struct se_node_acl *nacl,
-				      u32 unpacked_lun, u8 asc, u8 ascq)
-{
-	struct se_dev_entry *deve;
-
-	if (!nacl)
-		return;
-
-	rcu_read_lock();
-	deve = target_nacl_find_deve(nacl, unpacked_lun);
-	if (!deve) {
-		rcu_read_unlock();
-		return;
-	}
-
-	core_scsi3_ua_allocate(deve, asc, ascq);
-	rcu_read_unlock();
-}
-
 static int target_check_scsi2_reservation_conflict(struct se_cmd *cmd)
 {
 	struct se_session *se_sess = cmd->se_sess;
@@ -2216,7 +2197,7 @@ core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64 res_key, u64 sa_res_key,
 					&pr_tmpl->registration_list,
 					pr_reg_list) {
 
-				core_scsi3_pr_ua_allocate(
+				target_ua_allocate_lun(
 					pr_reg_p->pr_reg_nacl,
 					pr_reg_p->pr_res_mapped_lun,
 					0x2A,
@@ -2643,7 +2624,7 @@ core_scsi3_emulate_pro_release(struct se_cmd *cmd, int type, int scope,
 		if (pr_reg_p == pr_reg)
 			continue;
 
-		core_scsi3_pr_ua_allocate(pr_reg_p->pr_reg_nacl,
+		target_ua_allocate_lun(pr_reg_p->pr_reg_nacl,
 				pr_reg_p->pr_res_mapped_lun,
 				0x2A, ASCQ_2AH_RESERVATIONS_RELEASED);
 	}
@@ -2728,7 +2709,7 @@ core_scsi3_emulate_pro_clear(struct se_cmd *cmd, u64 res_key)
 		 *    additional sense code set to RESERVATIONS PREEMPTED.
 		 */
 		if (!calling_it_nexus)
-			core_scsi3_pr_ua_allocate(pr_reg_nacl, pr_res_mapped_lun,
+			target_ua_allocate_lun(pr_reg_nacl, pr_res_mapped_lun,
 				0x2A, ASCQ_2AH_RESERVATIONS_PREEMPTED);
 	}
 	spin_unlock(&pr_tmpl->registration_lock);
@@ -2937,7 +2918,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 						NULL, 0);
 			}
 			if (!calling_it_nexus)
-				core_scsi3_pr_ua_allocate(pr_reg_nacl,
+				target_ua_allocate_lun(pr_reg_nacl,
 					pr_res_mapped_lun, 0x2A,
 					ASCQ_2AH_REGISTRATIONS_PREEMPTED);
 		}
@@ -3043,7 +3024,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 		 *    persistent reservation and/or registration, with the
 		 *    additional sense code set to REGISTRATIONS PREEMPTED;
 		 */
-		core_scsi3_pr_ua_allocate(pr_reg_nacl, pr_res_mapped_lun, 0x2A,
+		target_ua_allocate_lun(pr_reg_nacl, pr_res_mapped_lun, 0x2A,
 				ASCQ_2AH_REGISTRATIONS_PREEMPTED);
 	}
 	spin_unlock(&pr_tmpl->registration_lock);
@@ -3076,7 +3057,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 			if (calling_it_nexus)
 				continue;
 
-			core_scsi3_pr_ua_allocate(pr_reg->pr_reg_nacl,
+			target_ua_allocate_lun(pr_reg->pr_reg_nacl,
 					pr_reg->pr_res_mapped_lun, 0x2A,
 					ASCQ_2AH_RESERVATIONS_RELEASED);
 		}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index bd63254..201c33c 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1678,15 +1678,9 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 		 */
 		if (cmd->se_sess &&
 		    cmd->se_dev->dev_attrib.emulate_ua_intlck_ctrl == 2) {
-			struct se_dev_entry *deve;
-
-			rcu_read_lock();
-			deve = target_nacl_find_deve(cmd->se_sess->se_node_acl,
-						     cmd->orig_fe_lun);
-			if (deve)
-				core_scsi3_ua_allocate(deve, 0x2C,
+			target_ua_allocate_lun(cmd->se_sess->se_node_acl,
+					       cmd->orig_fe_lun, 0x2C,
 					ASCQ_2CH_PREVIOUS_RESERVATION_CONFLICT_STATUS);
-			rcu_read_unlock();
 		}
 		trace_target_cmd_complete(cmd);
 		ret = cmd->se_tfo->queue_status(cmd);
diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index e97a708..fc095ae 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -169,6 +169,25 @@ int core_scsi3_ua_allocate(
 	return 0;
 }
 
+void target_ua_allocate_lun(struct se_node_acl *nacl,
+			    u32 unpacked_lun, u8 asc, u8 ascq)
+{
+	struct se_dev_entry *deve;
+
+	if (!nacl)
+		return;
+
+	rcu_read_lock();
+	deve = target_nacl_find_deve(nacl, unpacked_lun);
+	if (!deve) {
+		rcu_read_unlock();
+		return;
+	}
+
+	core_scsi3_ua_allocate(deve, asc, ascq);
+	rcu_read_unlock();
+}
+
 void core_scsi3_ua_release_all(
 	struct se_dev_entry *deve)
 {
diff --git a/drivers/target/target_core_ua.h b/drivers/target/target_core_ua.h
index 45e3b6d..bd6e78b 100644
--- a/drivers/target/target_core_ua.h
+++ b/drivers/target/target_core_ua.h
@@ -32,6 +32,7 @@ extern struct kmem_cache *se_ua_cache;
 
 extern sense_reason_t target_scsi3_ua_check(struct se_cmd *);
 extern int core_scsi3_ua_allocate(struct se_dev_entry *, u8, u8);
+extern void target_ua_allocate_lun(struct se_node_acl *, u32, u8, u8);
 extern void core_scsi3_ua_release_all(struct se_dev_entry *);
 extern void core_scsi3_ua_for_check_condition(struct se_cmd *, u8 *, u8 *);
 extern int core_scsi3_ua_clear_for_request_sense(struct se_cmd *,

Re: [PATCH 3/6] target: use 'se_dev_entry' when allocating UAs

[Hannes Reinecke]

On 06/17/2015 08:06 AM, Nicholas A. Bellinger wrote:
> Hey Hannes,
> 
> Apologies for the delayed follow-up on these, one comment below.
> 
> On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote:
>> We need to use 'se_dev_entry' as argument when allocating
>> UAs, otherwise we'll never see any UAs for an implicit
>> ALUA state transition triggered from userspace.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/target/target_core_alua.c      | 27 ++++++++++++++++++---------
>>  drivers/target/target_core_pr.c        | 31 +++++++++++++++++++++++++------
>>  drivers/target/target_core_transport.c | 18 ++++++++++++------
>>  drivers/target/target_core_ua.c        | 23 +++--------------------
>>  drivers/target/target_core_ua.h        |  2 +-
>>  5 files changed, 59 insertions(+), 42 deletions(-)
>>
> 
> <SNIP>
> 
>> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
>> index 436e30b..bb28a97 100644
>> --- a/drivers/target/target_core_pr.c
>> +++ b/drivers/target/target_core_pr.c
>> @@ -125,6 +125,25 @@ static struct t10_pr_registration *core_scsi3_locate_pr_reg(struct se_device *,
>>  					struct se_node_acl *, struct se_session *);
>>  static void core_scsi3_put_pr_reg(struct t10_pr_registration *);
>>  
>> +static void core_scsi3_pr_ua_allocate(struct se_node_acl *nacl,
>> +				      u32 unpacked_lun, u8 asc, u8 ascq)
>> +{
>> +	struct se_dev_entry *deve;
>> +
>> +	if (!nacl)
>> +		return;
>> +
>> +	rcu_read_lock();
>> +	deve = target_nacl_find_deve(nacl, unpacked_lun);
>> +	if (!deve) {
>> +		rcu_read_unlock();
>> +		return;
>> +	}
>> +
>> +	core_scsi3_ua_allocate(deve, asc, ascq);
>> +	rcu_read_unlock();
>> +}
>> +
> 
> This should be common for TCM_RESERVATION_CONFLICT case outside of PR
> code too.
> 
> Any objections for squashing the following into your original patch..?
> 
> Thank you,
> 
> --nab
> 
[ .. ]

None at all.
Do go ahead.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH 4/6] target: Send UA on ALUA target port group change

[Hannes Reinecke]

When the ALUA target port group changes an INQUIRY DATA CHANGE
UA needs to be sent.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_alua.c | 7 +++++++
 drivers/target/target_core_ua.h   | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index aa2e4b1..edaf1b9 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -1880,12 +1880,19 @@ static void core_alua_put_tg_pt_gp_from_name(
 static void __target_attach_tg_pt_gp(struct se_lun *lun,
 		struct t10_alua_tg_pt_gp *tg_pt_gp)
 {
+	struct se_dev_entry *se_deve;
+
 	assert_spin_locked(&lun->lun_tg_pt_gp_lock);
 
 	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
 	lun->lun_tg_pt_gp = tg_pt_gp;
 	list_add_tail(&lun->lun_tg_pt_gp_link, &tg_pt_gp->tg_pt_gp_lun_list);
 	tg_pt_gp->tg_pt_gp_members++;
+	spin_lock_bh(&lun->lun_deve_lock);
+	list_for_each_entry(se_deve, &lun->lun_deve_list, lun_link)
+		core_scsi3_ua_allocate(se_deve, 0x3f,
+				       ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
+	spin_unlock_bh(&lun->lun_deve_lock);
 	spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
 }
 
diff --git a/drivers/target/target_core_ua.h b/drivers/target/target_core_ua.h
index a9c4693..948ae1e 100644
--- a/drivers/target/target_core_ua.h
+++ b/drivers/target/target_core_ua.h
@@ -25,6 +25,8 @@
 
 #define ASCQ_2CH_PREVIOUS_RESERVATION_CONFLICT_STATUS		0x09
 
+#define ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED			0x03
+
 extern struct kmem_cache *se_ua_cache;
 
 extern sense_reason_t target_scsi3_ua_check(struct se_cmd *);
-- 
1.8.5.2

Re: [PATCH 4/6] target: Send UA on ALUA target port group change

[Christoph Hellwig]

> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -1880,12 +1880,19 @@ static void core_alua_put_tg_pt_gp_from_name(
>  static void __target_attach_tg_pt_gp(struct se_lun *lun,
>  		struct t10_alua_tg_pt_gp *tg_pt_gp)
>  {
> +	struct se_dev_entry *se_deve;
> +
>  	assert_spin_locked(&lun->lun_tg_pt_gp_lock);
>  
>  	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
>  	lun->lun_tg_pt_gp = tg_pt_gp;
>  	list_add_tail(&lun->lun_tg_pt_gp_link, &tg_pt_gp->tg_pt_gp_lun_list);
>  	tg_pt_gp->tg_pt_gp_members++;
> +	spin_lock_bh(&lun->lun_deve_lock);
> +	list_for_each_entry(se_deve, &lun->lun_deve_list, lun_link)
> +		core_scsi3_ua_allocate(se_deve, 0x3f,
> +				       ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> +	spin_unlock_bh(&lun->lun_deve_lock);
>  	spin_unlock(&tg_pt_gp->tg_pt_gp_lock);

Taking a _bh lock inside a regular spinlock is completely broken.

Fortunately I don't think lun_deve_lock needs to disable bottom halves,
but this needs to be fixed first.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH 4/6] target: Send UA on ALUA target port group change

[Hannes Reinecke]

On 06/19/2015 03:05 PM, Christoph Hellwig wrote:
>> --- a/drivers/target/target_core_alua.c
>> +++ b/drivers/target/target_core_alua.c
>> @@ -1880,12 +1880,19 @@ static void core_alua_put_tg_pt_gp_from_name(
>>  static void __target_attach_tg_pt_gp(struct se_lun *lun,
>>  		struct t10_alua_tg_pt_gp *tg_pt_gp)
>>  {
>> +	struct se_dev_entry *se_deve;
>> +
>>  	assert_spin_locked(&lun->lun_tg_pt_gp_lock);
>>  
>>  	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
>>  	lun->lun_tg_pt_gp = tg_pt_gp;
>>  	list_add_tail(&lun->lun_tg_pt_gp_link, &tg_pt_gp->tg_pt_gp_lun_list);
>>  	tg_pt_gp->tg_pt_gp_members++;
>> +	spin_lock_bh(&lun->lun_deve_lock);
>> +	list_for_each_entry(se_deve, &lun->lun_deve_list, lun_link)
>> +		core_scsi3_ua_allocate(se_deve, 0x3f,
>> +				       ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
>> +	spin_unlock_bh(&lun->lun_deve_lock);
>>  	spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
> 
> Taking a _bh lock inside a regular spinlock is completely broken.
> 
> Fortunately I don't think lun_deve_lock needs to disable bottom halves,
> but this needs to be fixed first.
> 
This harks back to my previous mail:
Under which circumstances will there be more than one se_dev_entry
structures in lun_deve_list?
Isn't there a 1:1 relationship?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH 4/6] target: Send UA on ALUA target port group change

[Christoph Hellwig]

On Fri, Jun 19, 2015 at 03:09:34PM +0200, Hannes Reinecke wrote:
> This harks back to my previous mail:
> Under which circumstances will there be more than one se_dev_entry
> structures in lun_deve_list?
> Isn't there a 1:1 relationship?

No.  See my answer to your previous mail.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH 4/6] target: Send UA on ALUA target port group change

[Nicholas A. Bellinger]

On Fri, 2015-06-19 at 15:05 +0200, Christoph Hellwig wrote:
> > --- a/drivers/target/target_core_alua.c
> > +++ b/drivers/target/target_core_alua.c
> > @@ -1880,12 +1880,19 @@ static void core_alua_put_tg_pt_gp_from_name(
> >  static void __target_attach_tg_pt_gp(struct se_lun *lun,
> >  		struct t10_alua_tg_pt_gp *tg_pt_gp)
> >  {
> > +	struct se_dev_entry *se_deve;
> > +
> >  	assert_spin_locked(&lun->lun_tg_pt_gp_lock);
> >  
> >  	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
> >  	lun->lun_tg_pt_gp = tg_pt_gp;
> >  	list_add_tail(&lun->lun_tg_pt_gp_link, &tg_pt_gp->tg_pt_gp_lun_list);
> >  	tg_pt_gp->tg_pt_gp_members++;
> > +	spin_lock_bh(&lun->lun_deve_lock);
> > +	list_for_each_entry(se_deve, &lun->lun_deve_list, lun_link)
> > +		core_scsi3_ua_allocate(se_deve, 0x3f,
> > +				       ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> > +	spin_unlock_bh(&lun->lun_deve_lock);
> >  	spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
> 
> Taking a _bh lock inside a regular spinlock is completely broken.
> 

...

> Fortunately I don't think lun_deve_lock needs to disable bottom halves,
> but this needs to be fixed first.

<nod>

Applying the following + updating this original patch to use normal
spinlock_t access.

>From 1adff1b3a7f75a1c255b7fcab5676edf29d4a5d8 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Mon, 22 Jun 2015 23:44:05 -0700
Subject: [PATCH 65/76] target: Convert se_lun->lun_deve_lock to normal
 spinlock

This patch converts se_lun->lun_deve_lock acquire/release access
to use a normal, non bottom-half spin_lock_t for protecting
se_lun->lun_deve_list access.

Reported-by: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_alua.c   |  4 ++--
 drivers/target/target_core_device.c | 12 ++++++------
 drivers/target/target_core_pr.c     |  8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index aa2e4b1..c56ae02 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -968,7 +968,7 @@ static void core_alua_queue_state_change_ua(struct t10_alua_tg_pt_gp *tg_pt_gp)
 			continue;
 		spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
 
-		spin_lock_bh(&lun->lun_deve_lock);
+		spin_lock(&lun->lun_deve_lock);
 		list_for_each_entry(se_deve, &lun->lun_deve_list, lun_link) {
 			lacl = rcu_dereference_check(se_deve->se_lun_acl,
 					lockdep_is_held(&lun->lun_deve_lock));
@@ -1000,7 +1000,7 @@ static void core_alua_queue_state_change_ua(struct t10_alua_tg_pt_gp *tg_pt_gp)
 			core_scsi3_ua_allocate(se_deve, 0x2A,
 				ASCQ_2AH_ASYMMETRIC_ACCESS_STATE_CHANGED);
 		}
-		spin_unlock_bh(&lun->lun_deve_lock);
+		spin_unlock(&lun->lun_deve_lock);
 
 		spin_lock(&tg_pt_gp->tg_pt_gp_lock);
 		percpu_ref_put(&lun->lun_ref);
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index ed08402..b6df5b9 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -352,10 +352,10 @@ int core_enable_device_list_for_node(
 		hlist_add_head_rcu(&new->link, &nacl->lun_entry_hlist);
 		mutex_unlock(&nacl->lun_entry_mutex);
 
-		spin_lock_bh(&lun->lun_deve_lock);
+		spin_lock(&lun->lun_deve_lock);
 		list_del(&orig->lun_link);
 		list_add_tail(&new->lun_link, &lun->lun_deve_list);
-		spin_unlock_bh(&lun->lun_deve_lock);
+		spin_unlock(&lun->lun_deve_lock);
 
 		kref_put(&orig->pr_kref, target_pr_kref_release);
 		wait_for_completion(&orig->pr_comp);
@@ -369,9 +369,9 @@ int core_enable_device_list_for_node(
 	hlist_add_head_rcu(&new->link, &nacl->lun_entry_hlist);
 	mutex_unlock(&nacl->lun_entry_mutex);
 
-	spin_lock_bh(&lun->lun_deve_lock);
+	spin_lock(&lun->lun_deve_lock);
 	list_add_tail(&new->lun_link, &lun->lun_deve_list);
-	spin_unlock_bh(&lun->lun_deve_lock);
+	spin_unlock(&lun->lun_deve_lock);
 
 	return 0;
 }
@@ -403,9 +403,9 @@ void core_disable_device_list_for_node(
 	 * NodeACL context specific PR metadata for demo-mode
 	 * MappedLUN *deve will be released below..
 	 */
-	spin_lock_bh(&lun->lun_deve_lock);
+	spin_lock(&lun->lun_deve_lock);
 	list_del(&orig->lun_link);
-	spin_unlock_bh(&lun->lun_deve_lock);
+	spin_unlock(&lun->lun_deve_lock);
 	/*
 	 * Disable struct se_dev_entry LUN ACL mapping
 	 */
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 0bb3292..7403b03 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -709,7 +709,7 @@ static struct t10_pr_registration *__core_scsi3_alloc_registration(
 			continue;
 		spin_unlock(&dev->se_port_lock);
 
-		spin_lock_bh(&lun_tmp->lun_deve_lock);
+		spin_lock(&lun_tmp->lun_deve_lock);
 		list_for_each_entry(deve_tmp, &lun_tmp->lun_deve_list, lun_link) {
 			/*
 			 * This pointer will be NULL for demo mode MappedLUNs
@@ -742,7 +742,7 @@ static struct t10_pr_registration *__core_scsi3_alloc_registration(
 				continue;
 
 			kref_get(&deve_tmp->pr_kref);
-			spin_unlock_bh(&lun_tmp->lun_deve_lock);
+			spin_unlock(&lun_tmp->lun_deve_lock);
 			/*
 			 * Grab a configfs group dependency that is released
 			 * for the exception path at label out: below, or upon
@@ -779,9 +779,9 @@ static struct t10_pr_registration *__core_scsi3_alloc_registration(
 
 			list_add_tail(&pr_reg_atp->pr_reg_atp_mem_list,
 				      &pr_reg->pr_reg_atp_list);
-			spin_lock_bh(&lun_tmp->lun_deve_lock);
+			spin_lock(&lun_tmp->lun_deve_lock);
 		}
-		spin_unlock_bh(&lun_tmp->lun_deve_lock);
+		spin_unlock(&lun_tmp->lun_deve_lock);
 
 		spin_lock(&dev->se_port_lock);
 		percpu_ref_put(&lun_tmp->lun_ref);
-- 
1.9.1

[PATCH 5/6] target: Send UA upon LUN RESET tmr completion

[Hannes Reinecke]

SAM mandates that an BUS DEVICE RESET FUNCTION OCCURRED
UA needs to be send after a LUN RESET tmr has completed.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_transport.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index a0e0d3a..bb60c0c4 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3064,6 +3064,17 @@ static void target_tmr_work(struct work_struct *work)
 		ret = core_tmr_lun_reset(dev, tmr, NULL, NULL);
 		tmr->response = (!ret) ? TMR_FUNCTION_COMPLETE :
 					 TMR_FUNCTION_REJECTED;
+		if (tmr->response == TMR_FUNCTION_COMPLETE) {
+			struct se_dev_entry *deve;
+
+			rcu_read_lock();
+			deve = target_nacl_find_deve(cmd->se_sess->se_node_acl,
+						     cmd->orig_fe_lun);
+			if (deve)
+				core_scsi3_ua_allocate(deve, 0x29,
+					ASCQ_29H_BUS_DEVICE_RESET_FUNCTION_OCCURRED);
+			rcu_read_unlock();
+		}
 		break;
 	case TMR_TARGET_WARM_RESET:
 		tmr->response = TMR_FUNCTION_REJECTED;
-- 
1.8.5.2

Re: [PATCH 5/6] target: Send UA upon LUN RESET tmr completion

[Christoph Hellwig]

On Thu, Jun 11, 2015 at 10:01:28AM +0200, Hannes Reinecke wrote:
> SAM mandates that an BUS DEVICE RESET FUNCTION OCCURRED
> UA needs to be send after a LUN RESET tmr has completed.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/target/target_core_transport.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index a0e0d3a..bb60c0c4 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -3064,6 +3064,17 @@ static void target_tmr_work(struct work_struct *work)
>  		ret = core_tmr_lun_reset(dev, tmr, NULL, NULL);
>  		tmr->response = (!ret) ? TMR_FUNCTION_COMPLETE :
>  					 TMR_FUNCTION_REJECTED;
> +		if (tmr->response == TMR_FUNCTION_COMPLETE) {
> +			struct se_dev_entry *deve;
> +
> +			rcu_read_lock();
> +			deve = target_nacl_find_deve(cmd->se_sess->se_node_acl,
> +						     cmd->orig_fe_lun);
> +			if (deve)
> +				core_scsi3_ua_allocate(deve, 0x29,
> +					ASCQ_29H_BUS_DEVICE_RESET_FUNCTION_OCCURRED);
> +			rcu_read_unlock();

This should use the target_ua_allocate_lun helper.

Re: [PATCH 5/6] target: Send UA upon LUN RESET tmr completion

[Hannes Reinecke]

On 06/19/2015 03:06 PM, Christoph Hellwig wrote:
> On Thu, Jun 11, 2015 at 10:01:28AM +0200, Hannes Reinecke wrote:
>> SAM mandates that an BUS DEVICE RESET FUNCTION OCCURRED
>> UA needs to be send after a LUN RESET tmr has completed.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/target/target_core_transport.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>> index a0e0d3a..bb60c0c4 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -3064,6 +3064,17 @@ static void target_tmr_work(struct work_struct *work)
>>  		ret = core_tmr_lun_reset(dev, tmr, NULL, NULL);
>>  		tmr->response = (!ret) ? TMR_FUNCTION_COMPLETE :
>>  					 TMR_FUNCTION_REJECTED;
>> +		if (tmr->response == TMR_FUNCTION_COMPLETE) {
>> +			struct se_dev_entry *deve;
>> +
>> +			rcu_read_lock();
>> +			deve = target_nacl_find_deve(cmd->se_sess->se_node_acl,
>> +						     cmd->orig_fe_lun);
>> +			if (deve)
>> +				core_scsi3_ua_allocate(deve, 0x29,
>> +					ASCQ_29H_BUS_DEVICE_RESET_FUNCTION_OCCURRED);
>> +			rcu_read_unlock();
> 
> This should use the target_ua_allocate_lun helper.
> 
Yep, will be doing so.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH 5/6] target: Send UA upon LUN RESET tmr completion

[Nicholas A. Bellinger]

On Fri, 2015-06-19 at 15:06 +0200, Christoph Hellwig wrote:
> On Thu, Jun 11, 2015 at 10:01:28AM +0200, Hannes Reinecke wrote:
> > SAM mandates that an BUS DEVICE RESET FUNCTION OCCURRED
> > UA needs to be send after a LUN RESET tmr has completed.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> >  drivers/target/target_core_transport.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index a0e0d3a..bb60c0c4 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -3064,6 +3064,17 @@ static void target_tmr_work(struct work_struct *work)
> >  		ret = core_tmr_lun_reset(dev, tmr, NULL, NULL);
> >  		tmr->response = (!ret) ? TMR_FUNCTION_COMPLETE :
> >  					 TMR_FUNCTION_REJECTED;
> > +		if (tmr->response == TMR_FUNCTION_COMPLETE) {
> > +			struct se_dev_entry *deve;
> > +
> > +			rcu_read_lock();
> > +			deve = target_nacl_find_deve(cmd->se_sess->se_node_acl,
> > +						     cmd->orig_fe_lun);
> > +			if (deve)
> > +				core_scsi3_ua_allocate(deve, 0x29,
> > +					ASCQ_29H_BUS_DEVICE_RESET_FUNCTION_OCCURRED);
> > +			rcu_read_unlock();
> 
> This should use the target_ua_allocate_lun helper.

Fixed.

[PATCH 6/6] target: Send UA when changing LUN inventory

[Hannes Reinecke]

When changind the LUN inventory via core_enable_device_list_for_node()
or core_disable_device_list_for_node() a REPORTED LUNS DATA HAS CHANGED
UA should be send.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_device.c | 26 +++++++++++++++++++++++++-
 drivers/target/target_core_ua.h     |  1 +
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 650613e..ac191a9 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -305,7 +305,7 @@ int core_enable_device_list_for_node(
 	struct se_node_acl *nacl,
 	struct se_portal_group *tpg)
 {
-	struct se_dev_entry *orig, *new;
+	struct se_dev_entry *orig, *new, *tmp;
 
 	new = kzalloc(sizeof(*new), GFP_KERNEL);
 	if (!new) {
@@ -360,6 +360,15 @@ int core_enable_device_list_for_node(
 		kref_put(&orig->pr_kref, target_pr_kref_release);
 		wait_for_completion(&orig->pr_comp);
 
+		rcu_read_lock();
+		hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
+			if (tmp == new)
+				continue;
+			core_scsi3_ua_allocate(tmp, 0x3F,
+				ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
+		}
+		rcu_read_unlock();
+
 		kfree_rcu(orig, rcu_head);
 		return 0;
 	}
@@ -373,6 +382,14 @@ int core_enable_device_list_for_node(
 	list_add_tail(&new->lun_link, &lun->lun_deve_list);
 	spin_unlock_bh(&lun->lun_deve_lock);
 
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
+		if (tmp == new)
+			continue;
+		core_scsi3_ua_allocate(tmp, 0x3F,
+			ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
+	}
+	rcu_read_unlock();
 	return 0;
 }
 
@@ -385,6 +402,7 @@ void core_disable_device_list_for_node(
 	struct se_node_acl *nacl,
 	struct se_portal_group *tpg)
 {
+	struct se_dev_entry *tmp;
 	/*
 	 * rcu_dereference_raw protected by se_lun->lun_group symlink
 	 * reference to se_device->dev_group.
@@ -428,6 +446,12 @@ void core_disable_device_list_for_node(
 	kfree_rcu(orig, rcu_head);
 
 	core_scsi3_free_pr_reg_from_nacl(dev, nacl);
+
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link)
+		core_scsi3_ua_allocate(tmp, 0x3F,
+			ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
+	rcu_read_unlock();
 }
 
 /*      core_clear_lun_from_tpg():
diff --git a/drivers/target/target_core_ua.h b/drivers/target/target_core_ua.h
index 948ae1e..45e3b6d 100644
--- a/drivers/target/target_core_ua.h
+++ b/drivers/target/target_core_ua.h
@@ -26,6 +26,7 @@
 #define ASCQ_2CH_PREVIOUS_RESERVATION_CONFLICT_STATUS		0x09
 
 #define ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED			0x03
+#define ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED			0x0E
 
 extern struct kmem_cache *se_ua_cache;
 
-- 
1.8.5.2

Re: [PATCH 6/6] target: Send UA when changing LUN inventory

[Christoph Hellwig]

> +		hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
> +			if (tmp == new)
> +				continue;
> +			core_scsi3_ua_allocate(tmp, 0x3F,
> +				ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
> +		}
> +		rcu_read_unlock();
> +

> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
> +		if (tmp == new)
> +			continue;
> +		core_scsi3_ua_allocate(tmp, 0x3F,
> +			ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
> +	}
> +	rcu_read_unlock();

> +
> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link)
> +		core_scsi3_ua_allocate(tmp, 0x3F,
> +			ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
> +	rcu_read_unlock();

Please add a helper instead of duplicating this three times.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in

Re: [PATCH 6/6] target: Send UA when changing LUN inventory

[Hannes Reinecke]

On 06/19/2015 03:07 PM, Christoph Hellwig wrote:
>> +		hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
>> +			if (tmp == new)
>> +				continue;
>> +			core_scsi3_ua_allocate(tmp, 0x3F,
>> +				ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
>> +		}
>> +		rcu_read_unlock();
>> +
> 
>> +	rcu_read_lock();
>> +	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
>> +		if (tmp == new)
>> +			continue;
>> +		core_scsi3_ua_allocate(tmp, 0x3F,
>> +			ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
>> +	}
>> +	rcu_read_unlock();
> 
>> +
>> +	rcu_read_lock();
>> +	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link)
>> +		core_scsi3_ua_allocate(tmp, 0x3F,
>> +			ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
>> +	rcu_read_unlock();
> 
> Please add a helper instead of duplicating this three times.
> 
Okay.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 6/6] target: Send UA when changing LUN inventory

[Nicholas A. Bellinger]

On Fri, 2015-06-19 at 15:07 +0200, Christoph Hellwig wrote:
> > +		hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
> > +			if (tmp == new)
> > +				continue;
> > +			core_scsi3_ua_allocate(tmp, 0x3F,
> > +				ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
> > +		}
> > +		rcu_read_unlock();
> > +
> 
> > +	rcu_read_lock();
> > +	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
> > +		if (tmp == new)
> > +			continue;
> > +		core_scsi3_ua_allocate(tmp, 0x3F,
> > +			ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
> > +	}
> > +	rcu_read_unlock();
> 
> > +
> > +	rcu_read_lock();
> > +	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link)
> > +		core_scsi3_ua_allocate(tmp, 0x3F,
> > +			ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
> > +	rcu_read_unlock();
> 
> Please add a helper instead of duplicating this three times.

<nod>

Applying the following squashed commit:

>From 7c0d0d51d26497866d2951a35f1736fc765e4fcf Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Thu, 11 Jun 2015 10:01:29 +0200
Subject: [PATCH 68/76] target: Send UA when changing LUN inventory

When changind the LUN inventory via core_enable_device_list_for_node()
or core_disable_device_list_for_node() a REPORTED LUNS DATA HAS CHANGED
UA should be send.

(Convert to target_luns_data_has_changed helper usage - hch)

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_device.c | 23 +++++++++++++++++++----
 drivers/target/target_core_ua.h     |  1 +
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index b6df5b9..5244848 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -293,10 +293,22 @@ void target_pr_kref_release(struct kref *kref)
 	complete(&deve->pr_comp);
 }
 
-/*      core_enable_device_list_for_node():
- *
- *
- */
+static void
+target_luns_data_has_changed(struct se_node_acl *nacl, struct se_dev_entry *new,
+			     bool skip_new)
+{
+	struct se_dev_entry *tmp;
+
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(tmp, &nacl->lun_entry_hlist, link) {
+		if (skip_new && tmp == new)
+			continue;
+		core_scsi3_ua_allocate(tmp, 0x3F,
+				       ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED);
+	}
+	rcu_read_unlock();
+}
+
 int core_enable_device_list_for_node(
 	struct se_lun *lun,
 	struct se_lun_acl *lun_acl,
@@ -360,6 +372,7 @@ int core_enable_device_list_for_node(
 		kref_put(&orig->pr_kref, target_pr_kref_release);
 		wait_for_completion(&orig->pr_comp);
 
+		target_luns_data_has_changed(nacl, new, true);
 		kfree_rcu(orig, rcu_head);
 		return 0;
 	}
@@ -373,6 +386,7 @@ int core_enable_device_list_for_node(
 	list_add_tail(&new->lun_link, &lun->lun_deve_list);
 	spin_unlock(&lun->lun_deve_lock);
 
+	target_luns_data_has_changed(nacl, new, true);
 	return 0;
 }
 
@@ -428,6 +442,7 @@ void core_disable_device_list_for_node(
 	kfree_rcu(orig, rcu_head);
 
 	core_scsi3_free_pr_reg_from_nacl(dev, nacl);
+	target_luns_data_has_changed(nacl, NULL, false);
 }
 
 /*      core_clear_lun_from_tpg():
diff --git a/drivers/target/target_core_ua.h b/drivers/target/target_core_ua.h
index 59278d6..bd6e78b 100644
--- a/drivers/target/target_core_ua.h
+++ b/drivers/target/target_core_ua.h
@@ -26,6 +26,7 @@
 #define ASCQ_2CH_PREVIOUS_RESERVATION_CONFLICT_STATUS		0x09
 
 #define ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED			0x03
+#define ASCQ_3FH_REPORTED_LUNS_DATA_HAS_CHANGED			0x0E
 
 extern struct kmem_cache *se_ua_cache;
 
-- 
1.9.1

Re: [PATCH 0/6] target: Update UA handling

[Nicholas A. Bellinger]

On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote:
> Hi Nic,
> 
> lio-target is very minimalistic when it comes to generate UAs;
> primarily they are generated for persistent reservations, but
> generic changes tend to be ignored.
> 
> This patchset updates the UA handling and generates UA for internal
> state changes (REPORTED LUNS DATA CHANGED, INQUIRY DATA CHANGED,
> and LUN RESET OCCURRED).
> 
> Funnily enough this triggers some issues with the SCSI stack;
> I'll be sending out patches for that, too.
> 
> Hannes Reinecke (6):
>   target_core_alua: Correct UA handling when switching states
>   target: Remove 'ua_nacl' pointer from se_ua structure
>   target: use 'se_dev_entry' when allocating UAs
>   target: Send UA on ALUA target port group change
>   target: Send UA upon LUN RESET tmr completion
>   target: Send UA when changing LUN inventory
> 
>  drivers/target/target_core_alua.c      | 56 +++++++++++++++++++++++++---------
>  drivers/target/target_core_device.c    | 26 +++++++++++++++-
>  drivers/target/target_core_pr.c        | 31 +++++++++++++++----
>  drivers/target/target_core_transport.c | 29 ++++++++++++++----
>  drivers/target/target_core_ua.c        | 24 ++-------------
>  drivers/target/target_core_ua.h        |  5 ++-
>  include/target/target_core_base.h      |  1 -
>  7 files changed, 121 insertions(+), 51 deletions(-)
> 

Applied to target-pending/for-next, with the extra incremental patch for
a common target_ua_alloc_lun() caller.

Btw, very happy to see REPORTED_LUNS_DATA_HAS_CHANGED support include
for v4.2-rc1 code.  8-)

Thanks Hannes!

--nab

Re: [PATCH 0/6] target: Update UA handling

[Hannes Reinecke]

On 06/17/2015 08:10 AM, Nicholas A. Bellinger wrote:
> On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote:
>> Hi Nic,
>>
>> lio-target is very minimalistic when it comes to generate UAs;
>> primarily they are generated for persistent reservations, but
>> generic changes tend to be ignored.
>>
>> This patchset updates the UA handling and generates UA for internal
>> state changes (REPORTED LUNS DATA CHANGED, INQUIRY DATA CHANGED,
>> and LUN RESET OCCURRED).
>>
>> Funnily enough this triggers some issues with the SCSI stack;
>> I'll be sending out patches for that, too.
>>
>> Hannes Reinecke (6):
>>   target_core_alua: Correct UA handling when switching states
>>   target: Remove 'ua_nacl' pointer from se_ua structure
>>   target: use 'se_dev_entry' when allocating UAs
>>   target: Send UA on ALUA target port group change
>>   target: Send UA upon LUN RESET tmr completion
>>   target: Send UA when changing LUN inventory
>>
>>  drivers/target/target_core_alua.c      | 56 +++++++++++++++++++++++++---------
>>  drivers/target/target_core_device.c    | 26 +++++++++++++++-
>>  drivers/target/target_core_pr.c        | 31 +++++++++++++++----
>>  drivers/target/target_core_transport.c | 29 ++++++++++++++----
>>  drivers/target/target_core_ua.c        | 24 ++-------------
>>  drivers/target/target_core_ua.h        |  5 ++-
>>  include/target/target_core_base.h      |  1 -
>>  7 files changed, 121 insertions(+), 51 deletions(-)
>>
> 
> Applied to target-pending/for-next, with the extra incremental patch for
> a common target_ua_alloc_lun() caller.
> 
> Btw, very happy to see REPORTED_LUNS_DATA_HAS_CHANGED support include
> for v4.2-rc1 code.  8-)
> 
Yeah; I needed a quick testbed for my ALUA update, and thought that
tcm_loop would fit the bill.

As it turned out, not quite. Hence the patches.

BTW: the main issue I have with current lio-target is that you can
only configure it _after_ the target has been enabled.

IE if you want to add another ALUA state you have to create another
TPG, and set this to the required ALUA state.
But you can modify the TPG allegiance only _after_ the LUN has been
created and is visible to the host.
Which means that the initiator inevitably sees both states, and it's
impossible to have the LUN start off with a different than the
default ALUA state.
(This is especially important if one would want to test the READ
CAPACITY support in ALUA standby state).

Would you be okay with changing that?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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 0/6] target: Update UA handling

[Nicholas A. Bellinger]

On Wed, 2015-06-17 at 08:25 +0200, Hannes Reinecke wrote:
> On 06/17/2015 08:10 AM, Nicholas A. Bellinger wrote:
> > On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote:
> >> Hi Nic,
> >>
> >> lio-target is very minimalistic when it comes to generate UAs;
> >> primarily they are generated for persistent reservations, but
> >> generic changes tend to be ignored.
> >>
> >> This patchset updates the UA handling and generates UA for internal
> >> state changes (REPORTED LUNS DATA CHANGED, INQUIRY DATA CHANGED,
> >> and LUN RESET OCCURRED).
> >>
> >> Funnily enough this triggers some issues with the SCSI stack;
> >> I'll be sending out patches for that, too.
> >>
> >> Hannes Reinecke (6):
> >>   target_core_alua: Correct UA handling when switching states
> >>   target: Remove 'ua_nacl' pointer from se_ua structure
> >>   target: use 'se_dev_entry' when allocating UAs
> >>   target: Send UA on ALUA target port group change
> >>   target: Send UA upon LUN RESET tmr completion
> >>   target: Send UA when changing LUN inventory
> >>
> >>  drivers/target/target_core_alua.c      | 56 +++++++++++++++++++++++++---------
> >>  drivers/target/target_core_device.c    | 26 +++++++++++++++-
> >>  drivers/target/target_core_pr.c        | 31 +++++++++++++++----
> >>  drivers/target/target_core_transport.c | 29 ++++++++++++++----
> >>  drivers/target/target_core_ua.c        | 24 ++-------------
> >>  drivers/target/target_core_ua.h        |  5 ++-
> >>  include/target/target_core_base.h      |  1 -
> >>  7 files changed, 121 insertions(+), 51 deletions(-)
> >>
> > 
> > Applied to target-pending/for-next, with the extra incremental patch for
> > a common target_ua_alloc_lun() caller.
> > 
> > Btw, very happy to see REPORTED_LUNS_DATA_HAS_CHANGED support include
> > for v4.2-rc1 code.  8-)
> > 
> Yeah; I needed a quick testbed for my ALUA update, and thought that
> tcm_loop would fit the bill.
> 
> As it turned out, not quite. Hence the patches.
> 
> BTW: the main issue I have with current lio-target is that you can
> only configure it _after_ the target has been enabled.
> 
> IE if you want to add another ALUA state you have to create another
> TPG, and set this to the required ALUA state.
> But you can modify the TPG allegiance only _after_ the LUN has been
> created and is visible to the host.
> Which means that the initiator inevitably sees both states, and it's
> impossible to have the LUN start off with a different than the
> default ALUA state.
> (This is especially important if one would want to test the READ
> CAPACITY support in ALUA standby state).
> 
> Would you be okay with changing that?
> 

Sounds like a reasonable case to support.

No objections to allowing default ALUA access state to be changed, ahead
of actual se_device configfs symlink to fabric se_lun export.