Re: [PATCH 1/5] sd: configure ZBC devices

2016-08-01 Thread Shaun Tancheff 
On Tue, Jul 19, 2016 at 8:25 AM, Hannes Reinecke  wrote:
> For ZBC devices I/O must not cross zone boundaries, so setup
> the 'chunk_sectors' block queue setting to the zone size.
> This is only valid for REPORT ZONES SAME type 2 or 3;
> for other types the zone sizes might be different
> for individual zones. So issue a warning if the type is
> found to be different.
> Also the capacity might be different from the announced
> capacity, so adjust it as needed.
>
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/scsi/sd.c | 120 
> --
>  drivers/scsi/sd.h |  12 +
>  include/scsi/scsi_proto.h |  17 +++
>  3 files changed, 144 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 428c03e..249ea81 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1972,6 +1972,57 @@ sd_spinup_disk(struct scsi_disk *sdkp)
> }
>  }
>
> +/**
> + * sd_zbc_report_zones - Issue a REPORT ZONES scsi command
> + * @sdkp: SCSI disk to which the command should be send
> + * @buffer: response buffer
> + * @bufflen: length of @buffer
> + * @start_sector: logical sector for the zone information should be reported
> + * @option: option for report zones command
> + * @partial: flag to set 'partial' bit for report zones command
> + */
> +static int
> +sd_zbc_report_zones(struct scsi_disk *sdkp, unsigned char *buffer,
> +   int bufflen, sector_t start_sector,
> +   enum zbc_zone_reporting_options option, bool partial)
> +{
> +   struct scsi_device *sdp = sdkp->device;
> +   const int timeout = sdp->request_queue->rq_timeout
> +   * SD_FLUSH_TIMEOUT_MULTIPLIER;
> +   struct scsi_sense_hdr sshdr;
> +   sector_t start_lba = sectors_to_logical(sdkp->device, start_sector);
> +   unsigned char cmd[16];
> +   int result;
> +
> +   if (!scsi_device_online(sdp)) {
> +   sd_printk(KERN_INFO, sdkp, "device not online\n");
> +   return -ENODEV;
> +   }
> +
> +   sd_printk(KERN_INFO, sdkp, "REPORT ZONES lba %zu len %d\n",
> + start_lba, bufflen);
> +
> +   memset(cmd, 0, 16);
> +   cmd[0] = ZBC_IN;
> +   cmd[1] = ZI_REPORT_ZONES;
> +   put_unaligned_be64(start_lba, [2]);
> +   put_unaligned_be32(bufflen, [10]);
> +   cmd[14] = (partial ? ZBC_REPORT_ZONE_PARTIAL : 0) | option;
> +   memset(buffer, 0, bufflen);
> +
> +   result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
> + buffer, bufflen, ,
> + timeout, SD_MAX_RETRIES, NULL);
> +
> +   if (result) {
> +   sd_printk(KERN_NOTICE, sdkp,
> + "REPORT ZONES lba %zu failed with %d/%d\n",
> + start_lba, host_byte(result), driver_byte(result));
> +
> +   return -EIO;
> +   }
> +   return 0;
> +}
>
>  /*
>   * Determine whether disk supports Data Integrity Field.
> @@ -2014,6 +2065,59 @@ static int sd_read_protection_type(struct scsi_disk 
> *sdkp, unsigned char *buffer
> return ret;
>  }
>
> +static void sd_read_zones(struct scsi_disk *sdkp, unsigned char *buffer)
> +{
> +   int retval;
> +   unsigned char *desc;
> +   u32 rep_len;
> +   u8 same;
> +   u64 zone_len, lba;
> +
> +   if (sdkp->zoned != 1)
> +   /* Device managed, no special handling required */
> +   return;
> +
> +   retval = sd_zbc_report_zones(sdkp, buffer, SD_BUF_SIZE,
> +0, ZBC_ZONE_REPORTING_OPTION_ALL, false);
> +   if (retval < 0)
> +   return;
> +
> +   rep_len = get_unaligned_be32([0]);
> +   if (rep_len < 64) {
> +   sd_printk(KERN_WARNING, sdkp,
> + "REPORT ZONES report invalid length %u\n",
> + rep_len);
> +   return;
> +   }
> +
> +   if (sdkp->rc_basis == 0) {
> +   /* The max_lba field is the capacity of a zoned device */
> +   lba = get_unaligned_be64([8]);
> +   if (lba + 1 > sdkp->capacity) {
> +   sd_printk(KERN_WARNING, sdkp,
> + "Max LBA %zu (capacity %zu)\n",
> + (sector_t) lba + 1, sdkp->capacity);
> +   sdkp->capacity = lba + 1;
> +   }
> +   }
> +
> +   /*
> +* Adjust 'chunk_sectors' to the zone length if the device
> +* supports equal zone sizes.
> +*/
> +   same = buffer[4] & 0xf;
> +   if (same == 0 || same > 3) {
> +   sd_printk(KERN_WARNING, sdkp,
> + "REPORT ZONES SAME type %d not supported\n", same);
> +   return;
> +   }

