[PATCH] target: tcmu: add missing pr attributes to passthrough backends

[PATCH] target: tcmu: add missing pr attributes to passthrough backends

[Bodo Stroesser]

In commit b49d6f7885306ee636d5c1af52170f3069ccf5f7 the new attribute
emulate_pr was added.
passthrough_parse_cdb() uses the attribute's value to distinguish,
whether reservation commands should be rejected or not.
But the new attribute was not added to passthrough_attrib_attrs, so in
pscsi and tcmu - the users of passthrough_parse_cdb() - the attribute
is not available to change parser's behavior.
This patch adds the new attribute as well as the "pr control" attributes
enforce_pr_isids and force_pr_aptpl to passthrough_attrib_attrs.

Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
---
 drivers/target/target_core_configfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index ff82b21fdcce..c5a974c5b31d 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1203,6 +1203,9 @@ struct configfs_attribute *passthrough_attrib_attrs[] = {
 	&attr_hw_block_size,
 	&attr_hw_max_sectors,
 	&attr_hw_queue_depth,
+	&attr_emulate_pr,
+	&attr_enforce_pr_isids,
+	&attr_force_pr_aptpl,
 	&attr_alua_support,
 	&attr_pgr_support,
 	NULL,
-- 
2.12.3

Re: [PATCH] target: tcmu: add missing pr attributes to passthrough backends

[Michael Christie]

On 04/01/2020 08:21 AM, Bodo Stroesser wrote:
> In commit b49d6f7885306ee636d5c1af52170f3069ccf5f7 the new attribute> emulate_pr was added.
> passthrough_parse_cdb() uses the attribute's value to distinguish,
> whether reservation commands should be rejected or not.
> But the new attribute was not added to passthrough_attrib_attrs, so in
> pscsi and tcmu - the users of passthrough_parse_cdb() - the attribute
> is not available to change parser's behavior.
> This patch adds the new attribute as well as the "pr control" attributes
> enforce_pr_isids and force_pr_aptpl to passthrough_attrib_attrs.
> 
> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> ---
>  drivers/target/target_core_configfs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index ff82b21fdcce..c5a974c5b31d 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1203,6 +1203,9 @@ struct configfs_attribute *passthrough_attrib_attrs[] = {
>  	&attr_hw_block_size,
>  	&attr_hw_max_sectors,
>  	&attr_hw_queue_depth,
> +	&attr_emulate_pr,

This one seems ok here, because it works for both pscsi and tcmu.

> +	&attr_enforce_pr_isids,
> +	&attr_force_pr_aptpl,

These only work for tcmu. pscsi will do whatever the physical device
does, and we can't control it. I guess the options would be:

1. Just add them above.

2. Add them above and add some TRANSPORT_FLAG_PASSTHROUGH_PGR checks in
functions like force_pr_aptpl_store like we did for the pr functions, so
the user gets some feedback if they try to use them for pscsi. We would
have to add a isid function too.

3. Export the attrs or some common lib/helper functions to get/set the
values then target_core_user.c can setup the attrs and add it to
tcmu_attrib_attrs.


>  	&attr_alua_support,
>  	&attr_pgr_support,
>  	NULL,

Re: [PATCH] target: tcmu: add missing pr attributes to passthrough backends

[Bodo Stroesser]

On 04/01/20 22:05, Michael Christie wrote:
> On 04/01/2020 08:21 AM, Bodo Stroesser wrote:
>> In commit b49d6f7885306ee636d5c1af52170f3069ccf5f7 the new attribute> emulate_pr was added.
>> passthrough_parse_cdb() uses the attribute's value to distinguish,
>> whether reservation commands should be rejected or not.
>> But the new attribute was not added to passthrough_attrib_attrs, so in
>> pscsi and tcmu - the users of passthrough_parse_cdb() - the attribute
>> is not available to change parser's behavior.
>> This patch adds the new attribute as well as the "pr control" attributes
>> enforce_pr_isids and force_pr_aptpl to passthrough_attrib_attrs.
>>
>> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
>> ---
>>   drivers/target/target_core_configfs.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>> index ff82b21fdcce..c5a974c5b31d 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -1203,6 +1203,9 @@ struct configfs_attribute *passthrough_attrib_attrs[] = {
>>   	&attr_hw_block_size,
>>   	&attr_hw_max_sectors,
>>   	&attr_hw_queue_depth,
>> +	&attr_emulate_pr,
> 
> This one seems ok here, because it works for both pscsi and tcmu.
> 
>> +	&attr_enforce_pr_isids,
>> +	&attr_force_pr_aptpl,
> 
> These only work for tcmu. pscsi will do whatever the physical device
> does, and we can't control it. I guess the options would be:

Sorry, I forgot to add a note, that I'm preparing a further patch, that
allows to write pgr_support attribute (TRANSPORT_FLAG_PASSTHROUGH_PGR),
if the backend allows it.

We need this to be able to set TRANSPORT_FLAG_PASSTHROUGH_PGR for tcmu.

But of course it also would be an option for pscsi to allow resetting of
TRANSPORT_FLAG_PASSTHROUGH_PGR. Then the pr emulation in target core can 
be used by pscsi. (And then the two other attributes become useful.)

I'm wondering anyway, whether reservation passthrough makes sense for 
pscsi? For tcmu we will also send a patch allowing to pass nexus info up 
to userland.

> 
> 1. Just add them above.

According to what I wrote above, I'd still prefer option 1. :)

Thank you,
Bodo

> 
> 2. Add them above and add some TRANSPORT_FLAG_PASSTHROUGH_PGR checks in
> functions like force_pr_aptpl_store like we did for the pr functions, so
> the user gets some feedback if they try to use them for pscsi. We would
> have to add a isid function too.
> 
> 3. Export the attrs or some common lib/helper functions to get/set the
> values then target_core_user.c can setup the attrs and add it to
> tcmu_attrib_attrs.
> 
> 
>>   	&attr_alua_support,
>>   	&attr_pgr_support,
>>   	NULL,
> 
> 
> 

Re: [PATCH] target: tcmu: add missing pr attributes to passthrough backends

[Michael Christie]

On 04/02/2020 09:11 AM, Bodo Stroesser wrote:
> 
> On 04/01/20 22:05, Michael Christie wrote:
>> On 04/01/2020 08:21 AM, Bodo Stroesser wrote:
>>> In commit b49d6f7885306ee636d5c1af52170f3069ccf5f7 the new attribute>
>>> emulate_pr was added.
>>> passthrough_parse_cdb() uses the attribute's value to distinguish,
>>> whether reservation commands should be rejected or not.
>>> But the new attribute was not added to passthrough_attrib_attrs, so in
>>> pscsi and tcmu - the users of passthrough_parse_cdb() - the attribute
>>> is not available to change parser's behavior.
>>> This patch adds the new attribute as well as the "pr control" attributes
>>> enforce_pr_isids and force_pr_aptpl to passthrough_attrib_attrs.
>>>
>>> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
>>> ---
>>>   drivers/target/target_core_configfs.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/target/target_core_configfs.c
>>> b/drivers/target/target_core_configfs.c
>>> index ff82b21fdcce..c5a974c5b31d 100644
>>> --- a/drivers/target/target_core_configfs.c
>>> +++ b/drivers/target/target_core_configfs.c
>>> @@ -1203,6 +1203,9 @@ struct configfs_attribute
>>> *passthrough_attrib_attrs[] = {
>>>       &attr_hw_block_size,
>>>       &attr_hw_max_sectors,
>>>       &attr_hw_queue_depth,
>>> +    &attr_emulate_pr,
>>
>> This one seems ok here, because it works for both pscsi and tcmu.
>>
>>> +    &attr_enforce_pr_isids,
>>> +    &attr_force_pr_aptpl,
>>
>> These only work for tcmu. pscsi will do whatever the physical device
>> does, and we can't control it. I guess the options would be:
> 
> Sorry, I forgot to add a note, that I'm preparing a further patch, that
> allows to write pgr_support attribute (TRANSPORT_FLAG_PASSTHROUGH_PGR),
> if the backend allows it.
> 
> We need this to be able to set TRANSPORT_FLAG_PASSTHROUGH_PGR for tcmu.
> 
> But of course it also would be an option for pscsi to allow resetting of
> TRANSPORT_FLAG_PASSTHROUGH_PGR. Then the pr emulation in target core can
> be used by pscsi. (And then the two other attributes become useful.)
> 
> I'm wondering anyway, whether reservation passthrough makes sense for

Yeah, I wondered the same thing. Some commands that use transport info
might not work correctly right now or apps could get confused when the
transport it sees is FC because the qla2xxx fabric driver is used to
export a pscsi LU, but the pscsi scsi_device struct is for a iSCSI
device so inquiry name and port info will look mismatched. But then for
really specific SCSI 2 reservation cases it might be working.

When I added the extra passthrough flags, I did not know how users were
using it, and I didn't want to break existing setups. I just kept the
same behavior/support incase someone had a specific setup that was working.

> pscsi? For tcmu we will also send a patch allowing to pass nexus info up
> to userland.
> 
>>
>> 1. Just add them above.
> 
> According to what I wrote above, I'd still prefer option 1. :)

I think it might be best to get them in at the same time then. If not,
we might end up where in some kernels those files do nothing and report
success, then in other kernels they actually work.


> 
> Thank you,
> Bodo
> 
>>
>> 2. Add them above and add some TRANSPORT_FLAG_PASSTHROUGH_PGR checks in
>> functions like force_pr_aptpl_store like we did for the pr functions, so
>> the user gets some feedback if they try to use them for pscsi. We would
>> have to add a isid function too.
>>
>> 3. Export the attrs or some common lib/helper functions to get/set the
>> values then target_core_user.c can setup the attrs and add it to
>> tcmu_attrib_attrs.
>>
>>
>>>       &attr_alua_support,
>>>       &attr_pgr_support,
>>>       NULL,
>>
>>
>>
> 

Re: [PATCH] target: tcmu: add missing pr attributes to passthrough backends

[David Disseldorp]

Hi,

On Wed,  1 Apr 2020 15:21:53 +0200, Bodo Stroesser wrote:

> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index ff82b21fdcce..c5a974c5b31d 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1203,6 +1203,9 @@ struct configfs_attribute *passthrough_attrib_attrs[] = {
>  	&attr_hw_block_size,
>  	&attr_hw_max_sectors,
>  	&attr_hw_queue_depth,
> +	&attr_emulate_pr,
> +	&attr_enforce_pr_isids,
> +	&attr_force_pr_aptpl,
>  	&attr_alua_support,
>  	&attr_pgr_support,
>  	NULL,

The attr_emulate_pr addition here looks fine. If you and Mike agree to
proceed with the other two attrs then it probably makes sense to add
them via a separate patch.

Cheers, David

Re: [PATCH] target: tcmu: add missing pr attributes to passthrough backends

[Bodo Stroesser]

Hi David,

On 04/03/20 01:30, David Disseldorp wrote:
> Hi,
> 
> On Wed,  1 Apr 2020 15:21:53 +0200, Bodo Stroesser wrote:
> 
>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>> index ff82b21fdcce..c5a974c5b31d 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -1203,6 +1203,9 @@ struct configfs_attribute *passthrough_attrib_attrs[] = {
>>   	&attr_hw_block_size,
>>   	&attr_hw_max_sectors,
>>   	&attr_hw_queue_depth,
>> +	&attr_emulate_pr,
>> +	&attr_enforce_pr_isids,
>> +	&attr_force_pr_aptpl,
>>   	&attr_alua_support,
>>   	&attr_pgr_support,
>>   	NULL,
> 
> The attr_emulate_pr addition here looks fine. If you and Mike agree to
> proceed with the other two attrs then it probably makes sense to add
> them via a separate patch.

Before my patch and also after it if setting of emulate_pr is not
changed tcmu uses the core's pr emulation.
So I think at least for tcmu it makes sense to add the two "pr control"
attributes here, because they are missing for the default mode of tcmu,
while the emulate_pr attribute should be added to be able to switch off
pr in total.

Of course we end up then having the "pr control" attributes for pscsi
also and they have no effect because TRANSPORT_FLAG_PASSTHROUGH_PGR
is set in pscsi. Regarding this I'll soon send a patch that converts
pgr_support and alua_support attribute from readonly to read write,
if the backend allows writing it.
I need that for full userspace emulation with tcmu, but I guess for
pscsi it would also be good to allow writing of at least pgr_support.
Writing 1 to that attribute would reset TRANSPORT_FLAG_PASSTHROUGH_PGR
and thus switch on core's pr emulation for pscsi, making
enforce_pr_isids and force_pr_aptpl useful for pscsi.

Thank you,
Bodo

> 
> Cheers, David
>