[libvirt] [PATCH 1/1] Add SCSI pool support.
Dave Allan
dallan at redhat.com
Fri Mar 27 15:16:42 UTC 2009
Daniel P. Berrange wrote:
> 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.
[Forgive me if you've been through this before and I just don't know the
history.]
This is a race with no easy answer if udev settle does not provide the
protection we need. I removed the retries because I thought udevadm
settle provided the correct protection against the race, so I thought
the retries only added complexity.
As your example demonstrates, though, that's not enough everywhere. I
can put the timeout and retry back to preserve the existing behavior,
but that only makes it more likely that the path we want will win the
race, it doesn't actually fix the problem. The iSCSI login should not
return until the devices have appeared in the kernel, which should
populate the udev queue and cause udev settle to pause until the queue
clears. Is that assumption not valid everywhere?
I think we need to understand why this is happening; if we have to pad
the timing until the right guy wins the race, so be it, but I'd really
like to understand the situation before we go there. What OS is this
on? As you are, I am testing iSCSI with the IET. I have 20 LUs, and I
don't see the problem. Does your system have udevadm settle?
> 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 at dhcp-1-98 ~]#
> [root at 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 at dhcp-1-98 ~]# virsh pool-refresh lettuceiscsi
> Pool lettuceiscsi refreshed
>
> [root at 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 at dhcp-1-98 ~]#
> [root at 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 at dhcp-1-98 ~]# virsh pool-refresh lettuceiscsi
> Pool lettuceiscsi refreshed
>
> [root at 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.
Oops, so it is. I'll remove it. Good catch.
Dave
More information about the libvir-list
mailing list