It's a bit unfortunate that you abort here. The current Seagate Host
Aware drives
must report a same code of 0

Re: [PATCH 1/5] sd: configure ZBC devices

2016-08-01 Thread Hannes Reinecke 

On 08/01/2016 04:24 PM, Shaun Tancheff wrote:

On Tue, Jul 19, 2016 at 8:25 AM, Hannes Reinecke  wrote:

For ZBC devices I/O must not cross zone boundaries, so setup
the 'chunk_sectors' block queue setting to the zone size.
This is only valid for REPORT ZONES SAME type 2 or 3;
for other types the zone sizes might be different
for individual zones. So issue a warning if the type is
found to be different.
Also the capacity might be different from the announced
capacity, so adjust it as needed.

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/sd.c | 120 --
 drivers/scsi/sd.h |  12 +
 include/scsi/scsi_proto.h |  17 +++
 3 files changed, 144 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 428c03e..249ea81 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1972,6 +1972,57 @@ sd_spinup_disk(struct scsi_disk *sdkp)
}
 }

+/**
+ * sd_zbc_report_zones - Issue a REPORT ZONES scsi command
+ * @sdkp: SCSI disk to which the command should be send
+ * @buffer: response buffer
+ * @bufflen: length of @buffer
+ * @start_sector: logical sector for the zone information should be reported
+ * @option: option for report zones command
+ * @partial: flag to set 'partial' bit for report zones command
+ */
+static int
+sd_zbc_report_zones(struct scsi_disk *sdkp, unsigned char *buffer,
+   int bufflen, sector_t start_sector,
+   enum zbc_zone_reporting_options option, bool partial)
+{
+   struct scsi_device *sdp = sdkp->device;
+   const int timeout = sdp->request_queue->rq_timeout
+   * SD_FLUSH_TIMEOUT_MULTIPLIER;
+   struct scsi_sense_hdr sshdr;
+   sector_t start_lba = sectors_to_logical(sdkp->device, start_sector);
+   unsigned char cmd[16];
+   int result;
+
+   if (!scsi_device_online(sdp)) {
+   sd_printk(KERN_INFO, sdkp, "device not online\n");
+   return -ENODEV;
+   }
+
+   sd_printk(KERN_INFO, sdkp, "REPORT ZONES lba %zu len %d\n",
+ start_lba, bufflen);
+
+   memset(cmd, 0, 16);
+   cmd[0] = ZBC_IN;
+   cmd[1] = ZI_REPORT_ZONES;
+   put_unaligned_be64(start_lba, [2]);
+   put_unaligned_be32(bufflen, [10]);
+   cmd[14] = (partial ? ZBC_REPORT_ZONE_PARTIAL : 0) | option;
+   memset(buffer, 0, bufflen);
+
+   result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
+ buffer, bufflen, ,
+ timeout, SD_MAX_RETRIES, NULL);
+
+   if (result) {
+   sd_printk(KERN_NOTICE, sdkp,
+ "REPORT ZONES lba %zu failed with %d/%d\n",
+ start_lba, host_byte(result), driver_byte(result));
+
+   return -EIO;
+   }
+   return 0;
+}

 /*
  * Determine whether disk supports Data Integrity Field.
@@ -2014,6 +2065,59 @@ static int sd_read_protection_type(struct scsi_disk 
*sdkp, unsigned char *buffer
return ret;
 }

+static void sd_read_zones(struct scsi_disk *sdkp, unsigned char *buffer)
+{
+   int retval;
+   unsigned char *desc;
+   u32 rep_len;
+   u8 same;
+   u64 zone_len, lba;
+
+   if (sdkp->zoned != 1)
+   /* Device managed, no special handling required */
+   return;
+
+   retval = sd_zbc_report_zones(sdkp, buffer, SD_BUF_SIZE,
+0, ZBC_ZONE_REPORTING_OPTION_ALL, false);
+   if (retval < 0)
+   return;
+
+   rep_len = get_unaligned_be32([0]);
+   if (rep_len < 64) {
+   sd_printk(KERN_WARNING, sdkp,
+ "REPORT ZONES report invalid length %u\n",
+ rep_len);
+   return;
+   }
+
+   if (sdkp->rc_basis == 0) {
+   /* The max_lba field is the capacity of a zoned device */
+   lba = get_unaligned_be64([8]);
+   if (lba + 1 > sdkp->capacity) {
+   sd_printk(KERN_WARNING, sdkp,
+ "Max LBA %zu (capacity %zu)\n",
+ (sector_t) lba + 1, sdkp->capacity);
+   sdkp->capacity = lba + 1;
+   }
+   }
+
+   /*
+* Adjust 'chunk_sectors' to the zone length if the device
+* supports equal zone sizes.
+*/
+   same = buffer[4] & 0xf;
+   if (same == 0 || same > 3) {
+   sd_printk(KERN_WARNING, sdkp,
+ "REPORT ZONES SAME type %d not supported\n", same);
+   return;
+   }


