How about loosen restriction of VPD83 id to support NVMe disks.

[cathay4t]

@joehandzik @tasleson

Currently, we are defining the VPD83 ID as VPD83 NAA ID which is not supported by NVMe disks yet.

The NVMe disks only expose SCSI VPD83 string ID which translate from NVMe serial:

[fge@megadeth ~]$ sudo sg_vpd --page=0x83 /dev/nvme0n1
Device Identification VPD page:
  Addressed logical unit:
    designator type: SCSI name string,  code set: UTF-8
      SCSI name string:
      8086INTEL SSDPEDMD400G4                     1000CVFT5194002T400BGN  

How about we make an exception in lsm_local_disk_vpd83_get() and lsm_local_disk_vpd83_search()
to allow VPD83 sting ID as fallback?

[joehandzik]

I think that's fine, Gris. I saw some similar trickery in Ceph's spdk usage.

Joe

On May 2, 2016, at 7:21 AM, Gris Ge <[email protected]mailto:[email protected]> wrote:

@joehandzikhttps://github.com/joehandzik @taslesonhttps://github.com/tasleson

Currently, we are defining the VPD83 ID as VPD83 NAA ID which is not supported by NVMe disks yet.

The NVMe disks only expose SCSI VPD83 string ID which translate from NVMe serial:

[fge@megadeth ~]$ sudo sg_vpd --page=0x83 /dev/nvme0n1
Device Identification VPD page:
Addressed logical unit:
designator type: SCSI name string, code set: UTF-8
SCSI name string:
8086INTEL SSDPEDMD400G4 1000CVFT5194002T400BGN

How about we make an exception in lsm_local_disk_vpd83_get() and lsm_local_disk_vpd83_search()
to allow VPD83 sting ID as fallback?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//issues/135

[cathay4t]

Another update on NVMe, with NVMe 1.2+(not sure what does this mean yet), linux kernel expose VPD83 EUI64 ID for NVMe disks.

[joehandzik]

I guess we have three options (as far as I see):

1. Stick with the VPD83 string ID for all NVMe devices (I'm assuming they still expose it even with NVMe 1.2+)
2. Make the EUI64 ID the desired ID for NVMe devices, and fall back to the VPD83 string ID when it's not present (kind of like how we handle the the sas address sysfs attribute versus the value from the page inquiry)
3. Mark NVMe as unsupported for 1.3, and take our time to find the right solution in 1.4.

I think option 2 would be fine, especially if we somehow communicate via documentation that NVMe support is very early and the management standards are still working their way into reality. But I can see an argument for 3.

[cathay4t]

From the kernel code, it seems once spec 1.2 is supported, EUI64 ID is replacing string ID. So option 1) out.

Using option 2) will alter the definition of lsm_local_disk_vpd83_search() and also make it complex[1].

Let's just add this line into lsm_local_disk_vpd83_get() and lsm_local_disk_vpd83_search():

NVMe is not supported yet, the definition of _vpd83_get() and _vpd83_search() might be changed in the future version in order to support it, but action of SCSI disk and ATA disk will not be changed

[1]: Currently _vpd83_get() and _vpd83_search() does not require root privilege considering wide use of it. But EUI64 or string ID require SGIO to retrieve them, so user might get different result with/without root privilege on _vpd83_get() or _search() against a NVMe disk or NVMe ID. Yeah, I will try to send patch to systemd udev to expose those ID via udev.

[joehandzik]

@cathay4t Ok, gotcha. It does sound complex and awfully in-flux. We'll see what @tasleson thinks, but for now I'm with you and leaning towards keeping NVMe unsupported. It could be made use of in Ceph today, but most other scale-out solutions are still using SAS/SATA for cost reasons. I think we're in great shape if we can support NVMe well within 2016, even if that is in release 1.4.

[tasleson]

I agree, lets leave it unsupported at this time.

[cathay4t]

@joehandzik Any change to review my code in libnvme? I am planing to use it for NVMe support in libstoragemgmt.

[joehandzik]

@cathay4t Hey, sorry for the delay, back online after the holiday break. I'll take a look and see if I find anything. I assume all this code will be brought into libstoragemgmt? Or will your libnvme remain a separate repository?

[joehandzik]

@cathay4t An initial pass through the source went fine, I think it looks really good actually! I may have more detailed feedback when we see how it fits in with libstoragemgmt itself. But I think it's in a state where a PR would be the next step. Do you happen to have documentation on what hardware you're testing this against? We have some NVMe here at HPE, but I would find it useful to have an initial list of hardware you've tested against.

[cathay4t]

@joehandzik The libnvme will be in separate library in order to expose more features than libstoragemgmt needs.

I only tested on Intel DC P3700 which is all I have right now, it's a old NVMe SSD which only support NVMe SPEC 1.0. I have updated the README.md file of libnvme to note down tested hardwares.

The code has zero vendor specific code, purely based on NVMe SPEC 1.0+. You make try these commands on your gears:

./autogen.sh
./configure
make run

For fitting into libstoragemgmt:

• Add NVMe support to disk WWID/VPD83 query and other properties query.
• Add NVMe support to disk LED notification. (Simply blink PCI LED, no need to use SES, but I have no hardware to code on yet).

[yosshy]

BTW, storcli for MegaRAID cards can return WNN = NA for disks and it causes INVALID_ARGUMENT error on "lsmcli ld" like below:

$ sudo /opt/MegaRAID/storcli/storcli64 /call/eall/sall show all | less
...
Drive /c0/e252/s0 Device attributes :
===================================
SN = GTF402P6GV8J0F
Manufacturer Id = ATA
Model Number = Hitachi HUA721050KLA330
NAND Vendor = NA
WWN = NA
Raw size = 465.661 GB [0x3a352d30 Sectors]
Coerced size = 465.156 GB [0x3a250000 Sectors]
Non Coerced size = 465.161 GB [0x3a252d30 Sectors]
Device Speed = 3.0Gb/s
Link Speed = 3.0Gb/s
Write Cache = N/A
Logical Sector Size = 512B
Physical Sector Size = 512B
Connector Name =
...
$ export LSMCLI_URI=megaraid://
$ sudo -E lsmcli ld
INVALID_ARGUMENT(101): Incorrect format of VPD 0x83 NAA(3) string: 'na', expecting 32 or 16 lower case hex characters

[cathay4t]

@yosshy I am creating new issue to trace your issue at #376