* [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
* Re: [PATCH] block: add a bi_error field to struct bio
From: Jens Axboe @ 2015-07-24 16:36 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: <20150724104946.GA25145@lst.de>
On 07/24/2015 04:49 AM, Christoph Hellwig wrote:
> On Wed, Jul 22, 2015 at 03:59:46PM -0600, Jens Axboe wrote:
>> One possible solution would be to shrink bi_flags to an unsigned int, no
>> problems fitting that in. Then we could stuff bi_error in that (new) hole,
>> and we would end up having the same size again.
>
> As long as we use set/test/clear_bt on bi_flags that won't work unfortunately.
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.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH] raid0: data corruption when using trim
From: Martin K. Petersen @ 2015-07-24 15:03 UTC (permalink / raw)
To: Gionatan Danti
Cc: Seunguk Shin, 'Martin K. Petersen', neilb, linux-raid
In-Reply-To: <55B1DF90.7090904@assyoma.it>
>>>>> "Gionatan" == Gionatan Danti <g.danti@assyoma.it> writes:
Gionatan> Hi, any idea on why the bug affects/manifests only on specific
Gionatan> SATA SSDs?
Timing and a very heavy discard load.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH] raid0: data corruption when using trim
From: Gionatan Danti @ 2015-07-24 14:42 UTC (permalink / raw)
To: Seunguk Shin; +Cc: linux-raid, g.danti
In-Reply-To: <023401d0c60a$fff39330$ffdab990$@samsung.com>
Hi Seunguk,
thank you for your further explanation.
Does it means that only driver with Queued TRIM are affected, or it
affect all TRIM-capable driver?
What I wounder is that, if it is only a timing-triggered problem, why it
is only apparent on specific SSDs? I would expect it to manifest on
other devices also, albeit with different frequency.
Sorry if some questions sound dumb, I'm only trying to understand.
Thanks.
On 24/07/15 14:19, Seunguk Shin wrote:
> Gionatan,
>
> Because it is related with timing.
>
> 1st trim is issued and before complete it, 2nd trim is started
> (allocates memory but is not issued to device)
> Then, when 1st trim is completed, it frees 2nd trim's memory,
> because they share the pointer due to bug.
> If 3rd trim is started before 2nd trim is issued to device,
> 3rd trim can allocate the same memory with 2nd trim,
> because it is freed when 1st trim is completed.
> Only this case makes the corruption.
>
> Thank you,
> Seunguk Shin
>
>>
>> Hi, any idea on why the bug affects/manifests only on specific SATA SSDs?
>> It is timing related, or it depends on how the controller manage TRIM
>> requests?
>>
>> Thanks.
>>
>> --
>> Danti Gionatan
>> Supporto Tecnico
>> Assyoma S.r.l. - www.assyoma.it
>> email: g.danti@assyoma.it - info@assyoma.it GPG public key ID: FF5F32A8
>> --
>> 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
>
--
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8
^ permalink raw reply
* Re: [PATCH] block: add a bi_error field to struct bio
From: Christoph Hellwig @ 2015-07-24 10:49 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: <55B01252.90004@kernel.dk>
On Wed, Jul 22, 2015 at 03:59:46PM -0600, Jens Axboe wrote:
> One possible solution would be to shrink bi_flags to an unsigned int, no
> problems fitting that in. Then we could stuff bi_error in that (new) hole,
> and we would end up having the same size again.
As long as we use set/test/clear_bt on bi_flags that won't work unfortunately.
^ permalink raw reply
* Re: Does sync_action repair raid6 zero mismatch_cnt?
From: Robin Hill @ 2015-07-24 7:47 UTC (permalink / raw)
To: John Bridges; +Cc: linux-raid
In-Reply-To: <CAJ8DDcPoUayq2ucykkPVEYXyGpTVM-Cq3rzZ5mxitiP1tjH=DA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]
On Thu Jul 23, 2015 at 06:07:34PM -0400, John Bridges wrote:
> I have had a problem with mismatch_cnt being huge (well over 500000) on a raid6
>
> /dev/md2:
> Version : 1.0
> Creation Time : Mon Oct 15 20:55:58 2012
> Raid Level : raid6
> Array Size : 35163174912 (33534.22 GiB 36007.09 GB)
> Used Dev Size : 2930264576 (2794.52 GiB 3000.59 GB)
> Raid Devices : 14
> Total Devices : 15
> Persistence : Superblock is persistent
>
> Intent Bitmap : Internal
>
> Update Time : Wed Jul 22 13:04:39 2015
> State : active
> Active Devices : 14
> Working Devices : 15
> Failed Devices : 0
> Spare Devices : 1
>
> Layout : left-symmetric
> Chunk Size : 512K
>
> Name : monster:2 (local to host monster)
> UUID : c9f7456d:39b575fd:5d8e3791:54bffeae
> Events : 459283
>
> Number Major Minor RaidDevice State
> 19 65 129 0 active sync /dev/sdy1
> 1 65 145 1 active sync /dev/sdz1
> 17 65 17 2 active sync /dev/sdr1
> 12 8 193 3 active sync /dev/sdm1
> 8 65 1 4 active sync /dev/sdq1
> 5 65 65 5 active sync /dev/sdu1
> 6 65 81 6 active sync /dev/sdv1
> 7 65 97 7 active sync /dev/sdw1
> 16 8 225 8 active sync /dev/sdo1
> 18 8 209 9 active sync /dev/sdn1
> 15 8 241 10 active sync /dev/sdp1
> 14 65 113 11 active sync /dev/sdx1
> 13 65 177 12 active sync /dev/sdab1
> 20 65 161 13 active sync /dev/sdaa1
>
> 21 8 177 - spare /dev/sdl1
>
>
> mdadm - v3.2.5 - 18th May 2012
>
>
> sync_action check did not fix it.
> I tried sync_action repair, still a huge number.
> Then I ran raid6check (built from latest mdadm source), took over a week to run.
> Found no errors, I was expecting a flaky drive since mismatch_cnt was so huge.
> raid6check does not update mismatch_cnt, so I did a sync_action check
> which finally zeroed the mismatch_cnt.
> I don't know if the newer raid6check fixed it or the repair?
>
> If mismatch_cnt is non zero on a raid6, do I need to do a repair and
> then a check? I thought repair would update mismatch_cnt.
>
>
> Does not inspire confidence.
>
Repair updates mismatch_cnt with the number of mismatches repaired, so
at the end this should equal the number reported by check. I would
always recommend re-running the check after a repair anyway.
That number of mismatches would suggest a major issue with your array
though. I would suggest checking SMART statistics and running full SMART
tests on all member disks.
Cheers,
Robin
--
___
( ' } | Robin Hill <robin@robinhill.me.uk> |
/ / ) | Little Jim says .... |
// !! | "He fallen in de water !!" |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH] raid0: data corruption when using trim
From: Gionatan Danti @ 2015-07-24 6:47 UTC (permalink / raw)
To: Seunguk Shin, 'Martin K. Petersen'; +Cc: neilb, linux-raid, g.danti
In-Reply-To: <005601d0c470$951ea860$bf5bf920$@samsung.com>
> Martine,
>
> I have tested the script from Algolia with your patch, I cannot see the same
> symptoms I saw with original kernel. I think the patch works and there is no
> problem.
>
> Thank you,
> Seunguk Shin
>
Hi, any idea on why the bug affects/manifests only on specific SATA
SSDs? It is timing related, or it depends on how the controller manage
TRIM requests?
Thanks.
--
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8
^ permalink raw reply
* Re: [PATCH] raid0: data corruption when using trim
From: Gionatan Danti @ 2015-07-24 6:37 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Piergiorgio Sartor, Seunguk Shin, neilb, linux-raid, g.danti
In-Reply-To: <yq1lhe6zfgx.fsf@sermon.lab.mkp.net>
>
> The problem only occurs if you are using MD linear/raid0/raid10 on top
> of a device that implements either DSM TRIM (SATA) or UNMAP (SCSI). If
> the device prefers WRITE SAME w/ UNMAP or if it does not support
> discards at all there is no problem.
>
Thank you for confirmation! :)
--
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8
^ permalink raw reply
* Subject: ANNOUNCE: mdadm 3.3.3 - A tool for managing md Soft RAID under Linux
From: NeilBrown @ 2015-07-24 6:35 UTC (permalink / raw)
To: linux-raid
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
^ permalink raw reply
* Re: raid1_end_read_request does not retry failed READ from a recovering drive
From: NeilBrown @ 2015-07-23 23:24 UTC (permalink / raw)
To: Alexander Lyakas; +Cc: linux-raid
In-Reply-To: <CAGRgLy5DsbiPsjJ2ZK_aOZ_UOqxa-zwbdDREv7hdpt7vLXCwOw@mail.gmail.com>
On Wed, 22 Jul 2015 18:10:31 +0200 Alexander Lyakas
<alex.bolshoy@gmail.com> wrote:
> Hi Neil,
> In continuation of our discussion, I see that you have added a
> commit[1], which has a diff[2].
> But this is only the second part of the fix. We also need the first
> part, I believe, where in raid1_end_read_request we need to replace
> "!test_bit(Faulty)" with "test_bit(In_sync)". Otherwise, we will never
> retry the READ, and thus will never reach the fix_read_error code.
> And we also need to "put all of raid1_spare_active inside the
> spinlock", like you advised.
> Do you agree?
>
> We tested both parts of the fix, not the second part alone. Quoting myself:
> "With this addition, the problem appears to be fixed", i.e., I meant
> we applied both parts.
>
> I am sorry for catching this so late. I never looked at what you
> applied until now, because we are moving to kernel 3.18 long term, and
> I am checking the raid1 changes. I just assumed you applied both
> parts.
>
> If you agree, it would be good if you tag the first part of the fix as
> "cc stable" too.
>
> Thanks,
> Alex.
>
>
>
> [1]
> commit b8cb6b4c121e1bf1963c16ed69e7adcb1bc301cd
> Author: NeilBrown <neilb@suse.de>
> Date: Thu Sep 18 11:09:04 2014 +1000
>
> md/raid1: fix_read_error should act on all non-faulty devices.
>
> If a devices is being recovered it is not InSync and is not Faulty.
>
> If a read error is experienced on that device, fix_read_error()
> will be called, but it ignores non-InSync devices. So it will
> neither fix the error nor fail the device.
>
> It is incorrect that fix_read_error() ignores non-InSync devices.
> It should only ignore Faulty devices. So fix it.
>
> This became a bug when we allowed reading from a device that was being
> recovered. It is suitable for any subsequent -stable kernel.
>
> Fixes: da8840a747c0dbf49506ec906757a6b87b9741e9
> Cc: stable@vger.kernel.org (v3.5+)
> Reported-by: Alexander Lyakas <alex.bolshoy@gmail.com>
> Tested-by: Alexander Lyakas <alex.bolshoy@gmail.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
>
> [2]
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 35649dd..55de4f6 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2155,7 +2155,7 @@ static void fix_read_error(struct r1conf *conf,
> int read_disk,
> d--;
> rdev = conf->mirrors[d].rdev;
> if (rdev &&
> - test_bit(In_sync, &rdev->flags))
> + !test_bit(Faulty, &rdev->flags))
> r1_sync_page_io(rdev, sect, s,
> conf->tmppage, WRITE);
> }
> @@ -2167,7 +2167,7 @@ static void fix_read_error(struct r1conf *conf,
> int read_disk,
> d--;
> rdev = conf->mirrors[d].rdev;
> if (rdev &&
> - test_bit(In_sync, &rdev->flags)) {
> + !test_bit(Faulty, &rdev->flags)) {
> if (r1_sync_page_io(rdev, sect, s,
> conf->tmppage, READ)) {
> atomic_add(s, &rdev->corrected_errors);
>
> On Mon, Sep 22, 2014 at 2:17 AM, NeilBrown <neilb@suse.de> wrote:
> > On Sun, 21 Sep 2014 19:47:14 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> > wrote:
> >
> >> Thanks, Neil,
> >>
> >> On Thu, Sep 18, 2014 at 4:05 AM, NeilBrown <neilb@suse.de> wrote:
> >> > On Wed, 17 Sep 2014 20:57:13 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> >> > wrote:
> >> >
> >> >> Hi Neil,
> >> >>
> >> >> On Mon, Sep 8, 2014 at 10:17 AM, NeilBrown <neilb@suse.de> wrote:
> >> >> > On Sun, 7 Sep 2014 17:18:16 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> >> >> > wrote:
> >> >> >
> >> >> >> Hi Neil,
> >> >> >> we see the following issue:
> >> >> >>
> >> >> >> # RAID1 has 2 drives A and B, drive B is recovering
> >> >> >> # READ request arrives
> >> >> >> # read_balanace selects drive B to read from, because READ sector
> >> >> >> comes before B->recovery_offset
> >> >> >> # READ is issued to drive B, but fails (drive B fails again)
> >> >> >>
> >> >> >> Now raid1_end_read_request() has the following code:
> >> >> >>
> >> >> >> 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);
> >> >> >> 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;
> >> >> >> spin_unlock_irqrestore(&conf->device_lock, flags);
> >> >> >> }
> >> >> >>
> >> >> >> According to this code uptodate wrongly becomes 1, because:
> >> >> >> r1_bio->mddev->degraded == conf->raid_disks-1 is TRUE
> >> >> >> and
> >> >> >> !test_bit(Faulty, &conf->mirrors[mirror].rdev->flags) is also TRUE
> >> >> >>
> >> >> >> Indeed, drive B is not marked as Faulty, but also not marked as In_sync.
> >> >> >> However, this function treats !Faulty being equal to In_Sync, so it
> >> >> >> decides that the last good drive failed, so it does not retry the
> >> >> >> READ.
> >> >> >>
> >> >> >> As a result, there is IO error, while we should have retried the READ
> >> >> >> from the healthy drive.
> >> >> >>
> >> >> >> This is happening in 3.8.13, but your master branch seems to have the
> >> >> >> same issue.
> >> >> >>
> >> >> >> What is a reasonable fix?
> >> >> >> 1) Do not read from drives which are !In_sync (a bit scary to read
> >> >> >> from such drive)
> >> >> >
> >> >> > It is perfectly safe to read from a !In_sync device providing you are before
> >> >> > ->recovery_offset.
> >> >> >
> >> >> >
> >> >> >> 2) replace !Faulty to In_sync check
> >> >> >
> >> >> > That probably makes sense... though that could race with raid1_spare_active().
> >> >> > If a read-error returned just after raid1_spare_active() set In_sync, and
> >> >> > before 'count' was subtracted from ->degraded, we would still set uptodate
> >> >> > when we shouldn't.
> >> >> > It probably make sense to put all of raid1_spare_active inside the spinlock -
> >> >> > it doesn't get call often enough that performance is an issue (I hope).
> >> >> >
> >> >> > So:
> >> >> > 1/ change !Faulty to In_sync
> >> >> > 2/ extend the spinlock in raid1_spare_active to cover the whole function.
> >> >>
> >> >>
> >> >> I made these fixes and reproduced the issue. However, the result is
> >> >> not what we expect:
> >> >>
> >> >> # raid1_end_read_request() now indeed adds the r1_bio into retry_list,
> >> >> as we wanted
> >> >> # raid1d calls fix_read_error()
> >> >> # fix_read_error() searches for an In_sync drive to read the data
> >> >> from. It finds such drive (this is our good drive A)
> >> >> # now fix_read_error() wants to rewrite the bad area. But it rewrites
> >> >> only on those drives that are In_sync (except the drive it got the
> >> >> data from). In our case, it never tries to rewrite the data on drive B
> >> >> (drive B is not marked Faulty and not marked In_sync). As a result,
> >> >> md_error() is not called, so drive B is still not marked as Failed
> >> >> when fix_read_error() completes
> >> >> # so handle_read_error() retries the original READ by calling
> >> >> read_balance(), which again in my case selects the recovering drive
> >> >> B...
> >> >>
> >> >> And then the whole flow repeats itself again and again...and READ
> >> >> never completes.
> >> >>
> >> >> Maybe we should not allow selecting recovering drives for READ? Or
> >> >> some other approach?
> >> >>
> >> >
> >> > Thanks for the testing and analysis.
> >> > Presumably we just want handle_read_error() to write to all non-faulty
> >> > devices, not just the InSync ones.
> >> > i.e. the following patch.
> >> >
> >> > Thanks,
> >> > NeilBrown
> >> >
> >> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> >> > index 6a9c73435eb8..a95f9e179e6f 100644
> >> > --- a/drivers/md/raid1.c
> >> > +++ b/drivers/md/raid1.c
> >> > @@ -2153,7 +2153,7 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
> >> > d--;
> >> > rdev = conf->mirrors[d].rdev;
> >> > if (rdev &&
> >> > - test_bit(In_sync, &rdev->flags))
> >> > + !test_bit(Faulty, &rdev->flags))
> >> > r1_sync_page_io(rdev, sect, s,
> >> > conf->tmppage, WRITE);
> >> > }
> >> > @@ -2165,7 +2165,7 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
> >> > d--;
> >> > rdev = conf->mirrors[d].rdev;
> >> > if (rdev &&
> >> > - test_bit(In_sync, &rdev->flags)) {
> >> > + !test_bit(Faulty, &rdev->flags)) {
> >> > if (r1_sync_page_io(rdev, sect, s,
> >> > conf->tmppage, READ)) {
> >> > atomic_add(s, &rdev->corrected_errors);
> >>
> >> With this addition, the problem appears to be fixed. We will give it
> >> some regression testing & will let you know if we see any issues.
> >>
> >> I presume you will be applying this fix upstream as well.
> >
> > Yes, it is already in my for-next branch.
> > I've just added your tested-by.
> >
> > Thanks,
> > NeilBrown
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);
}
^ permalink raw reply related
* Re: [PATCH] raid0: data corruption when using trim
From: Martin K. Petersen @ 2015-07-23 22:17 UTC (permalink / raw)
To: Gionatan Danti
Cc: Martin K. Petersen, Piergiorgio Sartor, Seunguk Shin, neilb,
linux-raid
In-Reply-To: <55B11A68.4080706@assyoma.it>
>>>>> "Gionatan" == Gionatan Danti <g.danti@assyoma.it> writes:
Gionatan> I *think* that, as this TRIM command is processed way above
Gionatan> the ATA layer (it is processed inside the device-mapper code),
Gionatan> the bug should not happen here. Is it correct? Are we safe
Gionatan> with ThinLVM + MDRAID10 + HDDs?
The problem only occurs if you are using MD linear/raid0/raid10 on top
of a device that implements either DSM TRIM (SATA) or UNMAP (SCSI). If
the device prefers WRITE SAME w/ UNMAP or if it does not support
discards at all there is no problem.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Does sync_action repair raid6 zero mismatch_cnt?
From: John Bridges @ 2015-07-23 22:07 UTC (permalink / raw)
To: linux-raid
I have had a problem with mismatch_cnt being huge (well over 500000) on a raid6
/dev/md2:
Version : 1.0
Creation Time : Mon Oct 15 20:55:58 2012
Raid Level : raid6
Array Size : 35163174912 (33534.22 GiB 36007.09 GB)
Used Dev Size : 2930264576 (2794.52 GiB 3000.59 GB)
Raid Devices : 14
Total Devices : 15
Persistence : Superblock is persistent
Intent Bitmap : Internal
Update Time : Wed Jul 22 13:04:39 2015
State : active
Active Devices : 14
Working Devices : 15
Failed Devices : 0
Spare Devices : 1
Layout : left-symmetric
Chunk Size : 512K
Name : monster:2 (local to host monster)
UUID : c9f7456d:39b575fd:5d8e3791:54bffeae
Events : 459283
Number Major Minor RaidDevice State
19 65 129 0 active sync /dev/sdy1
1 65 145 1 active sync /dev/sdz1
17 65 17 2 active sync /dev/sdr1
12 8 193 3 active sync /dev/sdm1
8 65 1 4 active sync /dev/sdq1
5 65 65 5 active sync /dev/sdu1
6 65 81 6 active sync /dev/sdv1
7 65 97 7 active sync /dev/sdw1
16 8 225 8 active sync /dev/sdo1
18 8 209 9 active sync /dev/sdn1
15 8 241 10 active sync /dev/sdp1
14 65 113 11 active sync /dev/sdx1
13 65 177 12 active sync /dev/sdab1
20 65 161 13 active sync /dev/sdaa1
21 8 177 - spare /dev/sdl1
mdadm - v3.2.5 - 18th May 2012
sync_action check did not fix it.
I tried sync_action repair, still a huge number.
Then I ran raid6check (built from latest mdadm source), took over a week to run.
Found no errors, I was expecting a flaky drive since mismatch_cnt was so huge.
raid6check does not update mismatch_cnt, so I did a sync_action check
which finally zeroed the mismatch_cnt.
I don't know if the newer raid6check fixed it or the repair?
If mismatch_cnt is non zero on a raid6, do I need to do a repair and
then a check? I thought repair would update mismatch_cnt.
Does not inspire confidence.
^ permalink raw reply
* Re: [PATCH] raid0: data corruption when using trim
From: Gionatan Danti @ 2015-07-23 16:46 UTC (permalink / raw)
To: Martin K. Petersen, Piergiorgio Sartor
Cc: Seunguk Shin, neilb, linux-raid, g.danti
In-Reply-To: <yq11tg22kyz.fsf@sermon.lab.mkp.net>
>
> Piergiorgio> There is LVM on top, I wonder if this makes a difference.
>
> It does not.
>
Hi all,
it is my understanding that, when backed by SSDs (and configured for
passing down TRIM requests), ThinLVM are prime condidate for trigger the
bug.
But what about ThinLVM + MDRAID10 on top of normal (spinning HDD)?
Generally SATA HDDs do not support TRIM/UNMAP, but when using ThinLVM,
the thin-provision mapper advertise TRIM support, and indeed an "fstrim
-v /" succeeds.
I *think* that, as this TRIM command is processed way above the ATA
layer (it is processed inside the device-mapper code), the bug should
not happen here. Is it correct? Are we safe with ThinLVM + MDRAID10 + HDDs?
Thanks.
--
Danti Gionatan
Supporto Tecnico
Assyoma S.r.l. - www.assyoma.it
email: g.danti@assyoma.it - info@assyoma.it
GPG public key ID: FF5F32A8
^ permalink raw reply
* [PATCH] dm cache: fix alloc_bitset check that always evaluates as false
From: Colin King @ 2015-07-23 15:47 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, dm-devel, Neil Brown, linux-raid
Cc: linux-kernel
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>
---
drivers/md/dm-cache-policy-smq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm-cache-policy-smq.c b/drivers/md/dm-cache-policy-smq.c
index b6f2265..48a4a82 100644
--- a/drivers/md/dm-cache-policy-smq.c
+++ b/drivers/md/dm-cache-policy-smq.c
@@ -1686,7 +1686,7 @@ static struct dm_cache_policy *smq_create(dm_cblock_t cache_size,
if (from_cblock(cache_size)) {
mq->cache_hit_bits = alloc_bitset(from_cblock(cache_size));
- if (!mq->cache_hit_bits && mq->cache_hit_bits) {
+ if (!mq->cache_hit_bits) {
DMERR("couldn't allocate cache hit bitset");
goto bad_cache_hit_bits;
}
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 3/3] Skip cluster setup for dm-raid
From: NeilBrown @ 2015-07-22 23:21 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-raid
In-Reply-To: <1437584957-23879-3-git-send-email-rgoldwyn@suse.com>
On Wed, 22 Jul 2015 12:09:17 -0500 Goldwyn Rodrigues
<rgoldwyn@suse.com> wrote:
> There is a bug that the bitmap superblock isn't initialised properly for
> dm-raid, so a new field can have garbage in new fields.
> (dm-raid does initialisation in the kernel - md initialised the
> superblock in mdadm).
>
> This means that for dm-raid we cannot currently trust the new ->nodes
> field. So:
> - use __GFP_ZERO to initialise the superblock properly for all new
> arrays
> - initialise all field in bitmap_info in bitmap_new_disk_sb
> - ignore ->nodes for dm arrays (yes, this is a hack)
>
> References: https://bugzilla.kernel.org/show_bug.cgi?id=100491
>
> Signed-off-By: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/bitmap.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index f23b8e4..7ff37e0 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -494,7 +494,7 @@ static int bitmap_new_disk_sb(struct bitmap *bitmap)
> bitmap_super_t *sb;
> unsigned long chunksize, daemon_sleep, write_behind;
>
> - bitmap->storage.sb_page = alloc_page(GFP_KERNEL);
> + bitmap->storage.sb_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> if (bitmap->storage.sb_page == NULL)
> return -ENOMEM;
> bitmap->storage.sb_page->index = 0;
> @@ -541,6 +541,7 @@ static int bitmap_new_disk_sb(struct bitmap *bitmap)
> sb->state = cpu_to_le32(bitmap->flags);
> bitmap->events_cleared = bitmap->mddev->events;
> sb->events_cleared = cpu_to_le64(bitmap->mddev->events);
> + bitmap->mddev->bitmap_info.nodes = 0;
>
> kunmap_atomic(sb);
>
> @@ -611,8 +612,16 @@ re_read:
> daemon_sleep = le32_to_cpu(sb->daemon_sleep) * HZ;
> write_behind = le32_to_cpu(sb->write_behind);
> sectors_reserved = le32_to_cpu(sb->sectors_reserved);
> - nodes = le32_to_cpu(sb->nodes);
> - strlcpy(bitmap->mddev->bitmap_info.cluster_name, sb->cluster_name, 64);
> + /* XXX: This is a hack to ensure that we don't use clustering
> + * in case:
> + * - dm-raid is in use and
> + * - the nodes written in bitmap_sb is erroneous.
> + */
> + if (!bitmap->mddev->sync_super) {
> + nodes = le32_to_cpu(sb->nodes);
> + strlcpy(bitmap->mddev->bitmap_info.cluster_name,
> + sb->cluster_name, 64);
> + }
>
> /* verify that the bitmap-specific fields are valid */
> if (sb->magic != cpu_to_le32(BITMAP_MAGIC))
Thanks for these.
I've applied them all and marked this one for -stable ... it should stop
the cluster code from being touched by mistake.
NeilBrown
^ permalink raw reply
* Re: [PATCH] block: add a bi_error field to struct bio
From: Jens Axboe @ 2015-07-22 21:59 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: <55AFE643.5000704@kernel.dk>
On 07/22/2015 12:51 PM, Jens Axboe wrote:
> On 07/20/2015 07:29 AM, Christoph Hellwig wrote:
>> Currently we have two different ways to signal an I/O error on a BIO:
>>
>> (1) by clearing the BIO_UPTODATE flag
>> (2) by returning a Linux errno value to the bi_end_io callback
>>
>> The first one has the drawback of only communicating a single possible
>> error (-EIO), and the second one has the drawback of not beeing
>> persistent
>> when bios are queued up, and are not passed along from child to parent
>> bio in the ever more popular chaining scenario. Having both mechanisms
>> available has the additional drawback of utterly confusing driver authors
>> and introducing bugs where various I/O submitters only deal with one of
>> them, and the others have to add boilerplate code to deal with both kinds
>> of error returns.
>>
>> So add a new bi_error field to store an errno value directly in struct
>> bio and remove the existing mechanisms to clean all this up.
>
> I think this is a good change, the only part I _really_ dislike is that
> this now bumps a struct bio from 2 cache lines to 3. Have you done any
> perf testing?
One possible solution would be to shrink bi_flags to an unsigned int, no
problems fitting that in. Then we could stuff bi_error in that (new)
hole, and we would end up having the same size again.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH] block: add a bi_error field to struct bio
From: Jens Axboe @ 2015-07-22 18:51 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: <1437398977-8492-2-git-send-email-hch@lst.de>
On 07/20/2015 07:29 AM, Christoph Hellwig wrote:
> Currently we have two different ways to signal an I/O error on a BIO:
>
> (1) by clearing the BIO_UPTODATE flag
> (2) by returning a Linux errno value to the bi_end_io callback
>
> The first one has the drawback of only communicating a single possible
> error (-EIO), and the second one has the drawback of not beeing persistent
> when bios are queued up, and are not passed along from child to parent
> bio in the ever more popular chaining scenario. Having both mechanisms
> available has the additional drawback of utterly confusing driver authors
> and introducing bugs where various I/O submitters only deal with one of
> them, and the others have to add boilerplate code to deal with both kinds
> of error returns.
>
> So add a new bi_error field to store an errno value directly in struct
> bio and remove the existing mechanisms to clean all this up.
I think this is a good change, the only part I _really_ dislike is that
this now bumps a struct bio from 2 cache lines to 3. Have you done any
perf testing?
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH] block: Do a full clone when splitting discard bios
From: Christoph Hellwig @ 2015-07-22 17:17 UTC (permalink / raw)
To: Martin K. Petersen
Cc: axboe, linux-raid, neilb, Seunguk Shin, Kent Overstreet, stable
In-Reply-To: <1437566232-25422-1-git-send-email-martin.petersen@oracle.com>
On Wed, Jul 22, 2015 at 07:57:12AM -0400, Martin K. Petersen wrote:
> This fixes a data corruption bug when using discard on top of MD linear,
> raid0 and raid10 personalities.
>
> Commit 20d0189b1012 "block: Introduce new bio_split()" permits sharing
> the bio_vec between the two resulting bios. That is fine for read/write
> requests where the bio_vec is immutable. For discards, however, we need
> to be able to attach a payload and update the bio_vec so the page can
> get mapped to a scatterlist entry. Therefore the bio_vec can not be
> shared when splitting discards and we must do a full clone.
Eww. The magic discard payload comes bac to bite us again. I wish
someone smarter than me could come up with a scheme to avoid this issue.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* [PATCH 3/3] Skip cluster setup for dm-raid
From: Goldwyn Rodrigues @ 2015-07-22 17:09 UTC (permalink / raw)
To: linux-raid; +Cc: neilb, Goldwyn Rodrigues
In-Reply-To: <1437584957-23879-1-git-send-email-rgoldwyn@suse.com>
There is a bug that the bitmap superblock isn't initialised properly for
dm-raid, so a new field can have garbage in new fields.
(dm-raid does initialisation in the kernel - md initialised the
superblock in mdadm).
This means that for dm-raid we cannot currently trust the new ->nodes
field. So:
- use __GFP_ZERO to initialise the superblock properly for all new
arrays
- initialise all field in bitmap_info in bitmap_new_disk_sb
- ignore ->nodes for dm arrays (yes, this is a hack)
References: https://bugzilla.kernel.org/show_bug.cgi?id=100491
Signed-off-By: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
drivers/md/bitmap.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index f23b8e4..7ff37e0 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -494,7 +494,7 @@ static int bitmap_new_disk_sb(struct bitmap *bitmap)
bitmap_super_t *sb;
unsigned long chunksize, daemon_sleep, write_behind;
- bitmap->storage.sb_page = alloc_page(GFP_KERNEL);
+ bitmap->storage.sb_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
if (bitmap->storage.sb_page == NULL)
return -ENOMEM;
bitmap->storage.sb_page->index = 0;
@@ -541,6 +541,7 @@ static int bitmap_new_disk_sb(struct bitmap *bitmap)
sb->state = cpu_to_le32(bitmap->flags);
bitmap->events_cleared = bitmap->mddev->events;
sb->events_cleared = cpu_to_le64(bitmap->mddev->events);
+ bitmap->mddev->bitmap_info.nodes = 0;
kunmap_atomic(sb);
@@ -611,8 +612,16 @@ re_read:
daemon_sleep = le32_to_cpu(sb->daemon_sleep) * HZ;
write_behind = le32_to_cpu(sb->write_behind);
sectors_reserved = le32_to_cpu(sb->sectors_reserved);
- nodes = le32_to_cpu(sb->nodes);
- strlcpy(bitmap->mddev->bitmap_info.cluster_name, sb->cluster_name, 64);
+ /* XXX: This is a hack to ensure that we don't use clustering
+ * in case:
+ * - dm-raid is in use and
+ * - the nodes written in bitmap_sb is erroneous.
+ */
+ if (!bitmap->mddev->sync_super) {
+ nodes = le32_to_cpu(sb->nodes);
+ strlcpy(bitmap->mddev->bitmap_info.cluster_name,
+ sb->cluster_name, 64);
+ }
/* verify that the bitmap-specific fields are valid */
if (sb->magic != cpu_to_le32(BITMAP_MAGIC))
--
2.1.4
^ permalink raw reply related
* [PATCH 2/3] Skip cluster setup in case of error while reading bitmap
From: Goldwyn Rodrigues @ 2015-07-22 17:09 UTC (permalink / raw)
To: linux-raid; +Cc: neilb, Goldwyn Rodrigues
In-Reply-To: <1437584957-23879-1-git-send-email-rgoldwyn@suse.com>
If the bitmap read fails, the error code set is -EINVAL. However,
we don't check for errors and go ahead with cluster_setup.
Skip the cluster setup in case of error.
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
drivers/md/bitmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
index ed2346d..f23b8e4 100644
--- a/drivers/md/bitmap.c
+++ b/drivers/md/bitmap.c
@@ -671,7 +671,7 @@ out:
kunmap_atomic(sb);
/* Assiging chunksize is required for "re_read" */
bitmap->mddev->bitmap_info.chunksize = chunksize;
- if (nodes && (bitmap->cluster_slot < 0)) {
+ if (err == 0 && nodes && (bitmap->cluster_slot < 0)) {
err = md_setup_cluster(bitmap->mddev, nodes);
if (err) {
pr_err("%s: Could not setup cluster service (%d)\n",
--
2.1.4
^ permalink raw reply related
* [PATCH 1/3] Return error if request_module fails and returns positive value
From: Goldwyn Rodrigues @ 2015-07-22 17:09 UTC (permalink / raw)
To: linux-raid; +Cc: neilb, Goldwyn Rodrigues
request_module() can return 256 (process exited) in some cases,
which is not as specified in the documentation before the
request_module() definition. Convert the error to -ENOENT.
This fixes edb39c9deda8 ("Introduce md_cluster_operations to
handle cluster functions")
Signed-off-By: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
drivers/md/md.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d429c30..7a7870f 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7437,7 +7437,7 @@ int md_setup_cluster(struct mddev *mddev, int nodes)
err = request_module("md-cluster");
if (err) {
pr_err("md-cluster module not found.\n");
- return err;
+ return -ENOENT;
}
spin_lock(&pers_lock);
--
2.1.4
^ permalink raw reply related
* Re: raid1_end_read_request does not retry failed READ from a recovering drive
From: Alexander Lyakas @ 2015-07-22 16:10 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <20140922101704.53be2056@notabene.brown>
Hi Neil,
In continuation of our discussion, I see that you have added a
commit[1], which has a diff[2].
But this is only the second part of the fix. We also need the first
part, I believe, where in raid1_end_read_request we need to replace
"!test_bit(Faulty)" with "test_bit(In_sync)". Otherwise, we will never
retry the READ, and thus will never reach the fix_read_error code.
And we also need to "put all of raid1_spare_active inside the
spinlock", like you advised.
Do you agree?
We tested both parts of the fix, not the second part alone. Quoting myself:
"With this addition, the problem appears to be fixed", i.e., I meant
we applied both parts.
I am sorry for catching this so late. I never looked at what you
applied until now, because we are moving to kernel 3.18 long term, and
I am checking the raid1 changes. I just assumed you applied both
parts.
If you agree, it would be good if you tag the first part of the fix as
"cc stable" too.
Thanks,
Alex.
[1]
commit b8cb6b4c121e1bf1963c16ed69e7adcb1bc301cd
Author: NeilBrown <neilb@suse.de>
Date: Thu Sep 18 11:09:04 2014 +1000
md/raid1: fix_read_error should act on all non-faulty devices.
If a devices is being recovered it is not InSync and is not Faulty.
If a read error is experienced on that device, fix_read_error()
will be called, but it ignores non-InSync devices. So it will
neither fix the error nor fail the device.
It is incorrect that fix_read_error() ignores non-InSync devices.
It should only ignore Faulty devices. So fix it.
This became a bug when we allowed reading from a device that was being
recovered. It is suitable for any subsequent -stable kernel.
Fixes: da8840a747c0dbf49506ec906757a6b87b9741e9
Cc: stable@vger.kernel.org (v3.5+)
Reported-by: Alexander Lyakas <alex.bolshoy@gmail.com>
Tested-by: Alexander Lyakas <alex.bolshoy@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>
[2]
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 35649dd..55de4f6 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2155,7 +2155,7 @@ static void fix_read_error(struct r1conf *conf,
int read_disk,
d--;
rdev = conf->mirrors[d].rdev;
if (rdev &&
- test_bit(In_sync, &rdev->flags))
+ !test_bit(Faulty, &rdev->flags))
r1_sync_page_io(rdev, sect, s,
conf->tmppage, WRITE);
}
@@ -2167,7 +2167,7 @@ static void fix_read_error(struct r1conf *conf,
int read_disk,
d--;
rdev = conf->mirrors[d].rdev;
if (rdev &&
- test_bit(In_sync, &rdev->flags)) {
+ !test_bit(Faulty, &rdev->flags)) {
if (r1_sync_page_io(rdev, sect, s,
conf->tmppage, READ)) {
atomic_add(s, &rdev->corrected_errors);
On Mon, Sep 22, 2014 at 2:17 AM, NeilBrown <neilb@suse.de> wrote:
> On Sun, 21 Sep 2014 19:47:14 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> wrote:
>
>> Thanks, Neil,
>>
>> On Thu, Sep 18, 2014 at 4:05 AM, NeilBrown <neilb@suse.de> wrote:
>> > On Wed, 17 Sep 2014 20:57:13 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
>> > wrote:
>> >
>> >> Hi Neil,
>> >>
>> >> On Mon, Sep 8, 2014 at 10:17 AM, NeilBrown <neilb@suse.de> wrote:
>> >> > On Sun, 7 Sep 2014 17:18:16 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
>> >> > wrote:
>> >> >
>> >> >> Hi Neil,
>> >> >> we see the following issue:
>> >> >>
>> >> >> # RAID1 has 2 drives A and B, drive B is recovering
>> >> >> # READ request arrives
>> >> >> # read_balanace selects drive B to read from, because READ sector
>> >> >> comes before B->recovery_offset
>> >> >> # READ is issued to drive B, but fails (drive B fails again)
>> >> >>
>> >> >> Now raid1_end_read_request() has the following code:
>> >> >>
>> >> >> 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);
>> >> >> 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;
>> >> >> spin_unlock_irqrestore(&conf->device_lock, flags);
>> >> >> }
>> >> >>
>> >> >> According to this code uptodate wrongly becomes 1, because:
>> >> >> r1_bio->mddev->degraded == conf->raid_disks-1 is TRUE
>> >> >> and
>> >> >> !test_bit(Faulty, &conf->mirrors[mirror].rdev->flags) is also TRUE
>> >> >>
>> >> >> Indeed, drive B is not marked as Faulty, but also not marked as In_sync.
>> >> >> However, this function treats !Faulty being equal to In_Sync, so it
>> >> >> decides that the last good drive failed, so it does not retry the
>> >> >> READ.
>> >> >>
>> >> >> As a result, there is IO error, while we should have retried the READ
>> >> >> from the healthy drive.
>> >> >>
>> >> >> This is happening in 3.8.13, but your master branch seems to have the
>> >> >> same issue.
>> >> >>
>> >> >> What is a reasonable fix?
>> >> >> 1) Do not read from drives which are !In_sync (a bit scary to read
>> >> >> from such drive)
>> >> >
>> >> > It is perfectly safe to read from a !In_sync device providing you are before
>> >> > ->recovery_offset.
>> >> >
>> >> >
>> >> >> 2) replace !Faulty to In_sync check
>> >> >
>> >> > That probably makes sense... though that could race with raid1_spare_active().
>> >> > If a read-error returned just after raid1_spare_active() set In_sync, and
>> >> > before 'count' was subtracted from ->degraded, we would still set uptodate
>> >> > when we shouldn't.
>> >> > It probably make sense to put all of raid1_spare_active inside the spinlock -
>> >> > it doesn't get call often enough that performance is an issue (I hope).
>> >> >
>> >> > So:
>> >> > 1/ change !Faulty to In_sync
>> >> > 2/ extend the spinlock in raid1_spare_active to cover the whole function.
>> >>
>> >>
>> >> I made these fixes and reproduced the issue. However, the result is
>> >> not what we expect:
>> >>
>> >> # raid1_end_read_request() now indeed adds the r1_bio into retry_list,
>> >> as we wanted
>> >> # raid1d calls fix_read_error()
>> >> # fix_read_error() searches for an In_sync drive to read the data
>> >> from. It finds such drive (this is our good drive A)
>> >> # now fix_read_error() wants to rewrite the bad area. But it rewrites
>> >> only on those drives that are In_sync (except the drive it got the
>> >> data from). In our case, it never tries to rewrite the data on drive B
>> >> (drive B is not marked Faulty and not marked In_sync). As a result,
>> >> md_error() is not called, so drive B is still not marked as Failed
>> >> when fix_read_error() completes
>> >> # so handle_read_error() retries the original READ by calling
>> >> read_balance(), which again in my case selects the recovering drive
>> >> B...
>> >>
>> >> And then the whole flow repeats itself again and again...and READ
>> >> never completes.
>> >>
>> >> Maybe we should not allow selecting recovering drives for READ? Or
>> >> some other approach?
>> >>
>> >
>> > Thanks for the testing and analysis.
>> > Presumably we just want handle_read_error() to write to all non-faulty
>> > devices, not just the InSync ones.
>> > i.e. the following patch.
>> >
>> > Thanks,
>> > NeilBrown
>> >
>> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> > index 6a9c73435eb8..a95f9e179e6f 100644
>> > --- a/drivers/md/raid1.c
>> > +++ b/drivers/md/raid1.c
>> > @@ -2153,7 +2153,7 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
>> > d--;
>> > rdev = conf->mirrors[d].rdev;
>> > if (rdev &&
>> > - test_bit(In_sync, &rdev->flags))
>> > + !test_bit(Faulty, &rdev->flags))
>> > r1_sync_page_io(rdev, sect, s,
>> > conf->tmppage, WRITE);
>> > }
>> > @@ -2165,7 +2165,7 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
>> > d--;
>> > rdev = conf->mirrors[d].rdev;
>> > if (rdev &&
>> > - test_bit(In_sync, &rdev->flags)) {
>> > + !test_bit(Faulty, &rdev->flags)) {
>> > if (r1_sync_page_io(rdev, sect, s,
>> > conf->tmppage, READ)) {
>> > atomic_add(s, &rdev->corrected_errors);
>>
>> With this addition, the problem appears to be fixed. We will give it
>> some regression testing & will let you know if we see any issues.
>>
>> I presume you will be applying this fix upstream as well.
>
> Yes, it is already in my for-next branch.
> I've just added your tested-by.
>
> Thanks,
> NeilBrown
^ permalink raw reply related
* Re: [PATCH] Enable loading of bitmaps for dm-md-raid1.
From: Goldwyn Rodrigues @ 2015-07-22 12:52 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid
In-Reply-To: <20150722104105.5caf47f4@noble>
On 07/21/2015 07:41 PM, NeilBrown wrote:
> On Mon, 20 Jul 2015 12:06:28 -0500 Goldwyn Rodrigues <rgoldwyn@suse.de>
> wrote:
>
>> This is also a hack for systems with junk in the rest
>> of the bitmap super (instead of zeroes) to boot. This is done by
>> checking mddev->sync_super (which is exclusively set by dm-raid)
>> is null.
>>
>> These changes also include zeroing of most bitmap pages while
>> allocating so we are sure that the junk is not coming from memory.
>>
>> References: https://bugzilla.kernel.org/show_bug.cgi?id=100491
>>
>> Signed-off-by: Neil Brown <neilb@suse.de>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>> index 135a090..dfa5ef3 100644
>> --- a/drivers/md/bitmap.c
>> +++ b/drivers/md/bitmap.c
>> @@ -494,7 +494,7 @@ static int bitmap_new_disk_sb(struct bitmap *bitmap)
>> bitmap_super_t *sb;
>> unsigned long chunksize, daemon_sleep, write_behind;
>>
>> - bitmap->storage.sb_page = alloc_page(GFP_KERNEL);
>> + bitmap->storage.sb_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> if (bitmap->storage.sb_page == NULL)
>> return -ENOMEM;
>> bitmap->storage.sb_page->index = 0;
>> @@ -541,6 +541,7 @@ static int bitmap_new_disk_sb(struct bitmap *bitmap)
>> sb->state = cpu_to_le32(bitmap->flags);
>> bitmap->events_cleared = bitmap->mddev->events;
>> sb->events_cleared = cpu_to_le64(bitmap->mddev->events);
>> + bitmap->mddev->bitmap_info.nodes = 0;
>>
>> kunmap_atomic(sb);
>>
>> @@ -568,7 +569,7 @@ static int bitmap_read_sb(struct bitmap *bitmap)
>> goto out_no_sb;
>> }
>> /* page 0 is the superblock, read it... */
>> - sb_page = alloc_page(GFP_KERNEL);
>> + sb_page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> if (!sb_page)
>> return -ENOMEM;
>> bitmap->storage.sb_page = sb_page;
>> @@ -611,8 +612,15 @@ re_read:
>> daemon_sleep = le32_to_cpu(sb->daemon_sleep) * HZ;
>> write_behind = le32_to_cpu(sb->write_behind);
>> sectors_reserved = le32_to_cpu(sb->sectors_reserved);
>> - nodes = le32_to_cpu(sb->nodes);
>> - strlcpy(bitmap->mddev->bitmap_info.cluster_name, sb->cluster_name, 64);
>> + /* XXX: This is an ugly hack to ensure that we don't use clustering
>> + in case dm-raid is in use and the nodes written in bitmap_sb
>> + is erroneous.
>> + */
>> + if (!bitmap->mddev->sync_super) {
>> + nodes = le32_to_cpu(sb->nodes);
>> + strlcpy(bitmap->mddev->bitmap_info.cluster_name,
>> + sb->cluster_name, 64);
>> + }
>>
>> /* verify that the bitmap-specific fields are valid */
>> if (sb->magic != cpu_to_le32(BITMAP_MAGIC))
>> @@ -649,7 +657,7 @@ re_read:
>> goto out;
>> }
>> events = le64_to_cpu(sb->events);
>> - if (!nodes && (events < bitmap->mddev->events)) {
>> + if (err == 0 && !nodes && (events < bitmap->mddev->events)) {
>> printk(KERN_INFO
>> "%s: bitmap file is out of date (%llu < %llu) "
>> "-- forcing full recovery\n",
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 4dbed4a..6bd8bc3 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -7415,7 +7415,7 @@ int md_setup_cluster(struct mddev *mddev, int nodes)
>> err = request_module("md-cluster");
>> if (err) {
>> pr_err("md-cluster module not found.\n");
>> - return err;
>> + return -ENOENT;
>> }
>>
>> spin_lock(&pers_lock);
>
>
> Thanks... but I think this is about 3 patches.
>
> The patch to md.c is because request_module() returns a status
> different from what the documentation says. And
> Fixes: edb39c9deda8 ("Introduce md_cluster_operations to handle cluster functions")
> (though it doesn't need to go to stable.
>
> Adding "err == 0 &&' test is ... why is that? It looks to me like
> err == -EINVAL at that point, always. Can you explain/
Oh no! This is placed at the incorrect location. It should have been
placed before setting up the cluster.
>
> Using __GFP_ZERO in read_sb_page seems wrong and so misleading.
>
> The rest are for the main bug you are trying to fix .. though I think
> it could be described better.
>
> -------------------
> There is a bug that the bitmap superblock isn't initialised properly for
> dm-raid, so a new field can have garbage in new fields.
> (dm-raid does initialisation in the kernel - md initialised the
> superblock in mdadm).
> This means that for dm-raid we cannot currently trust the new ->nodes
> field.
> So:
> - use __GFP_ZERO to initialise the superblock properly for all new
> arrays
> - initialise all field in bitmap_info in bitmap_new_disk_sb
> - ignore ->nodes for dm arrays (yes, this is a hack)
> -----------------
>
> Could you make it 3 patches for me please?
>
Sure, I will post the 3 patches.
--
Goldwyn
^ permalink raw reply
* Re: [PATCH] raid0: data corruption when using trim
From: Martin K. Petersen @ 2015-07-22 11:59 UTC (permalink / raw)
To: Seunguk Shin; +Cc: 'Martin K. Petersen', neilb, linux-raid
In-Reply-To: <005601d0c470$951ea860$bf5bf920$@samsung.com>
>>>>> "Seunguk" == Seunguk Shin <seunguk.shin@samsung.com> writes:
Seunguk,
Seunguk> I have tested the script from Algolia with your patch, I cannot
Seunguk> see the same symptoms I saw with original kernel. I think the
Seunguk> patch works and there is no problem.
Thank you for your diligent work on this issue! I just submitted the
patch.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* [PATCH] block: Do a full clone when splitting discard bios
From: Martin K. Petersen @ 2015-07-22 11:57 UTC (permalink / raw)
To: axboe
Cc: linux-raid, neilb, Martin K. Petersen, Seunguk Shin,
Kent Overstreet, stable
In-Reply-To: <005601d0c470ea860samsung.com>
This fixes a data corruption bug when using discard on top of MD linear,
raid0 and raid10 personalities.
Commit 20d0189b1012 "block: Introduce new bio_split()" permits sharing
the bio_vec between the two resulting bios. That is fine for read/write
requests where the bio_vec is immutable. For discards, however, we need
to be able to attach a payload and update the bio_vec so the page can
get mapped to a scatterlist entry. Therefore the bio_vec can not be
shared when splitting discards and we must do a full clone.
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Reported-by: Seunguk Shin <seunguk.shin@samsung.com>
Tested-by: Seunguk Shin <seunguk.shin@samsung.com>
Cc: Seunguk Shin <seunguk.shin@samsung.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Kent Overstreet <kent.overstreet@gmail.com>
Cc: <stable@vger.kernel.org> # v3.14+
---
block/bio.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 2a00d349cd68..d18de96919f3 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1831,8 +1831,9 @@ EXPORT_SYMBOL(bio_endio);
* Allocates and returns a new bio which represents @sectors from the start of
* @bio, and updates @bio to represent the remaining sectors.
*
- * The newly allocated bio will point to @bio's bi_io_vec; it is the caller's
- * responsibility to ensure that @bio is not freed before the split.
+ * Unless this is a discard request the newly allocated bio will point
+ * to @bio's bi_io_vec; it is the caller's responsibility to ensure that
+ * @bio is not freed before the split.
*/
struct bio *bio_split(struct bio *bio, int sectors,
gfp_t gfp, struct bio_set *bs)
@@ -1842,7 +1843,15 @@ struct bio *bio_split(struct bio *bio, int sectors,
BUG_ON(sectors <= 0);
BUG_ON(sectors >= bio_sectors(bio));
- split = bio_clone_fast(bio, gfp, bs);
+ /*
+ * Discards need a mutable bio_vec to accommodate the payload
+ * required by the DSM TRIM and UNMAP commands.
+ */
+ if (bio->bi_rw & REQ_DISCARD)
+ split = bio_clone_bioset(bio, gfp, bs);
+ else
+ split = bio_clone_fast(bio, gfp, bs);
+
if (!split)
return NULL;
--
2.4.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox