Re: How to automatically add a nbd to a raid1?

Peter Breuer <[email protected]> Wed, 17 Dec 2008 22:35:27 +0100 (CET)
Newsgroups gmane.linux.enbd.general
Message-ID <[email protected]>
"Also sprach Michael Rendell:"
> > > While "mdadm --detail --verbose /dev/mdNN" doesn't list the removed
> > > devices,
> >
> > It should .. morally, I mean. It KNOWS what devices are notionally in
> > the array.
> 
> It may know, but it's not telling (via the mdadm interface anyway);
> seems the --verbose option doesn't do much for the --detail mode.

I suppose you're right. You'll have to get the UUID from the degraded
raid with --detail

   UUID : 8604059f:b695ead2:0a76e71e:56c99031 

then check every other partition in sight with --examine
for the same UUID. 

Oh .. no. You're not quite right. mdadm --examine on the component
which didn't make it into the array shows all the details:

      Number   Major   Minor   RaidDevice State
this     1       7        1        1      active sync   /dev/loop1

   0     0       7        0        0      active sync   /dev/loop0
   1     1       7        1        1      active sync   /dev/loop1
  

If I look at the component that IS in the raid, it tells me nothing:

      Number   Major   Minor   RaidDevice State
this     0       7        0        0      active sync   /dev/loop0

   0     0       7        0        0      active sync   /dev/loop0
   1     1       0        0        1      faulty removed

So you need to check every new partition that appears :(. Or if you
start enbd you can check it for a uuid, and add to whichever raid has
that uuid.





> > Yes.  But what you want is the ONLINE and OFFLINE notices that enbd
> > sends to udev, not the ADD.
> >
> > The ADD and REMOVE events are just generated by the kernel disk
> > partition subsystem as the driver registers control of its majors and
> > minors.  They're what udev rules send to the MAKEDEV and REMOVEDEV
> > scripts in the enbd distr, via the provided enbd.rules file.
> 
> Used udevmonitor to see what events are generated and there
> only appear to be the add and remove events (no online/offline).

There are the other events too (I don't know what they are called).
You can see the log messages from enbd sending them.


> Am running enbd-2.4.35 and running the stock CentOS-5 kernel

You're right.  2.4.35 doesn't have the UDEV messages in.
You'll need the bit from 2.4.35a that has that in.



This supports talking to udev about connection or disconnection, using
kobjs.


diff -urbN enbd-2.4.35/kernel/linux-2.6.x/drivers/block/enbd/enbd_base.c enbd-2.4.35a/kernel/linux-2.6.x/drivers/block/enbd/enbd_base.c
--- enbd-2.4.35/kernel/linux-2.6.x/drivers/block/enbd/enbd_base.c	2008-06-01 10:43:05.000000000 +0200
+++ enbd-2.4.35a/kernel/linux-2.6.x/drivers/block/enbd/enbd_base.c	2008-07-22 12:15:32.000000000 +0200
@@ -270,6 +274,7 @@
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20)
 # include <linux/freezer.h>
 #endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20) */
+#include <linux/kobject.h>
 
 #if defined(MODULE)
 int linux_version_code = LINUX_VERSION_CODE;
@@ -3354,6 +3397,42 @@
         return;
 }
 
+/*
+ * PTB - For use in set_disk and enbd_notify_md_devices nowhere else.
+ * Needs lo->disk.
+ *
+ * The problem with using get_disk here is that it raises the refcount by
+ * one and then prevents use of rmmod, since the count is 1. Running
+ * rmmod would lower the count to 0, but we can't, since the count is
+ * one (so use rmmod -f). The alternative is to let close lower the count.
+ */
+static struct kobject *
+enbd_find_kobj (dev_t dev, int *part, void *data)
+{
+	struct enbd_device *lo = data;
+        if (!lo) {
+                ENBD_ERROR("null enbd device given\n");
+                return ERR_PTR(-ENOMEM);
+        }
+	if (lo->magic != ENBD_DEV_MAGIC) {
+                ENBD_ERROR("enbd device has bad magic\n");
+                return ERR_PTR(-EINVAL);
+        }
+        if (!lo->disk) {
+                ENBD_ERROR("no disk yet set for nd%s\n", lo->devnam);
+                return ERR_PTR(-EINVAL);
+        }
+        if (part && (*part < 0 || *part >= ENBD_MAXCONN)) {
+                ENBD_ERROR("invalid partition %d for nd%s\n",
+                    part ? *part : -2, lo->devnam);
+                return ERR_PTR(-EINVAL);
+        }
+        if (!lo->kobj)
+	        lo->kobj = &lo->disk->kobj; // PTB avoids incrementing owners
+        return lo->kobj;
+}
+
+
 #ifndef HOT_ADD_DISK
   #define HOT_ADD_DISK          _IO (MD_MAJOR, 0x28)
 #endif
@@ -3370,9 +3453,11 @@
 {
 	int j;
         struct enbd_md *md = &enbd_md;
+	dev_t enbd_dev;
+	struct kobject *kobj;
 
 	for (j = 0; j - 1 < lo->nslot; j++) {
-		dev_t enbd_dev = MKDEV (major, j + (lo->nbd << ENBD_SHIFT));
+		enbd_dev = MKDEV (major, j + (lo->nbd << ENBD_SHIFT));
 		if (j == 0) {
 			if (atomic_read (&lo->md_count) <= 0)
 				continue;
@@ -3381,8 +3466,34 @@
 			if (slot->md_count <= 0)
 				continue;
 		}
                 md->notify(md, enbd_dev, cmd);
 	}
+        if (!lo->disk) {
+	       	ENBD_ALERT ("no disk found, can't notify udev nd%s is %s\n",
+                    lo->devnam, cmd==HOT_ADD_DISK?"up":"down");
+                // PTB if no disk, lo->kobj can't equal &disk->kobj
+                return 0;
+        }
+        if (!lo->kobj) {
+	       	ENBD_ALERT ("no kobj found, can't notify udev nd%s is %s\n",
+                    lo->devnam, cmd==HOT_ADD_DISK?"up":"down");
+                return 0;
+        }
+        kobj = lo->kobj;
+
+        switch (cmd) {
+            case HOT_ADD_DISK:
+                kobject_uevent(kobj, KOBJ_ONLINE);
+	        ENBD_ALERT ("notified udev nd%s is up\n", lo->devnam);
+                break;
+            case SET_DISK_FAULTY:
+                kobject_uevent(kobj, KOBJ_OFFLINE);
+	        ENBD_ALERT ("notified udev nd%s is down\n", lo->devnam);
+                break;
+            default:
+	        ENBD_ERROR ("unknown cmd %d on nd%s\n", cmd, lo->devnam);
+                return -EINVAL;
+        }
 	return 0;
 }
 
@@ -6190,25 +6367,6 @@
 
 
 /*
- * PTB - For use in set_disk and nowhere else. Needs lo->disk.
- */
-static struct kobject *
-enbd_find (dev_t dev, int *part, void *data)
-{
-	struct enbd_device *lo = data;
-        if (!lo)
-                return NULL;
-	if (lo->magic != ENBD_DEV_MAGIC)
-                return NULL;
-        if (!lo->disk)
-                return NULL;
-        if (part && (*part < 0 || *part >= ENBD_MAXCONN))
-                return NULL;
-	return get_disk (lo->disk);
-}
-
-
-/*
  * PTB - used in setup to fix the gendisk data, chain the gendisk,
  * claim the blockdev region, etc.
  */
@@ -6234,7 +6392,7 @@
         // PTB mark disk removable! FIXME
         disk->flags |= GENHD_FL_REMOVABLE;
 	blk_register_region (MKDEV (major, first_minor),
-			     npart, THIS_MODULE, enbd_find, NULL, lo);
+			     npart, THIS_MODULE, enbd_find_kobj, NULL, lo);
 	set_capacity (disk, lo->sectors);
 	// we should rescan later. From userland?
 	add_disk (disk);
@@ -6748,6 +7139,8 @@
                 }
 
                 lo->disk = disk;
+                lo->kobj = &disk->kobj;
+
                 enbd_init_queue(lo, disk->queue);
                 enbd_init_ioctl_stub(&lo->remote_ioctl);
 
@@ -6857,6 +7250,7 @@
                 }
                 put_disk(disk);
                 lo->disk = NULL;
+                lo->kobj = NULL;
         }
         return err < 0 ? err : -EIO;
 }
@@ -6886,13 +7283,10 @@
                         blk_cleanup_queue (disk->queue);
                         disk->queue = NULL;
                 }
-                del_gendisk (disk);
                 put_disk (disk);
+                del_gendisk (disk);
                 lo->disk = NULL;
+                lo->kobj = NULL;
         }
-        if (lo->blockmap) {
-                kfree (lo->blockmap);
-                lo->blockmap = NULL;
-                lo->kobj = NULL;
-        }
         // PTB this should be under lock, but do we really care now?
         spin_lock(&lo->lock);

