[PATCHv3] nvme: skip noiob for zoned devices

[PATCHv3] nvme: skip noiob for zoned devices

[Keith Busch]

Zoned block devices reuse the chunk_sectors queue limit to define zone
boundaries. If a such a device happens to also report an optimal
boundary, log a warning of the conflicting attributes and do not use
that to define the chunk_sectors as that may intermittently interfere
with io splitting and zone size queries.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v2 -> v3:

 Emit a warning on the initial validation if a zoned device reports an
 optimal boundary.

 drivers/nvme/host/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 88cff309d8e4..10cf40d2cfef 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2075,8 +2075,13 @@ static int __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
 		}
 	}
 
-	if (iob)
-		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
+	if (iob) {
+		if (!blk_queue_is_zoned(disk->queue))
+			blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
+		else if (!(disk->flags & GENHD_FL_UP))
+			pr_warn("%s: zone namespace has unused IO boundary:%u\n",
+				disk->disk_name, iob);
+	}
 	nvme_update_disk_info(disk, ns, id);
 #ifdef CONFIG_NVME_MULTIPATH
 	if (ns->head->disk) {
-- 
2.24.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv3] nvme: skip noiob for zoned devices

[Christoph Hellwig]

On Tue, Aug 18, 2020 at 09:29:41AM -0700, Keith Busch wrote:
> Zoned block devices reuse the chunk_sectors queue limit to define zone
> boundaries. If a such a device happens to also report an optimal
> boundary, log a warning of the conflicting attributes and do not use
> that to define the chunk_sectors as that may intermittently interfere
> with io splitting and zone size queries.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> v2 -> v3:
> 
>  Emit a warning on the initial validation if a zoned device reports an
>  optimal boundary.
> 
>  drivers/nvme/host/core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 88cff309d8e4..10cf40d2cfef 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2075,8 +2075,13 @@ static int __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
>  		}
>  	}
>  
> -	if (iob)
> -		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> +	if (iob) {
> +		if (!blk_queue_is_zoned(disk->queue))
> +			blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> +		else if (!(disk->flags & GENHD_FL_UP))
> +			pr_warn("%s: zone namespace has unused IO boundary:%u\n",
> +				disk->disk_name, iob);

This has some overly long line and looks fairly convoluted, what
about something like:

	if (iob) {
		unsigned int chunk_sectors = rounddown_pow_of_two(iob);

		if (blk_queue_is_zoned(disk->queue) &&
		    chunk_sectors > blk_queue_zone_sectors(disk->queue))
			pr_warn("%s: ignoring too large IO boundary: %u\n",
				disk->disk_name, iob);
		else
			blk_queue_chunk_sectors(ns->queue, chunk_sectors);
	}

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv3] nvme: skip noiob for zoned devices

[Keith Busch]

On Tue, Aug 18, 2020 at 07:25:04PM +0200, Christoph Hellwig wrote:
> > -	if (iob)
> > -		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> > +	if (iob) {
> > +		if (!blk_queue_is_zoned(disk->queue))
> > +			blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> > +		else if (!(disk->flags & GENHD_FL_UP))
> > +			pr_warn("%s: zone namespace has unused IO boundary:%u\n",
> > +				disk->disk_name, iob);
> 
> This has some overly long line and looks fairly convoluted, what
> about something like:
> 
> 	if (iob) {
> 		unsigned int chunk_sectors = rounddown_pow_of_two(iob);

Unrelated to this particular patch, but ... why is this rounddown here?
While we can't use chunk_sectors if it is not a power of two, but what
benefit are we expecting to get from using the wrong boundary just
because it's convenient to our block stack?
 
> 		if (blk_queue_is_zoned(disk->queue) &&
> 		    chunk_sectors > blk_queue_zone_sectors(disk->queue))

I don't think we should skip iob just for the case where chunk_sectors
is the larger value. A smaller value still ruins things like
blk_queue_zone_sectors() and blk_queue_zone_no().

Also, we call blk_revalidate_disk_zones() after the iob check, so the
blk_queue_zone_sectors() hasn't been initialized for this revalidate.

> 			pr_warn("%s: ignoring too large IO boundary: %u\n",
> 				disk->disk_name, iob);

We don't want to emit a repeated warning on every disk revalidation, so
I added the check for GENHD_FL_UP to warn only on the very first disk
validation.

> 		else
> 			blk_queue_chunk_sectors(ns->queue, chunk_sectors);
> 	}

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv3] nvme: skip noiob for zoned devices

[Christoph Hellwig]

On Tue, Aug 18, 2020 at 10:50:54AM -0700, Keith Busch wrote:
> On Tue, Aug 18, 2020 at 07:25:04PM +0200, Christoph Hellwig wrote:
> > > -	if (iob)
> > > -		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> > > +	if (iob) {
> > > +		if (!blk_queue_is_zoned(disk->queue))
> > > +			blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> > > +		else if (!(disk->flags & GENHD_FL_UP))
> > > +			pr_warn("%s: zone namespace has unused IO boundary:%u\n",
> > > +				disk->disk_name, iob);
> > 
> > This has some overly long line and looks fairly convoluted, what
> > about something like:
> > 
> > 	if (iob) {
> > 		unsigned int chunk_sectors = rounddown_pow_of_two(iob);
> 
> Unrelated to this particular patch, but ... why is this rounddown here?
> While we can't use chunk_sectors if it is not a power of two, but what
> benefit are we expecting to get from using the wrong boundary just
> because it's convenient to our block stack?

True, might make sense to add a validity check instead.

>  
> > 		if (blk_queue_is_zoned(disk->queue) &&
> > 		    chunk_sectors > blk_queue_zone_sectors(disk->queue))
> 
> I don't think we should skip iob just for the case where chunk_sectors
> is the larger value. A smaller value still ruins things like
> blk_queue_zone_sectors() and blk_queue_zone_no().
> 
> Also, we call blk_revalidate_disk_zones() after the iob check, so the
> blk_queue_zone_sectors() hasn't been initialized for this revalidate.
> 
> > 			pr_warn("%s: ignoring too large IO boundary: %u\n",
> > 				disk->disk_name, iob);
> 
> We don't want to emit a repeated warning on every disk revalidation, so
> I added the check for GENHD_FL_UP to warn only on the very first disk
> validation.

Ok.  Maybe we just need to move the whole thing into a helper to look
less confusing.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv3] nvme: skip noiob for zoned devices

[Sagi Grimberg]

I'll wait for a new version, reverting the existing patch
from nvme-5.9-rc for now...

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv3] nvme: skip noiob for zoned devices

[Christoph Hellwig]

On Tue, Aug 18, 2020 at 12:10:22PM -0700, Sagi Grimberg wrote:
> I'll wait for a new version, reverting the existing patch
> from nvme-5.9-rc for now...

Let's just go back to v2, sorry for all the fuzz I caused.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv3] nvme: skip noiob for zoned devices

[Sagi Grimberg]

On 8/18/20 12:20 PM, Christoph Hellwig wrote:
> On Tue, Aug 18, 2020 at 12:10:22PM -0700, Sagi Grimberg wrote:
>> I'll wait for a new version, reverting the existing patch
>> from nvme-5.9-rc for now...
> 
> Let's just go back to v2, sorry for all the fuzz I caused.

OK, requeued v2

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme