[PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Paul Clements]

[-- Attachment #1: Type: text/plain, Size: 124 bytes --]

Following is a simple patch to remove deprecated MOD_INC_USE_COUNT from
md.c.

If it looks good, please apply.

Thanks,
Paul

[-- Attachment #2: md_mod_unsafe.diff --]
[-- Type: text/plain, Size: 540 bytes --]

--- md.c.PRISTINE	2003-04-22 12:59:37.000000000 -0400
+++ md.c	2003-04-22 12:59:50.000000000 -0400
@@ -181,7 +181,6 @@ static void mddev_put(mddev_t *mddev)
 		list_del(&mddev->all_mddevs);
 		mddev_map[mdidx(mddev)] = NULL;
 		kfree(mddev);
-		MOD_DEC_USE_COUNT;
 	}
 	spin_unlock(&all_mddevs_lock);
 }
@@ -203,7 +202,6 @@ static mddev_t * mddev_find(int unit)
 		mddev_map[unit] = new;
 		list_add(&new->all_mddevs, &all_mddevs);
 		spin_unlock(&all_mddevs_lock);
-		MOD_INC_USE_COUNT;
 		return new;
 	}
 	spin_unlock(&all_mddevs_lock);

Re: [PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Neil Brown]

On Tuesday April 22, Paul.Clements@SteelEye.com wrote:
> Following is a simple patch to remove deprecated MOD_INC_USE_COUNT from
> md.c.
> 
> If it looks good, please apply.

I don't think it is that easy.
We take/drop a reference count there for a reason, and just removing
the calls isn't likely to do the right thing.

With md, we need to hold a refcount on the module not only when any
md device is open, but also when any md device is active.

I *think* changing them to "module_put(THIS_MODULE)" and
"try_module_get(THIS_MODULE)" would do the right thing, but I would
rather spend a bit little bit longer understanding the new block dev
stuff before I was sure.

NeilBrown

> 
> Thanks,
> Paul--- md.c.PRISTINE	2003-04-22 12:59:37.000000000 -0400
> +++ md.c	2003-04-22 12:59:50.000000000 -0400
> @@ -181,7 +181,6 @@ static void mddev_put(mddev_t *mddev)
>  		list_del(&mddev->all_mddevs);
>  		mddev_map[mdidx(mddev)] = NULL;
>  		kfree(mddev);
> -		MOD_DEC_USE_COUNT;
>  	}
>  	spin_unlock(&all_mddevs_lock);
>  }
> @@ -203,7 +202,6 @@ static mddev_t * mddev_find(int unit)
>  		mddev_map[unit] = new;
>  		list_add(&new->all_mddevs, &all_mddevs);
>  		spin_unlock(&all_mddevs_lock);
> -		MOD_INC_USE_COUNT;
>  		return new;
>  	}
>  	spin_unlock(&all_mddevs_lock);

Re: [PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Paul Clements]

Neil Brown wrote:
> On Tuesday April 22, Paul.Clements@SteelEye.com wrote:
> > Following is a simple patch to remove deprecated MOD_INC_USE_COUNT from
> > md.c.
> >
> > If it looks good, please apply.
> 
> I don't think it is that easy.

Well, I think that it's safe to remove the MOD_INC_USE_COUNT...here's
why:

We're protected while a device is open (by the infrastructure in the
block layer that takes a module reference when an md device is opened
and keeps it until the device is closed).

So, while any ioctl is being done, there's an md device open...and...the
ioctls are currently the only way to activate md devices (autorun,
RUN_ARRAY, etc.). 

Now, none of these ioctls will succeed (activate a device) unless there
is an underlying raid personality module (raid0, raid1, et al.) for the
array already loaded. The underlying raid modules also take module
references against md, so by the time a device is activated, md is
already pinned into memory...

So we're safe from every angle...the MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT
appears to have been superfluous in the first place...(at least with the
way 2.5 kernel module refs work)

--
Paul

