Thread (4 messages) 4 messages, 2 authors, 2013-11-07

Re: [PATCH] raid0: Set discard_granularity to correct value after reshape.

From: NeilBrown <hidden>
Date: 2013-11-07 05:03:55
Also in: lkml
Subsystem: software raid (multiple disks) support, the rest · Maintainers: Song Liu, Yu Kuai, Linus Torvalds

On Tue, 5 Nov 2013 14:25:19 +0000 "Baldysiak, Pawel"
[off-list ref] wrote:
On Thursday, October 31, 2013 1:16 AM NeilBrown [off-list ref] wrote:
quoted
On Wed, 30 Oct 2013 13:20:22 +0100 Pawel Baldysiak
[off-list ref] wrote:
quoted
In case of reshape of raid0 through raid4 a value of
discard_granularity will be set to stripe size. MD driver should
re-set this value to correct one when migration will be finished.
Otherwise array will be left with wrong value and discard operations will not
work properly.
quoted
Signed-off-by: Pawel Baldysiak <redacted>
Cc: Shaohua Li <shli@kernel.org>
---
 drivers/md/raid0.c |    2 ++
 1 file changed, 2 insertions(+)
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c index
c4d420b..807ca3a 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -266,6 +266,8 @@ static int create_strip_zones(struct mddev *mddev,
struct r0conf **private_conf)
quoted
 	}
 	mddev->queue->backing_dev_info.congested_fn = raid0_congested;
 	mddev->queue->backing_dev_info.congested_data = mddev;
+	mddev->queue->limits.discard_granularity =
+			queue_logical_block_size(mddev->queue);

 	/*
 	 * now since we have the hard sector sizes, we can make sure
Thanks, but this doesn't seem like the right sort of fix.  It is to specific to the
symptom rather than trying to address the underlying problem.

Maybe something like this?  Can you review and test?

Thanks,
NeilBrown

diff --git a/drivers/md/md.c b/drivers/md/md.c index
628cd529343f..740b6340f980 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3620,6 +3620,7 @@ level_store(struct mddev *mddev, const char
*buf, size_t len)
 		mddev->in_sync = 1;
 		del_timer_sync(&mddev->safemode_timer);
 	}
+	blk_set_stacking_limit(&mddev->queue->limits);
 	pers->run(mddev);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);
 	mddev_resume(mddev);
Hi Neil,
I have tested Yours patch, and everything works well.
TRIM operation are made correctly.
Could You apply it to upstream? 

BTW. Correct name of this function is blk_set_stacking_limits();

Thanks,
Paweł Baldysiak
Thanks for testing.  I've queue the following up for submission soonish.

NeilBrown

From 3d2b31bb23ccb8170fafdf358098bb3cb0836080 Mon Sep 17 00:00:00 2001
From: NeilBrown <redacted>
Date: Thu, 7 Nov 2013 13:36:36 +1100
Subject: [PATCH] md: fix calculation of stacking limits on level change.

The various ->run routines of md personalities assume that the 'queue'
has been initialised by the blk_set_stacking_limits() call in
md_alloc().

However when the level is changed (by level_store()) the ->run routine
for the new level is called for an array which has already had the
stacking limits modified.  This can result in incorrect final
settings.

So call blk_set_stacking_limits() before ->run in level_store().

A specific consequence of this bug is that it causes
discard_granularity to be set incorrectly when reshaping a RAID4 to a
RAID0.

This is suitable for any -stable kernel since 3.3 in which
blk_set_stacking_limits() was introduced.

Cc: stable@vger.kernel.org (3.3+)
Reported-by: "Baldysiak, Pawel" <redacted>
Signed-off-by: NeilBrown <redacted>
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 628cd529343f..b3a75afee560 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3620,6 +3620,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
 		mddev->in_sync = 1;
 		del_timer_sync(&mddev->safemode_timer);
 	}
+	blk_set_stacking_limits(&mddev->queue->limits);
 	pers->run(mddev);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);
 	mddev_resume(mddev);

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help