[RFC PATCH 0/2] a couple of readonly handling fixups

[RFC PATCH 0/2] a couple of readonly handling fixups

[Dan Williams]

The BUG_ON(mddev->ro == 1) in md_write_start can be triggered under two
circumstances in recent kernels.  One was reported by Justin Maggard:

1) Create an md array with >= 1 disk
2) Start a task writing to the array ("dd if=/dev/zero of=/dev/md0
bs=1M count=10000 &" does the trick for me)
3) Force an improper reboot with reboot -fn

...the other was discovered while investigating this issue.

1) Set a raid5 readyonly with mdadm
2) Set the array writable with blockdev
3) Attempt to write to the array

---

Dan Williams (2):
      Revert "Restore force switch of md array to readonly at reboot time."
      md: set mddev readonly flag on blkdev BLKROSET ioctl

 drivers/md/md.c |   35 ++++++++++++++++++++++++++++++-----
 1 files changed, 30 insertions(+), 5 deletions(-)

[PATCH 1/2] md: set mddev readonly flag on blkdev BLKROSET ioctl

[Dan Williams]

When the user sets the block device to readwrite then the mddev should
follow suit.  Otherwise, the BUG_ON in md_write_start() will be set to
trigger.

The reverse direction, setting mddev->ro to match a set readonly
request, can be ignored because the blkdev level readonly flag precludes
the need to have mddev->ro set correctly.  Nevermind the fact that
setting mddev->ro to 1 may fail if the array is in use.

Cc: <stable@kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/md/md.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 1b1d326..7ac2b56 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4805,6 +4805,7 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
 	int err = 0;
 	void __user *argp = (void __user *)arg;
 	mddev_t *mddev = NULL;
+	int ro;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -4940,6 +4941,34 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
 			err = do_md_stop(mddev, 1, 1);
 			goto done_unlock;
 
+		case BLKROSET:
+			if (get_user(ro, (int __user *)(arg))) {
+				err = -EFAULT;
+				goto done_unlock;
+			}
+			err = -EINVAL;
+
+			/* if the bdev is going readonly the value of mddev->ro
+			 * does not matter, no writes are coming
+			 */
+			if (ro)
+				goto done_unlock;
+
+			/* are we are already prepared for writes? */
+			if (mddev->ro != 1)
+				goto done_unlock;
+
+			/* transitioning to readauto need only happen for
+			 * arrays that call md_write_start
+			 */
+			if (mddev->pers) {
+				err = restart_array(mddev);
+				if (err == 0) {
+					mddev->ro = 2;
+					set_disk_ro(mddev->gendisk, 0);
+				}
+			}
+			goto done_unlock;
 	}
 
 	/*

[PATCH 2/2] Revert "Restore force switch of md array to readonly at reboot time."

[Dan Williams]

This reverts commit 2b25000bf5157c28d8591f03f0575248a8cbd900.

Justin provided a test case that can reliably trigger the md_write_start
BUG_ON(mddev->ro == 1) at shutdown time.  Kernels prior to 2.6.27 do not fail
this test because the array remains readwrite.  The block layer does not
quiesce in-flight writes after setting the readonly flag so there will need to
be new infrastructure (stop writes at shutdown), or support for barriers
before MD arrays can reliably force readonly while writes may be in flight.

Reported-by: Justin Maggard <jmaggard10@gmail.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/md/md.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 7ac2b56..d3640b8 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6281,11 +6281,7 @@ static int md_notify_reboot(struct notifier_block *this,
 
 		for_each_mddev(mddev, tmp)
 			if (mddev_trylock(mddev)) {
-				/* Force a switch to readonly even array
-				 * appears to still be in use.  Hence
-				 * the '100'.
-				 */
-				do_md_stop(mddev, 1, 100);
+				do_md_stop(mddev, 1, 0);
 				mddev_unlock(mddev);
 			}
 		/*

Re: [RFC PATCH 0/2] a couple of readonly handling fixups

[Neil Brown]

On Thursday January 8, dan.j.williams@intel.com wrote:
> The BUG_ON(mddev->ro == 1) in md_write_start can be triggered under two
> circumstances in recent kernels.  One was reported by Justin Maggard:

Hi Dan,
 Thanks for following up with this.....

(I meant to send this over a week ago, but I've been on vaction
in Tasmania and my mobile has no coverage....)

> 
> 1) Create an md array with >= 1 disk
> 2) Start a task writing to the array ("dd if=/dev/zero of=/dev/md0
> bs=1M count=10000 &" does the trick for me)
> 3) Force an improper reboot with reboot -fn
> 
> ...the other was discovered while investigating this issue.
> 
> 1) Set a raid5 readyonly with mdadm
> 2) Set the array writable with blockdev
> 3) Attempt to write to the array
> 
> ---
> 
> Dan Williams (2):
>       Revert "Restore force switch of md array to readonly at reboot time."

I'm not comfortable with this.  That patch itself fixed a regression.
In some situations, the reboot notifier is the only thing that makes
sure the array is marked clean at shutdown time.  This revert
effectively disables that so sometimes a reboot will leave an array
dirty, which is not good.

There must be some other recent change that causes IO still to be in
flight this late.  Maybe I'll try to track it down.

>       md: set mddev readonly flag on blkdev BLKROSET ioctl

This one I'm happy with.  I'll make sure it gets through.

Thanks,
NeilBrown

Re: [RFC PATCH 0/2] a couple of readonly handling fixups

[Dan Williams]

On Sun, Jan 18, 2009 at 4:07 PM, Neil Brown <neilb@suse.de> wrote:
> On Thursday January 8, dan.j.williams@intel.com wrote:
>> The BUG_ON(mddev->ro == 1) in md_write_start can be triggered under two
>> circumstances in recent kernels.  One was reported by Justin Maggard:
>
> Hi Dan,
>  Thanks for following up with this.....
>
> (I meant to send this over a week ago, but I've been on vaction
> in Tasmania and my mobile has no coverage....)
>
[..]
>>       md: set mddev readonly flag on blkdev BLKROSET ioctl
>
> This one I'm happy with.  I'll make sure it gets through.

This slippery bugger seems to have evaded upstream, do you want me to resend?

Here is a link for reference:
http://marc.info/?l=linux-raid&m=123139948817348&w=2

I have seen another report of the md_write_start() BUG_ON triggering
so I'm wondering if there are other ways for mddev->ro and
bdev->bd_part->policy to get out of sync, or if they are just hitting
the bug that this patch fixes?

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/2] a couple of readonly handling fixups

[Neil Brown]

On Tue, 11 May 2010 11:06:48 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Sun, Jan 18, 2009 at 4:07 PM, Neil Brown <neilb@suse.de> wrote:
> > On Thursday January 8, dan.j.williams@intel.com wrote:
> >> The BUG_ON(mddev->ro == 1) in md_write_start can be triggered under two
> >> circumstances in recent kernels.  One was reported by Justin Maggard:
> >
> > Hi Dan,
> >  Thanks for following up with this.....
> >
> > (I meant to send this over a week ago, but I've been on vaction
> > in Tasmania and my mobile has no coverage....)
> >
> [..]
> >>       md: set mddev readonly flag on blkdev BLKROSET ioctl
> >
> > This one I'm happy with.  I'll make sure it gets through.
> 
> This slippery bugger seems to have evaded upstream, do you want me to resend?

No thanks, the link below is good enough.
That patch is now in my for-next branch and will go for-linus in
2.6.whatevercomesnext.

I normally stick that sort of thing somewhere in my stack before replying
that "I will make it so".  I guess I failed that time.


> 
> Here is a link for reference:
> http://marc.info/?l=linux-raid&m=123139948817348&w=2
> 
> I have seen another report of the md_write_start() BUG_ON triggering
> so I'm wondering if there are other ways for mddev->ro and
> bdev->bd_part->policy to get out of sync, or if they are just hitting
> the bug that this patch fixes?

Maybe we should just get rid of the BUG_ON?? 
Let's set how it goes once this patch is really in.

Thanks,
NeilBrown

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html