Patchwork [UBI] 1/5 - UBI notifications, take two

login
register
mail settings
Submitter dmitry pervushin
Date Dec. 15, 2008, 11:13 a.m.
Message ID <1229339635.7900.21.camel@hp.diimka.lan>
Download mbox | patch
Permalink /patch/14016/
State New
Headers show

Comments

dmitry pervushin - Dec. 15, 2008, 11:13 a.m.
Implement notifications about UBI volume changes

Signed-off-by: dmitry pervushin <dimka@embeddedalley.com>

---
 drivers/mtd/ubi/build.c |   39 +++++++++++++++++++++++++++++++++++++++
 drivers/mtd/ubi/cdev.c  |    2 ++
 drivers/mtd/ubi/kapi.c  |   25 +++++++++++++++++++++++++
 drivers/mtd/ubi/ubi.h   |   35 +++++++++++++++++++++++++++++++++++
 drivers/mtd/ubi/vmt.c   |    8 ++++++++
 include/linux/mtd/ubi.h |   16 ++++++++++++++++
 6 files changed, 125 insertions(+)
Artem Bityutskiy - Dec. 15, 2008, 2:04 p.m.
Thanks for the patch, but I have some concerns. Please clarify some
things.

1. If X is subscribed to the notification, you assume that can X open
UBI volumes from within the notification. At least I can see this in
your "simple FTL". But this is recursion, ant it is very tricky. And I
see problems related to this in your code. I may be mistaken, though.
See below.

Could you please document things like this at the kernel-doc comments of
the 'ubi_unregister_volume_notifier()' function? Even if no UBI API
calls are allowed, this should be documented there.

Some of my notes below are very minor and are really nit-picking. I do
not _require_ you to fix them, although would appreciate.

On Mon, 2008-12-15 at 14:13 +0300, dmitry pervushin wrote:
> Index: ubifs-2.6/drivers/mtd/ubi/build.c
> ===================================================================
> --- ubifs-2.6.orig/drivers/mtd/ubi/build.c
> +++ ubifs-2.6/drivers/mtd/ubi/build.c
> @@ -122,6 +122,51 @@ static struct device_attribute dev_mtd_n
>  	__ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
>  
>  /**
> + * ubi_enum_volumes - enumerate all existing volumes and send notification
Dot at the end please. Glance at other doc-book comments, let's be
consistent.
> + *
Kernel-doc used to issue an error if there is an empty line like this
between the function name and parameters list. Again, glance at other
doc-book comments, let's be consistent.
> + * @t: notification type to send
> + * @ubi: UBI device number
> + * @nb: notifier to be called
> + *
> + * Walk on volume list that are created on device @ubi, or if @ubi < 0, on all
> + * available UBI devices. For each volume, send the notification - either
> + * system-wide if @nb is NULL, or only to the registered @nb
%NULL, dot.
> + *
> + * Returns number of volumes processed
Dot.
> + */
> +int ubi_enum_volumes(enum ubi_volume_notification_type t, int ubi,
> +		struct notifier_block *nb)
Other UBI code always alling the second line to the start of parameters
description. E.g., glance at 'dev_attribute_show()'.

'ubi' name is used only for 'struct ubi_info' everywhere, please, pick a
different name. I would rename it to 'ubi_num', as the other code has.
> +{
> +	int ubi_num, i, count = 0;
And I'd re-name this 'ubi_num' to 'n' or something.
> +	struct ubi_device *ubi_dev;
> +	struct ubi_volume_notification nt;
> +
> +	ubi_num = ubi < 0 ? 0 : ubi;
> +	spin_lock(&ubi_devices_lock);
> +	do {
> +		nt.ubi_num = ubi_num++;
> +		ubi_dev = ubi_devices[nt.ubi_num];
> +		if (!ubi_dev)
> +			continue;
> +		spin_lock(&ubi_dev->volumes_lock);
> +		for (i = 0; i < ubi_dev->vtbl_slots; i++) {
> +			if (!ubi_dev->volumes[i])
> +				continue;
> +			nt.vol_id = ubi_dev->volumes[i]->vol_id;
> +			if (nb)
> +				nb->notifier_call(nb, t, &nt);
> +			else
> +				blocking_notifier_call_chain(&ubi_notifiers,
> +					t, &nt);
And here the rest of the UBI code would use

		blocking_notifier_call_chain(&ubi_notifiers,
		                             t, &nt);
> +			count++;
> +		}
> +		spin_unlock(&ubi_dev->volumes_lock);
> +	} while (ubi < 0 && ubi_num < UBI_MAX_DEVICES);
> +	spin_unlock(&ubi_devices_lock);
> +	return count;
> +}

