| Message ID | 20210513223757.3938-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | Rename scsi_get_lba() into scsi_get_sector() | expand |
On 2021/05/14 7:38, Bart Van Assche wrote: > 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_sector(). 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 bc5535b269c5..7f55faa70301 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -289,6 +289,11 @@ static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd) > return blk_rq_pos(scmd->request); > } > > +static inline sector_t scsi_get_sector(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; > Looks good. Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
On 2021/05/14 7:38, Bart Van Assche wrote: > Use scsi_get_sector() 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..3d9a3dc4975b 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_sector(scsi_cmnd)); > } > > if (zfcp_fsf_set_data_dir(scsi_cmnd, &io->data_direction)) > Looks good. Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
On 2021/05/14 7:38, Bart Van Assche wrote: > Use scsi_get_sector() instead of scsi_get_lba() since the name of the > latter is confusing. This patch does not change any functionality. > > Cc: James Smart <james.smart@broadcom.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/lpfc/lpfc_scsi.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c > index eefbb9b22798..8c6b25807f12 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_sector(cmd), Why the cast ? scsi_get_sector() returns a 64bits sector_t. > 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_sector(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_sector(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_sector(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_sector(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_sector(cmd); > failing_sector += bghm; > > /* Descriptor Information */ >
On 2021/05/14 7:38, Bart Van Assche wrote: > Remove scsi_get_lba() since all callers have been converted to > scsi_get_sector(). > > 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 7f55faa70301..77a0916c8769 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -284,11 +284,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_sector(struct scsi_cmnd *scmd) > { > return blk_rq_pos(scmd->request); > Looks good. Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
On 5/13/21 7:00 PM, Damien Le Moal wrote: > On 2021/05/14 7:38, Bart Van Assche wrote: >> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c >> index eefbb9b22798..8c6b25807f12 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_sector(cmd), > > Why the cast ? scsi_get_sector() returns a 64bits sector_t. Right, the cast can be left out. Commit 72deb455b5ec ("block: remove CONFIG_LBDAF") changed sector_t from sometimes 32-bit / sometimes 64-bit into always 64-bit. Bart.
On Thu, May 13, 2021 at 03:37:50PM -0700, Bart Van Assche wrote: > 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_sector(). A later patch will remove scsi_get_lba(). Why not just do a quick rename in a single patch instead of the hard to review series?
On 5/13/21 11:20 PM, Christoph Hellwig wrote: > On Thu, May 13, 2021 at 03:37:50PM -0700, Bart Van Assche wrote: >> 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_sector(). A later patch will remove scsi_get_lba(). > > Why not just do a quick rename in a single patch instead of the hard to > review series? Hi Christoph, Something I noticed in the past is that driver maintainers refuse to add their Reviewed-by: or Acked-by: if a patch modifies code outside the driver they maintain. Another reason I split this patch series is that I really want driver maintainers to take a look. My understanding of section "4.19 Protection Information" in SBC-4 is that the LOGICAL BLOCK REFERENCE TAG field should contain the least significant four bytes of the LBA. However, in multiple SCSI LLDs I found code similar to the following: ref_tag = cpu_to_le32(lower_32_bits(scsi_get_lba(cmd))) or in other words, the starting offset divided by 512 is assigned to the reference tag instead of the starting offset divided by the logical block size. I think that's a bug. Thanks, Bart.
Bart, > or in other words, the starting offset divided by 512 is assigned to > the reference tag instead of the starting offset divided by the > logical block size. I think that's a bug. It is. And I have most of these fixed in my integrity branch that reworks how LLDs interact with the SCSI midlayer. I've only done mpt3sas, qla2xxx, and lpfc because that's what I test with. I.e. I missed zfcp and iser. I will review your series shortly. But since all the integrity stuff is transitioning to the t10_pi_foo* interfaces, my initial hunch is that we probably just want to get rid of scsi_get_lba() completely and use blk_rq_pos() directly for the places where we want block layer addressing. I'm not a big fan of _pos() and like _sector() much better. But I do think that the blk_ prefix makes it clear that we're referring to sectors and not logical blocks, physical blocks, or protection intervals. Anyway. More in an hour or so... -- Martin K. Petersen Oracle Linux Engineering
On Thu, May 13, 2021 at 03:37:52PM -0700, Bart Van Assche wrote: > Use scsi_get_sector() 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..3d9a3dc4975b 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_sector(scsi_cmnd)); > } > > if (zfcp_fsf_set_data_dir(scsi_cmnd, &io->data_direction)) Looks good. Acked-by: Benjamin Block <bblock@linux.ibm.com> -- Best Regards, Benjamin Block / Linux on IBM Z Kernel Development / IBM Systems IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy Vorsitz. AufsR.: Gregor Pillen / Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294