diff mbox series

[v2] scsi: target: core: check SR field in REPORT LUNS

Message ID 20210120102700.5514-1-d.bogdanov@yadro.com
State New
Headers show
Series [v2] scsi: target: core: check SR field in REPORT LUNS | expand

Commit Message

Dmitry Bogdanov Jan. 20, 2021, 10:27 a.m. UTC 
Now REPORT LUNS for software device servers always reports all luns
regardless of SELECT REPORT field.
Add handling of that field according to SPC-4:
* accept known values,
* reject unknown values.

Changes since v1:
- unmap transport data in error handling case

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
patch to 5.11/scsi-queue

 drivers/target/target_core_spc.c | 27 +++++++++++++++++++++++++++
 include/scsi/scsi_proto.h        | 10 ++++++++++
 2 files changed, 37 insertions(+)

Comments

David Disseldorp Jan. 22, 2021, 10:42 p.m. UTC | #1 
Hi,

On Wed, 20 Jan 2021 13:27:00 +0300, Dmitry Bogdanov wrote:

> Now REPORT LUNS for software device servers always reports all luns

> regardless of SELECT REPORT field.

> Add handling of that field according to SPC-4:

> * accept known values,

> * reject unknown values.


We currently advertise SPC-3 VERSION compliance via standard INQUIRY
data, so I think we should either support SPC-3 SELECT REPORT values or
bump the VERSION field (SPC-4 behaviour is already scattered throughout
LIO).
Out of curiosity, do you know of any initiators which use this field?

Cheers, David
Roman Bolshakov Jan. 26, 2021, 9:13 a.m. UTC | #2 
On Fri, Jan 22, 2021 at 11:42:51PM +0100, David Disseldorp wrote:
> On Wed, 20 Jan 2021 13:27:00 +0300, Dmitry Bogdanov wrote:

> 

> > Now REPORT LUNS for software device servers always reports all luns

> > regardless of SELECT REPORT field.

> > Add handling of that field according to SPC-4:

> > * accept known values,

> > * reject unknown values.

> 

> We currently advertise SPC-3 VERSION compliance via standard INQUIRY

> data, so I think we should either support SPC-3 SELECT REPORT values or

> bump the VERSION field (SPC-4 behaviour is already scattered throughout

> LIO).

> Out of curiosity, do you know of any initiators which use this field?

> 


Hi David,

SELECT REPORT field can be used for vVOL (LU conglomerate) discovery and
for well-known lun listing.

The field is used by VMware ESXi:
https://support.purestorage.com/Solutions/VMware_Platform_Guide/User_Guides_for_VMware_Solutions/Virtual_Volumes_User_Guide/vVols_User_Guide%3A_Protocol_Endpoints

"PEs greatly extend the number of vVols that can be connected to an ESXi
cluster; each PE can have up to 16,383 vVols per host bound to it
simultaneously. Moreover, a new binding does not require a complete I/O
rescan. Instead, ESXi issues a REPORT_LUNS SCSI command with SELECT
REPORT to the PE to which the sub-lun is bound. The PE returns a list of
sub-lun IDs for the vVols bound to that host. In large clusters,
REPORT_LUNS is significantly faster than a full I/O rescan because it is
more precisely targeted."

The post also confirms that:
https://sourceforge.net/p/scst/mailman/message/33030432/

A few more targets that support SELECT REPORT field below.

Sun/Oracle tape library:
https://docs.oracle.com/en/storage/tape-storage/storagetek-sl150-modular-tape-library/slorm/report-luns-a0h.html#GUID-4140F40D-BD9A-495C-9A86-8BD7E91C985C

IBM Flash Storage:
https://www.ibm.com/support/pages/sites/default/files/support/ssg/ssgdocs.nsf/0/95d7115d7eb428e485257f80005cc3a7/%24FILE/FlashSystem_840_SCSI_Interface_Manual_1.2.pdf

With regards to bumping TCM to SPC-4, are there any objections if we
submit a separate patch for that? Or resubmit a series with the patch?

Thanks,
Roman
Martin K. Petersen Jan. 27, 2021, 4:36 a.m. UTC | #3 
Dmitry,

