[PATCHv2] nvme: only use power of two io boundaries

[PATCHv2] nvme: only use power of two io boundaries

[Keith Busch]

The kernel requires a power of two for boundaries because that's the
only way it can efficiently split commands that cross them. A
controller, however, may report a non-power of two boundary.

The driver had been rounding the controller's value to one the kernel
can use, but splitting on the wrong boundary provides no benefit on the
device side, and actually incurs additional submission overhead from
non-optimal splits. Don't provide any boundary hint if the controller's
value can't be used and log a warning when initially observing a disk's
unreported io boundary.

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

  Warn for the namespace with unusable io boundary.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 154942fc64eb..9bbbd600621f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2097,8 +2097,12 @@ static int __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
 		}
 	}
 
-	if (iob && !blk_queue_is_zoned(ns->queue))
-		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
+	if (is_power_of_2(iob) && !blk_queue_is_zoned(ns->queue))
+		blk_queue_chunk_sectors(ns->queue, iob);
+	else if (iob && !(disk->flags & GENHD_FL_UP))
+		dev_warn(ctrl->device, "namespace:%u has unused io boundary:%u\n",
+			ns->head->ns_id, 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: [PATCHv2] nvme: only use power of two io boundaries

[Martin K. Petersen]

Keith,

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 154942fc64eb..9bbbd600621f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2097,8 +2097,12 @@ static int __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
>  		}
>  	}
>  
> -	if (iob && !blk_queue_is_zoned(ns->queue))
> -		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> +	if (is_power_of_2(iob) && !blk_queue_is_zoned(ns->queue))
> +		blk_queue_chunk_sectors(ns->queue, iob);
> +	else if (iob && !(disk->flags & GENHD_FL_UP))
> +		dev_warn(ctrl->device, "namespace:%u has unused io boundary:%u\n",

You used "unusable" in the patch description above. Did you mean that
here too?

Maybe something like:

	"namespace%u: ignoring unsupported I/O boundary:%u\n"

to make it clear that this is due to a kernel limitation and not the
drive reporting garbage.

> +			ns->head->ns_id, iob);
> +
>  	nvme_update_disk_info(disk, ns, id);
>  #ifdef CONFIG_NVME_MULTIPATH
>  	if (ns->head->disk) {

Otherwise OK.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Keith Busch]

On Thu, Aug 27, 2020 at 11:48:21AM -0400, Martin K. Petersen wrote:
> You used "unusable" in the patch description above. Did you mean that
> here too?
> 
> Maybe something like:
> 
> 	"namespace%u: ignoring unsupported I/O boundary:%u\n"
> 
> to make it clear that this is due to a kernel limitation and not the
> drive reporting garbage.

Thanks, your suggestion definitely improves the clarity. I'll use this
for a v3.

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Christoph Hellwig]

On Thu, Aug 27, 2020 at 08:28:24AM -0700, Keith Busch wrote:
> +	if (is_power_of_2(iob) && !blk_queue_is_zoned(ns->queue))
> +		blk_queue_chunk_sectors(ns->queue, iob);
> +	else if (iob && !(disk->flags & GENHD_FL_UP))
> +		dev_warn(ctrl->device, "namespace:%u has unused io boundary:%u\n",
> +			ns->head->ns_id, iob);
> +

Similar to the zoned stuff I just find this super hard to read.

I wonder if we need to untangle this a little.  Something like

static inline bool nvme_first_scan(struct nvme_ns *)
{
	/* add a comment here */
	return !(ns->disk->flags & GENHD_FL_UP)
}

static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
{
	u32 chunk_sectors = nvme_lba_to_sect(ns, le16_to_cpu(id->noiob));

	if ((ctrl->quirks & NVME_QUIRK_STRIPE_SIZE) &&
	    is_power_of_2(ctrl->max_hw_sectors))
	    	chunk_sectors = ctrl->max_hw_sectors;

	if (!chunk_sectors)
		return;

	if (!is_power_of_2(chunk_sectors)) {
		if (nvme_first_scan(ns)) {
			dev_warn(ctrl->device,
				"namespace%u: ignoring unaligned I/O boundary: %u\n",
				ns->head->ns_id, chunk_sectors);
		}
		return;
	}

	if (blk_queue_is_zoned(ns->queue)) {
		if (nvme_first_scan(ns)) {
			dev_info(ctrl->device,
				"namespace%u: ignoring I/O boundary for zoned namespace\n",
				ns->head->ns_id);
		}
		return;
	}
			
	blk_queue_chunk_sectors(ns->queue, chunk_sectors);
}

>  	nvme_update_disk_info(disk, ns, id);
>  #ifdef CONFIG_NVME_MULTIPATH
>  	if (ns->head->disk) {
> -- 
> 2.24.1
---end quoted text---

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Keith Busch]

On Thu, Aug 27, 2020 at 06:06:44PM +0200, Christoph Hellwig wrote:
> On Thu, Aug 27, 2020 at 08:28:24AM -0700, Keith Busch wrote:
> > +	if (is_power_of_2(iob) && !blk_queue_is_zoned(ns->queue))
> > +		blk_queue_chunk_sectors(ns->queue, iob);
> > +	else if (iob && !(disk->flags & GENHD_FL_UP))
> > +		dev_warn(ctrl->device, "namespace:%u has unused io boundary:%u\n",
> > +			ns->head->ns_id, iob);
> > +
> 
> Similar to the zoned stuff I just find this super hard to read.
> 
> I wonder if we need to untangle this a little.  Something like

That's fine. Do you want to see a prep patch splitting chunk settings
first then power-of-2 check second, or okay as a single patch?

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Christoph Hellwig]

On Thu, Aug 27, 2020 at 09:44:05AM -0700, Keith Busch wrote:
> On Thu, Aug 27, 2020 at 06:06:44PM +0200, Christoph Hellwig wrote:
> > On Thu, Aug 27, 2020 at 08:28:24AM -0700, Keith Busch wrote:
> > > +	if (is_power_of_2(iob) && !blk_queue_is_zoned(ns->queue))
> > > +		blk_queue_chunk_sectors(ns->queue, iob);
> > > +	else if (iob && !(disk->flags & GENHD_FL_UP))
> > > +		dev_warn(ctrl->device, "namespace:%u has unused io boundary:%u\n",
> > > +			ns->head->ns_id, iob);
> > > +
> > 
> > Similar to the zoned stuff I just find this super hard to read.
> > 
> > I wonder if we need to untangle this a little.  Something like
> 
> That's fine. Do you want to see a prep patch splitting chunk settings
> first then power-of-2 check second, or okay as a single patch?

I don't really care, whatever suits best.

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Damien Le Moal]

On 2020/08/28 0:35, Keith Busch wrote:
> The kernel requires a power of two for boundaries because that's the
> only way it can efficiently split commands that cross them. A
> controller, however, may report a non-power of two boundary.

I can understand that using a power of 2 limit for splitting can be more
efficient CPU-wise, but I do not think the kernel "requires" such a limit
alignment for splitting BIOs. BLK_SAFE_MAX_SECTORS is 255 after all. What I am
missing here ?

> 
> The driver had been rounding the controller's value to one the kernel
> can use, but splitting on the wrong boundary provides no benefit on the
> device side, and actually incurs additional submission overhead from
> non-optimal splits. Don't provide any boundary hint if the controller's
> value can't be used and log a warning when initially observing a disk's
> unreported io boundary.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> v1->v2:
> 
>   Warn for the namespace with unusable io boundary.
> 
>  drivers/nvme/host/core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 154942fc64eb..9bbbd600621f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2097,8 +2097,12 @@ static int __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
>  		}
>  	}
>  
> -	if (iob && !blk_queue_is_zoned(ns->queue))
> -		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> +	if (is_power_of_2(iob) && !blk_queue_is_zoned(ns->queue))
> +		blk_queue_chunk_sectors(ns->queue, iob);
> +	else if (iob && !(disk->flags & GENHD_FL_UP))
> +		dev_warn(ctrl->device, "namespace:%u has unused io boundary:%u\n",
> +			ns->head->ns_id, iob);

