* Re: raid 5 crashed
From: Brad Campbell @ 2016-06-03 1:05 UTC (permalink / raw)
To: Wols Lists, Edward Kuns
Cc: Phil Turmel, bobzer, linux-raid, Mikael Abrahamsson
In-Reply-To: <57503C3F.6020005@youngman.org.uk>
On 02/06/16 22:01, Wols Lists wrote:
> On 02/06/16 00:15, Brad Campbell wrote:
>> People keep saying that. I've never encountered it. I suspect it's just
>> not the problem that the hysterical ranting makes it out to be (either
>> that or the pile of cheap and nasty drives I have here are model citizens).
>> I've *never* seen a read error unless the drive was in trouble, and that
>> includes running dd reads in a loop over multiple days continuously.
>> If it were that bad I'd see drives failing SMART long tests routinely
>> also, and that does not happen either.
>
> Note I didn't say you *will* see an error. BUT. If I recall correctly,
> the specs say that one read error per 10TB read is acceptable for a
> desktop drive that is designated healthy. In other words, if a 4TB drive
> throws an error every third pass, then according to the spec it's a
> perfectly healthy drive.
>
> Yes. We know that most drives are far better than spec, and if it
> degrades to spec then it's probably heading for failure, but the fact
> remains. If you have 3 x 4TB desktop drives in an array, then the spec
> says you should expect, and be able to deal with, an error EVERY time
> you scan the array.
No, it really doesn't. Those URE figures say <' 1 in' 10^14, not '= 1'
in 10^14. So that's a statistical worst case rather than a "this is what
you should expect". In addition, it's not a linear extrapolation, it's a
probability.
By that logic I should "expect" to roll a 6 at least once every 6 dice
rolls.
You can't extrapolate statistical figures like that. Just the same as
you can't calculate drive failures from MTBF figures.
Just perform regular read tests on all drives and periodic array scrubs
and you'll be much better off.
I've never had a reported URE on any of my arrays with SAS drvies, most
have reallocated sectors. They perform background reads periodically and
auto-reallocate anything that is looking dodgy.
SATA drives don't do that, but we can manage that externally with long
SMART tests and array scrubs to force rewrite/reallocation.
Just don't go trying to extrapolate from manufacturers probability data.
There are plenty of garbage web pages littered around the net where
"experts" do that, leading to 'hysterical ranting' about how the world
is ending and RAID5 is the devil. Sure RAID5 can be an issue when
dealing with a catastrophic drive failure requiring a rebuild if you
don't look after your drives, and I use and prefer RAID6 to mitigate
that, but it's not the end of the world.
Now, on an interesting, related and completely different note. To get
back to the concept of using dd or dd_rescue, I had a thought last night
and I've never seen it mentioned anywhere.
When you clone a dud drive using dd_rescue, it creates a bad block log.
The reason we don't like doing this is because when you put the
replacement drive back into the array, md does not see the errors and
will happily return zero data when it reads any sector that was bad on
the old drive.
hdparm has a neat feature called --make-bad-sector. It uses a feature of
the ATA protocol to write a sector that contains an invalid CRC, so the
drive returns an error when you try and read it. The sector is restored
by a normal re-write, so no reallocation or permanent damage takes place.
If we took the bad block list from the dd_rescue, and fed it to hdparm
to create bad sectors in all those locations on the cloned disk, md
would get a bad sector on read and attempt a recovery rather than
returning zero, This would "in theory" cause a re-write of good data
back to that disk and minimise the chance of data loss.
This might be a useful "last ditch" recovery method to allow you to
bring up an array with a cloned disk and minimise data loss. On the
other hand, lets say you are using it to bring up a RAID 5 with 2 failed
disks. One completely dead and one that you managed to clone most of.
When you extract the data from the running and degraded array, md will
pass the read error up the stack when it encounters the bad sectors,
allowing your copy or rsync session to log which files are affected as
you backup the remaining contents rather than just return silently
corrupted files.
^ permalink raw reply
* Re: [md PATCH 09/18] md/raid10: stop print_conf from being too verbose.
From: NeilBrown @ 2016-06-02 22:48 UTC (permalink / raw)
To: John Stoffel; +Cc: Shaohua Li, linux-raid
In-Reply-To: <22352.32563.81868.525891@quad.stoffel.home>
[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]
On Fri, Jun 03 2016, John Stoffel wrote:
> NeilBrown> raid10 arrays can usefully be very large. When they are,
> NeilBrown> the noise generated by print_conf can over-whelm logs. So
> NeilBrown> truncate the listing at 16 devices.
>
> Why is this too noisy and how often does this print_conf() happen?
> And why 16 devices? I guess I don't like the magic number of 16 here,
> I'd prefer it to be a define, and possibly even something that can by
> dynamically changed.
print_conf happens whenever a device becomes active in the array, or a
device is removed from the array (usually because it has failed).
I got 16 by choosing a random number and multiplying by 4 (or maybe by
raising 2 to that power) :-)
More seriously, I guessed that most arrays were 16 devices or less, so
this would not affect most arrays.
I definitely don't think it needs a define. I'm very tempted to remove
print_conf() completely, but it is sometimes useful. So having it
present as long as it isn't a burden seems reasonable.
If configuration was important I would change it to use pr_debug(), but
then the default would be to not see the messages at all, and they can
be useful in diagnosing problems reported on mailing lists.
>
> But how big a problem is this really? And what about for big RAID5/6
> arrays as well?
When you have 2000 devices in your RAID10 and half of them are removed
at once, it currently reports on 2,000,000 devices. With the patch it
is only 32000. Still possibly too many.
If you have 2000 devices in your RAID5/6 and half of them are removed,
you have other problems.
>
> Or would it be also good to condence the output of print_conf()
> itself?
Probably a very good idea. Maybe the default could print a fixed-size
summary and the rest goes in pr_debug()??
>
> Of if it's noise, why not just remove it completely? Can this
> information be found in /proc/mdstat instead?
Its value is historical - trying to understand a past sequence of
events. For that, something in the logs beats something in /proc.
>
> Sorry I havent' looked in the code deeply, but this just struck me as
> a change that might not be ideal.
Fair enough - your comments are very valid. I'm not really sure what is
best. I only know what is worst :-) and want to avoid that.
Suggestions very welcome.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [md PATCH 09/18] md/raid10: stop print_conf from being too verbose.
From: John Stoffel @ 2016-06-02 18:47 UTC (permalink / raw)
To: NeilBrown; +Cc: Shaohua Li, linux-raid
In-Reply-To: <20160602061952.2939.83586.stgit@noble>
NeilBrown> raid10 arrays can usefully be very large. When they are,
NeilBrown> the noise generated by print_conf can over-whelm logs. So
NeilBrown> truncate the listing at 16 devices.
Why is this too noisy and how often does this print_conf() happen?
And why 16 devices? I guess I don't like the magic number of 16 here,
I'd prefer it to be a define, and possibly even something that can by
dynamically changed.
But how big a problem is this really? And what about for big RAID5/6
arrays as well?
Or would it be also good to condence the output of print_conf()
itself?
Of if it's noise, why not just remove it completely? Can this
information be found in /proc/mdstat instead?
Sorry I havent' looked in the code deeply, but this just struck me as
a change that might not be ideal.
Thanks,
John
NeilBrown> Signed-off-by: NeilBrown <neilb@suse.com>
NeilBrown> ---
NeilBrown> drivers/md/raid10.c | 4 +++-
NeilBrown> 1 file changed, 3 insertions(+), 1 deletion(-)
NeilBrown> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
NeilBrown> index 61091ab02a4b..5d40612d6219 100644
NeilBrown> --- a/drivers/md/raid10.c
NeilBrown> +++ b/drivers/md/raid10.c
NeilBrown> @@ -1610,7 +1610,7 @@ static void print_conf(struct r10conf *conf)
NeilBrown> /* This is only called with ->reconfix_mutex held, so
NeilBrown> * rcu protection of rdev is not needed */
NeilBrown> - for (i = 0; i < conf->geo.raid_disks; i++) {
NeilBrown> + for (i = 0; i < conf->geo.raid_disks && i < 16; i++) {
NeilBrown> char b[BDEVNAME_SIZE];
NeilBrown> rdev = conf->mirrors[i].rdev;
NeilBrown> if (rdev)
NeilBrown> @@ -1619,6 +1619,8 @@ static void print_conf(struct r10conf *conf)
NeilBrown> !test_bit(Faulty, &rdev->flags),
NeilBrown> bdevname(rdev->bdev,b));
NeilBrown> }
NeilBrown> + if (conf->geo.raid_disks > 16)
NeilBrown> + printk(KERN_DEBUG " remaining devices excluded for brevity\n");
NeilBrown> }
NeilBrown> static void close_sync(struct r10conf *conf)
NeilBrown> --
NeilBrown> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
NeilBrown> the body of a message to majordomo@vger.kernel.org
NeilBrown> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: raid 5 crashed
From: Andreas Klauer @ 2016-06-02 15:27 UTC (permalink / raw)
To: Wols Lists
Cc: Brad Campbell, Edward Kuns, Phil Turmel, bobzer, linux-raid,
Mikael Abrahamsson
In-Reply-To: <57503C3F.6020005@youngman.org.uk>
On Thu, Jun 02, 2016 at 03:01:35PM +0100, Wols Lists wrote:
> If you have 3 x 4TB desktop drives in an array, then the spec
> says you should expect, and be able to deal with, an error EVERY time
> you scan the array.
It doesn't happen in practice, though. (Thank god.)
There was a paper about disk failures that said URE was simply not useful.
(Empirical Measurements of Disk Failure Rates).
There's this ZDnet article that declared RAID5 dead in 2009 but it still
works fine for me.
I just ignore the URE spec entirely.
(Until someone can prove that it actually matters.)
IMHO the main reason people notice disk failures during rebuilds, is that
they never ever tested their disks for read errors before. You should do
so regularly.
A long SMART self-test takes ages on a large disk, on a busy server with
today's disk sizes it can take days, which is why people avoid running
them (the other reason is lazyness).
However, SMART also supports selective self-tests; so you can run a
relatively short test every day and cover the entire disk over time.
You can schedule these partial tests at night when server load is lowest.
I think mdadm can also do a selective RAID check by fiddling with some
variables in /proc but there is no obvious way of doing so via
the userspace program.
Regards
Andreas Klauer
^ permalink raw reply
* Re: raid 5 crashed
From: Wols Lists @ 2016-06-02 14:01 UTC (permalink / raw)
To: Brad Campbell, Edward Kuns
Cc: Phil Turmel, bobzer, linux-raid, Mikael Abrahamsson
In-Reply-To: <2f3e2fe6-1810-e7bd-7dc0-483ed4f0d46b@fnarfbargle.com>
On 02/06/16 00:15, Brad Campbell wrote:
> People keep saying that. I've never encountered it. I suspect it's just
> not the problem that the hysterical ranting makes it out to be (either
> that or the pile of cheap and nasty drives I have here are model citizens).
> I've *never* seen a read error unless the drive was in trouble, and that
> includes running dd reads in a loop over multiple days continuously.
> If it were that bad I'd see drives failing SMART long tests routinely
> also, and that does not happen either.
Note I didn't say you *will* see an error. BUT. If I recall correctly,
the specs say that one read error per 10TB read is acceptable for a
desktop drive that is designated healthy. In other words, if a 4TB drive
throws an error every third pass, then according to the spec it's a
perfectly healthy drive.
Yes. We know that most drives are far better than spec, and if it
degrades to spec then it's probably heading for failure, but the fact
remains. If you have 3 x 4TB desktop drives in an array, then the spec
says you should expect, and be able to deal with, an error EVERY time
you scan the array. (Yes, I know I would probably be panicking if I got
even one error, too :-)
Cheers,
Wol
^ permalink raw reply
* Re: Why does raid0 set max_hw_sectors as chunk size but the other raid types doesn't?
From: Joey Liao @ 2016-06-02 9:30 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <87lh2op5mi.fsf@notabene.neil.brown.name>
Hi NeilBrown,
Thanks for your kindly reply.
Originally, I doubt that would it affect the performance or not, but I
agree with your point.
Thanks.
2016-06-02 12:38 GMT+08:00 NeilBrown <nfbrown@novell.com>:
> On Mon, May 30 2016, Joey Liao wrote:
>
>> Hi,
>>
>> I have no idea why does raid0_run() in raid0.c use
>> blk_queue_max_hw_sectors() to set max_hw_sectors as the chunk size,
>> but the other raid types doesn't?
>
> git is your friend.... admittedly you need the 'history' git tree to go
> back before 2.6.12, but it is available.
>
> http://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=f556ef000efc90a45a285f4f0b4fd70bb70f
>
>
>
>>
>> What's the purpose to limit the max_hw_sectors in raid0?
>
> unfortunately the commit doesn't answer that question. I think it was
> to ensure requests larger than one chunk were not created. If they were
> they would just have to be split, so there is no much to gain.
>
>>
>> Is it related to the source code logic issue or the performance issue?
>>
>> Besides, I have an interesting observation. If I remove all the
>> following queue limitation codes in raid0_run() of raid0.c, the block
>> size in iostat is still the same as chunk size even the input block
>> size is much larger than the chunk size. Why???
>
> Because when you write to a RAID0 you *must* divide each request up into
> chunk-sizes sub-requests, and send them to different devices.
>
> NeilBrown
>
>
>>
>> - blk_queue_max_hw_sectors(mddev->queue, mddev->chunk_sectors);
>> - blk_queue_max_write_same_sectors(mddev->queue, mddev->chunk_sectors);
>> + //blk_queue_max_hw_sectors(mddev->queue, mddev->chunk_sectors);
>> + //blk_queue_max_write_same_sectors(mddev->queue, mddev->chunk_sectors);
>>
>> - blk_queue_io_min(mddev->queue, mddev->chunk_sectors << 9);
>> - blk_queue_io_opt(mddev->queue,
>> - (mddev->chunk_sectors << 9) * mddev->raid_disks);
>> + //blk_queue_io_min(mddev->queue, mddev->chunk_sectors << 9);
>> + /*blk_queue_io_opt(mddev->queue,
>> + (mddev->chunk_sectors << 9) * mddev->raid_disks);*/
>> --
>> 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
^ permalink raw reply
* Re: [RFC v2 2/3] crypto: Introduce CRYPTO_ALG_BULK flag
From: Herbert Xu @ 2016-06-02 8:26 UTC (permalink / raw)
To: Baolin Wang
Cc: axboe, agk, snitzer, dm-devel, davem, ebiggers3, js1304,
tadeusz.struk, smueller, standby24x7, shli, dan.j.williams,
martin.petersen, sagig, kent.overstreet, keith.busch, tj,
ming.lei, broonie, arnd, linux-crypto, linux-block, linux-raid,
linux-kernel
In-Reply-To: <47e9ddd8c9ea9ad9e29c8cb027d19d8459ea1479.1464346333.git.baolin.wang@linaro.org>
On Fri, May 27, 2016 at 07:11:23PM +0800, Baolin Wang wrote:
> Now some cipher hardware engines prefer to handle bulk block rather than one
> sector (512 bytes) created by dm-crypt, cause these cipher engines can handle
> the intermediate values (IV) by themselves in one bulk block. This means we
> can increase the size of the request by merging request rather than always 512
> bytes and thus increase the hardware engine processing speed.
>
> So introduce 'CRYPTO_ALG_BULK' flag to indicate this cipher can support bulk
> mode.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
I think a better aproach would be to explicitly move the IV generation
into the crypto API, similar to how we handle IPsec. Once you do
that then every algorithm can be handled through the bulk interface.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Doubts on async_tx xor offload for raid5/6
From: Vikas Aggarwal @ 2016-06-02 6:24 UTC (permalink / raw)
To: linux-raid, dmaengine
Hi list,
Will appreciate if someone can clear my doubts with "some theoretical
examples". I am trying to improve XOR/GF offload for a multi-core SoC
(currently it has single XOR/GF channel).
1) What is the purpose of device_prep_dma_interrupt callback ?
2) My driver currently polls for posting xor completions
(dma_cookie_complete) and does'nt use device_prep_dma_interrupt
callback at all. What gains/loss in terms of latency/cpu
idleness/WRITE throughput I can expect by implementing this callback
in my async_tx driver.
3) Purpose of DMA_ACK as I read - it is for higher layers to inform
dma driver that descriptors can now be freed. Can someone explain this
with an example as applicable with raid5/6 clients.
4) With example - why dma_run_dependencies(tx) needed after the
hardware engine post completion for a descriptor.
5) Purpose of tx->callback(cb_arg) - again with an example from a
raid5/6 offload perspective.
I want to use offload engine efficiently with multithreaded raid5/6.
I tried to dig through dma/drivers, crypto/async_xor.c, online
archives, linux/Documentation etc but could not get satisfactory
answers. Added two mail-lists as saw issues being discussed on both
ones.
Thanks and
Best Regards
Vikas Aggarwal
^ permalink raw reply
* [md PATCH 18/18] md: reduce the number of synchronize_rcu() calls when multiple devices fail.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
Every time a device is removed with ->hot_remove_disk() a synchronize_rcu() call is made
which can delay several milliseconds in some case.
If lots of devices fail at once - as could happen with a large RAID10 where one set
of devices are removed all at once - these delays can add up to be very inconcenient.
As failure is not reversible we can check for that first, setting a
separate flag if it is found, and then all synchronize_rcu() once for
all the flagged devices. Then ->hot_remove_disk() function can skip the
synchronize_rcu() step if the flag is set.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/md.c | 27 +++++++++++++++++++++++++--
drivers/md/md.h | 5 +++++
drivers/md/multipath.c | 14 ++++++++------
drivers/md/raid1.c | 17 ++++++++++-------
drivers/md/raid10.c | 19 +++++++++++--------
drivers/md/raid5.c | 15 +++++++++------
6 files changed, 68 insertions(+), 29 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 2d26099e1160..1af69d15d0df 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8186,12 +8186,31 @@ static int remove_and_add_spares(struct mddev *mddev,
struct md_rdev *rdev;
int spares = 0;
int removed = 0;
+ bool remove_some = false;
- rdev_for_each(rdev, mddev)
+ rdev_for_each(rdev, mddev) {
if ((this == NULL || rdev == this) &&
rdev->raid_disk >= 0 &&
!test_bit(Blocked, &rdev->flags) &&
- (test_bit(Faulty, &rdev->flags) ||
+ test_bit(Faulty, &rdev->flags) &&
+ atomic_read(&rdev->nr_pending)==0) {
+ /* Faulty non-Blocked devices with nr_pending == 0
+ * never get nr_pending incremented,
+ * never get Faulty cleared, and never get Blocked set.
+ * So we can synchronize_rcu now rather than once per device
+ */
+ remove_some = true;
+ set_bit(RemoveSynchronized, &rdev->flags);
+ }
+ }
+
+ if (remove_some)
+ synchronize_rcu();
+ rdev_for_each(rdev, mddev) {
+ if ((this == NULL || rdev == this) &&
+ rdev->raid_disk >= 0 &&
+ !test_bit(Blocked, &rdev->flags) &&
+ ((test_bit(RemoveSynchronized, &rdev->flags) ||
(!test_bit(In_sync, &rdev->flags) &&
!test_bit(Journal, &rdev->flags))) &&
atomic_read(&rdev->nr_pending)==0) {
@@ -8202,6 +8221,10 @@ static int remove_and_add_spares(struct mddev *mddev,
removed++;
}
}
+ if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
+ clear_bit(RemoveSynchronized, &rdev->flags);
+ }
+
if (removed && mddev->kobj.sd)
sysfs_notify(&mddev->kobj, NULL, "degraded");
diff --git a/drivers/md/md.h b/drivers/md/md.h
index b5c4be73e6e4..cfd883e96a38 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -163,6 +163,11 @@ enum flag_bits {
* than other devices in the array
*/
ClusterRemove,
+ RemoveSynchronized, /* synchronize_rcu() was called after
+ * this device was known to be faulty,
+ * so it is safe to remove without
+ * another synchronize_rcu() call.
+ */
};
static inline int is_badblock(struct md_rdev *rdev, sector_t s, int sectors,
diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index 7eb9972a37e6..c145a5a114eb 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -298,12 +298,14 @@ static int multipath_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
goto abort;
}
p->rdev = NULL;
- synchronize_rcu();
- if (atomic_read(&rdev->nr_pending)) {
- /* lost the race, try later */
- err = -EBUSY;
- p->rdev = rdev;
- goto abort;
+ if (!test_bit(RemoveSynchronized, &rdev->flags)) {
+ synchronize_rcu();
+ if (atomic_read(&rdev->nr_pending)) {
+ /* lost the race, try later */
+ err = -EBUSY;
+ p->rdev = rdev;
+ goto abort;
+ }
}
err = md_integrity_register(mddev);
}
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 0ee901ccc9c5..8b27904c4f37 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1656,13 +1656,16 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
goto abort;
}
p->rdev = NULL;
- synchronize_rcu();
- if (atomic_read(&rdev->nr_pending)) {
- /* lost the race, try later */
- err = -EBUSY;
- p->rdev = rdev;
- goto abort;
- } else if (conf->mirrors[conf->raid_disks + number].rdev) {
+ if (!test_bit(RemoveSynchronized, &rdev->flags)) {
+ synchronize_rcu();
+ if (atomic_read(&rdev->nr_pending)) {
+ /* lost the race, try later */
+ err = -EBUSY;
+ p->rdev = rdev;
+ goto abort;
+ }
+ }
+ if (conf->mirrors[conf->raid_disks + number].rdev) {
/* We just removed a device that is being replaced.
* Move down the replacement. We drain all IO before
* doing this to avoid confusion.
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 78016667ec00..5e8fa7751920 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1768,7 +1768,7 @@ static int raid10_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
err = -EBUSY;
goto abort;
}
- /* Only remove faulty devices if recovery
+ /* Only remove non-faulty devices if recovery
* is not possible.
*/
if (!test_bit(Faulty, &rdev->flags) &&
@@ -1780,13 +1780,16 @@ static int raid10_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
goto abort;
}
*rdevp = NULL;
- synchronize_rcu();
- if (atomic_read(&rdev->nr_pending)) {
- /* lost the race, try later */
- err = -EBUSY;
- *rdevp = rdev;
- goto abort;
- } else if (p->replacement) {
+ if (!test_bit(RemoveSynchronized, &rdev->flags)) {
+ synchronize_rcu();
+ if (atomic_read(&rdev->nr_pending)) {
+ /* lost the race, try later */
+ err = -EBUSY;
+ *rdevp = rdev;
+ goto abort;
+ }
+ }
+ if (p->replacement) {
/* We must have just cleared 'rdev' */
p->rdev = p->replacement;
clear_bit(Replacement, &p->replacement->flags);
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 94c180f16294..4c6270e10b12 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7197,12 +7197,15 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
goto abort;
}
*rdevp = NULL;
- synchronize_rcu();
- if (atomic_read(&rdev->nr_pending)) {
- /* lost the race, try later */
- err = -EBUSY;
- *rdevp = rdev;
- } else if (p->replacement) {
+ if (!test_bit(RemoveSynchronized, &rdev->flags)) {
+ synchronize_rcu();
+ if (atomic_read(&rdev->nr_pending)) {
+ /* lost the race, try later */
+ err = -EBUSY;
+ *rdevp = rdev;
+ }
+ }
+ if (p->replacement) {
/* We must have just cleared 'rdev' */
p->rdev = p->replacement;
clear_bit(Replacement, &p->replacement->flags);
^ permalink raw reply related
* [md PATCH 17/18] md: be extra careful not to take a reference to a Faulty device.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
It is important that we never increment rdev->nr_pending on a Faulty
device as ->hot_remove_disk() assumes that once the Faulty flag is visible
no code will take a new reference.
Some places take a new reference after only check In_sync. This should
be safe as the two are changed together. However to make the code more
obviously safe, add checks for 'Faulty' as well.
Note: the actual rule is:
Never increment nr_pending if Faulty is set and Blocked is clear,
never clear Faulty, and never set Blocked without holding a reference
through nr_pending.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/multipath.c | 3 ++-
drivers/md/raid10.c | 6 ++++++
drivers/md/raid5.c | 3 ++-
3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index 69244de2036b..7eb9972a37e6 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -43,7 +43,8 @@ static int multipath_map (struct mpconf *conf)
rcu_read_lock();
for (i = 0; i < disks; i++) {
struct md_rdev *rdev = rcu_dereference(conf->multipaths[i].rdev);
- if (rdev && test_bit(In_sync, &rdev->flags)) {
+ if (rdev && test_bit(In_sync, &rdev->flags) &&
+ !test_bit(Faulty, &rdev->flags)) {
atomic_inc(&rdev->nr_pending);
rcu_read_unlock();
return i;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 5d40612d6219..78016667ec00 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2289,6 +2289,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
rdev = rcu_dereference(conf->mirrors[d].rdev);
if (rdev &&
test_bit(In_sync, &rdev->flags) &&
+ !test_bit(Faulty, &rdev->flags) &&
is_badblock(rdev, r10_bio->devs[sl].addr + sect, s,
&first_bad, &bad_sectors) == 0) {
atomic_inc(&rdev->nr_pending);
@@ -2341,6 +2342,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
d = r10_bio->devs[sl].devnum;
rdev = rcu_dereference(conf->mirrors[d].rdev);
if (!rdev ||
+ test_bit(Faulty, &rdev->flags) ||
!test_bit(In_sync, &rdev->flags))
continue;
@@ -2380,6 +2382,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
d = r10_bio->devs[sl].devnum;
rdev = rcu_dereference(conf->mirrors[d].rdev);
if (!rdev ||
+ test_bit(Faulty, &rdev->flags))
!test_bit(In_sync, &rdev->flags))
continue;
@@ -2948,6 +2951,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
mreplace = rcu_dereference(mirror->replacement);
if ((mrdev == NULL ||
+ test_bit(Faulty, &mrdev->flags) ||
test_bit(In_sync, &mrdev->flags))) {
rcu_read_unlock();
continue;
@@ -2964,6 +2968,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
rcu_read_unlock();
continue;
}
+ if (mreplace && test_bit(Faulty, &mreplace->flags))
+ mreplace = NULL;
/* Unless we are doing a full sync, or a replacement
* we only need to recover the block if it is set in
* the bitmap
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index e9beba258f4f..94c180f16294 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3080,7 +3080,8 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
struct md_rdev *rdev;
rcu_read_lock();
rdev = rcu_dereference(conf->disks[i].rdev);
- if (rdev && test_bit(In_sync, &rdev->flags))
+ if (rdev && test_bit(In_sync, &rdev->flags) &&
+ !test_bit(Faulty, &rdev->flags))
atomic_inc(&rdev->nr_pending);
else
rdev = NULL;
^ permalink raw reply related
* [md PATCH 16/18] md/multipath: add rcu protection to rdev access in multipath_status.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/multipath.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index dd483bb2e111..69244de2036b 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -141,17 +141,19 @@ static void multipath_make_request(struct mddev *mddev, struct bio * bio)
return;
}
-static void multipath_status (struct seq_file *seq, struct mddev *mddev)
+static void multipath_status(struct seq_file *seq, struct mddev *mddev)
{
struct mpconf *conf = mddev->private;
int i;
seq_printf (seq, " [%d/%d] [", conf->raid_disks,
conf->raid_disks - mddev->degraded);
- for (i = 0; i < conf->raid_disks; i++)
- seq_printf (seq, "%s",
- conf->multipaths[i].rdev &&
- test_bit(In_sync, &conf->multipaths[i].rdev->flags) ? "U" : "_");
+ rcu_read_lock();
+ for (i = 0; i < conf->raid_disks; i++) {
+ struct md_rdev *rdev = rcu_dereference(conf->multipaths[i].rdev);
+ seq_printf (seq, "%s", rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
+ }
+ rcu_read_unlock();
seq_printf (seq, "]");
}
^ permalink raw reply related
* [md PATCH 15/18] md/raid5: add rcu protection to rdev accesses in raid5_status.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid5.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7da89ee22a31..e9beba258f4f 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7069,10 +7069,12 @@ static void raid5_status(struct seq_file *seq, struct mddev *mddev)
seq_printf(seq, " level %d, %dk chunk, algorithm %d", mddev->level,
conf->chunk_sectors / 2, mddev->layout);
seq_printf (seq, " [%d/%d] [", conf->raid_disks, conf->raid_disks - mddev->degraded);
- for (i = 0; i < conf->raid_disks; i++)
- seq_printf (seq, "%s",
- conf->disks[i].rdev &&
- test_bit(In_sync, &conf->disks[i].rdev->flags) ? "U" : "_");
+ rcu_read_lock();
+ for (i = 0; i < conf->raid_disks; i++) {
+ struct md_rdev *rdev = rcu_dereference(conf->disks[i].rdev);
+ seq_printf (seq, "%s", rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
+ }
+ rcu_read_unlock();
seq_printf (seq, "]");
}
^ permalink raw reply related
* [md PATCH 14/18] md/raid5: add rcu protection to rdev accesses in want_replace
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
Being in the middle of resync is no longer protection against failed
rdevs disappearing. So add rcu protection.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid5.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index e9ad22d64bd7..7da89ee22a31 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3239,15 +3239,16 @@ static int want_replace(struct stripe_head *sh, int disk_idx)
{
struct md_rdev *rdev;
int rv = 0;
- /* Doing recovery so rcu locking not required */
- rdev = sh->raid_conf->disks[disk_idx].replacement;
+
+ rcu_read_lock();
+ rdev = rcu_dereference(sh->raid_conf->disks[disk_idx].replacement);
if (rdev
&& !test_bit(Faulty, &rdev->flags)
&& !test_bit(In_sync, &rdev->flags)
&& (rdev->recovery_offset <= sh->sector
|| rdev->mddev->recovery_cp <= sh->sector))
rv = 1;
-
+ rcu_read_unlock();
return rv;
}
^ permalink raw reply related
* [md PATCH 11/18] md/raid1: small code cleanup in end_sync_write
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
'mirror' is only used to find 'rdev', several times.
So just find 'rdev' once, and use it instead.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid1.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 9585a6b62123..cc53120e4d68 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1709,11 +1709,9 @@ static void end_sync_write(struct bio *bio)
struct r1bio *r1_bio = bio->bi_private;
struct mddev *mddev = r1_bio->mddev;
struct r1conf *conf = mddev->private;
- int mirror=0;
sector_t first_bad;
int bad_sectors;
-
- mirror = find_bio_disk(r1_bio, bio);
+ struct md_rdev *rdev = conf->mirrors[find_bio_disk(r1_bio, bio)].rdev;
if (!uptodate) {
sector_t sync_blocks = 0;
@@ -1726,16 +1724,12 @@ static void end_sync_write(struct bio *bio)
s += sync_blocks;
sectors_to_go -= sync_blocks;
} while (sectors_to_go > 0);
- set_bit(WriteErrorSeen,
- &conf->mirrors[mirror].rdev->flags);
- if (!test_and_set_bit(WantReplacement,
- &conf->mirrors[mirror].rdev->flags))
+ set_bit(WriteErrorSeen, &rdev->flags);
+ if (!test_and_set_bit(WantReplacement, &rdev->flags))
set_bit(MD_RECOVERY_NEEDED, &
mddev->recovery);
set_bit(R1BIO_WriteError, &r1_bio->state);
- } else if (is_badblock(conf->mirrors[mirror].rdev,
- r1_bio->sector,
- r1_bio->sectors,
+ } else if (is_badblock(rdev, r1_bio->sector, r1_bio->sectors,
&first_bad, &bad_sectors) &&
!is_badblock(conf->mirrors[r1_bio->read_disk].rdev,
r1_bio->sector,
^ permalink raw reply related
* [md PATCH 13/18] md/raid5: add rcu protection to rdev accesses in handle_failed_sync.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
The rdev could be freed while handle_failed_sync is running, so
rcu protection is needed.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid5.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 8959e6dd31dd..e9ad22d64bd7 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3210,15 +3210,16 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
/* During recovery devices cannot be removed, so
* locking and refcounting of rdevs is not needed
*/
+ rcu_read_lock();
for (i = 0; i < conf->raid_disks; i++) {
- struct md_rdev *rdev = conf->disks[i].rdev;
+ struct md_rdev *rdev = rcu_dereference(conf->disks[i].rdev);
if (rdev
&& !test_bit(Faulty, &rdev->flags)
&& !test_bit(In_sync, &rdev->flags)
&& !rdev_set_badblocks(rdev, sh->sector,
STRIPE_SECTORS, 0))
abort = 1;
- rdev = conf->disks[i].replacement;
+ rdev = rcu_dereference(conf->disks[i].replacement);
if (rdev
&& !test_bit(Faulty, &rdev->flags)
&& !test_bit(In_sync, &rdev->flags)
@@ -3226,6 +3227,7 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
STRIPE_SECTORS, 0))
abort = 1;
}
+ rcu_read_unlock();
if (abort)
conf->recovery_disabled =
conf->mddev->recovery_disabled;
^ permalink raw reply related
* [md PATCH 10/18] md/raid1: small cleanup in raid1_end_read/write_request
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
Both functions use conf->mirrors[mirror].rdev several times, so
improve readability by storing this in a local variable.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid1.c | 35 +++++++++++++++--------------------
1 file changed, 15 insertions(+), 20 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index a54edbe741ff..9585a6b62123 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -319,14 +319,13 @@ static void raid1_end_read_request(struct bio *bio)
{
int uptodate = !bio->bi_error;
struct r1bio *r1_bio = bio->bi_private;
- int mirror;
struct r1conf *conf = r1_bio->mddev->private;
+ struct md_rdev *rdev = conf->mirrors[r1_bio->read_disk].rdev;
- mirror = r1_bio->read_disk;
/*
* this branch is our 'one mirror IO has finished' event handler:
*/
- update_head_pos(mirror, r1_bio);
+ update_head_pos(r1_bio->read_disk, r1_bio);
if (uptodate)
set_bit(R1BIO_Uptodate, &r1_bio->state);
@@ -339,14 +338,14 @@ static void raid1_end_read_request(struct bio *bio)
spin_lock_irqsave(&conf->device_lock, flags);
if (r1_bio->mddev->degraded == conf->raid_disks ||
(r1_bio->mddev->degraded == conf->raid_disks-1 &&
- test_bit(In_sync, &conf->mirrors[mirror].rdev->flags)))
+ test_bit(In_sync, &rdev->flags)))
uptodate = 1;
spin_unlock_irqrestore(&conf->device_lock, flags);
}
if (uptodate) {
raid_end_bio_io(r1_bio);
- rdev_dec_pending(conf->mirrors[mirror].rdev, conf->mddev);
+ rdev_dec_pending(rdev, conf->mddev);
} else {
/*
* oops, read error:
@@ -356,7 +355,7 @@ static void raid1_end_read_request(struct bio *bio)
KERN_ERR "md/raid1:%s: %s: "
"rescheduling sector %llu\n",
mdname(conf->mddev),
- bdevname(conf->mirrors[mirror].rdev->bdev,
+ bdevname(rdev->bdev,
b),
(unsigned long long)r1_bio->sector);
set_bit(R1BIO_ReadError, &r1_bio->state);
@@ -403,20 +402,18 @@ static void r1_bio_write_done(struct r1bio *r1_bio)
static void raid1_end_write_request(struct bio *bio)
{
struct r1bio *r1_bio = bio->bi_private;
- int mirror, behind = test_bit(R1BIO_BehindIO, &r1_bio->state);
+ int behind = test_bit(R1BIO_BehindIO, &r1_bio->state);
struct r1conf *conf = r1_bio->mddev->private;
struct bio *to_put = NULL;
-
- mirror = find_bio_disk(r1_bio, bio);
+ int mirror = find_bio_disk(r1_bio, bio);
+ struct md_rdev *rdev = conf->mirrors[mirror].rdev;
/*
* 'one mirror IO has finished' event handler:
*/
if (bio->bi_error) {
- set_bit(WriteErrorSeen,
- &conf->mirrors[mirror].rdev->flags);
- if (!test_and_set_bit(WantReplacement,
- &conf->mirrors[mirror].rdev->flags))
+ set_bit(WriteErrorSeen, &rdev->flags);
+ if (!test_and_set_bit(WantReplacement, &rdev->flags))
set_bit(MD_RECOVERY_NEEDED, &
conf->mddev->recovery);
@@ -445,13 +442,12 @@ static void raid1_end_write_request(struct bio *bio)
* before rdev->recovery_offset, but for simplicity we don't
* check this here.
*/
- if (test_bit(In_sync, &conf->mirrors[mirror].rdev->flags) &&
- !test_bit(Faulty, &conf->mirrors[mirror].rdev->flags))
+ if (test_bit(In_sync, &rdev->flags) &&
+ !test_bit(Faulty, &rdev->flags))
set_bit(R1BIO_Uptodate, &r1_bio->state);
/* Maybe we can clear some bad blocks. */
- if (is_badblock(conf->mirrors[mirror].rdev,
- r1_bio->sector, r1_bio->sectors,
+ if (is_badblock(rdev, r1_bio->sector, r1_bio->sectors,
&first_bad, &bad_sectors)) {
r1_bio->bios[mirror] = IO_MADE_GOOD;
set_bit(R1BIO_MadeGood, &r1_bio->state);
@@ -459,7 +455,7 @@ static void raid1_end_write_request(struct bio *bio)
}
if (behind) {
- if (test_bit(WriteMostly, &conf->mirrors[mirror].rdev->flags))
+ if (test_bit(WriteMostly, &rdev->flags))
atomic_dec(&r1_bio->behind_remaining);
/*
@@ -483,8 +479,7 @@ static void raid1_end_write_request(struct bio *bio)
}
}
if (r1_bio->bios[mirror] == NULL)
- rdev_dec_pending(conf->mirrors[mirror].rdev,
- conf->mddev);
+ rdev_dec_pending(rdev, conf->mddev);
/*
* Let's see if all mirrored write operations have finished
^ permalink raw reply related
* [md PATCH 12/18] md/raid1: add rcu protection to rdev in fix_read_error
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
Since remove_and_add_spares() was added to hot_remove_disk() it has
been possible for an rdev to be hot-removed while fix_read_error()
was running, so we need to be more careful, and take a reference to
the rdev while performing IO.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid1.c | 52 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 20 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index cc53120e4d68..0ee901ccc9c5 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2056,29 +2056,30 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
s = PAGE_SIZE >> 9;
do {
- /* Note: no rcu protection needed here
- * as this is synchronous in the raid1d thread
- * which is the thread that might remove
- * a device. If raid1d ever becomes multi-threaded....
- */
sector_t first_bad;
int bad_sectors;
- rdev = conf->mirrors[d].rdev;
+ rcu_read_lock();
+ rdev = rcu_dereference(conf->mirrors[d].rdev);
if (rdev &&
(test_bit(In_sync, &rdev->flags) ||
(!test_bit(Faulty, &rdev->flags) &&
rdev->recovery_offset >= sect + s)) &&
is_badblock(rdev, sect, s,
- &first_bad, &bad_sectors) == 0 &&
- sync_page_io(rdev, sect, s<<9,
- conf->tmppage, READ, false))
- success = 1;
- else {
- d++;
- if (d == conf->raid_disks * 2)
- d = 0;
- }
+ &first_bad, &bad_sectors) == 0) {
+ atomic_inc(&rdev->nr_pending);
+ rcu_read_unlock();
+ if (sync_page_io(rdev, sect, s<<9,
+ conf->tmppage, READ, false))
+ success = 1;
+ rdev_dec_pending(rdev, mddev);
+ if (success)
+ break;
+ } else
+ rcu_read_unlock();
+ d++;
+ if (d == conf->raid_disks * 2)
+ d = 0;
} while (!success && d != read_disk);
if (!success) {
@@ -2094,11 +2095,17 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
if (d==0)
d = conf->raid_disks * 2;
d--;
- rdev = conf->mirrors[d].rdev;
+ rcu_read_lock();
+ rdev = rcu_dereference(conf->mirrors[d].rdev);
if (rdev &&
- !test_bit(Faulty, &rdev->flags))
+ !test_bit(Faulty, &rdev->flags)) {
+ atomic_inc(&rdev->nr_pending);
+ rcu_read_unlock();
r1_sync_page_io(rdev, sect, s,
conf->tmppage, WRITE);
+ rdev_dec_pending(rdev, mddev);
+ } else
+ rcu_read_unlock();
}
d = start;
while (d != read_disk) {
@@ -2106,9 +2113,12 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
if (d==0)
d = conf->raid_disks * 2;
d--;
- rdev = conf->mirrors[d].rdev;
+ rcu_read_lock();
+ rdev = rcu_dereference(conf->mirrors[d].rdev);
if (rdev &&
!test_bit(Faulty, &rdev->flags)) {
+ atomic_inc(&rdev->nr_pending);
+ rcu_read_unlock();
if (r1_sync_page_io(rdev, sect, s,
conf->tmppage, READ)) {
atomic_add(s, &rdev->corrected_errors);
@@ -2117,10 +2127,12 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
"(%d sectors at %llu on %s)\n",
mdname(mddev), s,
(unsigned long long)(sect +
- rdev->data_offset),
+ rdev->data_offset),
bdevname(rdev->bdev, b));
}
- }
+ rdev_dec_pending(rdev, mddev);
+ } else
+ rcu_read_unlock();
}
sectors -= s;
sect += s;
^ permalink raw reply related
* [md PATCH 09/18] md/raid10: stop print_conf from being too verbose.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
raid10 arrays can usefully be very large. When they are, the noise generated by print_conf
can over-whelm logs.
So truncate the listing at 16 devices.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid10.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 61091ab02a4b..5d40612d6219 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1610,7 +1610,7 @@ static void print_conf(struct r10conf *conf)
/* This is only called with ->reconfix_mutex held, so
* rcu protection of rdev is not needed */
- for (i = 0; i < conf->geo.raid_disks; i++) {
+ for (i = 0; i < conf->geo.raid_disks && i < 16; i++) {
char b[BDEVNAME_SIZE];
rdev = conf->mirrors[i].rdev;
if (rdev)
@@ -1619,6 +1619,8 @@ static void print_conf(struct r10conf *conf)
!test_bit(Faulty, &rdev->flags),
bdevname(rdev->bdev,b));
}
+ if (conf->geo.raid_disks > 16)
+ printk(KERN_DEBUG " remaining devices excluded for brevity\n");
}
static void close_sync(struct r10conf *conf)
^ permalink raw reply related
* [md PATCH 08/18] md/raid10: simplify print_conf a little.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
'tmp' is only ever used to extract 'tmp->rdev', so just use 'rdev' directly.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid10.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 819c6d5767a4..61091ab02a4b 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1598,7 +1598,7 @@ static void raid10_error(struct mddev *mddev, struct md_rdev *rdev)
static void print_conf(struct r10conf *conf)
{
int i;
- struct raid10_info *tmp;
+ struct md_rdev *rdev;
printk(KERN_DEBUG "RAID10 conf printout:\n");
if (!conf) {
@@ -1608,14 +1608,16 @@ static void print_conf(struct r10conf *conf)
printk(KERN_DEBUG " --- wd:%d rd:%d\n", conf->geo.raid_disks - conf->mddev->degraded,
conf->geo.raid_disks);
+ /* This is only called with ->reconfix_mutex held, so
+ * rcu protection of rdev is not needed */
for (i = 0; i < conf->geo.raid_disks; i++) {
char b[BDEVNAME_SIZE];
- tmp = conf->mirrors + i;
- if (tmp->rdev)
+ rdev = conf->mirrors[i].rdev;
+ if (rdev)
printk(KERN_DEBUG " disk %d, wo:%d, o:%d, dev:%s\n",
- i, !test_bit(In_sync, &tmp->rdev->flags),
- !test_bit(Faulty, &tmp->rdev->flags),
- bdevname(tmp->rdev->bdev,b));
+ i, !test_bit(In_sync, &rdev->flags),
+ !test_bit(Faulty, &rdev->flags),
+ bdevname(rdev->bdev,b));
}
}
^ permalink raw reply related
* [md PATCH 07/18] md/raid10: minor code improvement in fix_read_error()
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
rdev already holds conf->mirrors[d].rdev, so no need to load it again.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid10.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 588cf6544f07..819c6d5767a4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2262,7 +2262,7 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
printk(KERN_NOTICE
"md/raid10:%s: %s: Failing raid device\n",
mdname(mddev), b);
- md_error(mddev, conf->mirrors[d].rdev);
+ md_error(mddev, rdev);
r10_bio->devs[r10_bio->read_slot].bio = IO_BLOCKED;
return;
}
^ permalink raw reply related
* [md PATCH 05/18] md/raid10: add rcu protection to rdev access in raid10_sync_request.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
mirrors[].rdev can become NULL at any point unless:
- a counted reference is held
- ->reconfig_mutex is held, or
- rcu_read_lock() is held
Previously they could not become NULL during a resync/recovery/reshape either.
However when remove_and_add_spares() was added to hot_remove_disk(), that
changed.
So raid10_sync_request didn't previously need to protect rdev access,
but now it does.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid10.c | 120 +++++++++++++++++++++++++++++++--------------------
1 file changed, 72 insertions(+), 48 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index e6f3a11f8f70..f6f21a253926 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2871,11 +2871,14 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
/* Completed a full sync so the replacements
* are now fully recovered.
*/
- for (i = 0; i < conf->geo.raid_disks; i++)
- if (conf->mirrors[i].replacement)
- conf->mirrors[i].replacement
- ->recovery_offset
- = MaxSector;
+ rcu_read_lock();
+ for (i = 0; i < conf->geo.raid_disks; i++) {
+ struct md_rdev *rdev =
+ rcu_dereference(conf->mirrors[i].replacement);
+ if (rdev)
+ rdev->recovery_offset = MaxSector;
+ }
+ rcu_read_unlock();
}
conf->fullsync = 0;
}
@@ -2934,14 +2937,17 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
int must_sync;
int any_working;
struct raid10_info *mirror = &conf->mirrors[i];
+ struct md_rdev *mrdev, *mreplace;
- if ((mirror->rdev == NULL ||
- test_bit(In_sync, &mirror->rdev->flags))
- &&
- (mirror->replacement == NULL ||
- test_bit(Faulty,
- &mirror->replacement->flags)))
+ rcu_read_lock();
+ mrdev = rcu_dereference(mirror->rdev);
+ mreplace = rcu_dereference(mirror->replacement);
+
+ if ((mrdev == NULL ||
+ test_bit(In_sync, &mrdev->flags))) {
+ rcu_read_unlock();
continue;
+ }
still_degraded = 0;
/* want to reconstruct this device */
@@ -2951,6 +2957,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
/* last stripe is not complete - don't
* try to recover this sector.
*/
+ rcu_read_unlock();
continue;
}
/* Unless we are doing a full sync, or a replacement
@@ -2962,14 +2969,19 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (sync_blocks < max_sync)
max_sync = sync_blocks;
if (!must_sync &&
- mirror->replacement == NULL &&
+ mreplace == NULL &&
!conf->fullsync) {
/* yep, skip the sync_blocks here, but don't assume
* that there will never be anything to do here
*/
chunks_skipped = -1;
+ rcu_read_unlock();
continue;
}
+ atomic_inc(&mrdev->nr_pending);
+ if (mreplace)
+ atomic_inc(&mreplace->nr_pending);
+ rcu_read_unlock();
r10_bio = mempool_alloc(conf->r10buf_pool, GFP_NOIO);
r10_bio->state = 0;
@@ -2988,12 +3000,15 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
/* Need to check if the array will still be
* degraded
*/
- for (j = 0; j < conf->geo.raid_disks; j++)
- if (conf->mirrors[j].rdev == NULL ||
- test_bit(Faulty, &conf->mirrors[j].rdev->flags)) {
+ rcu_read_lock();
+ for (j = 0; j < conf->geo.raid_disks; j++) {
+ struct md_rdev *rdev = rcu_dereference(
+ conf->mirrors[j].rdev);
+ if (rdev == NULL || test_bit(Faulty, &rdev->flags)) {
still_degraded = 1;
break;
}
+ }
must_sync = bitmap_start_sync(mddev->bitmap, sect,
&sync_blocks, still_degraded);
@@ -3003,15 +3018,15 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
int k;
int d = r10_bio->devs[j].devnum;
sector_t from_addr, to_addr;
- struct md_rdev *rdev;
+ struct md_rdev *rdev =
+ rcu_dereference(conf->mirrors[d].rdev);
sector_t sector, first_bad;
int bad_sectors;
- if (!conf->mirrors[d].rdev ||
- !test_bit(In_sync, &conf->mirrors[d].rdev->flags))
+ if (!rdev ||
+ !test_bit(In_sync, &rdev->flags))
continue;
/* This is where we read from */
any_working = 1;
- rdev = conf->mirrors[d].rdev;
sector = r10_bio->devs[j].addr;
if (is_badblock(rdev, sector, max_sync,
@@ -3050,8 +3065,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
r10_bio->devs[1].devnum = i;
r10_bio->devs[1].addr = to_addr;
- rdev = mirror->rdev;
- if (!test_bit(In_sync, &rdev->flags)) {
+ if (!test_bit(In_sync, &mrdev->flags)) {
bio = r10_bio->devs[1].bio;
bio_reset(bio);
bio->bi_next = biolist;
@@ -3060,8 +3074,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
bio->bi_end_io = end_sync_write;
bio->bi_rw = WRITE;
bio->bi_iter.bi_sector = to_addr
- + rdev->data_offset;
- bio->bi_bdev = rdev->bdev;
+ + mrdev->data_offset;
+ bio->bi_bdev = mrdev->bdev;
atomic_inc(&r10_bio->remaining);
} else
r10_bio->devs[1].bio->bi_end_io = NULL;
@@ -3070,8 +3084,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
bio = r10_bio->devs[1].repl_bio;
if (bio)
bio->bi_end_io = NULL;
- rdev = mirror->replacement;
- /* Note: if rdev != NULL, then bio
+ /* Note: if mreplace != NULL, then bio
* cannot be NULL as r10buf_pool_alloc will
* have allocated it.
* So the second test here is pointless.
@@ -3079,8 +3092,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
* this comment keeps human reviewers
* happy.
*/
- if (rdev == NULL || bio == NULL ||
- test_bit(Faulty, &rdev->flags))
+ if (mreplace == NULL || bio == NULL ||
+ test_bit(Faulty, &mreplace->flags))
break;
bio_reset(bio);
bio->bi_next = biolist;
@@ -3089,11 +3102,12 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
bio->bi_end_io = end_sync_write;
bio->bi_rw = WRITE;
bio->bi_iter.bi_sector = to_addr +
- rdev->data_offset;
- bio->bi_bdev = rdev->bdev;
+ mreplace->data_offset;
+ bio->bi_bdev = mreplace->bdev;
atomic_inc(&r10_bio->remaining);
break;
}
+ rcu_read_unlock();
if (j == conf->copies) {
/* Cannot recover, so abort the recovery or
* record a bad block */
@@ -3106,15 +3120,15 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (r10_bio->devs[k].devnum == i)
break;
if (!test_bit(In_sync,
- &mirror->rdev->flags)
+ &mrdev->flags)
&& !rdev_set_badblocks(
- mirror->rdev,
+ mrdev,
r10_bio->devs[k].addr,
max_sync, 0))
any_working = 0;
- if (mirror->replacement &&
+ if (mreplace &&
!rdev_set_badblocks(
- mirror->replacement,
+ mreplace,
r10_bio->devs[k].addr,
max_sync, 0))
any_working = 0;
@@ -3132,8 +3146,14 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (rb2)
atomic_dec(&rb2->remaining);
r10_bio = rb2;
+ rdev_dec_pending(mrdev, mddev);
+ if (mreplace)
+ rdev_dec_pending(mreplace, mddev);
break;
}
+ rdev_dec_pending(mrdev, mddev);
+ if (mreplace)
+ rdev_dec_pending(mreplace, mddev);
}
if (biolist == NULL) {
while (r10_bio) {
@@ -3178,6 +3198,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
int d = r10_bio->devs[i].devnum;
sector_t first_bad, sector;
int bad_sectors;
+ struct md_rdev *rdev;
if (r10_bio->devs[i].repl_bio)
r10_bio->devs[i].repl_bio->bi_end_io = NULL;
@@ -3185,12 +3206,14 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
bio = r10_bio->devs[i].bio;
bio_reset(bio);
bio->bi_error = -EIO;
- if (conf->mirrors[d].rdev == NULL ||
- test_bit(Faulty, &conf->mirrors[d].rdev->flags))
+ rcu_read_lock();
+ rdev = rcu_dereference(conf->mirrors[d].rdev);
+ if (rdev == NULL || test_bit(Faulty, &rdev->flags)) {
+ rcu_read_unlock();
continue;
+ }
sector = r10_bio->devs[i].addr;
- if (is_badblock(conf->mirrors[d].rdev,
- sector, max_sync,
+ if (is_badblock(rdev, sector, max_sync,
&first_bad, &bad_sectors)) {
if (first_bad > sector)
max_sync = first_bad - sector;
@@ -3198,25 +3221,28 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
bad_sectors -= (sector - first_bad);
if (max_sync > bad_sectors)
max_sync = bad_sectors;
+ rcu_read_unlock();
continue;
}
}
- atomic_inc(&conf->mirrors[d].rdev->nr_pending);
+ atomic_inc(&rdev->nr_pending);
atomic_inc(&r10_bio->remaining);
bio->bi_next = biolist;
biolist = bio;
bio->bi_private = r10_bio;
bio->bi_end_io = end_sync_read;
bio->bi_rw = READ;
- bio->bi_iter.bi_sector = sector +
- conf->mirrors[d].rdev->data_offset;
- bio->bi_bdev = conf->mirrors[d].rdev->bdev;
+ bio->bi_iter.bi_sector = sector + rdev->data_offset;
+ bio->bi_bdev = rdev->bdev;
count++;
- if (conf->mirrors[d].replacement == NULL ||
- test_bit(Faulty,
- &conf->mirrors[d].replacement->flags))
+ rdev = rcu_dereference(conf->mirrors[d].replacement);
+ if (rdev == NULL || test_bit(Faulty, &rdev->flags)) {
+ rcu_read_unlock();
continue;
+ }
+ atomic_inc(&rdev->nr_pending);
+ rcu_read_unlock();
/* Need to set up for writing to the replacement */
bio = r10_bio->devs[i].repl_bio;
@@ -3224,15 +3250,13 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
bio->bi_error = -EIO;
sector = r10_bio->devs[i].addr;
- atomic_inc(&conf->mirrors[d].replacement->nr_pending);
bio->bi_next = biolist;
biolist = bio;
bio->bi_private = r10_bio;
bio->bi_end_io = end_sync_write;
bio->bi_rw = WRITE;
- bio->bi_iter.bi_sector = sector +
- conf->mirrors[d].replacement->data_offset;
- bio->bi_bdev = conf->mirrors[d].replacement->bdev;
+ bio->bi_iter.bi_sector = sector + rdev->data_offset;
+ bio->bi_bdev = rdev->bdev;
count++;
}
^ permalink raw reply related
* [md PATCH 06/18] md/raid10: add rcu protection to rdev access during reshape.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
mirrors[].rdev can become NULL at any point unless:
- a counted reference is held
- ->reconfig_mutex is held, or
- rcu_read_lock() is held
Reshape isn't always suitably careful as in the past rdev couldn't be
removed during reshape. It can now, so add protection.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid10.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index f6f21a253926..588cf6544f07 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4352,15 +4352,16 @@ read_more:
blist = read_bio;
read_bio->bi_next = NULL;
+ rcu_read_lock();
for (s = 0; s < conf->copies*2; s++) {
struct bio *b;
int d = r10_bio->devs[s/2].devnum;
struct md_rdev *rdev2;
if (s&1) {
- rdev2 = conf->mirrors[d].replacement;
+ rdev2 = rcu_dereference(conf->mirrors[d].replacement);
b = r10_bio->devs[s/2].repl_bio;
} else {
- rdev2 = conf->mirrors[d].rdev;
+ rdev2 = rcu_dereference(conf->mirrors[d].rdev);
b = r10_bio->devs[s/2].bio;
}
if (!rdev2 || test_bit(Faulty, &rdev2->flags))
@@ -4405,6 +4406,7 @@ read_more:
nr_sectors += len >> 9;
}
bio_full:
+ rcu_read_unlock();
r10_bio->sectors = nr_sectors;
/* Now submit the read */
@@ -4456,16 +4458,20 @@ static void reshape_request_write(struct mddev *mddev, struct r10bio *r10_bio)
struct bio *b;
int d = r10_bio->devs[s/2].devnum;
struct md_rdev *rdev;
+ rcu_read_lock();
if (s&1) {
- rdev = conf->mirrors[d].replacement;
+ rdev = rcu_dereference(conf->mirrors[d].replacement);
b = r10_bio->devs[s/2].repl_bio;
} else {
- rdev = conf->mirrors[d].rdev;
+ rdev = rcu_dereference(conf->mirrors[d].rdev);
b = r10_bio->devs[s/2].bio;
}
- if (!rdev || test_bit(Faulty, &rdev->flags))
+ if (!rdev || test_bit(Faulty, &rdev->flags)) {
+ rcu_read_unlock();
continue;
+ }
atomic_inc(&rdev->nr_pending);
+ rcu_read_unlock();
md_sync_acct(b->bi_bdev, r10_bio->sectors);
atomic_inc(&r10_bio->remaining);
b->bi_next = NULL;
@@ -4526,9 +4532,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
if (s > (PAGE_SIZE >> 9))
s = PAGE_SIZE >> 9;
+ rcu_read_lock();
while (!success) {
int d = r10b->devs[slot].devnum;
- struct md_rdev *rdev = conf->mirrors[d].rdev;
+ struct md_rdev *rdev = rcu_dereference(conf->mirrors[d].rdev);
sector_t addr;
if (rdev == NULL ||
test_bit(Faulty, &rdev->flags) ||
@@ -4536,11 +4543,15 @@ static int handle_reshape_read_error(struct mddev *mddev,
goto failed;
addr = r10b->devs[slot].addr + idx * PAGE_SIZE;
+ atomic_inc(&rdev->nr_pending);
+ rcu_read_unlock();
success = sync_page_io(rdev,
addr,
s << 9,
bvec[idx].bv_page,
READ, false);
+ rdev_dec_pending(rdev, mddev);
+ rcu_read_lock();
if (success)
break;
failed:
@@ -4550,6 +4561,7 @@ static int handle_reshape_read_error(struct mddev *mddev,
if (slot == first_slot)
break;
}
+ rcu_read_unlock();
if (!success) {
/* couldn't read this block, must give up */
set_bit(MD_RECOVERY_INTR,
@@ -4619,16 +4631,18 @@ static void raid10_finish_reshape(struct mddev *mddev)
}
} else {
int d;
+ rcu_read_lock();
for (d = conf->geo.raid_disks ;
d < conf->geo.raid_disks - mddev->delta_disks;
d++) {
- struct md_rdev *rdev = conf->mirrors[d].rdev;
+ struct md_rdev *rdev = rcu_dereference(conf->mirrors[d].rdev);
if (rdev)
clear_bit(In_sync, &rdev->flags);
- rdev = conf->mirrors[d].replacement;
+ rdev = rcu_dereference(conf->mirrors[d].replacement);
if (rdev)
clear_bit(In_sync, &rdev->flags);
}
+ rcu_read_unlock();
}
mddev->layout = mddev->new_layout;
mddev->chunk_sectors = 1 << conf->geo.chunk_shift;
^ permalink raw reply related
* [md PATCH 03/18] md/raid10: fix refounct imbalance when resyncing an array with a replacement device.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
If you have a raid10 with a replacement device that is resyncing -
e.g. after a crash before the replacement was complete - the write to
the replacement will increment nr_pending on the wrong device, which
will lead to strangeness.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid10.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index cc9e3813e1a4..f371d45f786a 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3222,7 +3222,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
bio->bi_error = -EIO;
sector = r10_bio->devs[i].addr;
- atomic_inc(&conf->mirrors[d].rdev->nr_pending);
+ atomic_inc(&conf->mirrors[d].replacement->nr_pending);
bio->bi_next = biolist;
biolist = bio;
bio->bi_private = r10_bio;
^ permalink raw reply related
* [md PATCH 04/18] md/raid10: add rcu protection in raid10_status.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
mirrors[].rdev can become NULL at any point unless:
- a counted reference is held
- ->reconfig_mutex is held, or
- rcu_read_lock() is held
raid10_status holds none of these. So add rcu_read_lock()
protection.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid10.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index f371d45f786a..e6f3a11f8f70 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1495,10 +1495,12 @@ static void raid10_status(struct seq_file *seq, struct mddev *mddev)
}
seq_printf(seq, " [%d/%d] [", conf->geo.raid_disks,
conf->geo.raid_disks - mddev->degraded);
- for (i = 0; i < conf->geo.raid_disks; i++)
- seq_printf(seq, "%s",
- conf->mirrors[i].rdev &&
- test_bit(In_sync, &conf->mirrors[i].rdev->flags) ? "U" : "_");
+ rcu_read_lock();
+ for (i = 0; i < conf->geo.raid_disks; i++) {
+ struct md_rdev *rdev = rcu_dereference(conf->mirrors[i].rdev);
+ seq_printf(seq, "%s", rdev && test_bit(In_sync, &rdev->flags) ? "U" : "_");
+ }
+ rcu_read_unlock();
seq_printf(seq, "]");
}
^ permalink raw reply related
* [md PATCH 02/18] md/raid1, raid10: don't recheck "Faulty" flag in read-balance.
From: NeilBrown @ 2016-06-02 6:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160602061319.2939.72280.stgit@noble>
Re-checking the faulty flag here brings no value.
The comment about "risk" refers to the risk that the device could
be in the process of being removed by ->hot_remove_disk().
However providing that the ->nr_pending count is incremented inside
an rcu_read_locked() region, there is no risk of that happening.
This is because the rdev pointer (in the personalities array) is set
to NULL before synchronize_rcu(), and ->nr_pending is tested
afterwards. If the rcu_read_locked region happens before the
synchronize_rcu(), the test will see that nr_pending has been incremented.
If it happens afterwards, the rdev pointer will be NULL so there is nothing
to increment.
Signed-off-by: NeilBrown <neilb@suse.com>
---
drivers/md/raid1.c | 7 -------
drivers/md/raid10.c | 8 --------
2 files changed, 15 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index c7c8cde0ab21..a54edbe741ff 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -689,13 +689,6 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
if (!rdev)
goto retry;
atomic_inc(&rdev->nr_pending);
- if (test_bit(Faulty, &rdev->flags)) {
- /* cannot risk returning a device that failed
- * before we inc'ed nr_pending
- */
- rdev_dec_pending(rdev, conf->mddev);
- goto retry;
- }
sectors = best_good_sectors;
if (conf->mirrors[best_disk].next_seq_sect != this_sector)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index c7de2a53e625..cc9e3813e1a4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -707,7 +707,6 @@ static struct md_rdev *read_balance(struct r10conf *conf,
raid10_find_phys(conf, r10_bio);
rcu_read_lock();
-retry:
sectors = r10_bio->sectors;
best_slot = -1;
best_rdev = NULL;
@@ -804,13 +803,6 @@ retry:
if (slot >= 0) {
atomic_inc(&rdev->nr_pending);
- if (test_bit(Faulty, &rdev->flags)) {
- /* Cannot risk returning a device that failed
- * before we inc'ed nr_pending
- */
- rdev_dec_pending(rdev, conf->mddev);
- goto retry;
- }
r10_bio->read_slot = slot;
} else
rdev = NULL;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox