From: Dmitry Bogdanov <d.bogdanov@yadro.com>
To: Mike Christie <michael.christie@oracle.com>
Cc: Martin Petersen <martin.petersen@oracle.com>,
<target-devel@vger.kernel.org>, <linux-scsi@vger.kernel.org>,
<linux@yadro.com>, Konstantin Shelekhin <k.shelekhin@yadro.com>
Subject: Re: [RESEND] target: add virtual remote target
Date: Fri, 3 Feb 2023 18:40:06 +0300 [thread overview]
Message-ID: <20230203154006.GM31614@yadro.com> (raw)
In-Reply-To: <221bc4d0-082d-e313-a11b-4d0eabed002f@oracle.com>
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
prev parent reply other threads:[~2023-02-03 15:40 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-02 12:23 [RESEND] target: add virtual remote target Dmitry Bogdanov
2023-01-31 19:09 ` Mike Christie
2023-02-01 8:48 ` Dmitry Bogdanov
2023-02-01 17:00 ` Mike Christie
2023-02-03 15:40 ` Dmitry Bogdanov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230203154006.GM31614@yadro.com \
--to=d.bogdanov@yadro.com \
--cc=k.shelekhin@yadro.com \
--cc=linux-scsi@vger.kernel.org \
--cc=linux@yadro.com \
--cc=martin.petersen@oracle.com \
--cc=michael.christie@oracle.com \
--cc=target-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).