| Message ID | 1229339635.7900.21.camel@hp.diimka.lan |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 (Битюцкий Артём) >
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.
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.
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.
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.
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 ¬ifier_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 ¬ifier_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__ */
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(+)