Re: [dm-devel] [PATCH 1/4] block: Enhance blk_revalidate_disk_zones()

2019-10-24 Thread Christoph Hellwig
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 4bc5f260248a..293891b7068a 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -441,6 +441,57 @@ void blk_queue_free_zone_bitmaps(struct request_queue *q)
>   q->seq_zones_wlock = NULL;
>  }
>  
> +/**
> + * blk_check_zone - Check a zone information
> + * @q:   request queue
> + * @zone:the zone to  check
> + * @sector:  start sector of the zone
> + *
> + * Helper function to check zones of a zoned block device. Returns true if 
> the
> + * zone is correct and false if a problem is detected.
> + */
> +static bool blk_check_zone(struct gendisk *disk, struct blk_zone *zone,
> +sector_t *sector)

Maybe call this blk_zone_valid?  Also in many places we don't really
do the verbose kerneldoc comments with the duplicated parameter names
for local functions, as the scripts only pick up non-stack ones anyway.

> + /*
> +  * All zones must have the same size, with the exception on an eventual
> +  * smaller last zone.
> +  */
> + if (zone->start + zone_sectors < capacity &&
> + zone->len != zone_sectors) {
> + pr_warn("%s: Invalid zone device with non constant zone size\n",
> + disk->disk_name);
> + return false;
> + }

I think we should also move the power of two zone size check here
instead of leaving it in the driver.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



Re: [dm-devel] [PATCH 1/4] block: Enhance blk_revalidate_disk_zones()

2019-10-24 Thread Keith Busch
On Thu, Oct 24, 2019 at 03:50:03PM +0900, Damien Le Moal wrote:
> - /* Do a report zone to get max_lba and the same field */
> - ret = sd_zbc_do_report_zones(sdkp, buf, bufsize, 0, false);
> + /* Do a report zone to get max_lba and the size of the first zone */
> + ret = sd_zbc_do_report_zones(sdkp, buf, SD_BUF_SIZE, 0, false);

This is no longer reading all the zones here, so you could set the
'partial' field to true. And then since this was the only caller that
had set partial to false, you can also remove that parameter.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



Re: [dm-devel] [PATCH 1/4] block: Enhance blk_revalidate_disk_zones()

2019-10-24 Thread Damien Le Moal
On 2019/10/24 16:08, Christoph Hellwig wrote:
>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>> index 4bc5f260248a..293891b7068a 100644
>> --- a/block/blk-zoned.c
>> +++ b/block/blk-zoned.c
>> @@ -441,6 +441,57 @@ void blk_queue_free_zone_bitmaps(struct request_queue 
>> *q)
>>  q->seq_zones_wlock = NULL;
>>  }
>>  
>> +/**
>> + * blk_check_zone - Check a zone information
>> + * @q:  request queue
>> + * @zone:   the zone to  check
>> + * @sector: start sector of the zone
>> + *
>> + * Helper function to check zones of a zoned block device. Returns true if 
>> the
>> + * zone is correct and false if a problem is detected.
>> + */
>> +static bool blk_check_zone(struct gendisk *disk, struct blk_zone *zone,
>> +   sector_t *sector)
> 
> Maybe call this blk_zone_valid?  Also in many places we don't really
> do the verbose kerneldoc comments with the duplicated parameter names
> for local functions, as the scripts only pick up non-stack ones anyway.
> 
>> +/*
>> + * All zones must have the same size, with the exception on an eventual
>> + * smaller last zone.
>> + */
>> +if (zone->start + zone_sectors < capacity &&
>> +zone->len != zone_sectors) {
>> +pr_warn("%s: Invalid zone device with non constant zone size\n",
>> +disk->disk_name);
>> +return false;
>> +}
> 
> I think we should also move the power of two zone size check here
> instead of leaving it in the driver.

We should not since the drivers calls blk_queue_chunk_sectors() first to
set the zone size and that function has a power of 2 check. So better
check that earlier than here.

-- 
Damien Le Moal
Western Digital Research

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel