| Message ID | 20160822042321.24367-2-shaun@tancheff.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 08/22/2016 06:23 AM, Shaun Tancheff wrote: > Safely overwriting the attached page to ATA format from the SCSI formatted > variant. > > Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com> > --- > v6: > - Fix bisect bug reported by Tom Yan <tom.ty89@gmail.com> > - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de> > v5: > - Added prep patch to work with non-page aligned scatterlist pages > and use kmap_atomic() to lock page during modification. > > drivers/ata/libata-scsi.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- > include/linux/ata.h | 26 ---------------------- > 2 files changed, 51 insertions(+), 31 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On 22 August 2016 at 12:23, Shaun Tancheff <shaun@tancheff.com> wrote: > Safely overwriting the attached page to ATA format from the SCSI formatted > variant. > > Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com> > --- > v6: > - Fix bisect bug reported by Tom Yan <tom.ty89@gmail.com> > - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de> > v5: > - Added prep patch to work with non-page aligned scatterlist pages > and use kmap_atomic() to lock page during modification. > > drivers/ata/libata-scsi.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- > include/linux/ata.h | 26 ---------------------- > 2 files changed, 51 insertions(+), 31 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index e207b33..7990cb2 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -3282,6 +3282,54 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc) > return 1; > } > > +/** > + * ata_format_dsm_trim_descr() - SATL Write Same to DSM Trim > + * @cmd: SCSI command being translated > + * @num: Maximum number of entries (nominally 64). > + * @sector: Starting sector > + * @count: Total Range of request > + * > + * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted > + * descriptor. > + * > + * Upto 64 entries of the format: > + * 63:48 Range Length > + * 47:0 LBA > + * > + * Range Length of 0 is ignored. > + * LBA's should be sorted order and not overlap. > + * > + * NOTE: this is the same format as ADD LBA(S) TO NV CACHE PINNED SET > + */ > +static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num, > + u64 sector, u32 count) > +{ > + __le64 *buffer; > + u32 i = 0, used_bytes; > + unsigned long flags; > + > + BUILD_BUG_ON(512 > ATA_SCSI_RBUF_SIZE); > + > + spin_lock_irqsave(&ata_scsi_rbuf_lock, flags); > + buffer = ((void *)ata_scsi_rbuf); > + while (i < num) { > + u64 entry = sector | > + ((u64)(count > 0xffff ? 0xffff : count) << 48); > + buffer[i++] = __cpu_to_le64(entry); > + if (count <= 0xffff) > + break; > + count -= 0xffff; > + sector += 0xffff; > + } > + > + used_bytes = ALIGN(i * 8, 512); > + memset(buffer + i, 0, used_bytes - i * 8); > + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); > + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); > + > + return used_bytes; > +} > + > static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) > { > struct ata_taskfile *tf = &qc->tf; > @@ -3290,8 +3338,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) > const u8 *cdb = scmd->cmnd; > u64 block; > u32 n_block; > + const u32 trmax = ATA_MAX_TRIM_RNUM; This does not seem like a good idea. > u32 size; > - void *buf; > u16 fp; > u8 bp = 0xff; > > @@ -3319,10 +3367,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) > if (!scsi_sg_count(scmd)) > goto invalid_param_len; > > - buf = page_address(sg_page(scsi_sglist(scmd))); > - > - if (n_block <= 65535 * ATA_MAX_TRIM_RNUM) { > - size = ata_set_lba_range_entries(buf, ATA_MAX_TRIM_RNUM, block, n_block); > + if (n_block <= 0xffff * trmax) { Note that the limit here would always be the advertised Maximum Write Same Length (ata_scsiop_inq_b0). It would be best for them to look the same. Besides, it doesn't seem necessary to create this trmax anyway. > + size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block); > } else { > fp = 2; > goto invalid_fld; > diff --git a/include/linux/ata.h b/include/linux/ata.h > index adbc812..45a1d71 100644 > --- a/include/linux/ata.h > +++ b/include/linux/ata.h > @@ -1071,32 +1071,6 @@ static inline void ata_id_to_hd_driveid(u16 *id) > #endif > } > > -/* > - * Write LBA Range Entries to the buffer that will cover the extent from > - * sector to sector + count. This is used for TRIM and for ADD LBA(S) > - * TO NV CACHE PINNED SET. > - */ > -static inline unsigned ata_set_lba_range_entries(void *_buffer, > - unsigned num, u64 sector, unsigned long count) > -{ > - __le64 *buffer = _buffer; > - unsigned i = 0, used_bytes; > - > - while (i < num) { > - u64 entry = sector | > - ((u64)(count > 0xffff ? 0xffff : count) << 48); > - buffer[i++] = __cpu_to_le64(entry); > - if (count <= 0xffff) > - break; > - count -= 0xffff; > - sector += 0xffff; > - } > - > - used_bytes = ALIGN(i * 8, 512); > - memset(buffer + i, 0, used_bytes - i * 8); > - return used_bytes; > -} > - > static inline bool ata_ok(u8 status) > { > return ((status & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ | ATA_ERR)) > -- > 2.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 22, 2016 at 2:27 PM, Tom Yan <tom.ty89@gmail.com> wrote: > On 22 August 2016 at 12:23, Shaun Tancheff <shaun@tancheff.com> wrote: >> Safely overwriting the attached page to ATA format from the SCSI formatted >> variant. >> >> Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com> >> --- >> v6: >> - Fix bisect bug reported by Tom Yan <tom.ty89@gmail.com> >> - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de> >> v5: >> - Added prep patch to work with non-page aligned scatterlist pages >> and use kmap_atomic() to lock page during modification. >> >> drivers/ata/libata-scsi.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- >> include/linux/ata.h | 26 ---------------------- >> 2 files changed, 51 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index e207b33..7990cb2 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -3282,6 +3282,54 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc) >> return 1; >> } >> >> +/** >> + * ata_format_dsm_trim_descr() - SATL Write Same to DSM Trim >> + * @cmd: SCSI command being translated >> + * @num: Maximum number of entries (nominally 64). >> + * @sector: Starting sector >> + * @count: Total Range of request >> + * >> + * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted >> + * descriptor. >> + * >> + * Upto 64 entries of the format: >> + * 63:48 Range Length >> + * 47:0 LBA >> + * >> + * Range Length of 0 is ignored. >> + * LBA's should be sorted order and not overlap. >> + * >> + * NOTE: this is the same format as ADD LBA(S) TO NV CACHE PINNED SET >> + */ >> +static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num, >> + u64 sector, u32 count) >> +{ >> + __le64 *buffer; >> + u32 i = 0, used_bytes; >> + unsigned long flags; >> + >> + BUILD_BUG_ON(512 > ATA_SCSI_RBUF_SIZE); >> + >> + spin_lock_irqsave(&ata_scsi_rbuf_lock, flags); >> + buffer = ((void *)ata_scsi_rbuf); >> + while (i < num) { >> + u64 entry = sector | >> + ((u64)(count > 0xffff ? 0xffff : count) << 48); >> + buffer[i++] = __cpu_to_le64(entry); >> + if (count <= 0xffff) >> + break; >> + count -= 0xffff; >> + sector += 0xffff; >> + } >> + >> + used_bytes = ALIGN(i * 8, 512); >> + memset(buffer + i, 0, used_bytes - i * 8); >> + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); >> + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); >> + >> + return used_bytes; >> +} >> + >> static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >> { >> struct ata_taskfile *tf = &qc->tf; >> @@ -3290,8 +3338,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >> const u8 *cdb = scmd->cmnd; >> u64 block; >> u32 n_block; >> + const u32 trmax = ATA_MAX_TRIM_RNUM; > > This does not seem like a good idea. > >> u32 size; >> - void *buf; >> u16 fp; >> u8 bp = 0xff; >> >> @@ -3319,10 +3367,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >> if (!scsi_sg_count(scmd)) >> goto invalid_param_len; >> >> - buf = page_address(sg_page(scsi_sglist(scmd))); >> - >> - if (n_block <= 65535 * ATA_MAX_TRIM_RNUM) { >> - size = ata_set_lba_range_entries(buf, ATA_MAX_TRIM_RNUM, block, n_block); >> + if (n_block <= 0xffff * trmax) { > > Note that the limit here would always be the advertised Maximum Write > Same Length (ata_scsiop_inq_b0). It would be best for them to look the > same. Besides, it doesn't seem necessary to create this trmax anyway. It is entirely a style thing. I tend to prefer hex when describing an interface that specifies number of bits. The trmax is just to keep the following line under 80 chars. I am not tied to either. I can change it if people really find it confusing. >> + size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block); >> } else { >> fp = 2; >> goto invalid_fld; >> diff --git a/include/linux/ata.h b/include/linux/ata.h >> index adbc812..45a1d71 100644 >> --- a/include/linux/ata.h >> +++ b/include/linux/ata.h >> @@ -1071,32 +1071,6 @@ static inline void ata_id_to_hd_driveid(u16 *id) >> #endif >> } >> >> -/* >> - * Write LBA Range Entries to the buffer that will cover the extent from >> - * sector to sector + count. This is used for TRIM and for ADD LBA(S) >> - * TO NV CACHE PINNED SET. >> - */ >> -static inline unsigned ata_set_lba_range_entries(void *_buffer, >> - unsigned num, u64 sector, unsigned long count) >> -{ >> - __le64 *buffer = _buffer; >> - unsigned i = 0, used_bytes; >> - >> - while (i < num) { >> - u64 entry = sector | >> - ((u64)(count > 0xffff ? 0xffff : count) << 48); >> - buffer[i++] = __cpu_to_le64(entry); >> - if (count <= 0xffff) >> - break; >> - count -= 0xffff; >> - sector += 0xffff; >> - } >> - >> - used_bytes = ALIGN(i * 8, 512); >> - memset(buffer + i, 0, used_bytes - i * 8); >> - return used_bytes; >> -} >> - >> static inline bool ata_ok(u8 status) >> { >> return ((status & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ | ATA_ERR)) >> -- >> 2.9.3 >>
Oh, 0xffff is fine for me, as long as you change it in ata_scsiop_inq_b0 as well. Actually I think replacing it with a macro (as suggested by Martin in another thread) is a good idea as well. A line split would be better than introducing an unnecessary variable that costs readability in the logic sense. On 23 August 2016 at 03:51, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: > On Mon, Aug 22, 2016 at 2:27 PM, Tom Yan <tom.ty89@gmail.com> wrote: >> On 22 August 2016 at 12:23, Shaun Tancheff <shaun@tancheff.com> wrote: >>> Safely overwriting the attached page to ATA format from the SCSI formatted >>> variant. >>> >>> Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com> >>> --- >>> v6: >>> - Fix bisect bug reported by Tom Yan <tom.ty89@gmail.com> >>> - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de> >>> v5: >>> - Added prep patch to work with non-page aligned scatterlist pages >>> and use kmap_atomic() to lock page during modification. >>> >>> drivers/ata/libata-scsi.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- >>> include/linux/ata.h | 26 ---------------------- >>> 2 files changed, 51 insertions(+), 31 deletions(-) >>> >>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >>> index e207b33..7990cb2 100644 >>> --- a/drivers/ata/libata-scsi.c >>> +++ b/drivers/ata/libata-scsi.c >>> @@ -3282,6 +3282,54 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc) >>> return 1; >>> } >>> >>> +/** >>> + * ata_format_dsm_trim_descr() - SATL Write Same to DSM Trim >>> + * @cmd: SCSI command being translated >>> + * @num: Maximum number of entries (nominally 64). >>> + * @sector: Starting sector >>> + * @count: Total Range of request >>> + * >>> + * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted >>> + * descriptor. >>> + * >>> + * Upto 64 entries of the format: >>> + * 63:48 Range Length >>> + * 47:0 LBA >>> + * >>> + * Range Length of 0 is ignored. >>> + * LBA's should be sorted order and not overlap. >>> + * >>> + * NOTE: this is the same format as ADD LBA(S) TO NV CACHE PINNED SET >>> + */ >>> +static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num, >>> + u64 sector, u32 count) >>> +{ >>> + __le64 *buffer; >>> + u32 i = 0, used_bytes; >>> + unsigned long flags; >>> + >>> + BUILD_BUG_ON(512 > ATA_SCSI_RBUF_SIZE); >>> + >>> + spin_lock_irqsave(&ata_scsi_rbuf_lock, flags); >>> + buffer = ((void *)ata_scsi_rbuf); >>> + while (i < num) { >>> + u64 entry = sector | >>> + ((u64)(count > 0xffff ? 0xffff : count) << 48); >>> + buffer[i++] = __cpu_to_le64(entry); >>> + if (count <= 0xffff) >>> + break; >>> + count -= 0xffff; >>> + sector += 0xffff; >>> + } >>> + >>> + used_bytes = ALIGN(i * 8, 512); >>> + memset(buffer + i, 0, used_bytes - i * 8); >>> + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); >>> + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); >>> + >>> + return used_bytes; >>> +} >>> + >>> static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >>> { >>> struct ata_taskfile *tf = &qc->tf; >>> @@ -3290,8 +3338,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >>> const u8 *cdb = scmd->cmnd; >>> u64 block; >>> u32 n_block; >>> + const u32 trmax = ATA_MAX_TRIM_RNUM; >> >> This does not seem like a good idea. >> >>> u32 size; >>> - void *buf; >>> u16 fp; >>> u8 bp = 0xff; >>> >>> @@ -3319,10 +3367,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >>> if (!scsi_sg_count(scmd)) >>> goto invalid_param_len; >>> >>> - buf = page_address(sg_page(scsi_sglist(scmd))); >>> - >>> - if (n_block <= 65535 * ATA_MAX_TRIM_RNUM) { >>> - size = ata_set_lba_range_entries(buf, ATA_MAX_TRIM_RNUM, block, n_block); >>> + if (n_block <= 0xffff * trmax) { >> >> Note that the limit here would always be the advertised Maximum Write >> Same Length (ata_scsiop_inq_b0). It would be best for them to look the >> same. Besides, it doesn't seem necessary to create this trmax anyway. > > It is entirely a style thing. I tend to prefer hex when describing an interface > that specifies number of bits. The trmax is just to keep the following line > under 80 chars. > I am not tied to either. I can change it if people really find it confusing. > >>> + size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block); >>> } else { >>> fp = 2; >>> goto invalid_fld; >>> diff --git a/include/linux/ata.h b/include/linux/ata.h >>> index adbc812..45a1d71 100644 >>> --- a/include/linux/ata.h >>> +++ b/include/linux/ata.h >>> @@ -1071,32 +1071,6 @@ static inline void ata_id_to_hd_driveid(u16 *id) >>> #endif >>> } >>> >>> -/* >>> - * Write LBA Range Entries to the buffer that will cover the extent from >>> - * sector to sector + count. This is used for TRIM and for ADD LBA(S) >>> - * TO NV CACHE PINNED SET. >>> - */ >>> -static inline unsigned ata_set_lba_range_entries(void *_buffer, >>> - unsigned num, u64 sector, unsigned long count) >>> -{ >>> - __le64 *buffer = _buffer; >>> - unsigned i = 0, used_bytes; >>> - >>> - while (i < num) { >>> - u64 entry = sector | >>> - ((u64)(count > 0xffff ? 0xffff : count) << 48); >>> - buffer[i++] = __cpu_to_le64(entry); >>> - if (count <= 0xffff) >>> - break; >>> - count -= 0xffff; >>> - sector += 0xffff; >>> - } >>> - >>> - used_bytes = ALIGN(i * 8, 512); >>> - memset(buffer + i, 0, used_bytes - i * 8); >>> - return used_bytes; >>> -} >>> - >>> static inline bool ata_ok(u8 status) >>> { >>> return ((status & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ | ATA_ERR)) >>> -- >>> 2.9.3 >>> > > > > -- > Shaun Tancheff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 August 2016 at 04:23, Shaun Tancheff <shaun@tancheff.com> wrote: > Safely overwriting the attached page to ATA format from the SCSI formatted > variant. > > Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com> > --- > v6: > - Fix bisect bug reported by Tom Yan <tom.ty89@gmail.com> > - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de> > v5: > - Added prep patch to work with non-page aligned scatterlist pages > and use kmap_atomic() to lock page during modification. > > drivers/ata/libata-scsi.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- > include/linux/ata.h | 26 ---------------------- > 2 files changed, 51 insertions(+), 31 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index e207b33..7990cb2 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -3282,6 +3282,54 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc) > return 1; > } > > +/** > + * ata_format_dsm_trim_descr() - SATL Write Same to DSM Trim > + * @cmd: SCSI command being translated > + * @num: Maximum number of entries (nominally 64). > + * @sector: Starting sector > + * @count: Total Range of request > + * > + * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted > + * descriptor. > + * > + * Upto 64 entries of the format: > + * 63:48 Range Length > + * 47:0 LBA > + * > + * Range Length of 0 is ignored. > + * LBA's should be sorted order and not overlap. > + * > + * NOTE: this is the same format as ADD LBA(S) TO NV CACHE PINNED SET > + */ > +static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num, > + u64 sector, u32 count) > +{ > + __le64 *buffer; > + u32 i = 0, used_bytes; > + unsigned long flags; > + > + BUILD_BUG_ON(512 > ATA_SCSI_RBUF_SIZE); Why do we want to check "512" (a literal number) against ATA_SCSI_RBUF_SIZE here? > + > + spin_lock_irqsave(&ata_scsi_rbuf_lock, flags); > + buffer = ((void *)ata_scsi_rbuf); > + while (i < num) { > + u64 entry = sector | > + ((u64)(count > 0xffff ? 0xffff : count) << 48); > + buffer[i++] = __cpu_to_le64(entry); > + if (count <= 0xffff) > + break; > + count -= 0xffff; > + sector += 0xffff; > + } > + > + used_bytes = ALIGN(i * 8, 512); > + memset(buffer + i, 0, used_bytes - i * 8); > + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); > + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); > + > + return used_bytes; > +} > + > static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) > { > struct ata_taskfile *tf = &qc->tf; > @@ -3290,8 +3338,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) > const u8 *cdb = scmd->cmnd; > u64 block; > u32 n_block; > + const u32 trmax = ATA_MAX_TRIM_RNUM; > u32 size; > - void *buf; > u16 fp; > u8 bp = 0xff; > > @@ -3319,10 +3367,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) > if (!scsi_sg_count(scmd)) > goto invalid_param_len; > > - buf = page_address(sg_page(scsi_sglist(scmd))); > - > - if (n_block <= 65535 * ATA_MAX_TRIM_RNUM) { > - size = ata_set_lba_range_entries(buf, ATA_MAX_TRIM_RNUM, block, n_block); > + if (n_block <= 0xffff * trmax) { > + size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block); > } else { > fp = 2; > goto invalid_fld; > diff --git a/include/linux/ata.h b/include/linux/ata.h > index adbc812..45a1d71 100644 > --- a/include/linux/ata.h > +++ b/include/linux/ata.h > @@ -1071,32 +1071,6 @@ static inline void ata_id_to_hd_driveid(u16 *id) > #endif > } > > -/* > - * Write LBA Range Entries to the buffer that will cover the extent from > - * sector to sector + count. This is used for TRIM and for ADD LBA(S) > - * TO NV CACHE PINNED SET. > - */ > -static inline unsigned ata_set_lba_range_entries(void *_buffer, > - unsigned num, u64 sector, unsigned long count) > -{ > - __le64 *buffer = _buffer; > - unsigned i = 0, used_bytes; > - > - while (i < num) { > - u64 entry = sector | > - ((u64)(count > 0xffff ? 0xffff : count) << 48); > - buffer[i++] = __cpu_to_le64(entry); > - if (count <= 0xffff) > - break; > - count -= 0xffff; > - sector += 0xffff; > - } > - > - used_bytes = ALIGN(i * 8, 512); > - memset(buffer + i, 0, used_bytes - i * 8); > - return used_bytes; > -} > - > static inline bool ata_ok(u8 status) > { > return ((status & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ | ATA_ERR)) > -- > 2.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 22, 2016 at 4:20 PM, Tom Yan <tom.ty89@gmail.com> wrote: > On 22 August 2016 at 04:23, Shaun Tancheff <shaun@tancheff.com> wrote: >> Safely overwriting the attached page to ATA format from the SCSI formatted >> variant. >> >> Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com> >> --- >> v6: >> - Fix bisect bug reported by Tom Yan <tom.ty89@gmail.com> >> - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de> >> v5: >> - Added prep patch to work with non-page aligned scatterlist pages >> and use kmap_atomic() to lock page during modification. >> >> drivers/ata/libata-scsi.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- >> include/linux/ata.h | 26 ---------------------- >> 2 files changed, 51 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index e207b33..7990cb2 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -3282,6 +3282,54 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc) >> return 1; >> } >> >> +/** >> + * ata_format_dsm_trim_descr() - SATL Write Same to DSM Trim >> + * @cmd: SCSI command being translated >> + * @num: Maximum number of entries (nominally 64). >> + * @sector: Starting sector >> + * @count: Total Range of request >> + * >> + * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted >> + * descriptor. >> + * >> + * Upto 64 entries of the format: >> + * 63:48 Range Length >> + * 47:0 LBA >> + * >> + * Range Length of 0 is ignored. >> + * LBA's should be sorted order and not overlap. >> + * >> + * NOTE: this is the same format as ADD LBA(S) TO NV CACHE PINNED SET >> + */ >> +static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num, >> + u64 sector, u32 count) >> +{ >> + __le64 *buffer; >> + u32 i = 0, used_bytes; >> + unsigned long flags; >> + >> + BUILD_BUG_ON(512 > ATA_SCSI_RBUF_SIZE); > > Why do we want to check "512" (a literal number) against > ATA_SCSI_RBUF_SIZE here? Because this is re-using the response buffer in a new way. Such reuse could be a surprise to someone refactoring that code, although it's pretty unlikely. The build bug on gives some level of confidence that it won't go unnoticed. It also codifies the assumption that we can write 512 bytes to the global ata_scsi_rbuf buffer. As to using a literal 512, I was just following what was here before. It looks like there is a ATA_SECT_SIZE that can replace most of the literal 512's in here though. That might be a nice cleanup overall. Not sure it belongs here though. >> + >> + spin_lock_irqsave(&ata_scsi_rbuf_lock, flags); >> + buffer = ((void *)ata_scsi_rbuf); >> + while (i < num) { >> + u64 entry = sector | >> + ((u64)(count > 0xffff ? 0xffff : count) << 48); >> + buffer[i++] = __cpu_to_le64(entry); >> + if (count <= 0xffff) >> + break; >> + count -= 0xffff; >> + sector += 0xffff; >> + } >> + >> + used_bytes = ALIGN(i * 8, 512); >> + memset(buffer + i, 0, used_bytes - i * 8); >> + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); >> + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); >> + >> + return used_bytes; >> +} >> + >> static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >> { >> struct ata_taskfile *tf = &qc->tf; >> @@ -3290,8 +3338,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >> const u8 *cdb = scmd->cmnd; >> u64 block; >> u32 n_block; >> + const u32 trmax = ATA_MAX_TRIM_RNUM; >> u32 size; >> - void *buf; >> u16 fp; >> u8 bp = 0xff; >> >> @@ -3319,10 +3367,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) >> if (!scsi_sg_count(scmd)) >> goto invalid_param_len; >> >> - buf = page_address(sg_page(scsi_sglist(scmd))); >> - >> - if (n_block <= 65535 * ATA_MAX_TRIM_RNUM) { >> - size = ata_set_lba_range_entries(buf, ATA_MAX_TRIM_RNUM, block, n_block); >> + if (n_block <= 0xffff * trmax) { >> + size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block); >> } else { >> fp = 2; >> goto invalid_fld; >> diff --git a/include/linux/ata.h b/include/linux/ata.h >> index adbc812..45a1d71 100644 >> --- a/include/linux/ata.h >> +++ b/include/linux/ata.h >> @@ -1071,32 +1071,6 @@ static inline void ata_id_to_hd_driveid(u16 *id) >> #endif >> } >> >> -/* >> - * Write LBA Range Entries to the buffer that will cover the extent from >> - * sector to sector + count. This is used for TRIM and for ADD LBA(S) >> - * TO NV CACHE PINNED SET. >> - */ >> -static inline unsigned ata_set_lba_range_entries(void *_buffer, >> - unsigned num, u64 sector, unsigned long count) >> -{ >> - __le64 *buffer = _buffer; >> - unsigned i = 0, used_bytes; >> - >> - while (i < num) { >> - u64 entry = sector | >> - ((u64)(count > 0xffff ? 0xffff : count) << 48); >> - buffer[i++] = __cpu_to_le64(entry); >> - if (count <= 0xffff) >> - break; >> - count -= 0xffff; >> - sector += 0xffff; >> - } >> - >> - used_bytes = ALIGN(i * 8, 512); >> - memset(buffer + i, 0, used_bytes - i * 8); >> - return used_bytes; >> -} >> - >> static inline bool ata_ok(u8 status) >> { >> return ((status & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ | ATA_ERR)) >> -- >> 2.9.3 >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
The only 512 I can see in the old code is the one in: > - used_bytes = ALIGN(i * 8, 512); where the alignment is necessary because 'used_bytes' will be returned as 'size', which will be used for setting the number of 512-byte block payload when we construct the ATA taskfile / command. It may NOT be a good idea to replace it with ATA_SECT_SIZE. Some comment could be nice. So I don't think it makes any sense to check ATA_SCSI_RBUF_SIZE against 512 here. On 22 August 2016 at 22:00, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: > Because this is re-using the response buffer in a new way. > Such reuse could be a surprise to someone refactoring that > code, although it's pretty unlikely. The build bug on > gives some level of confidence that it won't go unnoticed. > It also codifies the assumption that we can write 512 bytes > to the global ata_scsi_rbuf buffer. > > As to using a literal 512, I was just following what was here > before. > > It looks like there is a ATA_SECT_SIZE that can replace most > of the literal 512's in here though. That might be a nice cleanup > overall. Not sure it belongs here though. > >>> + used_bytes = ALIGN(i * 8, 512); >>> + memset(buffer + i, 0, used_bytes - i * 8); >>> + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); And I don't think you have a reason to use 512 here either. It appears to me that you should use ATA_SCSI_RBUF_SIZE instead (see ata_scsi_rbuf_put in libata-scsi). If not, it should probably be a _derived_ value (e.g. from `i`) that tells the _actual_ size of `buffer`. Again, note that even when we prefer to stick with _one_ 512-byte block TRIM payload, we should probably NEVER _hard code_ such limit (for it's really ugly and unnecessary) in functions like this. All we need to do is to advertise the limit (or lower) as Maximum Write Length, and make sure our SATL works as per SBC that it will reject SCSI Write Same commands that is "larger" than that. >>> + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); >>> + >>> + return used_bytes; >>> +} -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 22, 2016 at 6:49 PM, Tom Yan <tom.ty89@gmail.com> wrote: > The only 512 I can see in the old code is the one in: > >> - used_bytes = ALIGN(i * 8, 512); > > where the alignment is necessary because 'used_bytes' will be returned > as 'size', which will be used for setting the number of 512-byte block > payload when we construct the ATA taskfile / command. It may NOT be a > good idea to replace it with ATA_SECT_SIZE. Some comment could be > nice. Not sure I agree with that analysis. Could just as well assign it directly, as ALIGN() is just superfluous here. Later the count is always 512/512 -> 1. Always. "i" is used here to limit the number of bytes that need to be memset() after filling to payload. Personally I think memset is fast enough that it is better to do before rather than later but I figure if the code works let it be. > So I don't think it makes any sense to check ATA_SCSI_RBUF_SIZE > against 512 here. Again, not sure I agree, but I don't really care on that point. Just many years of defensive coding. > On 22 August 2016 at 22:00, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: >> Because this is re-using the response buffer in a new way. >> Such reuse could be a surprise to someone refactoring that >> code, although it's pretty unlikely. The build bug on >> gives some level of confidence that it won't go unnoticed. >> It also codifies the assumption that we can write 512 bytes >> to the global ata_scsi_rbuf buffer. >> >> As to using a literal 512, I was just following what was here >> before. >> >> It looks like there is a ATA_SECT_SIZE that can replace most >> of the literal 512's in here though. That might be a nice cleanup >> overall. Not sure it belongs here though. >> > >>>> + used_bytes = ALIGN(i * 8, 512); >>>> + memset(buffer + i, 0, used_bytes - i * 8); >>>> + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); > > And I don't think you have a reason to use 512 here either. It appears > to me that you should use ATA_SCSI_RBUF_SIZE instead (see > ata_scsi_rbuf_put in libata-scsi). If not, it should probably be a > _derived_ value (e.g. from `i`) that tells the _actual_ size of > `buffer`. Nope. We *must* copy the whole 512 byte payload into the sglist. Otherwise what was the point of the memset to clear out an cruft? Failing to move the whole payload into place could leave whatever garbage is in the buffer to be interpreted as an actual trim and do real damage. I certainly can't use ATA_SCSI_RBUF_SIZE because the payload attached to the cmd need only be 512 bytes. Trying to copy in 4k is going to cause bad things when you check the return from sg_copy_from_buffer() and notice you failed to copy in you payload. > Again, note that even when we prefer to stick with _one_ 512-byte > block TRIM payload, we should probably NEVER _hard code_ such limit > (for it's really ugly and unnecessary) in functions like this. All we The interface requires well formed 512 byte chunks so we have to at least have to do enough to ensure that we work in multiples of 512. Since 512 is all over the spec for this type of thing I think it would be reasonable to have a define or enum if you don't think reusing ATA_SECT_SIZE is good maybe something like ATA_CMD_XFER_SIZE ? > need to do is to advertise the limit (or lower) as Maximum Write > Length, and make sure our SATL works as per SBC that it will reject > SCSI Write Same commands that is "larger" than that. >>>> + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); >>>> + >>>> + return used_bytes; >>>> +}
On 23 August 2016 at 01:01, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: > On Mon, Aug 22, 2016 at 6:49 PM, Tom Yan <tom.ty89@gmail.com> wrote: >> The only 512 I can see in the old code is the one in: >> >>> - used_bytes = ALIGN(i * 8, 512); >> >> where the alignment is necessary because 'used_bytes' will be returned >> as 'size', which will be used for setting the number of 512-byte block >> payload when we construct the ATA taskfile / command. It may NOT be a >> good idea to replace it with ATA_SECT_SIZE. Some comment could be >> nice. > > Not sure I agree with that analysis. Could just as well assign it directly, > as ALIGN() is just superfluous here. > Later the count is always 512/512 -> 1. Always. It is always 1 merely because we prefer sticking to that. Say we want to enable multi-block payload now, it won't be 1 anymore. Also note that the payload is NOT always fully-filled. Say, I issue a discard request of 1M or a SCSI Write Same (16) commands of 2048 blocks. If you do not round up used_bytes, the result of the division will be 0 in those cases. Basically that's the mathematical version of the story of why we need the following memset(). > > "i" is used here to limit the number of bytes that need to be memset() > after filling to payload. Personally I think memset is fast enough that > it is better to do before rather than later but I figure if the code works > let it be. Not sure what you mean by "fast enough"/"do before". What memset() do here is to locate the offset/location to starting filling the memory (buffer + i) and fill zero until the payload is 512-byte aligned (used_bytes - i * 8, where used_bytes is an aligned/rounded-up-to-512 version of i * 8). Again, it does NOT and should NOT limit the allocated memory to _one_ 512-byte block. > >> So I don't think it makes any sense to check ATA_SCSI_RBUF_SIZE >> against 512 here. > > Again, not sure I agree, but I don't really care on that point. Just many > years of defensive coding. The thing is you can't even tell what exactly you are defending here. This would only make the perhaps already obfuscated code more obfuscated, without actually preventing any bad thing to happen. The only case you would want to check ATA_SCSI_RBUF_SIZE here is, you somehow want to use it as buflen parameter in the following sg_copy_from_buffer call. Then you may say "I want to make sure that we wouldn't carelessly decrease the currently 4096 ATA_SCSI_RBUF_SIZE to a value smaller than 512 someday, which cannot even accommodate a single full-block payload." And even that's a weak reason. > >> On 22 August 2016 at 22:00, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: >>> Because this is re-using the response buffer in a new way. >>> Such reuse could be a surprise to someone refactoring that >>> code, although it's pretty unlikely. The build bug on >>> gives some level of confidence that it won't go unnoticed. >>> It also codifies the assumption that we can write 512 bytes >>> to the global ata_scsi_rbuf buffer. >>> >>> As to using a literal 512, I was just following what was here >>> before. >>> >>> It looks like there is a ATA_SECT_SIZE that can replace most >>> of the literal 512's in here though. That might be a nice cleanup >>> overall. Not sure it belongs here though. >>> >> >>>>> + used_bytes = ALIGN(i * 8, 512); >>>>> + memset(buffer + i, 0, used_bytes - i * 8); >>>>> + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); >> >> And I don't think you have a reason to use 512 here either. It appears >> to me that you should use ATA_SCSI_RBUF_SIZE instead (see >> ata_scsi_rbuf_put in libata-scsi). If not, it should probably be a >> _derived_ value (e.g. from `i`) that tells the _actual_ size of >> `buffer`. > > Nope. We *must* copy the whole 512 byte payload into the sglist. > Otherwise what was the point of the memset to clear out an cruft? > Failing to move the whole payload into place could leave whatever > garbage is in the buffer to be interpreted as an actual trim and do > real damage. I certainly can't use ATA_SCSI_RBUF_SIZE because > the payload attached to the cmd need only be 512 bytes. Trying > to copy in 4k is going to cause bad things when you check > the return from sg_copy_from_buffer() and notice you failed to > copy in you payload. I don't really know how scatter/gather list and the related functions work to be honest. You should probably use used_bytes then, since used_bytes is exactly the final size of buffer (after memset, and it is 512-byte aligned, but not always 512 in terms of logic). > >> Again, note that even when we prefer to stick with _one_ 512-byte >> block TRIM payload, we should probably NEVER _hard code_ such limit >> (for it's really ugly and unnecessary) in functions like this. All we > > The interface requires well formed 512 byte chunks so we have to > at least have to do enough to ensure that we work in multiples of > 512. Since 512 is all over the spec for this type of thing I think > it would be reasonable to have a define or enum if you don't think > reusing ATA_SECT_SIZE is good maybe something > like ATA_CMD_XFER_SIZE ? I have no idea. Perhaps it is fine to use ATA_SECT_SIZE if what you described ("512 is all over the spec for this type of thing") is true. It's just personally I don't see any benefit in terms of readability introducing macro like that. If anything should be done, I would say it is to add comment above ALIGN() and the ATA taskfiles pointing out 512 is used because ACS specifically pointed out that "TRIM payload blocks are always 512-byte, but not the logical sector size or so". > >> need to do is to advertise the limit (or lower) as Maximum Write >> Length, and make sure our SATL works as per SBC that it will reject >> SCSI Write Same commands that is "larger" than that. >>>>> + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); >>>>> + >>>>> + return used_bytes; >>>>> +} > > > -- > Shaun Tancheff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 23, 2016 at 12:26 AM, Tom Yan <tom.ty89@gmail.com> wrote: > It is always 1 merely because we prefer sticking to that. Say we want > to enable multi-block payload now, it won't be 1 anymore. Sorry, I though that DSM TRIM is using 512 bytes here because WRITE_SAME_16 has a payload of a single logical sector. > Also note that the payload is NOT always fully-filled. Oh, I was considering filling the remainder of the buffer with 0's using memset() as you described to be "filly-filled" in this case. Sorry for the confusion.
On 23 August 2016 at 06:20, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: > On Tue, Aug 23, 2016 at 12:26 AM, Tom Yan <tom.ty89@gmail.com> wrote: > >> It is always 1 merely because we prefer sticking to that. Say we want >> to enable multi-block payload now, it won't be 1 anymore. > > Sorry, I though that DSM TRIM is using 512 bytes here because > WRITE_SAME_16 has a payload of a single logical sector. Nope, SCSI Write Same commands does not have payload (or in SCSI terms, parameter list / data-out buffer). > >> Also note that the payload is NOT always fully-filled. > > Oh, I was considering filling the remainder of the buffer > with 0's using memset() as you described to be "filly-filled" > in this case. Sorry for the confusion. Well yeah _after_ memset() it should always be "fully-filled" (i.e. 512-byte aligned). > > -- > Shaun Tancheff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 23, 2016 at 2:53 AM, Tom Yan <tom.ty89@gmail.com> wrote: > On 23 August 2016 at 06:20, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: >> On Tue, Aug 23, 2016 at 12:26 AM, Tom Yan <tom.ty89@gmail.com> wrote: >> >>> It is always 1 merely because we prefer sticking to that. Say we want >>> to enable multi-block payload now, it won't be 1 anymore. >> >> Sorry, I though that DSM TRIM is using 512 bytes here because >> WRITE_SAME_16 has a payload of a single logical sector. > > Nope, SCSI Write Same commands does not have payload (or in SCSI > terms, parameter list / data-out buffer). Hmm. Not sure about T10, but that's not how I read sd_setup_write_same_cmnd(), it always setups up a transfer of sector_size for scsi_init_io(). As I understand things, this is how the cmd's sglist is populated and why this should be using sg_copy_from_buffer() rather than open coding to the page.
There's comment about `rq->__data_len` in sd.c (and blkdev.h, definition of struct `request`). Apparently it is "internal only" (in terms of the kernel). As for `cmd->transfersize`, it's probably talking about the CDB itself. Although different commands have CDB of various lengths (much shorter than 512-byte), but I guess when the actual command is sent to the device, we will still transfer it in a full logical block for some reason. Anyway these will not have anything to do with how we form the TRIM payload. The SATL simply read the LBA (starting offset) and NUMBER OF BLOCK (to TRIM) field in the CDB and pack ranges according to that. All we care is the actual TRIM limit of the ATA device (and conservative concern on bogus ones). On 23 August 2016 at 08:42, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: > On Tue, Aug 23, 2016 at 2:53 AM, Tom Yan <tom.ty89@gmail.com> wrote: >> On 23 August 2016 at 06:20, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: >>> On Tue, Aug 23, 2016 at 12:26 AM, Tom Yan <tom.ty89@gmail.com> wrote: >>> >>>> It is always 1 merely because we prefer sticking to that. Say we want >>>> to enable multi-block payload now, it won't be 1 anymore. >>> >>> Sorry, I though that DSM TRIM is using 512 bytes here because >>> WRITE_SAME_16 has a payload of a single logical sector. >> >> Nope, SCSI Write Same commands does not have payload (or in SCSI >> terms, parameter list / data-out buffer). > > Hmm. Not sure about T10, but that's not how I read > sd_setup_write_same_cmnd(), it always setups up a transfer of > sector_size for scsi_init_io(). > > As I understand things, this is how the cmd's sglist is populated and why > this should be using sg_copy_from_buffer() rather than open coding > to the page. > -- > Shaun Tancheff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "Tom" == Tom Yan <tom.ty89@gmail.com> writes:
Tom> Nope, SCSI Write Same commands does not have payload (or in SCSI
Tom> terms, parameter list / data-out buffer).
WRITE SAME has a a payload of 1 logical block (unless NDOB is set but we
have had no good reason to support that yet).
UNMAP has a payload that varies based on the number of range
descriptors. The SCSI disk driver only ever issues a single descriptor
but since libata doesn't support UNMAP this doesn't really come into
play.
Ideally there would be a way to distinguish between device limits for
WRITE SAME with the UNMAP bit and for "regular" WRITE SAME. One way to
do that would be to transition the libata discard implementation over to
single-range UNMAP, fill out the relevant VPD page B0 fields and leave
the WRITE SAME bits for writing zeroes.
One reason that has not been particularly compelling is that the WRITE
SAME payload buffer does double duty to hold the ATA DSM TRIM range
descriptors and matches the required ATA payload size. Whereas the UNMAP
command would only provide 24 bytes of TRIM range space.
Also, please be careful with transfer lengths, __data_len, etc. As
mentioned, the transfer length WRITE SAME is typically 512 bytes and
that's the number of bytes that need to be DMA'ed and transferred over
the wire by the controller. But from a command completion perspective we
need to complete however many bytes the command acted upon. Unlike reads
and writes there is not a 1:1 mapping between the transfer length and
the affected area. So we do a bit of magic after the buffer has been
mapped to ensure that the completion byte count matches the number of
blocks that were affected by the command rather than the size of the
data buffer in bytes.
On 24 August 2016 at 11:33, Martin K. Petersen <martin.petersen@oracle.com> wrote: >>>>>> "Tom" == Tom Yan <tom.ty89@gmail.com> writes: > > Tom> Nope, SCSI Write Same commands does not have payload (or in SCSI > Tom> terms, parameter list / data-out buffer). > > WRITE SAME has a a payload of 1 logical block (unless NDOB is set but we > have had no good reason to support that yet). Interesting, I wasn't aware of the bit. I just didn't see any parameter list defined for any of the Write Same commands. Ah wait, it carries the pattern (the "same") and so. Hmm, it doesn't seem like the translation implemented in this patch series cares about the payload though? > > UNMAP has a payload that varies based on the number of range > descriptors. The SCSI disk driver only ever issues a single descriptor > but since libata doesn't support UNMAP this doesn't really come into > play. > > Ideally there would be a way to distinguish between device limits for > WRITE SAME with the UNMAP bit and for "regular" WRITE SAME. One way to > do that would be to transition the libata discard implementation over to > single-range UNMAP, fill out the relevant VPD page B0 fields and leave > the WRITE SAME bits for writing zeroes. > > One reason that has not been particularly compelling is that the WRITE > SAME payload buffer does double duty to hold the ATA DSM TRIM range Huh? Why would the SATL care about its payload buffer for TRIM (i.e. when the UNMAP bit is set)? Doesn't it just read the LBA and NUMBER OF BLOCKS field and pack TRIM ranges/payload according to that? > descriptors and matches the required ATA payload size. Whereas the UNMAP Why would it need to "matches the required ATA payload size"? > command would only provide 24 bytes of TRIM range space. I don't really follow. The UNMAP descriptor has LBA (8 bytes / 64-bit) and NUMBER OF BLOCKS (4 bytes / 32-bit) field of the same length as Write Same (16). Even if the SCSI disk driver will only send one descriptor, it should work as good as Write Same (16). > > Also, please be careful with transfer lengths, __data_len, etc. As > mentioned, the transfer length WRITE SAME is typically 512 bytes and > that's the number of bytes that need to be DMA'ed and transferred over > the wire by the controller. But from a command completion perspective we > need to complete however many bytes the command acted upon. Unlike reads > and writes there is not a 1:1 mapping between the transfer length and > the affected area. So we do a bit of magic after the buffer has been > mapped to ensure that the completion byte count matches the number of > blocks that were affected by the command rather than the size of the > data buffer in bytes. > > -- > Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 24, 2016 at 12:31 AM, Tom Yan <tom.ty89@gmail.com> wrote: > On 24 August 2016 at 11:33, Martin K. Petersen > <martin.petersen@oracle.com> wrote: >>>>>>> "Tom" == Tom Yan <tom.ty89@gmail.com> writes: >> >> Tom> Nope, SCSI Write Same commands does not have payload (or in SCSI >> Tom> terms, parameter list / data-out buffer). >> >> WRITE SAME has a a payload of 1 logical block (unless NDOB is set but we >> have had no good reason to support that yet). > > Interesting, I wasn't aware of the bit. I just didn't see any > parameter list defined for any of the Write Same commands. Ah wait, it > carries the pattern (the "same") and so. > > Hmm, it doesn't seem like the translation implemented in this patch > series cares about the payload though? As repeated here and elsewhere the payload is: scsi_sglist(cmd) and it was put there by scsi_init_io() when it called scsi_init_sgtable() >> UNMAP has a payload that varies based on the number of range >> descriptors. The SCSI disk driver only ever issues a single descriptor >> but since libata doesn't support UNMAP this doesn't really come into >> play. >> >> Ideally there would be a way to distinguish between device limits for >> WRITE SAME with the UNMAP bit and for "regular" WRITE SAME. One way to >> do that would be to transition the libata discard implementation over to >> single-range UNMAP, fill out the relevant VPD page B0 fields and leave >> the WRITE SAME bits for writing zeroes. >> >> One reason that has not been particularly compelling is that the WRITE >> SAME payload buffer does double duty to hold the ATA DSM TRIM range > > Huh? Why would the SATL care about its payload buffer for TRIM (i.e. > when the UNMAP bit is set)? Doesn't it just read the LBA and NUMBER OF > BLOCKS field and pack TRIM ranges/payload according to that? > >> descriptors and matches the required ATA payload size. Whereas the UNMAP > > Why would it need to "matches the required ATA payload size"? > >> command would only provide 24 bytes of TRIM range space. > > I don't really follow. The UNMAP descriptor has LBA (8 bytes / 64-bit) > and NUMBER OF BLOCKS (4 bytes / 32-bit) field of the same length as > Write Same (16). Even if the SCSI disk driver will only send one > descriptor, it should work as good as Write Same (16). The "payload" is the data block transferred with the command. The "descriptor" is, in this context, the contents of the payload as it "describes" what the action the command is supposed to perform. The "payload" contains the "descriptor" that "describes" how DSM TRIM should determine which logical blocks it should UNMAP. >> Also, please be careful with transfer lengths, __data_len, etc. As >> mentioned, the transfer length WRITE SAME is typically 512 bytes and >> that's the number of bytes that need to be DMA'ed and transferred over >> the wire by the controller. But from a command completion perspective we >> need to complete however many bytes the command acted upon. Unlike reads >> and writes there is not a 1:1 mapping between the transfer length and >> the affected area. So we do a bit of magic after the buffer has been >> mapped to ensure that the completion byte count matches the number of >> blocks that were affected by the command rather than the size of the >> data buffer in bytes. >> >> -- >> Martin K. Petersen Oracle Linux Engineering
On 25 August 2016 at 05:28, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: > On Wed, Aug 24, 2016 at 12:31 AM, Tom Yan <tom.ty89@gmail.com> wrote: >> On 24 August 2016 at 11:33, Martin K. Petersen >> <martin.petersen@oracle.com> wrote: >>>>>>>> "Tom" == Tom Yan <tom.ty89@gmail.com> writes: >>> >>> Tom> Nope, SCSI Write Same commands does not have payload (or in SCSI >>> Tom> terms, parameter list / data-out buffer). >>> >>> WRITE SAME has a a payload of 1 logical block (unless NDOB is set but we >>> have had no good reason to support that yet). >> >> Interesting, I wasn't aware of the bit. I just didn't see any >> parameter list defined for any of the Write Same commands. Ah wait, it >> carries the pattern (the "same") and so. >> >> Hmm, it doesn't seem like the translation implemented in this patch >> series cares about the payload though? > > As repeated here and elsewhere the payload is: > scsi_sglist(cmd) > and it was put there by scsi_init_io() when it called scsi_init_sgtable() What I mean is: put_unaligned_le32(0u, &sctpg[10]); So even if the payload of the SCSI Write Same commands instruct a non-zero pattern writing, SCT Write Same will conveniently ignore that do zero-filling anyway. That's what I mean by "doesn't care about the payload". Though that would only be case with SG_IO though. SCSI Write Same issued from block layer (BLKZEROOUT) will be instructing zero-filling anyway. > >>> UNMAP has a payload that varies based on the number of range >>> descriptors. The SCSI disk driver only ever issues a single descriptor >>> but since libata doesn't support UNMAP this doesn't really come into >>> play. >>> >>> Ideally there would be a way to distinguish between device limits for >>> WRITE SAME with the UNMAP bit and for "regular" WRITE SAME. One way to >>> do that would be to transition the libata discard implementation over to >>> single-range UNMAP, fill out the relevant VPD page B0 fields and leave >>> the WRITE SAME bits for writing zeroes. >>> >>> One reason that has not been particularly compelling is that the WRITE >>> SAME payload buffer does double duty to hold the ATA DSM TRIM range >> >> Huh? Why would the SATL care about its payload buffer for TRIM (i.e. >> when the UNMAP bit is set)? Doesn't it just read the LBA and NUMBER OF >> BLOCKS field and pack TRIM ranges/payload according to that? >> >>> descriptors and matches the required ATA payload size. Whereas the UNMAP >> >> Why would it need to "matches the required ATA payload size"? >> >>> command would only provide 24 bytes of TRIM range space. >> >> I don't really follow. The UNMAP descriptor has LBA (8 bytes / 64-bit) >> and NUMBER OF BLOCKS (4 bytes / 32-bit) field of the same length as >> Write Same (16). Even if the SCSI disk driver will only send one >> descriptor, it should work as good as Write Same (16). > > The "payload" is the data block transferred with the command. > The "descriptor" is, in this context, the contents of the payload as > it "describes" what the action the command is supposed to perform. > I know right. > The "payload" contains the "descriptor" that "describes" how > DSM TRIM should determine which logical blocks it should UNMAP. This should only be the case of UNMAP command, but not SCSI Write Same with UNMAP bit set. And either way it should not affect how large the ATA TRIM payload can be. > >>> Also, please be careful with transfer lengths, __data_len, etc. As >>> mentioned, the transfer length WRITE SAME is typically 512 bytes and >>> that's the number of bytes that need to be DMA'ed and transferred over >>> the wire by the controller. But from a command completion perspective we >>> need to complete however many bytes the command acted upon. Unlike reads >>> and writes there is not a 1:1 mapping between the transfer length and >>> the affected area. So we do a bit of magic after the buffer has been >>> mapped to ensure that the completion byte count matches the number of >>> blocks that were affected by the command rather than the size of the >>> data buffer in bytes. >>> >>> -- >>> Martin K. Petersen Oracle Linux Engineering > -- > Shaun Tancheff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 25, 2016 at 1:31 AM, Tom Yan <tom.ty89@gmail.com> wrote: > On 25 August 2016 at 05:28, Shaun Tancheff <shaun.tancheff@seagate.com> wrote: >> On Wed, Aug 24, 2016 at 12:31 AM, Tom Yan <tom.ty89@gmail.com> wrote: >>> On 24 August 2016 at 11:33, Martin K. Petersen >>> <martin.petersen@oracle.com> wrote: >>>>>>>>> "Tom" == Tom Yan <tom.ty89@gmail.com> writes: >>>> >>>> Tom> Nope, SCSI Write Same commands does not have payload (or in SCSI >>>> Tom> terms, parameter list / data-out buffer). >>>> >>>> WRITE SAME has a a payload of 1 logical block (unless NDOB is set but we >>>> have had no good reason to support that yet). >>> >>> Interesting, I wasn't aware of the bit. I just didn't see any >>> parameter list defined for any of the Write Same commands. Ah wait, it >>> carries the pattern (the "same") and so. >>> >>> Hmm, it doesn't seem like the translation implemented in this patch >>> series cares about the payload though? >> >> As repeated here and elsewhere the payload is: >> scsi_sglist(cmd) >> and it was put there by scsi_init_io() when it called scsi_init_sgtable() > > What I mean is: > > put_unaligned_le32(0u, &sctpg[10]); > > So even if the payload of the SCSI Write Same commands instruct a > non-zero pattern writing, SCT Write Same will conveniently ignore that > do zero-filling anyway. That's what I mean by "doesn't care about the > payload". If you would like to add support for that it would be nice. I am not planning to do so here. > Though that would only be case with SG_IO though. SCSI Write Same > issued from block layer (BLKZEROOUT) will be instructing zero-filling > anyway. >>>> UNMAP has a payload that varies based on the number of range >>>> descriptors. The SCSI disk driver only ever issues a single descriptor >>>> but since libata doesn't support UNMAP this doesn't really come into >>>> play. >>>> >>>> Ideally there would be a way to distinguish between device limits for >>>> WRITE SAME with the UNMAP bit and for "regular" WRITE SAME. One way to >>>> do that would be to transition the libata discard implementation over to >>>> single-range UNMAP, fill out the relevant VPD page B0 fields and leave >>>> the WRITE SAME bits for writing zeroes. >>>> >>>> One reason that has not been particularly compelling is that the WRITE >>>> SAME payload buffer does double duty to hold the ATA DSM TRIM range >>> >>> Huh? Why would the SATL care about its payload buffer for TRIM (i.e. >>> when the UNMAP bit is set)? Doesn't it just read the LBA and NUMBER OF >>> BLOCKS field and pack TRIM ranges/payload according to that? >>> >>>> descriptors and matches the required ATA payload size. Whereas the UNMAP >>> >>> Why would it need to "matches the required ATA payload size"? >>> >>>> command would only provide 24 bytes of TRIM range space. >>> >>> I don't really follow. The UNMAP descriptor has LBA (8 bytes / 64-bit) >>> and NUMBER OF BLOCKS (4 bytes / 32-bit) field of the same length as >>> Write Same (16). Even if the SCSI disk driver will only send one >>> descriptor, it should work as good as Write Same (16). >> >> The "payload" is the data block transferred with the command. >> The "descriptor" is, in this context, the contents of the payload as >> it "describes" what the action the command is supposed to perform. >> > > I know right. > >> The "payload" contains the "descriptor" that "describes" how >> DSM TRIM should determine which logical blocks it should UNMAP. > > This should only be the case of UNMAP command, but not SCSI Write Same > with UNMAP bit set. And either way it should not affect how large the > ATA TRIM payload can be. The current SATL does not report support for UNMAP. If you think it should be added please submit a patch. If you would like to extend the current translate to support sending multiple blocks of trim data please submit a patch. >>>> Also, please be careful with transfer lengths, __data_len, etc. As >>>> mentioned, the transfer length WRITE SAME is typically 512 bytes and >>>> that's the number of bytes that need to be DMA'ed and transferred over >>>> the wire by the controller. But from a command completion perspective we >>>> need to complete however many bytes the command acted upon. Unlike reads >>>> and writes there is not a 1:1 mapping between the transfer length and >>>> the affected area. So we do a bit of magic after the buffer has been >>>> mapped to ensure that the completion byte count matches the number of >>>> blocks that were affected by the command rather than the size of the >>>> data buffer in bytes. >>>> >>>> -- >>>> Martin K. Petersen Oracle Linux Engineering >> -- >> Shaun Tancheff
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index e207b33..7990cb2 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -3282,6 +3282,54 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc) return 1; } +/** + * ata_format_dsm_trim_descr() - SATL Write Same to DSM Trim + * @cmd: SCSI command being translated + * @num: Maximum number of entries (nominally 64). + * @sector: Starting sector + * @count: Total Range of request + * + * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted + * descriptor. + * + * Upto 64 entries of the format: + * 63:48 Range Length + * 47:0 LBA + * + * Range Length of 0 is ignored. + * LBA's should be sorted order and not overlap. + * + * NOTE: this is the same format as ADD LBA(S) TO NV CACHE PINNED SET + */ +static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num, + u64 sector, u32 count) +{ + __le64 *buffer; + u32 i = 0, used_bytes; + unsigned long flags; + + BUILD_BUG_ON(512 > ATA_SCSI_RBUF_SIZE); + + spin_lock_irqsave(&ata_scsi_rbuf_lock, flags); + buffer = ((void *)ata_scsi_rbuf); + while (i < num) { + u64 entry = sector | + ((u64)(count > 0xffff ? 0xffff : count) << 48); + buffer[i++] = __cpu_to_le64(entry); + if (count <= 0xffff) + break; + count -= 0xffff; + sector += 0xffff; + } + + used_bytes = ALIGN(i * 8, 512); + memset(buffer + i, 0, used_bytes - i * 8); + sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buffer, 512); + spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags); + + return used_bytes; +} + static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) { struct ata_taskfile *tf = &qc->tf; @@ -3290,8 +3338,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) const u8 *cdb = scmd->cmnd; u64 block; u32 n_block; + const u32 trmax = ATA_MAX_TRIM_RNUM; u32 size; - void *buf; u16 fp; u8 bp = 0xff; @@ -3319,10 +3367,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) if (!scsi_sg_count(scmd)) goto invalid_param_len; - buf = page_address(sg_page(scsi_sglist(scmd))); - - if (n_block <= 65535 * ATA_MAX_TRIM_RNUM) { - size = ata_set_lba_range_entries(buf, ATA_MAX_TRIM_RNUM, block, n_block); + if (n_block <= 0xffff * trmax) { + size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block); } else { fp = 2; goto invalid_fld; diff --git a/include/linux/ata.h b/include/linux/ata.h index adbc812..45a1d71 100644 --- a/include/linux/ata.h +++ b/include/linux/ata.h @@ -1071,32 +1071,6 @@ static inline void ata_id_to_hd_driveid(u16 *id) #endif } -/* - * Write LBA Range Entries to the buffer that will cover the extent from - * sector to sector + count. This is used for TRIM and for ADD LBA(S) - * TO NV CACHE PINNED SET. - */ -static inline unsigned ata_set_lba_range_entries(void *_buffer, - unsigned num, u64 sector, unsigned long count) -{ - __le64 *buffer = _buffer; - unsigned i = 0, used_bytes; - - while (i < num) { - u64 entry = sector | - ((u64)(count > 0xffff ? 0xffff : count) << 48); - buffer[i++] = __cpu_to_le64(entry); - if (count <= 0xffff) - break; - count -= 0xffff; - sector += 0xffff; - } - - used_bytes = ALIGN(i * 8, 512); - memset(buffer + i, 0, used_bytes - i * 8); - return used_bytes; -} - static inline bool ata_ok(u8 status) { return ((status & (ATA_BUSY | ATA_DRDY | ATA_DF | ATA_DRQ | ATA_ERR))
Safely overwriting the attached page to ATA format from the SCSI formatted variant. Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com> --- v6: - Fix bisect bug reported by Tom Yan <tom.ty89@gmail.com> - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de> v5: - Added prep patch to work with non-page aligned scatterlist pages and use kmap_atomic() to lock page during modification. drivers/ata/libata-scsi.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- include/linux/ata.h | 26 ---------------------- 2 files changed, 51 insertions(+), 31 deletions(-)