* [GIT PULL REQUEST] md bugfix
From: Neil Brown @ 2015-10-11 5:30 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-raid, LKML, Mikulas Patocka
[-- Attachment #1: Type: text/plain, Size: 840 bytes --]
The following changes since commit da6fb7a9e5bd6f04f7e15070f630bdf1ea502841:
md/bitmap: don't pass -1 to bitmap_storage_alloc. (2015-10-02 17:24:13 +1000)
are available in the git repository at:
git://neil.brown.name/md tags/md/4.3-rc4-fix
for you to fetch changes up to a452744bcbf706eac65abb4c98496a366820c60a:
crash in md-raid1 and md-raid10 due to incorrect list manipulation (2015-10-09 08:33:46 +1100)
----------------------------------------------------------------
One bug fix for raid1/raid10.
Very careless bug earler in 4.3-rc, now fixed :-)
----------------------------------------------------------------
Mikulas Patocka (1):
crash in md-raid1 and md-raid10 due to incorrect list manipulation
drivers/md/raid1.c | 4 ++--
drivers/md/raid10.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH] md: fix 32-bit build warning
From: Neil Brown @ 2015-10-12 4:59 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-raid, linux-kernel, Goldwyn Rodrigues, linux-arm-kernel
In-Reply-To: <5304496.D8N6u074t0@wuerfel>
[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]
Arnd Bergmann <arnd@arndb.de> writes:
> On 32-bit architectures, the md code produces this warning when CONFIG_LDAF
> is set:
>
> drivers/md/md.c: In function 'check_sb_changes':
> drivers/md/md.c:8990:10: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'sector_t {aka long long unsigned int}' [-Wformat=]
> pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
>
> The code was only recently introduced, and uses the wrong format string
> for sector_t. As a workaround, this patch adds an explicit cast to 'u64'
> so we can use the %llu format string on all architectures.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: e0212320066e ("md-cluster: Improve md_reload_sb to be less error prone")
> Cc: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>
> I also noticed that some commmits in md/for-next including the one causing
> the problem lack a Signed-off-by line. It might make sense to just fold this
> patch and add the lines at the same time.
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 7fff1e6884d6..e13f72a3b561 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8987,9 +8987,9 @@ static void check_sb_changes(struct mddev *mddev, struct md_rdev *rdev)
>
> /* recovery_cp changed */
> if (le64_to_cpu(sb->resync_offset) != mddev->recovery_cp) {
> - pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
> - __LINE__, mddev->recovery_cp,
> - (unsigned long) le64_to_cpu(sb->resync_offset));
> + pr_info("%s:%d recovery_cp changed from %llu to %llu\n", __func__,
> + __LINE__, (u64)mddev->recovery_cp,
> + (u64) le64_to_cpu(sb->resync_offset));
> mddev->recovery_cp = le64_to_cpu(sb->resync_offset);
> }
>
Thanks, but is this really right?
I think u64 is "unsigned long" on 64bit.
I have always used (unsigned long long) when I want to use %llu on
sector_t.
How confident are you of using "u64" ?
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 1/9] MD: fix info output for journal disk
From: Neil Brown @ 2015-10-12 5:37 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <2537e2bcb31eaf918df28f25d096b31c2b9daa70.1444360850.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]
Shaohua Li <shli@fb.com> writes:
> journal disk can be faulty. The Journal and Faulty aren't exclusive with
> each other.
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/md.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0729cc7..daf42bb 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5857,7 +5857,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
> else if (test_bit(In_sync, &rdev->flags)) {
> info.state |= (1<<MD_DISK_ACTIVE);
> info.state |= (1<<MD_DISK_SYNC);
> - } else if (test_bit(Journal, &rdev->flags))
> + }
> + if (test_bit(Journal, &rdev->flags))
> info.state |= (1<<MD_DISK_JOURNAL);
> if (test_bit(WriteMostly, &rdev->flags))
> info.state |= (1<<MD_DISK_WRITEMOSTLY);
> @@ -7335,18 +7336,21 @@ static int md_seq_show(struct seq_file *seq, void *v)
> rcu_read_lock();
> rdev_for_each_rcu(rdev, mddev) {
> char b[BDEVNAME_SIZE];
> + bool skip = false;
> seq_printf(seq, " %s[%d]",
> bdevname(rdev->bdev,b), rdev->desc_nr);
> if (test_bit(WriteMostly, &rdev->flags))
> seq_printf(seq, "(W)");
> if (test_bit(Faulty, &rdev->flags)) {
> seq_printf(seq, "(F)");
> - continue;
> + skip = true;
> }
> if (test_bit(Journal, &rdev->flags)) {
> seq_printf(seq, "(J)");
> - continue;
> + skip = true;
> }
> + if (skip)
> + continue;
Is this 'skip' stuff really needed? What would go wrong if we just left
it out? An active journal will have raid_disk>=0 now won't it? And
if we don't support replacement of journals (which I suspect we
shouldn't) then (R) will never get reported.
So can we just drop the 'skip'??
Thanks,
NeilBrown
> if (rdev->raid_disk < 0)
> seq_printf(seq, "(S)"); /* spare */
> if (test_bit(Replacement, &rdev->flags))
> --
> 2.4.6
>
> --
> 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: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 2/9] raid5-cache: add trim support for log
From: Neil Brown @ 2015-10-12 6:00 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <effe8247d5013e4951fd7efa06cc45efe66a94c4.1444360850.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 5087 bytes --]
Shaohua Li <shli@fb.com> writes:
> Since superblock is updated infrequently, we do a simple trim of log
> disk (a synchronous trim)
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5-cache.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index caeec10..5dbe084 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -85,6 +85,7 @@ struct r5l_log {
> spinlock_t no_space_stripes_lock;
>
> bool need_cache_flush;
> + bool in_teardown;
> };
>
> /*
> @@ -644,6 +645,60 @@ void r5l_flush_stripe_to_raid(struct r5l_log *log)
> }
>
> static void r5l_write_super(struct r5l_log *log, sector_t cp);
> +static void r5l_write_super_and_discard_space(struct r5l_log *log,
> + sector_t end)
> +{
> + struct block_device *bdev = log->rdev->bdev;
> + struct mddev *mddev;
> +
> + r5l_write_super(log, end);
> +
> + if (!blk_queue_discard(bdev_get_queue(bdev)))
> + return;
> +
> + mddev = log->rdev->mddev;
> + /*
> + * This is to avoid a deadlock. r5l_quiesce holds reconfig_mutex and
> + * wait for this thread to finish. This thread waits for
> + * MD_CHANGE_PENDING clear, which is supposed to be done in
> + * md_check_recovery(). md_check_recovery() tries to get
> + * reconfig_mutex. Since r5l_quiesce already holds the mutex,
> + * md_check_recovery() fails, so the PENDING never get cleared. The
> + * in_teardown check workaround this issue.
> + * */
Thanks for this comment (but can we just end the comment with "*/"
instead of "* */" - that's what everyone else does).
It helps a lot. If feels a bit clumsy, but maybe that is just me. At
least I understand now and it make sense.
> + if (!log->in_teardown) {
> + set_bit(MD_CHANGE_DEVS, &mddev->flags);
> + set_bit(MD_CHANGE_PENDING, &mddev->flags);
> + md_wakeup_thread(mddev->thread);
> + wait_event(mddev->sb_wait,
> + !test_bit(MD_CHANGE_PENDING, &mddev->flags) ||
> + log->in_teardown);
> + /*
> + * r5l_quiesce could run after in_teardown check and hold
> + * mutex first. Superblock might get updated twice.
> + * */
> + if (log->in_teardown)
> + md_update_sb(mddev, 1);
> + } else {
> + BUG_ON(!mddev_is_locked(mddev));
> + md_update_sb(mddev, 1);
> + }
> +
> + if (log->last_checkpoint < end) {
> + blkdev_issue_discard(bdev,
> + log->last_checkpoint + log->rdev->data_offset,
> + end - log->last_checkpoint, GFP_NOIO, 0);
> + } else {
> + blkdev_issue_discard(bdev,
> + log->last_checkpoint + log->rdev->data_offset,
> + log->device_size - log->last_checkpoint,
> + GFP_NOIO, 0);
> + blkdev_issue_discard(bdev, log->rdev->data_offset, end,
> + GFP_NOIO, 0);
> + }
> +}
> +
> +
> static void r5l_do_reclaim(struct r5l_log *log)
> {
> sector_t reclaim_target = xchg(&log->reclaim_target, 0);
> @@ -685,7 +740,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
> * here, because the log area might be reused soon and we don't want to
> * confuse recovery
> */
> - r5l_write_super(log, next_checkpoint);
> + r5l_write_super_and_discard_space(log, next_checkpoint);
>
> mutex_lock(&log->io_mutex);
> log->last_checkpoint = next_checkpoint;
> @@ -721,9 +776,11 @@ static void r5l_wake_reclaim(struct r5l_log *log, sector_t space)
>
> void r5l_quiesce(struct r5l_log *log, int state)
> {
> + struct mddev *mddev;
> if (!log || state == 2)
> return;
> if (state == 0) {
> + log->in_teardown = 0;
> log->reclaim_thread = md_register_thread(r5l_reclaim_thread,
> log->rdev->mddev, "reclaim");
> } else if (state == 1) {
> @@ -731,6 +788,10 @@ void r5l_quiesce(struct r5l_log *log, int state)
> * at this point all stripes are finished, so io_unit is at
> * least in STRIPE_END state
> */
> + log->in_teardown = 1;
> + /* make sure r5l_write_super_and_discard_space exits */
> + mddev = log->rdev->mddev;
> + wake_up(&mddev->sb_wait);
> r5l_wake_reclaim(log, -1L);
> md_unregister_thread(&log->reclaim_thread);
I think this is racey (though you haven't changed the code, so it must
have been racy before).
There is no guarantee that the thread will actually wake up to handle
the reclaim before md_unregister_thread marks the thread as
kthread_should_stop().
Maybe the thing to do would be
md_unregister_thread()
log->reclaim_target = (unsigned long) -1;
r5l_do_reclaim(log);
or something like that. i.e. just do it synchronously.
Then.... maybe you don't need in_teardown.
When r5l_do_reclaim() is called from the thread, you wait.
When called from r5l_quiesce(), you md_update_sb().
Would that work?
Thanks,
NeilBrown
> r5l_do_reclaim(log);
> --
> 2.4.6
>
> --
> 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: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 0/9] raid5-cache fixes
From: Neil Brown @ 2015-10-12 6:17 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <cover.1444360850.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 533 bytes --]
Shaohua Li <shli@fb.com> writes:
> Hi,
>
> some fixes for raid5-cache/md. Mainly supports trim, handle IO error and
> handling array assemble with missing/faulty disks.
> patch 1: fix a bug
> patch 2: support trim
These two I've replied to separately.
> patch 3-4, IO error handling
> patch 5-9, handle assemble issue when journal disk is missing/faulty
These I am happy with (I think, it is getting late, my brain slows
down...)
I was going to apply them but they depend on the trim support to apply
cleanly.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH] md: fix 32-bit build warning
From: Arnd Bergmann @ 2015-10-12 9:30 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, linux-kernel, Goldwyn Rodrigues, linux-arm-kernel
In-Reply-To: <87fv1g1y34.fsf@notabene.neil.brown.name>
On Monday 12 October 2015 15:59:27 Neil Brown wrote:
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 7fff1e6884d6..e13f72a3b561 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -8987,9 +8987,9 @@ static void check_sb_changes(struct mddev *mddev, struct md_rdev *rdev)
> >
> > /* recovery_cp changed */
> > if (le64_to_cpu(sb->resync_offset) != mddev->recovery_cp) {
> > - pr_info("%s:%d recovery_cp changed from %lu to %lu\n", __func__,
> > - __LINE__, mddev->recovery_cp,
> > - (unsigned long) le64_to_cpu(sb->resync_offset));
> > + pr_info("%s:%d recovery_cp changed from %llu to %llu\n", __func__,
> > + __LINE__, (u64)mddev->recovery_cp,
> > + (u64) le64_to_cpu(sb->resync_offset));
> > mddev->recovery_cp = le64_to_cpu(sb->resync_offset);
> > }
> >
>
> Thanks, but is this really right?
> I think u64 is "unsigned long" on 64bit.
> I have always used (unsigned long long) when I want to use %llu on
> sector_t.
>
> How confident are you of using "u64" ?
Very confident ;-)
This used to not work until some linux-2.6 version when we changed all
architectures to use asm-generic/int-ll64.h in the kernel, because
a lot of code relied on printing u64 variables using %lld.
I tend to use u64 for things like this because it's shorter than
'unsigned long long'.
Arnd
^ permalink raw reply
* Scalability of MD raid 1 mirror devices
From: Suresh Babu Kandukuru @ 2015-10-12 12:03 UTC (permalink / raw)
To: linux-raid, neilb
In-Reply-To: <98ad0e44-1ac1-4451-a3d3-0a48f3a1d9e7@default>
Dear Group,
We are doing scalability of MD raid 1 mirror devices on the Linux host running the 3.17.2 . we see the number of RAID 1 devices limited to 128 in one case and 511 in another case . we would like to know why this limitation ?. Appreciate any kind of inputs and pointers
We can create the RAID 1 device in 3 ways.
1. /dev/mdX -> here X is the number, we can specify from 0 to 511.
2. /dev/md/X -> here also X is a number from 0 to 511. It creates it as a link to the actual device /dev/mdX. ( similar to above but creates a link also).
3. /dev/md/”name” -> This creates a link to actual device, whichever is free starting from 127 to 0. Below is the function which is responsible for it.
char *find_free_devnm(int use_partitions)
{
static char devnm[32];
int devnum;
for (devnum = 127; devnum != 128; devnum = devnum ? devnum-1 : (1<<20)-1) {
if (use_partitions)
sprintf(devnm, "md_d%d", devnum);
else
sprintf(devnm, "md%d", devnum);
if (mddev_busy(devnm))
continue;
if (!conf_name_is_free(devnm))
continue;
if (!use_udev()) {
/* make sure it is new to /dev too, at least as a
* non-standard */
int devid = devnm2devid(devnm);
if (devid) {
char *dn = map_dev(major(devid),
minor(devid), 0);
if (dn && ! is_standard(dn, NULL))
continue;
}
}
break;
}
if (devnum == 128)
return NULL;
return devnm;
}
So ideally we should not create a device which is more than 128 { The program may crash }.
Then we tried to find how we are able to create up to 511 and why it is failing after that.
int dev_open(char *dev, int flags)
inside this function
fd = open(dev, flags); / this line is assigning fd to -1 , which is causing the program to fail. So I wrote a simple program to crosscheck it.
int main(){
char devname[32] = "/dev/hello1";
// The flags I have set according to the code.
int flags = O_RDWR;
flags |= O_DIRECT;
if (mknod(devname, S_IFBLK|0600, makedev(9,511)) == 0) {
int fd = open(devname, flags);
cout<<fd<<endl;
unlink(devname);
}
}
So if the minor number is more than 511, the “fd” is assigned to -1, if it is in the range of 0 to 511 It is working fine.
So should we go with the range of 511 or stick to 128 ?
Thanks
/Suresh
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] md: workqueue: Remove WQ_CPU_INTENSIVE from unbound workqueue allocations
From: Tejun Heo @ 2015-10-12 16:21 UTC (permalink / raw)
To: Xunlei Pang
Cc: linux-kernel, linux-raid, Alasdair Kergon, Mike Snitzer,
Mikulas Patocka, Neil Brown, Xunlei Pang
In-Reply-To: <1444361135-22624-1-git-send-email-xlpang@126.com>
On Fri, Oct 09, 2015 at 11:25:35AM +0800, Xunlei Pang wrote:
> From: Xunlei Pang <pang.xunlei@linaro.org>
>
> WQ_CPU_INTENSIVE is meaningless for the unbound workqueue, so remove it.
>
> Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 2/9] raid5-cache: add trim support for log
From: Shaohua Li @ 2015-10-12 16:57 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <877fms1v9t.fsf@notabene.neil.brown.name>
On Mon, Oct 12, 2015 at 05:00:14PM +1100, Neil Brown wrote:
> Shaohua Li <shli@fb.com> writes:
>
> > Since superblock is updated infrequently, we do a simple trim of log
> > disk (a synchronous trim)
> >
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> > drivers/md/raid5-cache.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 62 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> > index caeec10..5dbe084 100644
> > --- a/drivers/md/raid5-cache.c
> > +++ b/drivers/md/raid5-cache.c
> > @@ -85,6 +85,7 @@ struct r5l_log {
> > spinlock_t no_space_stripes_lock;
> >
> > bool need_cache_flush;
> > + bool in_teardown;
> > };
> >
> > /*
> > @@ -644,6 +645,60 @@ void r5l_flush_stripe_to_raid(struct r5l_log *log)
> > }
> >
> > static void r5l_write_super(struct r5l_log *log, sector_t cp);
> > +static void r5l_write_super_and_discard_space(struct r5l_log *log,
> > + sector_t end)
> > +{
> > + struct block_device *bdev = log->rdev->bdev;
> > + struct mddev *mddev;
> > +
> > + r5l_write_super(log, end);
> > +
> > + if (!blk_queue_discard(bdev_get_queue(bdev)))
> > + return;
> > +
> > + mddev = log->rdev->mddev;
> > + /*
> > + * This is to avoid a deadlock. r5l_quiesce holds reconfig_mutex and
> > + * wait for this thread to finish. This thread waits for
> > + * MD_CHANGE_PENDING clear, which is supposed to be done in
> > + * md_check_recovery(). md_check_recovery() tries to get
> > + * reconfig_mutex. Since r5l_quiesce already holds the mutex,
> > + * md_check_recovery() fails, so the PENDING never get cleared. The
> > + * in_teardown check workaround this issue.
> > + * */
>
> Thanks for this comment (but can we just end the comment with "*/"
> instead of "* */" - that's what everyone else does).
sorry, I will set vim correctly.
> It helps a lot. If feels a bit clumsy, but maybe that is just me. At
> least I understand now and it make sense.
> > + if (!log->in_teardown) {
> > + set_bit(MD_CHANGE_DEVS, &mddev->flags);
> > + set_bit(MD_CHANGE_PENDING, &mddev->flags);
> > + md_wakeup_thread(mddev->thread);
> > + wait_event(mddev->sb_wait,
> > + !test_bit(MD_CHANGE_PENDING, &mddev->flags) ||
> > + log->in_teardown);
> > + /*
> > + * r5l_quiesce could run after in_teardown check and hold
> > + * mutex first. Superblock might get updated twice.
> > + * */
> > + if (log->in_teardown)
> > + md_update_sb(mddev, 1);
> > + } else {
> > + BUG_ON(!mddev_is_locked(mddev));
> > + md_update_sb(mddev, 1);
> > + }
> > +
> > + if (log->last_checkpoint < end) {
> > + blkdev_issue_discard(bdev,
> > + log->last_checkpoint + log->rdev->data_offset,
> > + end - log->last_checkpoint, GFP_NOIO, 0);
> > + } else {
> > + blkdev_issue_discard(bdev,
> > + log->last_checkpoint + log->rdev->data_offset,
> > + log->device_size - log->last_checkpoint,
> > + GFP_NOIO, 0);
> > + blkdev_issue_discard(bdev, log->rdev->data_offset, end,
> > + GFP_NOIO, 0);
> > + }
> > +}
> > +
> > +
> > static void r5l_do_reclaim(struct r5l_log *log)
> > {
> > sector_t reclaim_target = xchg(&log->reclaim_target, 0);
> > @@ -685,7 +740,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
> > * here, because the log area might be reused soon and we don't want to
> > * confuse recovery
> > */
> > - r5l_write_super(log, next_checkpoint);
> > + r5l_write_super_and_discard_space(log, next_checkpoint);
> >
> > mutex_lock(&log->io_mutex);
> > log->last_checkpoint = next_checkpoint;
> > @@ -721,9 +776,11 @@ static void r5l_wake_reclaim(struct r5l_log *log, sector_t space)
> >
> > void r5l_quiesce(struct r5l_log *log, int state)
> > {
> > + struct mddev *mddev;
> > if (!log || state == 2)
> > return;
> > if (state == 0) {
> > + log->in_teardown = 0;
> > log->reclaim_thread = md_register_thread(r5l_reclaim_thread,
> > log->rdev->mddev, "reclaim");
> > } else if (state == 1) {
> > @@ -731,6 +788,10 @@ void r5l_quiesce(struct r5l_log *log, int state)
> > * at this point all stripes are finished, so io_unit is at
> > * least in STRIPE_END state
> > */
> > + log->in_teardown = 1;
> > + /* make sure r5l_write_super_and_discard_space exits */
> > + mddev = log->rdev->mddev;
> > + wake_up(&mddev->sb_wait);
> > r5l_wake_reclaim(log, -1L);
> > md_unregister_thread(&log->reclaim_thread);
>
> I think this is racey (though you haven't changed the code, so it must
> have been racy before).
> There is no guarantee that the thread will actually wake up to handle
> the reclaim before md_unregister_thread marks the thread as
> kthread_should_stop().
> Maybe the thing to do would be
> md_unregister_thread()
> log->reclaim_target = (unsigned long) -1;
> r5l_do_reclaim(log);
>
> or something like that. i.e. just do it synchronously.
This doesn't sound like a race. If the thread doesn't run because it
exits after kthread_should_stop check, the reclaim_target should remain
to be -1 after md_unregister_thread
> Then.... maybe you don't need in_teardown.
> When r5l_do_reclaim() is called from the thread, you wait.
> When called from r5l_quiesce(), you md_update_sb().
>
> Would that work?
It wouldn't. This only fixes the deadlock issue r5l_quiesce calls
r5l_do_reclaim() directly. The problem is r5l_wake_reclaim() will wake
the thread, md_unregister_thread will wait till the thread exits. At
that time the thread might try to lock the reconfig_mutex. Since
r5l_quesce already holds the mutex, the md_unregister_thread never
returns. In summary, r5l_do_reclaim() is called from the thread can't be
used to determine whether we need the deadlock avoidness.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 1/9] MD: fix info output for journal disk
From: Shaohua Li @ 2015-10-12 17:01 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <87a8ro1wb0.fsf@notabene.neil.brown.name>
On Mon, Oct 12, 2015 at 04:37:55PM +1100, Neil Brown wrote:
> Shaohua Li <shli@fb.com> writes:
>
> > journal disk can be faulty. The Journal and Faulty aren't exclusive with
> > each other.
> >
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> > drivers/md/md.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 0729cc7..daf42bb 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -5857,7 +5857,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
> > else if (test_bit(In_sync, &rdev->flags)) {
> > info.state |= (1<<MD_DISK_ACTIVE);
> > info.state |= (1<<MD_DISK_SYNC);
> > - } else if (test_bit(Journal, &rdev->flags))
> > + }
> > + if (test_bit(Journal, &rdev->flags))
> > info.state |= (1<<MD_DISK_JOURNAL);
> > if (test_bit(WriteMostly, &rdev->flags))
> > info.state |= (1<<MD_DISK_WRITEMOSTLY);
> > @@ -7335,18 +7336,21 @@ static int md_seq_show(struct seq_file *seq, void *v)
> > rcu_read_lock();
> > rdev_for_each_rcu(rdev, mddev) {
> > char b[BDEVNAME_SIZE];
> > + bool skip = false;
> > seq_printf(seq, " %s[%d]",
> > bdevname(rdev->bdev,b), rdev->desc_nr);
> > if (test_bit(WriteMostly, &rdev->flags))
> > seq_printf(seq, "(W)");
> > if (test_bit(Faulty, &rdev->flags)) {
> > seq_printf(seq, "(F)");
> > - continue;
> > + skip = true;
> > }
> > if (test_bit(Journal, &rdev->flags)) {
> > seq_printf(seq, "(J)");
> > - continue;
> > + skip = true;
> > }
> > + if (skip)
> > + continue;
>
> Is this 'skip' stuff really needed? What would go wrong if we just left
> it out? An active journal will have raid_disk>=0 now won't it? And
> if we don't support replacement of journals (which I suspect we
> shouldn't) then (R) will never get reported.
>
> So can we just drop the 'skip'??
Sounds good. Let me know if I should resend this one alone or the
series.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 1/9] MD: fix info output for journal disk
From: Neil Brown @ 2015-10-12 23:26 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20151012170148.GA3081164@devbig084.prn1.facebook.com>
[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]
Shaohua Li <shli@fb.com> writes:
> On Mon, Oct 12, 2015 at 04:37:55PM +1100, Neil Brown wrote:
>> Shaohua Li <shli@fb.com> writes:
>>
>> > journal disk can be faulty. The Journal and Faulty aren't exclusive with
>> > each other.
>> >
>> > Signed-off-by: Shaohua Li <shli@fb.com>
>> > ---
>> > drivers/md/md.c | 10 +++++++---
>> > 1 file changed, 7 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/md/md.c b/drivers/md/md.c
>> > index 0729cc7..daf42bb 100644
>> > --- a/drivers/md/md.c
>> > +++ b/drivers/md/md.c
>> > @@ -5857,7 +5857,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
>> > else if (test_bit(In_sync, &rdev->flags)) {
>> > info.state |= (1<<MD_DISK_ACTIVE);
>> > info.state |= (1<<MD_DISK_SYNC);
>> > - } else if (test_bit(Journal, &rdev->flags))
>> > + }
>> > + if (test_bit(Journal, &rdev->flags))
>> > info.state |= (1<<MD_DISK_JOURNAL);
>> > if (test_bit(WriteMostly, &rdev->flags))
>> > info.state |= (1<<MD_DISK_WRITEMOSTLY);
>> > @@ -7335,18 +7336,21 @@ static int md_seq_show(struct seq_file *seq, void *v)
>> > rcu_read_lock();
>> > rdev_for_each_rcu(rdev, mddev) {
>> > char b[BDEVNAME_SIZE];
>> > + bool skip = false;
>> > seq_printf(seq, " %s[%d]",
>> > bdevname(rdev->bdev,b), rdev->desc_nr);
>> > if (test_bit(WriteMostly, &rdev->flags))
>> > seq_printf(seq, "(W)");
>> > if (test_bit(Faulty, &rdev->flags)) {
>> > seq_printf(seq, "(F)");
>> > - continue;
>> > + skip = true;
>> > }
>> > if (test_bit(Journal, &rdev->flags)) {
>> > seq_printf(seq, "(J)");
>> > - continue;
>> > + skip = true;
>> > }
>> > + if (skip)
>> > + continue;
>>
>> Is this 'skip' stuff really needed? What would go wrong if we just left
>> it out? An active journal will have raid_disk>=0 now won't it? And
>> if we don't support replacement of journals (which I suspect we
>> shouldn't) then (R) will never get reported.
>>
>> So can we just drop the 'skip'??
>
> Sounds good. Let me know if I should resend this one alone or the
> series.
Just send this one alone. I'll apply the rest from the previous
posting.
I'm not entirely convinced about the race issue, but I don't really
want to think about it today and as you seem certain we will go with the
current code for now.
If I think of something that does convince me, it can always be fixed
later.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 1/9] MD: fix info output for journal disk
From: Shaohua Li @ 2015-10-12 23:38 UTC (permalink / raw)
To: Neil Brown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <87si5fzn1r.fsf@notabene.neil.brown.name>
On Tue, Oct 13, 2015 at 10:26:08AM +1100, Neil Brown wrote:
> Shaohua Li <shli@fb.com> writes:
>
> > On Mon, Oct 12, 2015 at 04:37:55PM +1100, Neil Brown wrote:
> >> Shaohua Li <shli@fb.com> writes:
> >>
> >> > journal disk can be faulty. The Journal and Faulty aren't exclusive with
> >> > each other.
> >> >
> >> > Signed-off-by: Shaohua Li <shli@fb.com>
> >> > ---
> >> > drivers/md/md.c | 10 +++++++---
> >> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> > index 0729cc7..daf42bb 100644
> >> > --- a/drivers/md/md.c
> >> > +++ b/drivers/md/md.c
> >> > @@ -5857,7 +5857,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
> >> > else if (test_bit(In_sync, &rdev->flags)) {
> >> > info.state |= (1<<MD_DISK_ACTIVE);
> >> > info.state |= (1<<MD_DISK_SYNC);
> >> > - } else if (test_bit(Journal, &rdev->flags))
> >> > + }
> >> > + if (test_bit(Journal, &rdev->flags))
> >> > info.state |= (1<<MD_DISK_JOURNAL);
> >> > if (test_bit(WriteMostly, &rdev->flags))
> >> > info.state |= (1<<MD_DISK_WRITEMOSTLY);
> >> > @@ -7335,18 +7336,21 @@ static int md_seq_show(struct seq_file *seq, void *v)
> >> > rcu_read_lock();
> >> > rdev_for_each_rcu(rdev, mddev) {
> >> > char b[BDEVNAME_SIZE];
> >> > + bool skip = false;
> >> > seq_printf(seq, " %s[%d]",
> >> > bdevname(rdev->bdev,b), rdev->desc_nr);
> >> > if (test_bit(WriteMostly, &rdev->flags))
> >> > seq_printf(seq, "(W)");
> >> > if (test_bit(Faulty, &rdev->flags)) {
> >> > seq_printf(seq, "(F)");
> >> > - continue;
> >> > + skip = true;
> >> > }
> >> > if (test_bit(Journal, &rdev->flags)) {
> >> > seq_printf(seq, "(J)");
> >> > - continue;
> >> > + skip = true;
> >> > }
> >> > + if (skip)
> >> > + continue;
> >>
> >> Is this 'skip' stuff really needed? What would go wrong if we just left
> >> it out? An active journal will have raid_disk>=0 now won't it? And
> >> if we don't support replacement of journals (which I suspect we
> >> shouldn't) then (R) will never get reported.
> >>
> >> So can we just drop the 'skip'??
> >
> > Sounds good. Let me know if I should resend this one alone or the
> > series.
>
> Just send this one alone. I'll apply the rest from the previous
> posting.
>
> I'm not entirely convinced about the race issue, but I don't really
> want to think about it today and as you seem certain we will go with the
> current code for now.
> If I think of something that does convince me, it can always be fixed
> later.
Cool, thanks!
^ permalink raw reply
* [PATCH V2] MD: fix info output for journal disk
From: Shaohua Li @ 2015-10-12 23:59 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
journal disk can be faulty. The Journal and Faulty aren't exclusive with
each other.
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/md.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0729cc7..f1114a6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5857,7 +5857,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
else if (test_bit(In_sync, &rdev->flags)) {
info.state |= (1<<MD_DISK_ACTIVE);
info.state |= (1<<MD_DISK_SYNC);
- } else if (test_bit(Journal, &rdev->flags))
+ }
+ if (test_bit(Journal, &rdev->flags))
info.state |= (1<<MD_DISK_JOURNAL);
if (test_bit(WriteMostly, &rdev->flags))
info.state |= (1<<MD_DISK_WRITEMOSTLY);
@@ -7339,14 +7340,12 @@ static int md_seq_show(struct seq_file *seq, void *v)
bdevname(rdev->bdev,b), rdev->desc_nr);
if (test_bit(WriteMostly, &rdev->flags))
seq_printf(seq, "(W)");
+ if (test_bit(Journal, &rdev->flags))
+ seq_printf(seq, "(J)");
if (test_bit(Faulty, &rdev->flags)) {
seq_printf(seq, "(F)");
continue;
}
- if (test_bit(Journal, &rdev->flags)) {
- seq_printf(seq, "(J)");
- continue;
- }
if (rdev->raid_disk < 0)
seq_printf(seq, "(S)"); /* spare */
if (test_bit(Replacement, &rdev->flags))
--
2.4.6
^ permalink raw reply related
* Re: Puzzled by raid 10 (f2) and raid 6 direct i/o read behaviour
From: Dragan Milivojević @ 2015-10-13 0:31 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <CALtW_ajvFR0dnxyH3XBmuosM2J7vFEGU1AQiNEtoqjvT0HvYRA@mail.gmail.com>
Feeling stupid to reply to my own email, hopefully nobody started to write a
reply. Maybe someone else will find it useful. Problem comes down to latency.
Second time this month that the same problem bites me :)
If one disk latency is 100us when issuing small reads with direct io and
iodepth of 1 the same disk has to wait for other to complete their reads.
In a way this makes single disk latency x times the disk array.
This is also the reason why full stripe reads are (almost) full speed.
I/O gets submitted concurrently. I wonder why don't I see any improvement with
readahead, direct i/o doesn't use it?
Sorry if a wasted someones time, maybe someones will find it useful :)
Dragan
On Mon, Oct 12, 2015 at 10:59 PM, Dragan Milivojević
<galileo@pkm-inc.com> wrote:
> Hi all
>
> I'm currently building a NAS/SAN server and I'm not quite sure why I'm getting
> these results.
> I would appreciate if someone could give me a theoretical explanation on why
> this is happening. Raid arrays were built from 7 drives with 128k chunk.
> I'm mostly concerned with read speed and small block direct i/o.
>
> Full raid and test details are attached and also uploaded (for easy viewing)
> at: http://pastebin.com/VeGr0Ehc
>
> Summary of results from a few tests:
>
> == Hard disk, Sequential read, O_DIRECT
> ----
> bs: 4k, bw: 80833KB/s
> bs: 32k, bw: 152000KB/s
> bs: 128k, bw: 152809KB/s
> ----
>
> == Hard disk, Sequential write, O_DIRECT
> ----
> bs: 16k, bw: 144392KB/s
> bs: 32k, bw: 145352KB/s
> bs: 128k, bw: 145433KB/s
> ----
>
> == Raid 10 f2, chunk: 128k, Sequential read, O_DIRECT
> ----
> bs: 4k, bw: 78136KB/s, per disk bw: 11MB/s, disk avgrq-sz: 8
> bs: 32k, bw: 289847KB/s, per disk bw: 41MB/s, disk avgrq-sz: 64
> bs: 128k, bw: 409674KB/s, per disk bw: 57MB/s, disk avgrq-sz: 256
> bs: 256k, bw: 739344KB/s, per disk bw: 104MB/s, disk avgrq-sz: 256
> bs: 896k, bw: 981517KB/s, per disk bw: 133MB/s, disk avgrq-sz: 256
> ----
>
> == Raid 6, chunk: 128k, Sequential read, O_DIRECT
> ----
> bs: 4k, bw: 46376KB/s, per disk bw: 6MB/s, disk avgrq-sz: 8
> bs: 32k, bw: 155531KB/s, per disk bw: 22MB/s, disk avgrq-sz: 64
> bs: 128k, bw: 182024KB/s, per disk bw: 26MB/s, disk avgrq-sz: 256
> bs: 256k, bw: 248591KB/s, per disk bw: 34MB/s, disk avgrq-sz: 256
> bs: 384k, bw: 299771KB/s, per disk bw: 40MB/s, disk avgrq-sz: 256
> bs: 512k, bw: 315374KB/s, per disk bw: 44MB/s, disk avgrq-sz: 256
> bs: 640k, bw: 296350KB/s, per disk bw: 44MB/s, disk avgrq-sz: 256
> bs: 1280k, bw: 543655KB/s, per disk bw: 75MB/s, disk avgrq-sz: 426
> bs: 2560k, bw: 618092KB/s, per disk bw: 83MB/s, disk avgrq-sz: 638
> ----
>
> == Raid 6, chunk: 128k, Sequential read, Buffered
> ----
> bs: 2560k, bw: 690546KB/s, per disk bw: 96MB/s, disk avgrq-sz: 512
> ----
>
> == Raid 6, chunk: 128k, Sequential write, O_DIRECT, stripe_cache_size: 32768
> ----
> bs: 640K, bw: 382778KB/s, per disk bw: 75MB/s, disk avgrq-sz: 256
> bs: 1280K, bw: 405909KB/s, per disk bw: 82MB/s, disk avgrq-sz: 512
> ----
>
> == Raid 6, chunk: 128k, Sequential write, Buffered, stripe_cache_size: 32768
> ----
> bs: 1280k, bw: 730527KB/s, per disk bw: 135M/s, disk avgrq-sz: 1024
>
>
> As can be seen from single disk tests, hard drives are capable of full speed
> with 32k block size. What baffles me (especially with raid 10) is why do
> I get such low speed with small request sizes?
>
> Even with full stripe reads performance is not at maximum. This is most
> obvious with raid6. If I get the theory correctly maximum read speed per disk
> in this configuration should be around 105Mb/s (0.7*150).
>
> With buffered reads I'm close to this figure so theory matches the real
> performance with buffered reads.
>
> When thinking about writes one can easily grasp the theory behind full stripe
> writes and why block writes less than full stripe incur a performance hit.
>
> I don really get why I'm getting a performance hit with reads. I get it that
> for every 7 chunks written to a single disk 2 of those will be parity. So when
> reading the disk I'm expecting a 30% hit?
>
> Raid 10 (f2) results confuse me even more. If I understand it correctly parity
> is written at the end of disk so (in theory) there should be no performance
> hit with raid 10?
>
> I'm also seeing similar performance figures with real world usage (windoze
> clients with iscsi and mpio). Per disk disk bw figures remind me more of
> random then sequential i/o.
>
> The only theory that I came up with revolves around read request merges and
> disk firmware. I'm seeing read request merges with fairly large block sizes
> and even greater with buffered io. I'm not that familiar with kernel block
> internals so I'm seeking an authoritative answer.
>
> Can someone point our some docs that I can read or offer an explanation?
>
> Thanks
> Dragan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] dm snapshot: fix memory leak
From: Sudip Mukherjee @ 2015-10-13 8:03 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, Neil Brown
Cc: linux-kernel, dm-devel, linux-raid, Sudip Mukherjee
If wrong option is given then we are returning but we missed releasing
the workqueue and pstore.
Add an error path and use it to correctly release resources on failure.
Fixes: b0d3cc011e53 ("dm snapshot: add new persistent store option to support overflow")
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/md/dm-snap-persistent.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index 229be55..5ff7c23 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -847,6 +847,7 @@ static void persistent_drop_snapshot(struct dm_exception_store *store)
static int persistent_ctr(struct dm_exception_store *store, char *options)
{
struct pstore *ps;
+ int ret;
/* allocate the pstore */
ps = kzalloc(sizeof(*ps), GFP_KERNEL);
@@ -868,9 +869,9 @@ static int persistent_ctr(struct dm_exception_store *store, char *options)
ps->metadata_wq = alloc_workqueue("ksnaphd", WQ_MEM_RECLAIM, 0);
if (!ps->metadata_wq) {
- kfree(ps);
DMERR("couldn't start header metadata update thread");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_free_ps;
}
if (options) {
@@ -879,13 +880,20 @@ static int persistent_ctr(struct dm_exception_store *store, char *options)
store->userspace_supports_overflow = true;
else {
DMERR("Unsupported persistent store option: %s", options);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_option;
}
}
store->context = ps;
return 0;
+
+err_option:
+ destroy_workqueue(ps->metadata_wq);
+err_free_ps:
+ kfree(ps);
+ return ret;
}
static unsigned persistent_status(struct dm_exception_store *store,
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 4/6] md: don't export log device
From: Christoph Hellwig @ 2015-10-13 12:07 UTC (permalink / raw)
To: Neil Brown
Cc: Shaohua Li, linux-raid, Kernel-team, songliubraving, hch,
dan.j.williams
In-Reply-To: <8737xl7v5l.fsf@notabene.neil.brown.name>
On Thu, Oct 08, 2015 at 05:04:54PM +1100, Neil Brown wrote:
> Having two disks with ->raid_disk==0 does seem a little weird, but we do
> already have that in some cases.
> When you have a hot-replace going, both the original and the replacement
> have the same ->raid_disk numbers. They can be distinguished by the
> Replacement flag.
> I'm suggesting the same (sort of) for journals, and distinguish by the
> Journal flag.
>
> I did quick audit and just found setup_conf, run() and md_update_sb().
> If you could do an audit to that would be good. I'd be surprised if you
> find many more places where Journal needs to be tested with ->raid_disk.
Overloading positive numbers for the journal disk sounds like a bad idea
to me as it will cause a lot of confusion. I'd rather assign specific
negative values to special roles outside the actual rate. This will
require an initial audit, but give us nicely understandable rules later
on.
^ permalink raw reply
* [PATCH] dm snapshot: fix memory leak in error path
From: Michael Opdenacker @ 2015-10-13 12:36 UTC (permalink / raw)
To: agk, snitzer
Cc: dm-devel, neilb, linux-raid, linux-kernel, Michael Opdenacker
Fix a memory leak in an error path (reported by Coverity CID 1326059)
Also add a missing workqueue deallocation in the same error path.
Signed-off-by: Michael Opdenacker <michael.opdenacker@free-electrons.com>
---
drivers/md/dm-snap-persistent.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index aeacad9be51d..1e980fa8bb8b 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -879,6 +879,8 @@ static int persistent_ctr(struct dm_exception_store *store, char *options)
store->userspace_supports_overflow = true;
else {
DMERR("Unsupported persistent store option: %s", options);
+ destroy_workqueue(ps->metadata_wq);
+ kfree(ps);
return -EINVAL;
}
}
--
2.1.4
^ permalink raw reply related
* Fwd: First 12Mb of data missing after accidental deletion of first drive (4 2TB raid5)
From: Marek @ 2015-10-13 13:44 UTC (permalink / raw)
To: linux-raid
Hi,
I'd like to ask about recovering partitions saved as images on eg a raid.
How do I recover such image - I tried to run test disk and it found
lots of images which I wasn't able to view via P (NFTS) and ext3
partitions but I'd like to save them to another harddrive, I tried
photo tree with only ext3 superblock and image file enabled but it
wouldn't find the partition in was looki for and found via testdisk.
What would you recommend me to do?
Marek
^ permalink raw reply
* Re: First 12Mb of data missing after accidental deletion of first drive (4 2TB raid5)
From: Alexander Afonyashin @ 2015-10-13 13:53 UTC (permalink / raw)
To: Marek; +Cc: Linux-RAID
In-Reply-To: <CA+sqOsZSmYUDvwWu2MY-EpQyePGmT6wBjCCR5kKPEygKByD=Xw@mail.gmail.com>
Hi Marek,
Do you mean you have files (on raid) that were used as loop devices?
Regards,
Alexander
On Tue, Oct 13, 2015 at 4:44 PM, Marek <mlf.conv@gmail.com> wrote:
> Hi,
>
> I'd like to ask about recovering partitions saved as images on eg a raid.
> How do I recover such image - I tried to run test disk and it found
> lots of images which I wasn't able to view via P (NFTS) and ext3
> partitions but I'd like to save them to another harddrive, I tried
> photo tree with only ext3 superblock and image file enabled but it
> wouldn't find the partition in was looki for and found via testdisk.
> What would you recommend me to do?
>
> Marek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: First 12Mb of data missing after accidental deletion of first drive (4 2TB raid5)
From: Marek @ 2015-10-13 14:04 UTC (permalink / raw)
To: Alexander Afonyashin; +Cc: Linux-RAID
In-Reply-To: <CAETWcfvePZiUBiCY-cWMktQ=F7rckKt-7YDGa6PsiNt6j1ONGg@mail.gmail.com>
no, i had a working raid consisting of 4 2TB drives (raid5)
sda,sdb,sdc,sdb ( no partitions were created on drives) - then i
accidentally deleted /dev/sda by installing ubuntu. when inspecting
dev/md127 with a hex editor i discovered first 12mb are missing.
On Tue, Oct 13, 2015 at 3:53 PM, Alexander Afonyashin
<a.afonyashin@madnet-team.ru> wrote:
> Hi Marek,
>
> Do you mean you have files (on raid) that were used as loop devices?
>
> Regards,
> Alexander
>
> On Tue, Oct 13, 2015 at 4:44 PM, Marek <mlf.conv@gmail.com> wrote:
>> Hi,
>>
>> I'd like to ask about recovering partitions saved as images on eg a raid.
>> How do I recover such image - I tried to run test disk and it found
>> lots of images which I wasn't able to view via P (NFTS) and ext3
>> partitions but I'd like to save them to another harddrive, I tried
>> photo tree with only ext3 superblock and image file enabled but it
>> wouldn't find the partition in was looki for and found via testdisk.
>> What would you recommend me to do?
>>
>> Marek
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: First 12Mb of data missing after accidental deletion of first drive (4 2TB raid5)
From: Marek @ 2015-10-13 14:08 UTC (permalink / raw)
To: Alexander Afonyashin; +Cc: Linux-RAID
In-Reply-To: <CA+sqOsaBm4jXrEpNyRMBoSMjE7f8MdAnq9Na3WcrLvVdAB7vSw@mail.gmail.com>
sorry i pasted the wrong email:
Hi,
I have a problem with my 4 2TB disk Raid5 (most likely btrfs) after I
accidentally deleted first drive by installing Ubuntu on it (/dev/sda
automatic choice) then I discovered the raid isn't working and by
closer inspecting it hexdump I discovered first ~12Mb of data are
missing. Is it possible to recover it somehow? What I did was to zero
superblock then add the first drive - everything seems fine according
to /proc/mdstat.
On Tue, Oct 13, 2015 at 4:04 PM, Marek <mlf.conv@gmail.com> wrote:
> no, i had a working raid consisting of 4 2TB drives (raid5)
> sda,sdb,sdc,sdb ( no partitions were created on drives) - then i
> accidentally deleted /dev/sda by installing ubuntu. when inspecting
> dev/md127 with a hex editor i discovered first 12mb are missing.
>
> On Tue, Oct 13, 2015 at 3:53 PM, Alexander Afonyashin
> <a.afonyashin@madnet-team.ru> wrote:
>> Hi Marek,
>>
>> Do you mean you have files (on raid) that were used as loop devices?
>>
>> Regards,
>> Alexander
>>
>> On Tue, Oct 13, 2015 at 4:44 PM, Marek <mlf.conv@gmail.com> wrote:
>>> Hi,
>>>
>>> I'd like to ask about recovering partitions saved as images on eg a raid.
>>> How do I recover such image - I tried to run test disk and it found
>>> lots of images which I wasn't able to view via P (NFTS) and ext3
>>> partitions but I'd like to save them to another harddrive, I tried
>>> photo tree with only ext3 superblock and image file enabled but it
>>> wouldn't find the partition in was looki for and found via testdisk.
>>> What would you recommend me to do?
>>>
>>> Marek
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 00/14] md-cluster: A better way for METADATA_UPDATED processing
From: rgoldwyn @ 2015-10-13 14:25 UTC (permalink / raw)
To: linux-raid, neilb; +Cc: gqjiang
The processing of METADATA_UPDATED message is too simple and prone to
errors. Besides, it would not update the internal data structures as
required.
This set of patches reads the superblock from one of the device of the MD
and checks for changes in the in-memory data structures. If there is a change,
it performs the necessary actions to keep the internal data structures
as it would be in the primary node.
An example is if a devices turns faulty. The algorithm is:
1. The initiator node marks the device as faulty and updates the superblock
2. The initiator node sends METADATA_UPDATED with an advisory device number to the rest of the nodes.
3. The receiving node on receiving the METADATA_UPDATED message
3.1 Reads the superblock
3.2 Detects a device has failed by comparing with memory structure
3.3 Calls the necessary functions to record the failure and get the device out of the active array.
3.4 Acknowledges the message.
The patch series also fixes adding the disk which was impacted because of
the changes.
Patches can also be found at
https://github.com/goldwynr/linux branch md-next
Changes since V2:
- Fix status synchrnoization after --add and --re-add operations
- Included Guoqing's patches on endian correctness, zeroing cmsg etc
- Restructure add_new_disk() and cancel()
^ permalink raw reply
* [PATCH 01/14] md-cluster: Wake up suspended process
From: rgoldwyn @ 2015-10-13 14:25 UTC (permalink / raw)
To: linux-raid, neilb; +Cc: gqjiang, Goldwyn Rodrigues
In-Reply-To: <1444746335-22624-1-git-send-email-rgoldwyn@suse.de>
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
When the suspended_area is deleted, the suspended processes
must be woken up in order to complete their I/O.
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
drivers/md/md-cluster.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 51e8552..58eadc0 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -366,11 +366,13 @@ static void __remove_suspend_info(struct md_cluster_info *cinfo, int slot)
}
}
-static void remove_suspend_info(struct md_cluster_info *cinfo, int slot)
+static void remove_suspend_info(struct mddev *mddev, int slot)
{
+ struct md_cluster_info *cinfo = mddev->cluster_info;
spin_lock_irq(&cinfo->suspend_lock);
__remove_suspend_info(cinfo, slot);
spin_unlock_irq(&cinfo->suspend_lock);
+ mddev->pers->quiesce(mddev, 2);
}
@@ -381,7 +383,7 @@ static void process_suspend_info(struct mddev *mddev,
struct suspend_info *s;
if (!hi) {
- remove_suspend_info(cinfo, slot);
+ remove_suspend_info(mddev, slot);
return;
}
s = kzalloc(sizeof(struct suspend_info), GFP_KERNEL);
@@ -397,6 +399,7 @@ static void process_suspend_info(struct mddev *mddev,
__remove_suspend_info(cinfo, slot);
list_add(&s->list, &cinfo->suspend_list);
spin_unlock_irq(&cinfo->suspend_lock);
+ mddev->pers->quiesce(mddev, 2);
}
static void process_add_new_disk(struct mddev *mddev, struct cluster_msg *cmsg)
--
1.8.5.6
^ permalink raw reply related
* [PATCH 02/14] md: remove_and_add_spares() to activate specific rdev
From: rgoldwyn @ 2015-10-13 14:25 UTC (permalink / raw)
To: linux-raid, neilb; +Cc: gqjiang, Goldwyn Rodrigues
In-Reply-To: <1444746335-22624-1-git-send-email-rgoldwyn@suse.de>
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
remove_and_add_spares() checks for all devices to activate spare.
Change it to activate a specific device if a non-null rdev
argument is passed.
remove_and_add_spares() can be used to activate spares in
slot_store() as well.
For hot_remove_disk(), check if rdev->raid_disk == -1 before
calling remove_and_add_spares()
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
drivers/md/md.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 9798a99..e21a2fe 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2691,15 +2691,9 @@ slot_store(struct md_rdev *rdev, const char *buf, size_t len)
rdev->saved_raid_disk = -1;
clear_bit(In_sync, &rdev->flags);
clear_bit(Bitmap_sync, &rdev->flags);
- err = rdev->mddev->pers->
- hot_add_disk(rdev->mddev, rdev);
- if (err) {
- rdev->raid_disk = -1;
- return err;
- } else
- sysfs_notify_dirent_safe(rdev->sysfs_state);
- if (sysfs_link_rdev(rdev->mddev, rdev))
- /* failure here is OK */;
+ remove_and_add_spares(rdev->mddev, rdev);
+ if (rdev->raid_disk == -1)
+ return -EBUSY;
/* don't wakeup anyone, leave that to userspace. */
} else {
if (slot >= rdev->mddev->raid_disks &&
@@ -6004,12 +5998,16 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
if (mddev_is_clustered(mddev))
md_cluster_ops->metadata_update_start(mddev);
+ if (rdev->raid_disk < 0)
+ goto kick_rdev;
+
clear_bit(Blocked, &rdev->flags);
remove_and_add_spares(mddev, rdev);
if (rdev->raid_disk >= 0)
goto busy;
+kick_rdev:
if (mddev_is_clustered(mddev))
md_cluster_ops->remove_disk(mddev, rdev);
@@ -6024,6 +6022,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
busy:
if (mddev_is_clustered(mddev))
md_cluster_ops->metadata_update_cancel(mddev);
+
printk(KERN_WARNING "md: cannot remove active disk %s from %s ...\n",
bdevname(rdev->bdev,b), mdname(mddev));
return -EBUSY;
@@ -8018,10 +8017,12 @@ static int remove_and_add_spares(struct mddev *mddev,
if (removed && mddev->kobj.sd)
sysfs_notify(&mddev->kobj, NULL, "degraded");
- if (this)
+ if (this && removed)
goto no_add;
rdev_for_each(rdev, mddev) {
+ if (this && this != rdev)
+ continue;
if (rdev->raid_disk >= 0 &&
!test_bit(In_sync, &rdev->flags) &&
!test_bit(Faulty, &rdev->flags))
--
1.8.5.6
^ permalink raw reply related
* [PATCH 03/14] md-cluster: Improve md_reload_sb to be less error prone
From: rgoldwyn @ 2015-10-13 14:25 UTC (permalink / raw)
To: linux-raid, neilb; +Cc: gqjiang, Goldwyn Rodrigues
In-Reply-To: <1444746335-22624-1-git-send-email-rgoldwyn@suse.de>
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
md_reload_sb is too simplistic and it explicitly needs to determine
the changes made by the writing node. However, there are multiple areas
where a simple reload could fail.
Instead, read the superblock of one of the "good" rdevs and update
the necessary information:
- read the superblock into a newly allocated page, by temporarily
swapping out rdev->sb_page and calling ->load_super.
- if that fails return
- if it succeeds, call check_sb_changes
1. iterates over list of active devices and checks the matching
dev_roles[] value.
If that is 'faulty', the device must be marked as faulty
- call md_error to mark the device as faulty. Make sure
not to set CHANGE_DEVS and wakeup mddev->thread or else
it would initiate a resync process, which is the responsibility
of the "primary" node.
- clear the Blocked bit
- Call remove_and_add_spares() to hot remove the device.
If the device is 'spare':
- call remove_and_add_spares() to get the number of spares
added in this operation.
- Reduce mddev->degraded to mark the array as not degraded.
2. reset recovery_cp
- read the rest of the rdevs to update recovery_offset. If recovery_offset
is equal to MaxSector, call spare_active() to set it In_sync
This required that recovery_offset be initialized to MaxSector, as
opposed to zero so as to communicate the end of sync for a rdev.
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
drivers/md/md-cluster.c | 27 ++++++-----
drivers/md/md.c | 121 ++++++++++++++++++++++++++++++++++++++++++------
drivers/md/md.h | 2 +-
drivers/md/raid1.c | 9 ++++
4 files changed, 133 insertions(+), 26 deletions(-)
diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index 58eadc0..2eb3a50 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -427,8 +427,7 @@ static void process_add_new_disk(struct mddev *mddev, struct cluster_msg *cmsg)
static void process_metadata_update(struct mddev *mddev, struct cluster_msg *msg)
{
struct md_cluster_info *cinfo = mddev->cluster_info;
-
- md_reload_sb(mddev);
+ md_reload_sb(mddev, le32_to_cpu(msg->raid_slot));
dlm_lock_sync(cinfo->no_new_dev_lockres, DLM_LOCK_CR);
}
@@ -834,11 +833,23 @@ static int metadata_update_finish(struct mddev *mddev)
{
struct md_cluster_info *cinfo = mddev->cluster_info;
struct cluster_msg cmsg;
- int ret;
+ struct md_rdev *rdev;
+ int ret = 0;
memset(&cmsg, 0, sizeof(cmsg));
cmsg.type = cpu_to_le32(METADATA_UPDATED);
- ret = __sendmsg(cinfo, &cmsg);
+ cmsg.raid_slot = -1;
+ /* Pick up a good active device number to send.
+ */
+ rdev_for_each(rdev, mddev)
+ if (rdev->raid_disk > -1 && !test_bit(Faulty, &rdev->flags)) {
+ cmsg.raid_slot = cpu_to_le32(rdev->desc_nr);
+ break;
+ }
+ if (cmsg.raid_slot >= 0)
+ ret = __sendmsg(cinfo, &cmsg);
+ else
+ pr_warn("md-cluster: No good device id found to send\n");
unlock_comm(cinfo);
return ret;
}
@@ -922,15 +933,9 @@ static int add_new_disk_start(struct mddev *mddev, struct md_rdev *rdev)
static int add_new_disk_finish(struct mddev *mddev)
{
- struct cluster_msg cmsg;
- struct md_cluster_info *cinfo = mddev->cluster_info;
- int ret;
/* Write sb and inform others */
md_update_sb(mddev, 1);
- cmsg.type = METADATA_UPDATED;
- ret = __sendmsg(cinfo, &cmsg);
- unlock_comm(cinfo);
- return ret;
+ return metadata_update_finish(mddev);
}
static int new_disk_ack(struct mddev *mddev, bool ack)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index e21a2fe..12cc28a 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8924,25 +8924,118 @@ err_wq:
return ret;
}
-void md_reload_sb(struct mddev *mddev)
+static void check_sb_changes(struct mddev *mddev, struct md_rdev *rdev)
{
- struct md_rdev *rdev, *tmp;
+ struct mdp_superblock_1 *sb = page_address(rdev->sb_page);
+ struct md_rdev *rdev2;
+ int role, ret;
+ char b[BDEVNAME_SIZE];
- rdev_for_each_safe(rdev, tmp, mddev) {
- rdev->sb_loaded = 0;
- ClearPageUptodate(rdev->sb_page);
+ /* Check for change of roles in the active devices */
+ rdev_for_each(rdev2, mddev) {
+ if (test_bit(Faulty, &rdev2->flags))
+ continue;
+
+ /* Check if the roles changed */
+ role = le16_to_cpu(sb->dev_roles[rdev2->desc_nr]);
+ if (role != rdev2->raid_disk) {
+ /* got activated */
+ if (rdev2->raid_disk == -1 && role != 0xffff) {
+ rdev2->saved_raid_disk = role;
+ ret = remove_and_add_spares(mddev, rdev2);
+ pr_info("Activated spare: %s\n",
+ bdevname(rdev2->bdev,b));
+ continue;
+ }
+ /* device faulty
+ * We just want to do the minimum to mark the disk
+ * as faulty. The recovery is performed by the
+ * one who initiated the error.
+ */
+ if ((role == 0xfffe) || (role == 0xfffd)) {
+ md_error(mddev, rdev2);
+ clear_bit(Blocked, &rdev2->flags);
+ }
+ }
}
- mddev->raid_disks = 0;
- analyze_sbs(mddev);
- rdev_for_each_safe(rdev, tmp, mddev) {
- struct mdp_superblock_1 *sb = page_address(rdev->sb_page);
- /* since we don't write to faulty devices, we figure out if the
- * disk is faulty by comparing events
- */
- if (mddev->events > sb->events)
- set_bit(Faulty, &rdev->flags);
+
+ /* recovery_cp changed */
+ if (le64_to_cpu(sb->resync_offset) != mddev->recovery_cp)
+ mddev->recovery_cp = le64_to_cpu(sb->resync_offset);
+
+ /* Finally set the event to be up to date */
+ mddev->events = le64_to_cpu(sb->events);
+}
+
+static int read_rdev(struct mddev *mddev, struct md_rdev *rdev)
+{
+ int err;
+ struct page *swapout = rdev->sb_page;
+ struct mdp_superblock_1 *sb;
+
+ /* Store the sb page of the rdev in the swapout temporary
+ * variable in case we err in the future
+ */
+ rdev->sb_page = NULL;
+ alloc_disk_sb(rdev);
+ ClearPageUptodate(rdev->sb_page);
+ rdev->sb_loaded = 0;
+ err = super_types[mddev->major_version].load_super(rdev, NULL, mddev->minor_version);
+
+ if (err < 0) {
+ pr_warn("%s: %d Could not reload rdev(%d) err: %d. Restoring old values\n",
+ __func__, __LINE__, rdev->desc_nr, err);
+ put_page(rdev->sb_page);
+ rdev->sb_page = swapout;
+ rdev->sb_loaded = 1;
+ return err;
}
+ sb = page_address(rdev->sb_page);
+ /* Read the offset unconditionally, even if MD_FEATURE_RECOVERY_OFFSET
+ * is not set
+ */
+
+ if ((le32_to_cpu(sb->feature_map) & MD_FEATURE_RECOVERY_OFFSET))
+ rdev->recovery_offset = le64_to_cpu(sb->recovery_offset);
+
+ /* The other node finished recovery, call spare_active to set
+ * device In_sync and mddev->degraded
+ */
+ if (rdev->recovery_offset == MaxSector &&
+ !test_bit(In_sync, &rdev->flags) &&
+ mddev->pers->spare_active(mddev))
+ sysfs_notify(&mddev->kobj, NULL, "degraded");
+
+ put_page(swapout);
+ return 0;
+}
+
+void md_reload_sb(struct mddev *mddev, int nr)
+{
+ struct md_rdev *rdev;
+ int err;
+
+ /* Find the rdev */
+ rdev_for_each_rcu(rdev, mddev) {
+ if (rdev->desc_nr == nr)
+ break;
+ }
+
+ if (!rdev || rdev->desc_nr != nr) {
+ pr_warn("%s: %d Could not find rdev with nr %d\n", __func__, __LINE__, nr);
+ return;
+ }
+
+ err = read_rdev(mddev, rdev);
+ if (err < 0)
+ return;
+
+ check_sb_changes(mddev, rdev);
+
+ /* Read all rdev's to update recovery_offset */
+ rdev_for_each_rcu(rdev, mddev)
+ read_rdev(mddev, rdev);
}
EXPORT_SYMBOL(md_reload_sb);
diff --git a/drivers/md/md.h b/drivers/md/md.h
index ab33957..2ea0035 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -658,7 +658,7 @@ extern struct bio *bio_alloc_mddev(gfp_t gfp_mask, int nr_iovecs,
struct mddev *mddev);
extern void md_unplug(struct blk_plug_cb *cb, bool from_schedule);
-extern void md_reload_sb(struct mddev *mddev);
+extern void md_reload_sb(struct mddev *mddev, int raid_disk);
extern void md_update_sb(struct mddev *mddev, int force);
extern void md_kick_rdev_from_array(struct md_rdev * rdev);
struct md_rdev *md_find_rdev_nr_rcu(struct mddev *mddev, int nr);
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 1dd13bb..b54fefc 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1592,6 +1592,15 @@ static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
if (rdev->raid_disk >= 0)
first = last = rdev->raid_disk;
+ /*
+ * find the disk ... but prefer rdev->saved_raid_disk
+ * if possible.
+ */
+ if (rdev->saved_raid_disk >= 0 &&
+ rdev->saved_raid_disk >= first &&
+ conf->mirrors[rdev->saved_raid_disk].rdev == NULL)
+ first = last = rdev->saved_raid_disk;
+
for (mirror = first; mirror <= last; mirror++) {
p = conf->mirrors+mirror;
if (!p->rdev) {
--
1.8.5.6
^ 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