So you call notifiers from withing spin-locks. Are they really blocking
notifiers? Note, if you call any UBI kernel API function from the
notifier, you'll deadlock. E.g., if you call 'ubi_get_device_info()',
you'll deadlock on 'ubi_devices_lock'. Did you test your code?

I guess you should prohibit recursion and pass full UBI device/volume
information _inside_ the notifier. And the subsystems which work above
UBI should never _open_ UBI volumes from within notifiers. E.g., the
"simple FTL" stuff should open the UBI volume only when the
corresponding FTL block device is opened, not in the notifier.

Also, this function seems to be over-complicated. I'd have 2 separate
functions instead - one for the purposes of build.c and one for kapi.c.
These "negative or positive @ubi, %NULL or non-%NULL @np" are too
confusing.

Enough, there were more tiny things though.
dmitry pervushin - Dec. 17, 2008, 7:53 p.m.
On Mon, 2008-12-15 at 16:04 +0200, Artem Bityutskiy wrote:
> Thanks for the patch, but I have some concerns. Please clarify some
> things.
> 
> 1. If X is subscribed to the notification, you assume that can X open
> UBI volumes from within the notification. At least I can see this in
> your "simple FTL". But this is recursion, ant it is very tricky. And I
> see problems related to this in your code. I may be mistaken, though.
> See below.
Thanks for reporting this. This is really a problem, and I'd suggest to
create additional mode in addition to
UBI_READONLY/UBI_READWRITE/UBI_EXCLUSIVE, say, UBI_OPEN_INTERNAL, to
increase reference count on the volume. 
> So you call notifiers from withing spin-locks. Are they really blocking
> notifiers? Note, if you call any UBI kernel API function from the
> notifier, you'll deadlock. E.g., if you call 'ubi_get_device_info()',
> you'll deadlock on 'ubi_devices_lock'. Did you test your code?
> 
> I guess you should prohibit recursion and pass full UBI device/volume
> information _inside_ the notifier. And the subsystems which work above
> UBI should never _open_ UBI volumes from within notifiers. E.g., the
> "simple FTL" stuff should open the UBI volume only when the
> corresponding FTL block device is opened, not in the notifier.
Although it is a good idea and it will save some time for notified
modules -- it won't help me. The block device created by ftl could be
opened immediately after creating, err.., actually, in the middle of
creating - e.g., to read partition table.

The ubi_enum_volumes could open the volume with the flag
UBI_OPEN_INTERNAL and thus release spinlock when calling notifiers.

Comments?
> -- 
> Best regards,
> Artem Bityutskiy (Битюцкий Артём)
>
Artem Bityutskiy - Dec. 18, 2008, 7:31 a.m.
On Wed, 2008-12-17 at 22:53 +0300, dmitry pervushin wrote:
> > So you call notifiers from withing spin-locks. Are they really blocking
> > notifiers? Note, if you call any UBI kernel API function from the
> > notifier, you'll deadlock. E.g., if you call 'ubi_get_device_info()',
> > you'll deadlock on 'ubi_devices_lock'. Did you test your code?
> > 
> > I guess you should prohibit recursion and pass full UBI device/volume
> > information _inside_ the notifier. And the subsystems which work above
> > UBI should never _open_ UBI volumes from within notifiers. E.g., the
> > "simple FTL" stuff should open the UBI volume only when the
> > corresponding FTL block device is opened, not in the notifier.
> Although it is a good idea and it will save some time for notified
> modules -- it won't help me. The block device created by ftl could be
> opened immediately after creating, err.., actually, in the middle of
> creating - e.g., to read partition table.
> 
> The ubi_enum_volumes could open the volume with the flag
> UBI_OPEN_INTERNAL and thus release spinlock when calling notifiers.