[PATCH 2.5.68] eliminate interdependency between fs/partitions and md driver and enable persistent superblocks/raidautorun with modular md

[Paul Clements]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

The attached patch allows the md driver to be built as a module, and
still use the raidautorun feature (basically, using RAID partition
tagging (0xfd) and persistent RAID superblocks to autorun detected
arrays at boot time). This was previously not possible. In the process,
the patch also cleans up some unnecessary, and ugly, interdependencies
between the partitioning code and the md driver. 

The patch should allow vendors to compile md as a module (saving roughly
50k in their default kernel images) without any loss in functionality.
It would also allow other volume managers to use the RAID partition
tagging to autodetect arrays (if desired).

This patch has been tested for several days against 2.5.67/68 with an
md/RAID5 root partition, which gets autorun at boot time (in the
initrd). 

Feedback welcome. Please test, if you're interested. If the patch looks
good, please apply.


Thanks,
Paul


P.S., If anyone is interested, I've also got a patch for UnitedLinux
mk_initrd to make it work properly with modular md and the 2.5 kernel
(look for .ko modules, etc.).

[-- Attachment #2: md_modular_patch.diff --]
[-- Type: text/plain, Size: 6841 bytes --]

diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.5.68/drivers/md/md.c linux-2.5.68-md_mod/drivers/md/md.c
--- linux-2.5.68/drivers/md/md.c	2003-04-22 12:59:37.000000000 -0400
+++ linux-2.5.68-md_mod/drivers/md/md.c	2003-04-22 12:57:15.000000000 -0400
@@ -34,6 +34,7 @@
 #include <linux/raid/md.h>
 #include <linux/sysctl.h>
 #include <linux/bio.h>
+#include <linux/blkpg.h>
 #include <linux/devfs_fs_kernel.h>
 #include <linux/buffer_head.h> /* for invalidate_bdev */
 #include <linux/suspend.h>
@@ -59,9 +60,7 @@
 #define dprintk(x...) ((void)(DEBUG && printk(x)))
 
 
-#ifndef MODULE
 static void autostart_arrays (void);
-#endif
 
 static mdk_personality_t *pers[MAX_PERSONALITY];
 static spinlock_t pers_lock = SPIN_LOCK_UNLOCKED;
@@ -1071,7 +1070,6 @@ static void unlock_rdev(mdk_rdev_t *rdev
 	blkdev_put(bdev, BDEV_RAW);
 }
 
-void md_autodetect_dev(dev_t dev);
 
 static void export_rdev(mdk_rdev_t * rdev)
 {
@@ -1081,9 +1079,7 @@ static void export_rdev(mdk_rdev_t * rde
 		MD_BUG();
 	free_disk_sb(rdev);
 	list_del_init(&rdev->same_set);
-#ifndef MODULE
-	md_autodetect_dev(rdev->bdev->bd_dev);
-#endif
+	raid_autodetect_dev(rdev->bdev->bd_dev);
 	unlock_rdev(rdev);
 	kfree(rdev);
 }
@@ -2378,12 +2374,10 @@ static int md_ioctl(struct inode *inode,
 			md_print_devices();
 			goto done;
 
-#ifndef MODULE
 		case RAID_AUTORUN:
 			err = 0;
 			autostart_arrays();
 			goto done;
-#endif
 		default:;
 	}
 
@@ -3497,23 +3491,8 @@ int __init md_init(void)
 	raid_table_header = register_sysctl_table(raid_root_table, 1);
 
 	md_geninit();
-	return (0);
-}
-
-
-#ifndef MODULE
 
-/*
- * Searches all registered partitions for autorun RAID arrays
- * at boot time.
- */
-static dev_t detected_devices[128];
-static int dev_cnt;
-
-void md_autodetect_dev(dev_t dev)
-{
-	if (dev_cnt >= 0 && dev_cnt < 127)
-		detected_devices[dev_cnt++] = dev;
+	return (0);
 }
 
 
@@ -3524,8 +3503,8 @@ static void autostart_arrays(void)
 
 	printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
 
-	for (i = 0; i < dev_cnt; i++) {
-		dev_t dev = detected_devices[i];
+	for (i = 0; i < raid_num_detected_devs(); i++) {
+		dev_t dev = raid_get_detected_dev(i);
 
 		rdev = md_import_device(dev,0, 0);
 		if (IS_ERR(rdev)) {
@@ -3539,13 +3518,10 @@ static void autostart_arrays(void)
 		}
 		list_add(&rdev->same_set, &pending_raid_disks);
 	}
-	dev_cnt = 0;
 
 	autorun_devices();
 }
 
-#endif
-
 static __exit void md_exit(void)
 {
 	int i;
diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.5.68/fs/partitions/Makefile linux-2.5.68-md_mod/fs/partitions/Makefile
--- linux-2.5.68/fs/partitions/Makefile	2003-04-19 22:49:14.000000000 -0400
+++ linux-2.5.68-md_mod/fs/partitions/Makefile	2003-04-22 14:31:44.000000000 -0400
@@ -2,7 +2,7 @@
 # Makefile for the linux kernel.
 #
 
-obj-y := check.o
+obj-y := check.o raid.o
 
 obj-$(CONFIG_ACORN_PARTITION) += acorn.o
 obj-$(CONFIG_AMIGA_PARTITION) += amiga.o
diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.5.68/fs/partitions/check.c linux-2.5.68-md_mod/fs/partitions/check.c
--- linux-2.5.68/fs/partitions/check.c	2003-04-19 22:50:49.000000000 -0400
+++ linux-2.5.68-md_mod/fs/partitions/check.c	2003-04-22 14:32:45.000000000 -0400
@@ -21,6 +21,7 @@
 #include <linux/ctype.h>
 
 #include "check.h"
+#include "raid.h"
 
 #include "acorn.h"
 #include "amiga.h"
@@ -36,10 +37,6 @@
 #include "ultrix.h"
 #include "efi.h"
 
-#if CONFIG_BLK_DEV_MD
-extern void md_autodetect_dev(dev_t dev);
-#endif
-
 int warn_no_part = 1; /*This is ugly: should make genhd removable media aware*/
 
 static int (*check_part[])(struct parsed_partitions *, struct block_device *) = {
@@ -307,11 +304,9 @@ void register_disk(struct gendisk *disk)
 			if (!size)
 				continue;
 			add_partition(disk, j, from, size);
-#if CONFIG_BLK_DEV_MD
 			if (!state->parts[j].flags)
 				continue;
-			md_autodetect_dev(bdev->bd_dev+j);
-#endif
+			raid_autodetect_dev(bdev->bd_dev+j);
 		}
 		kfree(state);
 	}
@@ -342,10 +337,8 @@ int rescan_partitions(struct gendisk *di
 		if (!size)
 			continue;
 		add_partition(disk, p, from, size);
-#if CONFIG_BLK_DEV_MD
 		if (state->parts[p].flags)
-			md_autodetect_dev(bdev->bd_dev+p);
-#endif
+			raid_autodetect_dev(bdev->bd_dev+p);
 	}
 	kfree(state);
 	return res;
diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.5.68/fs/partitions/raid.c linux-2.5.68-md_mod/fs/partitions/raid.c
--- linux-2.5.68/fs/partitions/raid.c	1969-12-31 19:00:00.000000000 -0500
+++ linux-2.5.68-md_mod/fs/partitions/raid.c	2003-04-22 15:21:25.000000000 -0400
@@ -0,0 +1,31 @@
+/*
+ * Save registered partitions that are marked as autorun RAID arrays.
+ *
+ * check.c saves the partitions by calling raid_autodetect_dev at boot time
+ * md.c uses the partition list to do its autostart_arrays 
+ *
+ * code taken from md.c and new code added - 2003, Paul Clements
+ */
+
+#include <linux/fs.h>
+
+static dev_t detected_devices[128];
+static int dev_cnt;
+
+void raid_autodetect_dev(dev_t dev)
+{
+        if (dev_cnt >= 0 && dev_cnt < 128)
+                detected_devices[dev_cnt++] = dev;
+}
+
+dev_t raid_get_detected_dev(int i)
+{
+	if (i >= 0 && i < dev_cnt)
+		return detected_devices[i];
+	return (dev_t) 0;
+}
+
+int raid_num_detected_devs(void)
+{
+	return dev_cnt;
+}
diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.5.68/fs/partitions/raid.h linux-2.5.68-md_mod/fs/partitions/raid.h
--- linux-2.5.68/fs/partitions/raid.h	1969-12-31 19:00:00.000000000 -0500
+++ linux-2.5.68-md_mod/fs/partitions/raid.h	2003-04-22 14:31:45.000000000 -0400
@@ -0,0 +1,5 @@
+/*
+ *  fs/partitions/raid.h
+ */
+
+void raid_autodetect_dev(dev_t dev);
diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.5.68/include/linux/blkpg.h linux-2.5.68-md_mod/include/linux/blkpg.h
--- linux-2.5.68/include/linux/blkpg.h	2003-04-19 22:49:25.000000000 -0400
+++ linux-2.5.68-md_mod/include/linux/blkpg.h	2003-04-22 15:35:50.000000000 -0400
@@ -57,6 +57,9 @@ struct blkpg_partition {
 #ifdef __KERNEL__
 
 extern char * partition_name(dev_t dev);
+extern void raid_autodetect_dev(dev_t dev);
+extern dev_t raid_get_detected_dev(int i);
+extern int raid_num_detected_devs(void);
 
 #endif /* __KERNEL__ */
 
diff -purN --exclude-from /export/public/clemep/tmp/dontdiff linux-2.5.68/kernel/ksyms.c linux-2.5.68-md_mod/kernel/ksyms.c
--- linux-2.5.68/kernel/ksyms.c	2003-04-19 22:48:49.000000000 -0400
+++ linux-2.5.68-md_mod/kernel/ksyms.c	2003-04-22 15:36:23.000000000 -0400
@@ -576,7 +576,11 @@ EXPORT_SYMBOL(fs_overflowgid);
 EXPORT_SYMBOL(fasync_helper);
 EXPORT_SYMBOL(kill_fasync);
 
+/* partition stuff */
 EXPORT_SYMBOL(partition_name);
+EXPORT_SYMBOL(raid_autodetect_dev);
+EXPORT_SYMBOL(raid_get_detected_dev);
+EXPORT_SYMBOL(raid_num_detected_devs);
 
 /* binfmt_aout */
 EXPORT_SYMBOL(get_write_access);

Re: [PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Neil Brown]

On Wednesday April 23, Paul.Clements@SteelEye.com wrote:
> Neil Brown wrote:
> > On Tuesday April 22, Paul.Clements@SteelEye.com wrote:
> > > Following is a simple patch to remove deprecated MOD_INC_USE_COUNT from
> > > md.c.
> > >
> > > If it looks good, please apply.
> > 
> > I don't think it is that easy.
> 
> Well, I think that it's safe to remove the MOD_INC_USE_COUNT...here's
> why:
> 
> We're protected while a device is open (by the infrastructure in the
> block layer that takes a module reference when an md device is opened
> and keeps it until the device is closed).
> 
> So, while any ioctl is being done, there's an md device open...and...the
> ioctls are currently the only way to activate md devices (autorun,
> RUN_ARRAY, etc.). 
> 
> Now, none of these ioctls will succeed (activate a device) unless there
> is an underlying raid personality module (raid0, raid1, et al.) for the
> array already loaded. The underlying raid modules also take module
> references against md, so by the time a device is activated, md is
> already pinned into memory...
> 
> So we're safe from every angle...the MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT
> appears to have been superfluous in the first place...(at least with the
> way 2.5 kernel module refs work)

Almost.. but not quite.

It is possible for an array to be half-assembled.  To have some drives
attached, but not to have been started.
This can happen (for example) if you use mdadm to assemble a raid5
array without given all of the drives, and without giving --run.

In this case you don't want 'md' to be unloaded, but you haven't
loaded a personality module yet.  So you need to take a reference to
the md module whenever you start putting an md array together.

NeilBrown

Re: [PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Paul Clements]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

Neil Brown wrote:
> 
> On Wednesday April 23, Paul.Clements@SteelEye.com wrote:
> > Neil Brown wrote:

> It is possible for an array to be half-assembled.  To have some drives
> attached, but not to have been started.
> This can happen (for example) if you use mdadm to assemble a raid5
> array without given all of the drives, and without giving --run.
> 
> In this case you don't want 'md' to be unloaded, but you haven't
> loaded a personality module yet.  So you need to take a reference to
> the md module whenever you start putting an md array together.

Ah...I had missed that...appears that was an existing problem, even with
the MOD_INC_USE_COUNT in place. 

So we need to do the ref counting at bind/unbind time, rather than
activation/deactivation time.

So here's the patch...compiled and tested briefly against
2.5.68-bklatest, the ref counting looks correct

--
Paul

[-- Attachment #2: md_mod_ref.diff --]
[-- Type: text/plain, Size: 1348 bytes --]

--- linux-2.5/drivers/md/md.c.2.5.68-bklatest	2003-05-02 11:56:18.000000000 -0400
+++ linux-2.5/drivers/md/md.c	2003-05-02 13:11:57.000000000 -0400
@@ -181,7 +181,6 @@ static void mddev_put(mddev_t *mddev)
 		list_del(&mddev->all_mddevs);
 		mddev_map[mdidx(mddev)] = NULL;
 		kfree(mddev);
-		MOD_DEC_USE_COUNT;
 	}
 	spin_unlock(&all_mddevs_lock);
 }
@@ -203,7 +202,6 @@ static mddev_t * mddev_find(int unit)
 		mddev_map[unit] = new;
 		list_add(&new->all_mddevs, &all_mddevs);
 		spin_unlock(&all_mddevs_lock);
-		MOD_INC_USE_COUNT;
 		return new;
 	}
 	spin_unlock(&all_mddevs_lock);
@@ -1016,7 +1014,12 @@ static int bind_rdev_to_array(mdk_rdev_t
 		if (find_rdev_nr(mddev, rdev->desc_nr))
 			return -EBUSY;
 	}
-			
+
+	/* get a module ref if this is the first disk in the array */
+	if (list_empty(&mddev->disks))
+		if (!try_module_get(THIS_MODULE))
+			return -EBUSY;
+
 	list_add(&rdev->same_set, &mddev->disks);
 	rdev->mddev = mddev;
 	printk(KERN_INFO "md: bind<%s>\n", bdev_partition_name(rdev->bdev));
@@ -1031,6 +1034,11 @@ static void unbind_rdev_from_array(mdk_r
 	}
 	list_del_init(&rdev->same_set);
 	printk(KERN_INFO "md: unbind<%s>\n", bdev_partition_name(rdev->bdev));
+
+	/* drop module ref if this array has no more disks */
+	if (list_empty(&rdev->mddev->disks))
+		module_put(THIS_MODULE);
+
 	rdev->mddev = NULL;
 }
 