It's a bit unfortunate that you abort here. The current Seagate Host
Aware drives
must report a same code of 0 here due to the final 'runt' zone and are therefore
not supported by your RB-Tree in the following patches.


Hmm. Yes, I am aware that Seagate is using '0' here.
However, I'm about to redo my patchset

Re: [PATCH 1/5] sd: configure ZBC devices

2016-07-25 Thread Ewan D. Milne 
On Mon, 2016-07-25 at 08:00 +0200, Hannes Reinecke wrote:
> On 07/24/2016 12:04 AM, Bart Van Assche wrote:
> > On 07/23/16 13:31, Hannes Reinecke wrote:
> >> On 07/22/2016 11:56 PM, Ewan D. Milne wrote:
> >>>
> >>> So, blk_queue_chunk_sectors() has:
> >>>
> >>> void blk_queue_chunk_sectors(struct request_queue *q, unsigned int
> >>> chunk_sectors)
> >>> {
> >>> BUG_ON(!is_power_of_2(chunk_sectors));
> >>> q->limits.chunk_sectors = chunk_sectors;
> >>> }
> >>>
> >>> and it seems like if some device reports a non-power-of-2 zone_len
> >>> then we
> >>> will BUG_ON().  Probably would be better if we reported an error
> >>> instead?
> >>>
> >> The ZBC spec mandates that the zone size must be a power of 2.
> >> So I don't have problems with triggering a BUG_ON for non-compliant
> >> drives.
> > 
> > Triggering BUG_ON() if zone_len is not a power of two is completely
> > unacceptable. No matter what zone information a ZBC drive exports that
> > shouldn't result in a kernel oops.
> > 
> Ok, will be fixing this.
> 
> Cheers,
> 
> Hannes

Yes, unfortunately we have too much history with non-compliant devices.
And, I much prefer to avoid crashing the kernel if it is not necessary.

Thanks.

-Ewan


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] sd: configure ZBC devices

2016-07-25 Thread Hannes Reinecke 
On 07/24/2016 12:04 AM, Bart Van Assche wrote:
> On 07/23/16 13:31, Hannes Reinecke wrote:
>> On 07/22/2016 11:56 PM, Ewan D. Milne wrote:
>>> On Tue, 2016-07-19 at 15:25 +0200, Hannes Reinecke wrote:
 For ZBC devices I/O must not cross zone boundaries, so setup
 the 'chunk_sectors' block queue setting to the zone size.
 This is only valid for REPORT ZONES SAME type 2 or 3;
 for other types the zone sizes might be different
 for individual zones. So issue a warning if the type is
 found to be different.
 Also the capacity might be different from the announced
 capacity, so adjust it as needed.

 Signed-off-by: Hannes Reinecke 
>>>
>>> ...
>>>
 +static void sd_read_zones(struct scsi_disk *sdkp, unsigned char
 *buffer)
 +{
 +int retval;
 +unsigned char *desc;
 +u32 rep_len;
 +u8 same;
 +u64 zone_len, lba;
 +
 +if (sdkp->zoned != 1)
 +/* Device managed, no special handling required */
 +return;
 +
 +retval = sd_zbc_report_zones(sdkp, buffer, SD_BUF_SIZE,
 + 0, ZBC_ZONE_REPORTING_OPTION_ALL, false);
 +if (retval < 0)
 +return;
 +
 +rep_len = get_unaligned_be32([0]);
 +if (rep_len < 64) {
 +sd_printk(KERN_WARNING, sdkp,
 +  "REPORT ZONES report invalid length %u\n",
 +  rep_len);
 +return;
 +}
 +
 +if (sdkp->rc_basis == 0) {
 +/* The max_lba field is the capacity of a zoned device */
 +lba = get_unaligned_be64([8]);
 +if (lba + 1 > sdkp->capacity) {
 +sd_printk(KERN_WARNING, sdkp,
 +  "Max LBA %zu (capacity %zu)\n",
 +  (sector_t) lba + 1, sdkp->capacity);
 +sdkp->capacity = lba + 1;
 +}
 +}
 +
 +/*
 + * Adjust 'chunk_sectors' to the zone length if the device
 + * supports equal zone sizes.
 + */
 +same = buffer[4] & 0xf;
 +if (same == 0 || same > 3) {
 +sd_printk(KERN_WARNING, sdkp,
 +  "REPORT ZONES SAME type %d not supported\n", same);
 +return;
 +}
 +/* Read the zone length from the first zone descriptor */
 +desc = [64];
 +zone_len = logical_to_sectors(sdkp->device,
 +  get_unaligned_be64([8]));
 +blk_queue_chunk_sectors(sdkp->disk->queue, zone_len);
 +}
 +
>>>
>>> So, blk_queue_chunk_sectors() has:
>>>
>>> void blk_queue_chunk_sectors(struct request_queue *q, unsigned int
>>> chunk_sectors)
>>> {
>>> BUG_ON(!is_power_of_2(chunk_sectors));
>>> q->limits.chunk_sectors = chunk_sectors;
>>> }
>>>
>>> and it seems like if some device reports a non-power-of-2 zone_len
>>> then we
>>> will BUG_ON().  Probably would be better if we reported an error
>>> instead?
>>>
>> The ZBC spec mandates that the zone size must be a power of 2.
>> So I don't have problems with triggering a BUG_ON for non-compliant
>> drives.
> 
> Triggering BUG_ON() if zone_len is not a power of two is completely
> unacceptable. No matter what zone information a ZBC drive exports that
> shouldn't result in a kernel oops.
> 
Ok, will be fixing this.

Cheers,

Hannes
-- 
Dr. Hannes ReineckezSeries & Storage
h...@suse.com  +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] sd: configure ZBC devices

2016-07-24 Thread Hannes Reinecke 
On 07/24/2016 12:04 AM, Bart Van Assche wrote:
> On 07/23/16 13:31, Hannes Reinecke wrote:
>> On 07/22/2016 11:56 PM, Ewan D. Milne wrote:
>>> On Tue, 2016-07-19 at 15:25 +0200, Hannes Reinecke wrote:
 For ZBC devices I/O must not cross zone boundaries, so setup
 the 'chunk_sectors' block queue setting to the zone size.
 This is only valid for REPORT ZONES SAME type 2 or 3;
 for other types the zone sizes might be different
 for individual zones. So issue a warning if the type is
 found to be different.
 Also the capacity might be different from the announced
 capacity, so adjust it as needed.

 Signed-off-by: Hannes Reinecke 
>>>
>>> ...
>>>
 +static void sd_read_zones(struct scsi_disk *sdkp, unsigned char
 *buffer)
 +{
 +int retval;
 +unsigned char *desc;
 +u32 rep_len;
 +u8 same;
 +u64 zone_len, lba;
 +
 +if (sdkp->zoned != 1)
 +/* Device managed, no special handling required */
 +return;
 +
 +retval = sd_zbc_report_zones(sdkp, buffer, SD_BUF_SIZE,
 + 0, ZBC_ZONE_REPORTING_OPTION_ALL, false);
 +if (retval < 0)
 +return;
 +
 +rep_len = get_unaligned_be32([0]);
 +if (rep_len < 64) {
 +sd_printk(KERN_WARNING, sdkp,
 +  "REPORT ZONES report invalid length %u\n",
 +  rep_len);
 +return;
 +}
 +
 +if (sdkp->rc_basis == 0) {
 +/* The max_lba field is the capacity of a zoned device */
 +lba = get_unaligned_be64([8]);
 +if (lba + 1 > sdkp->capacity) {
 +sd_printk(KERN_WARNING, sdkp,
 +  "Max LBA %zu (capacity %zu)\n",
 +  (sector_t) lba + 1, sdkp->capacity);
 +sdkp->capacity = lba + 1;
 +}
 +}
 +
 +/*
 + * Adjust 'chunk_sectors' to the zone length if the device
 + * supports equal zone sizes.
 + */
 +same = buffer[4] & 0xf;
 +if (same == 0 || same > 3) {
 +sd_printk(KERN_WARNING, sdkp,
 +  "REPORT ZONES SAME type %d not supported\n", same);
 +return;
 +}
 +/* Read the zone length from the first zone descriptor */
 +desc = [64];
 +zone_len = logical_to_sectors(sdkp->device,
 +  get_unaligned_be64([8]));
 +blk_queue_chunk_sectors(sdkp->disk->queue, zone_len);
 +}
 +
>>>
>>> So, blk_queue_chunk_sectors() has:
>>>
>>> void blk_queue_chunk_sectors(struct request_queue *q, unsigned int
>>> chunk_sectors)
>>> {
>>> BUG_ON(!is_power_of_2(chunk_sectors));
>>> q->limits.chunk_sectors = chunk_sectors;
>>> }
>>>
>>> and it seems like if some device reports a non-power-of-2 zone_len
>>> then we
>>> will BUG_ON().  Probably would be better if we reported an error
>>> instead?
>>>
>> The ZBC spec mandates that the zone size must be a power of 2.
>> So I don't have problems with triggering a BUG_ON for non-compliant
>> drives.
> 
> Triggering BUG_ON() if zone_len is not a power of two is completely
> unacceptable. No matter what zone information a ZBC drive exports that
> shouldn't result in a kernel oops.
> 
Okay, I'll be changing it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] sd: configure ZBC devices

2016-07-23 Thread Bart Van Assche 

On 07/23/16 13:31, Hannes Reinecke wrote:

On 07/22/2016 11:56 PM, Ewan D. Milne wrote:

On Tue, 2016-07-19 at 15:25 +0200, Hannes Reinecke wrote:

For ZBC devices I/O must not cross zone boundaries, so setup
the 'chunk_sectors' block queue setting to the zone size.
This is only valid for REPORT ZONES SAME type 2 or 3;
for other types the zone sizes might be different
for individual zones. So issue a warning if the type is
found to be different.
Also the capacity might be different from the announced
capacity, so adjust it as needed.

Signed-off-by: Hannes Reinecke 


...


+static void sd_read_zones(struct scsi_disk *sdkp, unsigned char *buffer)
+{
+   int retval;
+   unsigned char *desc;
+   u32 rep_len;
+   u8 same;
+   u64 zone_len, lba;
+
+   if (sdkp->zoned != 1)
+   /* Device managed, no special handling required */
+   return;
+
+   retval = sd_zbc_report_zones(sdkp, buffer, SD_BUF_SIZE,
+0, ZBC_ZONE_REPORTING_OPTION_ALL, false);
+   if (retval < 0)
+   return;
+
+   rep_len = get_unaligned_be32([0]);
+   if (rep_len < 64) {
+   sd_printk(KERN_WARNING, sdkp,
+ "REPORT ZONES report invalid length %u\n",
+ rep_len);
+   return;
+   }
+
+   if (sdkp->rc_basis == 0) {
+   /* The max_lba field is the capacity of a zoned device */
+   lba = get_unaligned_be64([8]);
+   if (lba + 1 > sdkp->capacity) {
+   sd_printk(KERN_WARNING, sdkp,
+ "Max LBA %zu (capacity %zu)\n",
+ (sector_t) lba + 1, sdkp->capacity);
+   sdkp->capacity = lba + 1;
+   }
+   }
+
+   /*
+* Adjust 'chunk_sectors' to the zone length if the device
+* supports equal zone sizes.
+*/
+   same = buffer[4] & 0xf;
+   if (same == 0 || same > 3) {
+   sd_printk(KERN_WARNING, sdkp,
+ "REPORT ZONES SAME type %d not supported\n", same);
+   return;
+   }
+   /* Read the zone length from the first zone descriptor */
+   desc = [64];
+   zone_len = logical_to_sectors(sdkp->device,
+ get_unaligned_be64([8]));
+   blk_queue_chunk_sectors(sdkp->disk->queue, zone_len);
+}
+


So, blk_queue_chunk_sectors() has:

void blk_queue_chunk_sectors(struct request_queue *q, unsigned int 
chunk_sectors)
{
BUG_ON(!is_power_of_2(chunk_sectors));
q->limits.chunk_sectors = chunk_sectors;
}

