* [PATCH] md: setup safemode_timer before it's being used
From: Sasha Levin @ 2015-07-24 22:19 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, linux-kernel, Sasha Levin
We used to set up the safemode_timer timer in md_run. If md_run
would fail before the timer was set up we'd end up trying to modify
a timer that doesn't have a callback function when we access safe_delay_store,
which would trigger a BUG.
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
drivers/md/md.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 80879dc..60fb47e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -502,6 +502,8 @@ static void mddev_put(struct mddev *mddev)
bioset_free(bs);
}
+static void md_safemode_timeout(unsigned long data);
+
void mddev_init(struct mddev *mddev)
{
mutex_init(&mddev->open_mutex);
@@ -510,6 +512,8 @@ void mddev_init(struct mddev *mddev)
INIT_LIST_HEAD(&mddev->disks);
INIT_LIST_HEAD(&mddev->all_mddevs);
init_timer(&mddev->safemode_timer);
+ setup_timer(&mddev->safemode_timer, md_safemode_timeout,
+ (unsigned long) mddev);
atomic_set(&mddev->active, 1);
atomic_set(&mddev->openers, 0);
atomic_set(&mddev->active_io, 0);
@@ -3276,8 +3280,6 @@ int strict_strtoul_scaled(const char *cp, unsigned long *res, int scale)
return 0;
}
-static void md_safemode_timeout(unsigned long data);
-
static ssize_t
safe_delay_show(struct mddev *mddev, char *page)
{
@@ -5204,8 +5206,6 @@ int md_run(struct mddev *mddev)
atomic_set(&mddev->max_corr_read_errors,
MD_DEFAULT_MAX_CORRECTED_READ_ERRORS);
mddev->safemode = 0;
- mddev->safemode_timer.function = md_safemode_timeout;
- mddev->safemode_timer.data = (unsigned long) mddev;
mddev->safemode_delay = (200 * HZ)/1000 +1; /* 200 msec delay */
mddev->in_sync = 1;
smp_wmb();
--
1.7.10.4
^ permalink raw reply related
* [PATCH] drivers/md/md.c: use kzalloc() when bitmap is disabled
From: Benjamin Randazzo @ 2015-07-25 14:36 UTC (permalink / raw)
To: neilb; +Cc: linux-raid, linux-kernel, Benjamin Randazzo
In drivers/md/md.c get_bitmap_file() uses kmalloc() for creating a
mdu_bitmap_file_t called "file".
5769 file = kmalloc(sizeof(*file), GFP_NOIO);
5770 if (!file)
5771 return -ENOMEM;
This structure is copied to user space at the end of the function.
5786 if (err == 0 &&
5787 copy_to_user(arg, file, sizeof(*file)))
5788 err = -EFAULT
But if bitmap is disabled only the first byte of "file" is initialized
with zero, so it's possible to read some bytes (up to 4095) of kernel
space memory from user space. This is an information leak.
5775 /* bitmap disabled, zero the first byte and copy out */
5776 if (!mddev->bitmap_info.file)
5777 file->pathname[0] = '\0';
Signed-off-by: Benjamin Randazzo <benjamin@randazzo.fr>
---
drivers/md/md.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 80879dc..382bdbc 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5766,22 +5766,21 @@ static int get_bitmap_file(struct mddev *mddev, void __user * arg)
char *ptr;
int err;
- file = kmalloc(sizeof(*file), GFP_NOIO);
+ file = kzalloc(sizeof(*file), GFP_NOIO);
if (!file)
return -ENOMEM;
err = 0;
spin_lock(&mddev->lock);
- /* bitmap disabled, zero the first byte and copy out */
- if (!mddev->bitmap_info.file)
- file->pathname[0] = '\0';
- else if ((ptr = file_path(mddev->bitmap_info.file,
- file->pathname, sizeof(file->pathname))),
- IS_ERR(ptr))
- err = PTR_ERR(ptr);
- else
- memmove(file->pathname, ptr,
- sizeof(file->pathname)-(ptr-file->pathname));
+ /* bitmap enabled */
+ if (mddev->bitmap_info.file) {
+ if ((ptr = file_path(mddev->bitmap_info.file, file->pathname,
+ sizeof(file->pathname))), IS_ERR(ptr))
+ err = PTR_ERR(ptr);
+ else
+ memmove(file->pathname, ptr,
+ sizeof(file->pathname)-(ptr-file->pathname));
+ }
spin_unlock(&mddev->lock);
if (err == 0 &&
--
2.3.6
^ permalink raw reply related
* Re: raid1_end_read_request does not retry failed READ from a recovering drive
From: Alexander Lyakas @ 2015-07-26 8:15 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <20150724092404.66dc284d@noble>
Hi Neil,
On Fri, Jul 24, 2015 at 1:24 AM, NeilBrown <neilb@suse.com> wrote:
> Hi Alex
> thanks for noticing!
> Just to be sure we mean the same thing: this is the patch which is
> missing - correct?
>
> Thanks,
> NeilBrown
>
> From: NeilBrown <neilb@suse.com>
> Date: Fri, 24 Jul 2015 09:22:16 +1000
> Subject: [PATCH] md/raid1: fix test for 'was read error from last working
> device'.
>
> When we get a read error from the last working device, we don't
> try to repair it, and don't fail the device. We simple report a
> read error to the caller.
>
> However the current test for 'is this the last working device' is
> wrong.
> When there is only one fully working device, it assumes that a
> non-faulty device is that device. However a spare which is rebuilding
> would be non-faulty but so not the only working device.
>
> So change the test from "!Faulty" to "In_sync". If ->degraded says
> there is only one fully working device and this device is in_sync,
> this must be the one.
>
> This bug has existed since we allowed read_balance to read from
> a recovering spare in v3.0
>
> Reported-and-tested-by: Alexander Lyakas <alex.bolshoy@gmail.com>
> Fixes: 76073054c95b ("md/raid1: clean up read_balance.")
> Cc: stable@vger.kernel.org (v3.0+)
> Signed-off-by: NeilBrown <neilb@suse.com>
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 166616411215..b368307a9651 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -336,7 +336,7 @@ static void raid1_end_read_request(struct bio *bio, int error)
> spin_lock_irqsave(&conf->device_lock, flags);
> if (r1_bio->mddev->degraded == conf->raid_disks ||
> (r1_bio->mddev->degraded == conf->raid_disks-1 &&
> - !test_bit(Faulty, &conf->mirrors[mirror].rdev->flags)))
> + test_bit(In_sync, &conf->mirrors[mirror].rdev->flags)))
> uptodate = 1;
> spin_unlock_irqrestore(&conf->device_lock, flags);
> }
Yes, this was the missing piece.
But you also advised to put the whole of "raid1_spare_active" under
the spinlock:
> 2/ extend the spinlock in raid1_spare_active to cover the whole function.
So we did this bit too. Can you pls comment if this is needed?
Also, will you be sending this patch to "stable"? We are moving to
kernel 3.18, so we should get this then.
Thanks!
Alex.
^ permalink raw reply
* Re: [PATCH] md: setup safemode_timer before it's being used
From: NeilBrown @ 2015-07-27 1:30 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-raid, linux-kernel
In-Reply-To: <1437776398-31079-1-git-send-email-sasha.levin@oracle.com>
On Fri, 24 Jul 2015 18:19:58 -0400 Sasha Levin <sasha.levin@oracle.com>
wrote:
> We used to set up the safemode_timer timer in md_run. If md_run
> would fail before the timer was set up we'd end up trying to modify
> a timer that doesn't have a callback function when we access safe_delay_store,
> which would trigger a BUG.
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
> drivers/md/md.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 80879dc..60fb47e 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -502,6 +502,8 @@ static void mddev_put(struct mddev *mddev)
> bioset_free(bs);
> }
>
> +static void md_safemode_timeout(unsigned long data);
> +
> void mddev_init(struct mddev *mddev)
> {
> mutex_init(&mddev->open_mutex);
> @@ -510,6 +512,8 @@ void mddev_init(struct mddev *mddev)
> INIT_LIST_HEAD(&mddev->disks);
> INIT_LIST_HEAD(&mddev->all_mddevs);
> init_timer(&mddev->safemode_timer);
> + setup_timer(&mddev->safemode_timer, md_safemode_timeout,
> + (unsigned long) mddev);
> atomic_set(&mddev->active, 1);
> atomic_set(&mddev->openers, 0);
> atomic_set(&mddev->active_io, 0);
> @@ -3276,8 +3280,6 @@ int strict_strtoul_scaled(const char *cp, unsigned long *res, int scale)
> return 0;
> }
>
> -static void md_safemode_timeout(unsigned long data);
> -
> static ssize_t
> safe_delay_show(struct mddev *mddev, char *page)
> {
> @@ -5204,8 +5206,6 @@ int md_run(struct mddev *mddev)
> atomic_set(&mddev->max_corr_read_errors,
> MD_DEFAULT_MAX_CORRECTED_READ_ERRORS);
> mddev->safemode = 0;
> - mddev->safemode_timer.function = md_safemode_timeout;
> - mddev->safemode_timer.data = (unsigned long) mddev;
> mddev->safemode_delay = (200 * HZ)/1000 +1; /* 200 msec delay */
> mddev->in_sync = 1;
> smp_wmb();
Thanks.
I've applied that patch, and also removed the 'init_timer' call which
is now not needed as setup_timer does that.
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH] drivers/md/md.c: use kzalloc() when bitmap is disabled
From: NeilBrown @ 2015-07-27 1:36 UTC (permalink / raw)
To: Benjamin Randazzo; +Cc: linux-raid, linux-kernel
In-Reply-To: <1437835010-11430-1-git-send-email-benjamin@randazzo.fr>
On Sat, 25 Jul 2015 16:36:50 +0200 Benjamin Randazzo
<benjamin@randazzo.fr> wrote:
> In drivers/md/md.c get_bitmap_file() uses kmalloc() for creating a
> mdu_bitmap_file_t called "file".
>
> 5769 file = kmalloc(sizeof(*file), GFP_NOIO);
> 5770 if (!file)
> 5771 return -ENOMEM;
>
> This structure is copied to user space at the end of the function.
>
> 5786 if (err == 0 &&
> 5787 copy_to_user(arg, file, sizeof(*file)))
> 5788 err = -EFAULT
>
> But if bitmap is disabled only the first byte of "file" is initialized
> with zero, so it's possible to read some bytes (up to 4095) of kernel
> space memory from user space. This is an information leak.
>
> 5775 /* bitmap disabled, zero the first byte and copy out */
> 5776 if (!mddev->bitmap_info.file)
> 5777 file->pathname[0] = '\0';
>
> Signed-off-by: Benjamin Randazzo <benjamin@randazzo.fr>
> ---
> drivers/md/md.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 80879dc..382bdbc 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5766,22 +5766,21 @@ static int get_bitmap_file(struct mddev *mddev, void __user * arg)
> char *ptr;
> int err;
>
> - file = kmalloc(sizeof(*file), GFP_NOIO);
> + file = kzalloc(sizeof(*file), GFP_NOIO);
> if (!file)
> return -ENOMEM;
>
> err = 0;
> spin_lock(&mddev->lock);
> - /* bitmap disabled, zero the first byte and copy out */
> - if (!mddev->bitmap_info.file)
> - file->pathname[0] = '\0';
> - else if ((ptr = file_path(mddev->bitmap_info.file,
> - file->pathname, sizeof(file->pathname))),
> - IS_ERR(ptr))
> - err = PTR_ERR(ptr);
> - else
> - memmove(file->pathname, ptr,
> - sizeof(file->pathname)-(ptr-file->pathname));
> + /* bitmap enabled */
> + if (mddev->bitmap_info.file) {
> + if ((ptr = file_path(mddev->bitmap_info.file, file->pathname,
> + sizeof(file->pathname))), IS_ERR(ptr))
> + err = PTR_ERR(ptr);
> + else
> + memmove(file->pathname, ptr,
> + sizeof(file->pathname)-(ptr-file->pathname));
> + }
> spin_unlock(&mddev->lock);
>
> if (err == 0 &&
Thanks.
I re-arranged the code a little bit more as there is no longer any
excuse for having the "ptr = file_path()" assignment inside the
condition of the 'if'.
Applied. Thanks,
NeilBrown
^ permalink raw reply
* Re: raid1_end_read_request does not retry failed READ from a recovering drive
From: NeilBrown @ 2015-07-27 1:56 UTC (permalink / raw)
To: Alexander Lyakas; +Cc: linux-raid
In-Reply-To: <CAGRgLy7qkyQquRDGCShjJsuREQXNN+WaNGaq5wK2psMLQ6BGtw@mail.gmail.com>
On Sun, 26 Jul 2015 10:15:05 +0200 Alexander Lyakas
<alex.bolshoy@gmail.com> wrote:
> Hi Neil,
>
> On Fri, Jul 24, 2015 at 1:24 AM, NeilBrown <neilb@suse.com> wrote:
> > Hi Alex
> > thanks for noticing!
> > Just to be sure we mean the same thing: this is the patch which is
> > missing - correct?
> >
> > Thanks,
> > NeilBrown
> >
> > From: NeilBrown <neilb@suse.com>
> > Date: Fri, 24 Jul 2015 09:22:16 +1000
> > Subject: [PATCH] md/raid1: fix test for 'was read error from last working
> > device'.
> >
> > When we get a read error from the last working device, we don't
> > try to repair it, and don't fail the device. We simple report a
> > read error to the caller.
> >
> > However the current test for 'is this the last working device' is
> > wrong.
> > When there is only one fully working device, it assumes that a
> > non-faulty device is that device. However a spare which is rebuilding
> > would be non-faulty but so not the only working device.
> >
> > So change the test from "!Faulty" to "In_sync". If ->degraded says
> > there is only one fully working device and this device is in_sync,
> > this must be the one.
> >
> > This bug has existed since we allowed read_balance to read from
> > a recovering spare in v3.0
> >
> > Reported-and-tested-by: Alexander Lyakas <alex.bolshoy@gmail.com>
> > Fixes: 76073054c95b ("md/raid1: clean up read_balance.")
> > Cc: stable@vger.kernel.org (v3.0+)
> > Signed-off-by: NeilBrown <neilb@suse.com>
> >
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index 166616411215..b368307a9651 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -336,7 +336,7 @@ static void raid1_end_read_request(struct bio *bio, int error)
> > spin_lock_irqsave(&conf->device_lock, flags);
> > if (r1_bio->mddev->degraded == conf->raid_disks ||
> > (r1_bio->mddev->degraded == conf->raid_disks-1 &&
> > - !test_bit(Faulty, &conf->mirrors[mirror].rdev->flags)))
> > + test_bit(In_sync, &conf->mirrors[mirror].rdev->flags)))
> > uptodate = 1;
> > spin_unlock_irqrestore(&conf->device_lock, flags);
> > }
>
> Yes, this was the missing piece.
>
> But you also advised to put the whole of "raid1_spare_active" under
> the spinlock:
> > 2/ extend the spinlock in raid1_spare_active to cover the whole function.
> So we did this bit too. Can you pls comment if this is needed?
When you say "we did this bit" it would help a lot to actually show the
patch - helps avoid misunderstanding. In fact I was probably hoping
that you would post a patch once you had it fixed.... no mater.
See below for that patch I have just queue. There is an extra place
were a spinlock is probably helpful.
>
> Also, will you be sending this patch to "stable"? We are moving to
> kernel 3.18, so we should get this then.
Putting "cc: stable@vger.kernel.org" means that it will automatically
migrated to the stable kernels once it has appeared in Linus' kernel.
So yes: it will go to -stable
>
> Thanks!
> Alex.
Thanks,
NeilBrown
From 04f58ef505b9d354dd06c477a94a7e314a38cb72 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.com>
Date: Mon, 27 Jul 2015 11:48:52 +1000
Subject: [PATCH] md/raid1: extend spinlock to protect raid1_end_read_request
against inconsistencies
raid1_end_read_request() assumes that the In_sync bits are consistent
with the ->degaded count.
raid1_spare_active updates the In_sync bit before the ->degraded count
and so exposes an inconsistency, as does error()
So extend the spinlock in raid1_spare_active() and error() to hide those
inconsistencies.
This should probably be part of
Commit: 34cab6f42003 ("md/raid1: fix test for 'was read error from
last working device'.")
as it addresses the same issue. It fixes the same bug and should go
to -stable for same reasons.
Fixes: 76073054c95b ("md/raid1: clean up read_balance.")
Cc: stable@vger.kernel.org (v3.0+)
Signed-off-by: NeilBrown <neilb@suse.com>
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index b368307a9651..742b50794dfd 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1476,6 +1476,7 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
{
char b[BDEVNAME_SIZE];
struct r1conf *conf = mddev->private;
+ unsigned long flags;
/*
* If it is not operational, then we have already marked it as dead
@@ -1495,14 +1496,13 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
return;
}
set_bit(Blocked, &rdev->flags);
+ spin_lock_irqsave(&conf->device_lock, flags);
if (test_and_clear_bit(In_sync, &rdev->flags)) {
- unsigned long flags;
- spin_lock_irqsave(&conf->device_lock, flags);
mddev->degraded++;
set_bit(Faulty, &rdev->flags);
- spin_unlock_irqrestore(&conf->device_lock, flags);
} else
set_bit(Faulty, &rdev->flags);
+ spin_unlock_irqrestore(&conf->device_lock, flags);
/*
* if recovery is running, make sure it aborts.
*/
@@ -1568,7 +1568,10 @@ static int raid1_spare_active(struct mddev *mddev)
* Find all failed disks within the RAID1 configuration
* and mark them readable.
* Called under mddev lock, so rcu protection not needed.
+ * device_lock used to avoid races with raid1_end_read_request
+ * which expects 'In_sync' flags and ->degraded to be consistent.
*/
+ spin_lock_irqsave(&conf->device_lock, flags);
for (i = 0; i < conf->raid_disks; i++) {
struct md_rdev *rdev = conf->mirrors[i].rdev;
struct md_rdev *repl = conf->mirrors[conf->raid_disks + i].rdev;
@@ -1599,7 +1602,6 @@ static int raid1_spare_active(struct mddev *mddev)
sysfs_notify_dirent_safe(rdev->sysfs_state);
}
}
- spin_lock_irqsave(&conf->device_lock, flags);
mddev->degraded -= count;
spin_unlock_irqrestore(&conf->device_lock, flags);
^ permalink raw reply related
* Re: raid1_end_read_request does not retry failed READ from a recovering drive
From: Alexander Lyakas @ 2015-07-27 8:07 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <20150727115653.530daba6@noble>
Hi Neil,
Thanks for the comments. Hopefully now we have the complete fix.
I am posting what we have applied on top of 3.8.13 (now we are moving
to 3.18.19, which already has part of the fix, so I will need to apply
a delta, until both your latest patches reach Mr. Stable). Locking the
spinlock in "error" function is not there (but I will apply it to
3.18.19).
Below patch is a bit ugly:
- CONFIG_MD_ZADARA is a define that we add, so that every engineer can
clearly distinguish our changes vs the original code. I know it's
ugly.
- zklog is a macro that eventually ends up in printk. This is only to
have a bit more prints, and is not functionally needed. zklog also
prints current->pid, function name etc, to have more context.
Hopefully, gmail will not make the patch even uglier.
Thanks for your help!
Alex.
diff --git a/md/3.8.13-030813-generic/drivers/md/raid1.c
b/md/3.8.13-030813-generic/drivers/md/raid1.c
index c864a6e..32170e9 100644
--- a/md/3.8.13-030813-generic/drivers/md/raid1.c
+++ b/md/3.8.13-030813-generic/drivers/md/raid1.c
@@ -322,24 +322,36 @@ static void raid1_end_read_request(struct bio
*bio, int error)
if (uptodate)
set_bit(R1BIO_Uptodate, &r1_bio->state);
else {
/* If all other devices have failed, we want to return
* the error upwards rather than fail the last device.
* Here we redefine "uptodate" to mean "Don't want to retry"
*/
unsigned long flags;
spin_lock_irqsave(&conf->device_lock, flags);
+#ifndef CONFIG_MD_ZADARA
if (r1_bio->mddev->degraded == conf->raid_disks ||
(r1_bio->mddev->degraded == conf->raid_disks-1 &&
!test_bit(Faulty, &conf->mirrors[mirror].rdev->flags)))
uptodate = 1;
+#else/*CONFIG_MD_ZADARA*/
+ /* Make sure we retry read error from a recovering device */
+ if (r1_bio->mddev->degraded == conf->raid_disks ||
+ (r1_bio->mddev->degraded == conf->raid_disks-1 &&
+ test_bit(In_sync, &conf->mirrors[mirror].rdev->flags))) {
+ char b[BDEVNAME_SIZE];
+ zklog_rl(mdname(conf->mddev), Z_KERR, "READ
rdev=%s[%llu:%u] ERROR - not retrying (last good drive)",
+ bdevname(conf->mirrors[mirror].rdev->bdev, b),
(unsigned long long)r1_bio->sector, r1_bio->sectors);
+ uptodate = 1;
+ }
+#endif/*CONFIG_MD_ZADARA*/
spin_unlock_irqrestore(&conf->device_lock, flags);
}
if (uptodate) {
raid_end_bio_io(r1_bio);
rdev_dec_pending(conf->mirrors[mirror].rdev, conf->mddev);
} else {
/*
* oops, read error:
*/
@@ -1418,20 +1430,31 @@ static void status(struct seq_file *seq,
struct mddev *mddev)
rcu_read_unlock();
seq_printf(seq, "]");
}
static void error(struct mddev *mddev, struct md_rdev *rdev)
{
char b[BDEVNAME_SIZE];
struct r1conf *conf = mddev->private;
+#ifdef CONFIG_MD_ZADARA
+ {
+ static DEFINE_RATELIMIT_STATE(_rs,
DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
+ if (__ratelimit(&_rs)) {
+ zklog(mdname(mddev), Z_KERR, "rdev=%s failure InSync=%u
Failed=%u STACK:",
+ bdevname(rdev->bdev, b), test_bit(In_sync,
&rdev->flags), test_bit(Faulty, &rdev->flags));
+ dump_stack();
+ }
+ }
+#endif/*CONFIG_MD_ZADARA*/
+
/*
* If it is not operational, then we have already marked it as dead
* else if it is the last working disks, ignore the error, let the
* next level up know.
* else mark the drive as failed
*/
if (test_bit(In_sync, &rdev->flags)
&& (conf->raid_disks - mddev->degraded) == 1) {
/*
* Don't fail the drive, act as though we were just a
@@ -1497,20 +1520,25 @@ static void close_sync(struct r1conf *conf)
conf->r1buf_pool = NULL;
}
static int raid1_spare_active(struct mddev *mddev)
{
int i;
struct r1conf *conf = mddev->private;
int count = 0;
unsigned long flags;
+#ifdef CONFIG_MD_ZADARA
+ /* we lock the whole function */
+ spin_lock_irqsave(&conf->device_lock, flags);
+#endif /*CONFIG_MD_ZADARA*/
+
/*
* Find all failed disks within the RAID1 configuration
* and mark them readable.
* Called under mddev lock, so rcu protection not needed.
*/
for (i = 0; i < conf->raid_disks; i++) {
struct md_rdev *rdev = conf->mirrors[i].rdev;
struct md_rdev *repl = conf->mirrors[conf->raid_disks + i].rdev;
if (repl
&& repl->recovery_offset == MaxSector
@@ -1530,21 +1558,24 @@ static int raid1_spare_active(struct mddev *mddev)
rdev->sysfs_state);
}
}
if (rdev
&& !test_bit(Faulty, &rdev->flags)
&& !test_and_set_bit(In_sync, &rdev->flags)) {
count++;
sysfs_notify_dirent_safe(rdev->sysfs_state);
}
}
+#ifndef CONFIG_MD_ZADARA
+ /* we lock the whole function */
spin_lock_irqsave(&conf->device_lock, flags);
+#endif /*CONFIG_MD_ZADARA*/
mddev->degraded -= count;
spin_unlock_irqrestore(&conf->device_lock, flags);
print_conf(conf);
return count;
}
static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
{
@@ -2027,20 +2058,27 @@ static void sync_request_write(struct mddev
*mddev, struct r1bio *r1_bio)
*
* 1. Retries failed read operations on working mirrors.
* 2. Updates the raid superblock when problems encounter.
* 3. Performs writes following reads for array synchronising.
*/
static void fix_read_error(struct r1conf *conf, int read_disk,
sector_t sect, int sectors)
{
struct mddev *mddev = conf->mddev;
+#ifdef CONFIG_MD_ZADARA
+ {
+ char b[BDEVNAME_SIZE] = {'\0'};
+ zklog_rl(mdname(mddev), Z_KWARN, "attempting to fix READ
rdev=%s[%llu:%u]",
+ bdevname(conf->mirrors[read_disk].rdev->bdev, b),
(unsigned long long)sect, sectors);
+ }
+#endif /*CONFIG_MD_ZADARA*/
while(sectors) {
int s = sectors;
int d = read_disk;
int success = 0;
int start;
struct md_rdev *rdev;
if (s > (PAGE_SIZE>>9))
s = PAGE_SIZE >> 9;
@@ -2078,33 +2116,41 @@ static void fix_read_error(struct r1conf
*conf, int read_disk,
break;
}
/* write it back and re-read */
start = d;
while (d != read_disk) {
if (d==0)
d = conf->raid_disks * 2;
d--;
rdev = conf->mirrors[d].rdev;
if (rdev &&
+#ifndef CONFIG_MD_ZADARA
test_bit(In_sync, &rdev->flags))
+#else /*CONFIG_MD_ZADARA*/
+ !test_bit(Faulty, &rdev->flags))
+#endif /*CONFIG_MD_ZADARA*/
r1_sync_page_io(rdev, sect, s,
conf->tmppage, WRITE);
}
d = start;
while (d != read_disk) {
char b[BDEVNAME_SIZE];
if (d==0)
d = conf->raid_disks * 2;
d--;
rdev = conf->mirrors[d].rdev;
if (rdev &&
+#ifndef CONFIG_MD_ZADARA
test_bit(In_sync, &rdev->flags)) {
+#else /*CONFIG_MD_ZADARA*/
+ !test_bit(Faulty, &rdev->flags)) {
+#endif /*CONFIG_MD_ZADARA*/
if (r1_sync_page_io(rdev, sect, s,
conf->tmppage, READ)) {
atomic_add(s, &rdev->corrected_errors);
printk(KERN_INFO
"md/raid1:%s: read error corrected "
"(%d sectors at %llu on %s)\n",
mdname(mddev), s,
(unsigned long long)(sect +
rdev->data_offset),
bdevname(rdev->bdev, b));
On Mon, Jul 27, 2015 at 3:56 AM, NeilBrown <neilb@suse.com> wrote:
> On Sun, 26 Jul 2015 10:15:05 +0200 Alexander Lyakas
> <alex.bolshoy@gmail.com> wrote:
>
>> Hi Neil,
>>
>> On Fri, Jul 24, 2015 at 1:24 AM, NeilBrown <neilb@suse.com> wrote:
>> > Hi Alex
>> > thanks for noticing!
>> > Just to be sure we mean the same thing: this is the patch which is
>> > missing - correct?
>> >
>> > Thanks,
>> > NeilBrown
>> >
>> > From: NeilBrown <neilb@suse.com>
>> > Date: Fri, 24 Jul 2015 09:22:16 +1000
>> > Subject: [PATCH] md/raid1: fix test for 'was read error from last working
>> > device'.
>> >
>> > When we get a read error from the last working device, we don't
>> > try to repair it, and don't fail the device. We simple report a
>> > read error to the caller.
>> >
>> > However the current test for 'is this the last working device' is
>> > wrong.
>> > When there is only one fully working device, it assumes that a
>> > non-faulty device is that device. However a spare which is rebuilding
>> > would be non-faulty but so not the only working device.
>> >
>> > So change the test from "!Faulty" to "In_sync". If ->degraded says
>> > there is only one fully working device and this device is in_sync,
>> > this must be the one.
>> >
>> > This bug has existed since we allowed read_balance to read from
>> > a recovering spare in v3.0
>> >
>> > Reported-and-tested-by: Alexander Lyakas <alex.bolshoy@gmail.com>
>> > Fixes: 76073054c95b ("md/raid1: clean up read_balance.")
>> > Cc: stable@vger.kernel.org (v3.0+)
>> > Signed-off-by: NeilBrown <neilb@suse.com>
>> >
>> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> > index 166616411215..b368307a9651 100644
>> > --- a/drivers/md/raid1.c
>> > +++ b/drivers/md/raid1.c
>> > @@ -336,7 +336,7 @@ static void raid1_end_read_request(struct bio *bio, int error)
>> > spin_lock_irqsave(&conf->device_lock, flags);
>> > if (r1_bio->mddev->degraded == conf->raid_disks ||
>> > (r1_bio->mddev->degraded == conf->raid_disks-1 &&
>> > - !test_bit(Faulty, &conf->mirrors[mirror].rdev->flags)))
>> > + test_bit(In_sync, &conf->mirrors[mirror].rdev->flags)))
>> > uptodate = 1;
>> > spin_unlock_irqrestore(&conf->device_lock, flags);
>> > }
>>
>> Yes, this was the missing piece.
>>
>> But you also advised to put the whole of "raid1_spare_active" under
>> the spinlock:
>> > 2/ extend the spinlock in raid1_spare_active to cover the whole function.
>> So we did this bit too. Can you pls comment if this is needed?
>
> When you say "we did this bit" it would help a lot to actually show the
> patch - helps avoid misunderstanding. In fact I was probably hoping
> that you would post a patch once you had it fixed.... no mater.
> See below for that patch I have just queue. There is an extra place
> were a spinlock is probably helpful.
>
>>
>> Also, will you be sending this patch to "stable"? We are moving to
>> kernel 3.18, so we should get this then.
>
> Putting "cc: stable@vger.kernel.org" means that it will automatically
> migrated to the stable kernels once it has appeared in Linus' kernel.
> So yes: it will go to -stable
>
>>
>> Thanks!
>> Alex.
>
> Thanks,
> NeilBrown
>
> From 04f58ef505b9d354dd06c477a94a7e314a38cb72 Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.com>
> Date: Mon, 27 Jul 2015 11:48:52 +1000
> Subject: [PATCH] md/raid1: extend spinlock to protect raid1_end_read_request
> against inconsistencies
>
> raid1_end_read_request() assumes that the In_sync bits are consistent
> with the ->degaded count.
> raid1_spare_active updates the In_sync bit before the ->degraded count
> and so exposes an inconsistency, as does error()
> So extend the spinlock in raid1_spare_active() and error() to hide those
> inconsistencies.
>
> This should probably be part of
> Commit: 34cab6f42003 ("md/raid1: fix test for 'was read error from
> last working device'.")
> as it addresses the same issue. It fixes the same bug and should go
> to -stable for same reasons.
>
> Fixes: 76073054c95b ("md/raid1: clean up read_balance.")
> Cc: stable@vger.kernel.org (v3.0+)
> Signed-off-by: NeilBrown <neilb@suse.com>
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index b368307a9651..742b50794dfd 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1476,6 +1476,7 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
> {
> char b[BDEVNAME_SIZE];
> struct r1conf *conf = mddev->private;
> + unsigned long flags;
>
> /*
> * If it is not operational, then we have already marked it as dead
> @@ -1495,14 +1496,13 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
> return;
> }
> set_bit(Blocked, &rdev->flags);
> + spin_lock_irqsave(&conf->device_lock, flags);
> if (test_and_clear_bit(In_sync, &rdev->flags)) {
> - unsigned long flags;
> - spin_lock_irqsave(&conf->device_lock, flags);
> mddev->degraded++;
> set_bit(Faulty, &rdev->flags);
> - spin_unlock_irqrestore(&conf->device_lock, flags);
> } else
> set_bit(Faulty, &rdev->flags);
> + spin_unlock_irqrestore(&conf->device_lock, flags);
> /*
> * if recovery is running, make sure it aborts.
> */
> @@ -1568,7 +1568,10 @@ static int raid1_spare_active(struct mddev *mddev)
> * Find all failed disks within the RAID1 configuration
> * and mark them readable.
> * Called under mddev lock, so rcu protection not needed.
> + * device_lock used to avoid races with raid1_end_read_request
> + * which expects 'In_sync' flags and ->degraded to be consistent.
> */
> + spin_lock_irqsave(&conf->device_lock, flags);
> for (i = 0; i < conf->raid_disks; i++) {
> struct md_rdev *rdev = conf->mirrors[i].rdev;
> struct md_rdev *repl = conf->mirrors[conf->raid_disks + i].rdev;
> @@ -1599,7 +1602,6 @@ static int raid1_spare_active(struct mddev *mddev)
> sysfs_notify_dirent_safe(rdev->sysfs_state);
> }
> }
> - spin_lock_irqsave(&conf->device_lock, flags);
> mddev->degraded -= count;
> spin_unlock_irqrestore(&conf->device_lock, flags);
>
^ permalink raw reply related
* Re: dm cache: fix alloc_bitset check that always evaluates as false
From: Mike Snitzer @ 2015-07-27 12:15 UTC (permalink / raw)
To: Colin King
Cc: Alasdair Kergon, dm-devel, Neil Brown, linux-raid, linux-kernel
In-Reply-To: <1437666479-24633-1-git-send-email-colin.king@canonical.com>
On Thu, Jul 23 2015 at 11:47P -0400,
Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> static analysis by cppcheck has found a check on alloc_bitset that
> always evaluates as false and hence never finds an allocation failure:
>
> [drivers/md/dm-cache-policy-smq.c:1689]: (warning) Logical conjunction
> always evaluates to false: !EXPR && EXPR.
>
> Fix this by removing the incorrect mq->cache_hit_bits check
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied for 4.2-rc5 inclussion, thanks:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=134bf30c06f057d6b8d90132e8f8b3cd2be79572
^ permalink raw reply
* Re: [PATCH 01/12] md-cluster: use %pU to print UUIDs
From: Goldwyn Rodrigues @ 2015-07-27 16:21 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518453-12660-2-git-send-email-gqjiang@suse.com>
This was proposed by Neil in one of the early review comments, but it
slipped through. Thanks Guoqing for doing this.
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
On 07/10/2015 03:54 AM, Guoqing Jiang wrote:
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
> drivers/md/md-cluster.c | 16 ++--------------
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 0072190..85ef5c5 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -177,18 +177,6 @@ static void lockres_free(struct dlm_lock_resource *res)
> kfree(res);
> }
>
> -static char *pretty_uuid(char *dest, char *src)
> -{
> - int i, len = 0;
> -
> - for (i = 0; i < 16; i++) {
> - if (i == 4 || i == 6 || i == 8 || i == 10)
> - len += sprintf(dest + len, "-");
> - len += sprintf(dest + len, "%02x", (__u8)src[i]);
> - }
> - return dest;
> -}
> -
> static void add_resync_info(struct mddev *mddev, struct dlm_lock_resource *lockres,
> sector_t lo, sector_t hi)
> {
> @@ -388,7 +376,7 @@ static void process_add_new_disk(struct mddev *mddev, struct cluster_msg *cmsg)
> int len;
>
> len = snprintf(disk_uuid, 64, "DEVICE_UUID=");
> - pretty_uuid(disk_uuid + len, cmsg->uuid);
> + sprintf(disk_uuid + len, "%pU", cmsg->uuid);
> snprintf(raid_slot, 16, "RAID_DISK=%d", cmsg->raid_slot);
> pr_info("%s:%d Sending kobject change with %s and %s\n", __func__, __LINE__, disk_uuid, raid_slot);
> init_completion(&cinfo->newdisk_completion);
> @@ -646,7 +634,7 @@ static int join(struct mddev *mddev, int nodes)
> mddev->cluster_info = cinfo;
>
> memset(str, 0, 64);
> - pretty_uuid(str, mddev->uuid);
> + sprintf(str, "%pU", mddev->uuid);
> ret = dlm_new_lockspace(str, mddev->bitmap_info.cluster_name,
> DLM_LSFL_FS, LVB_SIZE,
> &md_ls_ops, mddev, &ops_rv, &cinfo->lockspace);
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 03/12] md-cluster: transfer the resync ownership to another node
From: Goldwyn Rodrigues @ 2015-07-27 16:24 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518453-12660-4-git-send-email-gqjiang@suse.com>
On 07/10/2015 03:54 AM, Guoqing Jiang wrote:
> When node A stop an array while the array is doing resync, we need
> let another node B to take over the resync task.
>
> To achieve the goal, we need the A send an explicit BITMAP_NEEDS_SYNC
> message to the cluster. And the node B which received that message will
> invoke __recover_slot to do resync.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/md-cluster.c | 15 +++++++++++++++
> drivers/md/md.c | 6 +++---
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 24caabe..47199ad 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -75,6 +75,7 @@ enum msg_type {
> NEWDISK,
> REMOVE,
> RE_ADD,
> + BITMAP_NEEDS_SYNC,
> };
>
> struct cluster_msg {
> @@ -454,6 +455,11 @@ static void process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg)
> __func__, __LINE__, msg->slot);
> process_readd_disk(mddev, msg);
> break;
> + case BITMAP_NEEDS_SYNC:
> + pr_info("%s: %d Received BITMAP_NEEDS_SYNC from %d\n",
> + __func__, __LINE__, msg->slot);
> + __recover_slot(mddev, msg->slot);
> + break;
> default:
> pr_warn("%s:%d Received unknown message from %d\n",
> __func__, __LINE__, msg->slot);
> @@ -814,8 +820,17 @@ static int resync_start(struct mddev *mddev, sector_t lo, sector_t hi)
>
> static void resync_finish(struct mddev *mddev)
> {
> + struct md_cluster_info *cinfo = mddev->cluster_info;
> + struct cluster_msg cmsg;
> + int slot = cinfo->slot_number - 1;
> +
> pr_info("%s:%d\n", __func__, __LINE__);
> resync_send(mddev, RESYNCING, 0, 0);
> + if (test_bit(MD_RECOVERY_INTR, &mddev->recovery)) {
> + cmsg.type = cpu_to_le32(BITMAP_NEEDS_SYNC);
> + cmsg.slot = cpu_to_le32(slot);
> + sendmsg(cinfo, &cmsg);
> + }
> }
>
> static int area_resyncing(struct mddev *mddev, int direction,
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index df92d30..7d05dff 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7931,9 +7931,6 @@ void md_do_sync(struct md_thread *thread)
> /* tell personality that we are finished */
> mddev->pers->sync_request(mddev, max_sectors, &skipped);
>
> - if (mddev_is_clustered(mddev))
> - md_cluster_ops->resync_finish(mddev);
> -
> if (!test_bit(MD_RECOVERY_CHECK, &mddev->recovery) &&
> mddev->curr_resync > 2) {
> if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery)) {
> @@ -7967,6 +7964,9 @@ void md_do_sync(struct md_thread *thread)
> }
> }
> skip:
> + if (mddev_is_clustered(mddev))
> + md_cluster_ops->resync_finish(mddev);
> +
> set_bit(MD_CHANGE_DEVS, &mddev->flags);
>
> spin_lock(&mddev->lock);
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 04/12] md-cluster: fix deadlock issue on message lock
From: Goldwyn Rodrigues @ 2015-07-27 16:25 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-1-git-send-email-gqjiang@suse.com>
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> There is problem with previous communication mechanism, and we got below
> deadlock scenario with cluster which has 3 nodes.
>
> Sender Receiver Receiver
>
> token(EX)
> message(EX)
> writes message
> downconverts message(CR)
> requests ack(EX)
> get message(CR) gets message(CR)
> reads message reads message
> requests EX on message requests EX on message
>
> To fix this problem, we do the following changes:
>
> 1. the sender downconverts MESSAGE to CW rather than CR.
> 2. and the receiver request PR lock not EX lock on message.
>
> And in case we failed to down-convert EX to CW on message, it is better to
> unlock message otherthan still hold the lock.
>
> Signed-off-by: Lidong Zhong <ldzhong@suse.com>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> Documentation/md-cluster.txt | 4 ++--
> drivers/md/md-cluster.c | 14 +++++++-------
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/md-cluster.txt b/Documentation/md-cluster.txt
> index de1af7d..1b79436 100644
> --- a/Documentation/md-cluster.txt
> +++ b/Documentation/md-cluster.txt
> @@ -91,7 +91,7 @@ The algorithm is:
> this message inappropriate or redundant.
>
> 3. sender write LVB.
> - sender down-convert MESSAGE from EX to CR
> + sender down-convert MESSAGE from EX to CW
> sender try to get EX of ACK
> [ wait until all receiver has *processed* the MESSAGE ]
>
> @@ -112,7 +112,7 @@ The algorithm is:
> sender down-convert ACK from EX to CR
> sender release MESSAGE
> sender release TOKEN
> - receiver upconvert to EX of MESSAGE
> + receiver upconvert to PR of MESSAGE
> receiver get CR of ACK
> receiver release MESSAGE
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 47199ad..85b7836 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -488,8 +488,8 @@ static void recv_daemon(struct md_thread *thread)
>
> /*release CR on ack_lockres*/
> dlm_unlock_sync(ack_lockres);
> - /*up-convert to EX on message_lockres*/
> - dlm_lock_sync(message_lockres, DLM_LOCK_EX);
> + /*up-convert to PR on message_lockres*/
> + dlm_lock_sync(message_lockres, DLM_LOCK_PR);
> /*get CR on ack_lockres again*/
> dlm_lock_sync(ack_lockres, DLM_LOCK_CR);
> /*release CR on message_lockres*/
> @@ -522,7 +522,7 @@ static void unlock_comm(struct md_cluster_info *cinfo)
> * The function:
> * 1. Grabs the message lockresource in EX mode
> * 2. Copies the message to the message LVB
> - * 3. Downconverts message lockresource to CR
> + * 3. Downconverts message lockresource to CW
> * 4. Upconverts ack lock resource from CR to EX. This forces the BAST on other nodes
> * and the other nodes read the message. The thread will wait here until all other
> * nodes have released ack lock resource.
> @@ -543,12 +543,12 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg)
>
> memcpy(cinfo->message_lockres->lksb.sb_lvbptr, (void *)cmsg,
> sizeof(struct cluster_msg));
> - /*down-convert EX to CR on Message*/
> - error = dlm_lock_sync(cinfo->message_lockres, DLM_LOCK_CR);
> + /*down-convert EX to CW on Message*/
> + error = dlm_lock_sync(cinfo->message_lockres, DLM_LOCK_CW);
> if (error) {
> - pr_err("md-cluster: failed to convert EX to CR on MESSAGE(%d)\n",
> + pr_err("md-cluster: failed to convert EX to CW on MESSAGE(%d)\n",
> error);
> - goto failed_message;
> + goto failed_ack;
> }
>
> /*up-convert CR to EX on Ack*/
>
--
Goldwyn
^ permalink raw reply
* Re: Subject: ANNOUNCE: mdadm 3.3.3 - A tool for managing md Soft RAID under Linux
From: Piergiorgio Sartor @ 2015-07-27 16:29 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <20150724163530.768da131@noble>
On Fri, Jul 24, 2015 at 04:35:30PM +1000, NeilBrown wrote:
>
>
> I am pleased to announce the availability of
> mdadm version 3.3.3
>
> It is available at the usual places:
> http://www.kernel.org/pub/linux/utils/raid/mdadm/
> and via git at
> git://github.com/neilbrown/mdadm
> git://neil.brown.name/mdadm
> http://git.neil.brown.name/git/mdadm.git
>
> The 100 changes since 3.3.3 are mostly little bugfixes and some improvements
> to the selftests.
> raid6check now handle all RAID6 layouts including DDF correctly.
> See git log for the rest.
>
> NeilBrown 24th July 2015
Hi Neil,
I would like to thank you for clean up and improving
"raid6check.c".
I personally appreciate your contribution very much,
and I think it is good to have your expert eye
looking at the code.
Thanks again for your time,
bye,
--
piergiorgio
^ permalink raw reply
* Re: [PATCH 07/12] md-cluster: init suspend_list and suspend_lock early in join
From: Goldwyn Rodrigues @ 2015-07-27 16:29 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-4-git-send-email-gqjiang@suse.com>
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> If the node just join the cluster, and receive the msg from other nodes
> before init suspend_list, it will cause kernel crash due to NULL pointer
> dereference, so move the initializations early to fix the bug.
>
> md-cluster: Joined cluster 3578507b-e0cb-6d4f-6322-696cd7b1b10c slot 3
> BUG: unable to handle kernel NULL pointer dereference at (null)
> ... ... ...
> Call Trace:
> [<ffffffffa0444924>] process_recvd_msg+0x2e4/0x330 [md_cluster]
> [<ffffffffa0444a06>] recv_daemon+0x96/0x170 [md_cluster]
> [<ffffffffa045189d>] md_thread+0x11d/0x170 [md_mod]
> [<ffffffff810768c4>] kthread+0xb4/0xc0
> [<ffffffff8151927c>] ret_from_fork+0x7c/0xb0
> ... ... ...
> RIP [<ffffffffa0443581>] __remove_suspend_info+0x11/0xa0 [md_cluster]
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/md-cluster.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index b80a689..6f1ea3c 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -671,6 +671,8 @@ static int join(struct mddev *mddev, int nodes)
> if (!cinfo)
> return -ENOMEM;
>
> + INIT_LIST_HEAD(&cinfo->suspend_list);
> + spin_lock_init(&cinfo->suspend_lock);
> init_completion(&cinfo->completion);
>
> mutex_init(&cinfo->sb_mutex);
> @@ -736,9 +738,6 @@ static int join(struct mddev *mddev, int nodes)
> goto err;
> }
>
> - INIT_LIST_HEAD(&cinfo->suspend_list);
> - spin_lock_init(&cinfo->suspend_lock);
> -
> ret = gather_all_resync_info(mddev, nodes);
> if (ret)
> goto err;
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 08/12] md-cluster: remove the unused sb_lock
From: Goldwyn Rodrigues @ 2015-07-27 16:29 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-5-git-send-email-gqjiang@suse.com>
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> The sb_lock is not used anywhere, so let's remove it.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
Code cleanup.
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/md-cluster.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 6f1ea3c..057a973 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -52,7 +52,6 @@ struct md_cluster_info {
> dlm_lockspace_t *lockspace;
> int slot_number;
> struct completion completion;
> - struct dlm_lock_resource *sb_lock;
> struct mutex sb_mutex;
> struct dlm_lock_resource *bitmap_lockres;
> struct list_head suspend_list;
> @@ -692,12 +691,6 @@ static int join(struct mddev *mddev, int nodes)
> ret = -ERANGE;
> goto err;
> }
> - cinfo->sb_lock = lockres_init(mddev, "cmd-super",
> - NULL, 0);
> - if (!cinfo->sb_lock) {
> - ret = -ENOMEM;
> - goto err;
> - }
> /* Initiate the communication resources */
> ret = -ENOMEM;
> cinfo->recv_thread = md_register_thread(recv_daemon, mddev, "cluster_recv");
> @@ -749,7 +742,6 @@ err:
> lockres_free(cinfo->ack_lockres);
> lockres_free(cinfo->no_new_dev_lockres);
> lockres_free(cinfo->bitmap_lockres);
> - lockres_free(cinfo->sb_lock);
> if (cinfo->lockspace)
> dlm_release_lockspace(cinfo->lockspace, 2);
> mddev->cluster_info = NULL;
> @@ -770,7 +762,6 @@ static int leave(struct mddev *mddev)
> lockres_free(cinfo->token_lockres);
> lockres_free(cinfo->ack_lockres);
> lockres_free(cinfo->no_new_dev_lockres);
> - lockres_free(cinfo->sb_lock);
> lockres_free(cinfo->bitmap_lockres);
> dlm_release_lockspace(cinfo->lockspace, 2);
> return 0;
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 09/12] md-cluster: add missed lockres_free
From: Goldwyn Rodrigues @ 2015-07-27 16:30 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-6-git-send-email-gqjiang@suse.com>
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> We also need to free the lock resource before goto out.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/md-cluster.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 057a973..411b430 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -647,8 +647,10 @@ static int gather_all_resync_info(struct mddev *mddev, int total_slots)
> lockres_free(bm_lockres);
> continue;
> }
> - if (ret)
> + if (ret) {
> + lockres_free(bm_lockres);
> goto out;
> + }
> /* TODO: Read the disk bitmap sb and check if it needs recovery */
> dlm_unlock_sync(bm_lockres);
> lockres_free(bm_lockres);
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 11/12] md-cluster: Read the disk bitmap sb and check if it needs recovery
From: Goldwyn Rodrigues @ 2015-07-27 16:31 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-8-git-send-email-gqjiang@suse.com>
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> In gather_all_resync_info, we need to read the disk bitmap sb and
> check if it needs recovery.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/md-cluster.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 29f65e2..c35a03a 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -625,6 +625,7 @@ static int gather_all_resync_info(struct mddev *mddev, int total_slots)
> struct dlm_lock_resource *bm_lockres;
> struct suspend_info *s;
> char str[64];
> + sector_t lo, hi;
>
>
> for (i = 0; i < total_slots; i++) {
> @@ -659,7 +660,20 @@ static int gather_all_resync_info(struct mddev *mddev, int total_slots)
> lockres_free(bm_lockres);
> goto out;
> }
> - /* TODO: Read the disk bitmap sb and check if it needs recovery */
> +
> + /* Read the disk bitmap sb and check if it needs recovery */
> + ret = bitmap_copy_from_slot(mddev, i, &lo, &hi, false);
> + if (ret) {
> + pr_warn("md-cluster: Could not gather bitmaps from slot %d", i);
> + lockres_free(bm_lockres);
> + continue;
> + }
> + if ((hi > 0) && (lo < mddev->recovery_cp)) {
> + set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> + mddev->recovery_cp = lo;
> + md_check_recovery(mddev);
> + }
> +
> dlm_unlock_sync(bm_lockres);
> lockres_free(bm_lockres);
> }
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 12/12] md-cluster: handle error situations more precisely in lockres_init
From: Goldwyn Rodrigues @ 2015-07-27 16:34 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-9-git-send-email-gqjiang@suse.com>
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> In lockres_init, it's better to distinguish different err conditions.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
This is not required. kfree() is capable of ignoring null pointers.
> ---
> drivers/md/md-cluster.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index c35a03a..54d225c 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -131,14 +131,14 @@ static struct dlm_lock_resource *lockres_init(struct mddev *mddev,
> res->name = kzalloc(namelen + 1, GFP_KERNEL);
> if (!res->name) {
> pr_err("md-cluster: Unable to allocate resource name for resource %s\n", name);
> - goto out_err;
> + goto out_err_name;
> }
> strlcpy(res->name, name, namelen + 1);
> if (with_lvb) {
> res->lksb.sb_lvbptr = kzalloc(LVB_SIZE, GFP_KERNEL);
> if (!res->lksb.sb_lvbptr) {
> pr_err("md-cluster: Unable to allocate LVB for resource %s\n", name);
> - goto out_err;
> + goto out_err_lvb;
> }
> res->flags = DLM_LKF_VALBLK;
> }
> @@ -159,7 +159,9 @@ static struct dlm_lock_resource *lockres_init(struct mddev *mddev,
> return res;
> out_err:
> kfree(res->lksb.sb_lvbptr);
> +out_err_lvb:
> kfree(res->name);
> +out_err_name:
> kfree(res);
> return NULL;
> }
> @@ -627,7 +629,6 @@ static int gather_all_resync_info(struct mddev *mddev, int total_slots)
> char str[64];
> sector_t lo, hi;
>
> -
> for (i = 0; i < total_slots; i++) {
> memset(str, '\0', 64);
> snprintf(str, 64, "bitmap%04d", i);
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 05/12] md-cluster: init completion within lockres_init
From: Goldwyn Rodrigues @ 2015-07-27 16:44 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-2-git-send-email-gqjiang@suse.com>
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> We should init completion within lockres_init, otherwise
> completion could be initialized more than one time during
> it's life cycle.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/md-cluster.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 85b7836..2a57f19 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -100,7 +100,6 @@ static int dlm_lock_sync(struct dlm_lock_resource *res, int mode)
> {
> int ret = 0;
>
> - init_completion(&res->completion);
> ret = dlm_lock(res->ls, mode, &res->lksb,
> res->flags, res->name, strlen(res->name),
> 0, sync_ast, res, res->bast);
> @@ -125,6 +124,7 @@ static struct dlm_lock_resource *lockres_init(struct mddev *mddev,
> res = kzalloc(sizeof(struct dlm_lock_resource), GFP_KERNEL);
> if (!res)
> return NULL;
> + init_completion(&res->completion);
> res->ls = cinfo->lockspace;
> res->mddev = mddev;
> namelen = strlen(name);
> @@ -169,7 +169,6 @@ static void lockres_free(struct dlm_lock_resource *res)
> if (!res)
> return;
>
> - init_completion(&res->completion);
> dlm_unlock(res->ls, res->lksb.sb_lkid, 0, &res->lksb, res);
> wait_for_completion(&res->completion);
>
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 06/12] md-cluster: add the error check if failed to get dlm lock
From: Goldwyn Rodrigues @ 2015-07-27 16:48 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-3-git-send-email-gqjiang@suse.com>
Hi Guoqing,
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> In complicated cluster environment, it is possible that the
> dlm lock couldn't be get/convert on purpose, the related err
> info is added for better debug potential issue.
>
> For lockres_free, if the lock is blocking by a lock request or
> conversion request, then dlm_unlock just put it back to grant
> queue, so need to ensure the lock is free finally.
I cannot think of a scenario where a DLM_CANCEL will be returned. Could
you explain the situation a bit more?
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
> drivers/md/md-cluster.c | 41 +++++++++++++++++++++++++++++++++++------
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 2a57f19..b80a689 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -166,10 +166,24 @@ out_err:
>
> static void lockres_free(struct dlm_lock_resource *res)
> {
> + int ret;
> +
> if (!res)
> return;
>
> - dlm_unlock(res->ls, res->lksb.sb_lkid, 0, &res->lksb, res);
> + /* cancel a lock request or a conversion request that is blocked */
> + res->flags |= DLM_LKF_CANCEL;
> +retry:
> + ret = dlm_unlock(res->ls, res->lksb.sb_lkid, 0, &res->lksb, res);
> + if (unlikely(ret != 0)) {
> + pr_info("%s: failed to unlock %s return %d\n", __func__, res->name, ret);
> +
> + /* if a lock conversion is cancelled, then the lock is put
> + * back to grant queue, need to ensure it is unlocked */
> + if (ret == -DLM_ECANCEL)
> + goto retry;
> + }
> + res->flags &= ~DLM_LKF_CANCEL;
> wait_for_completion(&res->completion);
>
> kfree(res->name);
> @@ -474,6 +488,7 @@ static void recv_daemon(struct md_thread *thread)
> struct dlm_lock_resource *ack_lockres = cinfo->ack_lockres;
> struct dlm_lock_resource *message_lockres = cinfo->message_lockres;
> struct cluster_msg msg;
> + int ret;
>
> /*get CR on Message*/
> if (dlm_lock_sync(message_lockres, DLM_LOCK_CR)) {
> @@ -486,13 +501,21 @@ static void recv_daemon(struct md_thread *thread)
> process_recvd_msg(thread->mddev, &msg);
>
> /*release CR on ack_lockres*/
> - dlm_unlock_sync(ack_lockres);
> + ret = dlm_unlock_sync(ack_lockres);
> + if (unlikely(ret != 0))
> + pr_info("unlock ack failed return %d\n", ret);
> /*up-convert to PR on message_lockres*/
> - dlm_lock_sync(message_lockres, DLM_LOCK_PR);
> + ret = dlm_lock_sync(message_lockres, DLM_LOCK_PR);
> + if (unlikely(ret != 0))
> + pr_info("lock PR on msg failed return %d\n", ret);
> /*get CR on ack_lockres again*/
> - dlm_lock_sync(ack_lockres, DLM_LOCK_CR);
> + ret = dlm_lock_sync(ack_lockres, DLM_LOCK_CR);
> + if (unlikely(ret != 0))
> + pr_info("lock CR on ack failed return %d\n", ret);
> /*release CR on message_lockres*/
> - dlm_unlock_sync(message_lockres);
> + ret = dlm_unlock_sync(message_lockres);
> + if (unlikely(ret != 0))
> + pr_info("unlock msg failed return %d\n", ret);
> }
>
> /* lock_comm()
> @@ -567,7 +590,13 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg)
> }
>
> failed_ack:
> - dlm_unlock_sync(cinfo->message_lockres);
> + error = dlm_unlock_sync(cinfo->message_lockres);
> + if (unlikely(error != 0)) {
> + pr_err("md-cluster: failed convert to NL on MESSAGE(%d)\n",
> + error);
> + /* in case the message can't be released due to some reason */
> + goto failed_ack;
> + }
> failed_message:
> return error;
> }
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 10/12] md-cluster: only call complete(&cinfo->completion) when node join cluster
From: Goldwyn Rodrigues @ 2015-07-27 16:49 UTC (permalink / raw)
To: Guoqing Jiang, neilb; +Cc: linux-raid
In-Reply-To: <1436518883-12783-7-git-send-email-gqjiang@suse.com>
On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
> Introduce MD_CLUSTER_BEGIN_JOIN_CLUSTER flag to make sure
> complete(&cinfo->completion) is only be invoked when node
> join cluster. Otherwise node failure could also call the
> complete, and it doesn't make sense to do it.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/md-cluster.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 411b430..29f65e2 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -45,6 +45,7 @@ struct resync_info {
> /* md_cluster_info flags */
> #define MD_CLUSTER_WAITING_FOR_NEWDISK 1
> #define MD_CLUSTER_SUSPEND_READ_BALANCING 2
> +#define MD_CLUSTER_BEGIN_JOIN_CLUSTER 3
>
>
> struct md_cluster_info {
> @@ -320,10 +321,17 @@ static void recover_done(void *arg, struct dlm_slot *slots,
> struct md_cluster_info *cinfo = mddev->cluster_info;
>
> cinfo->slot_number = our_slot;
> - complete(&cinfo->completion);
> + /* completion is only need to be complete when node join cluster,
> + * it doesn't need to run during another node's failure */
> + if (test_bit(MD_CLUSTER_BEGIN_JOIN_CLUSTER, &cinfo->state)) {
> + complete(&cinfo->completion);
> + clear_bit(MD_CLUSTER_BEGIN_JOIN_CLUSTER, &cinfo->state);
> + }
> clear_bit(MD_CLUSTER_SUSPEND_READ_BALANCING, &cinfo->state);
> }
>
> +/* the ops is called when node join the cluster, and do lock recovery
> + * if node failure occurs */
> static const struct dlm_lockspace_ops md_ls_ops = {
> .recover_prep = recover_prep,
> .recover_slot = recover_slot,
> @@ -675,6 +683,7 @@ static int join(struct mddev *mddev, int nodes)
> INIT_LIST_HEAD(&cinfo->suspend_list);
> spin_lock_init(&cinfo->suspend_lock);
> init_completion(&cinfo->completion);
> + set_bit(MD_CLUSTER_BEGIN_JOIN_CLUSTER, &cinfo->state);
>
> mutex_init(&cinfo->sb_mutex);
> mddev->cluster_info = cinfo;
>
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 06/12] md-cluster: add the error check if failed to get dlm lock
From: Guoqing Jiang @ 2015-07-28 3:04 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: neilb, linux-raid
In-Reply-To: <55B660CD.2030600@suse.de>
Hi Goldwyn,
Goldwyn Rodrigues wrote:
> Hi Guoqing,
>
> On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
>> In complicated cluster environment, it is possible that the
>> dlm lock couldn't be get/convert on purpose, the related err
>> info is added for better debug potential issue.
>>
>> For lockres_free, if the lock is blocking by a lock request or
>> conversion request, then dlm_unlock just put it back to grant
>> queue, so need to ensure the lock is free finally.
>
>
> I cannot think of a scenario where a DLM_CANCEL will be returned.
> Could you explain the situation a bit more?
>
Thanks for the review. When the node is receiving message where it needs
to convert message
lock, and lockres_free is invoked if user stop array meanwhile, then the
message lock is put back
to grant queue at the CR mode and message lock is not released, am I
misunderstood the case?
Thanks,
Guoqing
^ permalink raw reply
* Re: [PATCH 12/12] md-cluster: handle error situations more precisely in lockres_init
From: Guoqing Jiang @ 2015-07-28 3:05 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: neilb, linux-raid
In-Reply-To: <55B65D9F.7010009@suse.de>
Hi Goldwyn,
> On 07/10/2015 04:01 AM, Guoqing Jiang wrote:
>> In lockres_init, it's better to distinguish different err conditions.
>>
>> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
>
> This is not required. kfree() is capable of ignoring null pointers.
Right, I missed about that.
Thanks,
Guoqing
^ permalink raw reply
* Re: [PATCH] block: add a bi_error field to struct bio
From: Christoph Hellwig @ 2015-07-28 11:12 UTC (permalink / raw)
To: Jens Axboe
Cc: Martin K. Petersen, Neil Brown, Liu Bo, linux-raid, dm-devel,
linux-btrfs, linux-kernel
In-Reply-To: <55B2699D.6010400@kernel.dk>
On Fri, Jul 24, 2015 at 10:36:45AM -0600, Jens Axboe wrote:
> Right, I don't think we need to do that though. If you look at the flags
> usage, it's all over the map. Some use test/set_bit, some set it just by
> OR'ing the mask. There's no reason we can't make this work without relying
> on set/test_bit, and then shrink it to an unsigned int.
Yes, the current mess doesn't look kosher. The bvec pool bits don't
really make it better.
But do we really need the cmpxchg hack? Seems like most flags aren't
exposed to concurrency at all, althugh this would need a careful audit.
^ permalink raw reply
* Re: [PATCH] block: add a bi_error field to struct bio
From: Jens Axboe @ 2015-07-28 14:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Martin K. Petersen, Neil Brown, Liu Bo, linux-raid, dm-devel,
linux-btrfs, linux-kernel
In-Reply-To: <20150728111249.GA19647@lst.de>
On 07/28/2015 05:12 AM, Christoph Hellwig wrote:
> On Fri, Jul 24, 2015 at 10:36:45AM -0600, Jens Axboe wrote:
>> Right, I don't think we need to do that though. If you look at the flags
>> usage, it's all over the map. Some use test/set_bit, some set it just by
>> OR'ing the mask. There's no reason we can't make this work without relying
>> on set/test_bit, and then shrink it to an unsigned int.
>
> Yes, the current mess doesn't look kosher. The bvec pool bits don't
> really make it better.
>
> But do we really need the cmpxchg hack? Seems like most flags aren't
> exposed to concurrency at all, althugh this would need a careful audit.
I actually don't think that we do need it at all. With the uptodate bit
gone, we really should not have any concurrency issues on it at all.
CHAIN and REFFED need serialization, but that is already done previous
to this change.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH v2 3/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation
From: Pavel Machek @ 2015-07-28 14:44 UTC (permalink / raw)
To: Pali Rohár
Cc: Alasdair Kergon, Mike Snitzer, Neil Brown, Rafael J. Wysocki,
Len Brown, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <1434885634-19895-4-git-send-email-pali.rohar@gmail.com>
On Sun 2015-06-21 13:20:34, Pali Rohár wrote:
> This patch adds dm message commands and option strings to optionally wipe key
> from dm-crypt device before entering suspend or hibernate state.
>
> Before key is wiped dm device must be suspended. To prevent race conditions with
> I/O and userspace processes, wiping action must be called after processes are
> freezed. Otherwise userspace processes could start reading/writing to disk after
> dm device is suspened and freezing processes before suspend/hibernate action
> will fail.
Are you sure this is enough?
We still may need to allocate memory after userspace is frozen, and
that could mean writing dirty buffers out to make some memory free...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox