Re: [PATCH v6 1/4] libata: Safely overwrite attached page in WRITE SAME xlat

[Shaun Tancheff]

On Mon, Aug 22, 2016 at 4:20 PM, Tom Yan <tom.ty89@xxxxxxxxx> wrote:
> On 22 August 2016 at 04:23, Shaun Tancheff <shaun@xxxxxxxxxxxx> wrote:
>> Safely overwriting the attached page to ATA format from the SCSI formatted
>> variant.
>>
>> Signed-off-by: Shaun Tancheff <shaun.tancheff@xxxxxxxxxxx>
>> ---
>> v6:
>>  - Fix bisect bug reported by Tom Yan <tom.ty89@xxxxxxxxx>
>>  - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@xxxxxx>
>> 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Shaun Tancheff