| Message ID | 20230929102726.2985188-20-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | block atomic writes | expand |
On 9/29/23 03:27, John Garry wrote: > +static blk_status_t sd_setup_atomic_cmnd(struct scsi_cmnd *cmd, > + sector_t lba, unsigned int nr_blocks, > + unsigned char flags) > +{ > + cmd->cmd_len = 16; > + cmd->cmnd[0] = WRITE_ATOMIC_16; > + cmd->cmnd[1] = flags; > + put_unaligned_be64(lba, &cmd->cmnd[2]); > + cmd->cmnd[10] = 0; > + cmd->cmnd[11] = 0; > + put_unaligned_be16(nr_blocks, &cmd->cmnd[12]); > + cmd->cmnd[14] = 0; > + cmd->cmnd[15] = 0; > + > + return BLK_STS_OK; > +} Please store the 'dld' value in the GROUP NUMBER field. See e.g. sd_setup_rw16_cmnd(). > @@ -1139,6 +1156,7 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd) > unsigned int nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq)); > unsigned int mask = logical_to_sectors(sdp, 1) - 1; > bool write = rq_data_dir(rq) == WRITE; > + bool atomic_write = !!(rq->cmd_flags & REQ_ATOMIC) && write; Please leave out the superfluous "!!". Thanks, Bart.
On 29/09/2023 18:59, Bart Van Assche wrote: > On 9/29/23 03:27, John Garry wrote: >> +static blk_status_t sd_setup_atomic_cmnd(struct scsi_cmnd *cmd, >> + sector_t lba, unsigned int nr_blocks, >> + unsigned char flags) >> +{ >> + cmd->cmd_len = 16; >> + cmd->cmnd[0] = WRITE_ATOMIC_16; >> + cmd->cmnd[1] = flags; >> + put_unaligned_be64(lba, &cmd->cmnd[2]); >> + cmd->cmnd[10] = 0; >> + cmd->cmnd[11] = 0; >> + put_unaligned_be16(nr_blocks, &cmd->cmnd[12]); >> + cmd->cmnd[14] = 0; >> + cmd->cmnd[15] = 0; >> + >> + return BLK_STS_OK; >> +} > > Please store the 'dld' value in the GROUP NUMBER field. See e.g. > sd_setup_rw16_cmnd(). Are you sure that WRITE ATOMIC (16) supports dld? > >> @@ -1139,6 +1156,7 @@ static blk_status_t >> sd_setup_read_write_cmnd(struct scsi_cmnd *cmd) >> unsigned int nr_blocks = sectors_to_logical(sdp, >> blk_rq_sectors(rq)); >> unsigned int mask = logical_to_sectors(sdp, 1) - 1; >> bool write = rq_data_dir(rq) == WRITE; >> + bool atomic_write = !!(rq->cmd_flags & REQ_ATOMIC) && write; > > Please leave out the superfluous "!!". ok, fine. Thanks, John
On 10/2/23 04:36, John Garry wrote: > On 29/09/2023 18:59, Bart Van Assche wrote: >> On 9/29/23 03:27, John Garry wrote: >>> +static blk_status_t sd_setup_atomic_cmnd(struct scsi_cmnd *cmd, >>> + sector_t lba, unsigned int nr_blocks, >>> + unsigned char flags) >>> +{ >>> + cmd->cmd_len = 16; >>> + cmd->cmnd[0] = WRITE_ATOMIC_16; >>> + cmd->cmnd[1] = flags; >>> + put_unaligned_be64(lba, &cmd->cmnd[2]); >>> + cmd->cmnd[10] = 0; >>> + cmd->cmnd[11] = 0; >>> + put_unaligned_be16(nr_blocks, &cmd->cmnd[12]); >>> + cmd->cmnd[14] = 0; >>> + cmd->cmnd[15] = 0; >>> + >>> + return BLK_STS_OK; >>> +} >> >> Please store the 'dld' value in the GROUP NUMBER field. See e.g. >> sd_setup_rw16_cmnd(). > > Are you sure that WRITE ATOMIC (16) supports dld? Hi John, I was assuming that DLD would be supported by the WRITE ATOMIC(16) command. After having taken another look at the latest SBC-5 draft I see that the DLD2/DLD1/DLD0 bits are not present in the WRITE ATOMIC(16) command. So please ignore my comment above. Thanks, Bart.
diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c index 41a950075913..3e47c4472a80 100644 --- a/drivers/scsi/scsi_trace.c +++ b/drivers/scsi/scsi_trace.c @@ -325,6 +325,26 @@ scsi_trace_zbc_out(struct trace_seq *p, unsigned char *cdb, int len) return ret; } +static const char * +scsi_trace_atomic_write16_out(struct trace_seq *p, unsigned char *cdb, int len) +{ + const char *ret = trace_seq_buffer_ptr(p); + unsigned int boundary_size; + unsigned int nr_blocks; + sector_t lba; + + lba = get_unaligned_be64(&cdb[2]); + boundary_size = get_unaligned_be16(&cdb[10]); + nr_blocks = get_unaligned_be16(&cdb[12]); + + trace_seq_printf(p, "lba=%llu txlen=%u boundary_size=%u", + lba, nr_blocks, boundary_size); + + trace_seq_putc(p, 0); + + return ret; +} + static const char * scsi_trace_varlen(struct trace_seq *p, unsigned char *cdb, int len) { @@ -385,6 +405,8 @@ scsi_trace_parse_cdb(struct trace_seq *p, unsigned char *cdb, int len) return scsi_trace_zbc_in(p, cdb, len); case ZBC_OUT: return scsi_trace_zbc_out(p, cdb, len); + case WRITE_ATOMIC_16: + return scsi_trace_atomic_write16_out(p, cdb, len); default: return scsi_trace_misc(p, cdb, len); } diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 7f6cadd1f8f3..1a41656dac2d 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1129,6 +1129,23 @@ static int sd_cdl_dld(struct scsi_disk *sdkp, struct scsi_cmnd *scmd) return (hint - IOPRIO_HINT_DEV_DURATION_LIMIT_1) + 1; } +static blk_status_t sd_setup_atomic_cmnd(struct scsi_cmnd *cmd, + sector_t lba, unsigned int nr_blocks, + unsigned char flags) +{ + cmd->cmd_len = 16; + cmd->cmnd[0] = WRITE_ATOMIC_16; + cmd->cmnd[1] = flags; + put_unaligned_be64(lba, &cmd->cmnd[2]); + cmd->cmnd[10] = 0; + cmd->cmnd[11] = 0; + put_unaligned_be16(nr_blocks, &cmd->cmnd[12]); + cmd->cmnd[14] = 0; + cmd->cmnd[15] = 0; + + return BLK_STS_OK; +} + static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd) { struct request *rq = scsi_cmd_to_rq(cmd); @@ -1139,6 +1156,7 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd) unsigned int nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq)); unsigned int mask = logical_to_sectors(sdp, 1) - 1; bool write = rq_data_dir(rq) == WRITE; + bool atomic_write = !!(rq->cmd_flags & REQ_ATOMIC) && write; unsigned char protect, fua; unsigned int dld; blk_status_t ret; @@ -1200,6 +1218,8 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd) if (protect && sdkp->protection_type == T10_PI_TYPE2_PROTECTION) { ret = sd_setup_rw32_cmnd(cmd, write, lba, nr_blocks, protect | fua, dld); + } else if (atomic_write) { + ret = sd_setup_atomic_cmnd(cmd, lba, nr_blocks, protect | fua); } else if (sdp->use_16_for_rw || (nr_blocks > 0xffff)) { ret = sd_setup_rw16_cmnd(cmd, write, lba, nr_blocks, protect | fua, dld); diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h index 07d65c1f59db..833de67305b5 100644 --- a/include/scsi/scsi_proto.h +++ b/include/scsi/scsi_proto.h @@ -119,6 +119,7 @@ #define WRITE_SAME_16 0x93 #define ZBC_OUT 0x94 #define ZBC_IN 0x95 +#define WRITE_ATOMIC_16 0x9c #define SERVICE_ACTION_BIDIRECTIONAL 0x9d #define SERVICE_ACTION_IN_16 0x9e #define SERVICE_ACTION_OUT_16 0x9f diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h index 8e2d9b1b0e77..05f1945ed204 100644 --- a/include/trace/events/scsi.h +++ b/include/trace/events/scsi.h @@ -102,6 +102,7 @@ scsi_opcode_name(WRITE_32), \ scsi_opcode_name(WRITE_SAME_32), \ scsi_opcode_name(ATA_16), \ + scsi_opcode_name(WRITE_ATOMIC_16), \ scsi_opcode_name(ATA_12)) #define scsi_hostbyte_name(result) { result, #result }
Add function sd_setup_atomic_cmnd() to setup an WRITE_ATOMIC_16 CDB for when REQ_ATOMIC flag is set for the request. Also add trace info. Signed-off-by: John Garry <john.g.garry@oracle.com> --- drivers/scsi/scsi_trace.c | 22 ++++++++++++++++++++++ drivers/scsi/sd.c | 20 ++++++++++++++++++++ include/scsi/scsi_proto.h | 1 + include/trace/events/scsi.h | 1 + 4 files changed, 44 insertions(+)