and it seems like if some device reports a non-power-of-2 zone_len then we
will BUG_ON().  Probably would be better if we reported an error instead?


The ZBC spec mandates that the zone size must be a power of 2.
So I don't have problems with triggering a BUG_ON for non-compliant
drives.


Triggering BUG_ON() if zone_len is not a power of two is completely 
unacceptable. No matter what zone information a ZBC drive exports that 
shouldn't result in a kernel oops.


Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] sd: configure ZBC devices

2016-07-23 Thread Hannes Reinecke 
On 07/22/2016 11:56 PM, Ewan D. Milne wrote:
> On Tue, 2016-07-19 at 15:25 +0200, Hannes Reinecke wrote:
>> For ZBC devices I/O must not cross zone boundaries, so setup
>> the 'chunk_sectors' block queue setting to the zone size.
>> This is only valid for REPORT ZONES SAME type 2 or 3;
>> for other types the zone sizes might be different
>> for individual zones. So issue a warning if the type is
>> found to be different.
>> Also the capacity might be different from the announced
>> capacity, so adjust it as needed.
>>
>> Signed-off-by: Hannes Reinecke 
> 
> ...
> 
>> +static void sd_read_zones(struct scsi_disk *sdkp, unsigned char *buffer)
>> +{
>> +int retval;
>> +unsigned char *desc;
>> +u32 rep_len;
>> +u8 same;
>> +u64 zone_len, lba;
>> +
>> +if (sdkp->zoned != 1)
>> +/* Device managed, no special handling required */
>> +return;
>> +
>> +retval = sd_zbc_report_zones(sdkp, buffer, SD_BUF_SIZE,
>> + 0, ZBC_ZONE_REPORTING_OPTION_ALL, false);
>> +if (retval < 0)
>> +return;
>> +
>> +rep_len = get_unaligned_be32([0]);
>> +if (rep_len < 64) {
>> +sd_printk(KERN_WARNING, sdkp,
>> +  "REPORT ZONES report invalid length %u\n",
>> +  rep_len);
>> +return;
>> +}
>> +
>> +if (sdkp->rc_basis == 0) {
>> +/* The max_lba field is the capacity of a zoned device */
>> +lba = get_unaligned_be64([8]);
>> +if (lba + 1 > sdkp->capacity) {
>> +sd_printk(KERN_WARNING, sdkp,
>> +  "Max LBA %zu (capacity %zu)\n",
>> +  (sector_t) lba + 1, sdkp->capacity);
>> +sdkp->capacity = lba + 1;
>> +}
>> +}
>> +
>> +/*
>> + * Adjust 'chunk_sectors' to the zone length if the device
>> + * supports equal zone sizes.
>> + */
>> +same = buffer[4] & 0xf;
>> +if (same == 0 || same > 3) {
>> +sd_printk(KERN_WARNING, sdkp,
>> +  "REPORT ZONES SAME type %d not supported\n", same);
>> +return;
>> +}
>> +/* Read the zone length from the first zone descriptor */
>> +desc = [64];
>> +zone_len = logical_to_sectors(sdkp->device,
>> +  get_unaligned_be64([8]));
>> +blk_queue_chunk_sectors(sdkp->disk->queue, zone_len);
>> +}
>> +
> 
> So, blk_queue_chunk_sectors() has:
> 
> void blk_queue_chunk_sectors(struct request_queue *q, unsigned int 
> chunk_sectors)
> {
> BUG_ON(!is_power_of_2(chunk_sectors));
> q->limits.chunk_sectors = chunk_sectors;
> }
> 
> and it seems like if some device reports a non-power-of-2 zone_len then we
> will BUG_ON().  Probably would be better if we reported an error instead?
> 
The ZBC spec mandates that the zone size must be a power of 2.
So I don't have problems with triggering a BUG_ON for non-compliant
drives.

Cheers,

Hannes

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] sd: configure ZBC devices

2016-07-22 Thread Ewan D. Milne 
On Tue, 2016-07-19 at 15:25 +0200, Hannes Reinecke wrote:
> For ZBC devices I/O must not cross zone boundaries, so setup
> the 'chunk_sectors' block queue setting to the zone size.
> This is only valid for REPORT ZONES SAME type 2 or 3;
> for other types the zone sizes might be different
> for individual zones. So issue a warning if the type is
> found to be different.
> Also the capacity might be different from the announced
> capacity, so adjust it as needed.
> 
> Signed-off-by: Hannes Reinecke 

