Partitions and end-to-end protection
Hi All,
I've had a problem reported against the NVMe driver on the v4.1 kernel. The user has formatted a namespace with 512 byte sectors, separate buffer 8 byte meta-data and protection type 1.
That should all be okay, and indeed reads and writes work fine. However, if a partition table is created in the namespace and then the machine is rebooted, or the driver is unloaded and loaded, the partitions don't appear again unless they are manually probed.
What seems to be happening is that the driver starts off by setting the capacity to zero whilst it validates the meta-data configuration and then sets the real size after this validation. However, setting the capacity does not seem to be enough to tell the kernel to re-read the partition table.
Anyone else seeing this?
Thanks,
Paul.
Quoted 5 lines · Paul Grabinar
What seems to be happening is that the driver starts off by setting the capacity to zero whilst it validates the meta-data configuration and then sets the real size after this validation. However, setting the capacity does not seem to be enough to tell the kernel to re-read the partition table.
The driver did it this way because we need a disk allocated before we can subscribe to block integrity extensions. I think we can fix the partition issue by making a call to blkdev_reread_part() right after the second revalidate_disk if the namespace is a meta-data formatted. I'll test it out and post a patch if it works out.
Quoted 7 lines · Keith Busch
Quoted 5 lines · Paul Grabinar
What seems to be happening is that the driver starts off by setting the capacity to zero whilst it validates the meta-data configuration and then sets the real size after this validation. However, setting the capacity does not seem to be enough to tell the kernel to re-read the partition table.
Quoted 6 lines
The driver did it this way because we need a disk allocated before we can subscribe to block integrity extensions. I think we can fix the partition issue by making a call to
blkdev_reread_part()right after the secondrevalidate_diskif the namespace is a meta-data formatted. I'll test it out and post a patch if it works out.
Thanks Keith.
Quoted 7 lines · Keith Busch
Quoted 5 lines · Paul Grabinar
What seems to be happening is that the driver starts off by setting the capacity to zero whilst it validates the meta-data configuration and then sets the real size after this validation. However, setting the capacity does not seem to be enough to tell the kernel to re-read the partition table.
Quoted 5 lines
The driver did it this way because we need a disk allocated before we can subscribe to block integrity extensions. I think we can fix the partition issue by making a call to
blkdev_reread_part()right after the secondrevalidate_diskif the namespace is a meta-data formatted. I'll test it out and post a patch if it works out.
I dont think requiring this crazy scheme is something that should spread in drivers. I'd suggest you work with Martin (on Cc) so that we can set up the integrity profile at the right time.
Quoted 14 lines · Christoph Hellwig
Quoted 7 lines · Keith Busch
Quoted 5 lines · Paul Grabinar
What seems to be happening is that the driver starts off by setting the capacity to zero whilst it validates the meta-data configuration and then sets the real size after this validation. However, setting the capacity does not seem to be enough to tell the kernel to re-read the partition table.
Quoted 5 lines
The driver did it this way because we need a disk allocated before we can subscribe to block integrity extensions. I think we can fix the partition issue by making a call to
blkdev_reread_part()right after the secondrevalidate_diskif the namespace is a meta-data formatted. I'll test it out and post a patch if it works out.
Quoted 4 lines
I dont think requiring this crazy scheme is something that should spread in drivers. I'd suggest you work with Martin (on Cc) so that we can set up the integrity profile at the right time.
We got bit by this in the nvdimm enabling as well, it is mostly hidden by the fact that sd triggers revalidate_disk() by default.
Quoted 2 lines · Dan Williams
We got bit by this in the nvdimm enabling as well, it is mostly hidden by the fact that sd triggers
revalidate_disk()by default.
But that's just revalidate_disk, which shouldn't be require but isn't too mad. blkdev_reread_part and the setup work for it is a totally different league.
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Dan Williams
We got bit by this in the nvdimm enabling as well, it is mostly hidden by the fact that sd triggers
revalidate_disk()by default.But that's just
revalidate_disk, which shouldn't be require but isn't too mad.blkdev_reread_partand the setup work for it is a totally different league.
Agreed, something is wrong if we need to do bdget() in the driver.
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Christoph" == Christoph Hellwig <hch at infradead.org> writes:
The driver did it this way because we need a disk allocated before we can subscribe to block integrity extensions.
The gendisk is a prerequisite by virtue of the integrity profile hanging off of it. What I don't understand is why you do this capacity zeroing/revalidate business in the first place?
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 7 lines · Martin K. Petersen
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Christoph" == Christoph Hellwig <hch at infradead.org> writes:
The driver did it this way because we need a disk allocated before we can subscribe to block integrity extensions.
The gendisk is a prerequisite by virtue of the integrity profile hanging off of it.
Having one allocated is. But why do we need to have it added (at which point it is live)?
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Christoph" == Christoph Hellwig <hch at infradead.org> writes:
The gendisk is a prerequisite by virtue of the integrity profile hanging off of it.
Christoph> Having one allocated is. But why do we need to have it added Christoph> (at which point it is live)?
Well, we're poking at the queue (for block size) and the bdi. I guess we could split things up and have another function to twiddle these parameters separately. During revalidate, perhaps.
But the fundamental question remains: Why is it a prerequisite for the NVMe integrity profile to be added before the disk is live? Nobody else requires this (sd, md, dm).
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 3 lines · Martin K. Petersen
But the fundamental question remains: Why is it a prerequisite for the NVMe integrity profile to be added before the disk is live? Nobody else requires this (sd, md, dm).
NVMe doesn't provide a way to disable transferring metadata per-io. Capacity zero prevents IO when it's added because we don't have a metadata buffer to assign CMD.MPTR. The drive will unconditionally attempt DMA to/from MPTR, so it's potential memory and data corruption if we let it happen.
An alternative to requiring blk integrity provide the metadata buffer, we could have the driver allocate a large scratch buffer just so we have something valid to set MPTR even. I didn't do that initially because we can't use the metadata for anything at that point, and it seems odd to have the driver implement this special case just to satisfy add_disk().
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Keith" == Keith Busch <keith.busch at intel.com> writes:
Keith> NVMe doesn't provide a way to disable transferring metadata Keith> per-io.
If you do a READ with PRACT=1 the PI should get stripped at the controller. That was the intent, anyway.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 7 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Keith" == Keith Busch <keith.busch at intel.com> writes:
Keith> NVMe doesn't provide a way to disable transferring metadata Keith> per-io.
If you do a READ with PRACT=1 the PI should get stripped at the controller. That was the intent, anyway.
PRACT=1 strips/generates PI only for a subset of valid NVMe meta-data formats, so we need something for the others.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Keith" == Keith Busch <keith.busch at intel.com> writes:
Keith> PRACT=1 strips/generates PI only for a subset of valid NVMe Keith> meta-data formats, so we need something for the others.
*sigh* I hate that non-PI metadata stuff so much.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 5 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Keith" == Keith Busch <keith.busch at intel.com> writes:
Keith> PRACT=1 strips/generates PI only for a subset of valid NVMe Keith> meta-data formats, so we need something for the others.
*sigh* I hate that non-PI metadata stuff so much.
And it gets even worse! PI formatted drives with metadata size > 8 bytes don't use PRACT either.