eliminate deprecated MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT in md.c

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

If it looks good, please apply.

Thanks,
Paul--- md.c.PRISTINE 2003-04-22 12:59:37.000000000 -0400
+++ md.c 2003-04-22 12:59:50.000000000 -0400

md.c renamed from md.c.PRISTINE +0 -2

@@ -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);

Quoted 4 lines · On Tuesday April 22, [email protected] 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.

Quoted 7 lines · Neil Brown

Quoted 4 lines · On Tuesday April 22, [email protected] 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)

Quoted 1 line · On Wednesday April 23, [email protected] 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.

Quoted 4 lines · Neil Brown

Quoted 1 line · On Wednesday April 23, [email protected] wrote:

Neil Brown wrote:

Quoted 8 lines

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

______________________________________________________________________

drivers/md/md.c renamed from drivers/md/md.c.2.5.68-bklatest +11 -3

@@ -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;
 }
 

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?

Quoted 62 lines

Quoted 4 lines · Neil Brown

Quoted 1 line · On Wednesday April 23, [email protected] wrote:

Neil Brown wrote:

Quoted 8 lines

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

______________________________________________________________________

drivers/md/md.c 2003-05-02 13:11:57.000000000 -0400 renamed from drivers/md/md.c.2.5.68-bklatest 2003-05-02 11:56:18.000000000 -0400 +6 -3

@@ -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
      }

drivers/md/md.c 2003-05-02 13:11:57.000000000 -0400 static void unbind_rdev_from_array(mdk_r · line 1044

Quoted 10 lines

drivers/md/md.c 2003-05-02 13:11:57.000000000 -0400 renamed from drivers/md/md.c.2.5.68-bklatest 2003-05-02 11:56:18.000000000 -0400 +5 -0

      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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

Quoted 1 line

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

Quoted 3 lines · Daniel McNeil

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