[PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[Bart Van Assche]

Hi Martin,

This patch series renames scsi_get_lba() into scsi_get_pos(). The name of
scsi_get_lba() is confusing since it does not return an LBA but instead the
start offset divided by 512.

Please consider this patch series for kernel v5.14.

Thanks,

Bart.

Changes compared to v1:
- Changed the "iscsi" prefix into "isci" for patch 4/7.
- Added support for READ(16) and WRITE(16) in patch 6/7.

Bart Van Assche (7):
  Introduce scsi_get_pos()
  iser: Use scsi_get_pos() instead of scsi_get_lba()
  zfcp: Use scsi_get_pos() instead of scsi_get_lba()
  isci: Use scsi_get_pos() instead of scsi_get_lba()
  qla2xxx: Use scsi_get_pos() instead of scsi_get_lba()
  ufs: Fix the tracing code
  Remove scsi_get_lba()

 drivers/infiniband/ulp/iser/iser_verbs.c |  2 +-
 drivers/s390/scsi/zfcp_fsf.c             |  2 +-
 drivers/scsi/isci/request.c              |  4 +--
 drivers/scsi/lpfc/lpfc_scsi.c            | 12 ++++-----
 drivers/scsi/qla2xxx/qla_iocb.c          |  9 +++----
 drivers/scsi/qla2xxx/qla_isr.c           |  8 +++---
 drivers/scsi/ufs/ufshcd.c                | 34 +++++++++++-------------
 include/scsi/scsi_cmnd.h                 |  2 +-
 include/trace/events/ufs.h               | 10 +++----
 9 files changed, 38 insertions(+), 45 deletions(-)

[PATCH v2 1/7] Introduce scsi_get_pos()

[Bart Van Assche]

Since scsi_get_lba() returns a sector_t value instead of the LBA, the name
of that function is confusing. Introduce an identical function
scsi_get_pos(). A later patch will remove scsi_get_lba().

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/scsi/scsi_cmnd.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index adb8df40b942..8147b1c0f265 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -290,6 +290,11 @@ static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
 	return blk_rq_pos(scmd->request);
 }
 
+static inline sector_t scsi_get_pos(struct scsi_cmnd *scmd)
+{
+	return blk_rq_pos(scmd->request);
+}
+
 static inline unsigned int scsi_prot_interval(struct scsi_cmnd *scmd)
 {
 	return scmd->device->sector_size;

[PATCH v2 2/7] iser: Use scsi_get_pos() instead of scsi_get_lba()

[Bart Van Assche]

Use scsi_get_pos() instead of scsi_get_lba() since the name of the latter
is confusing. This patch does not change any functionality.

Cc: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/iser/iser_verbs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 136f6c4492e0..c33e113eeee5 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -949,7 +949,7 @@ u8 iser_check_task_pi_status(struct iscsi_iser_task *iser_task,
 			sector_t sector_off = mr_status.sig_err.sig_err_offset;
 
 			sector_div(sector_off, sector_size + 8);
-			*sector = scsi_get_lba(iser_task->sc) + sector_off;
+			*sector = scsi_get_pos(iser_task->sc) + sector_off;
 
 			iser_err("PI error found type %d at sector %llx "
 			       "expected %x vs actual %x\n",

[PATCH v2 3/7] zfcp: Use scsi_get_pos() instead of scsi_get_lba()

[Bart Van Assche]

Use scsi_get_pos() instead of scsi_get_lba() since the name of the latter
is confusing. Additionally, use lower_32_bits() instead of open-coding it.
This patch does not change any functionality.

Cc: Benjamin Block <bblock@linux.ibm.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/s390/scsi/zfcp_fsf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 2e4804ef2fb9..ae308ef06714 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -2600,7 +2600,7 @@ int zfcp_fsf_fcp_cmnd(struct scsi_cmnd *scsi_cmnd)
 
 	if (scsi_get_prot_op(scsi_cmnd) != SCSI_PROT_NORMAL) {
 		io->data_block_length = scsi_cmnd->device->sector_size;
-		io->ref_tag_value = scsi_get_lba(scsi_cmnd) & 0xFFFFFFFF;
+		io->ref_tag_value = lower_32_bits(scsi_get_pos(scsi_cmnd));
 	}
 
 	if (zfcp_fsf_set_data_dir(scsi_cmnd, &io->data_direction))

[PATCH v2 4/7] isci: Use scsi_get_pos() instead of scsi_get_lba()

[Bart Van Assche]

Use scsi_get_pos() instead of scsi_get_lba() since the name of the latter
is confusing. Additionally, use lower_32_bits() instead of open-coding it.
This patch does not change any functionality.

Reviewed-by: Lee Duncan <lduncan@suse.com>
Cc: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/isci/request.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/isci/request.c b/drivers/scsi/isci/request.c
index e7c6cb4c1556..1c25f28385fd 100644
--- a/drivers/scsi/isci/request.c
+++ b/drivers/scsi/isci/request.c
@@ -341,7 +341,7 @@ static void scu_ssp_ireq_dif_insert(struct isci_request *ireq, u8 type, u8 op)
 	tc->reserved_E8_0 = 0;
 
 	if ((type & SCSI_PROT_DIF_TYPE1) || (type & SCSI_PROT_DIF_TYPE2))
-		tc->ref_tag_seed_gen = scsi_get_lba(scmd) & 0xffffffff;
+		tc->ref_tag_seed_gen = lower_32_bits(scsi_get_pos(scmd));
 	else if (type & SCSI_PROT_DIF_TYPE3)
 		tc->ref_tag_seed_gen = 0;
 }
@@ -369,7 +369,7 @@ static void scu_ssp_ireq_dif_strip(struct isci_request *ireq, u8 type, u8 op)
 	tc->app_tag_gen = 0;
 
 	if ((type & SCSI_PROT_DIF_TYPE1) || (type & SCSI_PROT_DIF_TYPE2))
-		tc->ref_tag_seed_verify = scsi_get_lba(scmd) & 0xffffffff;
+		tc->ref_tag_seed_verify = lower_32_bits(scsi_get_pos(scmd));
 	else if (type & SCSI_PROT_DIF_TYPE3)
 		tc->ref_tag_seed_verify = 0;
 

[PATCH v2 5/7] qla2xxx: Use scsi_get_pos() instead of scsi_get_lba()

[Bart Van Assche]

Use scsi_get_pos() instead of scsi_get_lba() since the name of the latter
is confusing. Additionally, use lower_32_bits() instead of open-coding it.
This patch does not change any functionality.

Cc: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/lpfc/lpfc_scsi.c   | 12 ++++++------
 drivers/scsi/qla2xxx/qla_iocb.c |  9 +++------
 drivers/scsi/qla2xxx/qla_isr.c  |  8 ++++----
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index eefbb9b22798..ebce57c7b742 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -2963,7 +2963,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9059 BLKGRD: Guard Tag error in cmd"
 				" 0x%x lba 0x%llx blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				(unsigned long long)scsi_get_lba(cmd),
+				(u64)scsi_get_pos(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 	}
 
@@ -2980,7 +2980,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9060 BLKGRD: Ref Tag error in cmd"
 				" 0x%x lba 0x%llx blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				(unsigned long long)scsi_get_lba(cmd),
+				(u64)scsi_get_pos(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 	}
 
@@ -2997,7 +2997,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9062 BLKGRD: App Tag error in cmd"
 				" 0x%x lba 0x%llx blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				(unsigned long long)scsi_get_lba(cmd),
+				(u64)scsi_get_pos(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 	}
 
@@ -3028,7 +3028,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 			break;
 		}
 
-		failing_sector = scsi_get_lba(cmd);
+		failing_sector = scsi_get_pos(cmd);
 		failing_sector += bghm;
 
 		/* Descriptor Information */
@@ -3041,7 +3041,7 @@ lpfc_sli4_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 				"9068 BLKGRD: Unknown error in cmd"
 				" 0x%x lba 0x%llx blk cnt 0x%x "
 				"bgstat=x%x bghm=x%x\n", cmd->cmnd[0],
-				(unsigned long long)scsi_get_lba(cmd),
+				(u64)scsi_get_pos(cmd),
 				blk_rq_sectors(cmd->request), bgstat, bghm);
 
 		/* Calcuate what type of error it was */
