[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]