Isn't this going to generate a warning all the time for a ZNS drive ?

> +
>  	nvme_update_disk_info(disk, ns, id);
>  #ifdef CONFIG_NVME_MULTIPATH
>  	if (ns->head->disk) {
> 


-- 
Damien Le Moal
Western Digital Research

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Keith Busch]

On Fri, Aug 28, 2020 at 01:03:30AM +0000, Damien Le Moal wrote:
> On 2020/08/28 0:35, Keith Busch wrote:
> > The kernel requires a power of two for boundaries because that's the
> > only way it can efficiently split commands that cross them. A
> > controller, however, may report a non-power of two boundary.
> 
> I can understand that using a power of 2 limit for splitting can be more
> efficient CPU-wise, but I do not think the kernel "requires" such a limit
> alignment for splitting BIOs. BLK_SAFE_MAX_SECTORS is 255 after all. What I am
> missing here ?

Ah, those two limits are a bit different. The max sectors just checks
length, but chunk_sectors needs to consider length and offset, so
checking if an IO crosses the boundary requires division. For CPU
efficiency, power of 2 alignment is enforced through a BUG_ON() in
blk_queue_chunk_sectors().
 
> > -	if (iob && !blk_queue_is_zoned(ns->queue))
> > -		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
> > +	if (is_power_of_2(iob) && !blk_queue_is_zoned(ns->queue))
> > +		blk_queue_chunk_sectors(ns->queue, iob);
> > +	else if (iob && !(disk->flags & GENHD_FL_UP))
> > +		dev_warn(ctrl->device, "namespace:%u has unused io boundary:%u\n",
> > +			ns->head->ns_id, iob);
> 
> Isn't this going to generate a warning all the time for a ZNS drive ?

Yes, if a ZNS drive reports NOIOB, this will warn on it's initial
discovery.

As long as zone sizes reuse the queue's chunk_sectors, the zone size
needs to take precedence since it's more than just a hint compared to
NOIOB. If people complain about the warning, that may indicate we ought
to find a way to get around overloading the chunk_sectors queue limit,
but I suspect ZNS drives won't report an iob value.

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Damien Le Moal]

On 2020/08/28 13:16, Keith Busch wrote:
> On Fri, Aug 28, 2020 at 01:03:30AM +0000, Damien Le Moal wrote:
>> On 2020/08/28 0:35, Keith Busch wrote:
>>> The kernel requires a power of two for boundaries because that's the
>>> only way it can efficiently split commands that cross them. A
>>> controller, however, may report a non-power of two boundary.
>>
>> I can understand that using a power of 2 limit for splitting can be more
>> efficient CPU-wise, but I do not think the kernel "requires" such a limit
>> alignment for splitting BIOs. BLK_SAFE_MAX_SECTORS is 255 after all. What I am
>> missing here ?
> 
> Ah, those two limits are a bit different. The max sectors just checks
> length, but chunk_sectors needs to consider length and offset, so
> checking if an IO crosses the boundary requires division. For CPU
> efficiency, power of 2 alignment is enforced through a BUG_ON() in
> blk_queue_chunk_sectors().

OK. Got it.

>  
>>> -	if (iob && !blk_queue_is_zoned(ns->queue))
>>> -		blk_queue_chunk_sectors(ns->queue, rounddown_pow_of_two(iob));
>>> +	if (is_power_of_2(iob) && !blk_queue_is_zoned(ns->queue))
>>> +		blk_queue_chunk_sectors(ns->queue, iob);
>>> +	else if (iob && !(disk->flags & GENHD_FL_UP))
>>> +		dev_warn(ctrl->device, "namespace:%u has unused io boundary:%u\n",
>>> +			ns->head->ns_id, iob);
>>
>> Isn't this going to generate a warning all the time for a ZNS drive ?
> 
> Yes, if a ZNS drive reports NOIOB, this will warn on it's initial
> discovery.
> 
> As long as zone sizes reuse the queue's chunk_sectors, the zone size
> needs to take precedence since it's more than just a hint compared to
> NOIOB. If people complain about the warning, that may indicate we ought
> to find a way to get around overloading the chunk_sectors queue limit,
> but I suspect ZNS drives won't report an iob value.

Understood. But for ZNS, since the zone size would take precedence over iob for
chunk sectors, wouldn't it make sense to have the entire code block under a
if (!blk_queue_is_zoned(ns->queue)) ? Something like this:

	if (!blk_queue_is_zoned(ns->queue)) {
		if (is_power_of_2(iob))
			blk_queue_chunk_sectors(ns->queue, iob);
		else if (iob && !(disk->flags & GENHD_FL_UP))
			dev_warn(ctrl->device,
				"namespace:%u has unused io boundary:%u\n",
				ns->head->ns_id, iob);
	}

Thanks !

-- 
Damien Le Moal
Western Digital Research

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Keith Busch]

On Fri, Aug 28, 2020 at 04:30:02AM +0000, Damien Le Moal wrote:
> On 2020/08/28 13:16, Keith Busch wrote:
> > As long as zone sizes reuse the queue's chunk_sectors, the zone size
> > needs to take precedence since it's more than just a hint compared to
> > NOIOB. If people complain about the warning, that may indicate we ought
> > to find a way to get around overloading the chunk_sectors queue limit,
> > but I suspect ZNS drives won't report an iob value.
> 
> Understood. But for ZNS, since the zone size would take precedence over iob for
> chunk sectors, wouldn't it make sense to have the entire code block under a
> if (!blk_queue_is_zoned(ns->queue)) ? Something like this:
> 
> 	if (!blk_queue_is_zoned(ns->queue)) {
> 		if (is_power_of_2(iob))
> 			blk_queue_chunk_sectors(ns->queue, iob);
> 		else if (iob && !(disk->flags & GENHD_FL_UP))
> 			dev_warn(ctrl->device,
> 				"namespace:%u has unused io boundary:%u\n",
> 				ns->head->ns_id, iob);
> 	}

Oh, I see. I think we should have the warning for ZNS too, though, just
to get an indication that devices really want this kind of
consideration.

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

Re: [PATCHv2] nvme: only use power of two io boundaries

[Damien Le Moal]

On 2020/08/28 13:44, Keith Busch wrote:
> On Fri, Aug 28, 2020 at 04:30:02AM +0000, Damien Le Moal wrote:
>> On 2020/08/28 13:16, Keith Busch wrote:
>>> As long as zone sizes reuse the queue's chunk_sectors, the zone size
>>> needs to take precedence since it's more than just a hint compared to
>>> NOIOB. If people complain about the warning, that may indicate we ought
>>> to find a way to get around overloading the chunk_sectors queue limit,
>>> but I suspect ZNS drives won't report an iob value.
>>
>> Understood. But for ZNS, since the zone size would take precedence over iob for
>> chunk sectors, wouldn't it make sense to have the entire code block under a
>> if (!blk_queue_is_zoned(ns->queue)) ? Something like this:
>>
>> 	if (!blk_queue_is_zoned(ns->queue)) {
>> 		if (is_power_of_2(iob))
>> 			blk_queue_chunk_sectors(ns->queue, iob);
>> 		else if (iob && !(disk->flags & GENHD_FL_UP))
>> 			dev_warn(ctrl->device,
>> 				"namespace:%u has unused io boundary:%u\n",
>> 				ns->head->ns_id, iob);
>> 	}
> 
> Oh, I see. I think we should have the warning for ZNS too, though, just
> to get an indication that devices really want this kind of
> consideration.

I see. The use of chunk_sectors for both the zone size and iob renders iob
impossible to satisfy. So indeed it makes sense to get the warning. Thanks !

-- 
Damien Le Moal
Western Digital Research

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