| Message ID | 20220906074903.18755-1-d.bogdanov@yadro.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: target: core: Add a way to hide a port group | expand |
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
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.
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?
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"?
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
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;