Re: [libvirt] [PATCH 1/1] Add SCSI pool support.

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.