...

> +static void sd_read_zones(struct scsi_disk *sdkp, unsigned char *buffer)
> +{
> + int retval;
> + unsigned char *desc;
> + u32 rep_len;
> + u8 same;
> + u64 zone_len, lba;
> +
> + if (sdkp->zoned != 1)
> + /* Device managed, no special handling required */
> + return;
> +
> + retval = sd_zbc_report_zones(sdkp, buffer, SD_BUF_SIZE,
> +  0, ZBC_ZONE_REPORTING_OPTION_ALL, false);
> + if (retval < 0)
> + return;
> +
> + rep_len = get_unaligned_be32([0]);
> + if (rep_len < 64) {
> + sd_printk(KERN_WARNING, sdkp,
> +   "REPORT ZONES report invalid length %u\n",
> +   rep_len);
> + return;
> + }
> +
> + if (sdkp->rc_basis == 0) {
> + /* The max_lba field is the capacity of a zoned device */
> + lba = get_unaligned_be64([8]);
> + if (lba + 1 > sdkp->capacity) {
> + sd_printk(KERN_WARNING, sdkp,
> +   "Max LBA %zu (capacity %zu)\n",
> +   (sector_t) lba + 1, sdkp->capacity);
> + sdkp->capacity = lba + 1;
> + }
> + }
> +
> + /*
> +  * Adjust 'chunk_sectors' to the zone length if the device
> +  * supports equal zone sizes.
> +  */
> + same = buffer[4] & 0xf;
> + if (same == 0 || same > 3) {
> + sd_printk(KERN_WARNING, sdkp,
> +   "REPORT ZONES SAME type %d not supported\n", same);
> + return;
> + }
> + /* Read the zone length from the first zone descriptor */
> + desc = [64];
> + zone_len = logical_to_sectors(sdkp->device,
> +   get_unaligned_be64([8]));
> + blk_queue_chunk_sectors(sdkp->disk->queue, zone_len);
> +}
> +

So, blk_queue_chunk_sectors() has:

void blk_queue_chunk_sectors(struct request_queue *q, unsigned int 
chunk_sectors)
{
BUG_ON(!is_power_of_2(chunk_sectors));
q->limits.chunk_sectors = chunk_sectors;
}

and it seems like if some device reports a non-power-of-2 zone_len then we
will BUG_ON().  Probably would be better if we reported an error instead?

-Ewan


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] sd: configure ZBC devices

2016-07-19 Thread Damien Le Moal 


On 7/19/16 22:25, Hannes Reinecke wrote:

For ZBC devices I/O must not cross zone boundaries, so setup
the 'chunk_sectors' block queue setting to the zone size.
This is only valid for REPORT ZONES SAME type 2 or 3;
for other types the zone sizes might be different
for individual zones. So issue a warning if the type is
found to be different.
Also the capacity might be different from the announced
capacity, so adjust it as needed.

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/sd.c | 120 --
 drivers/scsi/sd.h |  12 +
 include/scsi/scsi_proto.h |  17 +++
 3 files changed, 144 insertions(+), 5 deletions(-)


Reviewed-by: Damien Le Moal 
Tested-by: Damien Le Moal 

--
Damien Le Moal, Ph.D.
Sr. Manager, System Software Group, HGST Research,
HGST, a Western Digital brand
damien.lem...@hgst.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.hgst.com
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice 
& Disclaimer:

This e-mail and any files transmitted with it may contain confidential or 
legally privileged information of WDC and/or its affiliates, and are intended 
solely for the use of the individual or entity to which they are addressed. If 
you are not the intended recipient, any disclosure, copying, distribution or 
any action taken or omitted to be taken in reliance on it, is prohibited. If 
you have received this e-mail in error, please notify the sender immediately 
and delete the e-mail in its entirety from your system.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] sd: configure ZBC devices

2016-07-19 Thread Hannes Reinecke 
For ZBC devices I/O must not cross zone boundaries, so setup
the 'chunk_sectors' block queue setting to the zone size.
This is only valid for REPORT ZONES SAME type 2 or 3;
for other types the zone sizes might be different
for individual zones. So issue a warning if the type is
found to be different.
Also the capacity might be different from the announced
capacity, so adjust it as needed.

Signed-off-by: Hannes Reinecke 
---
 drivers/scsi/sd.c | 120 --
 drivers/scsi/sd.h |  12 +
 include/scsi/scsi_proto.h |  17 +++
 3 files changed, 144 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 428c03e..249ea81 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1972,6 +1972,57 @@ sd_spinup_disk(struct scsi_disk *sdkp)
}
 }
 
