On 2/1/23 02:48, Dmitry Bogdanov wrote:
> On Tue, Jan 31, 2023 at 01:09:57PM -0600, Mike Christie wrote:
>>
>> On 12/2/22 06:23, Dmitry Bogdanov wrote:
>>> +
>>> +static void tcm_remote_port_unlink(
>>> + struct se_portal_group *se_tpg,
>>> + struct se_lun *lun)
>>> +{
>>> + pr_debug("TCM_Remote_ConfigFS: Port Unlink LUN %lld Successful\n",
>>> + lun->unpacked_lun);
>>> +}
>>> +
>>> +/* End items for tcm_remote_port_cit */
>>> +
>>> +/* Start items for tcm_remote_naa_cit */
>>> +
>>> +static struct se_portal_group *tcm_remote_make_tpg(struct se_wwn *wwn,
>>> + const char *name)
>>> +{
>>
>> The patch seems ok.
>>
>> My only comments are on coding style:
>>
>> 1. I know we have a mismatch in other lio code like above where sometimes we
>> don't put any args on the first line after the "(" in tcm_remote_port_unlink
>> and sometimes we do the above. Since this is new code, could you do the more
>> standard style?
>
> Yes, I will do it.
>
>> 2. Maybe for some of these callouts where most drivers are returning the same
>> hard coded value we shouldn't make them mandatory. target_fabric_tf_ops_check
>> should just set a default callout.
>
> I see that those hardcoded values are different (0 or 1) in the drivers :)
> Most likely they can be the same, but those values are exported to sysfs
> and potentially it could break some userspace. That would add a much of
> questions.
I think you misunderstood me. I'm not saying we change any values. We just:
1. Add default callouts which are set if the driver does not have one
and/or
2. Remove the requirement for some callouts.
so drivers don't have to know about some of the LIO details and don't need
to fill the same thing for these callouts.
For example for the drivers that typically hard code values or have empty
callouts similar to the new driver then we could normally have a default
callout that LIO sets:
srp ibmv loop sbp fcoe usb vhost xen
tpg_check_demo_mode
0 1 1 1 0 1 1 1
tpg_check_demo_mode_cache
1 1 0 1 0 0 1 0
tpg_check_demo_mode_write_protect
1 0 0 0 0 0 0 0
tpg_check_prod_mode_write_protect
0 0 0 0 0 0 0 0
tpg_check_prod_mode_write_protect
0 0 0 0 0 0 0 0
tpg_get_inst_index
1 1 1 1 user 1 1 1
sess_get_index
0 0 1 0 user 0 0 0
set_default_node_attributes (only used by iscsi)
empty empty empty empty empty empty empty empty
Notes:
1. qla, iscsi and efct allow users to set some of these values so I didn't
include them as they would would always set the callout.
So you could probably have:
if (!tfo->tpg_check_demo_mode)
tfo->tpg_check_demo_mode = target_enable_feature,
/*
* This one is a weird one. I think vhost should actually be disabled
* like xen/usb/loop. I think srp/fcoe/efct should have worked like
* iscsi/qla. So just make it disabled by default below, then let
* srp/fcoe/efct/iscsi/qla/vhost set a callout to override for now so
* the behavior doesn't change.
*/
if (!tfo->tpg_check_demo_mode_cache)
tfo->tpg_check_demo_mode_cache = target_disable_feature,
if (!tfo->tpg_check_demo_mode_write_protect)
tfo->tpg_check_demo_mode_write_protect = target_disable_feature,
...
/*
* I think we want to have tcm_remote_sess_get_index use 0 like
* all the drivers but loop/fcoe, and can use the default then.
*/
if (!tfo->sess_get_index)
/* this would return 0 */
tfo->sess_get_index = target_get_def_sess_index;
if (!tfo->tpg_get_inst_index)
/* this would return 1 */
tfo->tpg_get_inst_index = target_get_def_inst_index;
...
if (!tfo->set_default_node_attributes)
tfo->set_default_node_attributes = target_set_no_attrs;
Or instead of making us always having a callout, then make it optional.
set_default_node_attributes can probably be optional because only iscsi
uses it.
> I would like to keep this patch as much as simple.
>
I think for new drivers and features it's common to add new code and
improve the infrastructure you are going to use. If you think my
suggestion does not improve the code then I can see that.
