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 <dal...@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
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