You may introduce a "ubi_get_volume(struct ubi_info *ubi, int vol_id)"
function, similar to the existing "ubi_get_device(int ubi_num)"
function. However, it looks like you may just use UBI_READONLY instead.
It will not prevent the "notifyees" to open the volume in UBI_READWRITE
mode.
dmitry pervushin - Dec. 18, 2008, 11:07 a.m.
On Thu, 2008-12-18 at 09:31 +0200, Artem Bityutskiy wrote:
> On Wed, 2008-12-17 at 22:53 +0300, dmitry pervushin wrote:
> > > So you call notifiers from withing spin-locks. Are they really blocking
> > > notifiers? Note, if you call any UBI kernel API function from the
> > > notifier, you'll deadlock. E.g., if you call 'ubi_get_device_info()',
> > > you'll deadlock on 'ubi_devices_lock'. Did you test your code?
> > > 
> > > I guess you should prohibit recursion and pass full UBI device/volume
> > > information _inside_ the notifier. And the subsystems which work above
> > > UBI should never _open_ UBI volumes from within notifiers. E.g., the
> > > "simple FTL" stuff should open the UBI volume only when the
> > > corresponding FTL block device is opened, not in the notifier.
> > Although it is a good idea and it will save some time for notified
> > modules -- it won't help me. The block device created by ftl could be
> > opened immediately after creating, err.., actually, in the middle of
> > creating - e.g., to read partition table.
> > 
> > The ubi_enum_volumes could open the volume with the flag
> > UBI_OPEN_INTERNAL and thus release spinlock when calling notifiers.
> 
> You may introduce a "ubi_get_volume(struct ubi_info *ubi, int vol_id)"
> function, similar to the existing "ubi_get_device(int ubi_num)"
> function. However, it looks like you may just use UBI_READONLY instead.
> It will not prevent the "notifyees" to open the volume in UBI_READWRITE
> mode.
But will prevent opening in UBI_EXCLUSIVE. OK, get_volume/put_volume
looks as the right way to go.
Artem Bityutskiy - Dec. 18, 2008, 11:11 a.m.
On Thu, 2008-12-18 at 14:07 +0300, dmitry pervushin wrote:
> On Thu, 2008-12-18 at 09:31 +0200, Artem Bityutskiy wrote:
> > On Wed, 2008-12-17 at 22:53 +0300, dmitry pervushin wrote:
> > > > So you call notifiers from withing spin-locks. Are they really blocking
> > > > notifiers? Note, if you call any UBI kernel API function from the
> > > > notifier, you'll deadlock. E.g., if you call 'ubi_get_device_info()',
> > > > you'll deadlock on 'ubi_devices_lock'. Did you test your code?
> > > > 
> > > > I guess you should prohibit recursion and pass full UBI device/volume
> > > > information _inside_ the notifier. And the subsystems which work above
> > > > UBI should never _open_ UBI volumes from within notifiers. E.g., the
> > > > "simple FTL" stuff should open the UBI volume only when the
> > > > corresponding FTL block device is opened, not in the notifier.
> > > Although it is a good idea and it will save some time for notified
> > > modules -- it won't help me. The block device created by ftl could be
> > > opened immediately after creating, err.., actually, in the middle of
> > > creating - e.g., to read partition table.
> > > 
> > > The ubi_enum_volumes could open the volume with the flag
> > > UBI_OPEN_INTERNAL and thus release spinlock when calling notifiers.
> > 
> > You may introduce a "ubi_get_volume(struct ubi_info *ubi, int vol_id)"
> > function, similar to the existing "ubi_get_device(int ubi_num)"
> > function. However, it looks like you may just use UBI_READONLY instead.
> > It will not prevent the "notifyees" to open the volume in UBI_READWRITE
> > mode.
> But will prevent opening in UBI_EXCLUSIVE. OK, get_volume/put_volume
> looks as the right way to go.

