[libvirt] [RFC 5/5]: Rewrite findLuns function
This rather large patch rewrites the virStorageBackendISCSIFindLUNs() function to only rely on sysfs for finding LUNs, given a session number. Along the way, it also fixes the bug where we wouldn't find LUNs for older kernels (with the block:sda format), and also (possibly) fixes a race condition where we could try to find the LUN before udev has finished connecting it. I say it "possibly" fixes it because I haven't been able to hit it so far, but I definitely need more testing to try and confirm. Signed-off-by: Chris Lalancette <clalance@redhat.com>
+ n = scandir(sysfs_path, &namelist, notdotdir, versionsort); + if (n <= 0) { + /* we didn't find any reasonable entries; return failure */ + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to find any LUNs for session %s: %s"), + session, strerror(errno)); + + return -1; + } Who told you/anyone who wrote this code before that the 0th lun cannot be a real entry? This assumption is wrong. Stefan
Stefan de Konink wrote:
+ n = scandir(sysfs_path, &namelist, notdotdir, versionsort); + if (n <= 0) { + /* we didn't find any reasonable entries; return failure */ + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to find any LUNs for session %s: %s"), + session, strerror(errno)); + + return -1; + }
Who told you/anyone who wrote this code before that the 0th lun cannot be a real entry? This assumption is wrong.
Well, that particular piece of code isn't looking at the 0'th LUN, but there is code below that that is. However, the code here is generally looking for block devices, and the 0'th LUN is definitely not a block device. Given that, I'm not sure what else you would want to do with 0th LUN; can you give me some examples? Chris Lalancette
Chris Lalancette schreef:
Stefan de Konink wrote:
+ n = scandir(sysfs_path, &namelist, notdotdir, versionsort); + if (n <= 0) { + /* we didn't find any reasonable entries; return failure */ + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to find any LUNs for session %s: %s"), + session, strerror(errno)); + + return -1; + }
Who told you/anyone who wrote this code before that the 0th lun cannot be a real entry? This assumption is wrong.
Well, that particular piece of code isn't looking at the 0'th LUN, but there is code below that that is. However, the code here is generally looking for block devices, and the 0'th LUN is definitely not a block device. Given that, I'm not sure what else you would want to do with 0th LUN; can you give me some examples?
The 0th LUN can be a perfect block device. xen01 3:0:0:0 # ls -l total 0 -r--r--r-- 1 root root 4096 Jun 12 16:25 FUA -rw-r--r-- 1 root root 4096 Jun 12 16:25 allow_restart -rw-r--r-- 1 root root 4096 Jun 12 16:25 cache_type lrwxrwxrwx 1 root root 0 Jun 12 16:23 device -> ../../../devices/platform/host3/session3/target3:0:0/3:0:0:0 -rw-r--r-- 1 root root 4096 Jun 12 16:25 manage_start_stop lrwxrwxrwx 1 root root 0 Jun 12 16:25 subsystem -> ../../../class/scsi_disk --w------- 1 root root 4096 Jun 12 16:25 uevent cat /sys/class/iscsi_session/session3/targetname iqn.1986-03.com.sun:02:c6719b9f-7993-444d-c6d2-80d4e15ee52c And the same counts for a netapp target. Stefan
Stefan de Konink wrote:
Who told you/anyone who wrote this code before that the 0th lun cannot be a real entry? This assumption is wrong. Well, that particular piece of code isn't looking at the 0'th LUN, but there is code below that that is. However, the code here is generally looking for block devices, and the 0'th LUN is definitely not a block device. Given that, I'm not sure what else you would want to do with 0th LUN; can you give me some examples?
The 0th LUN can be a perfect block device.
OK, cool, I misunderstood. Given that, can you give me the output of: # ls -l /sys/bus/scsi/devices/3:0:0:0 Just to make sure I get the right thing? Thanks, Chris Lalancette
Chris Lalancette schreef:
Stefan de Konink wrote:
Who told you/anyone who wrote this code before that the 0th lun cannot be a real entry? This assumption is wrong. Well, that particular piece of code isn't looking at the 0'th LUN, but there is code below that that is. However, the code here is generally looking for block devices, and the 0'th LUN is definitely not a block device. Given that, I'm not sure what else you would want to do with 0th LUN; can you give me some examples? The 0th LUN can be a perfect block device.
OK, cool, I misunderstood. Given that, can you give me the output of:
# ls -l /sys/bus/scsi/devices/3:0:0:0
Just to make sure I get the right thing?
skinkie@xen01 ~ $ ls -l /sys/bus/scsi/devices/3:0:0:0 lrwxrwxrwx 1 root root 0 May 26 11:28 /sys/bus/scsi/devices/3:0:0:0 -> ../../../devices/platform/host3/session3/target3:0:0/3:0:0:0 skinkie@xen01 ~ $ ls /sys/devices/platform/host3/session3/target3:0:0/3:0:0:0 block:sdar device_blocked generic iodone_cnt iorequest_cnt queue_depth rescan scsi_level subsystem type vendor delete driver iocounterbits ioerr_cnt model queue_type rev state timeout uevent skinkie@xen01 ~ $ cat /sys/devices/platform/host3/session3/target3:0:0/3:0:0:0/model SOLARIS Stefan
Stefan de Konink schreef:
Chris Lalancette schreef:
Stefan de Konink wrote:
Who told you/anyone who wrote this code before that the 0th lun cannot be a real entry? This assumption is wrong. Well, that particular piece of code isn't looking at the 0'th LUN, but there is code below that that is. However, the code here is generally looking for block devices, and the 0'th LUN is definitely not a block device. Given that, I'm not sure what else you would want to do with 0th LUN; can you give me some examples? The 0th LUN can be a perfect block device.
OK, cool, I misunderstood. Given that, can you give me the output of:
# ls -l /sys/bus/scsi/devices/3:0:0:0
Just to make sure I get the right thing?
skinkie@xen01 ~ $ ls -l /sys/devices/platform/host3/session3/target3:0:0/3:0:0:0 total 0 lrwxrwxrwx 1 root root 0 May 24 16:38 block:sdar -> ../../../../../../block/sdar --w------- 1 root root 4096 Jun 12 16:34 delete -r--r--r-- 1 root root 4096 Jun 12 16:34 device_blocked lrwxrwxrwx 1 root root 0 May 24 16:38 driver -> ../../../../../../bus/scsi/drivers/sd lrwxrwxrwx 1 root root 0 Jun 12 16:34 generic -> ../../../../../../class/scsi_generic/sg44 -r--r--r-- 1 root root 4096 Jun 12 16:34 iocounterbits -r--r--r-- 1 root root 4096 Jun 12 16:34 iodone_cnt -r--r--r-- 1 root root 4096 Jun 12 16:34 ioerr_cnt -r--r--r-- 1 root root 4096 Jun 12 16:34 iorequest_cnt -r--r--r-- 1 root root 4096 May 24 16:38 model -rw-r--r-- 1 root root 4096 Jun 12 16:34 queue_depth -r--r--r-- 1 root root 4096 Jun 12 16:34 queue_type --w------- 1 root root 4096 Jun 12 16:34 rescan -r--r--r-- 1 root root 4096 Jun 12 16:34 rev -r--r--r-- 1 root root 4096 Jun 12 16:34 scsi_level -rw-r--r-- 1 root root 4096 May 26 11:28 state lrwxrwxrwx 1 root root 0 May 24 16:38 subsystem -> ../../../../../../bus/scsi -rw-r--r-- 1 root root 4096 Jun 12 16:34 timeout -r--r--r-- 1 root root 4096 May 24 16:38 type --w------- 1 root root 4096 Jun 12 16:34 uevent -r--r--r-- 1 root root 4096 May 24 16:38 vendor -l was important ;) Stefan
Stefan de Konink wrote:
skinkie@xen01 ~ $ ls -l /sys/devices/platform/host3/session3/target3:0:0/3:0:0:0 total 0 lrwxrwxrwx 1 root root 0 May 24 16:38 block:sdar -> ../../../../../../block/sdar --w------- 1 root root 4096 Jun 12 16:34 delete -r--r--r-- 1 root root 4096 Jun 12 16:34 device_blocked lrwxrwxrwx 1 root root 0 May 24 16:38 driver -> ../../../../../../bus/scsi/drivers/sd lrwxrwxrwx 1 root root 0 Jun 12 16:34 generic -> ../../../../../../class/scsi_generic/sg44 -r--r--r-- 1 root root 4096 Jun 12 16:34 iocounterbits -r--r--r-- 1 root root 4096 Jun 12 16:34 iodone_cnt -r--r--r-- 1 root root 4096 Jun 12 16:34 ioerr_cnt -r--r--r-- 1 root root 4096 Jun 12 16:34 iorequest_cnt -r--r--r-- 1 root root 4096 May 24 16:38 model -rw-r--r-- 1 root root 4096 Jun 12 16:34 queue_depth -r--r--r-- 1 root root 4096 Jun 12 16:34 queue_type --w------- 1 root root 4096 Jun 12 16:34 rescan -r--r--r-- 1 root root 4096 Jun 12 16:34 rev -r--r--r-- 1 root root 4096 Jun 12 16:34 scsi_level -rw-r--r-- 1 root root 4096 May 26 11:28 state lrwxrwxrwx 1 root root 0 May 24 16:38 subsystem -> ../../../../../../bus/scsi -rw-r--r-- 1 root root 4096 Jun 12 16:34 timeout -r--r--r-- 1 root root 4096 May 24 16:38 type --w------- 1 root root 4096 Jun 12 16:34 uevent -r--r--r-- 1 root root 4096 May 24 16:38 vendor
OK, hopefully just one more request; I'm trying to figure out a way to distinguish a disk from a control LUN, since in my case (Linux iscsi target), 0 is a control LUN. Can you give me the output of: # cat /sys/bus/scsi/devices/3:0:0:0/model and # cat /sys/bus/scsi/devices/3:0:0:0/type from both your netapp target and your sun target? Here, at least, for our "control" LUN, model returns "Controller" while for the real LUNs model returns "VIRTUAL-DISK", so I'm hoping I can distinguish somehow based on what is in model, and if not that, based on type. Thanks again for the info, Chris Lalancette
Chris Lalancette schreef:
# cat /sys/bus/scsi/devices/3:0:0:0/model
skinkie@xen01 ~ $ cat /sys/bus/scsi/devices/3:0:0:0/model SOLARIS skinkie@xen01 ~ $ cat /sys/bus/scsi/devices/2\:0\:0\:0/model LUN
and
# cat /sys/bus/scsi/devices/3:0:0:0/type
skinkie@xen01 ~ $ cat /sys/bus/scsi/devices/3:0:0:0/type 0 skinkie@xen01 ~ $ cat /sys/bus/scsi/devices/2\:0\:0\:0/type 0
from both your netapp target and your sun target? Here, at least, for our "control" LUN, model returns "Controller" while for the real LUNs model returns "VIRTUAL-DISK", so I'm hoping I can distinguish somehow based on what is in model, and if not that, based on type.
What does the Controller actually do on a 'Linux' target? Stefan
Stefan de Konink wrote:
from both your netapp target and your sun target? Here, at least, for our "control" LUN, model returns "Controller" while for the real LUNs model returns "VIRTUAL-DISK", so I'm hoping I can distinguish somehow based on what is in model, and if not that, based on type.
What does the Controller actually do on a 'Linux' target?
I haven't the foggiest clue :). Thanks for the info, though; I'll see what I can come up with. Chris Lalancette
On Thu, Jun 12, 2008 at 04:54:07PM +0200, Chris Lalancette wrote:
Stefan de Konink wrote:
skinkie@xen01 ~ $ ls -l /sys/devices/platform/host3/session3/target3:0:0/3:0:0:0 total 0 lrwxrwxrwx 1 root root 0 May 24 16:38 block:sdar -> ../../../../../../block/sdar --w------- 1 root root 4096 Jun 12 16:34 delete -r--r--r-- 1 root root 4096 Jun 12 16:34 device_blocked lrwxrwxrwx 1 root root 0 May 24 16:38 driver -> ../../../../../../bus/scsi/drivers/sd lrwxrwxrwx 1 root root 0 Jun 12 16:34 generic -> ../../../../../../class/scsi_generic/sg44 -r--r--r-- 1 root root 4096 Jun 12 16:34 iocounterbits -r--r--r-- 1 root root 4096 Jun 12 16:34 iodone_cnt -r--r--r-- 1 root root 4096 Jun 12 16:34 ioerr_cnt -r--r--r-- 1 root root 4096 Jun 12 16:34 iorequest_cnt -r--r--r-- 1 root root 4096 May 24 16:38 model -rw-r--r-- 1 root root 4096 Jun 12 16:34 queue_depth -r--r--r-- 1 root root 4096 Jun 12 16:34 queue_type --w------- 1 root root 4096 Jun 12 16:34 rescan -r--r--r-- 1 root root 4096 Jun 12 16:34 rev -r--r--r-- 1 root root 4096 Jun 12 16:34 scsi_level -rw-r--r-- 1 root root 4096 May 26 11:28 state lrwxrwxrwx 1 root root 0 May 24 16:38 subsystem -> ../../../../../../bus/scsi -rw-r--r-- 1 root root 4096 Jun 12 16:34 timeout -r--r--r-- 1 root root 4096 May 24 16:38 type --w------- 1 root root 4096 Jun 12 16:34 uevent -r--r--r-- 1 root root 4096 May 24 16:38 vendor
OK, hopefully just one more request; I'm trying to figure out a way to distinguish a disk from a control LUN, since in my case (Linux iscsi target), 0 is a control LUN. Can you give me the output of:
# cat /sys/bus/scsi/devices/3:0:0:0/model
and
# cat /sys/bus/scsi/devices/3:0:0:0/type
from both your netapp target and your sun target? Here, at least, for our "control" LUN, model returns "Controller" while for the real LUNs model returns "VIRTUAL-DISK", so I'm hoping I can distinguish somehow based on what is in model, and if not that, based on type.
Can't you distinguish based on the fact that there'll be no /dev/sdNN node for it ? Daniel. -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange schreef:
Can't you distinguish based on the fact that there'll be no /dev/sdNN node for it ?
You mean an explicit check for the 'block:....' ? I really wonder how Linux internally distinguishes between a 'controller'. (And even more what a controller is...) Stefan
Daniel P. Berrange wrote:
# cat /sys/bus/scsi/devices/3:0:0:0/model
and
# cat /sys/bus/scsi/devices/3:0:0:0/type
from both your netapp target and your sun target? Here, at least, for our "control" LUN, model returns "Controller" while for the real LUNs model returns "VIRTUAL-DISK", so I'm hoping I can distinguish somehow based on what is in model, and if not that, based on type.
Can't you distinguish based on the fact that there'll be no /dev/sdNN node for it ?
Unfortunately, no. The /sys/bus/scsi/devices/3:0:0:0/block:sda (or block/sda) link will only exist once udev has finished plugging the device in. So we have to loop waiting for that to appear. In the case of a control LUN, that will never appear, so we will wait 5 seconds (the current default) to see it appear, and then when it doesn't, we won't know how to distinguish the error case from the control LUN case. I guess we could just not throw an error if we never see a block device, but we would still delay the libvirtd daemon (and the client) unnecessarily. Chris Lalancette
On Thu, Jun 12, 2008 at 05:20:19PM +0200, Chris Lalancette wrote:
Daniel P. Berrange wrote:
# cat /sys/bus/scsi/devices/3:0:0:0/model
and
# cat /sys/bus/scsi/devices/3:0:0:0/type
from both your netapp target and your sun target? Here, at least, for our "control" LUN, model returns "Controller" while for the real LUNs model returns "VIRTUAL-DISK", so I'm hoping I can distinguish somehow based on what is in model, and if not that, based on type.
Can't you distinguish based on the fact that there'll be no /dev/sdNN node for it ?
Unfortunately, no. The /sys/bus/scsi/devices/3:0:0:0/block:sda (or block/sda) link will only exist once udev has finished plugging the device in. So we have to loop waiting for that to appear. In the case of a control LUN, that will never appear, so we will wait 5 seconds (the current default) to see it appear, and then when it doesn't, we won't know how to distinguish the error case from the control LUN case. I guess we could just not throw an error if we never see a block device, but we would still delay the libvirtd daemon (and the client) unnecessarily.
Hmmm, well, presumably udev knows to not create a /dev/sdNN node based on some of the information in sysfs. So we just have to find out what udev's decision is based on and copy it. Perhaps the 'driver' link in sysfs will not exist ? Or point to something other than the 'sd' driver ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Chris Lalancette <clalance@redhat.com> wrote:
This rather large patch rewrites the virStorageBackendISCSIFindLUNs() function to only rely on sysfs for finding LUNs, given a session number. Along the way, it also fixes the bug where we wouldn't find LUNs for older kernels (with the block:sda format), and also (possibly) fixes a race condition where we could try to find the LUN before udev has finished connecting it. I say it "possibly" fixes it because I haven't been able to hit it so far, but I definitely need more testing to try and confirm. ...
Hi Chris, ACK to the first 4 parts. Here there's one nit and one problem:
+virStorageBackendISCSINewLun(virConnectPtr conn, virStoragePoolObjPtr pool, + unsigned int lun, char *dev)
"dev" const, and doesn't need to go past column 80. Dan already mentioned TABs.
+ if (strlen(block) == 5) { + /* OK, this is exactly "block"; must be new-style */ + snprintf(sysfs_path, PATH_MAX, + "/sys/bus/scsi/devices/%u:%u:%u:%u/block", + host, bus, target, lun); + sysdir = opendir(sysfs_path); + if (sysdir == NULL) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to opendir sysfs path %s: %s"), + sysfs_path, strerror(errno)); + retval = -1; + goto namelist_cleanup; + } + while ((sys_dirent = readdir(sysdir))) { + if (!notdotdir(sys_dirent)) + continue; + + scsidev = strdup(sys_dirent->d_name); + break; + } + closedir(sysdir); + } + else { + /* old-style; just parse out the sd */ + block2 = strrchr(block, ':'); + if (block2 == NULL) { + /* Hm, wasn't what we were expecting; have to give up */ + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to parse block path %s"), + block); + retval = -1; + goto namelist_cleanup; + } + block2++; + scsidev = strdup(block2); + }
This needs a check for scsidev == NULL, since virStorageBackendISCSINewLun would segfault on a NULL; it dereferences the pointer (via its "dev" parameter) with this:
+ if (asprintf(&devpath, "/dev/%s", dev) < 0) {
+ retval = virStorageBackendISCSINewLun(conn, pool, lun, scsidev); + if (retval < 0) + break; ...
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Jim Meyering -
Stefan de Konink