Bug#696650: [PATCH v5] md: protect against crash upon fsync on ro array
Hi Neil,
please apply this patch! It is correct, now.
It applies to 3.2.y, 3.4.y, 3.7.y and latest 3.8-rc5. All these versions
are affected by this bug.
3.0.y and 2.6.34.y are also affected. Please also find the patches for
these versions attached. I've tested them. They work.
The strange thing is that 2.6.32.y is immune against this bug. So it
must be a regression. The patch restores the same behavior as present in
2.6.32: fsync receives success.
I've tested against the following versions: 3.8-rc5, 3.7.5, 3.4.28,
3.2.37, 3.0.61, 2.6.34.14 and 2.6.32.60.
Cheers,
Sebastian
On 29.01.2013 13:29, Paul Menzel wrote:
Any further objection?
Small typo (occurs) in commit message.
From adfac4df99edc1a83dced9c732464634d3381a9f Mon Sep 17 00:00:00 2001
From: Sebastian Riemer sebastian.rie...@profitbricks.com
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v5] md: protect against crash upon fsync on ro array
If an fsync occurs on a read-only array, we need to send a
completion for the IO and may not increment the active IO count.
Otherwise, we hit a bug trace and can't stop the MD array anymore.
By advice of Christoph Hellwig we return success upon a flush
request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.
Cc: Christoph Hellwig h...@infradead.org
Cc: Ben Hutchings b...@decadent.org.uk
Cc: NeilBrown ne...@suse.de
Signed-off-by: Sebastian Riemer sebastian.rie...@profitbricks.com
Reported-by: Ben Hutchings b...@decadent.org.uk
Acked-by: Paul Menzel paulepan...@users.sourceforge.net
---
drivers/md/md.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..1e634a6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -307,6 +307,10 @@ static void md_make_request(struct request_queue *q,
struct bio *bio)
bio_io_error(bio);
return;
}
+ if (mddev-ro == 1 unlikely(rw == WRITE)) {
+ bio_endio(bio, bio_sectors(bio) == 0 ? 0 : -EROFS);
+ return;
+ }
smp_rmb(); /* Ensure implications of 'active' are visible */
rcu_read_lock();
if (mddev-suspended) {
--
1.7.1
From 58ecc54ef2ea1692b2a608183901708d3cad5120 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer sebastian.rie...@profitbricks.com
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH 3.0.y] md: protect against crash upon fsync on ro array
Reply-To: linux-raid linux-r...@vger.kernel.org
If an fsync occurs on a read-only array, we need to send a
completion for the IO and may not increment the active IO count.
Otherwise, we hit a bug trace and can't stop the MD array anymore.
By advice of Christoph Hellwig we return success upon a flush
request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.
Cc: Christoph Hellwig h...@infradead.org
Cc: Ben Hutchings b...@decadent.org.uk
Cc: NeilBrown ne...@suse.de
Signed-off-by: Sebastian Riemer sebastian.rie...@profitbricks.com
Reported-by: Ben Hutchings b...@decadent.org.uk
Acked-by: Paul Menzel paulepan...@users.sourceforge.net
---
drivers/md/md.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 98262e5..4ef75e9 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -299,6 +299,10 @@ static int md_make_request(struct request_queue *q, struct
bio *bio)
bio_io_error(bio);
return 0;
}
+ if (mddev-ro == 1 unlikely(rw == WRITE)) {
+ bio_endio(bio, bio_sectors(bio) == 0 ? 0 : -EROFS);
+ return 0;
+ }
smp_rmb(); /* Ensure implications of 'active' are visible */
rcu_read_lock();
if (mddev-suspended) {
--
1.7.1
From 58ecc54ef2ea1692b2a608183901708d3cad5120 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer sebastian.rie...@profitbricks.com
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH 2.6.34.y] md: protect against crash upon fsync on ro array
Reply-To: linux-raid linux-r...@vger.kernel.org
If an fsync occurs on a read-only array, we need to send a
completion for the IO and may not increment the active IO count.
Otherwise, we hit a bug trace and can't stop the MD array anymore.
By advice of Christoph Hellwig we return success upon a flush
request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.
Cc: Christoph Hellwig h...@infradead.org
Cc: Ben Hutchings b...@decadent.org.uk
Cc: NeilBrown ne...@suse.de
Signed-off-by: Sebastian Riemer sebastian.rie...@profitbricks.com
Reported-by: Ben Hutchings b...@decadent.org.uk
Acked-by: Paul Menzel paulepan...@users.sourceforge.net
---
drivers/md/md.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d8e5adc..ba1c0be 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -221,6 +221,10 @@ static int
Bug#696650: [PATCH v4] md: protect against crash upon fsync on ro array
On 29.01.2013 06:45, Ben Hutchings wrote:
I'm slightly uneasy about returning 0 for all writes to a read-only
deivce, because if someone ever fails to check the read-only flag
elsewhere this could turn into silent data loss. Does this work:
BUG_ON(bio_segments(bio));
Or should it be:
bio_endio(bio, bio_segments(bio) == 0 ? 0 : -EROFS);
to make the error survivable?
Good point. But it's better to use bio_sectors as bi_size is the
important information. I've seen that DRBD detects empty flushes the
same way.
For testing I've disabled the set_disk_ro in md_set_readonly. Then,
I did direct IO writes on the read-only array and it worked - I've
received Input/output error in the user space.
I've attached version 4 of the patch.
Any further objection?
From adfac4df99edc1a83dced9c732464634d3381a9f Mon Sep 17 00:00:00 2001
From: Sebastian Riemer sebastian.rie...@profitbricks.com
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v4] md: protect against crash upon fsync on ro array
If an fsync occurrs on a read-only array, we need to send a
completion for the IO and may not increment the active IO count.
Otherwise, we hit a bug trace and can't stop the MD array anymore.
By advice of Christoph Hellwig we return success upon a flush
request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.
Cc: Christoph Hellwig h...@infradead.org
Cc: Ben Hutchings b...@decadent.org.uk
Cc: NeilBrown ne...@suse.de
Signed-off-by: Sebastian Riemer sebastian.rie...@profitbricks.com
Reported-by: Ben Hutchings b...@decadent.org.uk
---
drivers/md/md.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..1e634a6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -307,6 +307,10 @@ static void md_make_request(struct request_queue *q,
struct bio *bio)
bio_io_error(bio);
return;
}
+ if (mddev-ro == 1 unlikely(rw == WRITE)) {
+ bio_endio(bio, bio_sectors(bio) == 0 ? 0 : -EROFS);
+ return;
+ }
smp_rmb(); /* Ensure implications of 'active' are visible */
rcu_read_lock();
if (mddev-suspended) {
--
1.7.1
Bug#696650: [PATCH v2] md: protect against crash upon fsync on ro array
Subject was: Re: fsync() on read-only RAID triggers BUG
On 27.01.2013 17:39, Christoph Hellwig wrote:
On Sat, Jan 26, 2013 at 07:44:40PM +, Ben Hutchings wrote:
I applied this on top of 3.2.37 and it certainly fixes the crash.
However I wonder whether fsync() should fail or should immediately
succeed. I don't know whether the installer expects it to succeed.
It should succeed.
O.K., then I hope Neil applies the attached patch. I've changed the
return value to success.
This is also something for linux-stable and should apply to many kernel
versions without an issue.
From fe0357344877c9b9cc623fd582a4e0670e448317 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer sebastian.rie...@profitbricks.com
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v2] md: protect against crash upon fsync on ro array
If an fsync occurrs on a read-only array, we need to send a
completion for the IO and may not increment the active IO count.
Otherwise, we hit a bug trace and can't stop the MD array anymore.
By advice of Christoph Hellwig we silently return success.
Cc: Christoph Hellwig h...@infradead.org
Cc: Ben Hutchings b...@decadent.org.uk
Cc: NeilBrown ne...@suse.de
Signed-off-by: Sebastian Riemer sebastian.rie...@profitbricks.com
Reported-by: Ben Hutchings b...@decadent.org.uk
---
drivers/md/md.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..475e0be 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -322,6 +322,11 @@ static void md_make_request(struct request_queue *q,
struct bio *bio)
}
finish_wait(mddev-sb_wait, __wait);
}
+ if (mddev-ro == 1 unlikely(rw == WRITE)) {
+ rcu_read_unlock();
+ bio_endio(bio, 0);
+ return;
+ }
atomic_inc(mddev-active_io);
rcu_read_unlock();
--
1.7.1
Bug#696650: [PATCH v3] md: protect against crash upon fsync on ro array
On 28.01.2013 11:32, Sebastian Riemer wrote:
O.K., then I hope Neil applies the attached patch. I've changed the
return value to success.
This is also something for linux-stable and should apply to many kernel
versions without an issue.
I've tried to race with continuous fsyncs against continuous mdadm -o
/dev/md0; mdadm -w /dev/md0; in parallel but couldn't break it.
Therefore, no additional locking is required and this part can be put
directly before the RCU locking stuff and the suspended handling.
I've attached version 3 of the patch.
From fe0357344877c9b9cc623fd582a4e0670e448317 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer sebastian.rie...@profitbricks.com
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v3] md: protect against crash upon fsync on ro array
If an fsync occurrs on a read-only array, we need to send a
completion for the IO and may not increment the active IO count.
Otherwise, we hit a bug trace and can't stop the MD array anymore.
By advice of Christoph Hellwig we silently return success.
Cc: Christoph Hellwig h...@infradead.org
Cc: Ben Hutchings b...@decadent.org.uk
Cc: NeilBrown ne...@suse.de
Signed-off-by: Sebastian Riemer sebastian.rie...@profitbricks.com
Reported-by: Ben Hutchings b...@decadent.org.uk
---
drivers/md/md.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..6ba20f7 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -307,6 +307,10 @@ static void md_make_request(struct request_queue *q,
struct bio *bio)
bio_io_error(bio);
return;
}
+ if (mddev-ro == 1 unlikely(rw == WRITE)) {
+ bio_endio(bio, 0);
+ return;
+ }
smp_rmb(); /* Ensure implications of 'active' are visible */
rcu_read_lock();
if (mddev-suspended) {
