Re: [PATCH RFC] nvme-multipath: fix diskstats for partitions

Re: [PATCH RFC] nvme-multipath: fix diskstats for partitions

[John Garry]

On 24/07/2026 06:01, Christoph Hellwig wrote:
> On Tue, Jul 21, 2026 at 11: 45: 53AM +0000, John Garry wrote: > Setting as an
> RFC as adding this extra bio field is not acceptable, but I > can't see how to
> lookup the original partition. In general it is not, but you should mark this a
> 
> 
> On Tue, Jul 21, 2026 at 11:45:53AM +0000, John Garry wrote:
>> Setting as an RFC as adding this extra bio field is not acceptable, but I
>> can't see how to lookup the original partition.
> 
> In general it is not, but you should mark this a block patch so that
> Jens can better cream at you :)
> 
> I'm also not sure that supporting per-partition diskstats on a multipath
> device makes too much sense, but then again there's a lot of setups
> that are crazy and actually used..
> 

It just seems to me that we should have same behaviour as if it were not 
multipath.

So we can't use bi_private as that can be set by original bio submitter. 
I was thinking that this bdev pointer could be temp stashed in bi_next 
(as it should be originally NULL), but that it dodgy and maybe won't 
even work.

Least worst I can think if is to alloc some temp memory per-bio in 
nvme_ns_head_submit_bio() to hold this bdev pointer and original 
bi_private, set bio->bi_private to that memory, and then set bi_private 
back to original when we end the bio. But this is all crappy, especially 
just for partition diskstats.

Re: [PATCH RFC] nvme-multipath: fix diskstats for partitions

[Keith Busch]

On Fri, Jul 24, 2026 at 08:09:51AM +0100, John Garry wrote:
> On 24/07/2026 06:01, Christoph Hellwig wrote:
> > On Tue, Jul 21, 2026 at 11: 45: 53AM +0000, John Garry wrote: > Setting as an
> > RFC as adding this extra bio field is not acceptable, but I > can't see how to
> > lookup the original partition. In general it is not, but you should mark this a
> > 
> > 
> > On Tue, Jul 21, 2026 at 11:45:53AM +0000, John Garry wrote:
> > > Setting as an RFC as adding this extra bio field is not acceptable, but I
> > > can't see how to lookup the original partition.
> > 
> > In general it is not, but you should mark this a block patch so that
> > Jens can better cream at you :)
> > 
> > I'm also not sure that supporting per-partition diskstats on a multipath
> > device makes too much sense, but then again there's a lot of setups
> > that are crazy and actually used..
> > 
> 
> It just seems to me that we should have same behaviour as if it were not
> multipath.
> 
> So we can't use bi_private as that can be set by original bio submitter. I
> was thinking that this bdev pointer could be temp stashed in bi_next (as it
> should be originally NULL), but that it dodgy and maybe won't even work.

Failover is corner case to consider here. We always reset the bio bdev
to the part0, so your stats will be incorrect when that happens. I think
you can quickly fix that in your proposal with the "bi_orig", though.

Can we just thread through partitions for the bio's block_device instead
of assuming part0? I know the hidden path devices skip partition
scanning, but maybe if we let it happen, then those will have the same
partition setup as the head gendisk. Then we can go right to the
disk->part_tbl for what we provide to bio_set_dev() for both submission
and failover, and everything should work out from there.

Re: [PATCH RFC] nvme-multipath: fix diskstats for partitions

[John Garry]

On 7/24/26 13:45, Keith Busch wrote:
> On Fri, Jul 24, 2026 at 08:09:51AM +0100, John Garry wrote:
>> On 24/07/2026 06:01, Christoph Hellwig wrote:
>>> On Tue, Jul 21, 2026 at 11: 45: 53AM +0000, John Garry wrote: > Setting as an
>>> RFC as adding this extra bio field is not acceptable, but I > can't see how to
>>> lookup the original partition. In general it is not, but you should mark this a
>>>
>>>
>>> On Tue, Jul 21, 2026 at 11:45:53AM +0000, John Garry wrote:
>>>> Setting as an RFC as adding this extra bio field is not acceptable, but I
>>>> can't see how to lookup the original partition.
>>>
>>> In general it is not, but you should mark this a block patch so that
>>> Jens can better cream at you :)
>>>
>>> I'm also not sure that supporting per-partition diskstats on a multipath
>>> device makes too much sense, but then again there's a lot of setups
>>> that are crazy and actually used..
>>>
>>
>> It just seems to me that we should have same behaviour as if it were not
>> multipath.
>>
>> So we can't use bi_private as that can be set by original bio submitter. I
>> was thinking that this bdev pointer could be temp stashed in bi_next (as it
>> should be originally NULL), but that it dodgy and maybe won't even work.
> 
> Failover is corner case to consider here. We always reset the bio bdev
> to the part0, so your stats will be incorrect when that happens. I think
> you can quickly fix that in your proposal with the "bi_orig", though.
> 
> Can we just thread through partitions for the bio's block_device instead
> of assuming part0? I know the hidden path devices skip partition
> scanning, but maybe if we let it happen, then those will have the same
> partition setup as the head gendisk. Then we can go right to the
> disk->part_tbl for what we provide to bio_set_dev() for both submission
> and failover, and everything should work out from there.

I guess that we would just scan through the per-path gendisk->part_tbl 
and match somehow to lookup the partition. Maybe vs start address of 
bio->bi_bdev. Or is there a simpler (and quicker) way?

Re: [PATCH RFC] nvme-multipath: fix diskstats for partitions

[Keith Busch]

On Fri, Jul 24, 2026 at 04:29:08PM +0100, John Garry wrote:
> On 7/24/26 13:45, Keith Busch wrote:
> > Can we just thread through partitions for the bio's block_device instead
> > of assuming part0? I know the hidden path devices skip partition
> > scanning, but maybe if we let it happen, then those will have the same
> > partition setup as the head gendisk. Then we can go right to the
> > disk->part_tbl for what we provide to bio_set_dev() for both submission
> > and failover, and everything should work out from there.
> 
> I guess that we would just scan through the per-path gendisk->part_tbl and
> match somehow to lookup the partition. Maybe vs start address of
> bio->bi_bdev. Or is there a simpler (and quicker) way?
> 

This is the idea, assuming we can get the partition tables of the head
and path disks to be aligned:

---
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -543,7 +543,10 @@ static void nvme_ns_head_submit_bio(struct bio *bio)
        srcu_idx = srcu_read_lock(&head->srcu);
        ns = nvme_find_path(head);
        if (likely(ns)) {
-               bio_set_dev(bio, ns->disk->part0);
+               struct block_device *bdev;
+
+               bdev = xa_load(&ns->disk->part_tbl, bdev_partno(bio->bi_bdev));
+               bio_set_dev(bio, bdev);
                /*
                 * Use BIO_REMAPPED to skip bio_check_eod() when this bio
                 * enters submit_bio_noacct() for the per-path device. The EOD
--

Re: [PATCH RFC] nvme-multipath: fix diskstats for partitions

[John Garry]

On 7/24/26 16:37, Keith Busch wrote:
> On Fri, Jul 24, 2026 at 04:29:08PM +0100, John Garry wrote:
>> On 7/24/26 13:45, Keith Busch wrote:
>>> Can we just thread through partitions for the bio's block_device instead
>>> of assuming part0? I know the hidden path devices skip partition
>>> scanning, but maybe if we let it happen, then those will have the same
>>> partition setup as the head gendisk. Then we can go right to the
>>> disk->part_tbl for what we provide to bio_set_dev() for both submission
>>> and failover, and everything should work out from there.
>>
>> I guess that we would just scan through the per-path gendisk->part_tbl and
>> match somehow to lookup the partition. Maybe vs start address of
>> bio->bi_bdev. Or is there a simpler (and quicker) way?
>>
> 
> This is the idea, assuming we can get the partition tables of the head
> and path disks to be aligned:
> 
> ---
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -543,7 +543,10 @@ static void nvme_ns_head_submit_bio(struct bio *bio)
>          srcu_idx = srcu_read_lock(&head->srcu);
>          ns = nvme_find_path(head);
>          if (likely(ns)) {
> -               bio_set_dev(bio, ns->disk->part0);
> +               struct block_device *bdev;
> +
> +               bdev = xa_load(&ns->disk->part_tbl, bdev_partno(bio->bi_bdev));
> +               bio_set_dev(bio, bdev);
>                  /*
>                   * Use BIO_REMAPPED to skip bio_check_eod() when this bio
>                   * enters submit_bio_noacct() for the per-path device. The EOD
> --

ok, got it.

And I think that we would need to do the reverse lookup in 
nvme_mpath_start_request() and nvme_mpath_end_request() to get the head 
disk partition, like:
bdev = xa_load(&ns->head->disk->part_tbl, bdev_partno(bio->bi_bdev));

I suppose that the tricky part now would be have the per-path disk scan 
run but keep those per-path disks hidden.

I experimented by stop setting GENHD_FL_HIDDEN for the per-path disk, 
and the diskstats look ok, FWIW.

Re: [PATCH RFC] nvme-multipath: fix diskstats for partitions

[John Garry]

On 24/07/2026 17:56, John Garry wrote:
>> This is the idea, assuming we can get the partition tables of the head
>> and path disks to be aligned:
>> 
>> ---
>> --- a/drivers/nvme/host/multipath.c
>> +++ b/drivers/nvme/host/multipath.c
>> @@ -543,7 +543,10 @@ static void nvme_ns_head_submit_bio(struct bio *bio)
>>          srcu_idx = srcu_read_lock(&head->srcu);
>>          ns = nvme_find_path(head);
>>          if (likely(ns)) {
>> -               bio_set_dev(bio, ns->disk->part0);
>> +               struct block_device *bdev;
>> +
>> +               bdev = xa_load(&ns->disk->part_tbl, bdev_partno(bio->bi_bdev));
>> +               bio_set_dev(bio, bdev);
>>                  /*
>>                   * Use BIO_REMAPPED to skip bio_check_eod() when this bio
>>                   * enters submit_bio_noacct() for the per-path device. The EOD
>> --
> 
> ok, got it.
> 
> And I think that we would need to do the reverse lookup in
> nvme_mpath_start_request() and nvme_mpath_end_request() to get the head
> disk partition, like:
> bdev = xa_load(&ns->head->disk->part_tbl, bdev_partno(bio->bi_bdev));
> 
> I suppose that the tricky part now would be have the per-path disk scan
> run but keep those per-path disks hidden.
> 
> I experimented by stop setting GENHD_FL_HIDDEN for the per-path disk,
> and the diskstats look ok, FWIW.

Here's an attempt to get the disk partition tables for hidden disks:

--->8----

 From d7e124982215648ae914a1fb48f34f915f94eb60 Mon Sep 17 00:00:00 2001
From: John Garry <john.garry@linux.dev>
Date: Thu, 30 Jul 2026 13:14:32 +0100
Subject: [PATCH] block: scan partitions for hidden disks

If a disk is hidden (GENHD_FL_HIDDEN flags set), we currently do not 
scan the disk partition table.

GENHD_FL_HIDDEN is used in NVMe multipath support to hide the per-path
disk.

However, it would be useful there to actually have the per-path disk
partition table available for situations where we want to send bios to
specific per-path disk partition.

Change GENHD_FL_HIDDEN to scan partitions. For anyone wanting to avoid
scanning the partition (and keep the disk hidden), flag GENHD_FL_NO_PART 
can still be used.

Signed-off-by: John Garry <john.g.garry@oracle.com>

diff --git a/block/genhd.c b/block/genhd.c
index df2c3c69b467..64469943fbfe 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -417,6 +417,10 @@ static void add_disk_final(struct gendisk *disk)
  		 */
  		dev_set_uevent_suppress(ddev, 0);
  		disk_uevent(disk, KOBJ_ADD);
+	} else {
+		bdev_add(disk->part0, ddev->devt);
+		if (get_capacity(disk))
+			disk_scan_partitions(disk, BLK_OPEN_READ);
  	}

  	blk_apply_bdi_limits(disk->bdi, &disk->queue->limits);
@@ -487,8 +491,7 @@ static int __add_disk(struct device *parent, struct 
gendisk *disk,
  	dev_set_name(ddev, "%s", disk->disk_name);
  	if (fwnode)
  		device_set_node(ddev, fwnode);
-	if (!(disk->flags & GENHD_FL_HIDDEN))
-		ddev->devt = MKDEV(disk->major, disk->first_minor);
+	ddev->devt = MKDEV(disk->major, disk->first_minor);
  	ret = device_add(ddev);
  	if (ret)
  		goto out_free_ext_minor;
diff --git a/block/partitions/core.c b/block/partitions/core.c
index b5c59b79ca7c..a48896c6c791 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -152,7 +152,8 @@ static struct parsed_partitions 
*check_partition(struct gendisk *hd)

  	}
  	if (res > 0) {
-		printk(KERN_INFO "%s", seq_buf_str(&state->pp_buf));
+		if (!(hd->flags & GENHD_FL_HIDDEN))
+			printk(KERN_INFO "%s", seq_buf_str(&state->pp_buf));

  		kfree(state->pp_buf.buffer);
  		return state;
@@ -164,7 +165,7 @@ static struct parsed_partitions 
*check_partition(struct gendisk *hd)
  	 */
  	if (err)
  		res = err;
-	if (res) {
+	if (res && !(hd->flags & GENHD_FL_HIDDEN)) {
  		seq_buf_puts(&state->pp_buf,
  			     " unable to read partition table\n");
  		printk(KERN_INFO "%s", seq_buf_str(&state->pp_buf));
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 453c1f0b2dd0..869fd05019d2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1790,10 +1790,6 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)

  static int nvme_ns_open(struct nvme_ns *ns)
  {
-
-	/* should never be called due to GENHD_FL_HIDDEN */
-	if (WARN_ON_ONCE(nvme_ns_head_multipath(ns->head)))
-		goto fail;
  	if (!nvme_get_ns(ns))
  		goto fail;
  	if (!try_module_get(ns->ctrl->ops->module))
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 9213a5716f95..a270cf1394c7 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -260,7 +260,7 @@ static inline unsigned int disk_openers(struct 
gendisk *disk)
   */
  static inline bool disk_has_partscan(struct gendisk *disk)
  {
-	return !(disk->flags & (GENHD_FL_NO_PART | GENHD_FL_HIDDEN)) &&
+	return !(disk->flags & GENHD_FL_NO_PART) &&
  		!test_bit(GD_SUPPRESS_PART_SCAN, &disk->state);
  }

-- 
2.43.0

---8<---

With that, /sys/class/block/ looks a bit different (with the per-path 
partitions):

#ls -l /sys/class/block/ | grep nvme0
lrwxrwxrwx    1 root     root             0 Jul 30 12:39 nvme0c0n1 ->
../../devices/virtual/nvme-fabrics/ctl/nvme0/nvme0c0n1
lrwxrwxrwx    1 root     root             0 Jul 30 12:39 nvme0c0n1p1
-> ../../devices/virtual/nvme-fabrics/ctl/nvme0/nvme0c0n1/nvme0c0n1p1
lrwxrwxrwx    1 root     root             0 Jul 30 12:39 nvme0c1n1 ->
../../devices/virtual/nvme-fabrics/ctl/nvme1/nvme0c1n1
lrwxrwxrwx    1 root     root             0 Jul 30 12:39 nvme0c1n1p1
-> ../../devices/virtual/nvme-fabrics/ctl/nvme1/nvme0c1n1/nvme0c1n1p1
lrwxrwxrwx    1 root     root             0 Jul 30 12:39 nvme0n1 ->
../../devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1
lrwxrwxrwx    1 root     root             0 Jul 30 12:39 nvme0n1p1 ->
../../devices/virtual/nvme-subsystem/nvme-subsys0/nvme0n1/nvme0n1p1

Thoughts?