On Wed, Feb 01, 2023 at 11:00:01AM -0600, Mike Christie wrote:
>
> 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
Thank you for the table! I was looking only at sess_get_index that has 1
or 0. Actually I may make sess_get_index_def return 0 and for tcm_loop
keep its own sess_get_index function with 1 to keep backward
compatibility.
>
> 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.
>
You've convinced me, I will do it.
BR,
Dmitry
