Re: [PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > In preparation for supporting user provided vendor strings, add an extra > byte to t10_wwn.vendor which will ensure null string termination when an > eight byte vendor string is provided by the user. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_device.c | 6 -- > drivers/target/target_core_pscsi.c | 6 -- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 8 +++- > 4 files changed, 17 insertions(+), 7 deletions(-) > > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > spc5r17.pdf specifies: > 4.3.1 ASCII data field requirements > ASCII data fields shall contain only ASCII printable characters (i.e., > code values 20h to 7Eh) and may be terminated with one or more ASCII > null (00h) characters. > ASCII data fields described as being left-aligned shall have any > unused bytes at the end of the field (i.e., highest offset) and the > unused bytes shall be filled with ASCII space characters (20h). > > LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT > IDENTIFICATION fields in the standard INQUIRY data. However, the > PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the > T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD > Page are zero-terminated/zero-padded. > > Fix this inconsistency by using space-padding for all of the above > fields. > > Signed-off-by: David Disseldorp > Reviewed-by: Christoph Hellwig > --- > drivers/target/target_core_spc.c | 17 - > 1 file changed, 12 insertions(+), 5 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but > can be reconfigured via the vendor_id ConfigFS attribute. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_spc.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 5/7] target: add device vendor_id configfs attribute
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The vendor_id attribute will allow for the modification of the T10 > Vendor Identification string returned in inquiry responses. Its value > can be viewed and modified via the ConfigFS path at: > target/core/$backstore/$name/wwn/vendor_id > > "LIO-ORG" remains the default value, which is set when the backstore > device is enabled. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_configfs.c | 48 +++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/target/target_core_configfs.c > b/drivers/target/target_core_configfs.c > index 9f49b1afd685..fc60c4db718d 100644 > --- a/drivers/target/target_core_configfs.c > +++ b/drivers/target/target_core_configfs.c > @@ -1213,6 +1213,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item > *item) > } > Thanks for making it configurable. I know back when I was at IBM we made changes internally to support LIO-ORG. Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths > don't currently explicitly null-terminate t10_wwn.model. > Add an extra byte to the t10_wwn.model buffer and perform null string > termination in all cases. > > dev_set_t10_wwn_model_alias() continues to truncate at the same length > to avoid changing the model string for existing deployments. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_configfs.c | 8 +--- > drivers/target/target_core_device.c | 8 +--- > drivers/target/target_core_pscsi.c| 6 -- > drivers/target/target_core_spc.c | 2 +- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 3 ++- > 6 files changed, 19 insertions(+), 12 deletions(-) > > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
Re: [PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision
> On Nov 28, 2018, at 7:01 PM, David Disseldorp wrote: > > The pscsi_set_inquiry_info() codepath doesn't currently explicitly > null-terminate t10_wwn.revision. > Add an extra byte to the t10_wwn.model buffer and perform null string > termination in all cases. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_device.c | 6 -- > drivers/target/target_core_pscsi.c | 4 +++- > drivers/target/target_core_spc.c| 5 +++-- > drivers/target/target_core_stat.c | 4 ++-- > include/target/target_core_base.h | 3 ++- > 5 files changed, 14 insertions(+), 8 deletions(-) > Reviewed-by: Bryant G. Ly b...@catalogicsoftware.com
[PATCH] scsi: ibmvscsi_tgt: Remove target_wait_for_sess_cmd()
From: "Bryant G. Ly"
There is currently a bug with the driver where there is never a
call to target_sess_cmd_list_set_waiting(), it only called
target_wait_for_sess_cmd(), which basically means that the
sess_wait_list would always be empty.
Thus, list_empty(>sess_wait_list) = true,
(eg: no se_cmd I/O is quiesced, because no se_cmd in sess_wait_list),
since commit 712db3eb2c35 ("scsi: ibmvscsis: Properly deregister
target sessions") in 4.9.y code.
ibmvscsi_tgt does not remove the I_T Nexus when a VM is
active so we can fix this issue by removing the call to
target_wait_for_sess_cmd() altogether.
Signed-off-by: Bryant G. Ly
---
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index fac3773..2175e9e 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -2266,7 +2266,6 @@ static int ibmvscsis_drop_nexus(struct ibmvscsis_tport
*tport)
/*
* Release the SCSI I_T Nexus to the emulated ibmvscsis Target Port
*/
- target_wait_for_sess_cmds(se_sess);
target_remove_session(se_sess);
tport->ibmv_nexus = NULL;
kfree(nexus);
--
1.8.3.1
Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals
> On Oct 11, 2018, at 12:56 AM, Nicholas A. Bellinger
> wrote:
>
> Hello MNC & Co,
>
> On Wed, 2018-10-10 at 11:58 -0500, Mike Christie wrote:
>> On 10/09/2018 10:23 PM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger
>>>
>>> With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes
>>> no
>>> signals will be pending for task_struct executing the normal session
>>> shutdown
>>> and I/O quiesce code-path.
>>>
>
>
>
>>> diff --git a/drivers/target/target_core_transport.c
>>> b/drivers/target/target_core_transport.c
>>> index 86c0156..fc3093d2 100644
>>> --- a/drivers/target/target_core_transport.c
>>> +++ b/drivers/target/target_core_transport.c
>>> @@ -2754,7 +2754,7 @@ static void target_release_cmd_kref(struct kref *kref)
>>> if (se_sess) {
>>> spin_lock_irqsave(_sess->sess_cmd_lock, flags);
>>> list_del_init(_cmd->se_cmd_list);
>>> - if (list_empty(_sess->sess_cmd_list))
>>> + if (se_sess->sess_tearing_down &&
>>> list_empty(_sess->sess_cmd_list))
>>
>> I think there is another issue with 00d909a107 and ibmvscsi_tgt.
>>
>> The problem is that ibmvscsi_tgt never called
>> target_sess_cmd_list_set_waiting. It only called
>> target_wait_for_sess_cmds. So before 00d909a107 there was a bug in that
>> driver and target_wait_for_sess_cmds never did what was intended because
>> sess_wait_list would always be empty.
>>
>> With 00d909a107, we no longer need to call
>> target_sess_cmd_list_set_waiting to wait for outstanding commands, so
>> for ibmvscsi_tgt will now wait for commands like we wanted. However, the
>> commit added a WARN_ON that is hit if target_sess_cmd_list_set_waiting
>> is not called, so we could hit that.
>>
>> So I think we need to add a target_sess_cmd_list_set_waiting call in
>> ibmvscsi_tgt to go along with your patch chunk above and make sure we do
>> not trigger the WARN_ON.
>>
>
> Nice catch. :)
>
> With target_wait_for_sess_cmd() usage pre 00d909a107 doing a list-splice
> in target_sess_cmd_list_set_waiting(), this particular usage in
> ibmvscsi_tgt has always been list_empty(>sess_wait_list) = true
> (eg: no se_cmd I/O is quiesced, because no se_cmd in sess_wait_list)
> since commit 712db3eb in 4.9.y code.
>
> That said, ibmvscsi_tgt usage is very similar to vhost/scsi in the
> respect individual /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/
> endpoints used by VMs do not remove their I_T nexus while the VM is
> active.
>
> So AFAICT, ibmvscsi_tgt doesn't strictly need target_sess_wait_for_cmd()
> at all if this is true.
>
VMs do not remove the I_T nexus while the VM is active, so we can remove
the target_wait_for_sess_cmd() call.
-Bryant
