* [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
@ 2005-06-16 22:51 ` Jun'ichi Nomura
0 siblings, 0 replies; 5+ messages in thread
From: Jun'ichi Nomura @ 2005-06-16 22:51 UTC (permalink / raw)
To: Alasdair Kergon, device-mapper development; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 947 bytes --]
Hello,
Attached patch fixes the another bug in dm-raid1.c that
the dirty region may stay in or be moved to clean list
and freed while in use.
It happens as follows:
CPU0 CPU1
------------------------------------------------------------------------------
rh_dec()
if (atomic_dec_and_test(pending))
<the region is still marked dirty>
rh_inc()
if the region is clean
mark the region dirty
and remove from clean list
mark the region clean
and move to clean list
atomic_inc(pending)
At this stage, the region is in clean list and
will be mistakenly reclaimed by rh_update_states() later.
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
[-- Attachment #2: dm-raid1-race2.patch --]
[-- Type: text/x-patch, Size: 1071 bytes --]
--- kernel/drivers/md/dm-raid1.c.orig 2005-06-16 07:13:50.610325768 -0400
+++ kernel/drivers/md/dm-raid1.c 2005-06-16 10:34:12.510719112 -0400
@@ -375,16 +380,18 @@ static void rh_inc(struct region_hash *r
read_lock(&rh->hash_lock);
reg = __rh_find(rh, region);
+
+ atomic_inc(®->pending);
+
+ spin_lock_irq(&rh->region_lock);
if (reg->state == RH_CLEAN) {
rh->log->type->mark_region(rh->log, reg->key);
- spin_lock_irq(&rh->region_lock);
reg->state = RH_DIRTY;
list_del_init(®->list); /* take off the clean list */
- spin_unlock_irq(&rh->region_lock);
}
+ spin_unlock_irq(&rh->region_lock);
- atomic_inc(®->pending);
read_unlock(&rh->hash_lock);
}
@@ -408,6 +414,10 @@ static void rh_dec(struct region_hash *r
if (atomic_dec_and_test(®->pending)) {
spin_lock_irqsave(&rh->region_lock, flags);
+ if (atomic_read(®->pending)) { /* check race */
+ spin_unlock_irqrestore(&rh->region_lock, flags);
+ return;
+ }
if (reg->state == RH_RECOVERING) {
list_add_tail(®->list, &rh->quiesced_regions);
} else {
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
@ 2005-06-16 22:51 ` Jun'ichi Nomura
0 siblings, 0 replies; 5+ messages in thread
From: Jun'ichi Nomura @ 2005-06-16 22:51 UTC (permalink / raw)
To: Alasdair Kergon, device-mapper development; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 947 bytes --]
Hello,
Attached patch fixes the another bug in dm-raid1.c that
the dirty region may stay in or be moved to clean list
and freed while in use.
It happens as follows:
CPU0 CPU1
------------------------------------------------------------------------------
rh_dec()
if (atomic_dec_and_test(pending))
<the region is still marked dirty>
rh_inc()
if the region is clean
mark the region dirty
and remove from clean list
mark the region clean
and move to clean list
atomic_inc(pending)
At this stage, the region is in clean list and
will be mistakenly reclaimed by rh_update_states() later.
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
[-- Attachment #2: dm-raid1-race2.patch --]
[-- Type: text/x-patch, Size: 1071 bytes --]
--- kernel/drivers/md/dm-raid1.c.orig 2005-06-16 07:13:50.610325768 -0400
+++ kernel/drivers/md/dm-raid1.c 2005-06-16 10:34:12.510719112 -0400
@@ -375,16 +380,18 @@ static void rh_inc(struct region_hash *r
read_lock(&rh->hash_lock);
reg = __rh_find(rh, region);
+
+ atomic_inc(®->pending);
+
+ spin_lock_irq(&rh->region_lock);
if (reg->state == RH_CLEAN) {
rh->log->type->mark_region(rh->log, reg->key);
- spin_lock_irq(&rh->region_lock);
reg->state = RH_DIRTY;
list_del_init(®->list); /* take off the clean list */
- spin_unlock_irq(&rh->region_lock);
}
+ spin_unlock_irq(&rh->region_lock);
- atomic_inc(®->pending);
read_unlock(&rh->hash_lock);
}
@@ -408,6 +414,10 @@ static void rh_dec(struct region_hash *r
if (atomic_dec_and_test(®->pending)) {
spin_lock_irqsave(&rh->region_lock, flags);
+ if (atomic_read(®->pending)) { /* check race */
+ spin_unlock_irqrestore(&rh->region_lock, flags);
+ return;
+ }
if (reg->state == RH_RECOVERING) {
list_add_tail(®->list, &rh->quiesced_regions);
} else {
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
2005-06-16 22:51 ` Jun'ichi Nomura
(?)
@ 2005-06-24 15:19 ` Jonathan E Brassow
2005-06-24 15:45 ` Jun'ichi Nomura
-1 siblings, 1 reply; 5+ messages in thread
From: Jonathan E Brassow @ 2005-06-24 15:19 UTC (permalink / raw)
To: device-mapper development; +Cc: Nomura Jun'ichi
could this be solved by doing your patch in rh_dec and just moving the
atomic_inc in rh_inc? The reason I ask is that the mark_region log
call can block.
brassow
On Jun 16, 2005, at 5:51 PM, Jun'ichi Nomura wrote:
> Hello,
>
> Attached patch fixes the another bug in dm-raid1.c that
> the dirty region may stay in or be moved to clean list
> and freed while in use.
>
> It happens as follows:
>
> CPU0 CPU1
>
> -----------------------------------------------------------------------
> -------
> rh_dec()
> if (atomic_dec_and_test(pending))
> <the region is still marked dirty>
> rh_inc()
> if the region is clean
> mark the region dirty
> and remove from clean
> list
> mark the region clean
> and move to clean list
> atomic_inc(pending)
>
> At this stage, the region is in clean list and
> will be mistakenly reclaimed by rh_update_states() later.
>
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
>
> --- kernel/drivers/md/dm-raid1.c.orig 2005-06-16 07:13:50.610325768
> -0400
> +++ kernel/drivers/md/dm-raid1.c 2005-06-16 10:34:12.510719112 -0400
> @@ -375,16 +380,18 @@ static void rh_inc(struct region_hash *r
>
> read_lock(&rh->hash_lock);
> reg = __rh_find(rh, region);
> +
> + atomic_inc(®->pending);
> +
> + spin_lock_irq(&rh->region_lock);
> if (reg->state == RH_CLEAN) {
> rh->log->type->mark_region(rh->log, reg->key);
>
> - spin_lock_irq(&rh->region_lock);
> reg->state = RH_DIRTY;
> list_del_init(®->list); /* take off the clean list */
> - spin_unlock_irq(&rh->region_lock);
> }
> + spin_unlock_irq(&rh->region_lock);
>
> - atomic_inc(®->pending);
> read_unlock(&rh->hash_lock);
> }
>
> @@ -408,6 +414,10 @@ static void rh_dec(struct region_hash *r
>
> if (atomic_dec_and_test(®->pending)) {
> spin_lock_irqsave(&rh->region_lock, flags);
> + if (atomic_read(®->pending)) { /* check race */
> + spin_unlock_irqrestore(&rh->region_lock, flags);
> + return;
> + }
> if (reg->state == RH_RECOVERING) {
> list_add_tail(®->list, &rh->quiesced_regions);
> } else {
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
2005-06-24 15:19 ` Jonathan E Brassow
@ 2005-06-24 15:45 ` Jun'ichi Nomura
2005-06-27 17:34 ` Jun'ichi Nomura
0 siblings, 1 reply; 5+ messages in thread
From: Jun'ichi Nomura @ 2005-06-24 15:45 UTC (permalink / raw)
To: Jonathan Brassow; +Cc: device-mapper development
Hi Jon,
Jonathan E Brassow wrote:
> could this be solved by doing your patch in rh_dec and just moving the
> atomic_inc in rh_inc? The reason I ask is that the mark_region log
> call can block.
No.
Unless they are serialized, it's possible that rh_inc() will see the
state RH_DIRTY, while rh_dec change it to RH_CLEAN.
As a result, the region which has I/O in-flight may be freed.
Is it reasonable to call mark_region() unconditionally?
Then we can call it outside of the lock.
>> CPU0 CPU1
>>
>> -----------------------------------------------------------------------
>> -------
>> rh_dec()
>> if (atomic_dec_and_test(pending))
>> <the region is still marked dirty>
if (atomic_read(pending)==0)
>> rh_inc()
>> atomic_inc(pending)
>> if the region is clean
>> mark the region dirty
>> and remove from clean list
else do nothing
>> mark the region clean
>> and move to clean list
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c
2005-06-24 15:45 ` Jun'ichi Nomura
@ 2005-06-27 17:34 ` Jun'ichi Nomura
0 siblings, 0 replies; 5+ messages in thread
From: Jun'ichi Nomura @ 2005-06-27 17:34 UTC (permalink / raw)
To: device-mapper development
[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]
Hello,
I revised the patch based on comments from Jon.
Attached patch should work on the version of mark_region which may block.
Thanks,
Jun'ichi Nomura wrote:
> Jonathan E Brassow wrote:
> > could this be solved by doing your patch in rh_dec and just moving the
> > atomic_inc in rh_inc? The reason I ask is that the mark_region log
> > call can block.
>
> No.
> Unless they are serialized, it's possible that rh_inc() will see the
> state RH_DIRTY, while rh_dec change it to RH_CLEAN.
> As a result, the region which has I/O in-flight may be freed.
>
> Is it reasonable to call mark_region() unconditionally?
> Then we can call it outside of the lock.
>
> >> CPU0 CPU1
> >>
> >> -----------------------------------------------------------------------
> >> -------
> >> rh_dec()
> >> if (atomic_dec_and_test(pending))
> >> <the region is still marked dirty>
> if (atomic_read(pending)==0)
> >> rh_inc()
> >> atomic_inc(pending)
> >> if the region is clean
> >> mark the region dirty
> >> and remove from clean
> list
> else do nothing
> >> mark the region clean
> >> and move to clean list
[-- Attachment #2: dm-raid1-race2.new.patch --]
[-- Type: text/x-patch, Size: 976 bytes --]
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -375,6 +375,9 @@ static void rh_inc(struct region_hash *r
read_lock(&rh->hash_lock);
reg = __rh_find(rh, region);
+ spin_lock_irq(&rh->region_lock);
+ atomic_inc(®->pending);
+ spin_unlock_irq(&rh->region_lock);
if (reg->state == RH_CLEAN) {
rh->log->type->mark_region(rh->log, reg->key);
@@ -384,7 +387,6 @@ static void rh_inc(struct region_hash *r
spin_unlock_irq(&rh->region_lock);
}
- atomic_inc(®->pending);
read_unlock(&rh->hash_lock);
}
@@ -408,6 +410,10 @@ static void rh_dec(struct region_hash *r
if (atomic_dec_and_test(®->pending)) {
spin_lock_irqsave(&rh->region_lock, flags);
+ if (atomic_read(®->pending)) { /* check race */
+ spin_unlock_irqrestore(&rh->region_lock, flags);
+ return;
+ }
if (reg->state == RH_RECOVERING) {
list_add_tail(®->list, &rh->quiesced_regions);
} else {
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2005-06-27 17:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-06-16 22:51 [PATCH] 2.6.12-rc6: fix rh_dec()/rh_inc() race in dm-raid1.c Jun'ichi Nomura
2005-06-16 22:51 ` Jun'ichi Nomura
2005-06-24 15:19 ` Jonathan E Brassow
2005-06-24 15:45 ` Jun'ichi Nomura
2005-06-27 17:34 ` Jun'ichi Nomura
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.