Re: [PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Daniel McNeil]

When we attach drives to an array, don't we already have a module
reference to the md module through the md_fops->owner?
The try_module_get() should be a __module_get() since we better already
have a module reference -- doing a try_module_get(THIS_MODULE)
better not add the 1st reference.

Wouldn't another choice be for md_exit() to loop through and drop
all drives that are attached?

Daniel

On Fri, 2003-05-02 at 15:00, Paul Clements wrote:
> Neil Brown wrote:
> > 
> > On Wednesday April 23, Paul.Clements@SteelEye.com wrote:
> > > Neil Brown wrote:
> 
> > It is possible for an array to be half-assembled.  To have some drives
> > attached, but not to have been started.
> > This can happen (for example) if you use mdadm to assemble a raid5
> > array without given all of the drives, and without giving --run.
> > 
> > In this case you don't want 'md' to be unloaded, but you haven't
> > loaded a personality module yet.  So you need to take a reference to
> > the md module whenever you start putting an md array together.
> 
> Ah...I had missed that...appears that was an existing problem, even with
> the MOD_INC_USE_COUNT in place. 
> 
> So we need to do the ref counting at bind/unbind time, rather than
> activation/deactivation time.
> 
> So here's the patch...compiled and tested briefly against
> 2.5.68-bklatest, the ref counting looks correct
> 
> --
> Paul
> 
> ______________________________________________________________________
> 
> --- linux-2.5/drivers/md/md.c.2.5.68-bklatest	2003-05-02 11:56:18.000000000 -0400
> +++ linux-2.5/drivers/md/md.c	2003-05-02 13:11:57.000000000 -0400
> @@ -181,7 +181,6 @@ static void mddev_put(mddev_t *mddev)
>  		list_del(&mddev->all_mddevs);
>  		mddev_map[mdidx(mddev)] = NULL;
>  		kfree(mddev);
> -		MOD_DEC_USE_COUNT;
>  	}
>  	spin_unlock(&all_mddevs_lock);
>  }
> @@ -203,7 +202,6 @@ static mddev_t * mddev_find(int unit)
>  		mddev_map[unit] = new;
>  		list_add(&new->all_mddevs, &all_mddevs);
>  		spin_unlock(&all_mddevs_lock);
> -		MOD_INC_USE_COUNT;
>  		return new;
>  	}
>  	spin_unlock(&all_mddevs_lock);
> @@ -1016,7 +1014,12 @@ static int bind_rdev_to_array(mdk_rdev_t
>  		if (find_rdev_nr(mddev, rdev->desc_nr))
>  			return -EBUSY;
>  	}
> -			
> +
> +	/* get a module ref if this is the first disk in the array */
> +	if (list_empty(&mddev->disks))
> +		if (!try_module_get(THIS_MODULE))
> +			return -EBUSY;
> +
>  	list_add(&rdev->same_set, &mddev->disks);
>  	rdev->mddev = mddev;
>  	printk(KERN_INFO "md: bind<%s>\n", bdev_partition_name(rdev->bdev));
> @@ -1031,6 +1034,11 @@ static void unbind_rdev_from_array(mdk_r
>  	}
>  	list_del_init(&rdev->same_set);
>  	printk(KERN_INFO "md: unbind<%s>\n", bdev_partition_name(rdev->bdev));
> +
> +	/* drop module ref if this array has no more disks */
> +	if (list_empty(&rdev->mddev->disks))
> +		module_put(THIS_MODULE);
> +
>  	rdev->mddev = NULL;
>  }
>  