+/**
+ * sd_zbc_report_zones - Issue a REPORT ZONES scsi command
+ * @sdkp: SCSI disk to which the command should be send
+ * @buffer: response buffer
+ * @bufflen: length of @buffer
+ * @start_sector: logical sector for the zone information should be reported
+ * @option: option for report zones command
+ * @partial: flag to set 'partial' bit for report zones command
+ */
+static int
+sd_zbc_report_zones(struct scsi_disk *sdkp, unsigned char *buffer,
+   int bufflen, sector_t start_sector,
+   enum zbc_zone_reporting_options option, bool partial)
+{
+   struct scsi_device *sdp = sdkp->device;
+   const int timeout = sdp->request_queue->rq_timeout
+   * SD_FLUSH_TIMEOUT_MULTIPLIER;
+   struct scsi_sense_hdr sshdr;
+   sector_t start_lba = sectors_to_logical(sdkp->device, start_sector);
+   unsigned char cmd[16];
+   int result;
+
+   if (!scsi_device_online(sdp)) {
+   sd_printk(KERN_INFO, sdkp, "device not online\n");
+   return -ENODEV;
+   }
+
+   sd_printk(KERN_INFO, sdkp, "REPORT ZONES lba %zu len %d\n",
+ start_lba, bufflen);
+
+   memset(cmd, 0, 16);
+   cmd[0] = ZBC_IN;
+   cmd[1] = ZI_REPORT_ZONES;
+   put_unaligned_be64(start_lba, [2]);
+   put_unaligned_be32(bufflen, [10]);
+   cmd[14] = (partial ? ZBC_REPORT_ZONE_PARTIAL : 0) | option;
+   memset(buffer, 0, bufflen);
+
+   result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
+ buffer, bufflen, ,
+ timeout, SD_MAX_RETRIES, NULL);
+
+   if (result) {
+   sd_printk(KERN_NOTICE, sdkp,
+ "REPORT ZONES lba %zu failed with %d/%d\n",
+ start_lba, host_byte(result), driver_byte(result));
+
+   return -EIO;
+   }
+   return 0;
+}
 
 /*
  * Determine whether disk supports Data Integrity Field.
@@ -2014,6 +2065,59 @@ static int sd_read_protection_type(struct scsi_disk 
*sdkp, unsigned char *buffer
return ret;
 }
 
+static void sd_read_zones(struct scsi_disk *sdkp, unsigned char *buffer)
+{
+   int retval;
+   unsigned char *desc;
+   u32 rep_len;
+   u8 same;
+   u64 zone_len, lba;
+
+   if (sdkp->zoned != 1)
+   /* Device managed, no special handling required */
+   return;
+
+   retval = sd_zbc_report_zones(sdkp, buffer, SD_BUF_SIZE,
+0, ZBC_ZONE_REPORTING_OPTION_ALL, false);
+   if (retval < 0)
+   return;
+
+   rep_len = get_unaligned_be32([0]);
+   if (rep_len < 64) {
+   sd_printk(KERN_WARNING, sdkp,
+ "REPORT ZONES report invalid length %u\n",
+ rep_len);
+   return;
+   }
+
+   if (sdkp->rc_basis == 0) {
+   /* The max_lba field is the capacity of a zoned device */
+   lba = get_unaligned_be64([8]);
+   if (lba + 1 > sdkp->capacity) {
+   sd_printk(KERN_WARNING, sdkp,
+ "Max LBA %zu (capacity %zu)\n",
+ (sector_t) lba + 1, sdkp->capacity);
+   sdkp->capacity = lba + 1;
+   }
+   }
+
+   /*
+* Adjust 'chunk_sectors' to the zone length if the device
+* supports equal zone sizes.
+*/
+   same = buffer[4] & 0xf;
+   if (same == 0 || same > 3) {
+   sd_printk(KERN_WARNING, sdkp,
+ "REPORT ZONES SAME type %d not supported\n", same);
+   return;
+   }
+   /* Read the zone length from the first zone descriptor */
+   desc = [64];
+   zone_len = logical_to_sectors(sdkp->device,
+ get_unaligned_be64([8]));
+   blk_queue_chunk_sectors(sdkp->disk->queue, zone_len);
+}
+
 static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device 
*sdp,
struct scsi_sense_hdr *sshdr, int sense_valid,