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(a)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