It will prevent exlusive, as well as get/put must prevent exclusive, so
there should be no difference for you at all. Exclusive is used when the
module is removed or re-sized, and there must be zero users if these
operations are performed.
Artem Bityutskiy - Dec. 18, 2008, 11:14 a.m.
On Thu, 2008-12-18 at 13:11 +0200, Artem Bityutskiy wrote:
> > > > The ubi_enum_volumes could open the volume with the flag
> > > > UBI_OPEN_INTERNAL and thus release spinlock when calling notifiers.
> > > 
> > > You may introduce a "ubi_get_volume(struct ubi_info *ubi, int vol_id)"
> > > function, similar to the existing "ubi_get_device(int ubi_num)"
> > > function. However, it looks like you may just use UBI_READONLY instead.
> > > It will not prevent the "notifyees" to open the volume in UBI_READWRITE
> > > mode.
> > But will prevent opening in UBI_EXCLUSIVE. OK, get_volume/put_volume
> > looks as the right way to go.
> 
> It will prevent exlusive, as well as get/put must prevent exclusive, so
> there should be no difference for you at all. Exclusive is used when the
> module is removed or re-sized, and there must be zero users if these
> operations are performed.

IOW, these get/put volume calls should not bring any value for you and
seem to be unnecessary.

Patch

Index: ubifs-2.6/drivers/mtd/ubi/build.c
===================================================================
--- ubifs-2.6.orig/drivers/mtd/ubi/build.c
+++ ubifs-2.6/drivers/mtd/ubi/build.c
@@ -122,6 +122,51 @@  static struct device_attribute dev_mtd_n
 	__ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
 
 /**
+ * ubi_enum_volumes - enumerate all existing volumes and send notification
+ *
+ * @t: notification type to send
+ * @ubi: UBI device number
+ * @nb: notifier to be called
+ *
+ * Walk on volume list that are created on device @ubi, or if @ubi < 0, on all
+ * available UBI devices. For each volume, send the notification - either
+ * system-wide if @nb is NULL, or only to the registered @nb
+ *
+ * Returns number of volumes processed
+ */
+int ubi_enum_volumes(enum ubi_volume_notification_type t, int ubi,
+		struct notifier_block *nb)
+{
+	int ubi_num, i, count = 0;
+	struct ubi_device *ubi_dev;
+	struct ubi_volume_notification nt;
+
+	ubi_num = ubi < 0 ? 0 : ubi;
+	spin_lock(&ubi_devices_lock);
+	do {
+		nt.ubi_num = ubi_num++;
+		ubi_dev = ubi_devices[nt.ubi_num];
+		if (!ubi_dev)
+			continue;
+		spin_lock(&ubi_dev->volumes_lock);
+		for (i = 0; i < ubi_dev->vtbl_slots; i++) {
+			if (!ubi_dev->volumes[i])
+				continue;
+			nt.vol_id = ubi_dev->volumes[i]->vol_id;
+			if (nb)
+				nb->notifier_call(nb, t, &nt);
+			else
+				blocking_notifier_call_chain(&ubi_notifiers,
+					t, &nt);
+			count++;
+		}
+		spin_unlock(&ubi_dev->volumes_lock);
+	} while (ubi < 0 && ubi_num < UBI_MAX_DEVICES);
+	spin_unlock(&ubi_devices_lock);
+	return count;
+}
+
+/**
  * ubi_get_device - get UBI device.
  * @ubi_num: UBI device number
  *
@@ -876,6 +921,7 @@  int ubi_attach_mtd_dev(struct mtd_info *
 	wake_up_process(ubi->bgt_thread);
 
 	ubi_devices[ubi_num] = ubi;
+	ubi_enum_volumes(UBI_VOLUME_ADDED, ubi_num, NULL);
 	return ubi_num;
 
 out_uif:
@@ -937,6 +983,8 @@  int ubi_detach_mtd_dev(int ubi_num, int
 	ubi_devices[ubi_num] = NULL;
 	spin_unlock(&ubi_devices_lock);
 
+	ubi_enum_volumes(UBI_VOLUME_DELETED, ubi_num, NULL);
+
 	ubi_assert(ubi_num == ubi->ubi_num);
 	dbg_msg("detaching mtd%d from ubi%d", ubi->mtd->index, ubi_num);
 
Index: ubifs-2.6/drivers/mtd/ubi/cdev.c
===================================================================
--- ubifs-2.6.orig/drivers/mtd/ubi/cdev.c
+++ ubifs-2.6/drivers/mtd/ubi/cdev.c
@@ -396,6 +396,8 @@  static ssize_t vol_cdev_write(struct fil
 		}
 		vol->checked = 1;
 		ubi_gluebi_updated(vol);
+		ubi_volume_notify(UBI_VOLUME_CHANGED,
+				ubi->ubi_num, vol->vol_id);
 		revoke_exclusive(desc, UBI_READWRITE);
 	}
 
Index: ubifs-2.6/drivers/mtd/ubi/kapi.c
===================================================================
--- ubifs-2.6.orig/drivers/mtd/ubi/kapi.c
+++ ubifs-2.6/drivers/mtd/ubi/kapi.c
@@ -656,3 +656,52 @@  int ubi_sync(int ubi_num)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ubi_sync);
+
+BLOCKING_NOTIFIER_HEAD(ubi_notifiers);
+
+/**
+ * ubi_register_volume_notifier - register the volume notification function
+ *
+ * @nb: pointer to the filled struct &notifier_block
+ * @ignore_existing: boolean flag; if set to 1, UBI will not send
+ * 	notifications about ADDing existing volumes
+ *
+ * The function @nb.notifier_call will be called when volume is added,
+ * removed, resized or renamed. Its first parameter is &enum
+ * ubi_volume_notification_type, and the second points to the structure
+ * that contains information about "changed" volume - ubi_device_num and
+ * volume_id. When the notifier is called, it is safe to use all UBI API.
+ *
+ * Returns %0 on success, error code otherwise
+ */
+int ubi_register_volume_notifier(struct notifier_block *nb,
+				int ignore_existing)
+{
+	int r;
+
+	r = blocking_notifier_chain_register(&ubi_notifiers, nb);
+	if (!r)
+		goto out;
+	if (ignore_existing)
+		goto out;
+	down_read(&ubi_notifiers.rwsem);
+	ubi_enum_volumes(UBI_VOLUME_ADDED, -1, nb);
+	up_read(&ubi_notifiers.rwsem);
+out:
+	return r;
+}
+EXPORT_SYMBOL_GPL(ubi_register_volume_notifier);
+
+/**
+ * ubi_unregister_volume_notifier - unregister the volume notifier registered
+ * by ubi_register_volume_notifier
+ *
+ * @nb: pointer to the filled struct &notifier_block
+ *
+ * Returns %0 on success, error code otherwise
+ */
+int ubi_unregister_volume_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&ubi_notifiers, nb);
+}
+EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier);
Index: ubifs-2.6/drivers/mtd/ubi/ubi.h
===================================================================
--- ubifs-2.6.orig/drivers/mtd/ubi/ubi.h
+++ ubifs-2.6/drivers/mtd/ubi/ubi.h
@@ -38,6 +38,7 @@ 
 #include <linux/vmalloc.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/ubi.h>
