| Message ID | 20210512200849.9002-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | Rename scsi_get_lba() into scsi_get_pos() | expand |
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
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.
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
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.
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
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.
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
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 !