[PATCH 18/21] scsi: sd: Support reading atomic properties from block limits VPD

John Garry posted 21 patches 2 years, 4 months ago
There is a newer version of this series
[PATCH 18/21] scsi: sd: Support reading atomic properties from block limits VPD
Posted by John Garry 2 years, 4 months ago
Also update block layer request queue sysfs properties.

See sbc4r22 section 6.6.4 - Block limits VPD page.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/sd.c | 37 ++++++++++++++++++++++++++++++++++++-
 drivers/scsi/sd.h |  7 +++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c92a317ba547..7f6cadd1f8f3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -837,6 +837,33 @@ static blk_status_t sd_setup_unmap_cmnd(struct scsi_cmnd *cmd)
 	return scsi_alloc_sgtables(cmd);
 }
 
+static void sd_config_atomic(struct scsi_disk *sdkp)
+{
+	unsigned int logical_block_size = sdkp->device->sector_size;
+	struct request_queue *q = sdkp->disk->queue;
+
+	if (sdkp->max_atomic) {
+		unsigned int physical_block_size_sectors =
+			sdkp->physical_block_size / sdkp->device->sector_size;
+		unsigned int max_atomic = max_t(unsigned int,
+			rounddown_pow_of_two(sdkp->max_atomic),
+			rounddown_pow_of_two(sdkp->max_atomic_with_boundary));
+		unsigned int unit_min = sdkp->atomic_granularity ?
+			rounddown_pow_of_two(sdkp->atomic_granularity) :
+			physical_block_size_sectors;
+		unsigned int unit_max = max_atomic;
+
+		if (sdkp->max_atomic_boundary)
+			unit_max = min_t(unsigned int, unit_max,
+				rounddown_pow_of_two(sdkp->max_atomic_boundary));
+
+		blk_queue_atomic_write_max_bytes(q, max_atomic * logical_block_size);
+		blk_queue_atomic_write_unit_min_sectors(q, unit_min);
+		blk_queue_atomic_write_unit_max_sectors(q, unit_max);
+		blk_queue_atomic_write_boundary_bytes(q, 0);
+	}
+}
+
 static blk_status_t sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd,
 		bool unmap)
 {
@@ -2982,7 +3009,7 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 		sdkp->max_ws_blocks = (u32)get_unaligned_be64(&vpd->data[36]);
 
 		if (!sdkp->lbpme)
-			goto out;
+			goto read_atomics;
 
 		lba_count = get_unaligned_be32(&vpd->data[20]);
 		desc_count = get_unaligned_be32(&vpd->data[24]);
@@ -3013,6 +3040,14 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
 			else
 				sd_config_discard(sdkp, SD_LBP_DISABLE);
 		}