+#include <linux/notifier.h>
 
 #include "ubi-media.h"
 #include "scan.h"
@@ -447,6 +448,7 @@  extern struct file_operations ubi_cdev_o
 extern struct file_operations ubi_vol_cdev_operations;
 extern struct class *ubi_class;
 extern struct mutex ubi_devices_mutex;
+extern struct blocking_notifier_head ubi_notifiers;
 
 /* vtbl.c */
 int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
@@ -539,7 +541,40 @@  struct ubi_device *ubi_get_device(int ub
 void ubi_put_device(struct ubi_device *ubi);
 struct ubi_device *ubi_get_by_major(int major);
 int ubi_major2num(int major);
+int ubi_enum_volumes(enum ubi_volume_notification_type t,
+		int ubi_num, struct notifier_block *nb);
 
+static inline char *ubi_notification(enum ubi_volume_notification_type t)
+{
+	static char *nstr[] = {
+	 [UBI_VOLUME_ADDED]	= "Added",
+	 [UBI_VOLUME_DELETED]	= "Deleted",
+	 [UBI_VOLUME_RENAMED]	= "Renamed",
+	 [UBI_VOLUME_CHANGED]	= "Changed",
+	};
+	return nstr[t];
+}
+/**
+ * ubi_volume_notify - notify all registered clients about volume changes
+ *
+ * @t: notification type
+ * @ubi_device: device number
+ * @volume_name: name of created/removed/changed volume
+ */
+static inline int ubi_volume_notify(enum ubi_volume_notification_type t,
+		int ubi_num, int volume_id)
+{
+       struct ubi_volume_notification nt;
+
+       nt.ubi_num = ubi_num;
+       nt.vol_id = volume_id;
+       dbg_gen("%s: %s volume id %d on device %d\n",
+		       __func__,
+		       ubi_notification(t),
+		       volume_id,
+		       ubi_num);
+       return blocking_notifier_call_chain(&ubi_notifiers, t, &nt);
+}
 /*
  * ubi_rb_for_each_entry - walk an RB-tree.
  * @rb: a pointer to type 'struct rb_node' to to use as a loop counter
Index: ubifs-2.6/drivers/mtd/ubi/vmt.c
===================================================================
--- ubifs-2.6.orig/drivers/mtd/ubi/vmt.c
+++ ubifs-2.6/drivers/mtd/ubi/vmt.c
@@ -361,6 +361,7 @@  int ubi_create_volume(struct ubi_device
 	ubi->vol_count += 1;
 	spin_unlock(&ubi->volumes_lock);
 
+	ubi_volume_notify(UBI_VOLUME_ADDED, ubi->ubi_num, vol->vol_id);
 	err = paranoid_check_volumes(ubi);
 	return err;
 
@@ -431,6 +432,7 @@  int ubi_remove_volume(struct ubi_volume_
 		err = -EBUSY;
 		goto out_unlock;
 	}
+	ubi_volume_notify(UBI_VOLUME_DELETED, ubi->ubi_num, vol->vol_id);
 	ubi->volumes[vol_id] = NULL;
 	spin_unlock(&ubi->volumes_lock);
 
@@ -473,6 +475,7 @@  int ubi_remove_volume(struct ubi_volume_
 	return err;
 
 out_err:
+	ubi_volume_notify(UBI_VOLUME_ADDED, ubi->ubi_num, vol->vol_id);
 	ubi_err("cannot remove volume %d, error %d", vol_id, err);
 	spin_lock(&ubi->volumes_lock);
 	ubi->volumes[vol_id] = vol;
@@ -590,6 +593,7 @@  int ubi_resize_volume(struct ubi_volume_
 			(long long)vol->used_ebs * vol->usable_leb_size;
 	}
 
+	ubi_volume_notify(UBI_VOLUME_CHANGED, ubi->ubi_num, vol->vol_id);
 	err = paranoid_check_volumes(ubi);
 	return err;
 
@@ -635,6 +639,8 @@  int ubi_rename_volumes(struct ubi_device
 			vol->name_len = re->new_name_len;
 			memcpy(vol->name, re->new_name, re->new_name_len + 1);
 			spin_unlock(&ubi->volumes_lock);
+			ubi_volume_notify(UBI_VOLUME_RENAMED,
+				ubi->ubi_num, vol->vol_id);
 		}
 	}
 
Index: ubifs-2.6/include/linux/mtd/ubi.h
===================================================================
--- ubifs-2.6.orig/include/linux/mtd/ubi.h
+++ ubifs-2.6/include/linux/mtd/ubi.h
@@ -184,4 +184,20 @@  static inline int ubi_change(struct ubi_
 	return ubi_leb_change(desc, lnum, buf, len, UBI_UNKNOWN);
 }
 
+int ubi_register_volume_notifier(struct notifier_block *nb,
+		int ignore_existing);
+int ubi_unregister_volume_notifier(struct notifier_block *nb);
+
+struct ubi_volume_notification {
+	int ubi_num;
+	int vol_id;
+};
+
+enum ubi_volume_notification_type {
+	UBI_VOLUME_ADDED,
+	UBI_VOLUME_DELETED,
+	UBI_VOLUME_CHANGED,
+	UBI_VOLUME_RENAMED,
+};
+
 #endif /* !__LINUX_UBI_H__ */