| Message ID | 20210513223757.3938-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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 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...
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;
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(+)