2023-09-29 14:17:40

by John Garry

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

Also update block layer request queue sysfs properties.

See sbc4r22 section 6.6.4 - Block limits VPD page.

Signed-off-by: John Garry <[email protected]>
---
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


2023-09-29 18:12:46

by Bart Van Assche

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

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.

2023-10-02 11:32:06

by John Garry

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

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

2023-10-06 17:53:08

by Bart Van Assche

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

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.

2023-10-06 23:49:47

by Martin K. Petersen

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


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