[PATCH] scsi: target: core: Add a way to hide a port group

[PATCH] scsi: target: core: Add a way to hide a port group

[Dmitry Bogdanov]

From: Roman Bolshakov <r.bolshakov@yadro.com>

Default target port group is always returned in the list of port groups,
even if the behaviour is unwanted, i.e. it has no members and
non-default port groups are primary port groups.

A new port group attribute - "hidden" can be used to hide empty port
groups with no ports in REPORT TARGET PORT GROUPS, including default
target port group:

  echo 1 > $DEVICE/alua/default_tg_pt_gp/hidden

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---

The patch is intended for 6.1/scsi-queue branch.

---
 drivers/target/target_core_alua.c     |  8 ++++++++
 drivers/target/target_core_configfs.c | 29 +++++++++++++++++++++++++++
 include/target/target_core_base.h     |  1 +
 3 files changed, 38 insertions(+)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index fb91423a4e2e..dbf9a950d01b 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -164,6 +164,8 @@ target_emulate_report_target_port_groups(struct se_cmd *cmd)
 	spin_lock(&dev->t10_alua.tg_pt_gps_lock);
 	list_for_each_entry(tg_pt_gp, &dev->t10_alua.tg_pt_gps_list,
 			tg_pt_gp_list) {
+		if (tg_pt_gp->tg_pt_gp_hidden && !tg_pt_gp->tg_pt_gp_members)
+			continue;
 		/*
 		 * Check if the Target port group and Target port descriptor list
 		 * based on tg_pt_gp_members count will fit into the response payload.
@@ -308,6 +310,12 @@ target_emulate_set_target_port_groups(struct se_cmd *cmd)
 		rc = TCM_UNSUPPORTED_SCSI_OPCODE;
 		goto out;
 	}
+	if (l_tg_pt_gp->tg_pt_gp_hidden && !tg_pt_gp->tg_pt_gp_members) {
+		spin_unlock(&l_lun->lun_tg_pt_gp_lock);
+		pr_err("Unable to change state for hidden port group with no members\n");
+		rc = TCM_INVALID_CDB_FIELD;
+		goto out;
+	}
 	valid_states = l_tg_pt_gp->tg_pt_gp_alua_supported_states;
 	rcu_read_unlock();
 
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 416514c5c7ac..7c0e42e782de 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -3029,6 +3029,33 @@ static ssize_t target_tg_pt_gp_preferred_store(struct config_item *item,
 	return core_alua_store_preferred_bit(to_tg_pt_gp(item), page, count);
 }
 
+static ssize_t target_tg_pt_gp_hidden_show(struct config_item *item,
+					   char *page)
+{
+	return sysfs_emit(page, "%d\n", to_tg_pt_gp(item)->tg_pt_gp_hidden);
+}
+
+static ssize_t target_tg_pt_gp_hidden_store(struct config_item *item,
+					    const char *page, size_t count)
+{
+	struct t10_alua_tg_pt_gp *tg_pt_gp = to_tg_pt_gp(item);
+	int tmp, ret;
+
+	ret = kstrtoint(page, 0, &tmp);
+	if (ret < 0) {
+		pr_err("Unable to extract hidden flag: %d\n", ret);
+		return ret;
+	}
+
+	if (tmp != 0 && tmp != 1) {
+		pr_err("Illegal value for hidden flag: %d\n", tmp);
+		return -EINVAL;
+	}
+	tg_pt_gp->tg_pt_gp_hidden = tmp;
+
+	return count;
+}
+
 static ssize_t target_tg_pt_gp_tg_pt_gp_id_show(struct config_item *item,
 		char *page)
 {
@@ -3115,6 +3142,7 @@ CONFIGFS_ATTR(target_tg_pt_gp_, alua_support_standby);
 CONFIGFS_ATTR(target_tg_pt_gp_, alua_support_active_optimized);
 CONFIGFS_ATTR(target_tg_pt_gp_, alua_support_active_nonoptimized);
 CONFIGFS_ATTR(target_tg_pt_gp_, alua_write_metadata);
+CONFIGFS_ATTR(target_tg_pt_gp_, hidden);
 CONFIGFS_ATTR(target_tg_pt_gp_, nonop_delay_msecs);
 CONFIGFS_ATTR(target_tg_pt_gp_, trans_delay_msecs);
 CONFIGFS_ATTR(target_tg_pt_gp_, implicit_trans_secs);
@@ -3138,6 +3166,7 @@ static struct configfs_attribute *target_core_alua_tg_pt_gp_attrs[] = {
 	&target_tg_pt_gp_attr_trans_delay_msecs,
 	&target_tg_pt_gp_attr_implicit_trans_secs,
 	&target_tg_pt_gp_attr_preferred,
+	&target_tg_pt_gp_attr_hidden,
 	&target_tg_pt_gp_attr_tg_pt_gp_id,
 	&target_tg_pt_gp_attr_members,
 	NULL,
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 8c920456edd9..28cce4ed3f0e 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -297,6 +297,7 @@ struct t10_alua_tg_pt_gp {
 	int	tg_pt_gp_trans_delay_msecs;
 	int	tg_pt_gp_implicit_trans_secs;
 	int	tg_pt_gp_pref;
+	int	tg_pt_gp_hidden;
 	int	tg_pt_gp_write_metadata;
 	u32	tg_pt_gp_members;
 	int	tg_pt_gp_alua_access_state;
-- 
2.25.1

[PATCH] scsi: target: core: Add a way to hide a port group

[Dmitry Bogdanov]

From: Roman Bolshakov <r.bolshakov@yadro.com>

Default target port group is always returned in the list of port groups,
even if the behaviour is unwanted, i.e. it has no members and
non-default port groups are primary port groups.

A new port group attribute - "hidden" can be used to hide empty port
groups with no ports in REPORT TARGET PORT GROUPS, including default
target port group:

  echo 1 > $DEVICE/alua/default_tg_pt_gp/hidden

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---

The patch is intended for 6.1/scsi-queue branch.

---
 drivers/target/target_core_alua.c     |  8 ++++++++
 drivers/target/target_core_configfs.c | 29 +++++++++++++++++++++++++++
 include/target/target_core_base.h     |  1 +
 3 files changed, 38 insertions(+)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index fb91423a4e2e..dbf9a950d01b 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -164,6 +164,8 @@ target_emulate_report_target_port_groups(struct se_cmd *cmd)
 	spin_lock(&dev->t10_alua.tg_pt_gps_lock);
 	list_for_each_entry(tg_pt_gp, &dev->t10_alua.tg_pt_gps_list,
 			tg_pt_gp_list) {
+		if (tg_pt_gp->tg_pt_gp_hidden && !tg_pt_gp->tg_pt_gp_members)
+			continue;
 		/*
 		 * Check if the Target port group and Target port descriptor list
 		 * based on tg_pt_gp_members count will fit into the response payload.
@@ -308,6 +310,12 @@ target_emulate_set_target_port_groups(struct se_cmd *cmd)
 		rc = TCM_UNSUPPORTED_SCSI_OPCODE;
 		goto out;
 	}
+	if (l_tg_pt_gp->tg_pt_gp_hidden && !tg_pt_gp->tg_pt_gp_members) {
+		spin_unlock(&l_lun->lun_tg_pt_gp_lock);
+		pr_err("Unable to change state for hidden port group with no members\n");
+		rc = TCM_INVALID_CDB_FIELD;
+		goto out;
+	}
 	valid_states = l_tg_pt_gp->tg_pt_gp_alua_supported_states;
 	rcu_read_unlock();
 
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 416514c5c7ac..7c0e42e782de 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -3029,6 +3029,33 @@ static ssize_t target_tg_pt_gp_preferred_store(struct config_item *item,
 	return core_alua_store_preferred_bit(to_tg_pt_gp(item), page, count);
 }
 
+static ssize_t target_tg_pt_gp_hidden_show(struct config_item *item,
+					   char *page)
+{
+	return sysfs_emit(page, "%d\n", to_tg_pt_gp(item)->tg_pt_gp_hidden);
+}
+
+static ssize_t target_tg_pt_gp_hidden_store(struct config_item *item,
+					    const char *page, size_t count)
+{
+	struct t10_alua_tg_pt_gp *tg_pt_gp = to_tg_pt_gp(item);
+	int tmp, ret;
+
+	ret = kstrtoint(page, 0, &tmp);
+	if (ret < 0) {
+		pr_err("Unable to extract hidden flag: %d\n", ret);
+		return ret;
+	}
+
+	if (tmp != 0 && tmp != 1) {
+		pr_err("Illegal value for hidden flag: %d\n", tmp);
+		return -EINVAL;
+	}
+	tg_pt_gp->tg_pt_gp_hidden = tmp;
+
+	return count;
+}
+
 static ssize_t target_tg_pt_gp_tg_pt_gp_id_show(struct config_item *item,
 		char *page)
 {
@@ -3115,6 +3142,7 @@ CONFIGFS_ATTR(target_tg_pt_gp_, alua_support_standby);
 CONFIGFS_ATTR(target_tg_pt_gp_, alua_support_active_optimized);
 CONFIGFS_ATTR(target_tg_pt_gp_, alua_support_active_nonoptimized);
 CONFIGFS_ATTR(target_tg_pt_gp_, alua_write_metadata);
+CONFIGFS_ATTR(target_tg_pt_gp_, hidden);
 CONFIGFS_ATTR(target_tg_pt_gp_, nonop_delay_msecs);
 CONFIGFS_ATTR(target_tg_pt_gp_, trans_delay_msecs);
 CONFIGFS_ATTR(target_tg_pt_gp_, implicit_trans_secs);
@@ -3138,6 +3166,7 @@ static struct configfs_attribute *target_core_alua_tg_pt_gp_attrs[] = {
 	&target_tg_pt_gp_attr_trans_delay_msecs,
 	&target_tg_pt_gp_attr_implicit_trans_secs,
 	&target_tg_pt_gp_attr_preferred,
+	&target_tg_pt_gp_attr_hidden,
 	&target_tg_pt_gp_attr_tg_pt_gp_id,
 	&target_tg_pt_gp_attr_members,
 	NULL,
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 8c920456edd9..28cce4ed3f0e 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -297,6 +297,7 @@ struct t10_alua_tg_pt_gp {
 	int	tg_pt_gp_trans_delay_msecs;
 	int	tg_pt_gp_implicit_trans_secs;
 	int	tg_pt_gp_pref;
+	int	tg_pt_gp_hidden;
 	int	tg_pt_gp_write_metadata;
 	u32	tg_pt_gp_members;
 	int	tg_pt_gp_alua_access_state;
-- 
2.25.1

Re: [PATCH] scsi: target: core: Add a way to hide a port group

[Mike Christie]

On 9/6/22 2:49 AM, Dmitry Bogdanov wrote:
> From: Roman Bolshakov <r.bolshakov@yadro.com>
> 
> Default target port group is always returned in the list of port groups,
> even if the behaviour is unwanted, i.e. it has no members and
> non-default port groups are primary port groups.
> 
> A new port group attribute - "hidden" can be used to hide empty port
> groups with no ports in REPORT TARGET PORT GROUPS, including default
> target port group:
> 
>   echo 1 > $DEVICE/alua/default_tg_pt_gp/hidden
> 

How about "enable"? I think that fits how we handle other objects like
targets that are setup automatically but are not yet usable (can't login
or reported in discovery commands) and devices we have setup but are not
reported in commands like REPORT_LUNs (technically you need to enable and
map them but you get the idea I'm going for).

Re: [PATCH] scsi: target: core: Add a way to hide a port group

[Dmitry Bogdanov]

On Wed, Sep 07, 2022 at 03:01:00PM -0500, Mike Christie wrote:
> 
> On 9/6/22 2:49 AM, Dmitry Bogdanov wrote:
> > From: Roman Bolshakov <r.bolshakov@yadro.com>
> >
> > Default target port group is always returned in the list of port groups,
> > even if the behaviour is unwanted, i.e. it has no members and
> > non-default port groups are primary port groups.
> >
> > A new port group attribute - "hidden" can be used to hide empty port
> > groups with no ports in REPORT TARGET PORT GROUPS, including default
> > target port group:
> >
> >   echo 1 > $DEVICE/alua/default_tg_pt_gp/hidden
> >
> 
> How about "enable"? I think that fits how we handle other objects like
> targets that are setup automatically but are not yet usable (can't login
> or reported in discovery commands) and devices we have setup but are not
> reported in commands like REPORT_LUNs (technically you need to enable and
> map them but you get the idea I'm going for).
There is already an enable semantic. It is pg_pt_gp_id field. Until it
(id) is not set the port group is treated as disabled and it is not
reported in RTPG. But the default_tg_pt_gp is enabled by default and can
not be deleted.

The patch solves the presence of non-deletable empty default_tg_pt_gp
in RTPG.
May be, a global attribute like target/core/alua/hide_emtpy_tpg would
fit better than an attribute per each port group?

I would always hide the empty default_lu_gp (not configurable) but I am
afraid that it will be considered as not backward compatible change. :(

BR,
 Dmitry 

Re: [PATCH] scsi: target: core: Add a way to hide a port group

[Konstantin Shelekhin]

On Fri, Sep 09, 2022 at 02:22:35PM +0300, Dmitry Bogdanov wrote:
> On Wed, Sep 07, 2022 at 03:01:00PM -0500, Mike Christie wrote:
> > 
> > On 9/6/22 2:49 AM, Dmitry Bogdanov wrote:
> > > From: Roman Bolshakov <r.bolshakov@yadro.com>
> > >
> > > Default target port group is always returned in the list of port groups,
> > > even if the behaviour is unwanted, i.e. it has no members and
> > > non-default port groups are primary port groups.
> > >
> > > A new port group attribute - "hidden" can be used to hide empty port
> > > groups with no ports in REPORT TARGET PORT GROUPS, including default
> > > target port group:
> > >
> > >   echo 1 > $DEVICE/alua/default_tg_pt_gp/hidden
> > >
> > 
> > How about "enable"? I think that fits how we handle other objects like
> > targets that are setup automatically but are not yet usable (can't login
> > or reported in discovery commands) and devices we have setup but are not
> > reported in commands like REPORT_LUNs (technically you need to enable and
> > map them but you get the idea I'm going for).
> There is already an enable semantic. It is pg_pt_gp_id field. Until it
> (id) is not set the port group is treated as disabled and it is not
> reported in RTPG. But the default_tg_pt_gp is enabled by default and can
> not be deleted.
> 
> The patch solves the presence of non-deletable empty default_tg_pt_gp
> in RTPG.
> May be, a global attribute like target/core/alua/hide_emtpy_tpg would
> fit better than an attribute per each port group?
> 
> I would always hide the empty default_lu_gp (not configurable) but I am
> afraid that it will be considered as not backward compatible change. :(

A module parameter perhaps? Or a CONFIG definition.

Re: [PATCH] scsi: target: core: Add a way to hide a port group

[Mike Christie]

On 9/9/22 6:22 AM, Dmitry Bogdanov wrote:
> On Wed, Sep 07, 2022 at 03:01:00PM -0500, Mike Christie wrote:
>>
>> On 9/6/22 2:49 AM, Dmitry Bogdanov wrote:
>>> From: Roman Bolshakov <r.bolshakov@yadro.com>
>>>
>>> Default target port group is always returned in the list of port groups,
>>> even if the behaviour is unwanted, i.e. it has no members and
>>> non-default port groups are primary port groups.
>>>
>>> A new port group attribute - "hidden" can be used to hide empty port
>>> groups with no ports in REPORT TARGET PORT GROUPS, including default
>>> target port group:
>>>
>>>   echo 1 > $DEVICE/alua/default_tg_pt_gp/hidden
>>>
>>
>> How about "enable"? I think that fits how we handle other objects like
>> targets that are setup automatically but are not yet usable (can't login
>> or reported in discovery commands) and devices we have setup but are not
>> reported in commands like REPORT_LUNs (technically you need to enable and
>> map them but you get the idea I'm going for).
> There is already an enable semantic. It is pg_pt_gp_id field. Until it
> (id) is not set the port group is treated as disabled and it is not

Can we just make it so userspace can set tg_pt_gp_id to a magic value and
that disables it by clearing tg_pt_gp_valid_id?

Re: [PATCH] scsi: target: core: Add a way to hide a port group

[Mike Christie]

On 9/9/22 6:32 AM, Konstantin Shelekhin wrote:
>> The patch solves the presence of non-deletable empty default_tg_pt_gp
>> in RTPG.
>> May be, a global attribute like target/core/alua/hide_emtpy_tpg would
>> fit better than an attribute per each port group?
>>
>> I would always hide the empty default_lu_gp (not configurable) but I am
>> afraid that it will be considered as not backward compatible change. 🙁
> A module parameter perhaps? Or a CONFIG definition.

For the ceph iscsi project we wanted this same behavior for a while and
we had to use distro kernels. There are probably others that need the same
thing so a kernel config option wouldn't work for them.

Module param or a global attr in target/core/alua like Dimitry mentioned
seem fine. If the new variable is set are you guys thinking that
core_tpg_add_lun would just not call target_attach_tg_pt_gp? So the variable
would be "make_default_tg_pt_gp"?

Re: [PATCH] scsi: target: core: Add a way to hide a port group

[Dmitry Bogdanov]

On Fri, Sep 09, 2022 at 12:24:51PM -0500, Mike Christie wrote:
> 
> On 9/9/22 6:32 AM, Konstantin Shelekhin wrote:
> >> The patch solves the presence of non-deletable empty default_tg_pt_gp
> >> in RTPG.
> >> May be, a global attribute like target/core/alua/hide_emtpy_tpg would
> >> fit better than an attribute per each port group?
> >>
> >> I would always hide the empty default_lu_gp (not configurable) but I am
> >> afraid that it will be considered as not backward compatible change. 🙁
> > A module parameter perhaps? Or a CONFIG definition.
> 
> For the ceph iscsi project we wanted this same behavior for a while and
> we had to use distro kernels. There are probably others that need the same
> thing so a kernel config option wouldn't work for them.
> 
> Module param or a global attr in target/core/alua like Dimitry mentioned
> seem fine. If the new variable is set are you guys thinking that
> core_tpg_add_lun would just not call target_attach_tg_pt_gp? So the variable
> would be "make_default_tg_pt_gp"?
I thought it over one more time.

1. To not report empty port group is a completely backward compatible
change becasue there is no impact on userspace at all. The only change
is in the network response.

2. SPC-4 ("5.15.2.7 Target port asymmetric access state reporting")
tells that a target MAY not provide info about port groups that do not
contain the current port through that the RTPG is received. 

So, according to SPC it is expected behaviour to not report the empty
port groups.

I will prepare new version of the patch with always skipping any empty
port group in RTPG response.

BR,
 Dmitry