* Re: Paranoid mode for RAID-1 ?
From: Adam Goryachev @ 2015-04-27 8:18 UTC (permalink / raw)
To: Mikael Abrahamsson, David Brown; +Cc: linux-raid
In-Reply-To: <alpine.DEB.2.02.1504270925480.16871@uplift.swm.pp.se>
On 27/04/15 17:35, Mikael Abrahamsson wrote:
> On Mon, 27 Apr 2015, David Brown wrote:
>
>> btrfs has data checksums like that. Like Neil, I question the
>> necessity for harddisks, but such checksums are lower cost than
>> reading the data twice from two disks (as they are stored as part of
>> the metadata that you already read), and can offer some protection
>> against serious hardware problems. (Checksums like this cannot
>> easily be implemented in a transparent block device such as md raid -
>> it is more practical to have them as part of the filesystem, as done
>> with btrfs.)
>
> Only way I can imagine this being done would be for instance to add a
> 4KiB block for every 128KiB chunk or something like that, and perhaps
> have a smaller checksum for each 4KiB block within that 128KiB chunk.
>
> I doubt anyone would be interested in putting efforts into creating
> this though as it would have "interesting" performance drawbacks, and
> that work is probably better spent by making sure that btrfs and/or
> zfs gets more development/testing than it is to put that effort into
> md. I personally prefer md to be fairly "simple" so we have as few
> bugs as possible in it, I'd say that md generally works and the number
> of developers working heroically on its current incarnation is barely
> enough to make sure that the codebase works as well as it must
> considering the critical function it serves for a lot of us.
>
> This has been discussed before and nobody has shown interest in
> actually developing code for it, so we're still at the feature request
> and "brainstorming about design" state, and without actual coder(s)
> willing to actually implement, it's not going to get further than this
> stage.
>
Speaking of which, I'm not convinced that we should spend that developer
time on each and every FS (eg, duplicated effort for btrfs, zfs, and any
others that do the same). It also means you must remove MD Raid, to
allow the FS to directly access each of the underlying devices.
Obviously, there are advantages in both methods.
As you and others said, without someone willing to implement/write this
feature, then it isn't going to happen.
Regards,
Adam
--
Adam Goryachev Website Managers www.websitemanagers.com.au
^ permalink raw reply
* Re: Paranoid mode for RAID-1 ?
From: Mikael Abrahamsson @ 2015-04-27 7:35 UTC (permalink / raw)
To: David Brown; +Cc: Adam Goryachev, linux-raid
In-Reply-To: <553DE20E.2020605@hesbynett.no>
On Mon, 27 Apr 2015, David Brown wrote:
> btrfs has data checksums like that. Like Neil, I question the necessity
> for harddisks, but such checksums are lower cost than reading the data
> twice from two disks (as they are stored as part of the metadata that
> you already read), and can offer some protection against serious
> hardware problems. (Checksums like this cannot easily be implemented in
> a transparent block device such as md raid - it is more practical to
> have them as part of the filesystem, as done with btrfs.)
Only way I can imagine this being done would be for instance to add a 4KiB
block for every 128KiB chunk or something like that, and perhaps have a
smaller checksum for each 4KiB block within that 128KiB chunk.
I doubt anyone would be interested in putting efforts into creating this
though as it would have "interesting" performance drawbacks, and that work
is probably better spent by making sure that btrfs and/or zfs gets more
development/testing than it is to put that effort into md. I personally
prefer md to be fairly "simple" so we have as few bugs as possible in it,
I'd say that md generally works and the number of developers working
heroically on its current incarnation is barely enough to make sure that
the codebase works as well as it must considering the critical function it
serves for a lot of us.
This has been discussed before and nobody has shown interest in actually
developing code for it, so we're still at the feature request and
"brainstorming about design" state, and without actual coder(s) willing to
actually implement, it's not going to get further than this stage.
--
Mikael Abrahamsson email: swmike@swm.pp.se
^ permalink raw reply
* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
From: Ming Lin @ 2015-04-27 7:29 UTC (permalink / raw)
To: NeilBrown
Cc: Christoph Hellwig, Jens Axboe, linux-raid, Kent Overstreet,
Dongsu Park
In-Reply-To: <20150427094911.0f71b986@notabene.brown>
On Sun, Apr 26, 2015 at 4:49 PM, NeilBrown <neilb@suse.de> wrote:
> On Fri, 24 Apr 2015 15:52:10 -0700 Ming Lin <mlin@kernel.org> wrote:
>
>> From: Kent Overstreet <kent.overstreet@gmail.com>
>>
>> Refactor sync_request_write() of md/raid10 to use bio_copy_data()
>> instead of open coding bio_vec iterations.
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
>> [dpark: add more description in commit message]
>> Signed-off-by: Dongsu Park <dongsu.park@profitbricks.com>
>> Signed-off-by: Ming Lin <mlin@kernel.org>
>> ---
>> drivers/md/raid10.c | 20 +++++---------------
>> 1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index a7196c4..02e33f1 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -2097,18 +2097,11 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
>> tbio->bi_vcnt = vcnt;
>> tbio->bi_iter.bi_size = r10_bio->sectors << 9;
>> tbio->bi_rw = WRITE;
>> - tbio->bi_private = r10_bio;
>> tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
>> -
>> - for (j=0; j < vcnt ; j++) {
>> - tbio->bi_io_vec[j].bv_offset = 0;
>> - tbio->bi_io_vec[j].bv_len = PAGE_SIZE;
>> -
>> - memcpy(page_address(tbio->bi_io_vec[j].bv_page),
>> - page_address(fbio->bi_io_vec[j].bv_page),
>> - PAGE_SIZE);
>> - }
>
> You removed the resetting of bv_offset and bv_len.
> So I assume this is being applied in a context where these things are now
> immutable - is that correct?
Correct. The full patchset is here:
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-req
I'll rebase it on top of 4.1-rc1, then post it.
>
>> tbio->bi_end_io = end_sync_write;
>> + tbio->bi_private = r10_bio;
>
> Any reason you are moving this assignment to bi_private?
> It doesn't hurt, but it doesn't seem to be necessary.
I'll update it to where it was.
>
>
>> +
>> + bio_copy_data(tbio, fbio);
>>
>> d = r10_bio->devs[i].devnum;
>> atomic_inc(&conf->mirrors[d].rdev->nr_pending);
>> @@ -2124,17 +2117,14 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
>> * that are active
>> */
>> for (i = 0; i < conf->copies; i++) {
>> - int j, d;
>> + int d;
>>
>> tbio = r10_bio->devs[i].repl_bio;
>> if (!tbio || !tbio->bi_end_io)
>> continue;
>> if (r10_bio->devs[i].bio->bi_end_io != end_sync_write
>> && r10_bio->devs[i].bio != fbio)
>> - for (j = 0; j < vcnt; j++)
>> - memcpy(page_address(tbio->bi_io_vec[j].bv_page),
>> - page_address(fbio->bi_io_vec[j].bv_page),
>> - PAGE_SIZE);
>> + bio_copy_data(tbio, fbio);
>> d = r10_bio->devs[i].devnum;
>> atomic_inc(&r10_bio->remaining);
>> md_sync_acct(conf->mirrors[d].replacement->bdev,
>
> Providing you are confident that bv_offset and bv_len don't need to be
> updated:
>
> Acked-by: NeilBrown <neilb@suse.de>
>
> though I'd prefer the bi_private assignment was left where it was.
Will update it.
Thanks.
>
> Thanks,
> NeilBrown
>
^ permalink raw reply
* Re: Paranoid mode for RAID-1 ?
From: David Brown @ 2015-04-27 7:15 UTC (permalink / raw)
To: Adam Goryachev, linux-raid
In-Reply-To: <553DDBB3.2010306@websitemanagers.com.au>
On 27/04/15 08:48, Adam Goryachev wrote:
> On 27/04/15 16:37, Jean-Baptiste Thomas wrote:
>> I'm looking for a way to get MD to operate in a mode in which
>> reading a sector from a RAID-1 device would not succeed until it
>> got matching data from at least two components.
>>
>> Recent experience[1] suggests that a transient problem in one
>> disk can completely hose a four way RAID-1 array, which is
>> otherwise supposed to still be fine after a triple total
>> failure. I'm hoping that a paranoid mode would have prevented
>> that.
>
> There isn't any such thing that I am aware of in Linux MD RAID. However,
> I've heard that if you want data integrity, then you could use zfs,
> which supports RAID as well as data checksums to ensure that the data
> read back matches the data you wrote....
>
> Personally, I've never used zfs, and there might be other FS's that will
> have the feature as well (eg, btrfs etc).
>
btrfs has data checksums like that. Like Neil, I question the necessity
for harddisks, but such checksums are lower cost than reading the data
twice from two disks (as they are stored as part of the metadata that
you already read), and can offer some protection against serious
hardware problems. (Checksums like this cannot easily be implemented in
a transparent block device such as md raid - it is more practical to
have them as part of the filesystem, as done with btrfs.)
> Hope that helps...
>
> Regards,
> Adam
^ permalink raw reply
* Re: BUG?: RAID6 reshape hung in reshape_request
From: NeilBrown @ 2015-04-27 6:59 UTC (permalink / raw)
To: David Wahler; +Cc: linux-raid
In-Reply-To: <CAGivzjHvk8pSUyWOMzNioNtgpQ7gRoL=hkTX0=m7Mi8KzgRWAg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]
On Sun, 26 Apr 2015 20:56:58 -0500 David Wahler <dwahler@gmail.com> wrote:
> [oops, forgot to cc the list]
>
> On Sun, Apr 26, 2015 at 8:20 PM, NeilBrown <neilb@suse.de> wrote:
> >> And the output of mdadm --detail/-E:
> >> https://gist.github.com/anonymous/0b090668b56ef54bb2f0
> >
> > What is wrong with simply including this directly in the email???
>
> My bad; I wasn't sure whether it was appropriate to paste such a long
> dump inline.
>
> > Anyway:
> >
> > Bad Block Log : 512 entries available at offset 72 sectors - bad blocks present.
> >
> > that is the only thing that looks at all interesting. Particularly the last
> > 3 words.
> > What does
> > mdadm --examine-badblocks /dev/sd[cde]1
> > show?
>
> root@ceres:~# mdadm --examine-badblocks /dev/sd[cde]1
> Bad-blocks on /dev/sdc1:
> 3699640928 for 32 sectors
> Bad-blocks on /dev/sdd1:
> 3699640928 for 32 sectors
> Bad-blocks on /dev/sde1:
> 3699640928 for 32 sectors
>
> Hmm, that seems kind of odd to me. For what it's worth, all four
> drives passed a SMART self-test, and "dd > /dev/null" completed
> without errors on all of them. I just read about the "badblocks" tool
> and I'm running it now.
The array is reshaping a RAID6 from 4->5 devices, so that is 2 data disks to
3 data disks.
Reshape pos'n : 5548735488 (5291.69 GiB 5681.91 GB)
so it is about 5.3TB through the array, so it has read about 2.6TB from the
devices and written about 1.7TB to the devices.
3699640928 sectors is about 1.8TB. That seems a little too close to be a co-incidence.
Maybe when reshape write to somewhere that is a bad-block, it gets confused.
On the other hand, when it copies from the 1.8TB address to the 1.2TB address
it should have record that it was bad-blocks that were being copied. It
doesn't seem like it did.
I'll have to look at the code and try to figure out what is happening.
I don't think there is anything useful you can do in the mean time...
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: Paranoid mode for RAID-1 ?
From: NeilBrown @ 2015-04-27 6:49 UTC (permalink / raw)
To: Jean-Baptiste Thomas; +Cc: linux-raid
In-Reply-To: <972437107.22507366.1430116679975.JavaMail.zimbra@laposte.net>
[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]
On Mon, 27 Apr 2015 08:37:59 +0200 (CEST) Jean-Baptiste Thomas
<cau2jeaf1honoq@laposte.net> wrote:
> I'm looking for a way to get MD to operate in a mode in which
> reading a sector from a RAID-1 device would not succeed until it
> got matching data from at least two components.
>
> Recent experience[1] suggests that a transient problem in one
> disk can completely hose a four way RAID-1 array, which is
> otherwise supposed to still be fine after a triple total
> failure. I'm hoping that a paranoid mode would have prevented
> that.
>
> If there is such a thing, PLEASE tell me. If not, please tell me
> so I don't waste any more time looking for it.
No, there is no such thing.
There "should" be no circumstance which would make it worth while.
A drive may well report an error, but it should *never* report incorrect data
as though it were correct. That is horribly broken.
The cost of running in a "safe" mode would be high, and the likely benefit
extremely low. So it is unlikely that anyone would use it for long. So
implementing it seems rather pointless.
That said: if someone were to provide an implementation I would certainly
consider reviewing it and adding it to md.
NeilBrown
>
> [1] "Massive RAID-1 desync"
> http://www.spinics.net/lists/raid/msg48681.html
> http://marc.info/?l=linux-raid&m=143003812706563&w=2
> --
> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: Paranoid mode for RAID-1 ?
From: Adam Goryachev @ 2015-04-27 6:48 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <972437107.22507366.1430116679975.JavaMail.zimbra@laposte.net>
On 27/04/15 16:37, Jean-Baptiste Thomas wrote:
> I'm looking for a way to get MD to operate in a mode in which
> reading a sector from a RAID-1 device would not succeed until it
> got matching data from at least two components.
>
> Recent experience[1] suggests that a transient problem in one
> disk can completely hose a four way RAID-1 array, which is
> otherwise supposed to still be fine after a triple total
> failure. I'm hoping that a paranoid mode would have prevented
> that.
There isn't any such thing that I am aware of in Linux MD RAID. However,
I've heard that if you want data integrity, then you could use zfs,
which supports RAID as well as data checksums to ensure that the data
read back matches the data you wrote....
Personally, I've never used zfs, and there might be other FS's that will
have the feature as well (eg, btrfs etc).
Hope that helps...
Regards,
Adam
--
Adam Goryachev Website Managers www.websitemanagers.com.au
^ permalink raw reply
* Paranoid mode for RAID-1 ?
From: Jean-Baptiste Thomas @ 2015-04-27 6:37 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <1610581828.22506051.1430116628556.JavaMail.zimbra@laposte.net>
I'm looking for a way to get MD to operate in a mode in which
reading a sector from a RAID-1 device would not succeed until it
got matching data from at least two components.
Recent experience[1] suggests that a transient problem in one
disk can completely hose a four way RAID-1 array, which is
otherwise supposed to still be fine after a triple total
failure. I'm hoping that a paranoid mode would have prevented
that.
If there is such a thing, PLEASE tell me. If not, please tell me
so I don't waste any more time looking for it.
[1] "Massive RAID-1 desync"
http://www.spinics.net/lists/raid/msg48681.html
http://marc.info/?l=linux-raid&m=143003812706563&w=2
^ permalink raw reply
* [PATCH 3/3 v2] md/raid5: per hash value and exclusive wait_for_stripe
From: Yuanhan Liu @ 2015-04-27 4:51 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, linux-kernel, Yuanhan Liu
In-Reply-To: <1430110263-23977-1-git-send-email-yuanhan.liu@linux.intel.com>
I noticed heavy spin lock contention at get_active_stripe() with fsmark
multiple thread write workloads.
Here is how this hot contention comes from. We have limited stripes, and
it's a multiple thread write workload. Hence, those stripes will be taken
soon, which puts later processes to sleep for waiting free stripes. When
enough stripes(>= 1/4 total stripes) are released, all process are woken,
trying to get the lock. But there is one only being able to get this lock
for each hash lock, making other processes spinning out there for acquiring
the lock.
Thus, it's effectiveless to wakeup all processes and let them battle for
a lock that permits one to access only each time. Instead, we could make
it be a exclusive wake up: wake up one process only. That avoids the heavy
spin lock contention naturally.
To do the exclusive wake up, we've to split wait_for_stripe into multiple
wait queues, to make it per hash value, just like the hash lock.
Here are some test results I have got with this patch applied(all test run
3 times):
`fsmark.files_per_sec'
=====================
next-20150317 this patch
------------------------- -------------------------
metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
------------------------- ------------------------- -------- ------------------------------
25.600 ±0.0 92.700 ±2.5 262.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
25.600 ±0.0 77.800 ±0.6 203.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
32.000 ±0.0 93.800 ±1.7 193.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
32.000 ±0.0 81.233 ±1.7 153.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
48.800 ±14.5 99.667 ±2.0 104.2% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
6.400 ±0.0 12.800 ±0.0 100.0% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
63.133 ±8.2 82.800 ±0.7 31.2% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
245.067 ±0.7 306.567 ±7.9 25.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-f2fs-4M-30G-fsyncBeforeClose
17.533 ±0.3 21.000 ±0.8 19.8% ivb44/fsmark/1x-1t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
188.167 ±1.9 215.033 ±3.1 14.3% ivb44/fsmark/1x-1t-4BRD_12G-RAID5-btrfs-4M-30G-NoSync
254.500 ±1.8 290.733 ±2.4 14.2% ivb44/fsmark/1x-1t-9BRD_6G-RAID5-btrfs-4M-30G-NoSync
`time.system_time'
=====================
next-20150317 this patch
------------------------- -------------------------
metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
------------------------- ------------------------- -------- ------------------------------
7235.603 ±1.2 185.163 ±1.9 -97.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
7666.883 ±2.9 202.750 ±1.0 -97.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
14567.893 ±0.7 421.230 ±0.4 -97.1% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
3697.667 ±14.0 148.190 ±1.7 -96.0% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
5572.867 ±3.8 310.717 ±1.4 -94.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
5565.050 ±0.5 313.277 ±1.5 -94.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
2420.707 ±17.1 171.043 ±2.7 -92.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
3743.300 ±4.6 379.827 ±3.5 -89.9% ivb44/fsmark/1x-64t-3HDD-RAID5-ext4-4M-40G-fsyncBeforeClose
3308.687 ±6.3 363.050 ±2.0 -89.0% ivb44/fsmark/1x-64t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
Where,
1x: where 'x' means iterations or loop, corresponding to the 'L' option of fsmark
1t, 64t: where 't' means thread
4M: means the single file size, corresponding to the '-s' option of fsmark
40G, 30G, 120G: means the total test size
4BRD_12G: BRD is the ramdisk, where '4' means 4 ramdisk, and where '12G' means
the size of one ramdisk. So, it would be 48G in total. And we made a
raid on those ramdisk
As you can see, though there are no much performance gain for hard disk
workload, the system time is dropped heavily, up to 97%. And as expected,
the performance increased a lot, up to 260%, for fast device(ram disk).
v2: use bits instead of array to note down wait queue need to wake up.
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
drivers/md/raid5.c | 27 +++++++++++++++++++--------
drivers/md/raid5.h | 2 +-
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 64d5bea..1b11bbf 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -344,7 +344,8 @@ static void release_inactive_stripe_list(struct r5conf *conf,
int hash)
{
int size;
- bool do_wakeup = false;
+ unsigned long do_wakeup = 0;
+ int i = 0;
unsigned long flags;
if (hash == NR_STRIPE_HASH_LOCKS) {
@@ -365,15 +366,19 @@ static void release_inactive_stripe_list(struct r5conf *conf,
!list_empty(list))
atomic_dec(&conf->empty_inactive_list_nr);
list_splice_tail_init(list, conf->inactive_list + hash);
- do_wakeup = true;
+ do_wakeup |= 1 << (size - 1);
spin_unlock_irqrestore(conf->hash_locks + hash, flags);
}
size--;
hash--;
}
+ for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
+ if (do_wakeup & (1 << i))
+ wake_up(&conf->wait_for_stripe[i]);
+ }
+
if (do_wakeup) {
- wake_up(&conf->wait_for_stripe);
if (atomic_read(&conf->active_stripes) == 0)
wake_up(&conf->wait_for_quiescent);
if (conf->retry_read_aligned)
@@ -686,14 +691,15 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
if (!sh) {
set_bit(R5_INACTIVE_BLOCKED,
&conf->cache_state);
- wait_event_lock_irq(
- conf->wait_for_stripe,
+ wait_event_cmd_exclusive(
+ conf->wait_for_stripe[hash],
!list_empty(conf->inactive_list + hash) &&
(atomic_read(&conf->active_stripes)
< (conf->max_nr_stripes * 3 / 4)
|| !test_bit(R5_INACTIVE_BLOCKED,
&conf->cache_state)),
- *(conf->hash_locks + hash));
+ spin_unlock_irq(conf->hash_locks + hash),
+ spin_lock_irq(conf->hash_locks + hash));
clear_bit(R5_INACTIVE_BLOCKED,
&conf->cache_state);
} else {
@@ -718,6 +724,9 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
}
} while (sh == NULL);
+ if (!list_empty(conf->inactive_list + hash))
+ wake_up(&conf->wait_for_stripe[hash]);
+
spin_unlock_irq(conf->hash_locks + hash);
return sh;
}
@@ -2138,7 +2147,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
cnt = 0;
list_for_each_entry(nsh, &newstripes, lru) {
lock_device_hash_lock(conf, hash);
- wait_event_cmd(conf->wait_for_stripe,
+ wait_event_cmd_exclusive(conf->wait_for_stripe[hash],
!list_empty(conf->inactive_list + hash),
unlock_device_hash_lock(conf, hash),
lock_device_hash_lock(conf, hash));
@@ -6402,7 +6411,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
spin_lock_init(&conf->device_lock);
seqcount_init(&conf->gen_lock);
init_waitqueue_head(&conf->wait_for_quiescent);
- init_waitqueue_head(&conf->wait_for_stripe);
+ for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
+ init_waitqueue_head(&conf->wait_for_stripe[i]);
+ }
init_waitqueue_head(&conf->wait_for_overlap);
INIT_LIST_HEAD(&conf->handle_list);
INIT_LIST_HEAD(&conf->hold_list);
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 4cc05ec..6307b90 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -509,7 +509,7 @@ struct r5conf {
atomic_t empty_inactive_list_nr;
struct llist_head released_stripes;
wait_queue_head_t wait_for_quiescent;
- wait_queue_head_t wait_for_stripe;
+ wait_queue_head_t wait_for_stripe[NR_STRIPE_HASH_LOCKS];
wait_queue_head_t wait_for_overlap;
unsigned long cache_state;
#define R5_INACTIVE_BLOCKED 1 /* release of inactive stripes blocked,
--
1.9.0
^ permalink raw reply related
* [PATCH 2/3 v2] md/raid5: split wait_for_stripe and introduce wait_for_quiescent
From: Yuanhan Liu @ 2015-04-27 4:51 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, linux-kernel, Yuanhan Liu
In-Reply-To: <1430110263-23977-1-git-send-email-yuanhan.liu@linux.intel.com>
I noticed heavy spin lock contention at get_active_stripe(), introduced
at being wake up stage, where a bunch of processes try to re-hold the
spin lock again.
After giving some thoughts on this issue, I found the lock could be
relieved(and even avoided) if we turn the wait_for_stripe to per
waitqueue for each lock hash and make the wake up exclusive: wake up
one process each time, which avoids the lock contention naturally.
Before go hacking with wait_for_stripe, I found it actually has 2
usages: for the array to enter or leave the quiescent state, and also
to wait for an available stripe in each of the hash lists.
So this patch splits the first usage off into a separate wait_queue,
wait_for_quiescent, and the next patch will turn the second usage into
one waitqueue for each hash value, and make it exclusive, to relieve
the lock contention.
v2: wake_up(wait_for_quiescent) when (active_stripes == 0)
Commit log refactor suggestion from Neil.
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
drivers/md/raid5.c | 15 +++++++++------
drivers/md/raid5.h | 1 +
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 77dfd72..64d5bea 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -374,6 +374,8 @@ static void release_inactive_stripe_list(struct r5conf *conf,
if (do_wakeup) {
wake_up(&conf->wait_for_stripe);
+ if (atomic_read(&conf->active_stripes) == 0)
+ wake_up(&conf->wait_for_quiescent);
if (conf->retry_read_aligned)
md_wakeup_thread(conf->mddev->thread);
}
@@ -667,7 +669,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
spin_lock_irq(conf->hash_locks + hash);
do {
- wait_event_lock_irq(conf->wait_for_stripe,
+ wait_event_lock_irq(conf->wait_for_quiescent,
conf->quiesce == 0 || noquiesce,
*(conf->hash_locks + hash));
sh = __find_stripe(conf, sector, conf->generation - previous);
@@ -4729,7 +4731,7 @@ static void raid5_align_endio(struct bio *bi, int error)
raid_bi, 0);
bio_endio(raid_bi, 0);
if (atomic_dec_and_test(&conf->active_aligned_reads))
- wake_up(&conf->wait_for_stripe);
+ wake_up(&conf->wait_for_quiescent);
return;
}
@@ -4824,7 +4826,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
align_bi->bi_iter.bi_sector += rdev->data_offset;
spin_lock_irq(&conf->device_lock);
- wait_event_lock_irq(conf->wait_for_stripe,
+ wait_event_lock_irq(conf->wait_for_quiescent,
conf->quiesce == 0,
conf->device_lock);
atomic_inc(&conf->active_aligned_reads);
@@ -5668,7 +5670,7 @@ static int retry_aligned_read(struct r5conf *conf, struct bio *raid_bio)
bio_endio(raid_bio, 0);
}
if (atomic_dec_and_test(&conf->active_aligned_reads))
- wake_up(&conf->wait_for_stripe);
+ wake_up(&conf->wait_for_quiescent);
return handled;
}
@@ -6399,6 +6401,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
goto abort;
spin_lock_init(&conf->device_lock);
seqcount_init(&conf->gen_lock);
+ init_waitqueue_head(&conf->wait_for_quiescent);
init_waitqueue_head(&conf->wait_for_stripe);
init_waitqueue_head(&conf->wait_for_overlap);
INIT_LIST_HEAD(&conf->handle_list);
@@ -7422,7 +7425,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
* active stripes can drain
*/
conf->quiesce = 2;
- wait_event_cmd(conf->wait_for_stripe,
+ wait_event_cmd(conf->wait_for_quiescent,
atomic_read(&conf->active_stripes) == 0 &&
atomic_read(&conf->active_aligned_reads) == 0,
unlock_all_device_hash_locks_irq(conf),
@@ -7436,7 +7439,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
case 0: /* re-enable writes */
lock_all_device_hash_locks_irq(conf);
conf->quiesce = 0;
- wake_up(&conf->wait_for_stripe);
+ wake_up(&conf->wait_for_quiescent);
wake_up(&conf->wait_for_overlap);
unlock_all_device_hash_locks_irq(conf);
break;
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 7dc0dd8..4cc05ec 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -508,6 +508,7 @@ struct r5conf {
struct list_head inactive_list[NR_STRIPE_HASH_LOCKS];
atomic_t empty_inactive_list_nr;
struct llist_head released_stripes;
+ wait_queue_head_t wait_for_quiescent;
wait_queue_head_t wait_for_stripe;
wait_queue_head_t wait_for_overlap;
unsigned long cache_state;
--
1.9.0
^ permalink raw reply related
* [PATCH 1/3] wait: introduce wait_event_cmd_exclusive
From: Yuanhan Liu @ 2015-04-27 4:51 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, linux-kernel, Yuanhan Liu, Ingo Molnar,
Peter Zijlstra
It's just a variant of wait_event_cmd, with exclusive flag being set.
For cases like RAID5, which puts many processes to sleep until 1/4
resources are free, a wake_up wakes up all processes to run, but
there is one process being able to get the resource as it's protected
by a spin lock. That ends up introducing heavy lock contentions, and
hurts performance badly.
Here introduce wait_event_cmd_exclusive to relieve the lock contention
naturally by letting wake_up() just wake up one process.
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
include/linux/wait.h | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 2db8334..6c3b4de 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -358,10 +358,18 @@ do { \
__ret; \
})
-#define __wait_event_cmd(wq, condition, cmd1, cmd2) \
- (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0, 0, \
+#define __wait_event_cmd(wq, condition, cmd1, cmd2, exclusive) \
+ (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, exclusive, 0, \
cmd1; schedule(); cmd2)
+
+#define wait_event_cmd_exclusive(wq, condition, cmd1, cmd2) \
+do { \
+ if (condition) \
+ break; \
+ __wait_event_cmd(wq, condition, cmd1, cmd2, 1); \
+} while (0)
+
/**
* wait_event_cmd - sleep until a condition gets true
* @wq: the waitqueue to wait on
@@ -380,7 +388,7 @@ do { \
do { \
if (condition) \
break; \
- __wait_event_cmd(wq, condition, cmd1, cmd2); \
+ __wait_event_cmd(wq, condition, cmd1, cmd2, 0); \
} while (0)
#define __wait_event_interruptible(wq, condition) \
--
1.9.0
^ permalink raw reply related
* [PATCH -stable] block: destroy bdi before blockdev is unregistered.
From: NeilBrown @ 2015-04-27 4:12 UTC (permalink / raw)
To: Jens Axboe
Cc: Azat Khuzhin, Christoph Hellwig, Kernel.org-Linux-RAID,
Guoqing Jiang, Tejun Heo, Jan Kara, lkml
In-Reply-To: <20150423160551.45345f96@notabene.brown>
[-- Attachment #1: Type: text/plain, Size: 3587 bytes --]
Because of the peculiar way that md devices are created (automatically
when the device node is opened), a new device can be created and
registered immediately after the
blk_unregister_region(disk_devt(disk), disk->minors);
call in del_gendisk().
Therefore it is important that all visible artifacts of the previous
device are removed before this call. In particular, the 'bdi'.
Since:
commit c4db59d31e39ea067c32163ac961e9c80198fd37
Author: Christoph Hellwig <hch@lst.de>
fs: don't reassign dirty inodes to default_backing_dev_info
moved the
device_unregister(bdi->dev);
call from bdi_unregister() to bdi_destroy() it has been quite easy to
lose a race and have a new (e.g.) "md127" be created after the
blk_unregister_region() call and before bdi_destroy() is ultimately
called by the final 'put_disk', which must come after del_gendisk().
The new device finds that the bdi name is already registered in sysfs
and complains
> [ 9627.630029] WARNING: CPU: 18 PID: 3330 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x5a/0x70()
> [ 9627.630032] sysfs: cannot create duplicate filename '/devices/virtual/bdi/9:127'
We can fix this by moving the bdi_destroy() call out of
blk_release_queue() (which can happen very late when a refcount
reaches zero) and into blk_cleanup_queue() - which happens exactly when the md
device driver calls it.
Then it is only necessary for md to call blk_cleanup_queue() before
del_gendisk(). As loop.c devices are also created on demand by
opening the device node, we make the same change there.
Fixes: c4db59d31e39ea067c32163ac961e9c80198fd37
Reported-by: Azat Khuzhin <a3at.mail@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org (v4.0)
Signed-off-by: NeilBrown <neilb@suse.de>
--
Hi Jens,
if you could check this and forward on to Linus I'd really appreciate it.
Thanks,
NeilBrown
diff --git a/block/blk-core.c b/block/blk-core.c
index fd154b94447a..7871603f0a29 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -552,6 +552,8 @@ void blk_cleanup_queue(struct request_queue *q)
q->queue_lock = &q->__queue_lock;
spin_unlock_irq(lock);
+ bdi_destroy(&q->backing_dev_info);
+
/* @q is and will stay empty, shutdown and put */
blk_put_queue(q);
}
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index faaf36ade7eb..2b8fd302f677 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -522,8 +522,6 @@ static void blk_release_queue(struct kobject *kobj)
blk_trace_shutdown(q);
- bdi_destroy(&q->backing_dev_info);
-
ida_simple_remove(&blk_queue_ida, q->id);
call_rcu(&q->rcu_head, blk_free_queue_rcu);
}
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ae3fcb4199e9..d7173cb1ea76 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1620,8 +1620,8 @@ out:
static void loop_remove(struct loop_device *lo)
{
- del_gendisk(lo->lo_disk);
blk_cleanup_queue(lo->lo_queue);
+ del_gendisk(lo->lo_disk);
blk_mq_free_tag_set(&lo->tag_set);
put_disk(lo->lo_disk);
kfree(lo);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d4f31e195e26..593a02476c78 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4818,12 +4818,12 @@ static void md_free(struct kobject *ko)
if (mddev->sysfs_state)
sysfs_put(mddev->sysfs_state);
+ if (mddev->queue)
+ blk_cleanup_queue(mddev->queue);
if (mddev->gendisk) {
del_gendisk(mddev->gendisk);
put_disk(mddev->gendisk);
}
- if (mddev->queue)
- blk_cleanup_queue(mddev->queue);
kfree(mddev);
}
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply related
* Re: [PATCH 1/2] md/raid5: split wait_for_stripe and introduce wait_for_quiesce
From: NeilBrown @ 2015-04-27 2:24 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: linux-raid, linux-kernel
In-Reply-To: <20150427021249.GG17176@yliu-dev.sh.intel.com>
[-- Attachment #1: Type: text/plain, Size: 7886 bytes --]
On Mon, 27 Apr 2015 10:12:49 +0800 Yuanhan Liu <yuanhan.liu@linux.intel.com>
wrote:
> On Mon, Apr 27, 2015 at 10:10:24AM +1000, NeilBrown wrote:
> > On Fri, 24 Apr 2015 21:39:03 +0800 Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > wrote:
> >
> > > If I read code correctly, current wait_for_stripe actually has 2 usage:
> > >
> > > - wait for there is enough free stripe cache, triggered when
> > > get_free_stripe() failed. This is what wait_for_stripe intend
> > > for literally.
> > >
> > > - wait for quiesce == 0 or
> > > active_aligned_reads == 0 && active_stripes == 0
> > >
> > > It has nothing to do with wait_for_stripe literally, and releasing
> > > an active stripe won't actually wake them up. On the contrary, wake_up
> > > from under this case won't actually wake up the process waiting for
> > > an free stripe being available.
> >
> > I disagree. Releasing an active stripe *will* (or *can*) wake up that third
> > case, as it decrements "active_stripes" which will eventually reach zero.
> >
> > I don't think your new code will properly wake up a process which is waiting
> > for "active_stripes == 0".
>
> Right, and thanks for pointing it out. So, is this enough?
>
> ---
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 2d8fcc1..3f23035 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -383,6 +383,9 @@ static void release_inactive_stripe_list(struct
> r5conf *conf,
> }
> }
> }
> +
> + if (!atomic_read(&conf->active_stripes))
> + wake_up(&conf->wait_for_quiesce);
> }
>
> /* should hold conf->device_lock already */
>
>
> Or, should I put it a bit ahead, trying to invoke wake_up(&conf->wait_for_quiesce)
> after each atomic_dec(&conf->active_stripes)?
>
> if (atomic_dec_return(&conf->active_stripes) == 0)
> wake_up(&conf->wait_for_quiesce);
I think the first version is fine. While waiting for active_stripes to be
zero, ->quiesce is set to 2, and so no new stripes should get used.
>
> >
> > >
> > > Hence, we'd better split wait_for_stripe, and here I introduce
> > > wait_for_quiesce for the second usage. The name may not well taken, or
> > > even taken wrongly. Feel free to correct me then.
> > >
> > > This is also a prepare patch for next patch: make wait_for_stripe
> > > exclusive.
> >
> > I think you have this commit description upside down :-)
> >
> > The real motivation is that you are seeing contention on some spinlock and so
> > you want to split 'wait_for_stripe' up in to multiple wait_queues so that you
> > can use exclusive wakeup. As this is the main motivation, it should be
> > stated first.
> >
> > Then explain that 'wait_for_stripe' is used to wait for the array to enter or
> > leave the quiescent state, and also to wait for an available stripe in each
> > of the hash lists.
> >
> > So this patch splits the first usage off into a separate wait_queue, and the
> > next patch will split the second usage into one waitqueue for each hash value.
> >
> > Then explain just is what is needed for that first step.
> >
> > When you put it that way around, the patch makes lots of sense.
>
> It does, and thanks!
>
> >
> > So: could you please resubmit with the description the right way around, and
>
> To make sure I followed you correctly, my patch order is correct(I mean,
> split lock first, and make wait_for_stripe per lock hash and exclusive
> second), and what I need to do is re-writing the commit log as you suggested,
> and fixing all issues you pointed out. Right?
Correct.
Thanks,
NeilBrown
>
> --yliu
>
> > with an appropriate wakeup call to ensure raid5_quiesce is woken up when
> > active_stripes reaches zero?
> >
> > Thanks,
> > NeilBrown
> >
> >
> > >
> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > > ---
> > > drivers/md/raid5.c | 13 +++++++------
> > > drivers/md/raid5.h | 1 +
> > > 2 files changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > > index 9716319..b7e385f 100644
> > > --- a/drivers/md/raid5.c
> > > +++ b/drivers/md/raid5.c
> > > @@ -667,7 +667,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> > > spin_lock_irq(conf->hash_locks + hash);
> > >
> > > do {
> > > - wait_event_lock_irq(conf->wait_for_stripe,
> > > + wait_event_lock_irq(conf->wait_for_quiesce,
> > > conf->quiesce == 0 || noquiesce,
> > > *(conf->hash_locks + hash));
> > > sh = __find_stripe(conf, sector, conf->generation - previous);
> > > @@ -4725,7 +4725,7 @@ static void raid5_align_endio(struct bio *bi, int error)
> > > raid_bi, 0);
> > > bio_endio(raid_bi, 0);
> > > if (atomic_dec_and_test(&conf->active_aligned_reads))
> > > - wake_up(&conf->wait_for_stripe);
> > > + wake_up(&conf->wait_for_quiesce);
> > > return;
> > > }
> > >
> > > @@ -4820,7 +4820,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
> > > align_bi->bi_iter.bi_sector += rdev->data_offset;
> > >
> > > spin_lock_irq(&conf->device_lock);
> > > - wait_event_lock_irq(conf->wait_for_stripe,
> > > + wait_event_lock_irq(conf->wait_for_quiesce,
> > > conf->quiesce == 0,
> > > conf->device_lock);
> > > atomic_inc(&conf->active_aligned_reads);
> > > @@ -5659,7 +5659,7 @@ static int retry_aligned_read(struct r5conf *conf, struct bio *raid_bio)
> > > bio_endio(raid_bio, 0);
> > > }
> > > if (atomic_dec_and_test(&conf->active_aligned_reads))
> > > - wake_up(&conf->wait_for_stripe);
> > > + wake_up(&conf->wait_for_quiesce);
> > > return handled;
> > > }
> > >
> > > @@ -6390,6 +6390,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> > > goto abort;
> > > spin_lock_init(&conf->device_lock);
> > > seqcount_init(&conf->gen_lock);
> > > + init_waitqueue_head(&conf->wait_for_quiesce);
> > > init_waitqueue_head(&conf->wait_for_stripe);
> > > init_waitqueue_head(&conf->wait_for_overlap);
> > > INIT_LIST_HEAD(&conf->handle_list);
> > > @@ -7413,7 +7414,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> > > * active stripes can drain
> > > */
> > > conf->quiesce = 2;
> > > - wait_event_cmd(conf->wait_for_stripe,
> > > + wait_event_cmd(conf->wait_for_quiesce,
> > > atomic_read(&conf->active_stripes) == 0 &&
> > > atomic_read(&conf->active_aligned_reads) == 0,
> > > unlock_all_device_hash_locks_irq(conf),
> > > @@ -7427,7 +7428,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> > > case 0: /* re-enable writes */
> > > lock_all_device_hash_locks_irq(conf);
> > > conf->quiesce = 0;
> > > - wake_up(&conf->wait_for_stripe);
> > > + wake_up(&conf->wait_for_quiesce);
> > > wake_up(&conf->wait_for_overlap);
> > > unlock_all_device_hash_locks_irq(conf);
> > > break;
> > > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > > index 7dc0dd8..fab53a3 100644
> > > --- a/drivers/md/raid5.h
> > > +++ b/drivers/md/raid5.h
> > > @@ -508,6 +508,7 @@ struct r5conf {
> > > struct list_head inactive_list[NR_STRIPE_HASH_LOCKS];
> > > atomic_t empty_inactive_list_nr;
> > > struct llist_head released_stripes;
> > > + wait_queue_head_t wait_for_quiesce;
> > > wait_queue_head_t wait_for_stripe;
> > > wait_queue_head_t wait_for_overlap;
> > > unsigned long cache_state;
> >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] md/raid5: exclusive wait_for_stripe
From: Yuanhan Liu @ 2015-04-27 2:16 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, linux-kernel
In-Reply-To: <20150427102405.40dc2ada@notabene.brown>
On Mon, Apr 27, 2015 at 10:24:05AM +1000, NeilBrown wrote:
> On Fri, 24 Apr 2015 21:39:04 +0800 Yuanhan Liu <yuanhan.liu@linux.intel.com>
> wrote:
>
> > I noticed heavy spin lock contention at get_active_stripe() with fsmark
> > multiple thread write workloads.
> >
> > Here is how this hot contention comes from. We have limited stripes, and
> > it's a multiple thread write workload. Hence, those stripes will be taken
> > soon, which puts later processes to sleep for waiting free stripes. When
> > enough stripes(> 1/4 total stripes) are released, all process are woken,
> > trying to get the lock. But there is one only being able to get this lock
> > for each hash lock, making other processes spinning out there for acquiring
> > the lock.
> >
> > Thus, it's effectiveless to wakeup all processes and let them battle for
> > a lock that permits one to access only each time. Instead, we could make
> > it be a exclusive wake up: wake up one process only. That avoids the heavy
> > spin lock contention naturally.
> >
> > Here are some test results I have got with this patch applied(all test run
> > 3 times):
> >
> > `fsmark.files_per_sec'
> > =====================
> >
> > next-20150317 this patch
> > ------------------------- -------------------------
> > metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
> > ------------------------- ------------------------- -------- ------------------------------
> > 25.600 ±0.0 92.700 ±2.5 262.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> > 25.600 ±0.0 77.800 ±0.6 203.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> > 32.000 ±0.0 93.800 ±1.7 193.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
> > 32.000 ±0.0 81.233 ±1.7 153.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
> > 48.800 ±14.5 99.667 ±2.0 104.2% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
> > 6.400 ±0.0 12.800 ±0.0 100.0% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
> > 63.133 ±8.2 82.800 ±0.7 31.2% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
> > 245.067 ±0.7 306.567 ±7.9 25.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-f2fs-4M-30G-fsyncBeforeClose
> > 17.533 ±0.3 21.000 ±0.8 19.8% ivb44/fsmark/1x-1t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
> > 188.167 ±1.9 215.033 ±3.1 14.3% ivb44/fsmark/1x-1t-4BRD_12G-RAID5-btrfs-4M-30G-NoSync
> > 254.500 ±1.8 290.733 ±2.4 14.2% ivb44/fsmark/1x-1t-9BRD_6G-RAID5-btrfs-4M-30G-NoSync
> >
> > `time.system_time'
> > =====================
> >
> > next-20150317 this patch
> > ------------------------- -------------------------
> > metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
> > ------------------------- ------------------------- -------- ------------------------------
> > 7235.603 ±1.2 185.163 ±1.9 -97.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> > 7666.883 ±2.9 202.750 ±1.0 -97.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> > 14567.893 ±0.7 421.230 ±0.4 -97.1% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
> > 3697.667 ±14.0 148.190 ±1.7 -96.0% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
> > 5572.867 ±3.8 310.717 ±1.4 -94.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
> > 5565.050 ±0.5 313.277 ±1.5 -94.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
> > 2420.707 ±17.1 171.043 ±2.7 -92.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
> > 3743.300 ±4.6 379.827 ±3.5 -89.9% ivb44/fsmark/1x-64t-3HDD-RAID5-ext4-4M-40G-fsyncBeforeClose
> > 3308.687 ±6.3 363.050 ±2.0 -89.0% ivb44/fsmark/1x-64t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
> >
> > Where,
> >
> > 1x: where 'x' means iterations or loop, corresponding to the 'L' option of fsmark
> >
> > 1t, 64t: where 't' means thread
> >
> > 4M: means the single file size, corresponding to the '-s' option of fsmark
> > 40G, 30G, 120G: means the total test size
> >
> > 4BRD_12G: BRD is the ramdisk, where '4' means 4 ramdisk, and where '12G' means
> > the size of one ramdisk. So, it would be 48G in total. And we made a
> > raid on those ramdisk
> >
> > As you can see, though there are no much performance gain for hard disk
> > workload, the system time is dropped heavily, up to 97%. And as expected,
> > the performance increased a lot, up to 260%, for fast device(ram disk).
> >
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> > drivers/md/raid5.c | 46 +++++++++++++++++++++++++++++++++++-----------
> > drivers/md/raid5.h | 2 +-
> > 2 files changed, 36 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index b7e385f..2d8fcc1 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -344,7 +344,8 @@ static void release_inactive_stripe_list(struct r5conf *conf,
> > int hash)
> > {
> > int size;
> > - bool do_wakeup = false;
> > + bool do_wakeup[NR_STRIPE_HASH_LOCKS] = { false, };
>
> I think I'd rather use an 'unsigned long' and set bits.
Will do that.
>
> > + int i = 0;
> > unsigned long flags;
> >
> > if (hash == NR_STRIPE_HASH_LOCKS) {
> > @@ -365,17 +366,22 @@ static void release_inactive_stripe_list(struct r5conf *conf,
> > !list_empty(list))
> > atomic_dec(&conf->empty_inactive_list_nr);
> > list_splice_tail_init(list, conf->inactive_list + hash);
> > - do_wakeup = true;
> > + do_wakeup[size - 1] = true;
>
> ... so this becomes
> do_wakeup |= 1 << (size - 1);
>
> > spin_unlock_irqrestore(conf->hash_locks + hash, flags);
> > }
> > size--;
> > hash--;
> > }
> >
> > - if (do_wakeup) {
> > - wake_up(&conf->wait_for_stripe);
> > - if (conf->retry_read_aligned)
> > - md_wakeup_thread(conf->mddev->thread);
> > + for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
> > + bool waked_thread = false;
> > + if (do_wakeup[i]) {
> > + wake_up(&conf->wait_for_stripe[i]);
> > + if (!waked_thread) {
> > + waked_thread = true;
> > + md_wakeup_thread(conf->mddev->thread);
> > + }
> > + }
>
> I don't think you want waked_thread to be local to this loop.
> As it is, the "if (!waked_thread)" test *always* succeeds.
>
> You can discard it if do_wakeup becomes and unsigned long, and just do
>
> if (do_wakeup && conf->retry_read_aligned)
> md_wakeup_thread(conf->mddev->thread);
>
> And why have you removed the test on conf->retry_read_aligned??
Oops, a careless editing.
>
> > }
> > }
> >
> > @@ -655,6 +661,18 @@ static int has_failed(struct r5conf *conf)
> > return 0;
> > }
> >
> > +/* XXX: might put it to linux/wait.h to be a public API? */
>
> Yes, definitely put it in linux/wait.h
I will send a seperate patch for that.
Thanks.
--yliu
>
>
>
>
> > +#define raid_wait_event_exclusive_cmd(wq, condition, cmd1, cmd2) \
> > +do { \
> > + if (condition) \
> > + break; \
> > + (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 1, 0, \
> > + cmd1; \
> > + schedule(); \
> > + cmd2); \
> > +} while (0)
> > +
> > +
> > static struct stripe_head *
> > get_active_stripe(struct r5conf *conf, sector_t sector,
> > int previous, int noblock, int noquiesce)
> > @@ -684,14 +702,15 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> > if (!sh) {
> > set_bit(R5_INACTIVE_BLOCKED,
> > &conf->cache_state);
> > - wait_event_lock_irq(
> > - conf->wait_for_stripe,
> > + raid_wait_event_exclusive_cmd(
> > + conf->wait_for_stripe[hash],
> > !list_empty(conf->inactive_list + hash) &&
> > (atomic_read(&conf->active_stripes)
> > < (conf->max_nr_stripes * 3 / 4)
> > || !test_bit(R5_INACTIVE_BLOCKED,
> > &conf->cache_state)),
> > - *(conf->hash_locks + hash));
> > + spin_unlock_irq(conf->hash_locks + hash),
> > + spin_lock_irq(conf->hash_locks + hash));
> > clear_bit(R5_INACTIVE_BLOCKED,
> > &conf->cache_state);
> > } else {
> > @@ -716,6 +735,9 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> > }
> > } while (sh == NULL);
> >
> > + if (!list_empty(conf->inactive_list + hash))
> > + wake_up(&conf->wait_for_stripe[hash]);
> > +
> > spin_unlock_irq(conf->hash_locks + hash);
> > return sh;
> > }
> > @@ -2136,7 +2158,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
> > cnt = 0;
> > list_for_each_entry(nsh, &newstripes, lru) {
> > lock_device_hash_lock(conf, hash);
> > - wait_event_cmd(conf->wait_for_stripe,
> > + raid_wait_event_exclusive_cmd(conf->wait_for_stripe[hash],
> > !list_empty(conf->inactive_list + hash),
> > unlock_device_hash_lock(conf, hash),
> > lock_device_hash_lock(conf, hash));
> > @@ -6391,7 +6413,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> > spin_lock_init(&conf->device_lock);
> > seqcount_init(&conf->gen_lock);
> > init_waitqueue_head(&conf->wait_for_quiesce);
> > - init_waitqueue_head(&conf->wait_for_stripe);
> > + for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
> > + init_waitqueue_head(&conf->wait_for_stripe[i]);
> > + }
> > init_waitqueue_head(&conf->wait_for_overlap);
> > INIT_LIST_HEAD(&conf->handle_list);
> > INIT_LIST_HEAD(&conf->hold_list);
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index fab53a3..cdad2d2 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> > @@ -509,7 +509,7 @@ struct r5conf {
> > atomic_t empty_inactive_list_nr;
> > struct llist_head released_stripes;
> > wait_queue_head_t wait_for_quiesce;
> > - wait_queue_head_t wait_for_stripe;
> > + wait_queue_head_t wait_for_stripe[NR_STRIPE_HASH_LOCKS];
> > wait_queue_head_t wait_for_overlap;
> > unsigned long cache_state;
> > #define R5_INACTIVE_BLOCKED 1 /* release of inactive stripes blocked,
>
^ permalink raw reply
* Re: [PATCH 1/2] md/raid5: split wait_for_stripe and introduce wait_for_quiesce
From: Yuanhan Liu @ 2015-04-27 2:12 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, linux-kernel
In-Reply-To: <20150427101024.2ae8edc1@notabene.brown>
On Mon, Apr 27, 2015 at 10:10:24AM +1000, NeilBrown wrote:
> On Fri, 24 Apr 2015 21:39:03 +0800 Yuanhan Liu <yuanhan.liu@linux.intel.com>
> wrote:
>
> > If I read code correctly, current wait_for_stripe actually has 2 usage:
> >
> > - wait for there is enough free stripe cache, triggered when
> > get_free_stripe() failed. This is what wait_for_stripe intend
> > for literally.
> >
> > - wait for quiesce == 0 or
> > active_aligned_reads == 0 && active_stripes == 0
> >
> > It has nothing to do with wait_for_stripe literally, and releasing
> > an active stripe won't actually wake them up. On the contrary, wake_up
> > from under this case won't actually wake up the process waiting for
> > an free stripe being available.
>
> I disagree. Releasing an active stripe *will* (or *can*) wake up that third
> case, as it decrements "active_stripes" which will eventually reach zero.
>
> I don't think your new code will properly wake up a process which is waiting
> for "active_stripes == 0".
Right, and thanks for pointing it out. So, is this enough?
---
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 2d8fcc1..3f23035 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -383,6 +383,9 @@ static void release_inactive_stripe_list(struct
r5conf *conf,
}
}
}
+
+ if (!atomic_read(&conf->active_stripes))
+ wake_up(&conf->wait_for_quiesce);
}
/* should hold conf->device_lock already */
Or, should I put it a bit ahead, trying to invoke wake_up(&conf->wait_for_quiesce)
after each atomic_dec(&conf->active_stripes)?
if (atomic_dec_return(&conf->active_stripes) == 0)
wake_up(&conf->wait_for_quiesce);
>
> >
> > Hence, we'd better split wait_for_stripe, and here I introduce
> > wait_for_quiesce for the second usage. The name may not well taken, or
> > even taken wrongly. Feel free to correct me then.
> >
> > This is also a prepare patch for next patch: make wait_for_stripe
> > exclusive.
>
> I think you have this commit description upside down :-)
>
> The real motivation is that you are seeing contention on some spinlock and so
> you want to split 'wait_for_stripe' up in to multiple wait_queues so that you
> can use exclusive wakeup. As this is the main motivation, it should be
> stated first.
>
> Then explain that 'wait_for_stripe' is used to wait for the array to enter or
> leave the quiescent state, and also to wait for an available stripe in each
> of the hash lists.
>
> So this patch splits the first usage off into a separate wait_queue, and the
> next patch will split the second usage into one waitqueue for each hash value.
>
> Then explain just is what is needed for that first step.
>
> When you put it that way around, the patch makes lots of sense.
It does, and thanks!
>
> So: could you please resubmit with the description the right way around, and
To make sure I followed you correctly, my patch order is correct(I mean,
split lock first, and make wait_for_stripe per lock hash and exclusive
second), and what I need to do is re-writing the commit log as you suggested,
and fixing all issues you pointed out. Right?
--yliu
> with an appropriate wakeup call to ensure raid5_quiesce is woken up when
> active_stripes reaches zero?
>
> Thanks,
> NeilBrown
>
>
> >
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> > drivers/md/raid5.c | 13 +++++++------
> > drivers/md/raid5.h | 1 +
> > 2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 9716319..b7e385f 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -667,7 +667,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> > spin_lock_irq(conf->hash_locks + hash);
> >
> > do {
> > - wait_event_lock_irq(conf->wait_for_stripe,
> > + wait_event_lock_irq(conf->wait_for_quiesce,
> > conf->quiesce == 0 || noquiesce,
> > *(conf->hash_locks + hash));
> > sh = __find_stripe(conf, sector, conf->generation - previous);
> > @@ -4725,7 +4725,7 @@ static void raid5_align_endio(struct bio *bi, int error)
> > raid_bi, 0);
> > bio_endio(raid_bi, 0);
> > if (atomic_dec_and_test(&conf->active_aligned_reads))
> > - wake_up(&conf->wait_for_stripe);
> > + wake_up(&conf->wait_for_quiesce);
> > return;
> > }
> >
> > @@ -4820,7 +4820,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
> > align_bi->bi_iter.bi_sector += rdev->data_offset;
> >
> > spin_lock_irq(&conf->device_lock);
> > - wait_event_lock_irq(conf->wait_for_stripe,
> > + wait_event_lock_irq(conf->wait_for_quiesce,
> > conf->quiesce == 0,
> > conf->device_lock);
> > atomic_inc(&conf->active_aligned_reads);
> > @@ -5659,7 +5659,7 @@ static int retry_aligned_read(struct r5conf *conf, struct bio *raid_bio)
> > bio_endio(raid_bio, 0);
> > }
> > if (atomic_dec_and_test(&conf->active_aligned_reads))
> > - wake_up(&conf->wait_for_stripe);
> > + wake_up(&conf->wait_for_quiesce);
> > return handled;
> > }
> >
> > @@ -6390,6 +6390,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> > goto abort;
> > spin_lock_init(&conf->device_lock);
> > seqcount_init(&conf->gen_lock);
> > + init_waitqueue_head(&conf->wait_for_quiesce);
> > init_waitqueue_head(&conf->wait_for_stripe);
> > init_waitqueue_head(&conf->wait_for_overlap);
> > INIT_LIST_HEAD(&conf->handle_list);
> > @@ -7413,7 +7414,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> > * active stripes can drain
> > */
> > conf->quiesce = 2;
> > - wait_event_cmd(conf->wait_for_stripe,
> > + wait_event_cmd(conf->wait_for_quiesce,
> > atomic_read(&conf->active_stripes) == 0 &&
> > atomic_read(&conf->active_aligned_reads) == 0,
> > unlock_all_device_hash_locks_irq(conf),
> > @@ -7427,7 +7428,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> > case 0: /* re-enable writes */
> > lock_all_device_hash_locks_irq(conf);
> > conf->quiesce = 0;
> > - wake_up(&conf->wait_for_stripe);
> > + wake_up(&conf->wait_for_quiesce);
> > wake_up(&conf->wait_for_overlap);
> > unlock_all_device_hash_locks_irq(conf);
> > break;
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index 7dc0dd8..fab53a3 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> > @@ -508,6 +508,7 @@ struct r5conf {
> > struct list_head inactive_list[NR_STRIPE_HASH_LOCKS];
> > atomic_t empty_inactive_list_nr;
> > struct llist_head released_stripes;
> > + wait_queue_head_t wait_for_quiesce;
> > wait_queue_head_t wait_for_stripe;
> > wait_queue_head_t wait_for_overlap;
> > unsigned long cache_state;
>
^ permalink raw reply related
* Fwd: BUG?: RAID6 reshape hung in reshape_request
From: David Wahler @ 2015-04-27 1:56 UTC (permalink / raw)
To: linux-raid, NeilBrown
In-Reply-To: <CAGivzjE4zVGpoUdGpgKR_e+EaiBE60R3Ta=o9mW+VFqO8McrrQ@mail.gmail.com>
[oops, forgot to cc the list]
On Sun, Apr 26, 2015 at 8:20 PM, NeilBrown <neilb@suse.de> wrote:
>> And the output of mdadm --detail/-E:
>> https://gist.github.com/anonymous/0b090668b56ef54bb2f0
>
> What is wrong with simply including this directly in the email???
My bad; I wasn't sure whether it was appropriate to paste such a long
dump inline.
> Anyway:
>
> Bad Block Log : 512 entries available at offset 72 sectors - bad blocks present.
>
> that is the only thing that looks at all interesting. Particularly the last
> 3 words.
> What does
> mdadm --examine-badblocks /dev/sd[cde]1
> show?
root@ceres:~# mdadm --examine-badblocks /dev/sd[cde]1
Bad-blocks on /dev/sdc1:
3699640928 for 32 sectors
Bad-blocks on /dev/sdd1:
3699640928 for 32 sectors
Bad-blocks on /dev/sde1:
3699640928 for 32 sectors
Hmm, that seems kind of odd to me. For what it's worth, all four
drives passed a SMART self-test, and "dd > /dev/null" completed
without errors on all of them. I just read about the "badblocks" tool
and I'm running it now.
-- David
^ permalink raw reply
* Re: BUG?: RAID6 reshape hung in reshape_request
From: NeilBrown @ 2015-04-27 1:20 UTC (permalink / raw)
To: David Wahler; +Cc: linux-raid
In-Reply-To: <CAGivzjGyifS3r0rypqM7n2P-fcA7NvPjJWa6kV_wfGx2biDrDg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3780 bytes --]
On Sat, 25 Apr 2015 16:35:24 -0500 David Wahler <dwahler@gmail.com> wrote:
> Hi,
>
> I'm trying to reshape a 4-disk RAID6 array by adding a fifth "missing"
> drive. Maybe that's a weird thing to do, so for context: I'm
> converting from a 3-disk RAID10, by creating a new RAID6 with the
> three new disks and then moving disks one at a time between the
> arrays. I did it this way so that I could test for problems with the
> reshape procedure before irrevocably modifying more than one of the
> original disks.
>
> (I do also have an offsite backup of the most important data, but it's
> inconvenient to access and I'm hoping not to need it.)
>
> Anyway, the reshape was going fine until about 70% completion, and
> then it got stuck. I've tried rebooting a few times: the array can be
> assembled in read-only mode, but as soon as it goes read-write and the
> reshape process continues, it gets through a few megabytes and hangs.
> At that point, any other process that tries to access the array also
> hangs uninterruptibly.
>
> Here's what shows up in dmesg:
>
> [ 721.183225] INFO: task md127_resync:1730 blocked for more than 120 seconds.
> [ 721.183978] Not tainted 4.0.0 #1
> [ 721.184751] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables this message.
> [ 721.185514] md127_resync D ffff88042ea94440 0 1730 2 0x00000000
> [ 721.185516] ffff88041a24ed20 0000000000000400 ffff88041ca82a20
> 0000000000000246
> [ 721.185518] ffff8800b8b5ffd8 ffff8800b8b5fbf0 ffff880419035a30
> 0000000000000004
> [ 721.185519] ffff8800b8b5fd1c ffff88040e91d000 ffffffff8155c73f
> ffff880419035800
> [ 721.185520] Call Trace:
> [ 721.185526] [<ffffffff8155c73f>] ? schedule+0x2f/0x80
> [ 721.185530] [<ffffffffa0888390>] ? reshape_request+0x1e0/0x8f0 [raid456]
> [ 721.185533] [<ffffffff810a86f0>] ? wait_woken+0x90/0x90
> [ 721.185535] [<ffffffffa0888dae>] ? sync_request+0x30e/0x390 [raid456]
> [ 721.185547] [<ffffffffa02cbf89>] ? is_mddev_idle+0xc9/0x130 [md_mod]
> [ 721.185550] [<ffffffffa02cf432>] ? md_do_sync+0x802/0xd30 [md_mod]
> [ 721.185555] [<ffffffff8101c356>] ? native_sched_clock+0x26/0x90
> [ 721.185558] [<ffffffffa02cbb30>] ? md_safemode_timeout+0x50/0x50 [md_mod]
> [ 721.185561] [<ffffffffa02cbc56>] ? md_thread+0x126/0x130 [md_mod]
> [ 721.185563] [<ffffffff8155c0c0>] ? __schedule+0x2a0/0x8f0
> [ 721.185565] [<ffffffffa02cbb30>] ? md_safemode_timeout+0x50/0x50 [md_mod]
> [ 721.185568] [<ffffffff81089403>] ? kthread+0xd3/0xf0
> [ 721.185570] [<ffffffff81089330>] ? kthread_create_on_node+0x180/0x180
> [ 721.185572] [<ffffffff81560598>] ? ret_from_fork+0x58/0x90
> [ 721.185574] [<ffffffff81089330>] ? kthread_create_on_node+0x180/0x180
>
> And the output of mdadm --detail/-E:
> https://gist.github.com/anonymous/0b090668b56ef54bb2f0
What is wrong with simply including this directly in the email???
Anyway:
Bad Block Log : 512 entries available at offset 72 sectors - bad blocks present.
that is the only thing that looks at all interesting. Particularly the last
3 words.
What does
mdadm --examine-badblocks /dev/sd[cde]1
show?
NeilBrown
>
> I was originally running a Debian 3.16.0 kernel, and then upgraded to
> 4.0 to see if it would help, but no such luck.
>
> Does anyone have any suggestions? Since the data on the array seems to
> be fine, hopefully there's a solution that doesn't involve re-creating
> it from scratch and restoring from backups.
>
> Thanks,
> -- David
> --
> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: PROBLEM: write to jbod with 3TB and 160GB drives hits BUG/oops
From: NeilBrown @ 2015-04-27 1:11 UTC (permalink / raw)
To: Charles Bertsch; +Cc: linux-raid, BertschC@acm.org
In-Reply-To: <553AA35A.5050300@cox.net>
[-- Attachment #1: Type: text/plain, Size: 4202 bytes --]
On Fri, 24 Apr 2015 13:11:06 -0700 Charles Bertsch <cbertsch@cox.net> wrote:
> On 04/23/2015 06:55 PM, NeilBrown wrote:
> >
> > By "jbod" I assume you mean "linear array".
> >
> > You say this happens without any filesystem on the array, yet the stack
> > traces clearly show ext2 in use.
> > Maybe some weird interaction is happening between the the filesystem and the
> > linear array.
> > But please confirm that the stack trace happened when there was no filesystem
> > on the array you were testing, and report what filesystems you do have which
> > use ext2.
> >
> Neil --
> Yes, I do mean linear array.
>
> At the point of the stack trace, there was no file-system on the linear
> 2-drive array. The test-jbod-2 script would create the array and then
> write directly to /dev/md0. Any evidence of previous existence of a
> file-system would have been obliterated by earlier runs copying
> /dev/zero everywhere.
>
> The file-systems in use --
> -- The rootfs is an initrd file, squashfs, and mounted read-only.
> -- An ext3 for configuration and logs is mounted RW on /flash
> -- An ext2 using 8MB of RAM is mounted RW on /var
> -- The file-server is derived from a much earlier design that required
> some RW directories within the root. These entries appear in the mount
> command as ext2, but are part of /var (and not separate file systems) --
> -- mount --bind /var/hd /hd
> -- mount --bind /var/home /home
>
> -- A devtmpfs mounted on /dev, tmpfs on /dev/shm, proc on /proc, sysfs
> on /sys, and another mount --bind from within /flash for nfs.
>
> # mount
> /dev/root on / type squashfs (ro,relatime)
> devtmpfs on /dev type devtmpfs
> (rw,relatime,size=1002600k,nr_inodes=250650,mode=755)
> proc on /proc type proc (rw,relatime)
> sysfs on /sys type sysfs (rw,relatime)
> /dev/ram1 on /var type ext2 (rw,relatime,errors=continue)
> /dev/ram1 on /hd type ext2 (rw,relatime,errors=continue)
> /dev/ram1 on /home type ext2 (rw,relatime,errors=continue)
> tmpfs on /dev/shm type tmpfs (rw,relatime)
> /dev/sdb1 on /flash type ext3
> (rw,noatime,errors=continue,commit=60,barrier=1,data=ordered)
> /dev/sdb1 on /var/lib/nfs type ext3
> (rw,noatime,errors=continue,commit=60,barrier=1,data=ordered)
> nfsd on /proc/fs/nfsd type nfsd (rw,relatime)
> #
Thanks for the details.
On the whole, I don't think it is likely that your problem is directly
related to md - just a coincidence that it happened when you were using md
things. But one never knows until that actual cause is found.
>
> > Is there any chance you could use "git bisect" to find out exactly which
> > commit introduced the problem? That is the mostly likely path to a
> solution.
> >
>
>
> I am not familiar with "git bisect". Would this be similar to
> downloading a series of kernel releases from linux-3.3.5 up to 3.18.5
> using a binary search to find which release (rather than which commit)
> has the problem ?
Similar, but (some of) the boring work is all done for you.
It would be best to stick to mainline kernels for testing. i.e. just '3.x',
not '3.x.y'.
So presumably 3.3 works, and 3.18 fails.
In that case:
git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux
cd linux
git bisect start
git bisect good v3.3
git bisect bad v3.18
That should get you started, except that it seems to take an incredibly long
time. So probably do the first few steps by hand.
e.g
git checkout v3.10
and test that. Then try v3.7 or v3.14.
Once you know which of those are good or bad, run e.g.
git bisect start
git bisect good v3.7
git bisect bad v3.10
and that will checkout a kernel somewhere in the middle and tell you there
are 14 (or so) steps to go.
Then build and test the kernel. If it is good, run "git bisect good".
If bad, "git bisect bad".
If you can persist through testing over a dozen kernels (takes some
patience!!) it should lead you to the commit that introduced the problem.
It is always best to be caution before declaring a kernel 'good' - run the
test a few times.
>
> Thanks
>
> Charles Bertsch
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] md/raid5: exclusive wait_for_stripe
From: NeilBrown @ 2015-04-27 0:24 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: linux-raid, linux-kernel
In-Reply-To: <1429882744-22655-2-git-send-email-yuanhan.liu@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 10501 bytes --]
On Fri, 24 Apr 2015 21:39:04 +0800 Yuanhan Liu <yuanhan.liu@linux.intel.com>
wrote:
> I noticed heavy spin lock contention at get_active_stripe() with fsmark
> multiple thread write workloads.
>
> Here is how this hot contention comes from. We have limited stripes, and
> it's a multiple thread write workload. Hence, those stripes will be taken
> soon, which puts later processes to sleep for waiting free stripes. When
> enough stripes(> 1/4 total stripes) are released, all process are woken,
> trying to get the lock. But there is one only being able to get this lock
> for each hash lock, making other processes spinning out there for acquiring
> the lock.
>
> Thus, it's effectiveless to wakeup all processes and let them battle for
> a lock that permits one to access only each time. Instead, we could make
> it be a exclusive wake up: wake up one process only. That avoids the heavy
> spin lock contention naturally.
>
> Here are some test results I have got with this patch applied(all test run
> 3 times):
>
> `fsmark.files_per_sec'
> =====================
>
> next-20150317 this patch
> ------------------------- -------------------------
> metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
> ------------------------- ------------------------- -------- ------------------------------
> 25.600 ±0.0 92.700 ±2.5 262.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> 25.600 ±0.0 77.800 ±0.6 203.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> 32.000 ±0.0 93.800 ±1.7 193.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
> 32.000 ±0.0 81.233 ±1.7 153.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
> 48.800 ±14.5 99.667 ±2.0 104.2% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
> 6.400 ±0.0 12.800 ±0.0 100.0% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
> 63.133 ±8.2 82.800 ±0.7 31.2% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
> 245.067 ±0.7 306.567 ±7.9 25.1% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-f2fs-4M-30G-fsyncBeforeClose
> 17.533 ±0.3 21.000 ±0.8 19.8% ivb44/fsmark/1x-1t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
> 188.167 ±1.9 215.033 ±3.1 14.3% ivb44/fsmark/1x-1t-4BRD_12G-RAID5-btrfs-4M-30G-NoSync
> 254.500 ±1.8 290.733 ±2.4 14.2% ivb44/fsmark/1x-1t-9BRD_6G-RAID5-btrfs-4M-30G-NoSync
>
> `time.system_time'
> =====================
>
> next-20150317 this patch
> ------------------------- -------------------------
> metric_value ±stddev metric_value ±stddev change testbox/benchmark/testcase-params
> ------------------------- ------------------------- -------- ------------------------------
> 7235.603 ±1.2 185.163 ±1.9 -97.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> 7666.883 ±2.9 202.750 ±1.0 -97.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-btrfs-4M-30G-fsyncBeforeClose
> 14567.893 ±0.7 421.230 ±0.4 -97.1% ivb44/fsmark/1x-64t-3HDD-RAID5-btrfs-4M-40G-fsyncBeforeClose
> 3697.667 ±14.0 148.190 ±1.7 -96.0% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-xfs-4M-30G-fsyncBeforeClose
> 5572.867 ±3.8 310.717 ±1.4 -94.4% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-ext4-4M-30G-fsyncBeforeClose
> 5565.050 ±0.5 313.277 ±1.5 -94.4% ivb44/fsmark/1x-64t-4BRD_12G-RAID5-ext4-4M-30G-fsyncBeforeClose
> 2420.707 ±17.1 171.043 ±2.7 -92.9% ivb44/fsmark/1x-64t-9BRD_6G-RAID5-xfs-4M-30G-fsyncBeforeClose
> 3743.300 ±4.6 379.827 ±3.5 -89.9% ivb44/fsmark/1x-64t-3HDD-RAID5-ext4-4M-40G-fsyncBeforeClose
> 3308.687 ±6.3 363.050 ±2.0 -89.0% ivb44/fsmark/1x-64t-3HDD-RAID5-xfs-4M-40G-fsyncBeforeClose
>
> Where,
>
> 1x: where 'x' means iterations or loop, corresponding to the 'L' option of fsmark
>
> 1t, 64t: where 't' means thread
>
> 4M: means the single file size, corresponding to the '-s' option of fsmark
> 40G, 30G, 120G: means the total test size
>
> 4BRD_12G: BRD is the ramdisk, where '4' means 4 ramdisk, and where '12G' means
> the size of one ramdisk. So, it would be 48G in total. And we made a
> raid on those ramdisk
>
> As you can see, though there are no much performance gain for hard disk
> workload, the system time is dropped heavily, up to 97%. And as expected,
> the performance increased a lot, up to 260%, for fast device(ram disk).
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> drivers/md/raid5.c | 46 +++++++++++++++++++++++++++++++++++-----------
> drivers/md/raid5.h | 2 +-
> 2 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b7e385f..2d8fcc1 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -344,7 +344,8 @@ static void release_inactive_stripe_list(struct r5conf *conf,
> int hash)
> {
> int size;
> - bool do_wakeup = false;
> + bool do_wakeup[NR_STRIPE_HASH_LOCKS] = { false, };
I think I'd rather use an 'unsigned long' and set bits.
> + int i = 0;
> unsigned long flags;
>
> if (hash == NR_STRIPE_HASH_LOCKS) {
> @@ -365,17 +366,22 @@ static void release_inactive_stripe_list(struct r5conf *conf,
> !list_empty(list))
> atomic_dec(&conf->empty_inactive_list_nr);
> list_splice_tail_init(list, conf->inactive_list + hash);
> - do_wakeup = true;
> + do_wakeup[size - 1] = true;
... so this becomes
do_wakeup |= 1 << (size - 1);
> spin_unlock_irqrestore(conf->hash_locks + hash, flags);
> }
> size--;
> hash--;
> }
>
> - if (do_wakeup) {
> - wake_up(&conf->wait_for_stripe);
> - if (conf->retry_read_aligned)
> - md_wakeup_thread(conf->mddev->thread);
> + for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
> + bool waked_thread = false;
> + if (do_wakeup[i]) {
> + wake_up(&conf->wait_for_stripe[i]);
> + if (!waked_thread) {
> + waked_thread = true;
> + md_wakeup_thread(conf->mddev->thread);
> + }
> + }
I don't think you want waked_thread to be local to this loop.
As it is, the "if (!waked_thread)" test *always* succeeds.
You can discard it if do_wakeup becomes and unsigned long, and just do
if (do_wakeup && conf->retry_read_aligned)
md_wakeup_thread(conf->mddev->thread);
And why have you removed the test on conf->retry_read_aligned??
> }
> }
>
> @@ -655,6 +661,18 @@ static int has_failed(struct r5conf *conf)
> return 0;
> }
>
> +/* XXX: might put it to linux/wait.h to be a public API? */
Yes, definitely put it in linux/wait.h
Thanks,
NeilBrown
> +#define raid_wait_event_exclusive_cmd(wq, condition, cmd1, cmd2) \
> +do { \
> + if (condition) \
> + break; \
> + (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 1, 0, \
> + cmd1; \
> + schedule(); \
> + cmd2); \
> +} while (0)
> +
> +
> static struct stripe_head *
> get_active_stripe(struct r5conf *conf, sector_t sector,
> int previous, int noblock, int noquiesce)
> @@ -684,14 +702,15 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> if (!sh) {
> set_bit(R5_INACTIVE_BLOCKED,
> &conf->cache_state);
> - wait_event_lock_irq(
> - conf->wait_for_stripe,
> + raid_wait_event_exclusive_cmd(
> + conf->wait_for_stripe[hash],
> !list_empty(conf->inactive_list + hash) &&
> (atomic_read(&conf->active_stripes)
> < (conf->max_nr_stripes * 3 / 4)
> || !test_bit(R5_INACTIVE_BLOCKED,
> &conf->cache_state)),
> - *(conf->hash_locks + hash));
> + spin_unlock_irq(conf->hash_locks + hash),
> + spin_lock_irq(conf->hash_locks + hash));
> clear_bit(R5_INACTIVE_BLOCKED,
> &conf->cache_state);
> } else {
> @@ -716,6 +735,9 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> }
> } while (sh == NULL);
>
> + if (!list_empty(conf->inactive_list + hash))
> + wake_up(&conf->wait_for_stripe[hash]);
> +
> spin_unlock_irq(conf->hash_locks + hash);
> return sh;
> }
> @@ -2136,7 +2158,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
> cnt = 0;
> list_for_each_entry(nsh, &newstripes, lru) {
> lock_device_hash_lock(conf, hash);
> - wait_event_cmd(conf->wait_for_stripe,
> + raid_wait_event_exclusive_cmd(conf->wait_for_stripe[hash],
> !list_empty(conf->inactive_list + hash),
> unlock_device_hash_lock(conf, hash),
> lock_device_hash_lock(conf, hash));
> @@ -6391,7 +6413,9 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> spin_lock_init(&conf->device_lock);
> seqcount_init(&conf->gen_lock);
> init_waitqueue_head(&conf->wait_for_quiesce);
> - init_waitqueue_head(&conf->wait_for_stripe);
> + for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++) {
> + init_waitqueue_head(&conf->wait_for_stripe[i]);
> + }
> init_waitqueue_head(&conf->wait_for_overlap);
> INIT_LIST_HEAD(&conf->handle_list);
> INIT_LIST_HEAD(&conf->hold_list);
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index fab53a3..cdad2d2 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -509,7 +509,7 @@ struct r5conf {
> atomic_t empty_inactive_list_nr;
> struct llist_head released_stripes;
> wait_queue_head_t wait_for_quiesce;
> - wait_queue_head_t wait_for_stripe;
> + wait_queue_head_t wait_for_stripe[NR_STRIPE_HASH_LOCKS];
> wait_queue_head_t wait_for_overlap;
> unsigned long cache_state;
> #define R5_INACTIVE_BLOCKED 1 /* release of inactive stripes blocked,
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] md/raid5: split wait_for_stripe and introduce wait_for_quiesce
From: NeilBrown @ 2015-04-27 0:10 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: linux-raid, linux-kernel
In-Reply-To: <1429882744-22655-1-git-send-email-yuanhan.liu@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 5586 bytes --]
On Fri, 24 Apr 2015 21:39:03 +0800 Yuanhan Liu <yuanhan.liu@linux.intel.com>
wrote:
> If I read code correctly, current wait_for_stripe actually has 2 usage:
>
> - wait for there is enough free stripe cache, triggered when
> get_free_stripe() failed. This is what wait_for_stripe intend
> for literally.
>
> - wait for quiesce == 0 or
> active_aligned_reads == 0 && active_stripes == 0
>
> It has nothing to do with wait_for_stripe literally, and releasing
> an active stripe won't actually wake them up. On the contrary, wake_up
> from under this case won't actually wake up the process waiting for
> an free stripe being available.
I disagree. Releasing an active stripe *will* (or *can*) wake up that third
case, as it decrements "active_stripes" which will eventually reach zero.
I don't think your new code will properly wake up a process which is waiting
for "active_stripes == 0".
>
> Hence, we'd better split wait_for_stripe, and here I introduce
> wait_for_quiesce for the second usage. The name may not well taken, or
> even taken wrongly. Feel free to correct me then.
>
> This is also a prepare patch for next patch: make wait_for_stripe
> exclusive.
I think you have this commit description upside down :-)
The real motivation is that you are seeing contention on some spinlock and so
you want to split 'wait_for_stripe' up in to multiple wait_queues so that you
can use exclusive wakeup. As this is the main motivation, it should be
stated first.
Then explain that 'wait_for_stripe' is used to wait for the array to enter or
leave the quiescent state, and also to wait for an available stripe in each
of the hash lists.
So this patch splits the first usage off into a separate wait_queue, and the
next patch will split the second usage into one waitqueue for each hash value.
Then explain just is what is needed for that first step.
When you put it that way around, the patch makes lots of sense.
So: could you please resubmit with the description the right way around, and
with an appropriate wakeup call to ensure raid5_quiesce is woken up when
active_stripes reaches zero?
Thanks,
NeilBrown
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> drivers/md/raid5.c | 13 +++++++------
> drivers/md/raid5.h | 1 +
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 9716319..b7e385f 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -667,7 +667,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
> spin_lock_irq(conf->hash_locks + hash);
>
> do {
> - wait_event_lock_irq(conf->wait_for_stripe,
> + wait_event_lock_irq(conf->wait_for_quiesce,
> conf->quiesce == 0 || noquiesce,
> *(conf->hash_locks + hash));
> sh = __find_stripe(conf, sector, conf->generation - previous);
> @@ -4725,7 +4725,7 @@ static void raid5_align_endio(struct bio *bi, int error)
> raid_bi, 0);
> bio_endio(raid_bi, 0);
> if (atomic_dec_and_test(&conf->active_aligned_reads))
> - wake_up(&conf->wait_for_stripe);
> + wake_up(&conf->wait_for_quiesce);
> return;
> }
>
> @@ -4820,7 +4820,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
> align_bi->bi_iter.bi_sector += rdev->data_offset;
>
> spin_lock_irq(&conf->device_lock);
> - wait_event_lock_irq(conf->wait_for_stripe,
> + wait_event_lock_irq(conf->wait_for_quiesce,
> conf->quiesce == 0,
> conf->device_lock);
> atomic_inc(&conf->active_aligned_reads);
> @@ -5659,7 +5659,7 @@ static int retry_aligned_read(struct r5conf *conf, struct bio *raid_bio)
> bio_endio(raid_bio, 0);
> }
> if (atomic_dec_and_test(&conf->active_aligned_reads))
> - wake_up(&conf->wait_for_stripe);
> + wake_up(&conf->wait_for_quiesce);
> return handled;
> }
>
> @@ -6390,6 +6390,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
> goto abort;
> spin_lock_init(&conf->device_lock);
> seqcount_init(&conf->gen_lock);
> + init_waitqueue_head(&conf->wait_for_quiesce);
> init_waitqueue_head(&conf->wait_for_stripe);
> init_waitqueue_head(&conf->wait_for_overlap);
> INIT_LIST_HEAD(&conf->handle_list);
> @@ -7413,7 +7414,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> * active stripes can drain
> */
> conf->quiesce = 2;
> - wait_event_cmd(conf->wait_for_stripe,
> + wait_event_cmd(conf->wait_for_quiesce,
> atomic_read(&conf->active_stripes) == 0 &&
> atomic_read(&conf->active_aligned_reads) == 0,
> unlock_all_device_hash_locks_irq(conf),
> @@ -7427,7 +7428,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
> case 0: /* re-enable writes */
> lock_all_device_hash_locks_irq(conf);
> conf->quiesce = 0;
> - wake_up(&conf->wait_for_stripe);
> + wake_up(&conf->wait_for_quiesce);
> wake_up(&conf->wait_for_overlap);
> unlock_all_device_hash_locks_irq(conf);
> break;
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 7dc0dd8..fab53a3 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -508,6 +508,7 @@ struct r5conf {
> struct list_head inactive_list[NR_STRIPE_HASH_LOCKS];
> atomic_t empty_inactive_list_nr;
> struct llist_head released_stripes;
> + wait_queue_head_t wait_for_quiesce;
> wait_queue_head_t wait_for_stripe;
> wait_queue_head_t wait_for_overlap;
> unsigned long cache_state;
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
From: NeilBrown @ 2015-04-26 23:49 UTC (permalink / raw)
To: Ming Lin
Cc: Christoph Hellwig, Jens Axboe, linux-raid, Kent Overstreet,
Dongsu Park
In-Reply-To: <1429915930-22449-1-git-send-email-mlin@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]
On Fri, 24 Apr 2015 15:52:10 -0700 Ming Lin <mlin@kernel.org> wrote:
> From: Kent Overstreet <kent.overstreet@gmail.com>
>
> Refactor sync_request_write() of md/raid10 to use bio_copy_data()
> instead of open coding bio_vec iterations.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> [dpark: add more description in commit message]
> Signed-off-by: Dongsu Park <dongsu.park@profitbricks.com>
> Signed-off-by: Ming Lin <mlin@kernel.org>
> ---
> drivers/md/raid10.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index a7196c4..02e33f1 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2097,18 +2097,11 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
> tbio->bi_vcnt = vcnt;
> tbio->bi_iter.bi_size = r10_bio->sectors << 9;
> tbio->bi_rw = WRITE;
> - tbio->bi_private = r10_bio;
> tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
> -
> - for (j=0; j < vcnt ; j++) {
> - tbio->bi_io_vec[j].bv_offset = 0;
> - tbio->bi_io_vec[j].bv_len = PAGE_SIZE;
> -
> - memcpy(page_address(tbio->bi_io_vec[j].bv_page),
> - page_address(fbio->bi_io_vec[j].bv_page),
> - PAGE_SIZE);
> - }
You removed the resetting of bv_offset and bv_len.
So I assume this is being applied in a context where these things are now
immutable - is that correct?
> tbio->bi_end_io = end_sync_write;
> + tbio->bi_private = r10_bio;
Any reason you are moving this assignment to bi_private?
It doesn't hurt, but it doesn't seem to be necessary.
> +
> + bio_copy_data(tbio, fbio);
>
> d = r10_bio->devs[i].devnum;
> atomic_inc(&conf->mirrors[d].rdev->nr_pending);
> @@ -2124,17 +2117,14 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
> * that are active
> */
> for (i = 0; i < conf->copies; i++) {
> - int j, d;
> + int d;
>
> tbio = r10_bio->devs[i].repl_bio;
> if (!tbio || !tbio->bi_end_io)
> continue;
> if (r10_bio->devs[i].bio->bi_end_io != end_sync_write
> && r10_bio->devs[i].bio != fbio)
> - for (j = 0; j < vcnt; j++)
> - memcpy(page_address(tbio->bi_io_vec[j].bv_page),
> - page_address(fbio->bi_io_vec[j].bv_page),
> - PAGE_SIZE);
> + bio_copy_data(tbio, fbio);
> d = r10_bio->devs[i].devnum;
> atomic_inc(&r10_bio->remaining);
> md_sync_acct(conf->mirrors[d].replacement->bdev,
Providing you are confident that bv_offset and bv_len don't need to be
updated:
Acked-by: NeilBrown <neilb@suse.de>
though I'd prefer the bi_private assignment was left where it was.
Thanks,
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: Massive RAID-1 desync
From: Jean-Baptiste Thomas @ 2015-04-26 8:48 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <20150425172527.21a34428@notabene.brown>
On 2015-04-25 17:25 +1000, NeilBrown wrote:
> Perfectly normal. Metadata is at the end, at least 64K from the end
> and 64K aligned.
Yes. Format 0.90.
> And what were those messages about sda?
The actual messages have been displaced by lockd's rambling but as I
remember, it was this sort of thing :
ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x0
ata1.00: BMDMA stat 0x4
ata1.00: failed command: READ DMA EXT
ata1.00: cmd 25/00:80:a9:54:70/00:00:74:00:00/e0 tag 0 dma 65536 in
res 51/40:00:25:55:70/40:00:74:00:00/e0 Emask 0x9 (media error)
ata1.00: status: { DRDY ERR }
ata1.00: error: { UNC }
ata1.00: configured for UDMA/133
ata1: EH complete
I ran e2fsck on copies of sda1 and sdc1. They are both heavily damaged,
not just sdc1.
Looks like I'm going to have to replace a disk and see. I'd like to
avoid replacing two, though. Or going through more crashes. Does
MD have a paranoid mode in which reading a sector from a RAID-1
device would not return successfully until it got matching data
from at least two components ?
^ permalink raw reply
* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
From: Ming Lin @ 2015-04-26 5:42 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Neil Brown, Jens Axboe, linux-raid, Kent Overstreet, Dongsu Park
In-Reply-To: <20150425094726.GB14363@infradead.org>
On Sat, Apr 25, 2015 at 2:47 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Apr 24, 2015 at 03:52:10PM -0700, Ming Lin wrote:
>> From: Kent Overstreet <kent.overstreet@gmail.com>
>>
>> Refactor sync_request_write() of md/raid10 to use bio_copy_data()
>> instead of open coding bio_vec iterations.
>
> Do you plan to submit an updated version of the immutable biovecs set
> soon? In that case it would be good to merge this patch through
> the block tree, as it's a requirement for it AFAICS. Might be worth to
> repost them together.
I have put the immutable biovecs set here:
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-req
Will add this patch also.
I'll submit the patch set soon after running some tests.
^ permalink raw reply
* BUG?: RAID6 reshape hung in reshape_request
From: David Wahler @ 2015-04-25 21:35 UTC (permalink / raw)
To: linux-raid
Hi,
I'm trying to reshape a 4-disk RAID6 array by adding a fifth "missing"
drive. Maybe that's a weird thing to do, so for context: I'm
converting from a 3-disk RAID10, by creating a new RAID6 with the
three new disks and then moving disks one at a time between the
arrays. I did it this way so that I could test for problems with the
reshape procedure before irrevocably modifying more than one of the
original disks.
(I do also have an offsite backup of the most important data, but it's
inconvenient to access and I'm hoping not to need it.)
Anyway, the reshape was going fine until about 70% completion, and
then it got stuck. I've tried rebooting a few times: the array can be
assembled in read-only mode, but as soon as it goes read-write and the
reshape process continues, it gets through a few megabytes and hangs.
At that point, any other process that tries to access the array also
hangs uninterruptibly.
Here's what shows up in dmesg:
[ 721.183225] INFO: task md127_resync:1730 blocked for more than 120 seconds.
[ 721.183978] Not tainted 4.0.0 #1
[ 721.184751] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.
[ 721.185514] md127_resync D ffff88042ea94440 0 1730 2 0x00000000
[ 721.185516] ffff88041a24ed20 0000000000000400 ffff88041ca82a20
0000000000000246
[ 721.185518] ffff8800b8b5ffd8 ffff8800b8b5fbf0 ffff880419035a30
0000000000000004
[ 721.185519] ffff8800b8b5fd1c ffff88040e91d000 ffffffff8155c73f
ffff880419035800
[ 721.185520] Call Trace:
[ 721.185526] [<ffffffff8155c73f>] ? schedule+0x2f/0x80
[ 721.185530] [<ffffffffa0888390>] ? reshape_request+0x1e0/0x8f0 [raid456]
[ 721.185533] [<ffffffff810a86f0>] ? wait_woken+0x90/0x90
[ 721.185535] [<ffffffffa0888dae>] ? sync_request+0x30e/0x390 [raid456]
[ 721.185547] [<ffffffffa02cbf89>] ? is_mddev_idle+0xc9/0x130 [md_mod]
[ 721.185550] [<ffffffffa02cf432>] ? md_do_sync+0x802/0xd30 [md_mod]
[ 721.185555] [<ffffffff8101c356>] ? native_sched_clock+0x26/0x90
[ 721.185558] [<ffffffffa02cbb30>] ? md_safemode_timeout+0x50/0x50 [md_mod]
[ 721.185561] [<ffffffffa02cbc56>] ? md_thread+0x126/0x130 [md_mod]
[ 721.185563] [<ffffffff8155c0c0>] ? __schedule+0x2a0/0x8f0
[ 721.185565] [<ffffffffa02cbb30>] ? md_safemode_timeout+0x50/0x50 [md_mod]
[ 721.185568] [<ffffffff81089403>] ? kthread+0xd3/0xf0
[ 721.185570] [<ffffffff81089330>] ? kthread_create_on_node+0x180/0x180
[ 721.185572] [<ffffffff81560598>] ? ret_from_fork+0x58/0x90
[ 721.185574] [<ffffffff81089330>] ? kthread_create_on_node+0x180/0x180
And the output of mdadm --detail/-E:
https://gist.github.com/anonymous/0b090668b56ef54bb2f0
I was originally running a Debian 3.16.0 kernel, and then upgraded to
4.0 to see if it would help, but no such luck.
Does anyone have any suggestions? Since the data on the array seems to
be fine, hopefully there's a solution that doesn't involve re-creating
it from scratch and restoring from backups.
Thanks,
-- David
^ permalink raw reply
* Re: [PATCH] md/raid10: make sync_request_write() call bio_copy_data()
From: Christoph Hellwig @ 2015-04-25 9:47 UTC (permalink / raw)
To: Ming Lin; +Cc: Neil Brown, Jens Axboe, linux-raid, Kent Overstreet, Dongsu Park
In-Reply-To: <1429915930-22449-1-git-send-email-mlin@kernel.org>
On Fri, Apr 24, 2015 at 03:52:10PM -0700, Ming Lin wrote:
> From: Kent Overstreet <kent.overstreet@gmail.com>
>
> Refactor sync_request_write() of md/raid10 to use bio_copy_data()
> instead of open coding bio_vec iterations.
Do you plan to submit an updated version of the immutable biovecs set
soon? In that case it would be good to merge this patch through
the block tree, as it's a requirement for it AFAICS. Might be worth to
repost them together.
^ permalink raw reply
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