* Re: [RFC v4 2/4] crypto: Introduce CRYPTO_ALG_BULK flag
From: Baolin Wang @ 2016-06-08 2:00 UTC (permalink / raw)
To: Herbert Xu
Cc: Jens Axboe, Alasdair G Kergon, Mike Snitzer,
open list:DEVICE-MAPPER (LVM), David Miller, Eric Biggers,
Joonsoo Kim, tadeusz.struk, smueller, Masanari Iida, Shaohua Li,
Dan Williams, Martin K. Petersen, Sagi Grimberg, Kent Overstreet,
Keith Busch, Tejun Heo, Ming Lei, Mark Brown, Arnd Bergmann,
linux-crypto, linux-block,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, LKML
In-Reply-To: <20160607141613.GA2237@gondor.apana.org.au>
Hi Herbert,
On 7 June 2016 at 22:16, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jun 07, 2016 at 08:17:05PM +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>
>
> Nack. As I said before, please do it using explicit IV generators
> like we do for IPsec.
OK. I would like to try your suggestion. Thanks.
> --
> 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
--
Baolin.wang
Best Regards
^ permalink raw reply
* Re: Raid check didn't fix Current_Pending_Sector, but badblocks -nsv did
From: Phil Turmel @ 2016-06-08 12:24 UTC (permalink / raw)
To: Brad Campbell, Marc MERLIN; +Cc: Sarah Newman, linux-raid@vger.kernel.org
In-Reply-To: <2c7e843f-58a9-a9f0-76bf-ff193935d13b@fnarfbargle.com>
On 06/07/2016 09:39 PM, Brad Campbell wrote:
> On 07/06/16 21:04, Phil Turmel wrote:
>> On 06/07/2016 12:51 AM, Marc MERLIN wrote:
>
>>> Right, I understand now, good to know.
>>> So I'll use badblocks next time I have this issue.
>>
>> Or just ignore them. You aren't using them, so they can't hurt you.
>
> That's actually not necessarily true.
>
> If you have a dud sector early on the disk (so before the start of the
> RAID data) you will terminate every SMART long test in the first couple
> of meg of the disk. So while a dud down there won't necessarily impact
> your usage from a RAID perspective, it'll knacker your ability to
> regularly check the disks in their entirety. SMART tests abort on the
> first bad read.
Don't bother doing long self-tests on drives participating in an array
-- check scrubs do everything a long self-test does on the area of
interest, plus actually fixing UREs that are found. And check scrubs
don't abort on a read failure.
My advice stands: ignore the UREs in unused areas of the disk.
> It's ugly, but in the single instance I had that happen, I removed the
> drive from the array, wrote zero to the entire disk and then added it
> back. That forced a reallocation in the affected area.
Completely pointless exercise that opened a window of higher-risk of
failure of your array. Unless you used --replace with another spare to
maintain redundancy on your array while that disk was out.
> Usually if it is in the RAID zone, a check scrub will clear it up.
> Having said that I've had a very peculiar one here in the last couple of
> days.
>
> A WD 2TB Green drive with TLER set to 7 seconds. The first read would
> error out in 7 seconds (as it should), but a second read succeeded.
> After returning the error, the drive must have kept trying to recover in
> the background and eventually succeeded and cached the result. So
> subsequent reads were ok. After reading and writing enough to other
> parts of the drive to flush the drives cache, the process would repeat.
Pure speculation. Unless you can show better evidence that those drives
will cache a read in that case, I would say it was just a mild enough
weak spot that it randomly succeeded more than not. And if you follow
my advice, it doesn't matter: if the array is the only process reading
from the disk, the first appearance of the URE would be the last, as the
array would re-write it immediately. Whether during a scrub or due to
normal access.
Regular long self-tests are highly recommended for stand-alone disks and
for array hot spares.
Phil
^ permalink raw reply
* [PATCH] md: use a mutex to protect a global list
From: Cong Wang @ 2016-06-08 16:20 UTC (permalink / raw)
To: linux-raid; +Cc: linux-kernel, Cong Wang, Shaohua Li
We saw a list corruption in the list all_detected_devices:
WARNING: CPU: 16 PID: 226 at lib/list_debug.c:29 __list_add+0x3c/0xa9()
list_add corruption. next->prev should be prev (ffff880859d58320), but was ffff880859ce74c0. (next=ffffffff81abfdb0).
Modules linked in: ahci libahci libata sd_mod scsi_mod
CPU: 16 PID: 226 Comm: kworker/u241:4 Not tainted 4.1.20 #1
Hardware name: Dell Inc. PowerEdge C6220/04GD66, BIOS 2.2.3 11/07/2013
Workqueue: events_unbound async_run_entry_fn
0000000000000000 ffff880859a5baf8 ffffffff81502872 ffff880859a5bb48
0000000000000009 ffff880859a5bb38 ffffffff810692a5 ffff880859ee8828
ffffffff812ad02c ffff880859d58320 ffffffff81abfdb0 ffff880859eb90c0
Call Trace:
[<ffffffff81502872>] dump_stack+0x4d/0x63
[<ffffffff810692a5>] warn_slowpath_common+0xa1/0xbb
[<ffffffff812ad02c>] ? __list_add+0x3c/0xa9
[<ffffffff81069305>] warn_slowpath_fmt+0x46/0x48
[<ffffffff812ad02c>] __list_add+0x3c/0xa9
[<ffffffff81406f28>] md_autodetect_dev+0x41/0x62
[<ffffffff81285862>] rescan_partitions+0x25f/0x29d
[<ffffffff81506372>] ? mutex_lock+0x13/0x31
[<ffffffff811a090f>] __blkdev_get+0x1aa/0x3cd
[<ffffffff811a0b91>] blkdev_get+0x5f/0x294
[<ffffffff81377ceb>] ? put_device+0x17/0x19
[<ffffffff8128227c>] ? disk_put_part+0x12/0x14
[<ffffffff812836f3>] add_disk+0x29d/0x407
[<ffffffff81384345>] ? __pm_runtime_use_autosuspend+0x5c/0x64
[<ffffffffa004a724>] sd_probe_async+0x115/0x1af [sd_mod]
[<ffffffff81083177>] async_run_entry_fn+0x72/0x12c
[<ffffffff8107c44c>] process_one_work+0x198/0x2ce
[<ffffffff8107cac7>] worker_thread+0x1dd/0x2bb
[<ffffffff8107c8ea>] ? cancel_delayed_work_sync+0x15/0x15
[<ffffffff8107c8ea>] ? cancel_delayed_work_sync+0x15/0x15
[<ffffffff81080d9c>] kthread+0xae/0xb6
[<ffffffff81080000>] ? param_array_set+0x40/0xfa
[<ffffffff81080cee>] ? __kthread_parkme+0x61/0x61
[<ffffffff81508152>] ret_from_fork+0x42/0x70
[<ffffffff81080cee>] ? __kthread_parkme+0x61/0x61
I suspect it is because there is no lock protecting this
global list, autostart_arrays() is called in ioctl() path
where there is no lock.
Cc: Shaohua Li <shli@kernel.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
drivers/md/md.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 866825f..f2f1912 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8799,6 +8799,7 @@ EXPORT_SYMBOL(md_reload_sb);
* at boot time.
*/
+static DEFINE_MUTEX(detected_devices_mutex);
static LIST_HEAD(all_detected_devices);
struct detected_devices_node {
struct list_head list;
@@ -8812,7 +8813,9 @@ void md_autodetect_dev(dev_t dev)
node_detected_dev = kzalloc(sizeof(*node_detected_dev), GFP_KERNEL);
if (node_detected_dev) {
node_detected_dev->dev = dev;
+ mutex_lock(&detected_devices_mutex);
list_add_tail(&node_detected_dev->list, &all_detected_devices);
+ mutex_unlock(&detected_devices_mutex);
} else {
printk(KERN_CRIT "md: md_autodetect_dev: kzalloc failed"
", skipping dev(%d,%d)\n", MAJOR(dev), MINOR(dev));
@@ -8831,6 +8834,7 @@ static void autostart_arrays(int part)
printk(KERN_INFO "md: Autodetecting RAID arrays.\n");
+ mutex_lock(&detected_devices_mutex);
while (!list_empty(&all_detected_devices) && i_scanned < INT_MAX) {
i_scanned++;
node_detected_dev = list_entry(all_detected_devices.next,
@@ -8849,6 +8853,7 @@ static void autostart_arrays(int part)
list_add(&rdev->same_set, &pending_raid_disks);
i_passed++;
}
+ mutex_unlock(&detected_devices_mutex);
printk(KERN_INFO "md: Scanned %d and added %d devices.\n",
i_scanned, i_passed);
--
2.1.0
^ permalink raw reply related
* Re: mdadm 3.4-1, error msg. on boot - no /usr/lib/systemd/scripts/mdadm_env.sh
From: David C. Rankin @ 2016-06-08 22:06 UTC (permalink / raw)
To: mdraid
In-Reply-To: <wrfjziqwyex7.fsf@redhat.com>
On 06/07/2016 02:31 PM, Jes Sorensen wrote:
> "David C. Rankin" <drankinatty@suddenlinkmail.com> writes:
>> Neil, All,
>>
>> I'm chasing down the reason there is no
>> /usr/lib/systemd/scripts/mdadm_env.sh provided with mdadm. This is
>> more a curiosity really as mdmonitor continues to run, but it always
>> throws an error at boot:
>>
>> Jun 05 17:12:47 valhalla systemd[454]: mdmonitor.service: Failed at step EXEC
>> spawning /usr/lib/systemd/scripts/mdadm_env.sh: No such file or directory
>>
>> I have several Arch servers, all on mdadm 3.4-1. I ran into an issue
>> a year or so ago with a boot error with mdmonitor due to my failure to
>> provide MAILADDR in /etc/mdadm.com, but that has long since been
>> corrected:
>
> If you're running Arch Linux, you probably want to direct this question
> to the Arch Linux maintainers.
>
> Jes
>
Thank you Jes,
I've posed the problem back to the Arch devs.
--
David C. Rankin, J.D.,P.E.
^ permalink raw reply
* Multithread raid5: Is check-for-completions also multithreaded (using async_xor) ?
From: Aggarwal, Vikas @ 2016-06-09 10:55 UTC (permalink / raw)
To: linux-raid@vger.kernel.org
Hello All,
With multithread raid5 enabled (echo 48 > /sys/block/md51/md/group_thread_cnt) what I understand is that all the cpu cores will be able to submit async_xor instructions in parallel for a single md raid5 device.
Question: About polling for completion, is that also multi-threaded ? - Does the same number of worker threads (48 in my test) will continue poll for XOR completion as well ?
My multi-core SoC has XOR/GF offload channel, and I am trying to see how to get the best throughput for raid5 rebuild using multithread functionality.
Regards
Vikas Aggarwal
^ permalink raw reply
* RE: restarting raid arrays with uuid
From: Bryan Hepworth @ 2016-06-09 12:23 UTC (permalink / raw)
To: linux-raid@vger.kernel.org
In-Reply-To: <DB5PR07MB1287703757699A1962149D82D55C0@DB5PR07MB1287.eurprd07.prod.outlook.com>
>-----Original Message-----
>From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
>owner@vger.kernel.org] On Behalf Of Bryan Hepworth
>Sent: 06 June 2016 19:55
>To: linux-raid@vger.kernel.org
>Subject: restarting raid arrays with uuid
>
>Hi All
>
>I'm not entirely sure this is rescuable. This is an old system that was kept on
>for scratch storage space, unfortunately it was discovered some data hadn't
>moved out of this area. The arrays disappeared after taking them offline and
>rebooting the machine.
>
>I've checked /etc/mdadm.conf
>And mdadm --examine across all of the disks
>I've assembled from uuid
>Ran /proc/mdstat
>
>Any suggestions as to what to try next?
>
After carefully looking at the arrays I have two back as they should be.
I've also discovered that one of the drives with gpt has a partition in it with ext4 data. Gparted has given me the device uuid which I can see in /dev/disk/by-uuid
Unfortunately it says no superblock information when I run mdadm --examine /dev/disk/by-uuid/<uuid number>
gparted does say ext4 is clean and it does have a --raid tag so my next question is is it safe to mount this drive, or is it best to dd it elsewhere to see what's on it?
Thanks
Bryan
^ permalink raw reply
* Low RAID10 performance during resync
From: Tomasz Majchrzak @ 2016-06-09 13:45 UTC (permalink / raw)
To: linux-raid
A low performance of mkfs has been observed on RAID10 array during resync. It is not so significant for NVMe drives but for my setup of RAID10 consisting of 4 SATA drives format time has increased by 200%.
I have looked into the problem and I have found out it is caused by this changeset:
commit 09314799e4f0589e52bafcd0ca3556c60468bc0e
md: remove 'go_faster' option from ->sync_request()
It seemed the code had been redundant and could be safely removed due to barriers mechanism but it proved otherwise. The barriers don't provide enough throttle to resync IOs. They only assure non-resync IOs and resync IOs are not being executed at the same time. In result resync IOs take around 25% of CPU time, mostly because there are many of them but only one at a time so a lot of CPU time is simply wasted waiting for a single IO to complete.
The removed sleep call in resync IO had allowed a lot of non-resync activity to be scheduled (nobody waiting for a barrier). Once sleep call had ended, resync IO had to wait longer to raise a barrier as all non-resync activity had to be completed first. It had nicely throttled a number of resync IOs in favour of non-resync activity. Since we lack it now, the performance has dropped badly.
I would like to revert the changeset. We don't have to put a resync IO to sleep for a second though. I have done some testing and it seems even a delay of 100ms is sufficient. It slows down resync IOs to the same extent as sleep for a second - the sleep call ends sooner but the barrier cannot be raised until non-resync IOs complete.
Is this solution ok?
Regards,
Tomek
^ permalink raw reply
* Re: [PATCH] md: use a mutex to protect a global list
From: Shaohua Li @ 2016-06-09 16:35 UTC (permalink / raw)
To: Cong Wang; +Cc: linux-raid, linux-kernel
In-Reply-To: <1465402816-10882-1-git-send-email-xiyou.wangcong@gmail.com>
On Wed, Jun 08, 2016 at 09:20:16AM -0700, Cong Wang wrote:
> We saw a list corruption in the list all_detected_devices:
>
> WARNING: CPU: 16 PID: 226 at lib/list_debug.c:29 __list_add+0x3c/0xa9()
> list_add corruption. next->prev should be prev (ffff880859d58320), but was ffff880859ce74c0. (next=ffffffff81abfdb0).
> Modules linked in: ahci libahci libata sd_mod scsi_mod
> CPU: 16 PID: 226 Comm: kworker/u241:4 Not tainted 4.1.20 #1
> Hardware name: Dell Inc. PowerEdge C6220/04GD66, BIOS 2.2.3 11/07/2013
> Workqueue: events_unbound async_run_entry_fn
> 0000000000000000 ffff880859a5baf8 ffffffff81502872 ffff880859a5bb48
> 0000000000000009 ffff880859a5bb38 ffffffff810692a5 ffff880859ee8828
> ffffffff812ad02c ffff880859d58320 ffffffff81abfdb0 ffff880859eb90c0
> Call Trace:
> [<ffffffff81502872>] dump_stack+0x4d/0x63
> [<ffffffff810692a5>] warn_slowpath_common+0xa1/0xbb
> [<ffffffff812ad02c>] ? __list_add+0x3c/0xa9
> [<ffffffff81069305>] warn_slowpath_fmt+0x46/0x48
> [<ffffffff812ad02c>] __list_add+0x3c/0xa9
> [<ffffffff81406f28>] md_autodetect_dev+0x41/0x62
> [<ffffffff81285862>] rescan_partitions+0x25f/0x29d
> [<ffffffff81506372>] ? mutex_lock+0x13/0x31
> [<ffffffff811a090f>] __blkdev_get+0x1aa/0x3cd
> [<ffffffff811a0b91>] blkdev_get+0x5f/0x294
> [<ffffffff81377ceb>] ? put_device+0x17/0x19
> [<ffffffff8128227c>] ? disk_put_part+0x12/0x14
> [<ffffffff812836f3>] add_disk+0x29d/0x407
> [<ffffffff81384345>] ? __pm_runtime_use_autosuspend+0x5c/0x64
> [<ffffffffa004a724>] sd_probe_async+0x115/0x1af [sd_mod]
> [<ffffffff81083177>] async_run_entry_fn+0x72/0x12c
> [<ffffffff8107c44c>] process_one_work+0x198/0x2ce
> [<ffffffff8107cac7>] worker_thread+0x1dd/0x2bb
> [<ffffffff8107c8ea>] ? cancel_delayed_work_sync+0x15/0x15
> [<ffffffff8107c8ea>] ? cancel_delayed_work_sync+0x15/0x15
> [<ffffffff81080d9c>] kthread+0xae/0xb6
> [<ffffffff81080000>] ? param_array_set+0x40/0xfa
> [<ffffffff81080cee>] ? __kthread_parkme+0x61/0x61
> [<ffffffff81508152>] ret_from_fork+0x42/0x70
> [<ffffffff81080cee>] ? __kthread_parkme+0x61/0x61
>
> I suspect it is because there is no lock protecting this
> global list, autostart_arrays() is called in ioctl() path
> where there is no lock.
>
> Cc: Shaohua Li <shli@kernel.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thanks! This probably is because deiver can do async probe now.
^ permalink raw reply
* Re: Low RAID10 performance during resync
From: Shaohua Li @ 2016-06-09 17:31 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: linux-raid, neilb
In-Reply-To: <20160609134555.GA9104@proton.igk.intel.com>
On Thu, Jun 09, 2016 at 03:45:55PM +0200, Tomasz Majchrzak wrote:
> A low performance of mkfs has been observed on RAID10 array during resync. It
> is not so significant for NVMe drives but for my setup of RAID10 consisting
> of 4 SATA drives format time has increased by 200%.
>
> I have looked into the problem and I have found out it is caused by this
> changeset:
>
> commit 09314799e4f0589e52bafcd0ca3556c60468bc0e md: remove 'go_faster' option
> from ->sync_request()
>
> It seemed the code had been redundant and could be safely removed due to
> barriers mechanism but it proved otherwise. The barriers don't provide enough
> throttle to resync IOs. They only assure non-resync IOs and resync IOs are
> not being executed at the same time. In result resync IOs take around 25% of
> CPU time, mostly because there are many of them but only one at a time so a
> lot of CPU time is simply wasted waiting for a single IO to complete.
>
> The removed sleep call in resync IO had allowed a lot of non-resync activity
> to be scheduled (nobody waiting for a barrier). Once sleep call had ended,
> resync IO had to wait longer to raise a barrier as all non-resync activity
> had to be completed first. It had nicely throttled a number of resync IOs in
> favour of non-resync activity. Since we lack it now, the performance has
> dropped badly.
>
> I would like to revert the changeset. We don't have to put a resync IO to
> sleep for a second though. I have done some testing and it seems even a delay
> of 100ms is sufficient. It slows down resync IOs to the same extent as sleep
> for a second - the sleep call ends sooner but the barrier cannot be raised
> until non-resync IOs complete.
Add Neil.
I'd like to make sure I understand the situation. With the change reverted, we
dispatch a lot of normal IO and then do a resync IO. Without it reverted, we
dispatch few normal IO and then do a resync IO. In other words, we don't batch
normal IO currently. Is this what you say?
Agree the barrier doesn't throttle resync IOs, it only assures normal IO and
resync IO run in different time.
On the other hand, the change makes resync faster. Did you try to revert this one:
ac8fa4196d205ac8fff3f8932bddbad4f16e4110
If resync is fast, reverting this one will throttle resync.
Thanks,
Shaohua
^ permalink raw reply
* [PATCH] mdadm --detail --scan causes SIGABRT
From: Nikhil Kshirsagar @ 2016-06-10 4:20 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: Type: text/plain, Size: 3701 bytes --]
Please find attached a patch to fix BZ 1343809.
Details:
mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
larger than 20 characters.
Core was generated by `mdadm --detail /dev/md0'.
Program terminated with signal 6, Aborted.
#0 0x0000003a93e325e5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
(gdb) where
#0 0x0000003a93e325e5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x0000003a93e33dc5 in abort () at abort.c:92
#2 0x0000003a93e704f7 in __libc_message (do_abort=2, fmt=0x3a93f578cf
"*** %s ***: %s terminated\n") at
../sysdeps/unix/sysv/linux/libc_fatal.c:198
#3 0x0000003a93f026d7 in __fortify_fail (msg=0x3a93f57875 "buffer
overflow detected") at fortify_fail.c:32
#4 0x0000003a93f005c0 in __chk_fail () at chk_fail.c:29
#5 0x000000000044fe59 in strcpy (fd=<value optimized out>, devnm=<value
optimized out>, options=<value optimized out>) at
/usr/include/bits/string3.h:105
#6 sysfs_read (fd=<value optimized out>, devnm=<value optimized out>,
options=<value optimized out>) at sysfs.c:272
#7 0x000000000041cdfa in Detail (dev=0x7fffe35f1473 "/dev/md0",
c=0x7fffe35ef590) at Detail.c:106
#8 0x0000000000405ed3 in misc_list (argc=<value optimized out>,
argv=<value optimized out>) at mdadm.c:1747
#9 main (argc=<value optimized out>, argv=<value optimized out>) at
mdadm.c:1425
(gdb)
The line that causes the fault is "sysfs.c" line 272
strcpy(dev->sys_name, de->d_name);
(gdb) print *de
$9 = {d_ino = 14458, d_off = 14471, d_reclen = 40, d_type = 4 '\004',
d_name =
"dev-oczpcie_23_0_ssd\000\207\070\000\000\000\000\000\000\264\070\000\000\000\000\000\000(\000\004dev-oczpcie_11_0_ssd\000\264\070\000\000\000\000\000\000\265\070\000\000\000\000\000\000
\000\bsync_action\000\b\265\070\000\000\000\000\000\000\266\070\000\000\000\000\000\000(\000\blast_sync_action\000\000\000\000\b\266\070\000\000\000\000\000\000\267\070\000\000\000\000\000\000
\000\bmismatch_cnt\000\267\070\000\000\000\000\000\000\270\070\000\000\000\000\000\000(\000\bsync_speed_min\000\000\000\000\000\000\b\270\070\000\000\000\000\000\000\271\070\000\000\000\000\000\000(\000\bsync_speed_max\000\000\000\000\000\000\b\271\070\000\000\000\000\000\000\272\070"}
(gdb)
dev-oczpcie_23_0_ssd itself is 20 bytes.
There is no place left for the terminating \0,
(gdb) ptype dev
type = struct mdinfo {
mdu_array_info_t array;
mdu_disk_info_t disk;
__u64 events;
int uuid[4];
char name[33];
long long unsigned int data_offset;
long long unsigned int new_data_offset;
long long unsigned int component_size;
long long unsigned int custom_array_size;
int reshape_active;
long long unsigned int reshape_progress;
int recovery_blocked;
long long unsigned int space_before;
long long unsigned int space_after;
union {
long long unsigned int resync_start;
long long unsigned int recovery_start;
};
long int bitmap_offset;
long unsigned int safe_mode_delay;
int new_level;
int delta_disks;
int new_layout;
int new_chunk;
int errors;
long unsigned int cache_size;
int mismatch_cnt;
char text_version[50];
int container_member;
int container_enough;
char sys_name[20]; <--- 20 .
struct mdinfo *devs;
struct mdinfo *next;
int recovery_fd;
int state_fd;
int prev_state;
int curr_state;
int next_state;
} *
(gdb)
The patch increases the size of sys_name[] to 32 bytes to match the size
of other device name arrays in the mdadm codebase. A customer reported
this issue in SFDC case 01621749.
Thanks,
nikhil.
[-- Attachment #2: 0001-Fix-for-bz-1343809.patch --]
[-- Type: text/x-patch, Size: 788 bytes --]
From 2c3b5692f8c5933e8746305f589efa4edcc00f3c Mon Sep 17 00:00:00 2001
From: Nikhil Kshirsagar <nkshirsa@redhat.com>
Date: Fri, 10 Jun 2016 08:50:10 +0530
Subject: [PATCH] Fix for bz 1343809.
The sys_name array in the mdinfo structure is 20 bytes of storage.
Increasing the size of this array to 32 bytes.
---
mdadm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mdadm.h b/mdadm.h
index b597658..eb2333a 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -235,7 +235,7 @@ struct mdinfo {
int container_enough; /* flag external handlers can set to
* indicate that subarrays have not enough (-1),
* enough to start (0), or all expected disks (1) */
- char sys_name[20];
+ char sys_name[32];
struct mdinfo *devs;
struct mdinfo *next;
--
1.8.3.1
^ permalink raw reply related
* Re: [md PATCH 01/18] md: disconnect device from personality before trying to remove it.
From: NeilBrown @ 2016-06-10 6:40 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160603223130.GB1898@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]
On Sat, Jun 04 2016, Shaohua Li wrote:
> On Thu, Jun 02, 2016 at 04:19:52PM +1000, Neil Brown wrote:
>> When the HOT_REMOVE_DISK ioctl is used to remove a device, we
>> call remove_and_add_spares() which will remove it from the personality
>> if possible. This improves the chances that the removal will succeed.
>>
>> When writing "remove" to dev-XX/state, we don't. So that can fail more easily.
>>
>> So add the remove_and_add_spares() into "remove" handling.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> drivers/md/md.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 866825f10b4c..2d26099e1160 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -2596,6 +2596,8 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
>> else
>> err = -EBUSY;
>> } else if (cmd_match(buf, "remove")) {
>> + clear_bit(Blocked, &rdev->flags);
>> + remove_and_add_spares(rdev->mddev, rdev);
>> if (rdev->raid_disk >= 0)
>> err = -EBUSY;
>> else {
>
> Do we need wakeup rdev->blocked_wait here? I noticed some times we do the
> wakeup but sometimes we not. This makes me worry about
> md_wait_for_blocked_rdev. Will timeout cause anything bad?
(sorry for delay)
Yes, we probably should but it isn't very important. If any code is
waiting then the 'remove' will fail and whatever would normally unblock
the device will still unblock and wake up as it would have done.
It is import to wake blocked_rdev when we write out the bad block list
or other metadata which makes it safe to use the device. If we clear
the flag at other time it doesn't really matter.
Adding a wakeup here might make it more likely for the "remove" to work
in some rare cases, but I think that is the most significant effect.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [md PATCH 05/18] md/raid10: add rcu protection to rdev access in raid10_sync_request.
From: NeilBrown @ 2016-06-10 6:46 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid
In-Reply-To: <20160603223315.GC1898@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]
On Sat, Jun 04 2016, Shaohua Li wrote:
> On Thu, Jun 02, 2016 at 04:19:52PM +1000, Neil Brown wrote:
>> 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;
>> + }
>
> We don't check mreplace here.
As you say ... I wonder if I thought I was being clever somehow...
I'll resubmit with that fixed but it probably won't be for a couple of
week (I'm actually on leave, but thought I should reply).
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: NeilBrown @ 2016-06-10 6:47 UTC (permalink / raw)
To: Shaohua Li; +Cc: John Stoffel, linux-raid
In-Reply-To: <20160603223909.GD1898@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 2873 bytes --]
On Sat, Jun 04 2016, Shaohua Li wrote:
> On Fri, Jun 03, 2016 at 08:48:33AM +1000, Neil Brown wrote:
>> 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.
>
> I would prefer pr_debug. pr_debug can be enabled dynamically. If the info is
> helpful for diagnosing, enabling it is simple.
Fair enough. Please drop this patch. I'll come up with an improved proposal
and resubmit.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: Low RAID10 performance during resync
From: NeilBrown @ 2016-06-10 7:08 UTC (permalink / raw)
To: Shaohua Li, Tomasz Majchrzak; +Cc: linux-raid
In-Reply-To: <20160609173118.GA17207@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 3319 bytes --]
On Fri, Jun 10 2016, Shaohua Li wrote:
> On Thu, Jun 09, 2016 at 03:45:55PM +0200, Tomasz Majchrzak wrote:
>> A low performance of mkfs has been observed on RAID10 array during resync. It
>> is not so significant for NVMe drives but for my setup of RAID10 consisting
>> of 4 SATA drives format time has increased by 200%.
>>
>> I have looked into the problem and I have found out it is caused by this
>> changeset:
>>
>> commit 09314799e4f0589e52bafcd0ca3556c60468bc0e md: remove 'go_faster' option
>> from ->sync_request()
>>
>> It seemed the code had been redundant and could be safely removed due to
>> barriers mechanism but it proved otherwise. The barriers don't provide enough
>> throttle to resync IOs. They only assure non-resync IOs and resync IOs are
>> not being executed at the same time. In result resync IOs take around 25% of
>> CPU time, mostly because there are many of them but only one at a time so a
>> lot of CPU time is simply wasted waiting for a single IO to complete.
>>
>> The removed sleep call in resync IO had allowed a lot of non-resync activity
>> to be scheduled (nobody waiting for a barrier). Once sleep call had ended,
>> resync IO had to wait longer to raise a barrier as all non-resync activity
>> had to be completed first. It had nicely throttled a number of resync IOs in
>> favour of non-resync activity. Since we lack it now, the performance has
>> dropped badly.
>>
>> I would like to revert the changeset. We don't have to put a resync IO to
>> sleep for a second though. I have done some testing and it seems even a delay
>> of 100ms is sufficient. It slows down resync IOs to the same extent as sleep
>> for a second - the sleep call ends sooner but the barrier cannot be raised
>> until non-resync IOs complete.
>
> Add Neil.
>
> I'd like to make sure I understand the situation. With the change reverted, we
> dispatch a lot of normal IO and then do a resync IO. Without it reverted, we
> dispatch few normal IO and then do a resync IO. In other words, we don't batch
> normal IO currently. Is this what you say?
>
> Agree the barrier doesn't throttle resync IOs, it only assures normal IO and
> resync IO run in different time.
I think the barrier mechanism will mostly let large batches of IO
through as a match. If there is a pending request, a new request will
always be let straight through. Resync needs to wait for all pending
regular IO to complete before it gets a turn.
So I would only expect that patch to cause problems when IO is very
synchronous: write, wait, write, wait, etc.
I really didn't like the "go_faster" mechanism, but it might be OK to
have something like
if (conf->nr_waiting)
schedule_timeout_uninterruptible(1);
so it will wait one jiffie if there is normal IO. This would batch this
a lot more.
It is very hard to know the exact consequences of this sort of change on
all different configurations, and the other commit you mentioned shows.
I keep thinking there must be a better way, but I haven't found it yet
:-(
NeilBrown
>
> On the other hand, the change makes resync faster. Did you try to revert this one:
> ac8fa4196d205ac8fff3f8932bddbad4f16e4110
> If resync is fast, reverting this one will throttle resync.
>
> Thanks,
> Shaohua
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: Low RAID10 performance during resync
From: Tomasz Majchrzak @ 2016-06-10 14:45 UTC (permalink / raw)
To: NeilBrown; +Cc: Shaohua Li, linux-raid
In-Reply-To: <87k2hxmsgz.fsf@notabene.neil.brown.name>
On Fri, Jun 10, 2016 at 05:08:12PM +1000, NeilBrown wrote:
> On Fri, Jun 10 2016, Shaohua Li wrote:
>
> > On Thu, Jun 09, 2016 at 03:45:55PM +0200, Tomasz Majchrzak wrote:
> >> A low performance of mkfs has been observed on RAID10 array during resync. It
> >> is not so significant for NVMe drives but for my setup of RAID10 consisting
> >> of 4 SATA drives format time has increased by 200%.
> >>
> >> I have looked into the problem and I have found out it is caused by this
> >> changeset:
> >>
> >> commit 09314799e4f0589e52bafcd0ca3556c60468bc0e md: remove 'go_faster' option
> >> from ->sync_request()
> >>
> >> It seemed the code had been redundant and could be safely removed due to
> >> barriers mechanism but it proved otherwise. The barriers don't provide enough
> >> throttle to resync IOs. They only assure non-resync IOs and resync IOs are
> >> not being executed at the same time. In result resync IOs take around 25% of
> >> CPU time, mostly because there are many of them but only one at a time so a
> >> lot of CPU time is simply wasted waiting for a single IO to complete.
> >>
> >> The removed sleep call in resync IO had allowed a lot of non-resync activity
> >> to be scheduled (nobody waiting for a barrier). Once sleep call had ended,
> >> resync IO had to wait longer to raise a barrier as all non-resync activity
> >> had to be completed first. It had nicely throttled a number of resync IOs in
> >> favour of non-resync activity. Since we lack it now, the performance has
> >> dropped badly.
> >>
> >> I would like to revert the changeset. We don't have to put a resync IO to
> >> sleep for a second though. I have done some testing and it seems even a delay
> >> of 100ms is sufficient. It slows down resync IOs to the same extent as sleep
> >> for a second - the sleep call ends sooner but the barrier cannot be raised
> >> until non-resync IOs complete.
> >
> > Add Neil.
> >
> > I'd like to make sure I understand the situation. With the change reverted, we
> > dispatch a lot of normal IO and then do a resync IO. Without it reverted, we
> > dispatch few normal IO and then do a resync IO. In other words, we don't batch
> > normal IO currently. Is this what you say?
> >
> > Agree the barrier doesn't throttle resync IOs, it only assures normal IO and
> > resync IO run in different time.
Yes, precisely, resync is faster. The problem is performance drop from user
perspective is too big.
>
> I think the barrier mechanism will mostly let large batches of IO
> through as a match. If there is a pending request, a new request will
> always be let straight through. Resync needs to wait for all pending
> regular IO to complete before it gets a turn.
>
> So I would only expect that patch to cause problems when IO is very
> synchronous: write, wait, write, wait, etc.
>
> I really didn't like the "go_faster" mechanism, but it might be OK to
> have something like
> if (conf->nr_waiting)
> schedule_timeout_uninterruptible(1);
>
> so it will wait one jiffie if there is normal IO. This would batch this
> a lot more.
>
> It is very hard to know the exact consequences of this sort of change on
> all different configurations, and the other commit you mentioned shows.
>
> I keep thinking there must be a better way, but I haven't found it yet
> :-(
>
> NeilBrown
>
>
> >
> > On the other hand, the change makes resync faster. Did you try to revert this one:
> > ac8fa4196d205ac8fff3f8932bddbad4f16e4110
> > If resync is fast, reverting this one will throttle resync.
> >
> > Thanks,
> > Shaohua
I reverted it and it brought performance to the initial level. It's not a
solution though, isn't it?
I have incorrectly reported current performance drop. At the moment mkfs on my
setup takes around 20 minutes. Before the change it used to take 1 min 20 secs.
I have checked Neil's proposal (schedule_timeout_uninterruptible for 1 jiffies)
- it would bring formatting time to 2 mins 16 secs - so it's a valid solution to
the problem.
I have also tried other approach. Neil has mentioned that pending requests will
be let straight through if there are requests already in progress. Well, the
code looks so, however current->bio_list is empty most of the time, even though
the requests are being processed. I added an extra time window which allows
requests to proceed, even though there is a barrier awaiting already. It brings
mkfs performance to the initial level (1 min 20 secs).
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index e3fd725..51caf87 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -916,6 +916,8 @@ static void raise_barrier(struct r10conf *conf, int force)
!conf->nr_pending && conf->barrier < RESYNC_DEPTH,
conf->resync_lock);
+ conf->last_resync_time = jiffies;
+
spin_unlock_irq(&conf->resync_lock);
}
@@ -945,8 +947,9 @@ static void wait_barrier(struct r10conf *conf)
wait_event_lock_irq(conf->wait_barrier,
!conf->barrier ||
(conf->nr_pending &&
- current->bio_list &&
- !bio_list_empty(current->bio_list)),
+ ((current->bio_list &&
+ !bio_list_empty(current->bio_list)) ||
+ (jiffies - conf->last_resync_time) < HZ / 20)),
conf->resync_lock);
conf->nr_waiting--;
}
Please tell me if you prefer Neil's or my solution.
Tomek
^ permalink raw reply related
* Re: [PATCH] mdadm --detail --scan causes SIGABRT
From: Nikhil Kshirsagar @ 2016-06-10 15:43 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <575A4018.8050500@redhat.com>
I forgot to add the 'signed off by' part, request the maintainer to add it during the merge.
Thanks,
Nikhil.
> On 10-Jun-2016, at 9:50 AM, Nikhil Kshirsagar <nkshirsa@redhat.com> wrote:
>
>
> Please find attached a patch to fix BZ 1343809.
>
> Details:
> mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
> larger than 20 characters.
>
> Core was generated by `mdadm --detail /dev/md0'.
> Program terminated with signal 6, Aborted.
> #0 0x0000003a93e325e5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> 64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> (gdb) where
> #0 0x0000003a93e325e5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #1 0x0000003a93e33dc5 in abort () at abort.c:92
> #2 0x0000003a93e704f7 in __libc_message (do_abort=2, fmt=0x3a93f578cf
> "*** %s ***: %s terminated\n") at
> ../sysdeps/unix/sysv/linux/libc_fatal.c:198
> #3 0x0000003a93f026d7 in __fortify_fail (msg=0x3a93f57875 "buffer
> overflow detected") at fortify_fail.c:32
> #4 0x0000003a93f005c0 in __chk_fail () at chk_fail.c:29
> #5 0x000000000044fe59 in strcpy (fd=<value optimized out>, devnm=<value
> optimized out>, options=<value optimized out>) at
> /usr/include/bits/string3.h:105
> #6 sysfs_read (fd=<value optimized out>, devnm=<value optimized out>,
> options=<value optimized out>) at sysfs.c:272
> #7 0x000000000041cdfa in Detail (dev=0x7fffe35f1473 "/dev/md0",
> c=0x7fffe35ef590) at Detail.c:106
> #8 0x0000000000405ed3 in misc_list (argc=<value optimized out>,
> argv=<value optimized out>) at mdadm.c:1747
> #9 main (argc=<value optimized out>, argv=<value optimized out>) at
> mdadm.c:1425
> (gdb)
>
>
> The line that causes the fault is "sysfs.c" line 272
>
> strcpy(dev->sys_name, de->d_name);
>
> (gdb) print *de
> $9 = {d_ino = 14458, d_off = 14471, d_reclen = 40, d_type = 4 '\004',
> d_name =
> "dev-oczpcie_23_0_ssd\000\207\070\000\000\000\000\000\000\264\070\000\000\000\000\000\000(\000\004dev-oczpcie_11_0_ssd\000\264\070\000\000\000\000\000\000\265\070\000\000\000\000\000\000
> \000\bsync_action\000\b\265\070\000\000\000\000\000\000\266\070\000\000\000\000\000\000(\000\blast_sync_action\000\000\000\000\b\266\070\000\000\000\000\000\000\267\070\000\000\000\000\000\000
> \000\bmismatch_cnt\000\267\070\000\000\000\000\000\000\270\070\000\000\000\000\000\000(\000\bsync_speed_min\000\000\000\000\000\000\b\270\070\000\000\000\000\000\000\271\070\000\000\000\000\000\000(\000\bsync_speed_max\000\000\000\000\000\000\b\271\070\000\000\000\000\000\000\272\070"}
> (gdb)
>
> dev-oczpcie_23_0_ssd itself is 20 bytes.
>
> There is no place left for the terminating \0,
>
> (gdb) ptype dev
> type = struct mdinfo {
> mdu_array_info_t array;
> mdu_disk_info_t disk;
> __u64 events;
> int uuid[4];
> char name[33];
> long long unsigned int data_offset;
> long long unsigned int new_data_offset;
> long long unsigned int component_size;
> long long unsigned int custom_array_size;
> int reshape_active;
> long long unsigned int reshape_progress;
> int recovery_blocked;
> long long unsigned int space_before;
> long long unsigned int space_after;
> union {
> long long unsigned int resync_start;
> long long unsigned int recovery_start;
> };
> long int bitmap_offset;
> long unsigned int safe_mode_delay;
> int new_level;
> int delta_disks;
> int new_layout;
> int new_chunk;
> int errors;
> long unsigned int cache_size;
> int mismatch_cnt;
> char text_version[50];
> int container_member;
> int container_enough;
> char sys_name[20]; <--- 20 .
> struct mdinfo *devs;
> struct mdinfo *next;
> int recovery_fd;
> int state_fd;
> int prev_state;
> int curr_state;
> int next_state;
> } *
> (gdb)
>
> The patch increases the size of sys_name[] to 32 bytes to match the size
> of other device name arrays in the mdadm codebase. A customer reported
> this issue in SFDC case 01621749.
>
> Thanks,
> nikhil.
>
> <0001-Fix-for-bz-1343809.patch>
^ permalink raw reply
* Re: [md PATCH 05/18] md/raid10: add rcu protection to rdev access in raid10_sync_request.
From: Shaohua Li @ 2016-06-10 16:22 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <87porpmtgq.fsf@notabene.neil.brown.name>
On Fri, Jun 10, 2016 at 04:46:45PM +1000, Neil Brown wrote:
> On Sat, Jun 04 2016, Shaohua Li wrote:
>
> > On Thu, Jun 02, 2016 at 04:19:52PM +1000, Neil Brown wrote:
> >> 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;
> >> + }
> >
> > We don't check mreplace here.
>
> As you say ... I wonder if I thought I was being clever somehow...
>
> I'll resubmit with that fixed but it probably won't be for a couple of
> week (I'm actually on leave, but thought I should reply).
I can fix this one if you like.
So I'll drop patch 9 and apply others with this patch fixed. Sounds good?
Thanks,
Shaohua
^ permalink raw reply
* Re: Low RAID10 performance during resync
From: Shaohua Li @ 2016-06-10 16:33 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: NeilBrown, linux-raid
In-Reply-To: <20160610144549.GA4251@proton.igk.intel.com>
On Fri, Jun 10, 2016 at 04:45:49PM +0200, Tomasz Majchrzak wrote:
> On Fri, Jun 10, 2016 at 05:08:12PM +1000, NeilBrown wrote:
> > On Fri, Jun 10 2016, Shaohua Li wrote:
> >
> > > On Thu, Jun 09, 2016 at 03:45:55PM +0200, Tomasz Majchrzak wrote:
> > >> A low performance of mkfs has been observed on RAID10 array during resync. It
> > >> is not so significant for NVMe drives but for my setup of RAID10 consisting
> > >> of 4 SATA drives format time has increased by 200%.
> > >>
> > >> I have looked into the problem and I have found out it is caused by this
> > >> changeset:
> > >>
> > >> commit 09314799e4f0589e52bafcd0ca3556c60468bc0e md: remove 'go_faster' option
> > >> from ->sync_request()
> > >>
> > >> It seemed the code had been redundant and could be safely removed due to
> > >> barriers mechanism but it proved otherwise. The barriers don't provide enough
> > >> throttle to resync IOs. They only assure non-resync IOs and resync IOs are
> > >> not being executed at the same time. In result resync IOs take around 25% of
> > >> CPU time, mostly because there are many of them but only one at a time so a
> > >> lot of CPU time is simply wasted waiting for a single IO to complete.
> > >>
> > >> The removed sleep call in resync IO had allowed a lot of non-resync activity
> > >> to be scheduled (nobody waiting for a barrier). Once sleep call had ended,
> > >> resync IO had to wait longer to raise a barrier as all non-resync activity
> > >> had to be completed first. It had nicely throttled a number of resync IOs in
> > >> favour of non-resync activity. Since we lack it now, the performance has
> > >> dropped badly.
> > >>
> > >> I would like to revert the changeset. We don't have to put a resync IO to
> > >> sleep for a second though. I have done some testing and it seems even a delay
> > >> of 100ms is sufficient. It slows down resync IOs to the same extent as sleep
> > >> for a second - the sleep call ends sooner but the barrier cannot be raised
> > >> until non-resync IOs complete.
> > >
> > > Add Neil.
> > >
> > > I'd like to make sure I understand the situation. With the change reverted, we
> > > dispatch a lot of normal IO and then do a resync IO. Without it reverted, we
> > > dispatch few normal IO and then do a resync IO. In other words, we don't batch
> > > normal IO currently. Is this what you say?
> > >
> > > Agree the barrier doesn't throttle resync IOs, it only assures normal IO and
> > > resync IO run in different time.
>
> Yes, precisely, resync is faster. The problem is performance drop from user
> perspective is too big.
>
> >
> > I think the barrier mechanism will mostly let large batches of IO
> > through as a match. If there is a pending request, a new request will
> > always be let straight through. Resync needs to wait for all pending
> > regular IO to complete before it gets a turn.
> >
> > So I would only expect that patch to cause problems when IO is very
> > synchronous: write, wait, write, wait, etc.
> >
> > I really didn't like the "go_faster" mechanism, but it might be OK to
> > have something like
> > if (conf->nr_waiting)
> > schedule_timeout_uninterruptible(1);
> >
> > so it will wait one jiffie if there is normal IO. This would batch this
> > a lot more.
> >
> > It is very hard to know the exact consequences of this sort of change on
> > all different configurations, and the other commit you mentioned shows.
> >
> > I keep thinking there must be a better way, but I haven't found it yet
> > :-(
> >
> > NeilBrown
> >
> >
> > >
> > > On the other hand, the change makes resync faster. Did you try to revert this one:
> > > ac8fa4196d205ac8fff3f8932bddbad4f16e4110
> > > If resync is fast, reverting this one will throttle resync.
> > >
> > > Thanks,
> > > Shaohua
>
> I reverted it and it brought performance to the initial level. It's not a
> solution though, isn't it?
>
> I have incorrectly reported current performance drop. At the moment mkfs on my
> setup takes around 20 minutes. Before the change it used to take 1 min 20 secs.
>
> I have checked Neil's proposal (schedule_timeout_uninterruptible for 1 jiffies)
> - it would bring formatting time to 2 mins 16 secs - so it's a valid solution to
> the problem.
>
> I have also tried other approach. Neil has mentioned that pending requests will
> be let straight through if there are requests already in progress. Well, the
> code looks so, however current->bio_list is empty most of the time, even though
> the requests are being processed. I added an extra time window which allows
> requests to proceed, even though there is a barrier awaiting already. It brings
> mkfs performance to the initial level (1 min 20 secs).
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index e3fd725..51caf87 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -916,6 +916,8 @@ static void raise_barrier(struct r10conf *conf, int force)
> !conf->nr_pending && conf->barrier < RESYNC_DEPTH,
> conf->resync_lock);
>
> + conf->last_resync_time = jiffies;
> +
> spin_unlock_irq(&conf->resync_lock);
> }
>
> @@ -945,8 +947,9 @@ static void wait_barrier(struct r10conf *conf)
> wait_event_lock_irq(conf->wait_barrier,
> !conf->barrier ||
> (conf->nr_pending &&
> - current->bio_list &&
> - !bio_list_empty(current->bio_list)),
> + ((current->bio_list &&
> + !bio_list_empty(current->bio_list)) ||
> + (jiffies - conf->last_resync_time) < HZ / 20)),
> conf->resync_lock);
> conf->nr_waiting--;
> }
>
> Please tell me if you prefer Neil's or my solution.
Thanks for the testing. So we have several potential solutions:
revert ac8fa4196d205ac8fff3f8932bddbad4f16e4110
revert 09314799e4f0589e52bafcd0ca3556c60468bc0e
Neil's proposal and your proposal
either one will workaround this issue. In the long term, I'd like to fix the
ac8fa4196d20. but this is a hard problem, I don't have a clear picture of the
impact of those fixes. For an immediate solution, I'd prefer Neil's proposal,
which is simple and a kind of revert of original patch. Probably we should do
the same for raid1 too.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH] mdadm --detail --scan causes SIGABRT
From: Jes Sorensen @ 2016-06-10 17:11 UTC (permalink / raw)
To: Nikhil Kshirsagar; +Cc: linux-raid
In-Reply-To: <575A4018.8050500@redhat.com>
Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
> Please find attached a patch to fix BZ 1343809.
>
> Details:
> mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
> larger than 20 characters.
>
> Core was generated by `mdadm --detail /dev/md0'.
> Program terminated with signal 6, Aborted.
> #0 0x0000003a93e325e5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> 64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> (gdb) where
> #0 0x0000003a93e325e5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #1 0x0000003a93e33dc5 in abort () at abort.c:92
> #2 0x0000003a93e704f7 in __libc_message (do_abort=2, fmt=0x3a93f578cf
> "*** %s ***: %s terminated\n") at
> ../sysdeps/unix/sysv/linux/libc_fatal.c:198
> #3 0x0000003a93f026d7 in __fortify_fail (msg=0x3a93f57875 "buffer
> overflow detected") at fortify_fail.c:32
> #4 0x0000003a93f005c0 in __chk_fail () at chk_fail.c:29
> #5 0x000000000044fe59 in strcpy (fd=<value optimized out>, devnm=<value
> optimized out>, options=<value optimized out>) at
> /usr/include/bits/string3.h:105
> #6 sysfs_read (fd=<value optimized out>, devnm=<value optimized out>,
> options=<value optimized out>) at sysfs.c:272
> #7 0x000000000041cdfa in Detail (dev=0x7fffe35f1473 "/dev/md0",
> c=0x7fffe35ef590) at Detail.c:106
> #8 0x0000000000405ed3 in misc_list (argc=<value optimized out>,
> argv=<value optimized out>) at mdadm.c:1747
> #9 main (argc=<value optimized out>, argv=<value optimized out>) at
> mdadm.c:1425
> (gdb)
>
>
> The line that causes the fault is "sysfs.c" line 272
>
> strcpy(dev->sys_name, de->d_name);
>
> (gdb) print *de
> $9 = {d_ino = 14458, d_off = 14471, d_reclen = 40, d_type = 4 '\004',
> d_name =
> "dev-oczpcie_23_0_ssd\000\207\070\000\000\000\000\000\000\264\070\000\000\000\000\000\000(\000\004dev-oczpcie_11_0_ssd\000\264\070\000\000\000\000\000\000\265\070\000\000\000\000\000\000
> \000\bsync_action\000\b\265\070\000\000\000\000\000\000\266\070\000\000\000\000\000\000(\000\blast_sync_action\000\000\000\000\b\266\070\000\000\000\000\000\000\267\070\000\000\000\000\000\000
> \000\bmismatch_cnt\000\267\070\000\000\000\000\000\000\270\070\000\000\000\000\000\000(\000\bsync_speed_min\000\000\000\000\000\000\b\270\070\000\000\000\000\000\000\271\070\000\000\000\000\000\000(\000\bsync_speed_max\000\000\000\000\000\000\b\271\070\000\000\000\000\000\000\272\070"}
> (gdb)
>
> dev-oczpcie_23_0_ssd itself is 20 bytes.
>
> There is no place left for the terminating \0,
Nikhil,
I am curious, how did you get that long device name? I tried to
reproduce the problem here using /dev/disk/by-id/ devices, but they get
converted to the shorter /dev/sdX names automatically so it doesn't
trigger.
I do not disagree we need to fix this, but I am curious of the real life
scenario how you hit the problem?
Right now I am more wondering whether we should handle this in a more
generic way by allocating an appriopriately sized buffer or bump up the
name buffer the way your patch did it.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH] mdadm --detail --scan causes SIGABRT
From: Nikhil Kshirsagar @ 2016-06-10 17:41 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <wrfjy46dufxl.fsf@redhat.com>
Hi Jes ,
Would it help to examine the core file ? It's present on the machine and location specified in the bz comments . That's how I saw the data structure that had the issue . Indeed there are other device names where the name does not overflow since they are 32 bytes (which is why I chose this value) or where the name *does* get truncated. However this truncation does not seem to happen for de->d_name which is then copied into dev->sys_name.
As for allocating an appropriate size on the heap instead of a static array it does make sense and I can correct the fix to do that but there are lots of other device names which are static arrays. So which ones do we change ?
-nikhil.
> On 10-Jun-2016, at 10:41 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>
> Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
>> Please find attached a patch to fix BZ 1343809.
>>
>> Details:
>> mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
>> larger than 20 characters.
>>
>> Core was generated by `mdadm --detail /dev/md0'.
>> Program terminated with signal 6, Aborted.
>> #0 0x0000003a93e325e5 in raise (sig=6) at
>> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
>> 64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
>> (gdb) where
>> #0 0x0000003a93e325e5 in raise (sig=6) at
>> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
>> #1 0x0000003a93e33dc5 in abort () at abort.c:92
>> #2 0x0000003a93e704f7 in __libc_message (do_abort=2, fmt=0x3a93f578cf
>> "*** %s ***: %s terminated\n") at
>> ../sysdeps/unix/sysv/linux/libc_fatal.c:198
>> #3 0x0000003a93f026d7 in __fortify_fail (msg=0x3a93f57875 "buffer
>> overflow detected") at fortify_fail.c:32
>> #4 0x0000003a93f005c0 in __chk_fail () at chk_fail.c:29
>> #5 0x000000000044fe59 in strcpy (fd=<value optimized out>, devnm=<value
>> optimized out>, options=<value optimized out>) at
>> /usr/include/bits/string3.h:105
>> #6 sysfs_read (fd=<value optimized out>, devnm=<value optimized out>,
>> options=<value optimized out>) at sysfs.c:272
>> #7 0x000000000041cdfa in Detail (dev=0x7fffe35f1473 "/dev/md0",
>> c=0x7fffe35ef590) at Detail.c:106
>> #8 0x0000000000405ed3 in misc_list (argc=<value optimized out>,
>> argv=<value optimized out>) at mdadm.c:1747
>> #9 main (argc=<value optimized out>, argv=<value optimized out>) at
>> mdadm.c:1425
>> (gdb)
>>
>>
>> The line that causes the fault is "sysfs.c" line 272
>>
>> strcpy(dev->sys_name, de->d_name);
>>
>> (gdb) print *de
>> $9 = {d_ino = 14458, d_off = 14471, d_reclen = 40, d_type = 4 '\004',
>> d_name =
>> "dev-oczpcie_23_0_ssd\000\207\070\000\000\000\000\000\000\264\070\000\000\000\000\000\000(\000\004dev-oczpcie_11_0_ssd\000\264\070\000\000\000\000\000\000\265\070\000\000\000\000\000\000
>> \000\bsync_action\000\b\265\070\000\000\000\000\000\000\266\070\000\000\000\000\000\000(\000\blast_sync_action\000\000\000\000\b\266\070\000\000\000\000\000\000\267\070\000\000\000\000\000\000
>> \000\bmismatch_cnt\000\267\070\000\000\000\000\000\000\270\070\000\000\000\000\000\000(\000\bsync_speed_min\000\000\000\000\000\000\b\270\070\000\000\000\000\000\000\271\070\000\000\000\000\000\000(\000\bsync_speed_max\000\000\000\000\000\000\b\271\070\000\000\000\000\000\000\272\070"}
>> (gdb)
>>
>> dev-oczpcie_23_0_ssd itself is 20 bytes.
>>
>> There is no place left for the terminating \0,
>
> Nikhil,
>
> I am curious, how did you get that long device name? I tried to
> reproduce the problem here using /dev/disk/by-id/ devices, but they get
> converted to the shorter /dev/sdX names automatically so it doesn't
> trigger.
>
> I do not disagree we need to fix this, but I am curious of the real life
> scenario how you hit the problem?
>
> Right now I am more wondering whether we should handle this in a more
> generic way by allocating an appriopriately sized buffer or bump up the
> name buffer the way your patch did it.
>
> Cheers,
> Jes
^ permalink raw reply
* Re: [PATCH] mdadm --detail --scan causes SIGABRT
From: Jes Sorensen @ 2016-06-10 17:48 UTC (permalink / raw)
To: Nikhil Kshirsagar; +Cc: linux-raid
In-Reply-To: <2F74FF87-7E89-4452-8F90-53923B1B2E80@redhat.com>
Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
> Hi Jes ,
>
> Would it help to examine the core file ? It's present on the machine
> and location specified in the bz comments . That's how I saw the data
> structure that had the issue . Indeed there are other device names
> where the name does not overflow since they are 32 bytes (which is why
> I chose this value) or where the name *does* get truncated. However
> this truncation does not seem to happen for de->d_name which is then
> copied into dev->sys_name.
>
> As for allocating an appropriate size on the heap instead of a static
> array it does make sense and I can correct the fix to do that but
> there are lots of other device names which are static arrays. So which
> ones do we change ?
This is the tricky part, sys_name is used in a lot of places in
different ways.
Do you know if they have a /dev/oczpcie_11_0_ssd on the system, and if
they do, how did that device get created?
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH] mdadm --detail --scan causes SIGABRT
From: Nikhil Kshirsagar @ 2016-06-10 18:12 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <wrfj8tydue97.fsf@redhat.com>
I think the sosreport should contain the info of the raid devices even though mdadm crashes so we don't see that output in there , there is proc/mdstat and other raid related info there. Let me check.
If nothing else I can ask them for proc/mdstat and mdadm -detail again with the working test binary i sent them.
-Nikhil.
> On 10-Jun-2016, at 11:18 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>
> Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
>> Hi Jes ,
>>
>> Would it help to examine the core file ? It's present on the machine
>> and location specified in the bz comments . That's how I saw the data
>> structure that had the issue . Indeed there are other device names
>> where the name does not overflow since they are 32 bytes (which is why
>> I chose this value) or where the name *does* get truncated. However
>> this truncation does not seem to happen for de->d_name which is then
>> copied into dev->sys_name.
>>
>> As for allocating an appropriate size on the heap instead of a static
>> array it does make sense and I can correct the fix to do that but
>> there are lots of other device names which are static arrays. So which
>> ones do we change ?
>
> This is the tricky part, sys_name is used in a lot of places in
> different ways.
>
> Do you know if they have a /dev/oczpcie_11_0_ssd on the system, and if
> they do, how did that device get created?
>
> Cheers,
> Jes
^ permalink raw reply
* Re: [PATCH] bcache: Remove deprecated create_workqueue
From: Tejun Heo @ 2016-06-11 22:52 UTC (permalink / raw)
To: Bhaktipriya Shridhar
Cc: Kent Overstreet, Shaohua Li, Jens Axboe, Eric Wheeler, Zheng Liu,
Hannes Reinecke, Jeff Moyer, Pekka Enberg, Al Viro, linux-bcache,
linux-raid, linux-kernel
In-Reply-To: <20160607202719.GA16208@Karyakshetra>
On Wed, Jun 08, 2016 at 01:57:19AM +0530, Bhaktipriya Shridhar wrote:
> alloc_workqueue replaces deprecated create_workqueue().
>
> Dedicated workqueues have been used since bcache_wq and moving_gc_wq
> are workqueues for writes and are being used on a memory reclaim path.
> WQ_MEM_RECLAIM has been set to ensure forward progress under memory
> pressure.
> Since there are only a fixed number of work items, explicit concurrency
> limit is unnecessary here.
>
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] bcache: Remove deprecated create_workqueue
From: Jens Axboe @ 2016-06-12 2:03 UTC (permalink / raw)
To: Tejun Heo, Bhaktipriya Shridhar
Cc: Kent Overstreet, Shaohua Li, Eric Wheeler, Zheng Liu,
Hannes Reinecke, Jeff Moyer, Pekka Enberg, Al Viro, linux-bcache,
linux-raid, linux-kernel
In-Reply-To: <20160611225202.GD31708@htj.duckdns.org>
On 06/11/2016 04:52 PM, Tejun Heo wrote:
> On Wed, Jun 08, 2016 at 01:57:19AM +0530, Bhaktipriya Shridhar wrote:
>> alloc_workqueue replaces deprecated create_workqueue().
>>
>> Dedicated workqueues have been used since bcache_wq and moving_gc_wq
>> are workqueues for writes and are being used on a memory reclaim path.
>> WQ_MEM_RECLAIM has been set to ensure forward progress under memory
>> pressure.
>> Since there are only a fixed number of work items, explicit concurrency
>> limit is unnecessary here.
>>
>> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com>
>
> Acked-by: Tejun Heo <tj@kernel.org>
Added to 4.8, thanks.
--
Jens Axboe
^ permalink raw reply
* [PATCH 1/1] MDADM:Check mdinfo->reshape_active more times before calling Grow_continue
From: Xiao Ni @ 2016-06-12 9:15 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: linux-raid
Hi
When reshaping a 3 drives raid5 to 4 drives raid5, there is a chance that
it can't start the reshape. If the disks are not enough to have spaces for
relocating the data_offset, it needs to call start_reshape and then run
mdadm --grow --continue by systemd. But mdadm --grow --continue fails
because it checkes that info->reshape_active is 0.
The info->reshape_active is got from the superblock of underlying devices.
Function start_reshape write reshape to /sys/../sync_action. Before writing
latest superblock to underlying devices, mdadm --grow --continue is called.
There is a chance info->reshape_active is 0. We should wait for superblock
updating more time before calling Grow_continue.
Regards
Xiao
---
Grow.c | 68 +++++++++++++++++++++++++++++++++++++-----------------------------
1 file changed, 38 insertions(+), 30 deletions(-)
diff --git a/Grow.c b/Grow.c
index f184d9c..1b0e4d1 100755
--- a/Grow.c
+++ b/Grow.c
@@ -4788,6 +4788,7 @@ int Grow_continue_command(char *devname, int fd,
dprintf("Grow continue is run for ");
if (st->ss->external == 0) {
int d;
+ int cnt = 5;
dprintf_cont("native array (%s)\n", devname);
if (ioctl(fd, GET_ARRAY_INFO, &array.array) < 0) {
pr_err("%s is not an active md array - aborting\n", devname);
@@ -4799,36 +4800,43 @@ int Grow_continue_command(char *devname, int fd,
* FIXME we should really get what we need from
* sysfs
*/
- for (d = 0; d < MAX_DISKS; d++) {
- mdu_disk_info_t disk;
- char *dv;
- int err;
- disk.number = d;
- if (ioctl(fd, GET_DISK_INFO, &disk) < 0)
- continue;
- if (disk.major == 0 && disk.minor == 0)
- continue;
- if ((disk.state & (1 << MD_DISK_ACTIVE)) == 0)
- continue;
- dv = map_dev(disk.major, disk.minor, 1);
- if (!dv)
- continue;
- fd2 = dev_open(dv, O_RDONLY);
- if (fd2 < 0)
- continue;
- err = st->ss->load_super(st, fd2, NULL);
- close(fd2);
- if (err)
- continue;
- break;
- }
- if (d == MAX_DISKS) {
- pr_err("Unable to load metadata for %s\n",
- devname);
- ret_val = 1;
- goto Grow_continue_command_exit;
- }
- st->ss->getinfo_super(st, content, NULL);
+ do {
+ for (d = 0; d < MAX_DISKS; d++) {
+ mdu_disk_info_t disk;
+ char *dv;
+ int err;
+ disk.number = d;
+ if (ioctl(fd, GET_DISK_INFO, &disk) < 0)
+ continue;
+ if (disk.major == 0 && disk.minor == 0)
+ continue;
+ if ((disk.state & (1 << MD_DISK_ACTIVE)) == 0)
+ continue;
+ dv = map_dev(disk.major, disk.minor, 1);
+ if (!dv)
+ continue;
+ fd2 = dev_open(dv, O_RDONLY);
+ if (fd2 < 0)
+ continue;
+ err = st->ss->load_super(st, fd2, NULL);
+ close(fd2);
+ if (err)
+ continue;
+ break;
+ }
+ if (d == MAX_DISKS) {
+ pr_err("Unable to load metadata for %s\n",
+ devname);
+ ret_val = 1;
+ goto Grow_continue_command_exit;
+ }
+ st->ss->getinfo_super(st, content, NULL);
+
+ if (!content->reshape_active)
+ sleep(3);
+ else
+ break;
+ } while (cnt-- > 0);
} else {
char *container;
--
2.4.3
^ 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