lkml.org 
[lkml]   [2008]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] block: fix block_class iteration locking
On Mon, 14 Jul 2008 15:06:08 +0900,
Tejun Heo <tj@kernel.org> wrote:

> genhd.c used block_class_lock when it wants to iterate over
> block_class.devices list. This is incorrect as block_class_lock has
> nothing to do with synchronization around block_class.devices list, so
> there's a window, albeit small, where the list can change while it's
> being iterated over. Fix it by using block_class.sem instead.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>

Greg has some patches sitting in his tree that convert those functions
to use class_{for_each,find}_device() instead - and that looks like the
clean solution, especially since the class implementation details will
be moved into a private structure. One worry though...


> @@ -285,7 +285,7 @@ static void *part_start(struct seq_file *part, loff_t *pos)
> loff_t k = *pos;
> struct device *dev;
>
> - mutex_lock(&block_class_lock);
> + down(&block_class.sem);
> list_for_each_entry(dev, &block_class.devices, node) {
> if (dev->type != &disk_type)
> continue;
> @@ -311,7 +311,7 @@ static void *part_next(struct seq_file *part, void *v, loff_t *pos)
>
> static void part_stop(struct seq_file *part, void *v)
> {
> - mutex_unlock(&block_class_lock);
> + up(&block_class.sem);
> }
>
> static int show_partition(struct seq_file *part, void *v)
> @@ -533,7 +533,7 @@ static void *diskstats_start(struct seq_file *part, loff_t *pos)
> loff_t k = *pos;
> struct device *dev;
>
> - mutex_lock(&block_class_lock);
> + down(&block_class.sem);
> list_for_each_entry(dev, &block_class.devices, node) {
> if (dev->type != &disk_type)
> continue;
> @@ -560,7 +560,7 @@ static void *diskstats_next(struct seq_file *part, void *v, loff_t *pos)
>
> static void diskstats_stop(struct seq_file *part, void *v)
> {
> - mutex_unlock(&block_class_lock);
> + up(&block_class.sem);
> }
>
> static int diskstats_show(struct seq_file *s, void *v)

With your patch, you protect against adding/removing devices to/from
the class between _start and _stop. This wasn't done previously, and
won't be done with Greg's patches (which leaves in the
locking/unlocking of block_class_lock). Not sure if I'm missing
something here...


\
 
 \ /
  Last update: 2008-07-14 13:45    [W:0.066 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site