Re: [RFC PATCH] target: add emulate_pr backstore attr to toggle PR support

[David Disseldorp <ddiss@suse.de>]

On Fri, 1 Jun 2018 11:22:50 -0500, Mike Christie wrote:

> On 05/31/2018 07:05 PM, David Disseldorp wrote:
> > The new emulate_pr backstore attribute allows for Persistent Reservation
> > and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be
> > useful for scenarios such as:
> > - Enforcing ATS (Compare & Write) usage on recent VMware ESXi initiators  
> 
> I was wondering why didn't you want your patch to be able to override
> the passthrough case like for pscsi when TRANSPORT_FLAG_PASSTHROUGH_PGR
> is set? It seems like for this ATS case you would want to do that for
> all backends.

Agreed, I'll fix the passthrough case in the next revision.

> For the ATS case would you ever need to separate the SCSI 2
> RESERVE/RELEASE case from the modern PR case (a emulate_scsi2_pr and
> emulate_pr)? For example would you ever be using a device for ESX, want
> to do ATS only, but have datastores on the same device that are exported
> to VMs with apps that are going to use PRs so you would need
> (emulate_scsi2_prúlse, emulate_pr=true)?

I considered more granular toggles, but decided against it due to the
added configuration/implementation complexity. Happy to revisit this if
others would prefer it though.
Perhaps emulate_reservations would make more sense as a single toggle,
given that RESERVE/RELEASE aren't "persistent", although the existing
configfs paths seem to use "pr" for both types.

> > - Allowing clustered (e.g. tcm-user) backends to block such requests,
> >   avoiding the multi-node reservation state propagation.
> > 
> > When explicitly disabled, PR and RESERVE/RELEASE requests receive
> > Invalid Command Operation Code response sense data.
> > 
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > ---
> >  drivers/target/target_core_configfs.c | 21 ++++++++++++++++-----
> >  drivers/target/target_core_device.c   |  1 +
> >  drivers/target/target_core_pr.c       | 22 ++++++++++++++++++++++
> >  include/target/target_core_base.h     |  3 +++
> >  4 files changed, 42 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> > index 3f4bf126eed0..17b830e35a08 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > @@ -530,6 +530,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
> >  DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
> >  DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
> >  DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
> > +DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
> >  DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
> >  DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
> >  DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);
> > @@ -590,6 +591,7 @@ static ssize_t _name##_store(struct config_item *item, const char *page,	\
> >  DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
> >  DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
> >  DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
> > +DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
> >  DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
> >  DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);
> >  
> > @@ -1114,6 +1116,7 @@ CONFIGFS_ATTR(, emulate_tpu);
> >  CONFIGFS_ATTR(, emulate_tpws);
> >  CONFIGFS_ATTR(, emulate_caw);
> >  CONFIGFS_ATTR(, emulate_3pc);
> > +CONFIGFS_ATTR(, emulate_pr);
> >  CONFIGFS_ATTR(, pi_prot_type);
> >  CONFIGFS_ATTR_RO(, hw_pi_prot_type);
> >  CONFIGFS_ATTR(, pi_prot_format);
> > @@ -1154,6 +1157,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
> >  	&attr_emulate_tpws,
> >  	&attr_emulate_caw,
> >  	&attr_emulate_3pc,
> > +	&attr_emulate_pr,
> >  	&attr_pi_prot_type,
> >  	&attr_hw_pi_prot_type,
> >  	&attr_pi_prot_format,
> > @@ -1428,6 +1432,9 @@ static ssize_t target_pr_res_holder_show(struct config_item *item, char *page)
> >  	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
> >  		return sprintf(page, "Passthrough\n");
> >  
> > +	if (!dev->dev_attrib.emulate_pr)
> > +		return sprintf(page, "Reservations Disabled\n");
> > +
> >  	spin_lock(&dev->dev_reservation_lock);
> >  	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> >  		ret = target_core_dev_pr_show_spc2_res(dev, page);
> > @@ -1567,10 +1574,12 @@ static ssize_t target_pr_res_type_show(struct config_item *item, char *page)
> >  
> >  	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
> >  		return sprintf(page, "SPC_PASSTHROUGH\n");
> > -	else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> > +	if (!dev->dev_attrib.emulate_pr)
> > +		return sprintf(page, "Reservations Disabled\n");  
> 
> Do we want this formatted like the other strings here in caps with no
> spaces between words incase parsers are expecting that format for this file?

AFAICT, targetcli (via rtslib-fb) only makes use of the
res_aptpl_metadata path. I'm not aware of any other consumers, so happy
to go with whatever the community preference is here.

> > +	if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
> >  		return sprintf(page, "SPC2_RESERVATIONS\n");
> > -	else
> > -		return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
> > +
> > +	return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
> >  }
> >  
> >  static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
> > @@ -1578,7 +1587,8 @@ static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
> >  {
> >  	struct se_device *dev = pr_to_dev(item);
> >  
> > -	if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
> > +	if ((dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)
> > +						|| !dev->dev_attrib.emulate_pr)  
> 
> We normally put the "||" on the line above it and the next line starts
> at the column after the opening "(" above it. I guess we sometimes tab
> over too, but we never tab way way over like above.
> 
> Same below.

Will fix these in the next round. Thanks for the feedback, Mike.

Cheers, David