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

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



Daniel P. Berrange wrote:
On Fri, Mar 27, 2009 at 11:16:42AM -0400, Dave Allan wrote:
Daniel P. Berrange wrote:
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?

Spot the obvious mistake...


    virStorageBackendWaitForDevices(conn);

    if ((session = virStorageBackendISCSISession(conn, pool, 0)) == NULL)
        goto cleanup;
    if (virStorageBackendISCSIRescanLUNs(conn, pool, session) < 0)
        goto cleanup;
    if (virStorageBackendISCSIFindLUNs(conn, pool, session) < 0)
        goto cleanup;


Its calling udev settle before it has actually told the iSCSI client
to rescan the target for new LUNs :-) Probably best to move the call
to virStorageBackendWaitForDevices() into the method
virStorageBackendISCSIFindLUNs() itself.

Hah...that would break things, wouldn't it? I've moved the call. Give the attached a go and see if it fixes it.

Dave

>From 106b4f9b91340b724d9768182bc8788ab48022c0 Mon Sep 17 00:00:00 2001
From: David Allan <dallan redhat com>
Date: Fri, 27 Mar 2009 11:45:12 -0400
Subject: [PATCH 1/1] Fixed broken placement of udevadm noticed by DanPB.

---
 src/storage_backend_iscsi.c |    2 --
 src/storage_backend_scsi.c  |    4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c
index 2ececdd..9da7cdc 100644
--- a/src/storage_backend_iscsi.c
+++ b/src/storage_backend_iscsi.c
@@ -298,8 +298,6 @@ virStorageBackendISCSIRefreshPool(virConnectPtr conn,
 
     pool->def->allocation = pool->def->capacity = pool->def->available = 0;
 
-    virStorageBackendWaitForDevices(conn);
-
     if ((session = virStorageBackendISCSISession(conn, pool, 0)) == NULL)
         goto cleanup;
     if (virStorageBackendISCSIRescanLUNs(conn, pool, session) < 0)
diff --git a/src/storage_backend_scsi.c b/src/storage_backend_scsi.c
index 62c05ae..e0ced8f 100644
--- a/src/storage_backend_scsi.c
+++ b/src/storage_backend_scsi.c
@@ -445,6 +445,8 @@ virStorageBackendSCSIFindTargets(virConnectPtr conn,
 
     VIR_DEBUG(_("Discovering targets in '%s'"), sysfs_path);
 
+    virStorageBackendWaitForDevices(conn);
+
     sysdir = opendir(sysfs_path);
 
     if (sysdir == NULL) {
@@ -482,8 +484,6 @@ virStorageBackendSCSIRefreshPool(virConnectPtr conn,
 
     pool->def->allocation = pool->def->capacity = pool->def->available = 0;
 
-    virStorageBackendWaitForDevices(conn);
-
     if (sscanf(pool->def->source.adapter, "host%u", &host) != 1) {
         VIR_DEBUG(_("Failed to get host number from '%s'"), pool->def->source.adapter);
         retval = -1;
-- 
1.6.0.6


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