diff mbox series

[2/4] block: Fix validation of zoned device with a runt zone

Message ID 20240530054035.491497-3-dlemoal@kernel.org (mailing list archive)
State New, archived
Headers show
Series Zone write plugging and DM zone fixes | expand

Commit Message

Damien Le Moal May 30, 2024, 5:40 a.m. UTC
Commit ecfe43b11b02 ("block: Remember zone capacity when revalidating
zones") introduced checks to ensure that the capacity of the zones of
a zoned device is constant for all zones. However, this check ignores
the possibility that a zoned device has a smaller last zone with a size
not equal to the capacity of other zones. Such device correspond in
practice to an SMR drive with a smaller last zone and all zones with a
capacity equal to the zone size, leading to the last zone capacity being
different than the capacity of other zones.

Correctly handle such device by fixing the check for the constant zone
capacity in blk_revalidate_seq_zone() using the new helper function
disk_zone_is_last(). This helper function is also used in
blk_revalidate_zone_cb() when checking the zone size.

Fixes: ecfe43b11b02 ("block: Remember zone capacity when revalidating zones")
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-zoned.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Niklas Cassel May 30, 2024, 7:37 a.m. UTC | #1
On Thu, May 30, 2024 at 02:40:33PM +0900, Damien Le Moal wrote:
> Commit ecfe43b11b02 ("block: Remember zone capacity when revalidating
> zones") introduced checks to ensure that the capacity of the zones of
> a zoned device is constant for all zones. However, this check ignores
> the possibility that a zoned device has a smaller last zone with a size
> not equal to the capacity of other zones. Such device correspond in
> practice to an SMR drive with a smaller last zone and all zones with a
> capacity equal to the zone size, leading to the last zone capacity being
> different than the capacity of other zones.
> 
> Correctly handle such device by fixing the check for the constant zone
> capacity in blk_revalidate_seq_zone() using the new helper function
> disk_zone_is_last(). This helper function is also used in
> blk_revalidate_zone_cb() when checking the zone size.
> 
> Fixes: ecfe43b11b02 ("block: Remember zone capacity when revalidating zones")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  block/blk-zoned.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 03aa4eead39e..402a50a1ac4d 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -450,6 +450,11 @@ static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector)
>  	return test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap);
>  }
>  
> +static bool disk_zone_is_last(struct gendisk *disk, struct blk_zone *zone)
> +{
> +	return zone->start + zone->len >= get_capacity(disk);
> +}
> +
>  static bool disk_insert_zone_wplug(struct gendisk *disk,
>  				   struct blk_zone_wplug *zwplug)
>  {
> @@ -1693,11 +1698,13 @@ static int blk_revalidate_seq_zone(struct blk_zone *zone, unsigned int idx,
>  
>  	/*
>  	 * Remember the capacity of the first sequential zone and check
> -	 * if it is constant for all zones.
> +	 * if it is constant for all zones, ignoring the last zone as it can be
> +	 * smaller.
>  	 */
>  	if (!args->zone_capacity)
>  		args->zone_capacity = zone->capacity;
> -	if (zone->capacity != args->zone_capacity) {
> +	if (!disk_zone_is_last(disk, zone) &&
> +	    zone->capacity != args->zone_capacity) {
>  		pr_warn("%s: Invalid variable zone capacity\n",
>  			disk->disk_name);
>  		return -ENODEV;
> @@ -1732,7 +1739,6 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
>  {
>  	struct blk_revalidate_zone_args *args = data;
>  	struct gendisk *disk = args->disk;
> -	sector_t capacity = get_capacity(disk);
>  	sector_t zone_sectors = disk->queue->limits.chunk_sectors;
>  	int ret;
>  
> @@ -1743,7 +1749,7 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
>  		return -ENODEV;
>  	}
>  
> -	if (zone->start >= capacity || !zone->len) {
> +	if (zone->start >= get_capacity(disk) || !zone->len) {
>  		pr_warn("%s: Invalid zone start %llu, length %llu\n",
>  			disk->disk_name, zone->start, zone->len);
>  		return -ENODEV;
> @@ -1753,7 +1759,7 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
>  	 * All zones must have the same size, with the exception on an eventual
>  	 * smaller last zone.
>  	 */
> -	if (zone->start + zone->len < capacity) {
> +	if (!disk_zone_is_last(disk, zone)) {
>  		if (zone->len != zone_sectors) {
>  			pr_warn("%s: Invalid zoned device with non constant zone size\n",
>  				disk->disk_name);
> -- 
> 2.45.1
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>
Bart Van Assche May 30, 2024, 8:37 p.m. UTC | #2
On 5/29/24 22:40, Damien Le Moal wrote:
> Commit ecfe43b11b02 ("block: Remember zone capacity when revalidating
> zones") introduced checks to ensure that the capacity of the zones of
> a zoned device is constant for all zones. However, this check ignores
> the possibility that a zoned device has a smaller last zone with a size
> not equal to the capacity of other zones. Such device correspond in
> practice to an SMR drive with a smaller last zone and all zones with a
> capacity equal to the zone size, leading to the last zone capacity being
> different than the capacity of other zones.
> 
> Correctly handle such device by fixing the check for the constant zone
> capacity in blk_revalidate_seq_zone() using the new helper function
> disk_zone_is_last(). This helper function is also used in
> blk_revalidate_zone_cb() when checking the zone size.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

> @@ -1732,7 +1739,6 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
>   {
>   	struct blk_revalidate_zone_args *args = data;
>   	struct gendisk *disk = args->disk;
> -	sector_t capacity = get_capacity(disk);
>   	sector_t zone_sectors = disk->queue->limits.chunk_sectors;
>   	int ret;

Thank you for having removed the local variable with the name 'capacity' from
this function. Having a variable with the name 'capacity' in a function that
deals both with disk capacity and zone capacity was confusing ...

Bart.
Christoph Hellwig June 1, 2024, 5:26 a.m. UTC | #3
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Hannes Reinecke June 3, 2024, 6:55 a.m. UTC | #4
On 5/30/24 07:40, Damien Le Moal wrote:
> Commit ecfe43b11b02 ("block: Remember zone capacity when revalidating
> zones") introduced checks to ensure that the capacity of the zones of
> a zoned device is constant for all zones. However, this check ignores
> the possibility that a zoned device has a smaller last zone with a size
> not equal to the capacity of other zones. Such device correspond in
> practice to an SMR drive with a smaller last zone and all zones with a
> capacity equal to the zone size, leading to the last zone capacity being
> different than the capacity of other zones.
> 
> Correctly handle such device by fixing the check for the constant zone
> capacity in blk_revalidate_seq_zone() using the new helper function
> disk_zone_is_last(). This helper function is also used in
> blk_revalidate_zone_cb() when checking the zone size.
> 
> Fixes: ecfe43b11b02 ("block: Remember zone capacity when revalidating zones")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   block/blk-zoned.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
diff mbox series

Patch

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 03aa4eead39e..402a50a1ac4d 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -450,6 +450,11 @@  static inline bool disk_zone_is_conv(struct gendisk *disk, sector_t sector)
 	return test_bit(disk_zone_no(disk, sector), disk->conv_zones_bitmap);
 }
 
+static bool disk_zone_is_last(struct gendisk *disk, struct blk_zone *zone)
+{
+	return zone->start + zone->len >= get_capacity(disk);
+}
+
 static bool disk_insert_zone_wplug(struct gendisk *disk,
 				   struct blk_zone_wplug *zwplug)
 {
@@ -1693,11 +1698,13 @@  static int blk_revalidate_seq_zone(struct blk_zone *zone, unsigned int idx,
 
 	/*
 	 * Remember the capacity of the first sequential zone and check
-	 * if it is constant for all zones.
+	 * if it is constant for all zones, ignoring the last zone as it can be
+	 * smaller.
 	 */
 	if (!args->zone_capacity)
 		args->zone_capacity = zone->capacity;
-	if (zone->capacity != args->zone_capacity) {
+	if (!disk_zone_is_last(disk, zone) &&
+	    zone->capacity != args->zone_capacity) {
 		pr_warn("%s: Invalid variable zone capacity\n",
 			disk->disk_name);
 		return -ENODEV;
@@ -1732,7 +1739,6 @@  static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 {
 	struct blk_revalidate_zone_args *args = data;
 	struct gendisk *disk = args->disk;
-	sector_t capacity = get_capacity(disk);
 	sector_t zone_sectors = disk->queue->limits.chunk_sectors;
 	int ret;
 
@@ -1743,7 +1749,7 @@  static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 		return -ENODEV;
 	}
 
-	if (zone->start >= capacity || !zone->len) {
+	if (zone->start >= get_capacity(disk) || !zone->len) {
 		pr_warn("%s: Invalid zone start %llu, length %llu\n",
 			disk->disk_name, zone->start, zone->len);
 		return -ENODEV;
@@ -1753,7 +1759,7 @@  static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 	 * All zones must have the same size, with the exception on an eventual
 	 * smaller last zone.
 	 */
-	if (zone->start + zone->len < capacity) {
+	if (!disk_zone_is_last(disk, zone)) {
 		if (zone->len != zone_sectors) {
 			pr_warn("%s: Invalid zoned device with non constant zone size\n",
 				disk->disk_name);