Re: [RESEND] target: add virtual remote target

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux