| Message ID | 20200401132153.23501-1-bstroesser@ts.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target: tcmu: add missing pr attributes to passthrough backends | expand |
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,
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, > > >
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, >> >> >> >
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
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 >
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,
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(+)