Re: [PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Paul Clements]

Daniel McNeil wrote:
> 
> When we attach drives to an array, don't we already have a module
> reference to the md module through the md_fops->owner?

No, we could add drives and then leave the md device inactive for some
period with drives attached (which allocates memory)...we have to do
something to avoid md getting unloaded with the drives attached.


> The try_module_get() should be a __module_get() since we better already
> have a module reference -- doing a try_module_get(THIS_MODULE)
> better not add the 1st reference.

Why is that?


> Wouldn't another choice be for md_exit() to loop through and drop
> all drives that are attached?

Yes, that is another option, but presumably you don't want md to get
unloaded if you're in the middle of configuring an array...

Admittedly, some of these scenarios are a little bit far fetched...but
since they can result in a kernel panic or memory leaks, I guess it's
better to be safe than sorry.

--
Paul 
 
> On Fri, 2003-05-02 at 15:00, Paul Clements wrote:
> > Neil Brown wrote:
> > >
> > > On Wednesday April 23, Paul.Clements@SteelEye.com wrote:
> > > > Neil Brown wrote:
> >
> > > It is possible for an array to be half-assembled.  To have some drives
> > > attached, but not to have been started.
> > > This can happen (for example) if you use mdadm to assemble a raid5
> > > array without given all of the drives, and without giving --run.
> > >
> > > In this case you don't want 'md' to be unloaded, but you haven't
> > > loaded a personality module yet.  So you need to take a reference to
> > > the md module whenever you start putting an md array together.
> >
> > Ah...I had missed that...appears that was an existing problem, even with
> > the MOD_INC_USE_COUNT in place.
> >
> > So we need to do the ref counting at bind/unbind time, rather than
> > activation/deactivation time.
> >
> > So here's the patch...compiled and tested briefly against
> > 2.5.68-bklatest, the ref counting looks correct
> >
> > --
> > Paul
> >
> > ______________________________________________________________________
> >
> > --- linux-2.5/drivers/md/md.c.2.5.68-bklatest 2003-05-02 11:56:18.000000000 -0400
> > +++ linux-2.5/drivers/md/md.c 2003-05-02 13:11:57.000000000 -0400
> > @@ -181,7 +181,6 @@ static void mddev_put(mddev_t *mddev)
> >               list_del(&mddev->all_mddevs);
> >               mddev_map[mdidx(mddev)] = NULL;
> >               kfree(mddev);
> > -             MOD_DEC_USE_COUNT;
> >       }
> >       spin_unlock(&all_mddevs_lock);
> >  }
> > @@ -203,7 +202,6 @@ static mddev_t * mddev_find(int unit)
> >               mddev_map[unit] = new;
> >               list_add(&new->all_mddevs, &all_mddevs);
> >               spin_unlock(&all_mddevs_lock);
> > -             MOD_INC_USE_COUNT;
> >               return new;
> >       }
> >       spin_unlock(&all_mddevs_lock);
> > @@ -1016,7 +1014,12 @@ static int bind_rdev_to_array(mdk_rdev_t
> >               if (find_rdev_nr(mddev, rdev->desc_nr))
> >                       return -EBUSY;
> >       }
> > -
> > +
> > +     /* get a module ref if this is the first disk in the array */
> > +     if (list_empty(&mddev->disks))
> > +             if (!try_module_get(THIS_MODULE))
> > +                     return -EBUSY;
> > +
> >       list_add(&rdev->same_set, &mddev->disks);
> >       rdev->mddev = mddev;
> >       printk(KERN_INFO "md: bind<%s>\n", bdev_partition_name(rdev->bdev));
> > @@ -1031,6 +1034,11 @@ static void unbind_rdev_from_array(mdk_r
> >       }
> >       list_del_init(&rdev->same_set);
> >       printk(KERN_INFO "md: unbind<%s>\n", bdev_partition_name(rdev->bdev));
> > +
> > +     /* drop module ref if this array has no more disks */
> > +     if (list_empty(&rdev->mddev->disks))
> > +             module_put(THIS_MODULE);
> > +
> >       rdev->mddev = NULL;
> >  }
> >
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Paul Clements]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

Daniel McNeil wrote:
> The try_module_get() should be a __module_get() since we better already
> have a module reference -- doing a try_module_get(THIS_MODULE)
> better not add the 1st reference.


OK, after looking a little more closely at module.c, I think I see what
you're saying:

try_module_get(THIS_MODULE) is overkill (either that or we're running
inside of md without a module reference to md, which is dangerous). So
__module_get(THIS_MODULE) is enough. 

Here's the updated patch, against 2.5.69, compiled and tested to verify
that module ref counting is correct (esp. in the case of an inactive
array with bound device(s)).

Daniel, thanks for your input on this...

Neil, I believe this patch finally closes all the holes...

--
Paul

[-- Attachment #2: md_mod_ref-2.diff --]
[-- Type: text/plain, Size: 1291 bytes --]

--- linux-2.5/drivers/md/md.c.2.5.69	Sun May 11 22:42:41 2003
+++ linux-2.5/drivers/md/md.c	Sun May 11 22:43:25 2003
@@ -181,7 +181,6 @@ static void mddev_put(mddev_t *mddev)
 		list_del(&mddev->all_mddevs);
 		mddev_map[mdidx(mddev)] = NULL;
 		kfree(mddev);
-		MOD_DEC_USE_COUNT;
 	}
 	spin_unlock(&all_mddevs_lock);
 }
@@ -203,7 +202,6 @@ static mddev_t * mddev_find(int unit)
 		mddev_map[unit] = new;
 		list_add(&new->all_mddevs, &all_mddevs);
 		spin_unlock(&all_mddevs_lock);
-		MOD_INC_USE_COUNT;
 		return new;
 	}
 	spin_unlock(&all_mddevs_lock);
@@ -1016,7 +1014,11 @@ static int bind_rdev_to_array(mdk_rdev_t
 		if (find_rdev_nr(mddev, rdev->desc_nr))
 			return -EBUSY;
 	}
-			
+
+	/* get a module ref if this is the first disk in the array */
+	if (list_empty(&mddev->disks))
+		__module_get(THIS_MODULE);
+
 	list_add(&rdev->same_set, &mddev->disks);
 	rdev->mddev = mddev;
 	printk(KERN_INFO "md: bind<%s>\n", bdev_partition_name(rdev->bdev));
@@ -1031,6 +1033,11 @@ static void unbind_rdev_from_array(mdk_r
 	}
 	list_del_init(&rdev->same_set);
 	printk(KERN_INFO "md: unbind<%s>\n", bdev_partition_name(rdev->bdev));
+
+	/* drop module ref if this array has no more disks */
+	if (list_empty(&rdev->mddev->disks))
+		module_put(THIS_MODULE);
+
 	rdev->mddev = NULL;
 }
 

Re: [PATCH 2.5.68] eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

[Daniel McNeil]

Paul,

This looks good.  I really don't like having the module adding a
reference to itself, but in this case we know that it is not the
first reference.  __module_get() checks this with a BUG_ON.

Having md_exit() removed the attached but not active drives would
be safe:
If someone is in the middle of an ioctl then the module reference 
would not zero and the module couldn't be unloaded.
If someone tries to open an md device after the unload starts,
the opens will fail because try_module_get() fails while a module
is being unloaded.
If the md_exit() cleans up all the attached drives -- no memory
leaks or kernel panics.

This would get rid of the internal __module_get(THIS_MODULE) calls
and makes the code cleaner for module referencing, but
how would an administrator expect this to function? 
Is it normal to have half-assembled arrays or arrays not running
with drives attached?

Anyway, your patch looks good.

Daniel

On Fri, 2003-05-09 at 21:16, Paul Clements wrote:
> Wouldn't another choice be for md_exit() to loop through and drop
> > all drives that are attached?
> 
> Yes, that is another option, but presumably you don't want md to get
> unloaded if you're in the middle of configuring an array...
> 
> Admittedly, some of these scenarios are a little bit far fetched...but
> since they can result in a kernel panic or memory leaks, I guess it's
> better to be safe than sorry.
> 
> --
> Paul

On Mon, 2003-05-12 at 00:13, Paul Clements wrote:
> Daniel McNeil wrote:
> > The try_module_get() should be a __module_get() since we better already
> > have a module reference -- doing a try_module_get(THIS_MODULE)
> > better not add the 1st reference.
> 
> 
> OK, after looking a little more closely at module.c, I think I see what
> you're saying:
> 
> try_module_get(THIS_MODULE) is overkill (either that or we're running
> inside of md without a module reference to md, which is dangerous). So
> __module_get(THIS_MODULE) is enough. 
> 
> Here's the updated patch, against 2.5.69, compiled and tested to verify
> that module ref counting is correct (esp. in the case of an inactive
> array with bound device(s)).
> 
> Daniel, thanks for your input on this...
> 
> Neil, I believe this patch finally closes all the holes...
> 
> --
> Paul