@@ -3174,7 +3174,7 @@ lpfc_parse_bg_err(struct lpfc_hba *phba, struct lpfc_io_buf *lpfc_cmd,
 			break;
 		}
 
-		failing_sector = scsi_get_lba(cmd);
+		failing_sector = scsi_get_pos(cmd);
 		failing_sector += bghm;
 
 		/* Descriptor Information */
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 38b5bdde2405..66e469890a7d 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -778,8 +778,7 @@ qla24xx_set_t10dif_tags(srb_t *sp, struct fw_dif_context *pkt,
 		 * No check for ql2xenablehba_err_chk, as it would be an
 		 * I/O error if hba tag generation is not done.
 		 */
-		pkt->ref_tag = cpu_to_le32((uint32_t)
-		    (0xffffffff & scsi_get_lba(cmd)));
+		pkt->ref_tag = cpu_to_le32(lower_32_bits(scsi_get_pos(cmd)));
 
 		if (!qla2x00_hba_err_chk_enabled(sp))
 			break;
@@ -799,8 +798,7 @@ qla24xx_set_t10dif_tags(srb_t *sp, struct fw_dif_context *pkt,
 		pkt->app_tag_mask[0] = 0x0;
 		pkt->app_tag_mask[1] = 0x0;
 
-		pkt->ref_tag = cpu_to_le32((uint32_t)
-		    (0xffffffff & scsi_get_lba(cmd)));
+		pkt->ref_tag = cpu_to_le32(lower_32_bits(scsi_get_pos(cmd)));
 
 		if (!qla2x00_hba_err_chk_enabled(sp))
 			break;
@@ -824,8 +822,7 @@ qla24xx_set_t10dif_tags(srb_t *sp, struct fw_dif_context *pkt,
 	 * 16 bit app tag.
 	 */
 	case SCSI_PROT_DIF_TYPE1:
-		pkt->ref_tag = cpu_to_le32((uint32_t)
-		    (0xffffffff & scsi_get_lba(cmd)));
+		pkt->ref_tag = cpu_to_le32(lower_32_bits(scsi_get_pos(cmd)));
 		pkt->app_tag = cpu_to_le16(0);
 		pkt->app_tag_mask[0] = 0x0;
 		pkt->app_tag_mask[1] = 0x0;
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 67229af4c142..d6b405d71f45 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -2632,7 +2632,7 @@ qla2x00_handle_dif_error(srb_t *sp, struct sts_entry_24xx *sts24)
 	    "DIF ERROR in cmd 0x%x lba 0x%llx act ref"
 	    " tag=0x%x, exp ref_tag=0x%x, act app tag=0x%x, exp app"
 	    " tag=0x%x, act guard=0x%x, exp guard=0x%x.\n",
-	    cmd->cmnd[0], (u64)scsi_get_lba(cmd), a_ref_tag, e_ref_tag,
+	    cmd->cmnd[0], (u64)scsi_get_pos(cmd), a_ref_tag, e_ref_tag,
 	    a_app_tag, e_app_tag, a_guard, e_guard);
 
 	/*
@@ -2644,10 +2644,10 @@ qla2x00_handle_dif_error(srb_t *sp, struct sts_entry_24xx *sts24)
 	    (scsi_get_prot_type(cmd) != SCSI_PROT_DIF_TYPE3 ||
 	     a_ref_tag == be32_to_cpu(T10_PI_REF_ESCAPE))) {
 		uint32_t blocks_done, resid;
-		sector_t lba_s = scsi_get_lba(cmd);
+		sector_t pos = scsi_get_pos(cmd);
 
 		/* 2TB boundary case covered automatically with this */
-		blocks_done = e_ref_tag - (uint32_t)lba_s + 1;
+		blocks_done = e_ref_tag - (uint32_t)pos + 1;
 
 		resid = scsi_bufflen(cmd) - (blocks_done *
 		    cmd->device->sector_size);
@@ -2677,7 +2677,7 @@ qla2x00_handle_dif_error(srb_t *sp, struct sts_entry_24xx *sts24)
 			if (k != blocks_done) {
 				ql_log(ql_log_warn, vha, 0x302f,
 				    "unexpected tag values tag:lba=%x:%llx)\n",
-				    e_ref_tag, (unsigned long long)lba_s);
+				    e_ref_tag, (u64)pos);
 				return 1;
 			}
 

[PATCH v2 6/7] ufs: Fix the tracing code

[Bart Van Assche]

Use scsi_get_pos() instead of scsi_get_lba() since the name of the latter
is confusing. Use scsi_get_pos() for all SCSI requests since all SCSI
requests have a block layer request attached and hence calling
scsi_get_pos() is allowed. Convert the scsi_get_pos() result from sector_t
into an LBA with sectors_to_logical(). Since READ(10), WRITE(10), READ(16)
and WRITE(16) all have a GROUP NUMBER field, extract the GROUP NUMBER field
for all four SCSI commands. Apply the 0x3f mask to that field since the
upper two bits are reserved. Rename the 'transfer_len' variable into
'affected_bytes' since it represents the number of bytes affected on the
storage medium instead of the size of the SCSI data buffer.

Cc: Can Guo <cang@codeaurora.org>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c  | 34 +++++++++++++++-------------------
 include/trace/events/ufs.h | 10 +++++-----
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0625da7a42ee..4f6b0e28735f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -16,6 +16,8 @@
 #include <linux/bitfield.h>
 #include <linux/blk-pm.h>
 #include <linux/blkdev.h>
+#include <scsi/scsi_cmnd.h>
+#include "../sd.h"
 #include "ufshcd.h"
 #include "ufs_quirks.h"
 #include "unipro.h"
@@ -366,7 +368,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 	u32 intr, doorbell;
 	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	struct scsi_cmnd *cmd = lrbp->cmd;
-	int transfer_len = -1;
+	int affected_bytes = -1;
 
 	if (!trace_ufshcd_command_enabled()) {
 		/* trace UPIU W/O tracing command */
@@ -378,30 +380,24 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 	if (cmd) { /* data phase exists */
 		/* trace UPIU also */
 		ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
-		opcode = cmd->cmnd[0];
-		if ((opcode == READ_10) || (opcode == WRITE_10)) {
-			/*
-			 * Currently we only fully trace read(10) and write(10)
-			 * commands
-			 */
-			if (cmd->request && cmd->request->bio)
-				lba = cmd->request->bio->bi_iter.bi_sector;
-			transfer_len = be32_to_cpu(
-				lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
-			if (opcode == WRITE_10)
-				group_id = lrbp->cmd->cmnd[6];
-		} else if (opcode == UNMAP) {
-			if (cmd->request) {
-				lba = scsi_get_lba(cmd);
-				transfer_len = blk_rq_bytes(cmd->request);
-			}
+		lba = sectors_to_logical(cmd->device, scsi_get_pos(cmd));
+		affected_bytes = blk_rq_bytes(cmd->request);
+		switch (cmd->cmnd[0]) {
+		case READ_10:
+		case WRITE_10:
+			group_id = lrbp->cmd->cmnd[6] & 0x3f;
+			break;
+		case READ_16:
+		case WRITE_16:
+			group_id = lrbp->cmd->cmnd[14] & 0x3f;
+			break;
 		}
 	}
 
 	intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
 	doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	trace_ufshcd_command(dev_name(hba->dev), str_t, tag,
-			doorbell, transfer_len, intr, lba, opcode, group_id);
+			doorbell, affected_bytes, intr, lba, opcode, group_id);
 }
 
 static void ufshcd_print_clk_freqs(struct ufs_hba *hba)
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index 1cb6f1afba0e..b36df5b20ad1 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -248,10 +248,10 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
 
 TRACE_EVENT(ufshcd_command,
 	TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t,
-		 unsigned int tag, u32 doorbell, int transfer_len, u32 intr,
+		 unsigned int tag, u32 doorbell, int affected_bytes, u32 intr,
 		 u64 lba, u8 opcode, u8 group_id),
 
-	TP_ARGS(dev_name, str_t, tag, doorbell, transfer_len,
+	TP_ARGS(dev_name, str_t, tag, doorbell, affected_bytes,
 				intr, lba, opcode, group_id),
 
 	TP_STRUCT__entry(
@@ -259,7 +259,7 @@ TRACE_EVENT(ufshcd_command,
 		__field(enum ufs_trace_str_t, str_t)
 		__field(unsigned int, tag)
 		__field(u32, doorbell)
-		__field(int, transfer_len)
+		__field(int, affected_bytes)
 		__field(u32, intr)
 		__field(u64, lba)
 		__field(u8, opcode)
@@ -271,7 +271,7 @@ TRACE_EVENT(ufshcd_command,
 		__entry->str_t = str_t;
 		__entry->tag = tag;
 		__entry->doorbell = doorbell;
-		__entry->transfer_len = transfer_len;
+		__entry->affected_bytes = affected_bytes;
 		__entry->intr = intr;
 		__entry->lba = lba;
 		__entry->opcode = opcode;
@@ -281,7 +281,7 @@ TRACE_EVENT(ufshcd_command,
 	TP_printk(
 		"%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, opcode: 0x%x (%s), group_id: 0x%x",
 		show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name),
-		__entry->tag, __entry->doorbell, __entry->transfer_len,
+		__entry->tag, __entry->doorbell, __entry->affected_bytes,
 		__entry->intr, __entry->lba, (u32)__entry->opcode,
 		str_opcode(__entry->opcode), (u32)__entry->group_id
 	)

[PATCH v2 7/7] Remove scsi_get_lba()

[Bart Van Assche]

Remove scsi_get_lba() since all callers have been converted to
scsi_get_pos().

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/scsi/scsi_cmnd.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 8147b1c0f265..f8084efa9838 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -285,11 +285,6 @@ static inline unsigned char scsi_get_prot_type(struct scsi_cmnd *scmd)
 	return scmd->prot_type;
 }
 
-static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
-{
-	return blk_rq_pos(scmd->request);
-}
-
 static inline sector_t scsi_get_pos(struct scsi_cmnd *scmd)
 {
 	return blk_rq_pos(scmd->request);

Re: [PATCH v2 2/7] iser: Use scsi_get_pos() instead of scsi_get_lba()

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[James Bottomley]

On Wed, 2021-05-12 at 13:08 -0700, Bart Van Assche wrote:
> Hi Martin,
> 
> This patch series renames scsi_get_lba() into scsi_get_pos(). The
> name of scsi_get_lba() is confusing since it does not return an LBA
> but instead the start offset divided by 512.

OK, I'll bite: given the logical block size for all drives is 512 why
is logical block address not the start offset in bytes divided by 512?

James

Re: [PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[Bart Van Assche]

On 5/12/21 3:10 PM, James Bottomley wrote:
> On Wed, 2021-05-12 at 13:08 -0700, Bart Van Assche wrote:
>> This patch series renames scsi_get_lba() into scsi_get_pos(). The
>> name of scsi_get_lba() is confusing since it does not return an LBA
>> but instead the start offset divided by 512.
> 
> OK, I'll bite: given the logical block size for all drives is 512 why
> is logical block address not the start offset in bytes divided by 512?

My understanding is that LBA = logical block address = (start offset in 
bytes) / (logical block size) and also that the Linux kernel supports 
logical block sizes between 512 bytes and 4 KiB. From drivers/scsi/sd.c 
(sector_size represents the logical block size):

	if (sector_size != 512 &&
	    sector_size != 1024 &&
	    sector_size != 2048 &&
	    sector_size != 4096) {
		sd_printk(KERN_NOTICE, sdkp,
			 "Unsupported sector size %d.\n",
			  sector_size);
		/*
		 * The user might want to re-format the drive with
		 * a supported sectorsize.  Once this happens, it
		 * would be relatively trivial to set the thing up.
		 * For this reason, we leave the thing in the table.
		 */
		sdkp->capacity = 0;
		/*
		 * set a bogus sector size so the normal read/write
		 * logic in the block layer will eventually refuse any
		 * request on this device without tripping over power
		 * of two sector size assumptions
		 */
		sector_size = 512;
	}

Thanks,

Bart.

Re: [PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[James Bottomley]

On Wed, 2021-05-12 at 15:20 -0700, Bart Van Assche wrote:
> On 5/12/21 3:10 PM, James Bottomley wrote:
> > On Wed, 2021-05-12 at 13:08 -0700, Bart Van Assche wrote:
> > > This patch series renames scsi_get_lba() into scsi_get_pos(). The
> > > name of scsi_get_lba() is confusing since it does not return an
> > > LBA but instead the start offset divided by 512.
> > 
> > OK, I'll bite: given the logical block size for all drives is 512
> > why is logical block address not the start offset in bytes divided
> > by 512?
> 
> My understanding is that LBA = logical block address = (start offset
> in  bytes) / (logical block size) and also that the Linux kernel
> supports  logical block sizes between 512 bytes and 4 KiB.

No, we support physical sector sizes up to 4k.  The logical block size
internal to the kernel and the block layer is always 512.  I can see
the utility in using consistent naming to the block layer, but I can't
see that logical block address is confusing ... especially now
manufacturers seem all to have aligned on 512 for the logical block
size even when it's usually 4k physical.

James

Re: [PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[Bart Van Assche]

On 5/12/21 4:23 PM, James Bottomley wrote:
> No, we support physical sector sizes up to 4k.  The logical block size
> internal to the kernel and the block layer is always 512.  I can see
> the utility in using consistent naming to the block layer, but I can't
> see that logical block address is confusing ... especially now
> manufacturers seem all to have aligned on 512 for the logical block
> size even when it's usually 4k physical.

Are we talking about the same? Just below the code that I included in my 
previous email there is the following line:

	blk_queue_logical_block_size(sdp->request_queue, sector_size);

where sector_size is the logical block size reported by the READ 
CAPACITY command and has a value between 512 and 4096.

At least the LIO code supports reporting logical block sizes larger than 
512 bytes. From drivers/target/target_core_sbc.c:

	put_unaligned_be32(dev->dev_attrib.block_size, &buf[8]);

block_size is configurable through configfs. From block_size_store():

	if (val != 512 && val != 1024 && val != 2048 && val != 4096) {
		pr_err("dev[%p]: Illegal value for block_device: %u"
		    " for SE device, must be 512, 1024, 2048 or 4096\n",
		    da->da_dev, val);
		return -EINVAL;
	}

Bart.

Re: [PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[James Bottomley]

On Wed, 2021-05-12 at 17:00 -0700, Bart Van Assche wrote:
> On 5/12/21 4:23 PM, James Bottomley wrote:
> > No, we support physical sector sizes up to 4k.  The logical block
> > size internal to the kernel and the block layer is always 512.  I
> > can see the utility in using consistent naming to the block layer,
> > but I can't see that logical block address is confusing ...
> > especially now manufacturers seem all to have aligned on 512 for
> > the logical block size even when it's usually 4k physical.
> 
> Are we talking about the same? Just below the code that I included in
> my previous email there is the following line:
> 
> 	blk_queue_logical_block_size(sdp->request_queue, sector_size);
> 
> where sector_size is the logical block size reported by the READ 
> CAPACITY command and has a value between 512 and 4096.

That was for devices from before the industry standardised, which are
getting harder and harder to find (In fact I'm thinking of making a NFT
out of my last 4k logical/physical disk).  But it didn't alter the fact
that the kernel internal block size is 512.

James

Re: [PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[Damien Le Moal]

On 2021/05/13 9:14, James Bottomley wrote:
> On Wed, 2021-05-12 at 17:00 -0700, Bart Van Assche wrote:
>> On 5/12/21 4:23 PM, James Bottomley wrote:
>>> No, we support physical sector sizes up to 4k.  The logical block
>>> size internal to the kernel and the block layer is always 512.  I
>>> can see the utility in using consistent naming to the block layer,
>>> but I can't see that logical block address is confusing ...
>>> especially now manufacturers seem all to have aligned on 512 for
>>> the logical block size even when it's usually 4k physical.
>>
>> Are we talking about the same? Just below the code that I included in
>> my previous email there is the following line:
>>
>> 	blk_queue_logical_block_size(sdp->request_queue, sector_size);
>>
>> where sector_size is the logical block size reported by the READ 
>> CAPACITY command and has a value between 512 and 4096.
> 
> That was for devices from before the industry standardised, which are
> getting harder and harder to find (In fact I'm thinking of making a NFT
> out of my last 4k logical/physical disk).  But it didn't alter the fact
> that the kernel internal block size is 512.

struct bio and struct request use 512B sector_t unit addressing. So does the
entire block layer, file systems device mapper etc. SAll users of block devices
use this unit. Yes, that is fixed to 512B, regardless of the characteristics of
the target device. But to avoid confusion, we never refer to this as the
"logical block size" or "block size". We use the term "sector" and reserve the
term "block" for the device layer.

The logical block size (the unit used for command addressing) may or may not be
512B (it may or may not be equal to the block layer sector size). These days,
most HDDs are 512e, that is, 512B logical block size and 4K physical block size.
Lots of SSDs are still 512/512. 4K/4K HDDs and SSDs are gaining ground and
spreading.

I agree with Bart's cleanup patches. They correct a non-standard use of the term
LBA to refer to a value using the block layer sector unit.

Bart suggested scsi_get_pos() as the new function name to solve the confusion. I
think that using scsi_get_sector() as a name would be even clearer about the
unit of the values being handled.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[James Bottomley]

On Thu, 2021-05-13 at 02:18 +0000, Damien Le Moal wrote:
> On 2021/05/13 9:14, James Bottomley wrote:
> > On Wed, 2021-05-12 at 17:00 -0700, Bart Van Assche wrote:
> > > On 5/12/21 4:23 PM, James Bottomley wrote:
> > > > No, we support physical sector sizes up to 4k.  The logical
> > > > block size internal to the kernel and the block layer is always
> > > > 512.  I can see the utility in using consistent naming to the
> > > > block layer, but I can't see that logical block address is
> > > > confusing ... especially now manufacturers seem all to have
> > > > aligned on 512 for the logical block size even when it's
> > > > usually 4k physical.
> > > 
> > > Are we talking about the same? Just below the code that I
> > > included in my previous email there is the following line:
> > > 
> > > 	blk_queue_logical_block_size(sdp->request_queue, sector_size);
> > > 
> > > where sector_size is the logical block size reported by the READ 
> > > CAPACITY command and has a value between 512 and 4096.
> > 
> > That was for devices from before the industry standardised, which
> > are getting harder and harder to find (In fact I'm thinking of
> > making a NFT out of my last 4k logical/physical disk).  But it
> > didn't alter the fact that the kernel internal block size is 512.
> 
> struct bio and struct request use 512B sector_t unit addressing. So
> does the entire block layer, file systems device mapper etc. SAll
> users of block devices use this unit. Yes, that is fixed to 512B,
> regardless of the characteristics of the target device. But to avoid
> confusion, we never refer to this as the "logical block size" or
> "block size". We use the term "sector" and reserve the term "block"
> for the device layer.

Doing a git grep -iw lba in block will refute this.  I think the
partition code still uses it because it's what most standards still
say.

> The logical block size (the unit used for command addressing) may or
> may not be 512B (it may or may not be equal to the block layer sector
> size). These days, most HDDs are 512e, that is, 512B logical block
> size and 4K physical block size. Lots of SSDs are still 512/512.
> 4K/4K HDDs and SSDs are gaining ground and spreading.
> 
> I agree with Bart's cleanup patches. They correct a non-standard use
> of the term LBA to refer to a value using the block layer sector
> unit.  Bart suggested scsi_get_pos() as the new function name to
> solve the confusion. I think that using scsi_get_sector() as a name
> would be even clearer about the unit of the values being handled.

To be clear, I think that using _pos everywhere is at least consistent,
even if I think it's not very logical, so I'm happy on that basis.  I'm
just not happy with the attempt to characterise LBA as confusing since
it's been the terminology forever and still permeates at least the
partition code in block and predates the logical/physical addition to
the SCSI standards.  Just say that for consistency we'd like to use
_pos everywhere ... or if you want to use _sector, that's OK, but then
update block as well.

Historically, logical meant our internal sector size, i.e. 512 and
physical meant whatever the device returned until the SCSI committee
suddenly wanted their own versions of logical and physical to cover for
the 4k block size fiasco.

James

Re: [PATCH v2 0/7] Rename scsi_get_lba() into scsi_get_pos()

[Damien Le Moal]

On 2021/05/13 14:42, James Bottomley wrote:
> On Thu, 2021-05-13 at 02:18 +0000, Damien Le Moal wrote:
>> On 2021/05/13 9:14, James Bottomley wrote:
>>> On Wed, 2021-05-12 at 17:00 -0700, Bart Van Assche wrote:
>>>> On 5/12/21 4:23 PM, James Bottomley wrote:
>>>>> No, we support physical sector sizes up to 4k.  The logical
>>>>> block size internal to the kernel and the block layer is always
>>>>> 512.  I can see the utility in using consistent naming to the
>>>>> block layer, but I can't see that logical block address is
>>>>> confusing ... especially now manufacturers seem all to have
>>>>> aligned on 512 for the logical block size even when it's
>>>>> usually 4k physical.
>>>>
>>>> Are we talking about the same? Just below the code that I
>>>> included in my previous email there is the following line:
>>>>
>>>> 	blk_queue_logical_block_size(sdp->request_queue, sector_size);
>>>>
>>>> where sector_size is the logical block size reported by the READ 
>>>> CAPACITY command and has a value between 512 and 4096.
>>>
>>> That was for devices from before the industry standardised, which
>>> are getting harder and harder to find (In fact I'm thinking of
>>> making a NFT out of my last 4k logical/physical disk).  But it
>>> didn't alter the fact that the kernel internal block size is 512.
>>
>> struct bio and struct request use 512B sector_t unit addressing. So
>> does the entire block layer, file systems device mapper etc. SAll
>> users of block devices use this unit. Yes, that is fixed to 512B,
>> regardless of the characteristics of the target device. But to avoid
>> confusion, we never refer to this as the "logical block size" or
>> "block size". We use the term "sector" and reserve the term "block"
>> for the device layer.
> 
> Doing a git grep -iw lba in block will refute this.  I think the
> partition code still uses it because it's what most standards still
> say.
> 
>> The logical block size (the unit used for command addressing) may or
>> may not be 512B (it may or may not be equal to the block layer sector
>> size). These days, most HDDs are 512e, that is, 512B logical block
>> size and 4K physical block size. Lots of SSDs are still 512/512.
>> 4K/4K HDDs and SSDs are gaining ground and spreading.
>>
>> I agree with Bart's cleanup patches. They correct a non-standard use
>> of the term LBA to refer to a value using the block layer sector
>> unit.  Bart suggested scsi_get_pos() as the new function name to
>> solve the confusion. I think that using scsi_get_sector() as a name
>> would be even clearer about the unit of the values being handled.
> 
> To be clear, I think that using _pos everywhere is at least consistent,
> even if I think it's not very logical, so I'm happy on that basis.  I'm
> just not happy with the attempt to characterise LBA as confusing since
> it's been the terminology forever and still permeates at least the
> partition code in block and predates the logical/physical addition to
> the SCSI standards.  Just say that for consistency we'd like to use
> _pos everywhere ... or if you want to use _sector, that's OK, but then
> update block as well.

Very good point. The block layer (despite its name), should refer to "sector"
and not to logical blocks to be consistent. That said, the partition table code
is probably an exception since the values in there really are LBAs and not 512B
block layer sectors. The code though should make it clear which unit is being
used. Will have a look to see if some cleanup is needed.

Thanks !


-- 
Damien Le Moal
Western Digital Research