diff -urbN enbd-2.4.35/kernel/linux-2.6.x/include/linux/enbd.h enbd-2.4.35a/kernel/linux-2.6.x/include/linux/enbd.h
--- enbd-2.4.35/kernel/linux-2.6.x/include/linux/enbd.h	2008-04-19 15:40:49.000000000 +0200
+++ enbd-2.4.35a/kernel/linux-2.6.x/include/linux/enbd.h	2008-05-30 12:24:15.000000000 +0200
@@ -356,7 +380,7 @@
       struct inode * inode;                /* PTB add - for ref */
       int  bufsiz;                         /* PTB add - userspace buffer size */
       atomic_t kmax;                       /* PTB add - max kernel threads */
-      char *blockmap;                      /* PTB add - map of block states */
+      struct kobject *kobj;
       unsigned long disabled;              /* PTB add - when was it disabled */
       int req_timeo;                       /* PTB add - net timeout */
       int pulse_intvl;                     /* PTB add - inactivity intvl */



And pray I see that right.


> (2.6.18-92.1.18.el5).
> 
> 
> Have been testing/playing with the rc.d init scripts and
> the pwprog scripts and have a number of patches for them,
> some of which may be of use.
> 
> Patches for pwprog:
>     - renamed to enbd-pwprog to help me keep things straight

OK

>     - when writing lock files, write the running process's PID
>       to the lock file and not "$$" ($$ after a "&" is not the
>       currently running process PID - it is the original process PID).

But that's right, isn't it? The only call to "lock" (which does the
write) that I can see is in the fail_action(), which launches a daemon
to wait for the failed server to come back online. It IS the parent
which does the lock. The launched daemon just sits around grumpily
doing pings every so often.

OK, the launched daemon runs an unlock when it dies, which requires
reading a lock from the lock file and comparing it with $$, which
IS that of its parent. So I don't see the problem.

Also the parent won't be able to launch a second daemon, since
the lockfile is already heald by a live process (whose pid is recorded
in it) - itself. That's correct too.

>     - update the pid in the lock file after a "&"

That's not necessary, by the argument I just gave. I see .. you want to
have the child daeon's pid in the pidfile instead of the parent's! But
no .. that won't really work .. it leaves one open to multiple launches
from the same parent. Better to have the parent's pid in there.


>     - added a "disable" file so can play with a drive without
>       pwprog starting up and fixing things; also useful when
>       shutting down the system.

Ah. Good idea.


>     - added disable/enable command line options to create/remove
>       the disable file.

Alos good.

>     - put logging functions in (they were in an old version of
>       pwprog that you wrote).

I must find where these are!

> Patches for etc/init/enbd script:
>     - added a comment header so "chkconfig" will work (you may not want this)

Sounds good. 

>     - added a default/sysconfig CLIENT_WAIT_FOR_SERVER variable to
>       control whether, when starting enbd-client, to wait for the
>       server to be pingable (find waiting not useful when using -t 0).

Probably good. Have to think about it.

>     - added a default/sysconfig OUTPUT_TO_DEV_NULL variable to send
>       enbd-client and enbd-server stdout/stderr to dev null so
>       the screen when booting looks nicer :-)

Yes! 

I suspect one can do that already ... by diverting stdout and stderr
from the shell :).

>     - similar lock file / pid changes as for pwprog (see above)

I'll have to check.

>     - when stopping the client, wait a bit longer for the SIGTERM to
>       have effect (found it wasn't quite long enough);

Oh .. that's the init script. OK.

>       also, use a single kill of $pid and fuser output instead
>       (not sure what the purpose of the fuser kill was, so this

Presumably I would have wanted to kill processes running
mounted on the device in order to remove openers and allow many things
to disappear.

>       may not be OK; change made to get rid of the two sleep commands
>       and just have one).

H.

>     - commented out the "echo .. | socket" as there is no socket command
>       on this system (other calls to it were already commended out);

That may need looking at. Socket can be done via netcat (nc).

>     - touch/remove the /var/lock/subsys/enbd when starting/stopping
>       both the client and server (needed for the /etc/rc.d/rc script,
>       if the lock files doesn't exist, the service isn't stopped when
>       the system is shutting down).  Only done if it looks like it
>       is being run by the rc script (i.e., $runlevel is set in the
>       environment).

Sounds good admin.

>     - use the new enable/disable commands to pwprog when being
>       started/stopped.

OK.

> 
> 
> Best wishes,
> Michael

Will take gleefully .. thanks.

Peter