> +	switch (sr) {

> +	case SCSI_SELECT_WELLKNOWN:

> +	case SCSI_SELECT_ADMINISTRATIVE:

> +	case SCSI_SELECT_SUBSIDIARY:

> +		/* report empty lun list */

> +		goto out;


I'm a bit concerned about things inadvertently breaking if we return an
empty list for the well known LUNs.

-- 
Martin K. Petersen	Oracle Linux Engineering
Dmitry Bogdanov Jan. 27, 2021, 2:45 p.m. UTC | #4 
Hi Martin,

>> +	switch (sr) {

>> +	case SCSI_SELECT_WELLKNOWN:

>> +	case SCSI_SELECT_ADMINISTRATIVE:

>> +	case SCSI_SELECT_SUBSIDIARY:

>> +		/* report empty lun list */

>> +		goto out;


> I'm a bit concerned about things inadvertently breaking if we return an empty list for the well known LUNs.

According to SPC we shall report an empty list if there is no well-known LUNS. 
FreeBSD has the same logic in REPORT LUNS handling. SCST does not support SELECT_WELLKNOWN case at all.

I don't know the history of the existing behaviour to send always LUN0 instead of empty list. Probably it was 
for the SCSI_SELECT_ALL_ACCESSIBLE(0x02) case, where SPC allows LUN0. My patch keeps it for the 0x00, 0x02, 0x11 cases.
Thus, I believe it does not break the backward compatibility.

BR,
 Dmitry
David Disseldorp Jan. 28, 2021, 8:28 p.m. UTC | #5 
Hi Roman,

On Tue, 26 Jan 2021 12:13:44 +0300, Roman Bolshakov wrote:

> Hi David,

> 

> SELECT REPORT field can be used for vVOL (LU conglomerate) discovery and

> for well-known lun listing.

> 

> The field is used by VMware ESXi:

> https://support.purestorage.com/Solutions/VMware_Platform_Guide/User_Guides_for_VMware_Solutions/Virtual_Volumes_User_Guide/vVols_User_Guide%3A_Protocol_Endpoints

> 

> "PEs greatly extend the number of vVols that can be connected to an ESXi

> cluster; each PE can have up to 16,383 vVols per host bound to it

> simultaneously. Moreover, a new binding does not require a complete I/O

> rescan. Instead, ESXi issues a REPORT_LUNS SCSI command with SELECT

> REPORT to the PE to which the sub-lun is bound. The PE returns a list of

> sub-lun IDs for the vVols bound to that host. In large clusters,

> REPORT_LUNS is significantly faster than a full I/O rescan because it is

> more precisely targeted."


Interesting, thanks.

...
> With regards to bumping TCM to SPC-4, are there any objections if we

> submit a separate patch for that? Or resubmit a series with the patch?


I don't object to that. FWIW, the following crude metrics could be seen
as an argument in favour of SPC-4 versioning:
  linux> git grep -ic -e "SPC4" -e "SPC-4" -- drivers/target/
  drivers/target/target_core_alua.c:7
  drivers/target/target_core_alua.h:5
  drivers/target/target_core_device.c:1
  drivers/target/target_core_fabric_lib.c:5
  drivers/target/target_core_pr.c:37
  drivers/target/target_core_spc.c:19
  drivers/target/target_core_tmr.c:3
  drivers/target/target_core_transport.c:2
  drivers/target/target_core_ua.c:1
  drivers/target/target_core_ua.h:1
  drivers/target/target_core_xcopy.c:3
  drivers/target/target_core_xcopy.h:1
  linux> git grep -ic -e "SPC3" -e "SPC-3" -- drivers/target/
  drivers/target/target_core_alua.c:1
  drivers/target/target_core_configfs.c:14
  drivers/target/target_core_pr.c:75
  drivers/target/target_core_spc.c:4
  drivers/target/target_core_tmr.c:1
  drivers/target/target_core_transport.c:4
  drivers/target/target_core_ua.c:1

Most of the SPC3 target_core_pr and target_core_configfs matches above
are debug/error messages, rather than spec references.

Cheers, David
Bart Van Assche Jan. 28, 2021, 8:47 p.m. UTC | #6 
On 1/27/21 6:45 AM, Dmitriy Bogdanov wrote:
> Hi Martin,

> 

>>> +	switch (sr) {

>>> +	case SCSI_SELECT_WELLKNOWN:

>>> +	case SCSI_SELECT_ADMINISTRATIVE:

>>> +	case SCSI_SELECT_SUBSIDIARY:

>>> +		/* report empty lun list */

>>> +		goto out;

> 

>> I'm a bit concerned about things inadvertently breaking if we return an empty list for the well known LUNs.

 >

> According to SPC we shall report an empty list if there is no well-known LUNS.

> FreeBSD has the same logic in REPORT LUNS handling. SCST does not support SELECT_WELLKNOWN case at all.

> 

> I don't know the history of the existing behaviour to send always LUN0 instead of empty list. Probably it was

> for the SCSI_SELECT_ALL_ACCESSIBLE(0x02) case, where SPC allows LUN0. My patch keeps it for the 0x00, 0x02, 0x11 cases.

> Thus, I believe it does not break the backward compatibility.


Will this change require users to update their LUN configuration? Some 
initiator operating systems require presence of a dummy LUN 0. Although 
I agree that it is cleaner not to provide a hardcoded LUN 0, I think 
Martin is concerned about this patch potentially breaking existing 
configurations and causing frustration among LIO users.

Bart.
Dmitry Bogdanov Feb. 19, 2021, 1:48 p.m. UTC | #7 
Hi,

>>> I'm a bit concerned about things inadvertently breaking if we return an empty list for the well known LUNs.

> >

>> According to SPC we shall report an empty list if there is no well-known LUNS.

>> FreeBSD has the same logic in REPORT LUNS handling. SCST does not support SELECT_WELLKNOWN case at all.

>> 

>> I don't know the history of the existing behaviour to send always LUN0 instead of empty list. Probably it was

>> for the SCSI_SELECT_ALL_ACCESSIBLE(0x02) case, where SPC allows LUN0. My patch keeps it for the 0x00, 0x02, 0x11 cases.

>> Thus, I believe it does not break the backward compatibility.


>Will this change require users to update their LUN configuration? Some 

>initiator operating systems require presence of a dummy LUN 0. Although 

>I agree that it is cleaner not to provide a hardcoded LUN 0, I think 

>Martin is concerned about this patch potentially breaking existing 

>configurations and causing frustration among LIO users.


No reconfiguration on initiator side is required.
W-LUN is a specific LUN for the specific SCSI target device that is well known
for the Initiator. Generic Linux TCM does not have W-LUNs. Some storage
systems based on Linux TCM may have W-LUNs. But then they shall / already have
its own handling of REPORT LUNS command.
Basically, it is an error to report LUN0 as W-LUN for the Initiator that
expects some other numbers.

BR,
 Dmitry
diff mbox series

Patch

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index ca5579ebc81d..044ac45cdf47 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -1210,10 +1210,12 @@  sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
 {
 	struct se_dev_entry *deve;
 	struct se_session *sess = cmd->se_sess;
+	unsigned char *cdb = cmd->t_task_cdb;
 	struct se_node_acl *nacl;
 	struct scsi_lun slun;
 	unsigned char *buf;
 	u32 lun_count = 0, offset = 8;
+	u8 sr = cdb[2];
 	__be32 len;
 
 	buf = transport_kmap_data_sg(cmd);
@@ -1230,6 +1232,28 @@  sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
 
 	nacl = sess->se_node_acl;
 
+	switch (sr) {
+	case SCSI_SELECT_WELLKNOWN:
+	case SCSI_SELECT_ADMINISTRATIVE:
+	case SCSI_SELECT_SUBSIDIARY:
+		/* report empty lun list */
+		goto out;
+	case SCSI_SELECT_TOP_LEVEL:
+		if (cmd->se_lun->unpacked_lun != 0)
+			goto out;
+		fallthrough;
+	case SCSI_SELECT_REGULAR:
+	case SCSI_SELECT_ALL_ACCESSIBLE:
+		break;
+	default:
+		pr_debug("TARGET_CORE[%s]: Invalid REPORT LUNS with unsupported "
+				 "SELECT REPORT %#x for 0x%08llx from %s\n",
+				 cmd->se_tfo->fabric_name, sr, cmd->se_lun->unpacked_lun,
+				 sess->se_node_acl->initiatorname);
+		transport_kunmap_data_sg(cmd);
+		return TCM_INVALID_CDB_FIELD;
+	}
+
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(deve, &nacl->lun_entry_hlist, link) {
 		/*
@@ -1252,6 +1276,8 @@  sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
 	 * See SPC3 r07, page 159.
 	 */
 done:
+	if ((sr != SCSI_SELECT_REGULAR) && (sr != SCSI_SELECT_ALL_ACCESSIBLE))
+		goto out;
 	/*
 	 * If no LUNs are accessible, report virtual LUN 0.
 	 */
@@ -1263,6 +1289,7 @@  sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
 		lun_count = 1;
 	}
 
+out:
 	if (buf) {
 		len = cpu_to_be32(lun_count * 8);
 		memcpy(buf, &len, min_t(int, sizeof len, cmd->data_length));
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index c36860111932..280169c75d85 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -341,4 +341,14 @@  enum zbc_zone_cond {
 	ZBC_ZONE_COND_OFFLINE		= 0xf,
 };
 
+/* Select Report fot REPORT LUNS */
+enum scsi_select_report {
+	SCSI_SELECT_REGULAR		= 0x0,
+	SCSI_SELECT_WELLKNOWN		= 0x1,
+	SCSI_SELECT_ALL_ACCESSIBLE	= 0x2,
+	SCSI_SELECT_ADMINISTRATIVE	= 0x10,
+	SCSI_SELECT_TOP_LEVEL		= 0x11,
+	SCSI_SELECT_SUBSIDIARY		= 0x12,
+};
+
 #endif /* _SCSI_PROTO_H_ */