+read_atomics:
+		sdkp->max_atomic = get_unaligned_be32(&vpd->data[44]);
+		sdkp->atomic_alignment  = get_unaligned_be32(&vpd->data[48]);
+		sdkp->atomic_granularity  = get_unaligned_be32(&vpd->data[52]);
+		sdkp->max_atomic_with_boundary  = get_unaligned_be32(&vpd->data[56]);
+		sdkp->max_atomic_boundary = get_unaligned_be32(&vpd->data[60]);
+
+		sd_config_atomic(sdkp);
 	}
 
  out:
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 5eea762f84d1..bca05fbd74df 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -121,6 +121,13 @@ struct scsi_disk {
 	u32		max_unmap_blocks;
 	u32		unmap_granularity;
 	u32		unmap_alignment;
+
+	u32		max_atomic;
+	u32		atomic_alignment;
+	u32		atomic_granularity;
+	u32		max_atomic_with_boundary;
+	u32		max_atomic_boundary;
+
 	u32		index;
 	unsigned int	physical_block_size;
 	unsigned int	max_medium_access_timeouts;
-- 
2.31.1
Re: [PATCH 18/21] scsi: sd: Support reading atomic properties from block limits VPD
Posted by Bart Van Assche 2 years, 4 months ago
On 9/29/23 03:27, John Garry wrote:
> +static void sd_config_atomic(struct scsi_disk *sdkp)
> +{
> +	unsigned int logical_block_size = sdkp->device->sector_size;
> +	struct request_queue *q = sdkp->disk->queue;
> +
> +	if (sdkp->max_atomic) {

Please use the "return early" style here to keep the indentation
level in this function low.

> +		unsigned int max_atomic = max_t(unsigned int,
> +			rounddown_pow_of_two(sdkp->max_atomic),
> +			rounddown_pow_of_two(sdkp->max_atomic_with_boundary));
> +		unsigned int unit_min = sdkp->atomic_granularity ?
> +			rounddown_pow_of_two(sdkp->atomic_granularity) :
> +			physical_block_size_sectors;
> +		unsigned int unit_max = max_atomic;
> +
> +		if (sdkp->max_atomic_boundary)
> +			unit_max = min_t(unsigned int, unit_max,
> +				rounddown_pow_of_two(sdkp->max_atomic_boundary));

Why does "rounddown_pow_of_two()" occur in the above code?

Thanks,

Bart.
Re: [PATCH 18/21] scsi: sd: Support reading atomic properties from block limits VPD
Posted by John Garry 2 years, 4 months ago
On 29/09/2023 18:54, Bart Van Assche wrote:
> On 9/29/23 03:27, John Garry wrote:
>> +static void sd_config_atomic(struct scsi_disk *sdkp)
>> +{
>> +    unsigned int logical_block_size = sdkp->device->sector_size;
>> +    struct request_queue *q = sdkp->disk->queue;
>> +
>> +    if (sdkp->max_atomic) {
> 
> Please use the "return early" style here to keep the indentation
> level in this function low.

ok, fine.

> 
>> +        unsigned int max_atomic = max_t(unsigned int,
>> +            rounddown_pow_of_two(sdkp->max_atomic),
>> +            rounddown_pow_of_two(sdkp->max_atomic_with_boundary));
>> +        unsigned int unit_min = sdkp->atomic_granularity ?
>> +            rounddown_pow_of_two(sdkp->atomic_granularity) :
>> +            physical_block_size_sectors;
>> +        unsigned int unit_max = max_atomic;
>> +
>> +        if (sdkp->max_atomic_boundary)
>> +            unit_max = min_t(unsigned int, unit_max,
>> +                rounddown_pow_of_two(sdkp->max_atomic_boundary));
> 
> Why does "rounddown_pow_of_two()" occur in the above code?

I assume that you are talking about all the code above to calculate 
atomic write values for the device.

The reason is that atomic write unit min and max are always a power-of-2 
- see rules described earlier - as so that we why we rounddown to a 
power-of-2.

Thanks,
John

Re: [PATCH 18/21] scsi: sd: Support reading atomic properties from block limits VPD
Posted by Bart Van Assche 2 years, 4 months ago
On 10/2/23 04:27, John Garry wrote:
> On 29/09/2023 18:54, Bart Van Assche wrote:
>> On 9/29/23 03:27, John Garry wrote:
>>> +static void sd_config_atomic(struct scsi_disk *sdkp)
>>> +{
>>> +    unsigned int logical_block_size = sdkp->device->sector_size;
>>> +    struct request_queue *q = sdkp->disk->queue;
>>> +
>>> +    if (sdkp->max_atomic) {
>>
>> Please use the "return early" style here to keep the indentation
>> level in this function low.
> 
> ok, fine.
> 
>>
>>> +        unsigned int max_atomic = max_t(unsigned int,
>>> +            rounddown_pow_of_two(sdkp->max_atomic),
>>> +            rounddown_pow_of_two(sdkp->max_atomic_with_boundary));
>>> +        unsigned int unit_min = sdkp->atomic_granularity ?
>>> +            rounddown_pow_of_two(sdkp->atomic_granularity) :
>>> +            physical_block_size_sectors;
>>> +        unsigned int unit_max = max_atomic;
>>> +
>>> +        if (sdkp->max_atomic_boundary)
>>> +            unit_max = min_t(unsigned int, unit_max,
>>> +                rounddown_pow_of_two(sdkp->max_atomic_boundary));
>>
>> Why does "rounddown_pow_of_two()" occur in the above code?
> 
> I assume that you are talking about all the code above to calculate 
> atomic write values for the device.
> 
> The reason is that atomic write unit min and max are always a power-of-2 
> - see rules described earlier - as so that we why we rounddown to a 
> power-of-2.

 From SBC-5: "The ATOMIC ALIGNMENT field indicates the required alignment
of the starting LBA in an atomic write command. If the ATOMIC ALIGNMENT
field is set to 0000_0000h, then there is no alignment requirement for
atomic write commands.

The ATOMIC TRANSFER LENGTH GRANULARITY field indicates the minimum
transfer length for an atomic write command. Atomic write operations are
required to have a transfer length that is a multiple of the atomic
transfer length granularity. An ATOMIC TRANSFER LENGTH GRANULARITY field
set to 0000_0000h indicates that there is no atomic transfer length
granularity requirement."

I think the above means that it is wrong to round down the ATOMIC
TRANSFER LENGTH GRANULARITY or the ATOMIC BOUNDARY values.

Thanks,

Bart.

Re: [PATCH 18/21] scsi: sd: Support reading atomic properties from block limits VPD
Posted by Martin K. Petersen 2 years, 4 months ago
Bart,

> I think the above means that it is wrong to round down the ATOMIC
> TRANSFER LENGTH GRANULARITY or the ATOMIC BOUNDARY values.

It was a deliberate design choice to facilitate supporting MAM. John
will look into relaxing the constraints.

-- 
Martin K. Petersen	Oracle Linux Engineering