| Message ID | 20230929102726.2985188-19-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | block atomic writes | expand |
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.
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
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.
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.
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;
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(-)