On Thu, Mar 26, 2009 at 05:55:48PM -0400, David Allan wrote:
This version of the code scans a particular host for targets and LUs and adds any LUs it finds as storage volumes.
-static int -virStorageBackendISCSINewLun(virConnectPtr conn, virStoragePoolObjPtr pool, - unsigned int lun, const char *dev) -{ - virStorageVolDefPtr vol; - int fd = -1; - char *devpath = NULL; - int opentries = 0; - - if (VIR_ALLOC(vol) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - vol->type = VIR_STORAGE_VOL_BLOCK; - - if (virAsprintf(&(vol->name), "lun-%d", lun) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - if (virAsprintf(&devpath, "/dev/%s", dev) < 0) { - virReportOOMError(conn); - goto cleanup; - } - - /* It can take a little while between logging into the ISCSI - * server and udev creating the /dev nodes, so if we get ENOENT - * we must retry a few times - they should eventually appear. - * We currently wait for upto 5 seconds. Is this good enough ? - * Perhaps not on a very heavily loaded system Any other - * options... ? - */ - reopen: - if ((fd = open(devpath, O_RDONLY)) < 0) { - opentries++; - if (errno == ENOENT && opentries < 50) { - usleep(100 * 1000); - goto reopen; - } - virReportSystemError(conn, errno, - _("cannot open '%s'"), - devpath); - goto cleanup; - }
The new generic 'NewLUN' method has lost this bit of retry / sleep logic. This unfortauntely causes us to randomly loose LUNs due to fact that udev may not yet have created the /dev/ node or the stable path. In this example, I have a iSCSI pool with a single LUN that is active. On the iSCSI target I then export a second LUNs, and refresh the pool on the libvirt client. The first time I refresh, the OS sees the new LUN (as per lsscsi), but libvirt misses it. I have to refresh libvirt again to see it # virsh vol-list lettuceiscsi Name Path ----------------------------------------- 5.0.0.4 /dev/disk/by-path/ip-10.33.15.20:3260-iscsi-demo-lun-4 [root@dhcp-1-98 ~]# [root@dhcp-1-98 ~]# lsscsi [0:0:0:0] disk ATA HDT722516DLA380 V43O /dev/sda [1:0:0:0] cd/dvd PLEXTOR DVDR PX-755A 1.04 /dev/sr0 [5:0:0:0] storage IET Controller 0001 - [5:0:0:4] disk IET VIRTUAL-DISK 0001 /dev/sdb [root@dhcp-1-98 ~]# virsh pool-refresh lettuceiscsi Pool lettuceiscsi refreshed [root@dhcp-1-98 ~]# lsscsi [0:0:0:0] disk ATA HDT722516DLA380 V43O /dev/sda [1:0:0:0] cd/dvd PLEXTOR DVDR PX-755A 1.04 /dev/sr0 [5:0:0:0] storage IET Controller 0001 - [5:0:0:2] disk IET VIRTUAL-DISK 0001 /dev/sdc [5:0:0:4] disk IET VIRTUAL-DISK 0001 /dev/sdb [root@dhcp-1-98 ~]# [root@dhcp-1-98 ~]# virsh vol-list lettuceiscsi Name Path ----------------------------------------- 5.0.0.4 /dev/disk/by-path/ip-10.33.15.20:3260-iscsi-demo-lun-4 [root@dhcp-1-98 ~]# virsh pool-refresh lettuceiscsi Pool lettuceiscsi refreshed [root@dhcp-1-98 ~]# virsh vol-list lettuceiscsi Name Path ----------------------------------------- 5.0.0.2 /dev/disk/by-path/ip-10.33.15.20:3260-iscsi-demo-lun-2 5.0.0.4 /dev/disk/by-path/ip-10.33.15.20:3260-iscsi-demo-lun-4
- - /* Now figure out the stable path - * - * XXX this method is O(N) because it scans the pool target - * dir every time its run. Should figure out a more efficient - * way of doing this... - */ - if ((vol->target.path = virStorageBackendStablePath(conn, - pool, - devpath)) == NULL) - goto cleanup; - - VIR_FREE(devpath); - - if (virStorageBackendUpdateVolTargetInfoFD(conn, - &vol->target, - fd, - &vol->allocation, - &vol->capacity) < 0) - goto cleanup; - - /* XXX use unique iSCSI id instead */ - vol->key = strdup(vol->target.path); - if (vol->key == NULL) { - virReportOOMError(conn); - goto cleanup; - } - - - pool->def->capacity += vol->capacity; - pool->def->allocation += vol->allocation; - - if (VIR_REALLOC_N(pool->volumes.objs, - pool->volumes.count+1) < 0) { - virReportOOMError(conn); - goto cleanup; - } - pool->volumes.objs[pool->volumes.count++] = vol; - - close(fd); - - return 0; - - cleanup: - if (fd != -1) close(fd); - VIR_FREE(devpath); - virStorageVolDefFree(vol); - return -1; -} - -static int notdotdir(const struct dirent *dir) -{ - return !(STREQLEN(dir->d_name, ".", 1) || STREQLEN(dir->d_name, "..", 2)); -} - -/* Function to check if the type file in the given sysfs_path is a - * Direct-Access device (i.e. type 0). Return -1 on failure, 0 if not - * a Direct-Access device, and 1 if a Direct-Access device - */ -static int directAccessDevice(const char *sysfs_path) -{ - char typestr[3]; - char *gottype, *p; - FILE *typefile; - int type; - - typefile = fopen(sysfs_path, "r"); - if (typefile == NULL) { - /* there was no type file; that doesn't seem right */ - return -1; - } - gottype = fgets(typestr, 3, typefile); - fclose(typefile); - - if (gottype == NULL) { - /* we couldn't read the type file; have to give up */ - return -1; - } - - /* we don't actually care about p, but if you pass NULL and the last - * character is not \0, virStrToLong_i complains - */ - if (virStrToLong_i(typestr, &p, 10, &type) < 0) { - /* Hm, type wasn't an integer; seems strange */ - return -1; - } - - if (type != 0) { - /* saw a device other than Direct-Access */ - return 0; - } - - return 1; -}
static int virStorageBackendISCSIFindLUNs(virConnectPtr conn, @@ -315,196 +177,17 @@ virStorageBackendISCSIFindLUNs(virConnectPtr conn, const char *session) { char sysfs_path[PATH_MAX]; - uint32_t host, bus, target, lun; - DIR *sysdir; - struct dirent *sys_dirent; - struct dirent **namelist; - int i, n, tries, retval, directaccess; - char *block, *scsidev, *block2; - - retval = 0; - block = NULL; - scsidev = NULL; + int retval = 0;
snprintf(sysfs_path, PATH_MAX, "/sys/class/iscsi_session/session%s/device", session);
- sysdir = opendir(sysfs_path); - if (sysdir == NULL) { + if (virStorageBackendSCSIFindTargets(conn, pool, sysfs_path, "target") < 0) { virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), + _("Failed to get target list for path '%s'"), sysfs_path); - return -1; + retval = -1; } - while ((sys_dirent = readdir(sysdir))) { - /* double-negative, so we can use the same function for scandir below */ - if (!notdotdir(sys_dirent)) - continue; - - if (STREQLEN(sys_dirent->d_name, "target", 6)) { - if (sscanf(sys_dirent->d_name, "target%u:%u:%u", - &host, &bus, &target) != 3) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to parse target from sysfs path %s/%s"), - sysfs_path, sys_dirent->d_name); - closedir(sysdir); - return -1; - } - break; - } - } - closedir(sysdir); - - /* we now have the host, bus, and target; let's scan for LUNs */ - snprintf(sysfs_path, PATH_MAX, - "/sys/class/iscsi_session/session%s/device/target%u:%u:%u", - session, host, bus, target); - - n = scandir(sysfs_path, &namelist, notdotdir, versionsort); - if (n <= 0) { - /* we didn't find any reasonable entries; return failure */ - virReportSystemError(conn, errno, - _("Failed to find any LUNs for session '%s'"), - session); - return -1; - } - - for (i=0; i<n; i++) { - block = NULL; - scsidev = NULL; - - if (sscanf(namelist[i]->d_name, "%u:%u:%u:%u\n", - &host, &bus, &target, &lun) != 4) - continue; - - /* we found a LUN */ - /* Note, however, that just finding a LUN doesn't mean it is - * actually useful to us. There are a few different types of - * LUNs, enumerated in the linux kernel in - * drivers/scsi/scsi.c:scsi_device_types[]. Luckily, these device - * types form part of the ABI between the kernel and userland, so - * are unlikely to change. For now, we ignore everything that isn't - * type 0; that is, a Direct-Access device - */ - snprintf(sysfs_path, PATH_MAX, - "/sys/bus/scsi/devices/%u:%u:%u:%u/type", - host, bus, target, lun); - - directaccess = directAccessDevice(sysfs_path); - if (directaccess < 0) { - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to determine if %u:%u:%u:%u is a Direct-Access LUN"), - host, bus, target, lun); - retval = -1; - goto namelist_cleanup; - } - else if (directaccess == 0) { - /* not a direct-access device; skip */ - continue; - } - /* implicit else if (access == 1); Direct-Access device */ - - /* It might take some time for the - * /sys/bus/scsi/devices/host:bus:target:lun/block{:sda,/sda} - * link to show up; wait up to 5 seconds for it, then give up - */ - tries = 0; - while (block == NULL && tries < 50) { - snprintf(sysfs_path, PATH_MAX, "/sys/bus/scsi/devices/%u:%u:%u:%u", - host, bus, target, lun); - - sysdir = opendir(sysfs_path); - if (sysdir == NULL) { - virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), - sysfs_path); - retval = -1; - goto namelist_cleanup; - } - while ((sys_dirent = readdir(sysdir))) { - if (!notdotdir(sys_dirent)) - continue; - if (STREQLEN(sys_dirent->d_name, "block", 5)) { - block = strdup(sys_dirent->d_name); - break; - } - } - closedir(sysdir); - tries++; - if (block == NULL) - usleep(100 * 1000); - } - - if (block == NULL) { - /* we couldn't find the device link for this device; fail */ - virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("Failed to find device link for lun %d"), - lun); - retval = -1; - goto namelist_cleanup; - } - - 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) { - virReportSystemError(conn, errno, - _("Failed to opendir sysfs path '%s'"), - sysfs_path); - 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); - } - if (scsidev == NULL) { - virReportOOMError(conn); - retval = -1; - goto namelist_cleanup; - } - - retval = virStorageBackendISCSINewLun(conn, pool, lun, scsidev); - if (retval < 0) - break; - VIR_FREE(scsidev); - VIR_FREE(block); - } - -namelist_cleanup: - /* we call these VIR_FREE here to make sure we don't leak memory on - * error cases; in the success case, these are already freed but NULL, - * which should be fine - */ - VIR_FREE(scsidev); - VIR_FREE(block); - - for (i=0; i<n; i++) - VIR_FREE(namelist[i]); - - VIR_FREE(namelist);
return retval; } diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c new file mode 100644 index 0000000..62c05ae --- /dev/null +++ b/src/storage_backend_scsi.c @@ -0,0 +1,508 @@ +/* + * storage_backend_scsi.c: storage backend for SCSI handling + * + * Copyright (C) 2007-2008 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: Daniel P. Berrange <berrange redhat com> + */ + +#include <config.h> + +#include <unistd.h> +#include <stdio.h> +#include <dirent.h> +#include <fcntl.h> + +#include "virterror_internal.h" +#include "storage_backend_scsi.h" +#include "memory.h" +#include "logging.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +/* Function to check if the type file in the given sysfs_path is a + * Direct-Access device (i.e. type 0). Return -1 on failure, type of + * the device otherwise. + */ +static int +getDeviceType(virConnectPtr conn, + uint32_t host, + uint32_t bus, + uint32_t target, + uint32_t lun, + int *type) +{ + char *type_path = NULL; + char typestr[3]; + char *gottype, *p; + FILE *typefile; + int retval = 0; + + if (virAsprintf(&type_path, "/sys/bus/scsi/devices/%u:%u:%u:%u/type", + host, bus, target, lun) < 0) { + virReportOOMError(conn); + goto out; + } + + typefile = fopen(type_path, "r"); + if (typefile == NULL) { + virReportSystemError(conn, errno, + _("Could not find typefile '%s'"), + type_path); + /* there was no type file; that doesn't seem right */ + retval = -1; + goto out; + } + + gottype = fgets(typestr, 3, typefile); + fclose(typefile); + + if (gottype == NULL) { + virReportSystemError(conn, errno, + _("Could not read typefile '%s'"), + type_path); + /* we couldn't read the type file; have to give up */ + retval = -1; + goto out; + } + + /* we don't actually care about p, but if you pass NULL and the last + * character is not \0, virStrToLong_i complains + */ + if (virStrToLong_i(typestr, &p, 10, type) < 0) { + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Device type '%s' is not an integer"), + typestr); + /* Hm, type wasn't an integer; seems strange */ + retval = -1; + goto out; + } + + VIR_DEBUG(_("Device type is %d"), *type); + +out: + VIR_FREE(type_path); + return retval; +} + + +static int +virStorageBackendSCSINewLun(virConnectPtr conn, + virStoragePoolObjPtr pool, + uint32_t host, + uint32_t bus, + uint32_t target, + uint32_t lun, + const char *dev) +{ + virStorageVolDefPtr vol; + char *devpath = NULL; + int retval = 0; + + if (VIR_ALLOC(vol) < 0) { + virReportOOMError(conn); + retval = -1; + goto out; + } + + vol->type = VIR_STORAGE_VOL_BLOCK; + + if (virAsprintf(&(vol->name), "%u.%u.%u.%u", host, bus, target, lun) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + if (virAsprintf(&devpath, "/dev/%s", dev) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + VIR_DEBUG(_("Trying to create volume for '%s'"), devpath); +
This is where I think we need to reintroduce the open/try loop to wait for the nodes to appear in /dev
+ /* Now figure out the stable path + * + * XXX this method is O(N) because it scans the pool target + * dir every time its run. Should figure out a more efficient + * way of doing this... + */ + if ((vol->target.path = virStorageBackendStablePath(conn, + pool, + devpath)) == NULL) { + retval = -1; + goto free_vol; + } + + if (STREQLEN(devpath, vol->target.path, PATH_MAX)) { + VIR_DEBUG(_("No stable path found for '%s' in '%s'"), + devpath, pool->def->target.path); + retval = -1; + goto free_vol; + }
Also this bit needs to be removed, because we need to fallback to the generic /dev path if no stable link is present, or if the user has explicitly configured /dev/ as the pool target
+ + if (virStorageBackendUpdateVolTargetInfo(conn, + &vol->target, + &vol->allocation, + &vol->capacity) < 0) { + + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Failed to update volume for '%s'"), + devpath); + retval = -1; + goto free_vol; + } + + /* XXX should use logical unit's UUID instead */ + vol->key = strdup(vol->target.path); + if (vol->key == NULL) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + + pool->def->capacity += vol->capacity; + pool->def->allocation += vol->allocation; + + if (VIR_REALLOC_N(pool->volumes.objs, + pool->volumes.count + 1) < 0) { + virReportOOMError(conn); + retval = -1; + goto free_vol; + } + pool->volumes.objs[pool->volumes.count++] = vol; + + goto out; + +free_vol: + virStorageVolDefFree(vol); +out: + VIR_FREE(devpath); + return retval; +} + + diff --git a/src/storage_backend_scsi.h b/src/storage_backend_scsi.h new file mode 100644 index 0000000..678ccd6 --- /dev/null +++ b/src/storage_backend_scsi.h @@ -0,0 +1,54 @@ + +#ifndef __VIR_STORAGE_BACKEND_SCSI_H__ +#define __VIR_STORAGE_BACKEND_SCSI_H__ + +#include "storage_backend.h" +#include "hash.h" + +#define LINUX_SYSFS_SCSI_HOST_PREFIX "/sys/class/scsi_host" +#define LINUX_SYSFS_SCSI_HOST_POSTFIX "device" + +struct _virDirectoryWalkData { + virConnectPtr conn; + virStoragePoolObjPtr pool; + const char *dir_path; + const char *pattern_to_match; + size_t expected_matches; + virHashTablePtr matches; // will be created by walk function + virHashIterator iterator; + virHashDeallocator deallocator; +}; +typedef struct _virDirectoryWalkData virDirectoryWalkData; +typedef virDirectoryWalkData *virDirectoryWalkDataPtr;
This struct/typedef doesn't appear to be used anywhere in the code. Guessing this is just a left-over from earlier version. FYI, my testing was on 2.6.18 RHEL-5 kernel, and 2.6.27.9 F10/9 kernels Regards, 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 :|