* [Patch v1 2/4] md/raid5: simplify handle_strype_dirtying()
From: stockhausen @ 2015-08-07 20:21 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: Type: text/plain, Size: 2135 bytes --]
md/raid5: simplify handle_strype_dirtying()
Calculating rcw/rmw inside handle_stripe_dirtying() depends on
nearly doubled code. Consolidate parts of it.
Signed-off-by: Markus Stockhausen <stockhausen@collogia.de>
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 817d125..6a7e638 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3548,7 +3548,7 @@ static void handle_stripe_dirtying(struct r5conf *conf,
struct stripe_head_state *s,
int disks)
{
- int rmw = 0, rcw = 0, i;
+ int rmw = 0, rcw = 0, i, cost;
sector_t recovery_cp = conf->mddev->recovery_cp;
/* Check whether resync is now happening or should start.
@@ -3569,27 +3569,17 @@ static void handle_stripe_dirtying(struct r5conf *conf,
conf->rmw_level, (unsigned long long)recovery_cp,
(unsigned long long)sh->sector);
} else for (i = disks; i--; ) {
- /* would I have to read this buffer for read_modify_write */
struct r5dev *dev = &sh->dev[i];
- if ((dev->towrite || i == sh->pd_idx || i == sh->qd_idx) &&
- !test_bit(R5_LOCKED, &dev->flags) &&
+ if (!test_bit(R5_LOCKED, &dev->flags) &&
!(test_bit(R5_UPTODATE, &dev->flags) ||
test_bit(R5_Wantcompute, &dev->flags))) {
- if (test_bit(R5_Insync, &dev->flags))
- rmw++;
- else
- rmw += 2*disks; /* cannot read it */
- }
- /* Would I have to read this buffer for reconstruct_write */
- if (!test_bit(R5_OVERWRITE, &dev->flags) &&
- i != sh->pd_idx && i != sh->qd_idx &&
- !test_bit(R5_LOCKED, &dev->flags) &&
- !(test_bit(R5_UPTODATE, &dev->flags) ||
- test_bit(R5_Wantcompute, &dev->flags))) {
- if (test_bit(R5_Insync, &dev->flags))
- rcw++;
- else
- rcw += 2*disks;
+ /* IO cost for RMW/RCW depends on block readability */
+ cost = (test_bit(R5_Insync, &dev->flags)) ? 1 : 2*MAX_DISKS;
+ if (dev->towrite || i == sh->pd_idx || i == sh->qd_idx)
+ rmw += cost;
+ if (!test_bit(R5_OVERWRITE, &dev->flags) &&
+ i != sh->pd_idx && i != sh->qd_idx)
+ rcw += cost;
}
}
pr_debug("for sector %llu, rmw=%d rcw=%d\n",
[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 1650 bytes --]
****************************************************************************
Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte
Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail
irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte
Weitergabe dieser Mail ist nicht gestattet.
Über das Internet versandte E-Mails können unter fremden Namen erstellt oder
manipuliert werden. Deshalb ist diese als E-Mail verschickte Nachricht keine
rechtsverbindliche Willenserklärung.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
Vorstand:
Kadir Akin
Dr. Michael Höhnerbach
Vorsitzender des Aufsichtsrates:
Hans Kristian Langva
Registergericht: Amtsgericht Köln
Registernummer: HRB 52 497
This e-mail may contain confidential and/or privileged information. If you
are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail. Any
unauthorized copying, disclosure or distribution of the material in this
e-mail is strictly forbidden.
e-mails sent over the internet may have been written under a wrong name or
been manipulated. That is why this message sent as an e-mail is not a
legally binding declaration of intention.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
executive board:
Kadir Akin
Dr. Michael Höhnerbach
President of the supervisory board:
Hans Kristian Langva
Registry office: district court Cologne
Register number: HRB 52 497
****************************************************************************
^ permalink raw reply related
* [Patch v1 1/4] md/raid5: MAX_DISKS define
From: stockhausen @ 2015-08-07 20:21 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]
md/raid5: MAX_DISKS define
The maximum number of disks raid5 can handle is not clearly defined.
To avoid arbitrary values at several places introduce a new constant.
Signed-off-by: Markus Stockhausen <stockhausen@collogia.de>
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 643d217..817d125 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7722,7 +7722,7 @@ static void *raid6_takeover(struct mddev *mddev)
return ERR_PTR(-EINVAL);
if (mddev->degraded > 1)
return ERR_PTR(-EINVAL);
- if (mddev->raid_disks > 253)
+ if (mddev->raid_disks >= MAX_DISKS)
return ERR_PTR(-EINVAL);
if (mddev->raid_disks < 3)
return ERR_PTR(-EINVAL);
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index d051442..112545b 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -542,6 +542,11 @@ struct r5conf {
int worker_cnt_per_group;
};
+/*
+ * Number of disks a raid456 device can support. Regarding physical constraints
+ * this is only a theoretical value. Nevertheless it improves readability
+ */
+#define MAX_DISKS 254
/*
* Our supported algorithms
[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 1650 bytes --]
****************************************************************************
Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte
Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail
irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte
Weitergabe dieser Mail ist nicht gestattet.
Über das Internet versandte E-Mails können unter fremden Namen erstellt oder
manipuliert werden. Deshalb ist diese als E-Mail verschickte Nachricht keine
rechtsverbindliche Willenserklärung.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
Vorstand:
Kadir Akin
Dr. Michael Höhnerbach
Vorsitzender des Aufsichtsrates:
Hans Kristian Langva
Registergericht: Amtsgericht Köln
Registernummer: HRB 52 497
This e-mail may contain confidential and/or privileged information. If you
are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail. Any
unauthorized copying, disclosure or distribution of the material in this
e-mail is strictly forbidden.
e-mails sent over the internet may have been written under a wrong name or
been manipulated. That is why this message sent as an e-mail is not a
legally binding declaration of intention.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
executive board:
Kadir Akin
Dr. Michael Höhnerbach
President of the supervisory board:
Hans Kristian Langva
Registry office: district court Cologne
Register number: HRB 52 497
****************************************************************************
^ permalink raw reply related
* [Patch v1 0/4] md/raid5: Cleanups for handle_stripe_dirtying()
From: stockhausen @ 2015-08-07 20:20 UTC (permalink / raw)
To: linux-raid
[-- Attachment #1: Type: text/plain, Size: 103 bytes --]
md/raid5: Cleanups for handle_stripe_dirtying()
Some cleanups for handle_stripe_dirtying code.
[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 1650 bytes --]
****************************************************************************
Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte
Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail
irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte
Weitergabe dieser Mail ist nicht gestattet.
Über das Internet versandte E-Mails können unter fremden Namen erstellt oder
manipuliert werden. Deshalb ist diese als E-Mail verschickte Nachricht keine
rechtsverbindliche Willenserklärung.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
Vorstand:
Kadir Akin
Dr. Michael Höhnerbach
Vorsitzender des Aufsichtsrates:
Hans Kristian Langva
Registergericht: Amtsgericht Köln
Registernummer: HRB 52 497
This e-mail may contain confidential and/or privileged information. If you
are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail. Any
unauthorized copying, disclosure or distribution of the material in this
e-mail is strictly forbidden.
e-mails sent over the internet may have been written under a wrong name or
been manipulated. That is why this message sent as an e-mail is not a
legally binding declaration of intention.
Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln
executive board:
Kadir Akin
Dr. Michael Höhnerbach
President of the supervisory board:
Hans Kristian Langva
Registry office: district court Cologne
Register number: HRB 52 497
****************************************************************************
^ permalink raw reply
* Re: Re-add not selecting drive for correct slot?
From: Mikael Abrahamsson @ 2015-08-07 12:38 UTC (permalink / raw)
To: Thomas Fjellstrom; +Cc: linux-raid
In-Reply-To: <1817382.9YhZ7ff5Yl@balsa>
On Thu, 6 Aug 2015, Thomas Fjellstrom wrote:
> Also, sdc has a much lower event count than the other drives, is that
> normal?
>
> /dev/sdc is the first drive that failed
> /dev/sdb is the one that was kicked after
Can you please post dmesg output from when you unplugged the drive and
what happened to the array then? I guess you do not have "cat
/proc/mdstat" from then? It's weird that sdb was busy. Please post mdadm
version and kernel version.
The event count is increased when writes occur to the array, so that sdc
would have a much lower event count is normal because it was out of the
array for much longer time than sdb was.
--
Mikael Abrahamsson email: swmike@swm.pp.se
^ permalink raw reply
* Re-add not selecting drive for correct slot?
From: Thomas Fjellstrom @ 2015-08-07 5:09 UTC (permalink / raw)
To: linux-raid
Hi again.
I had a drive disappear off the bus while I was out of town. I just
Pulled it and installed a replacement drive. The problem is that
another drive was kicked when I pulled the failed drive.
Looking at the array info, it should be re-add'able, the event counts
are very close.
Thinking I'm smarter than I really am, I tried a:
$ mdadm --manage /dev/md0 --re-add /dev/sdb
But that returned with device or resource busy, obviously the
array is still holding onto it. Then I:
$ mdadm --remove /dev/md0 /dev/sdb
$ mdadm --manage /dev/md0 --re-add /dev/sdb
But that just added it as a spare, and wiped the slot/role data that
was once in the --examine output.
After that I tried force assembling, but it refused saying it only found
5 active and 1 spare device which is not enough to start the array.
Is there a way to convince mdadm to assemble this into a working
degraded array?
Also, sdc has a much lower event count than the other drives, is that
normal?
/dev/sdc is the first drive that failed
/dev/sdb is the one that was kicked after
mdstat:
Personalities : [raid6] [raid5] [raid4]
md0 : active raid5 sdb[0](F) sdh[7] sdg[5] sdf[4] sde[3] sdd[2] sdc[1](F)
11720297472 blocks super 1.2 level 5, 512k chunk, algorithm 2 [7/5] [__UUUUU]
bitmap: 15/15 pages [60KB], 65536KB chunk
mdadm --examine:
/dev/sdb:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : c8032dba:053904e3:1648d92b:0695b265
Name : mrbig:0 (local to host mrbig)
Creation Time : Sun Jun 14 14:06:22 2015
Raid Level : raid5
Raid Devices : 7
Avail Dev Size : 3906767024 (1862.89 GiB 2000.26 GB)
Array Size : 11720297472 (11177.35 GiB 12001.58 GB)
Used Dev Size : 3906765824 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
State : clean
Device UUID : c1efb1f8:edd4ab44:c8b95abd:6b1cdbf8
Internal Bitmap : 8 sectors from superblock
Update Time : Thu Aug 6 18:28:32 2015
Checksum : b1dea42e - correct
Events : 2053
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 0
Array State : A.AAAAA ('A' == active, '.' == missing)
/dev/sdd:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : c8032dba:053904e3:1648d92b:0695b265
Name : mrbig:0 (local to host mrbig)
Creation Time : Sun Jun 14 14:06:22 2015
Raid Level : raid5
Raid Devices : 7
Avail Dev Size : 3906767024 (1862.89 GiB 2000.26 GB)
Array Size : 11720297472 (11177.35 GiB 12001.58 GB)
Used Dev Size : 3906765824 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
State : clean
Device UUID : 32917d29:916c3a57:41d93b82:6790c6bb
Internal Bitmap : 8 sectors from superblock
Update Time : Thu Aug 6 21:42:20 2015
Checksum : 7d069e27 - correct
Events : 2057
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 2
Array State : ..AAAAA ('A' == active, '.' == missing)
/dev/sde:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : c8032dba:053904e3:1648d92b:0695b265
Name : mrbig:0 (local to host mrbig)
Creation Time : Sun Jun 14 14:06:22 2015
Raid Level : raid5
Raid Devices : 7
Avail Dev Size : 3906767024 (1862.89 GiB 2000.26 GB)
Array Size : 11720297472 (11177.35 GiB 12001.58 GB)
Used Dev Size : 3906765824 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
State : clean
Device UUID : 7b68d82f:68da74b2:01837fec:ab08918e
Internal Bitmap : 8 sectors from superblock
Update Time : Thu Aug 6 21:42:20 2015
Checksum : 1baa054d - correct
Events : 2057
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 3
Array State : ..AAAAA ('A' == active, '.' == missing)
/dev/sdf:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : c8032dba:053904e3:1648d92b:0695b265
Name : mrbig:0 (local to host mrbig)
Creation Time : Sun Jun 14 14:06:22 2015
Raid Level : raid5
Raid Devices : 7
Avail Dev Size : 3906767024 (1862.89 GiB 2000.26 GB)
Array Size : 11720297472 (11177.35 GiB 12001.58 GB)
Used Dev Size : 3906765824 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
State : clean
Device UUID : 27de04b5:e5eabe72:e7f1ada8:36402334
Internal Bitmap : 8 sectors from superblock
Update Time : Thu Aug 6 21:42:20 2015
Checksum : c2e131e7 - correct
Events : 2057
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 4
Array State : ..AAAAA ('A' == active, '.' == missing)
/dev/sdg:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : c8032dba:053904e3:1648d92b:0695b265
Name : mrbig:0 (local to host mrbig)
Creation Time : Sun Jun 14 14:06:22 2015
Raid Level : raid5
Raid Devices : 7
Avail Dev Size : 3906767024 (1862.89 GiB 2000.26 GB)
Array Size : 11720297472 (11177.35 GiB 12001.58 GB)
Used Dev Size : 3906765824 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
State : clean
Device UUID : 001e01c6:7482a410:83a61537:05eb9308
Internal Bitmap : 8 sectors from superblock
Update Time : Thu Aug 6 21:42:20 2015
Checksum : d49b68ba - correct
Events : 2057
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 5
Array State : ..AAAAA ('A' == active, '.' == missing)
/dev/sdh:
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : c8032dba:053904e3:1648d92b:0695b265
Name : mrbig:0 (local to host mrbig)
Creation Time : Sun Jun 14 14:06:22 2015
Raid Level : raid5
Raid Devices : 7
Avail Dev Size : 3906767024 (1862.89 GiB 2000.26 GB)
Array Size : 11720297472 (11177.35 GiB 12001.58 GB)
Used Dev Size : 3906765824 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
State : clean
Device UUID : e1553922:dbf9da9f:060762fa:c6786762
Internal Bitmap : 8 sectors from superblock
Update Time : Thu Aug 6 21:42:20 2015
Checksum : dd2a0649 - correct
Events : 2057
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 6
Array State : ..AAAAA ('A' == active, '.' == missing)
/dev/sdj: (was /dev/sdc, its installed in a different machine now)
Magic : a92b4efc
Version : 1.2
Feature Map : 0x1
Array UUID : c8032dba:053904e3:1648d92b:0695b265
Name : mrbig:0
Creation Time : Sun Jun 14 14:06:22 2015
Raid Level : raid5
Raid Devices : 7
Avail Dev Size : 3906767024 (1862.89 GiB 2000.26 GB)
Array Size : 11720297472 (11177.35 GiB 12001.58 GB)
Used Dev Size : 3906765824 (1862.89 GiB 2000.26 GB)
Data Offset : 262144 sectors
Super Offset : 8 sectors
Unused Space : before=262064 sectors, after=1200 sectors
State : clean
Device UUID : 9f42a955:9a978bd1:baf8290b:d397e4f2
Internal Bitmap : 8 sectors from superblock
Update Time : Fri Jul 31 16:27:56 2015
Checksum : e389672e - correct
Events : 101
Layout : left-symmetric
Chunk Size : 512K
Device Role : Active device 1
Array State : AAAAAAA ('A' == active, '.' == missing, 'R' == replacing)
Distro: Debian Sid
Kernel: 3.16.7-ckt4-3~bpo70+1
Mdadm: v3.2.5 (from debian)
--
Thomas Fjellstrom
thomas@fjellstrom.ca
^ permalink raw reply
* Re: [dm-devel] kernel BUG at drivers/scsi/scsi_lib.c:1101! observed during md5sum for one file on (RAID4->RAID0) device
From: NeilBrown @ 2015-08-06 23:15 UTC (permalink / raw)
To: Yi Zhang
Cc: James Bottomley, linux-raid, Jes Sorensen, xni, dm-devel,
linux-scsi
In-Reply-To: <2095050658.4693864.1438843965011.JavaMail.zimbra@redhat.com>
On Thu, 6 Aug 2015 02:52:45 -0400 (EDT) Yi Zhang <yizhan@redhat.com>
wrote:
> Hi Neil
> I test 10 times with below patch on Linux 4.2-rc5, didn't reproduce the issue, thanks.
>
>
Thanks for the confirmation.
I will be submitting it for 4.3 and then it will flow into -stable
kernels.
NeilBrown
^ permalink raw reply
* Re: [dm-devel] kernel BUG at drivers/scsi/scsi_lib.c:1101! observed during md5sum for one file on (RAID4->RAID0) device
From: Yi Zhang @ 2015-08-06 6:52 UTC (permalink / raw)
To: NeilBrown
Cc: James Bottomley, linux-raid, Jes Sorensen, xni, dm-devel,
linux-scsi
In-Reply-To: <20150731112058.6e97b491@noble>
Hi Neil
I test 10 times with below patch on Linux 4.2-rc5, didn't reproduce the issue, thanks.
----- Original Message -----
From: "NeilBrown" <neilb@suse.com>
To: "yizhan" <yizhan@redhat.com>
Sent: Thursday, August 6, 2015 1:21:29 PM
Subject: Re: [dm-devel] kernel BUG at drivers/scsi/scsi_lib.c:1101! observed during md5sum for one file on (RAID4->RAID0) device
On Wed, 05 Aug 2015 22:11:07 +0800 yizhan <yizhan@redhat.com> wrote:
> Hi Neil
> Could you send me one patch for this issue, I cannot apply below code,
> thanks.
Sorry - didn't notice that had wrapped.
Try this:
http://git.neil.brown.name/?p=md.git;a=commitdiff;h=927d881980b74fa653e3992fd4a7283b0e11952b
or for that raw patch
http://git.neil.brown.name/?p=md.git;a=patch;h=927d881980b74fa653e3992fd4a7283b0e11952b
NeilBrown
Best Regards,
Yi Zhang
----- Original Message -----
From: "NeilBrown" <neilb@suse.com>
To: "James Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: "Yi Zhang" <yizhan@redhat.com>, linux-raid@vger.kernel.org, "Jes Sorensen" <Jes.Sorensen@redhat.com>, xni@redhat.com, dm-devel@redhat.com, linux-scsi@vger.kernel.org
Sent: Friday, July 31, 2015 9:20:58 AM
Subject: Re: [dm-devel] kernel BUG at drivers/scsi/scsi_lib.c:1101! observed during md5sum for one file on (RAID4->RAID0) device
On Thu, 30 Jul 2015 06:28:06 -0700 James Bottomley
<James.Bottomley@HansenPartnership.com> wrote:
> On Thu, 2015-07-30 at 05:03 -0400, Yi Zhang wrote:
> > Hi SCSI/RAID maintainer
> >
> > During raid test with 4.2.0-rc3, I observed below kernel BUG, pls check below info for the test log/environment/test steps.
> >
> > Log:
> > [ 306.741662] md: bind<sdb1>
> > [ 306.750865] md: bind<sdc1>
> > [ 306.753993] md: bind<sdd1>
> > [ 306.764475] md: bind<sde1>
> > [ 306.786156] md: bind<sdf1>
> > [ 306.789362] md: bind<sdh1>
> > [ 306.792555] md: bind<sdg1>
> > [ 306.868166] raid6: sse2x1 gen() 10589 MB/s
> > [ 306.889143] raid6: sse2x1 xor() 8218 MB/s
> > [ 306.910121] raid6: sse2x2 gen() 13453 MB/s
> > [ 306.931102] raid6: sse2x2 xor() 8990 MB/s
> > [ 306.952079] raid6: sse2x4 gen() 15539 MB/s
> > [ 306.973063] raid6: sse2x4 xor() 10771 MB/s
> > [ 306.994039] raid6: avx2x1 gen() 20582 MB/s
> > [ 307.015017] raid6: avx2x2 gen() 24019 MB/s
> > [ 307.035998] raid6: avx2x4 gen() 27824 MB/s
> > [ 307.040755] raid6: using algorithm avx2x4 gen() 27824 MB/s
> > [ 307.046869] raid6: using avx2x2 recovery algorithm
> > [ 307.058793] async_tx: api initialized (async)
> > [ 307.075428] xor: automatically using best checksumming function:
> > [ 307.091942] avx : 32008.000 MB/sec
> > [ 307.147662] md: raid6 personality registered for level 6
> > [ 307.153584] md: raid5 personality registered for level 5
> > [ 307.159505] md: raid4 personality registered for level 4
> > [ 307.165698] md/raid:md0: device sdf1 operational as raid disk 4
> > [ 307.172300] md/raid:md0: device sde1 operational as raid disk 3
> > [ 307.178899] md/raid:md0: device sdd1 operational as raid disk 2
> > [ 307.185497] md/raid:md0: device sdc1 operational as raid disk 1
> > [ 307.192093] md/raid:md0: device sdb1 operational as raid disk 0
> > [ 307.199052] md/raid:md0: allocated 6482kB
> > [ 307.203573] md/raid:md0: raid level 4 active with 5 out of 6 devices, algorithm 0
> > [ 307.211958] md0: detected capacity change from 0 to 53645148160
> > [ 307.218658] md: recovery of RAID array md0
> > [ 307.223226] md: minimum _guaranteed_ speed: 1000 KB/sec/disk.
> > [ 307.229729] md: using maximum available idle IO bandwidth (but not more than 200000 KB/sec) for recovery.
> > [ 307.240427] md: using 128k window, over a total of 10477568k.
> > [ 374.670951] md: md0: recovery done.
> > [ 375.722806] EXT4-fs (md0): mounted filesystem with ordered data mode. Opts: (null)
> > [ 447.553364] md: unbind<sdh1>
> > [ 447.559905] md: export_rdev(sdh1)
> > [ 447.572684] md: cannot remove active disk sdg1 from md0 ...
> > [ 447.578909] md/raid:md0: Disk failure on sdg1, disabling device.
> > [ 447.578909] md/raid:md0: Operation continuing on 5 devices.
> > [ 447.594850] md: unbind<sdg1>
> > [ 447.601834] md: export_rdev(sdg1)
> > [ 447.615446] md: raid0 personality registered for level 0
> > [ 447.629275] md/raid0:md0: md_size is 104775680 sectors.
> > [ 447.635094] md: RAID0 configuration for md0 - 1 zone
> > [ 447.640627] md: zone0=[sdb1/sdc1/sdd1/sde1/sdf1]
> > [ 447.645833] zone-offset= 0KB, device-offset= 0KB, size= 52387840KB
> > [ 447.654949]
> > [ 447.739443] EXT4-fs (md0): mounted filesystem with ordered data mode. Opts: (null)
> > [ 447.749258] bio too big device sde1 (768 > 512)
>
> This is the actual error. It looks like an md problem (md list copied).
Thanks. It certainly does look like an md problem.... ah, found it.
level_store in drivers/md/md.c calls blk_set_stacking_limits after
calling ->takeover and before calling ->run.
->run should impose the limits from the underlying device, but for
RAID0, ->takeover is doing that.
I can fix that... hopefully it will become irrelevant soon when the
immutable-bio patches go in.
This patch isn't quite right, but it should be pretty close.
Can you test and confirm?
Thanks,
NeilBrown
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index efb654eb5399..17804f374709 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -83,7 +83,6 @@ static int create_strip_zones(struct mddev *mddev,
struct r0conf **private_conf) char b[BDEVNAME_SIZE];
char b2[BDEVNAME_SIZE];
struct r0conf *conf = kzalloc(sizeof(*conf), GFP_KERNEL);
- bool discard_supported = false;
if (!conf)
return -ENOMEM;
@@ -188,19 +187,12 @@ static int create_strip_zones(struct mddev
*mddev, struct r0conf **private_conf) }
dev[j] = rdev1;
- if (mddev->queue)
- disk_stack_limits(mddev->gendisk, rdev1->bdev,
- rdev1->data_offset << 9);
-
if (rdev1->bdev->bd_disk->queue->merge_bvec_fn)
conf->has_merge_bvec = 1;
if (!smallest || (rdev1->sectors < smallest->sectors))
smallest = rdev1;
cnt++;
-
- if (blk_queue_discard(bdev_get_queue(rdev1->bdev)))
- discard_supported = true;
}
if (cnt != mddev->raid_disks) {
printk(KERN_ERR "md/raid0:%s: too few disks (%d of %d)
- " @@ -272,17 +264,6 @@ static int create_strip_zones(struct mddev
*mddev, struct r0conf **private_conf) goto abort;
}
- if (mddev->queue) {
- blk_queue_io_min(mddev->queue, mddev->chunk_sectors <<
9);
- blk_queue_io_opt(mddev->queue,
- (mddev->chunk_sectors << 9) *
mddev->raid_disks); -
- if (!discard_supported)
- queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD,
mddev->queue);
- else
- queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
mddev->queue);
- }
-
pr_debug("md/raid0:%s: done.\n", mdname(mddev));
*private_conf = conf;
@@ -433,12 +414,6 @@ static int raid0_run(struct mddev *mddev)
if (md_check_no_bitmap(mddev))
return -EINVAL;
- if (mddev->queue) {
- blk_queue_max_hw_sectors(mddev->queue,
mddev->chunk_sectors);
- blk_queue_max_write_same_sectors(mddev->queue,
mddev->chunk_sectors);
- blk_queue_max_discard_sectors(mddev->queue,
mddev->chunk_sectors);
- }
-
/* if private is not null, we are here after takeover */
if (mddev->private == NULL) {
ret = create_strip_zones(mddev, &conf);
@@ -447,6 +422,29 @@ static int raid0_run(struct mddev *mddev)
mddev->private = conf;
}
conf = mddev->private;
+ if (mddev->queue) {
+ struct md_rdev *rdev;
+ bool discard_supported = false;
+
+ rdev_for_each(rdev, mddev) {
+ disk_stack_limits(mddev->gendisk, rdev->bdev,
+ rdev->data_offset << 9);
+ if
(blk_queue_discard(bdev_get_queue(rdev->bdev)))
+ discard_supported = true;
+ }
+ blk_queue_max_hw_sectors(mddev->queue,
mddev->chunk_sectors);
+ blk_queue_max_write_same_sectors(mddev->queue,
mddev->chunk_sectors);
+ blk_queue_max_discard_sectors(mddev->queue,
mddev->chunk_sectors); +
+ blk_queue_io_min(mddev->queue, mddev->chunk_sectors <<
9);
+ blk_queue_io_opt(mddev->queue,
+ (mddev->chunk_sectors << 9) *
mddev->raid_disks); +
+ if (!discard_supported)
+ queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD,
mddev->queue);
+ else
+ queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
mddev->queue);
+ }
/* calculate array device size */
md_set_array_sectors(mddev, raid0_size(mddev, 0, 0));
^ permalink raw reply related
* Re: raid1 (re)-add recovery data corruption
From: Yi Zhang @ 2015-08-06 2:48 UTC (permalink / raw)
To: neilb; +Cc: Xiao Ni, jes sorensen, linux-raid
In-Reply-To: <705099586.901961.1438256110567.JavaMail.zimbra@redhat.com>
Hi Neil
Could you help check this issue, thanks.
Best Regards,
Yi Zhang
----- Original Message -----
From: "Yi Zhang" <yizhan@redhat.com>
To: linux-raid@vger.kernel.org
Cc: "Xiao Ni" <xni@redhat.com>, "jes sorensen" <jes.sorensen@redhat.com>, "Yi Zhang" <yizhan@redhat.com>
Sent: Thursday, July 30, 2015 7:35:10 PM
Subject: raid1 (re)-add recovery data corruption
Hi Neil
I observed raid1 data corruption on raid1 test, below is the test env/reproduce steps/log, pls check it.
Kernel-verison: 4.2.0-rc3
Test-steps:
1. First create one 2GB file bigfile
2. Execute below script
#!/bin/bash
Create_Loop()
{
for i in `seq 0 7`;do
dd if=/dev/zero of=/tmp/$i.tmp bs=1M count=3000 &
done
wait
for i in `seq 0 7`;do
losetup /dev/loop$i /tmp/$i.tmp
done
}
Prepare()
{
mdadm --create --run /dev/md0 --level 1 --metadata 1.2 --raid-devices 8 /dev/loop[0-7] --chunk 512 --bitmap=internal --bitmap-chunk=64M
mdadm --wait /dev/md0
mkfs.ext4 /dev/md0
mkdir /mnt/fortest
mount /dev/md0 /mnt/fortest
md5sum bigfile >md5sum1
}
Create_Loop
Prepare
cnt=0
while [ 1 ]; do
echo "-----------------------------------------------------$cnt"
cp bigfile /mnt/fortest &
sleep 10
mdadm /dev/md0 -f /dev/loop0
sleep 5
mdadm /dev/md0 -r /dev/loop0
while [ 1 ]; do
if [ $? -ne 0 ];then
sleep 5
mdadm /dev/md0 -r /dev/loop0
else
break
fi
done
sleep 30
mdadm /dev/md0 -a /dev/loop0
wait
echo "cp done"
mdadm --wait /dev/md0
echo "recovery done"
md5sum /mnt/fortest/bigfile > md5sum2
tmp1=`awk '{print $1}' ./md5sum1`
tmp2=`awk '{print $1}' ./md5sum2`
echo $tmp1 > a
echo $tmp2 > b
diff a b //data corruption observed
if [ $? -ne 0 ]; then
echo "There are some date corruption, cnt is $cnt"
exit 1
fi
((cnt++))
rm -rf /mnt/fortest/bigfile
done
Kernel-Log:
[ 1113.577378] loop: module loaded
[ 1290.190065] md: bind<loop0>
[ 1290.193214] md: bind<loop1>
[ 1290.196387] md: bind<loop2>
[ 1290.199542] md: bind<loop3>
[ 1290.202704] md: bind<loop4>
[ 1290.205854] md: bind<loop5>
[ 1290.209003] md: bind<loop6>
[ 1290.212170] md: bind<loop7>
[ 1290.229799] md: raid1 personality registered for level 1
[ 1290.235946] md/raid1:md0: not clean -- starting background reconstruction
[ 1290.243515] md/raid1:md0: active with 8 out of 8 mirrors
[ 1290.249449] created bitmap (1 pages) for device md0
[ 1290.254927] md0: bitmap initialized from disk: read 1 pages, set 47 of 47 bits
[ 1290.328736] md0: detected capacity change from 0 to 3143630848
[ 1290.335316] md: resync of RAID array md0
[ 1290.339689] md: minimum _guaranteed_ speed: 1000 KB/sec/disk.
[ 1290.346192] md: using maximum available idle IO bandwidth (but not more than 200000 KB/sec) for resync.
[ 1290.356702] md: using 128k window, over a total of 3069952k.
[ 1640.101181] md: md0: resync done.
[ 1668.352287] EXT4-fs (md0): mounted filesystem with ordered data mode. Opts: (null)
[ 1681.845966] md/raid1:md0: Disk failure on loop0, disabling device.
[ 1681.845966] md/raid1:md0: Operation continuing on 7 devices.
[ 1844.296614] md: unbind<loop0>
[ 1844.302013] md: export_rdev(loop0)
[ 1874.363488] md: bind<loop0>
[ 1874.566435] md: recovery of RAID array md0
[ 1874.571006] md: minimum _guaranteed_ speed: 1000 KB/sec/disk.
[ 1874.577514] md: using maximum available idle IO bandwidth (but not more than 200000 KB/sec) for recovery.
[ 1874.588224] md: using 128k window, over a total of 3069952k.
[ 1889.487210] md: md0: recovery done.
Test Log:
-----------------------------------------------------0
mdadm: set /dev/loop0 faulty in /dev/md0
mdadm: hot removed /dev/loop0 from /dev/md0
mdadm: re-added /dev/loop0
cp done
recovery done
-----------------------------------------------------1
mdadm: set /dev/loop0 faulty in /dev/md0
mdadm: hot removed /dev/loop0 from /dev/md0
mdadm: re-added /dev/loop0
cp done
recovery done
1c1
< c4eddcf325ba5741d37f164750412619
---
> 4444f8bbfb1d22f1731fb5b0c846ef8a
There are some date corruption, cnt is 1
Best Regards,
Yi Zhang
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/9] raid5: don't allow resize/reshape with cache(log) support
From: Shaohua Li @ 2015-08-05 21:42 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20150805141351.126bb888@noble>
On Wed, Aug 05, 2015 at 02:13:51PM +1000, NeilBrown wrote:
> On Wed, 29 Jul 2015 17:38:47 -0700 Shaohua Li <shli@fb.com> wrote:
>
> > If cache(log) support is enabled, don't allow resize/reshape in current
> > stage. In the future, we can flush all data from cache(log) to raid
> > before resize/reshape and then allow resize/reshape.
>
> Just to be on the safe side, you could probably add code to refuse to
> start an array that is in the middle of a reshape and also have a log
> configured.
ok
> I think it makes sense to plan ahead a little and make sure we can
> handle a cache on a reshaping array properly.
>
> If the log metadata block includes a before/after flag for each stripe,
> which recorded whether the stripe was "before" or "after"
> reshape_position when it was written, then when recovering the log we
> can check if the given addresses are still on that side. If they are,
> just recover using the appropriate geometry info from the superblock.
> If not, then reshape has passed over that stripe and it must now be
> fully up-to-date on the RAID so the data in the log can be discarded.
>
> There may be some details I missed, but I think it is worth thinking
> through properly. I don't expect the code to handle this straight
> away, but we need a clear plan to be sure there is sufficient
> information stored in the log.
Just adding a flag is enough? Sounds there is no way to avoid the write
hole issue if the array is reshapping. The data stored in log is valid,
but if a reshape runs, we can't guarantee the parity is valid.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 5/9] raid5: log recovery
From: Shaohua Li @ 2015-08-05 21:39 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20150805140525.0e0774ae@noble>
On Wed, Aug 05, 2015 at 02:05:25PM +1000, NeilBrown wrote:
> On Wed, 29 Jul 2015 17:38:45 -0700 Shaohua Li <shli@fb.com> wrote:
>
> > This is the log recovery support. The process is quite straightforward.
> > We scan the log and read all valid meta/data/parity into memory. If a
> > stripe's data/parity checksum is correct, the stripe will be recoveried.
> > Otherwise, it's discarded and we don't scan the log further. The reclaim
> > process guarantees stripe which starts to be flushed raid disks has
> > completed data/parity and has correct checksum. To recovery a stripe, we
> > just copy its data/parity to corresponding raid disks.
> >
> > The trick thing is superblock update after recovery. we can't let
> > superblock point to last valid meta block. The log might look like:
> > | meta 1| meta 2| meta 3|
> > meta 1 is valid, meta 2 is invalid. meta 3 could be valid. If superblock
> > points to meta 1, we write a new valid meta 2n. If crash happens again,
> > new recovery will start from meta 1. Since meta 2n is valid, recovery
> > will think meta 3 is valid, which is wrong. The solution is we create a
> > new meta in meta2 with its seq == meta 1's seq + 2 and let superblock
> > points to meta2. recovery will not think meta 3 is a valid meta,
> > because its seq is wrong
>
> I like the idea of using a slightly larger 'seq' to avoid collisions -
> except that I would probably feel safer with a much larger seq. May add
> 1024 or something (at least 10).
ok
> >
> > TODO:
> > -recovery should run the stripe cache state machine in case of disk
> > breakage.
>
> Why?
>
> when you write to the log, you write all of the blocks that need
> updating, whether they are destined for a failed device or not.
>
> When you recover, you then have all the blocks that you might want to
> write. So write all the ones for which you have working devices, and
> ignore the rest.
>
> Did I miss something?
>
> Not that I object, but if it works....
I mean the case of disk is broken. For example, log has a stripe with
data for disk 1, 2, 4. In recovery, disk 2 is broken. Just write 1, 4
isn't good. If we run the state machine, we can read disk 3 and have an
eventually consistent stripe.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 4/9] raid5: log reclaim support
From: Shaohua Li @ 2015-08-05 21:34 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20150805134330.3f4269e4@noble>
On Wed, Aug 05, 2015 at 01:43:30PM +1000, NeilBrown wrote:
> On Wed, 29 Jul 2015 17:38:44 -0700 Shaohua Li <shli@fb.com> wrote:
>
> > This is the reclaim support for raid5 log. A stripe write will have
> > following steps:
> >
> > 1. reconstruct the stripe, read data/calculate parity. ops_run_io
> > prepares to write data/parity to raid disks
> > 2. hijack ops_run_io. stripe data/parity is appending to log disk
> > 3. flush log disk cache
> > 4. ops_run_io run again and do normal operation. stripe data/parity is
> > written in raid array disks. raid core can return io to upper layer.
> > 5. flush cache of all raid array disks
> > 6. update super block
> > 7. log disk space used by the stripe can be reused
> >
> > In practice, several stripes consist of an io_unit and we will batch
> > several io_unit in different steps, but the whole process doesn't
> > change.
> >
> > It's possible io return just after data/parity hit log disk, but then
> > read IO will need read from log disk. For simplicity, IO return happens
> > at step 4, where read IO can directly read from raid disks.
> >
> > Currently reclaim run every minute or out of space. Reclaim is just to
> > free log disk spaces, it doesn't impact data consistency.
>
> Having arbitrary times lines "every minute" is a warning sign.
> "As soon as possible" and "Just it time" can both make sense easily.
> "every minute" needs more justification.
>
> I'll probably say more when I find the code.
The idea is if we reclaim periodically, recovery could scan less log
space. It's insane recovery scans a 1T disk. As I said this is just to
free disk spaces. It's not a signal we will lose data in minute
interval. I can change the relaim to run every 1G reclaimable space for
example.
> > + r5l_move_io_unit_list(&log->running_ios, &log->io_end_ios,
> > + IO_UNIT_IO_END);
> > + r5l_move_io_unit_list(&log->io_end_ios, &log->stripe_end_ios,
> > + IO_UNIT_STRIPE_END);
> > + r5l_compress_stripe_end_list(log);
> > + run_stripe = !list_empty(&log->io_end_ios);
> > + spin_unlock(&log->io_list_lock);
> > +
> > + if (!run_stripe)
> > + return;
> > +
> > + blkdev_issue_flush(r5l_bdev(log), GFP_NOIO, NULL);
> > +
> > + spin_lock(&log->io_list_lock);
> > + list_for_each_entry(io, &log->io_end_ios, log_sibling) {
> > + if (io->state >= IO_UNIT_STRIPE_START)
> > + continue;
> > + r5l_set_io_unit_state(io, IO_UNIT_STRIPE_START);
> > +
> > + while (!list_empty(&io->stripe_list)) {
> > + sh = list_first_entry(&io->stripe_list,
> > + struct stripe_head, log_list);
> > + list_del_init(&sh->log_list);
> > + set_bit(STRIPE_HANDLE, &sh->state);
> > + release_stripe(sh);
>
> This code makes me a bit nervous. handle_stripe() can potentially be
> called on any stripe at any time.
> Here you are scheduling a call the handle_stripe() without obviously
> changing the state of the stripe. So whatever is going to happen now
> could potentially have happened before... is that safe?
I'm not fully sure, but it's ok in my test.
> > + * move proper io_unit to reclaim list. We should not change the order.
> > + * reclaimable/unreclaimable io_unit can be mixed in the list, we
> > + * shouldn't reuse space of an unreclaimable io_unit
> > + * */
> > + while (1) {
> > + r5l_move_io_unit_list(&log->running_ios, &log->io_end_ios,
> > + IO_UNIT_IO_END);
> > + r5l_move_io_unit_list(&log->io_end_ios, &log->stripe_end_ios,
> > + IO_UNIT_STRIPE_END);
> > + while (!list_empty(&log->stripe_end_ios)) {
> > + io = list_first_entry(&log->stripe_end_ios,
> > + struct r5l_io_unit, log_sibling);
> > + list_move_tail(&io->log_sibling, &list);
> > + free += (io->log_end - io->log_start +
> > + log->total_blocks) % log->total_blocks;
> > + }
> > +
> > + if (free >= reclaim_target || (list_empty(&log->running_ios) &&
> > + list_empty(&log->io_end_ios) &&
> > + list_empty(&log->stripe_end_ios)))
> > + break;
> > +
> > + if (!list_empty(&log->io_end_ios)) {
> > + io = list_first_entry(&log->io_end_ios,
> > + struct r5l_io_unit, log_sibling);
> > + spin_unlock(&log->io_list_lock);
> > + /* nobody else can delete the io, we are safe */
> > + r5l_kick_io_unit(log, io);
> > + spin_lock(&log->io_list_lock);
> > + continue;
> > + }
> > +
> > + if (!list_empty(&log->running_ios)) {
> > + io = list_first_entry(&log->running_ios,
> > + struct r5l_io_unit, log_sibling);
> > + spin_unlock(&log->io_list_lock);
> > + /* nobody else can delete the io, we are safe */
> > + r5l_kick_io_unit(log, io);
> > + spin_lock(&log->io_list_lock);
> > + continue;
> > + }
> > + }
> > + spin_unlock(&log->io_list_lock);
>
> Well, here we are with the important parts of the reclaim code...
>
> The main result of the above section is to possibly call
> r5l_flush_stripe_to_raid() a few times, and to wait until 'list'
> contains enough io_units to satisfy the requirement.
>
> As raid5d already calls r5l_flush_stripe_to_raid - which it really must
> to make sure that writes complete quickly - this really comes down to
> some book keeping and some waiting.
> Book keeping can be done as changes happen, and waiting is best not
> done at all.
>
> To be more specific: when an io_unit transitions to IO_UNIT_STRIPE_END
> it can immediately be removed from the list and if it was the first
> io_unit on the list, then the log_start can immediately be updated.
Ok, the original idea is to avoid holding the io_list_lock for every IO
end/stripe end. Maybe over-designed, I'll fix this.
> > +
> > + if (list_empty(&list))
> > + return;
> > +
> > + r5l_flush_all_disks(log);
> > +
> > + /* super always point to last valid meta */
> > + last = list_last_entry(&list, struct r5l_io_unit, log_sibling);
> > + r5l_write_super(log, r5l_block_to_sector(log, last->log_start));
>
> This bit flushes all the disks and then updates the metadata and writes
> it.
> As md_super_write already uses WRITE_FLUSH_FUA I don't think the extra
> flush is needed.
great.
> I really think you should just update ->recovery_offset and set
> MD_CHANGE_DEVS (or similar) and let the update happen.
I think we should write super here. The reclaimed space might be reused
immediately, we don't want to confuse recovery.
> > +
> > + mutex_lock(&log->io_mutex);
> > + log->last_checkpoint = last->log_start;
> > + log->last_cp_seq = last->seq;
> > + mutex_unlock(&log->io_mutex);
> > + wake_up(&log->space_waitq);
> > +
> > + while (!list_empty(&list)) {
> > + io = list_first_entry(&list, struct r5l_io_unit, log_sibling);
> > + list_del(&io->log_sibling);
> > + r5l_free_io_unit(log, io);
> > + }
> > +}
>
> So I really think all of this can be done as-it-happens (the
> book-keeping) or asynchronously. There is no need to push something
> every minute.
Because we need flush raid disks cache, to avoid the overhead, we do
batch operation. Once I changed the reclaim to run every specific
reclaimable space, this should be ok.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH 3/9] raid5: add basic stripe log
From: Shaohua Li @ 2015-08-05 21:19 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20150805130736.7ba8a6d5@noble>
On Wed, Aug 05, 2015 at 01:07:36PM +1000, NeilBrown wrote:
> On Wed, 29 Jul 2015 17:38:43 -0700 Shaohua Li <shli@fb.com> wrote:
>
> > This introduces a simple log for raid5. Data/parity writting to raid
> > array first writes to the log, then write to raid array disks. If crash
> > happens, we can recovery data from the log. This can speed up raid
> > resync and fix write hole issue.
> >
> > The log structure is pretty simple. Data/meta data is stored in block
> > unit, which is 4k generally. It has only one type of meta data block.
> > The meta data block can track 3 types of data, stripe data, stripe
> > parity and flush block. MD superblock will point to the last valid meta
> > data block. Each meta data block has checksum/seq number, so recovery
> > can scan the log correctly. We store a checksum of stripe data/parity to
> > the metadata block, so meta data and stripe data/parity can be written
> > to log disk together. otherwise, meta data write must wait till stripe
> > data/parity is finished.
> >
> > For stripe data, meta data block will record stripe data sector and
> > size. Currently the size is always 4k. This meta data record can be made
> > simpler if we just fix write hole (eg, we can record data of a stripe's
> > different disks together), but this format can be extended to support
> > caching in the future, which must record data address/size.
> >
> > For stripe parity, meta data block will record stripe sector. It's size
> > should be 4k (for raid5) or 8k (for raid6). We always store p parity
> > first. This format should work for caching too.
>
> It feels a bit odd have a 8K parity block for RAID6 as it is really two
> blocks: a parity block and a Q-syndrome block. What would you think of
> introducing another block type for Q? So there are Data blocks, Parity
> blocks, and Q blocks ???
>
> Not a big issue, but I thought I would mention it.
I'd prefer not adding the complexity, it's just a naming.
> >
> > flush block indicates a stripe is in raid array disks. Fixing write hole
> > doesn't need this type of meta data, it's for caching extention.
> >
> > We should be careful about deadlock. Appending stripe data/parity to log
> > is done in raid5d. The append need log space, which can trigger log
> > reclaim, which might wait for raid5d to run (to flush data from log to
> > raid array disks). Here we always do the log write in a separate thread.
>
> I'm not convinced about the need for a separate thread. As
> raid5d/handle_stripe works as a state machine, and as all IO is async,
> we should at most need an extra state, not an extra thread.
>
> I think a key issue is that you don't call r5l_get_reserve() until you
> are about to submit writes to the log.
> If instead you reserved the space in r5l_write_stripe and delay the
> stripe if space is not available, there there would be no deadlock.
> Then when available space crosses some threshold, re-activate those
> delayed stripes.
ok, that way works too. As I said before, I really hate making the
log/cache stuff tie tightly together with stripe cache state machine if
possible (eg, adding new state/list/stripe analysis etc). I'd rather to
keep current logic if you don't strongly object.
> > +#include <linux/kernel.h>
> > +#include <linux/wait.h>
> > +#include <linux/blkdev.h>
> > +#include <linux/slab.h>
> > +#include <linux/raid/md_p.h>
> > +#include <linux/crc32.h>
> > +#include <linux/random.h>
> > +#include "md.h"
> > +#include "raid5.h"
> > +
> > +typedef u64 r5blk_t; /* log blocks, 512B - 4k */
>
> I would much rather use sector_t throughout and keep all addresses as
> sector addresses. Much less room for confusion that way.
>
> If we wanted to pack lots of addresses into limited space then using
> block addresses might be justified (filesystems do that), but I don't
> think it is called for here at all.
The original idea is metadata block size could be changed between 512B
to 4k. Sometimes the metadata block can't have enough data if we don't
want to delay IO. When the block size isn't a sector, the type helps me
avoid coding error (eg, remind it's a block instead of a sector). Do you
prefer metadata block size one sector?
> > +static void r5l_put_reserve(struct r5l_log *log, unsigned int reserved_blocks)
> > +{
> > + BUG_ON(!mutex_is_locked(&log->io_mutex));
> > +
> > + log->reserved_blocks -= reserved_blocks;
> > + if (r5l_free_space(log) > 0)
> > + wake_up(&log->space_waitq);
> > +}
> > +
> > +static struct r5l_io_unit *r5l_alloc_io_unit(struct r5l_log *log)
> > +{
> > + struct r5l_io_unit *io;
> > + gfp_t gfp = GFP_NOIO | __GFP_NOFAIL;
> > +
> > + io = kmem_cache_zalloc(log->io_kc, gfp);
> > + if (!io)
> > + return NULL;
> > + io->log = log;
> > + io->meta_page = alloc_page(gfp | __GFP_ZERO);
> > + if (!io->meta_page) {
> > + kmem_cache_free(log->io_kc, io);
> > + return NULL;
> > + }
>
> This can return NULL, but the one place where you call it you do not
> check for NULL.
> Maybe a mempool would be appropriate, maybe something else - I haven't
> examine the issue closely.
I use mempool for the cache patch, but eventually choose to use NOFAIL
allocation here, because don't know what the minimal mempool element
size should be. incorrect mempool element size could introduce trouble,
eg, the allocation expects we free an element, which might not possible
without increasing complexity in reclaim. Maybe I should just ignore the
NULL, it's a NOFAIL allocation. The log code isn't ready with allocation
failure.
> > +static int r5l_recovery_log(struct r5l_log *log)
> > +{
> > + /* fake recovery */
> > + log->seq = log->last_cp_seq + 1;
> > + log->log_start = r5l_ring_add(log, log->last_checkpoint, 1);
> > + return 0;
> > +}
> > +
> > +static void r5l_write_super(struct r5l_log *log, sector_t cp)
> > +{
> > + log->rdev->recovery_offset = cp;
> > + md_update_sb(log->mddev, 1);
> > +}
>
> This is only called from run() when the log is first initialised. At
> this point there is nothing useful in the log, so recording it's
> location is pointless. At most you could set the MD_SB_DIRTY flag (or
> whatever it is).
> So it really doesn't need to be a separate function.
We must write super here. If we start append data to the log and super
doesn't point to correct postion (reclaim might not run once yet),
recovery doesn't know where to find the log.
> > +void r5l_exit_log(struct r5l_log *log)
> > +{
> > + md_unregister_thread(&log->log_thread);
> > +
> > + kmem_cache_destroy(log->io_kc);
> > + kfree(log);
> > +}
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 59e44e9..9608a44 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -899,6 +899,8 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
> >
> > might_sleep();
> >
> > + if (!r5l_write_stripe(conf->log, sh))
> > + return;
>
> If no log is configured, r5l_write_stripe will return -EAGAIN, and so
> ops_run_io will never submit any IO....
I think you read it wrong, we don't return in that case.
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index 02c3bf8..a8daf39 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> > @@ -223,6 +223,9 @@ struct stripe_head {
> > struct stripe_head *batch_head; /* protected by stripe lock */
> > spinlock_t batch_lock; /* only header's lock is useful */
> > struct list_head batch_list; /* protected by head's batch lock*/
> > +
> > + struct r5l_io_unit *log_io;
> > + struct list_head log_list;
> > /**
> > * struct stripe_operations
> > * @target - STRIPE_OP_COMPUTE_BLK target
>
> I wonder if we really need yet another 'list_head' in 'stripe_head'.
> I guess one more is no great cost.
I'm pretty sure using the lru list of stripe_head is broken, that's why
I added the new list.
I'll fix other issues or add more comments.
Thanks,
Shaohua
^ permalink raw reply
* Re: RAID 6 reshape/grow interrupted
From: Mark Knecht @ 2015-08-05 19:57 UTC (permalink / raw)
To: George Rapp; +Cc: Linux-RAID
In-Reply-To: <CAF-KpgbSFTgDjwN8Kj0y2U4v81ofY1Lo08bzDei-XiE+1Lx7Ug@mail.gmail.com>
On Wed, Aug 5, 2015 at 12:39 PM, George Rapp <george.rapp@gmail.com> wrote:
<SNIP>
>
> First question - is anyone besides me seeing these messages?
>
> I see my postings in the archive at
> http://marc.info/?l=linux-raid&r=1&b=201508&w=2 so I assume they're
> going out to the distro list, but they have not been sent back to me
> nor have I received a reply.
>
I have seen 3 messages including this one.
I'm not qualified to talk about solutions but the messages are getting
through to the list.
HTH,
Mark
^ permalink raw reply
* Re: RAID 6 reshape/grow interrupted
From: George Rapp @ 2015-08-05 19:39 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <CAF-KpgaJTfh823O6ESPfDOk=vPfd-fcy5agtfbjpg4o1gYqmnA@mail.gmail.com>
> On Wed, Aug 5, 2015 at 11:59 AM, George Rapp <george.rapp@gmail.com> wrote:
>> On Wed, Aug 5, 2015 at 12:17 AM, George Rapp <george.rapp@gmail.com> wrote:
>>> Hello -
>>>
>>> Fedora 22 user (kernel 4.0.4-303.fc22.i686+PAE) using mdadm - v3.3.2 -
>>> 21st August 2014. (Of course, I don't have a backup ... 8^)
>>>
>>> I had a healthy RAID 6 array, and was trying to grow it from 5
>>> partitions of size 1.8 TB to 6 partitions.
>>>
>>> # mdadm --add /dev/md6 /dev/sdi1
>>>
>>> # mdadm --grow --raid-devices=6 --backup-file=/home/gwr/c/grow_md6.bak /dev/md6
>>>
>>> The second command threw a bunch of SELinux errors (ah, thank you,
>>> SELinux, for always being there to bite me in the ass when I don't
>>> expect it ... 8^) about access to /home/gwr/c/grow_md6.bak. The
>>> reshape operation sat for many minutes at 0% progress, according to
>>> /proc/mdstat. However, the file /home/gwr/c/grow_md6.bak *was*
>>> created; it's about 6MB.
>>
>> Additional data point: the backup file is all zeros, according to an
>> examination with 'od -v'.
>>
>>> In an attempt to kick off the reshape operation, I issued:
>>>
>>> # setenforce 0
>>>
>>> to turn off SELinux enforcement. That didn't help - the reshape sat
>>> still, showing no progress.
>>>
>>> Then I issued:
>>>
>>> # mdadm --stop /dev/md6
>>>
>>> which of course interrupted the reshape operation. It also threw up a
>>> bunch of error messages, which you can find in the dmesg.txt file
>>> found at https://app.box.com/s/3pksam3c7n79anpnzvsrwekzqwtsvlf6 --
>>> look for the words "cut here". It looks like a segfault or other
>>> runtime error:
>>>
>>> [ 796.84193] WARNING: CPU: 0 PID: 1444 at mm/backing-dev.c:372
>>> bdi_unregister+0x38/0x50()
>>>
>>> I then tried to restart the grow operation, without SELinux' help, and
>>> got the error message in the subject.
>>>
>>> First, I goofed, and tried the assemble without the backup file:
>>>
>>> # mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1 /dev/sdj1
>>>
>>> [ 1966.030411] md: md6 stopped.
>>>
>>> mdadm: Failed to restore critical section for reshape, sorry.
>>>
>>> # mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1
>>> /dev/sdj1 --backup-file=/home/gwr/c/grow_md6.bak
>>>
>>> [ 2242.492370] md: md6 stopped.
>>>
>>> mdadm: Failed to restore critical section for reshape, sorry.
>>>
>>> # mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1
>>> /dev/sdj1 /dev/sdi1 --backup-file=/home/gwr/c/grow_md6.bak
>>>
>>> [ 2403.741995] md: md6 stopped.
>>>
>>> mdadm: Failed to restore critical section for reshape, sorry.
>>>
>>> I ran an mdadm --examine on all my RAID partitions; the file is at
>>> https://app.box.com/s/9x2n2wc42i1wqzd1ayrt8ta6cyldrr6i. Of note in
>>> that file: the "Reshape pos'n" is 0 on all six drives. I take that to
>>> mean that the reshape operation never really got started.
>>>
>>> Is my next step to add the --invalid-backup switch? If not, what
>>> recommendations might you have to fix this?
>>
>> I tried the --invalid-backup switch this morning without success:
>>
>> md: md6 stopped.
>> md: bind<sdc4>
>> md: bind<sdg4>
>> md: bind<sdh1>
>> md: bind<sdj1>
>> md: bind<sdi1>
>> md: bind<sdd4>
>> md/raid:md6: reshape_position too early for auto-recovery - aborting.
>> md: pers->run() failed ...
>> mdadm: failed to RUN_ARRAY /dev/md6: Invalid argument
>> md: md6 stopped.
First question - is anyone besides me seeing these messages?
I see my postings in the archive at
http://marc.info/?l=linux-raid&r=1&b=201508&w=2 so I assume they're
going out to the distro list, but they have not been sent back to me
nor have I received a reply.
I had a rough time posting the initial message last night due to
Gmail's default use of HTML, which the linux-raid mailer-daemon does
not like -- it only accepted the message when I turned Gmail's "Plain
text mode" on.
Second, I see in http://lwn.net/Articles/565591/ that mdadm 3.3 has an
undocumented option:
"--assemble --update=revert-reshape" can be used to undo a reshape
that has just been started but isn't really wanted
Since I'm on mdadm v3.3.2, I assume I still have that option. Is it
worth trying in my situation?
Thanks for any advice.
George
^ permalink raw reply
* read errors aren't corrected
From: Mikael Abrahamsson @ 2015-08-05 17:43 UTC (permalink / raw)
To: linux-raid
Hi,
again, I have encountered drive with pending sectors, where a echo "check"
would complete, errors were reported, but sectors were not corrected:
Linux 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt11-1 (2015-05-24) x86_64 GNU/Linux
mdadm - v3.3.2 - 21st August 2014
[4915870.008999] md: data-check of RAID array md0
[4915870.009006] md: minimum _guaranteed_ speed: 1000 KB/sec/disk.
[4915870.009010] md: using maximum available idle IO bandwidth (but not more than 200000 KB/sec) for data-check.
[4915870.009021] md: using 128k window, over a total of 1953512960k.
[4944694.439086] mpt2sas0: log_info(0x31080000): originator(PL), code(0x08), sub_code(0x0000)
(repeat of above line approx 20 times)
[4944694.439167] sd 0:0:11:0: [sdl] Unhandled sense code
[4944694.439173] sd 0:0:11:0: [sdl]
[4944694.439178] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[4944694.439183] sd 0:0:11:0: [sdl]
[4944694.439188] Sense Key : Medium Error [current]
[4944694.439195] Info fld=0xddc6ccf0
[4944694.439202] sd 0:0:11:0: [sdl]
[4944694.439207] Add. Sense: Unrecovered read error
[4944694.439212] sd 0:0:11:0: [sdl] CDB:
[4944694.439216] Read(10): 28 00 dd c6 cb 28 00 04 00 00
[4944694.439231] end_request: critical medium error, dev sdl, sector 3720792872
[4946407.483424] md: md0: data-check done.
I ran the check 3 times, but still the pending sectors wouldn't go away.
Some of the times it would say it corrected errors:
[4828415.776842] sd 0:0:11:0: [sdl] Unhandled sense code
[4828415.776848] sd 0:0:11:0: [sdl]
[4828415.776852] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[4828415.776860] sd 0:0:11:0: [sdl]
[4828415.776864] Sense Key : Medium Error [current]
[4828415.776871] Info fld=0xddc44018
[4828415.776876] sd 0:0:11:0: [sdl]
[4828415.776881] Add. Sense: Unrecovered read error
[4828415.776886] sd 0:0:11:0: [sdl] CDB:
[4828415.776890] Read(10): 28 00 dd c4 40 00 00 00 80 00
[4828415.776905] end_request: critical medium error, dev sdl, sector 3720626176
[4828416.853170] raid5_end_read_request: 22 callbacks suppressed
[4828416.853189] md/raid:md0: read error corrected (8 sectors at 3720626176 on sdl)
[4828416.853198] md/raid:md0: read error corrected (8 sectors at 3720626184 on sdl)
[4828416.853203] md/raid:md0: read error corrected (8 sectors at 3720626192 on sdl)
[4828416.853208] md/raid:md0: read error corrected (8 sectors at 3720626200 on sdl)
[4828416.853213] md/raid:md0: read error corrected (8 sectors at 3720626208 on sdl)
[4828416.853217] md/raid:md0: read error corrected (8 sectors at 3720626216 on sdl)
[4828416.853223] md/raid:md0: read error corrected (8 sectors at 3720626224 on sdl)
[4828416.853228] md/raid:md0: read error corrected (8 sectors at 3720626232 on sdl)
[4828416.853236] md/raid:md0: read error corrected (8 sectors at 3720626240 on sdl)
[4828416.853242] md/raid:md0: read error corrected (8 sectors at 3720626248 on sdl)
I then gave up, proceeded to --replace the drive, take it out of the
md-array completely, do a destructive badblocks write test to it, it wrote
to the entire drive, and that made pending sectors go to 0.
What's weird is that there aren't any mentions of UNC in "smartctl -a"
error log. The drive is a Samsung HD204UI with 1AQ10001 firmware if that
makes any difference.
At no time was the drive kicked out of the array during any of these
tests. I run with 180 seconds timeouts in the kernel.
--
Mikael Abrahamsson email: swmike@swm.pp.se
^ permalink raw reply
* Re: RAID 6 reshape/grow interrupted
From: George Rapp @ 2015-08-05 15:59 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <CAF-Kpga0g_y_n_wJFWX84j0Aci_FjXXwNt1VRfMQ1wpcFpBZfg@mail.gmail.com>
On Wed, Aug 5, 2015 at 12:17 AM, George Rapp <george.rapp@gmail.com> wrote:
> Hello -
>
> Fedora 22 user (kernel 4.0.4-303.fc22.i686+PAE) using mdadm - v3.3.2 -
> 21st August 2014. (Of course, I don't have a backup ... 8^)
>
> I had a healthy RAID 6 array, and was trying to grow it from 5
> partitions of size 1.8 TB to 6 partitions.
>
> # mdadm --add /dev/md6 /dev/sdi1
>
> # mdadm --grow --raid-devices=6 --backup-file=/home/gwr/c/grow_md6.bak /dev/md6
>
> The second command threw a bunch of SELinux errors (ah, thank you,
> SELinux, for always being there to bite me in the ass when I don't
> expect it ... 8^) about access to /home/gwr/c/grow_md6.bak. The
> reshape operation sat for many minutes at 0% progress, according to
> /proc/mdstat. However, the file /home/gwr/c/grow_md6.bak *was*
> created; it's about 6MB.
Additional data point: the backup file is all zeros, according to an
examination with 'od -v'.
> In an attempt to kick off the reshape operation, I issued:
>
> # setenforce 0
>
> to turn off SELinux enforcement. That didn't help - the reshape sat
> still, showing no progress.
>
> Then I issued:
>
> # mdadm --stop /dev/md6
>
> which of course interrupted the reshape operation. It also threw up a
> bunch of error messages, which you can find in the dmesg.txt file
> found at https://app.box.com/s/3pksam3c7n79anpnzvsrwekzqwtsvlf6 --
> look for the words "cut here". It looks like a segfault or other
> runtime error:
>
> [ 796.84193] WARNING: CPU: 0 PID: 1444 at mm/backing-dev.c:372
> bdi_unregister+0x38/0x50()
>
> I then tried to restart the grow operation, without SELinux' help, and
> got the error message in the subject.
>
> First, I goofed, and tried the assemble without the backup file:
>
> # mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1 /dev/sdj1
>
> [ 1966.030411] md: md6 stopped.
>
> mdadm: Failed to restore critical section for reshape, sorry.
>
> # mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1
> /dev/sdj1 --backup-file=/home/gwr/c/grow_md6.bak
>
> [ 2242.492370] md: md6 stopped.
>
> mdadm: Failed to restore critical section for reshape, sorry.
>
> # mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1
> /dev/sdj1 /dev/sdi1 --backup-file=/home/gwr/c/grow_md6.bak
>
> [ 2403.741995] md: md6 stopped.
>
> mdadm: Failed to restore critical section for reshape, sorry.
>
> I ran an mdadm --examine on all my RAID partitions; the file is at
> https://app.box.com/s/9x2n2wc42i1wqzd1ayrt8ta6cyldrr6i. Of note in
> that file: the "Reshape pos'n" is 0 on all six drives. I take that to
> mean that the reshape operation never really got started.
>
> Is my next step to add the --invalid-backup switch? If not, what
> recommendations might you have to fix this?
I tried the --invalid-backup switch this morning without success:
md: md6 stopped.
md: bind<sdc4>
md: bind<sdg4>
md: bind<sdh1>
md: bind<sdj1>
md: bind<sdi1>
md: bind<sdd4>
md/raid:md6: reshape_position too early for auto-recovery - aborting.
md: pers->run() failed ...
mdadm: failed to RUN_ARRAY /dev/md6: Invalid argument
md: md6 stopped.
Any suggestions?
--
George Rapp (Pataskala, OH) Home: george.rapp -- at -- gmail.com
LinkedIn profile: https://www.linkedin.com/in/georgerapp
Phone: +1 740 936 RAPP (740 936 7277)
^ permalink raw reply
* RAID 6 reshape/grow interrupted
From: George Rapp @ 2015-08-05 4:17 UTC (permalink / raw)
To: linux-raid
Hello -
Fedora 22 user (kernel 4.0.4-303.fc22.i686+PAE) using mdadm - v3.3.2 -
21st August 2014. (Of course, I don't have a backup ... 8^)
I had a healthy RAID 6 array, and was trying to grow it from 5
partitions of size 1.8 TB to 6 partitions.
# mdadm --add /dev/md6 /dev/sdi1
# mdadm --grow --raid-devices=6 --backup-file=/home/gwr/c/grow_md6.bak /dev/md6
The second command threw a bunch of SELinux errors (ah, thank you,
SELinux, for always being there to bite me in the ass when I don't
expect it ... 8^) about access to /home/gwr/c/grow_md6.bak. The
reshape operation sat for many minutes at 0% progress, according to
/proc/mdstat. However, the file /home/gwr/c/grow_md6.bak *was*
created; it's about 6MB.
In an attempt to kick off the reshape operation, I issued:
# setenforce 0
to turn off SELinux enforcement. That didn't help - the reshape sat
still, showing no progress.
Then I issued:
# mdadm --stop /dev/md6
which of course interrupted the reshape operation. It also threw up a
bunch of error messages, which you can find in the dmesg.txt file
found at https://app.box.com/s/3pksam3c7n79anpnzvsrwekzqwtsvlf6 --
look for the words "cut here". It looks like a segfault or other
runtime error:
[ 796.84193] WARNING: CPU: 0 PID: 1444 at mm/backing-dev.c:372
bdi_unregister+0x38/0x50()
I then tried to restart the grow operation, without SELinux' help, and
got the error message in the subject.
First, I goofed, and tried the assemble without the backup file:
# mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1 /dev/sdj1
[ 1966.030411] md: md6 stopped.
mdadm: Failed to restore critical section for reshape, sorry.
# mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1
/dev/sdj1 --backup-file=/home/gwr/c/grow_md6.bak
[ 2242.492370] md: md6 stopped.
mdadm: Failed to restore critical section for reshape, sorry.
# mdadm --assemble /dev/md6 /dev/sdc4 /dev/sdd4 /dev/sdg4 /dev/sdh1
/dev/sdj1 /dev/sdi1 --backup-file=/home/gwr/c/grow_md6.bak
[ 2403.741995] md: md6 stopped.
mdadm: Failed to restore critical section for reshape, sorry.
I ran an mdadm --examine on all my RAID partitions; the file is at
https://app.box.com/s/9x2n2wc42i1wqzd1ayrt8ta6cyldrr6i. Of note in
that file: the "Reshape pos'n" is 0 on all six drives. I take that to
mean that the reshape operation never really got started.
Is my next step to add the --invalid-backup switch? If not, what
recommendations might you have to fix this?
Thanks.
George
^ permalink raw reply
* Re: [PATCH 9/9] raid5: skip resync if cache(log) is enabled
From: NeilBrown @ 2015-08-05 4:16 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <406d69eccd47d2eb57fca57bbafb9871e0858dcf.1438215986.git.shli@fb.com>
On Wed, 29 Jul 2015 17:38:49 -0700 Shaohua Li <shli@fb.com> wrote:
> If cache(log) is enabled, the log structure will guarantee data
> consistency, so skip resync for unclean shutdown
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 26ea100..330550a 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6967,6 +6967,15 @@ static int run(struct mddev *mddev)
> }
> }
>
> + if (conf->log) {
> + if (mddev->recovery_cp == 0) {
> + printk(KERN_NOTICE
> + "md/raid:%s: skip resync with caching enabled\n",
> + mdname(mddev));
> + mddev->recovery_cp = MaxSector;
> + }
> + }
> +
> return 0;
> abort:
> md_unregister_thread(&mddev->thread);
I don't think this is correct. When a RAID6 is started, recovery_cp
will be zero and we really do want a resync to happen even if there is
a log.
Rather, I think that recovery_cp should not be set to zero when a log
is being used. So keep the array appearing to be "clean".
If recovery_cp ever does get set to zero (mdadm --assemble
--update=resync can do this), then do the resync.
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH 7/9] raid5: don't allow resize/reshape with cache(log) support
From: NeilBrown @ 2015-08-05 4:13 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <7986246dabd1722b2ea9461da91651509049c2a6.1438215986.git.shli@fb.com>
On Wed, 29 Jul 2015 17:38:47 -0700 Shaohua Li <shli@fb.com> wrote:
> If cache(log) support is enabled, don't allow resize/reshape in current
> stage. In the future, we can flush all data from cache(log) to raid
> before resize/reshape and then allow resize/reshape.
Just to be on the safe side, you could probably add code to refuse to
start an array that is in the middle of a reshape and also have a log
configured.
I think it makes sense to plan ahead a little and make sure we can
handle a cache on a reshaping array properly.
If the log metadata block includes a before/after flag for each stripe,
which recorded whether the stripe was "before" or "after"
reshape_position when it was written, then when recovering the log we
can check if the given addresses are still on that side. If they are,
just recover using the appropriate geometry info from the superblock.
If not, then reshape has passed over that stripe and it must now be
fully up-to-date on the RAID so the data in the log can be discarded.
There may be some details I missed, but I think it is worth thinking
through properly. I don't expect the code to handle this straight
away, but we need a clear plan to be sure there is sufficient
information stored in the log.
Thanks,
NeilBrown
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 59f9312..b694d06 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7184,6 +7184,10 @@ static int raid5_resize(struct mddev *mddev, sector_t sectors)
> * worth it.
> */
> sector_t newsize;
> + struct r5conf *conf = mddev->private;
> +
> + if (conf->log)
> + return -EINVAL;
> sectors &= ~((sector_t)mddev->chunk_sectors - 1);
> newsize = raid5_size(mddev, sectors, mddev->raid_disks);
> if (mddev->external_size &&
> @@ -7235,6 +7239,8 @@ static int check_reshape(struct mddev *mddev)
> {
> struct r5conf *conf = mddev->private;
>
> + if (conf->log)
> + return -EINVAL;
> if (mddev->delta_disks == 0 &&
> mddev->new_layout == mddev->layout &&
> mddev->new_chunk_sectors == mddev->chunk_sectors)
^ permalink raw reply
* Re: [PATCH 5/9] raid5: log recovery
From: NeilBrown @ 2015-08-05 4:05 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <5683d4e81fe802c46350732560bbdd0a70212413.1438215986.git.shli@fb.com>
On Wed, 29 Jul 2015 17:38:45 -0700 Shaohua Li <shli@fb.com> wrote:
> This is the log recovery support. The process is quite straightforward.
> We scan the log and read all valid meta/data/parity into memory. If a
> stripe's data/parity checksum is correct, the stripe will be recoveried.
> Otherwise, it's discarded and we don't scan the log further. The reclaim
> process guarantees stripe which starts to be flushed raid disks has
> completed data/parity and has correct checksum. To recovery a stripe, we
> just copy its data/parity to corresponding raid disks.
>
> The trick thing is superblock update after recovery. we can't let
> superblock point to last valid meta block. The log might look like:
> | meta 1| meta 2| meta 3|
> meta 1 is valid, meta 2 is invalid. meta 3 could be valid. If superblock
> points to meta 1, we write a new valid meta 2n. If crash happens again,
> new recovery will start from meta 1. Since meta 2n is valid, recovery
> will think meta 3 is valid, which is wrong. The solution is we create a
> new meta in meta2 with its seq == meta 1's seq + 2 and let superblock
> points to meta2. recovery will not think meta 3 is a valid meta,
> because its seq is wrong
I like the idea of using a slightly larger 'seq' to avoid collisions -
except that I would probably feel safer with a much larger seq. May add
1024 or something (at least 10).
>
> TODO:
> -recovery should run the stripe cache state machine in case of disk
> breakage.
Why?
when you write to the log, you write all of the blocks that need
updating, whether they are destined for a failed device or not.
When you recover, you then have all the blocks that you might want to
write. So write all the ones for which you have working devices, and
ignore the rest.
Did I miss something?
Not that I object, but if it works....
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5-cache.c | 310 ++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/md/raid5.c | 4 +-
> drivers/md/raid5.h | 6 +
> 3 files changed, 315 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index a418e45..17dab66 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -785,11 +785,315 @@ static void r5l_wake_reclaim(struct r5l_log *log, r5blk_t space)
> md_wakeup_thread(log->reclaim_thread);
> }
>
> +struct r5l_recovery_ctx {
> + struct page *meta_page;
> + unsigned int meta_total_blocks;
> + r5blk_t pos;
> + u64 seq;
> +};
> +
> +static inline sector_t r5l_sector_to_stripe_sector(struct r5l_log *log,
> + sector_t sect)
> +{
> + struct r5conf *conf = log->mddev->private;
> + int dd;
> + return raid5_compute_sector(conf, sect, 0, &dd, NULL);
> +}
> +
> +static int r5l_read_meta_block(struct r5l_log *log,
> + struct r5l_recovery_ctx *ctx)
> +{
> + struct r5conf *conf = log->mddev->private;
> + struct page *page = ctx->meta_page;
> + struct r5l_meta_block *mb;
> + u32 crc, stored_crc;
> + struct r5l_payload_header *header;
> + int next_type = -1;
> + int last_type = -1;
> + sector_t last_stripe_sector = 0;
> + int offset;
> +
> + if (!sync_page_io(log->rdev, r5l_block_to_sector(log, ctx->pos),
> + log->block_size, page, READ, false))
> + return -EIO;
> +
> + mb = page_address(page);
> + stored_crc = le32_to_cpu(mb->checksum);
> + mb->checksum = 0;
> +
> + if (le32_to_cpu(mb->magic) != R5LOG_MAGIC ||
> + le64_to_cpu(mb->seq) != ctx->seq ||
> + mb->version != R5LOG_VERSION ||
> + le64_to_cpu(mb->position) != ctx->pos)
> + return -EINVAL;
> +
> + crc = r5l_calculate_checksum(log, log->uuid_checksum,
> + mb, log->block_size);
> + if (stored_crc != crc)
> + return -EINVAL;
> +
> + if (le32_to_cpu(mb->meta_size) > log->block_size)
> + return -EINVAL;
> +
> + ctx->meta_total_blocks = 1;
> + offset = sizeof(struct r5l_meta_block);
> + while (offset < le32_to_cpu(mb->meta_size)) {
> + u16 type;
> + header = page_address(page) + offset;
> + type = le16_to_cpu(header->type);
> +
> + if (next_type != -1 && type != next_type)
> + return -EINVAL;
> + if (type == R5LOG_PAYLOAD_DATA) {
> + struct r5l_payload_data_parity *payload;
> +
> + payload = (struct r5l_payload_data_parity *)header;
> + if (le32_to_cpu(payload->blocks) != r5l_page_blocks(log, 1))
> + return -EINVAL;
> + if (last_type != -1) {
> + if (r5l_sector_to_stripe_sector(log,
> + le64_to_cpu(payload->location)) !=
> + last_stripe_sector)
> + return -EINVAL;
> + } else
> + last_stripe_sector =
> + r5l_sector_to_stripe_sector(log,
> + le64_to_cpu(payload->location));
> +
> + ctx->meta_total_blocks += r5l_page_blocks(log, 1);
> + next_type = -1;
> + last_type = type;
> + offset += sizeof(struct r5l_payload_data_parity) +
> + sizeof(__le32);
> + } else if (type == R5LOG_PAYLOAD_PARITY) {
> + struct r5l_payload_data_parity *payload;
> +
> + payload = (struct r5l_payload_data_parity *)header;
> + if (last_type == -1)
> + return -EINVAL;
> +
> + if (le32_to_cpu(payload->blocks) !=
> + r5l_page_blocks(log, conf->max_degraded))
> + return -EINVAL;
> + if (le64_to_cpu(payload->location) != last_stripe_sector)
> + return -EINVAL;
> +
> + ctx->meta_total_blocks += r5l_page_blocks(log,
> + conf->max_degraded);
> + next_type = R5LOG_PAYLOAD_DATA;
> + last_type = -1;
> + offset += sizeof(struct r5l_payload_data_parity) +
> + sizeof(__le32) * conf->max_degraded;
> + } else
> + return -EINVAL;
> + }
> + if (offset > le32_to_cpu(mb->meta_size))
> + return -EINVAL;
> +
> + return 0;
> +}
I'm not sure the next_type/last_type stuff really helps. You are just
checking that there is at least one data block between pairs of parity
blocks, but that is a fairly weak test - there are plenty of other ways
that things could go wrong.
I think just trust that the data was written correctly, and that
checksum will detect the unlikely corruption.
> +
> +static int r5l_recovery_flush_one_stripe(struct r5l_log *log,
> + struct r5l_recovery_ctx *ctx, sector_t stripe_sect,
> + int *offset, r5blk_t *log_offset)
> +{
> + struct r5conf *conf = log->mddev->private;
> + struct stripe_head *dummy;
> + struct r5l_payload_data_parity *payload;
> + int disk_index;
> +
> + dummy = get_active_stripe(conf, stripe_sect, 0, 0, 0);
Why is this called 'dummy'. It is a real stripe and should be called
'sh'
> + while (1) {
> + payload = page_address(ctx->meta_page) + *offset;
> +
> + if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
> + raid5_compute_sector(conf,
> + le64_to_cpu(payload->location), 0,
> + &disk_index, dummy);
> +
> + sync_page_io(log->rdev, r5l_block_to_sector(log,
> + *log_offset), PAGE_SIZE,
> + dummy->dev[disk_index].page, READ, false);
> + dummy->dev[disk_index].log_checksum =
> + le32_to_cpu(payload->checksum[0]);
> + set_bit(R5_Wantwrite, &dummy->dev[disk_index].flags);
> + } else {
> + disk_index = dummy->pd_idx;
> + sync_page_io(log->rdev, r5l_block_to_sector(log,
> + *log_offset), PAGE_SIZE,
> + dummy->dev[disk_index].page, READ, false);
> + dummy->dev[disk_index].log_checksum =
> + le32_to_cpu(payload->checksum[0]);
> + set_bit(R5_Wantwrite, &dummy->dev[disk_index].flags);
> +
> + if (dummy->qd_idx >= 0) {
> + disk_index = dummy->qd_idx;
> + sync_page_io(log->rdev, r5l_block_to_sector(log,
> + r5l_ring_add(log, *log_offset,
> + r5l_page_blocks(log, 1))),
> + PAGE_SIZE,
> + dummy->dev[disk_index].page,
> + READ, false);
> + dummy->dev[disk_index].log_checksum =
> + le32_to_cpu(payload->checksum[1]);
> + set_bit(R5_Wantwrite,
> + &dummy->dev[disk_index].flags);
> + }
> + }
> +
> + *log_offset = r5l_ring_add(log, *log_offset,
> + le32_to_cpu(payload->blocks));
> + *offset += sizeof(struct r5l_payload_data_parity) +
> + sizeof(__le32) * (le32_to_cpu(payload->blocks) >>
> + log->page_block_shift);
> + if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY)
> + break;
> + }
> +
> + for (disk_index = 0; disk_index < dummy->disks; disk_index++) {
> + void *addr;
> + u32 checksum;
> +
> + if (!test_bit(R5_Wantwrite, &dummy->dev[disk_index].flags))
> + continue;
> + addr = kmap_atomic(dummy->dev[disk_index].page);
> + checksum = r5l_calculate_checksum(log,
> + log->uuid_checksum, addr, PAGE_SIZE);
> + kunmap_atomic(addr);
> + if (checksum != dummy->dev[disk_index].log_checksum)
> + goto error;
> + }
> +
> + /* FIXME: let raid core to handle the stripe */
> + for (disk_index = 0; disk_index < dummy->disks; disk_index++) {
> + struct md_rdev *rdev, *rrdev;
> + if (!test_and_clear_bit(R5_Wantwrite,
> + &dummy->dev[disk_index].flags))
> + continue;
> +
> + rdev = rcu_dereference(conf->disks[disk_index].rdev);
> + sync_page_io(rdev, stripe_sect, PAGE_SIZE,
> + dummy->dev[disk_index].page, WRITE, false);
> + rrdev = rcu_dereference(conf->disks[disk_index].replacement);
> + if (rrdev)
> + sync_page_io(rrdev, stripe_sect, PAGE_SIZE,
> + dummy->dev[disk_index].page, WRITE, false);
> + }
> + release_stripe(dummy);
> + return 0;
> +
> +error:
> + for (disk_index = 0; disk_index < dummy->disks; disk_index++)
> + dummy->dev[disk_index].flags = 0;
> + release_stripe(dummy);
> + return -EINVAL;
> +}
> +
> +static int r5l_recovery_flush_one_meta(struct r5l_log *log,
> + struct r5l_recovery_ctx *ctx)
> +{
> + struct r5l_payload_data_parity *payload;
> + struct r5l_meta_block *mb;
> + int offset;
> + r5blk_t log_offset;
> + sector_t stripe_sector;
> +
> + mb = page_address(ctx->meta_page);
> + offset = sizeof(struct r5l_meta_block);
> + log_offset = r5l_ring_add(log, ctx->pos, 1);
> +
> + while (offset < le32_to_cpu(mb->meta_size)) {
> + payload = (void *)mb + offset;
> + stripe_sector = r5l_sector_to_stripe_sector(log,
> + le64_to_cpu(payload->location));
> + if (r5l_recovery_flush_one_stripe(log, ctx, stripe_sector,
> + &offset, &log_offset))
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +/* copy data/parity from log to raid disks */
> +static void r5l_recovery_flush_log(struct r5l_log *log,
> + struct r5l_recovery_ctx *ctx)
> +{
> + while (1) {
> + if (r5l_read_meta_block(log, ctx))
> + return;
> + if (r5l_recovery_flush_one_meta(log, ctx))
> + return;
> + ctx->seq++;
> + ctx->pos = r5l_ring_add(log, ctx->pos, ctx->meta_total_blocks);
> + }
> +}
> +
> +static int r5l_log_write_empty_meta_block(struct r5l_log *log, r5blk_t pos,
> + u64 seq)
> +{
> + struct page *page;
> + struct r5l_meta_block *mb;
> + u32 crc;
> +
> + page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> + if (!page)
> + return -ENOMEM;
> + mb = page_address(page);
> + mb->magic = cpu_to_le32(R5LOG_MAGIC);
> + mb->version = R5LOG_VERSION;
> + mb->block_size = cpu_to_le16(log->block_size);
> + mb->meta_size = cpu_to_le32(sizeof(struct r5l_meta_block));
> + mb->seq = cpu_to_le64(seq);
> + mb->position = cpu_to_le64(pos);
> + crc = r5l_calculate_checksum(log, log->uuid_checksum, mb,
> + log->block_size);
> + mb->checksum = cpu_to_le32(crc);
> +
> + if (!sync_page_io(log->rdev, r5l_block_to_sector(log, pos),
> + log->block_size, page, WRITE_FUA, false)) {
> + __free_page(page);
> + return -EIO;
> + }
> + __free_page(page);
> + return 0;
> +}
> +
> static int r5l_recovery_log(struct r5l_log *log)
> {
> - /* fake recovery */
> - log->seq = log->last_cp_seq + 1;
> - log->log_start = r5l_ring_add(log, log->last_checkpoint, 1);
> + struct r5l_recovery_ctx ctx;
> +
> + ctx.pos = log->last_checkpoint;
> + ctx.seq = log->last_cp_seq;
> + ctx.meta_page = alloc_page(GFP_KERNEL);
> + if (!ctx.meta_page)
> + return -ENOMEM;
> +
> + r5l_recovery_flush_log(log, &ctx);
> + __free_page(ctx.meta_page);
> +
> + /*
> + * we did a recovery. Now ctx.pos points to an invalid meta block. New
> + * log will start here. but we can't let superblock point to last valid
> + * meta block. The log might looks like:
> + * | meta 1| meta 2| meta 3|
> + * meta 1 is valid, meta 2 is invalid. meta 3 could be valid. If
> + * superblock points to meta 1, we write a new valid meta 2n. if crash
> + * happens again, new recovery will start from meta 1. Since meta 2n is
> + * valid now, recovery will think meta 3 is valid, which is wrong.
> + * The solution is we create a new meta in meta2 with its seq == meta
> + * 1's seq + 2 and let superblock points to meta2. The same recovery will
> + * not think meta 3 is a valid meta, because its seq doesn't match
> + */
> + if (ctx.seq > log->last_cp_seq + 1) {
> + int ret;
> + r5l_flush_all_disks(log);
> +
> + ret = r5l_log_write_empty_meta_block(log, ctx.pos, ctx.seq + 1);
> + if (ret)
> + return ret;
> + log->seq = ctx.seq + 2;
> + log->log_start = r5l_ring_add(log, ctx.pos, 1);
> + r5l_write_super(log, r5l_block_to_sector(log, ctx.pos));
> + }
> return 0;
> }
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index d1ddd31..77af7f0 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -662,7 +662,7 @@ static int has_failed(struct r5conf *conf)
> return 0;
> }
>
> -static struct stripe_head *
> +struct stripe_head *
> get_active_stripe(struct r5conf *conf, sector_t sector,
> int previous, int noblock, int noquiesce)
> {
> @@ -2527,7 +2527,7 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
> * Input: a 'big' sector number,
> * Output: index of the data and parity disk, and the sector # in them.
> */
> -static sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
> +sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
> int previous, int *dd_idx,
> struct stripe_head *sh)
> {
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 23cc9c3..fd10d29 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -618,4 +618,10 @@ extern int r5l_write_stripe(struct r5l_log *log, struct stripe_head *head_sh);
> extern void r5l_write_stripe_run(struct r5l_log *log);
> extern void r5l_flush_stripe_to_raid(struct r5l_log *log);
> extern void r5l_stripe_write_finished(struct stripe_head *sh);
> +extern sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
> + int previous, int *dd_idx,
> + struct stripe_head *sh);
> +extern struct stripe_head *
> +get_active_stripe(struct r5conf *conf, sector_t sector,
> + int previous, int noblock, int noquiesce);
> #endif
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH 4/9] raid5: log reclaim support
From: NeilBrown @ 2015-08-05 3:52 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <41a6d2e74494506fcd6ffd68a5884d38dc0bfa8e.1438215986.git.shli@fb.com>
On Wed, 29 Jul 2015 17:38:44 -0700 Shaohua Li <shli@fb.com> wrote:
> + while (!list_empty(&log->stripe_end_ios)) {
> + io = list_first_entry(&log->stripe_end_ios,
> + struct r5l_io_unit, log_sibling);
> + list_move_tail(&io->log_sibling, &list);
> + free += (io->log_end - io->log_start +
> + log->total_blocks) % log->total_blocks;
> + }
sorry, forgot to mention this bit.
That '%' is acting on 64 but numbers, so it won't build in a 32bit
machine.
Maybe use SECTOR_DIV, maybe do an
if (x > y) free = x-y else free = x+total-y;
or something.
NeilBrown
^ permalink raw reply
* Re: [PATCH 4/9] raid5: log reclaim support
From: NeilBrown @ 2015-08-05 3:43 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <41a6d2e74494506fcd6ffd68a5884d38dc0bfa8e.1438215986.git.shli@fb.com>
On Wed, 29 Jul 2015 17:38:44 -0700 Shaohua Li <shli@fb.com> wrote:
> This is the reclaim support for raid5 log. A stripe write will have
> following steps:
>
> 1. reconstruct the stripe, read data/calculate parity. ops_run_io
> prepares to write data/parity to raid disks
> 2. hijack ops_run_io. stripe data/parity is appending to log disk
> 3. flush log disk cache
> 4. ops_run_io run again and do normal operation. stripe data/parity is
> written in raid array disks. raid core can return io to upper layer.
> 5. flush cache of all raid array disks
> 6. update super block
> 7. log disk space used by the stripe can be reused
>
> In practice, several stripes consist of an io_unit and we will batch
> several io_unit in different steps, but the whole process doesn't
> change.
>
> It's possible io return just after data/parity hit log disk, but then
> read IO will need read from log disk. For simplicity, IO return happens
> at step 4, where read IO can directly read from raid disks.
>
> Currently reclaim run every minute or out of space. Reclaim is just to
> free log disk spaces, it doesn't impact data consistency.
Having arbitrary times lines "every minute" is a warning sign.
"As soon as possible" and "Just it time" can both make sense easily.
"every minute" needs more justification.
I'll probably say more when I find the code.
>
> Recovery make sure raid disks and log disk have the same data of a
> stripe. If crash happens before 4, recovery might/might not recovery
> stripe's data/parity depending on if data/parity and its checksum
> matches. In either case, this doesn't change the syntax of an IO write.
> After step 3, stripe is guaranteed recoverable, because stripe's
> data/parity is persistent in log disk. In some cases, log disk content
> and raid disks content of a stripe are the same, but recovery will still
> copy log disk content to raid disks, this doesn't impact data
> consistency. space reuse happens after superblock update and cache
> flush.
>
> There is one situation we want to avoid. A broken meta in the middle of
> a log causes recovery can't find meta at the head of log. If operations
> require meta at the head persistent in log, we must make sure meta
> before it persistent in log too. The case is stripe data/parity is in
> log and we start write stripe to raid disks (before step 4). stripe
> data/parity must be persistent in log before we do the write to raid
> disks. The solution is we restrictly maintain io_unit list order. In
> this case, we only write stripes of an io_unit to raid disks till the
> io_unit is the first one whose data/parity is in log.
>
> The io_unit list order is important for other cases too. For example,
> some io_unit are reclaimable and others not. They can be mixed in the
> list, we shouldn't reuse space of an unreclaimable io_unit.
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5-cache.c | 261 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/md/raid5.c | 8 +-
> drivers/md/raid5.h | 3 +
> 3 files changed, 271 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 5c9bab6..a418e45 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -25,6 +25,7 @@
> #include "raid5.h"
>
> typedef u64 r5blk_t; /* log blocks, 512B - 4k */
> +#define RECLAIM_TIMEOUT (60 * HZ) /* reclaim run every 60s */
>
> struct r5l_log {
> struct mddev *mddev;
> @@ -54,9 +55,14 @@ struct r5l_log {
>
> spinlock_t io_list_lock;
> struct list_head running_ios; /* running io_unit list */
> + struct list_head io_end_ios; /* io end io_unit list */
> + struct list_head stripe_end_ios; /* stripe end io_unit list */
When the comment has nearly the same words as the code, it isn't adding
much, a bit like:
i++; /* increment i */
Maybe you mean:
struct list_head running_ios; /* io_units which are still running,
* and have not yet been completely
* written to the log. */
struct list_head io_end_ios; /* io_units which have been completely
* written to the log but not yet
* written to the RAID */
struct list_head stripe_end_ios;/* io_units which have been
* completely written to the RAID
* but have not yet been considered
* for updating the start of the
* log. */
>
> struct kmem_cache *io_kc;
>
> + struct md_thread *reclaim_thread;
> + r5blk_t reclaim_target; /* 0 means reclaiming possible io_unit */
> +
by "target" I think you mean "number of blocks that need to be
reclaimed". I can't quite guess what a "possible io_unit" is.
int reclaim_target; /* number of blocks that need to be reclaimed
* promptly. If 0, then .....
*/
> struct md_thread *log_thread;
> struct list_head log_stripes;
> spinlock_t log_stripes_lock;
> @@ -537,8 +543,246 @@ void r5l_write_stripe_run(struct r5l_log *log)
> md_wakeup_thread(log->log_thread);
> }
>
> +void r5l_stripe_write_finished(struct stripe_head *sh)
> +{
> + struct r5l_io_unit *io;
> +
> + /* Don't support stripe batch */
> + io = sh->log_io;
> + if (!io)
> + return;
> + sh->log_io = NULL;
> +
> + if (!atomic_dec_and_test(&io->pending_stripe))
> + return;
> + r5l_set_io_unit_state(io, IO_UNIT_STRIPE_END);
if (atomic_dec_and_test(...))
r5l_set_io_unit_state(...);
??
> +}
> +
> +static void r5l_compress_stripe_end_list(struct r5l_log *log)
> +{
> + struct r5l_io_unit *first, *last, *io;
> +
> + if (list_empty(&log->stripe_end_ios))
> + return;
> + first = list_first_entry(&log->stripe_end_ios,
> + struct r5l_io_unit, log_sibling);
> + last = list_last_entry(&log->stripe_end_ios,
> + struct r5l_io_unit, log_sibling);
> + /* Keep 2 io_unit in the list, superblock points to the last one */
> + if (first == last)
> + return;
> + list_del(&first->log_sibling);
> + list_del(&last->log_sibling);
> + while (!list_empty(&log->stripe_end_ios)) {
> + io = list_first_entry(&log->stripe_end_ios,
> + struct r5l_io_unit, log_sibling);
> + list_del(&io->log_sibling);
> + first->log_end = io->log_end;
> + r5l_free_io_unit(log, io);
> + }
> + list_add_tail(&first->log_sibling, &log->stripe_end_ios);
> + list_add_tail(&last->log_sibling, &log->stripe_end_ios);
> +}
A comment explaining why you might want to compress a list (which I
think means to only keep the first and last) would be quite helpful
here.
> +
> +static void r5l_move_io_unit_list(struct list_head *from, struct list_head *to,
> + int state)
> +{
> + struct r5l_io_unit *io;
> +
> + while (!list_empty(from)) {
> + io = list_first_entry(from, struct r5l_io_unit, log_sibling);
> + /* don't change list order */
> + if (io->state >= state)
> + list_move_tail(&io->log_sibling, to);
> + else
> + break;
> + }
> +}
> +
> +/*
> + * Starting dispatch IO to raid.
> + * io_unit(meta) consists of a log. There is one situation we want to avoid. A
> + * broken meta in the middle of a log causes recovery can't find meta at the
> + * head of log. If operations require meta at the head persistent in log, we
> + * must make sure meta before it persistent in log too. A case is:
> + *
> + * stripe data/parity is in log, we start write stripe to raid disks. stripe
> + * data/parity must be persistent in log before we do the write to raid disks.
> + *
> + * The solution is we restrictly maintain io_unit list order. In this case, we
> + * only write stripes of an io_unit to raid disks till the io_unit is the first
> + * one whose data/parity is in log.
> + * */
> +void r5l_flush_stripe_to_raid(struct r5l_log *log)
> +{
> + struct r5l_io_unit *io;
> + struct stripe_head *sh;
> + bool run_stripe;
> +
> + if (!log)
> + return;
> + /* find io_unit with io end but stripes are not running */
> + spin_lock(&log->io_list_lock);
> + r5l_move_io_unit_list(&log->running_ios, &log->io_end_ios,
> + IO_UNIT_IO_END);
> + r5l_move_io_unit_list(&log->io_end_ios, &log->stripe_end_ios,
> + IO_UNIT_STRIPE_END);
> + r5l_compress_stripe_end_list(log);
> + run_stripe = !list_empty(&log->io_end_ios);
> + spin_unlock(&log->io_list_lock);
> +
> + if (!run_stripe)
> + return;
> +
> + blkdev_issue_flush(r5l_bdev(log), GFP_NOIO, NULL);
> +
> + spin_lock(&log->io_list_lock);
> + list_for_each_entry(io, &log->io_end_ios, log_sibling) {
> + if (io->state >= IO_UNIT_STRIPE_START)
> + continue;
> + r5l_set_io_unit_state(io, IO_UNIT_STRIPE_START);
> +
> + while (!list_empty(&io->stripe_list)) {
> + sh = list_first_entry(&io->stripe_list,
> + struct stripe_head, log_list);
> + list_del_init(&sh->log_list);
> + set_bit(STRIPE_HANDLE, &sh->state);
> + release_stripe(sh);
This code makes me a bit nervous. handle_stripe() can potentially be
called on any stripe at any time.
Here you are scheduling a call the handle_stripe() without obviously
changing the state of the stripe. So whatever is going to happen now
could potentially have happened before... is that safe?
> + }
> + }
> + spin_unlock(&log->io_list_lock);
> +}
> +
> +static void r5l_disks_flush_end(struct bio *bio, int err)
> +{
> + struct completion *io_complete = bio->bi_private;
> +
> + complete(io_complete);
> + bio_put(bio);
> +}
> +
> +static void r5l_flush_all_disks(struct r5l_log *log)
> +{
> + struct mddev *mddev = log->mddev;
> + struct bio *bi;
> + DECLARE_COMPLETION_ONSTACK(io_complete);
> +
> + bi = bio_alloc_mddev(GFP_NOIO, 0, mddev);
> + bi->bi_end_io = r5l_disks_flush_end;
> + bi->bi_private = &io_complete;
> +
> + /* If bio hasn't payload, this function will just flush all disks */
> + md_flush_request(mddev, bi);
> +
> + wait_for_completion_io(&io_complete);
> +}
> +
> +static void r5l_kick_io_unit(struct r5l_log *log, struct r5l_io_unit *io)
> +{
> + /* the log thread will log the io unit */
> + r5l_wait_io_unit_state(io, IO_UNIT_IO_END);
> + if (io->state < IO_UNIT_STRIPE_START)
> + r5l_flush_stripe_to_raid(log);
> + r5l_wait_io_unit_state(io, IO_UNIT_STRIPE_END);
> +}
> +
> +static void r5l_write_super(struct r5l_log *log, sector_t cp);
> +static void r5l_do_reclaim(struct r5l_log *log)
> +{
> + struct r5l_io_unit *io, *last;
> + LIST_HEAD(list);
> + r5blk_t free = 0;
> + r5blk_t reclaim_target = xchg(&log->reclaim_target, 0);
> +
> + spin_lock(&log->io_list_lock);
> + /*
> + * move proper io_unit to reclaim list. We should not change the order.
> + * reclaimable/unreclaimable io_unit can be mixed in the list, we
> + * shouldn't reuse space of an unreclaimable io_unit
> + * */
> + while (1) {
> + r5l_move_io_unit_list(&log->running_ios, &log->io_end_ios,
> + IO_UNIT_IO_END);
> + r5l_move_io_unit_list(&log->io_end_ios, &log->stripe_end_ios,
> + IO_UNIT_STRIPE_END);
> + while (!list_empty(&log->stripe_end_ios)) {
> + io = list_first_entry(&log->stripe_end_ios,
> + struct r5l_io_unit, log_sibling);
> + list_move_tail(&io->log_sibling, &list);
> + free += (io->log_end - io->log_start +
> + log->total_blocks) % log->total_blocks;
> + }
> +
> + if (free >= reclaim_target || (list_empty(&log->running_ios) &&
> + list_empty(&log->io_end_ios) &&
> + list_empty(&log->stripe_end_ios)))
> + break;
> +
> + if (!list_empty(&log->io_end_ios)) {
> + io = list_first_entry(&log->io_end_ios,
> + struct r5l_io_unit, log_sibling);
> + spin_unlock(&log->io_list_lock);
> + /* nobody else can delete the io, we are safe */
> + r5l_kick_io_unit(log, io);
> + spin_lock(&log->io_list_lock);
> + continue;
> + }
> +
> + if (!list_empty(&log->running_ios)) {
> + io = list_first_entry(&log->running_ios,
> + struct r5l_io_unit, log_sibling);
> + spin_unlock(&log->io_list_lock);
> + /* nobody else can delete the io, we are safe */
> + r5l_kick_io_unit(log, io);
> + spin_lock(&log->io_list_lock);
> + continue;
> + }
> + }
> + spin_unlock(&log->io_list_lock);
Well, here we are with the important parts of the reclaim code...
The main result of the above section is to possibly call
r5l_flush_stripe_to_raid() a few times, and to wait until 'list'
contains enough io_units to satisfy the requirement.
As raid5d already calls r5l_flush_stripe_to_raid - which it really must
to make sure that writes complete quickly - this really comes down to
some book keeping and some waiting.
Book keeping can be done as changes happen, and waiting is best not
done at all.
To be more specific: when an io_unit transitions to IO_UNIT_STRIPE_END
it can immediately be removed from the list and if it was the first
io_unit on the list, then the log_start can immediately be updated.
> +
> + if (list_empty(&list))
> + return;
> +
> + r5l_flush_all_disks(log);
> +
> + /* super always point to last valid meta */
> + last = list_last_entry(&list, struct r5l_io_unit, log_sibling);
> + r5l_write_super(log, r5l_block_to_sector(log, last->log_start));
This bit flushes all the disks and then updates the metadata and writes
it.
As md_super_write already uses WRITE_FLUSH_FUA I don't think the extra
flush is needed.
I really think you should just update ->recovery_offset and set
MD_CHANGE_DEVS (or similar) and let the update happen.
> +
> + mutex_lock(&log->io_mutex);
> + log->last_checkpoint = last->log_start;
> + log->last_cp_seq = last->seq;
> + mutex_unlock(&log->io_mutex);
> + wake_up(&log->space_waitq);
> +
> + while (!list_empty(&list)) {
> + io = list_first_entry(&list, struct r5l_io_unit, log_sibling);
> + list_del(&io->log_sibling);
> + r5l_free_io_unit(log, io);
> + }
> +}
So I really think all of this can be done as-it-happens (the
book-keeping) or asynchronously. There is no need to push something
every minute.
> +
> +static void r5l_reclaim_thread(struct md_thread *thread)
> +{
> + struct mddev *mddev = thread->mddev;
> + struct r5conf *conf = mddev->private;
> + struct r5l_log *log = conf->log;
> +
> + if (!log)
> + return;
> + r5l_do_reclaim(log);
> +}
> +
> static void r5l_wake_reclaim(struct r5l_log *log, r5blk_t space)
> {
> + r5blk_t target;
> +
> + do {
> + target = log->reclaim_target;
> + if (space < target)
> + return;
> + } while (cmpxchg(&log->reclaim_target, target, space) != target);
> + md_wakeup_thread(log->reclaim_thread);
> }
>
> static int r5l_recovery_log(struct r5l_log *log)
> @@ -642,11 +886,19 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
>
> spin_lock_init(&log->io_list_lock);
> INIT_LIST_HEAD(&log->running_ios);
> + INIT_LIST_HEAD(&log->io_end_ios);
> + INIT_LIST_HEAD(&log->stripe_end_ios);
>
> log->io_kc = KMEM_CACHE(r5l_io_unit, 0);
> if (!log->io_kc)
> goto io_kc;
>
> + log->reclaim_thread = md_register_thread(r5l_reclaim_thread,
> + log->mddev, "reclaim");
> + if (!log->reclaim_thread)
> + goto reclaim_thread;
> + log->reclaim_thread->timeout = RECLAIM_TIMEOUT;
> +
> INIT_LIST_HEAD(&log->log_stripes);
> spin_lock_init(&log->log_stripes_lock);
> log->log_thread = md_register_thread(r5l_log_thread,
> @@ -662,6 +914,8 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
> error:
> md_unregister_thread(&log->log_thread);
> log_thread:
> + md_unregister_thread(&log->reclaim_thread);
> +reclaim_thread:
> kmem_cache_destroy(log->io_kc);
> io_kc:
> kfree(log);
> @@ -670,6 +924,13 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
>
> void r5l_exit_log(struct r5l_log *log)
> {
> + /*
> + * at this point all stripes are finished, so io_unit is at least in
> + * STRIPE_END state
> + * */
> + r5l_wake_reclaim(log, -1L);
> + md_unregister_thread(&log->reclaim_thread);
> + r5l_do_reclaim(log);
> md_unregister_thread(&log->log_thread);
>
> kmem_cache_destroy(log->io_kc);
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 9608a44..d1ddd31 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -417,7 +417,7 @@ static int release_stripe_list(struct r5conf *conf,
> return count;
> }
>
> -static void release_stripe(struct stripe_head *sh)
> +void release_stripe(struct stripe_head *sh)
> {
> struct r5conf *conf = sh->raid_conf;
> unsigned long flags;
> @@ -3101,6 +3101,8 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
> if (bi)
> bitmap_end = 1;
>
> + r5l_stripe_write_finished(sh);
> +
> if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
> wake_up(&conf->wait_for_overlap);
>
> @@ -3499,6 +3501,8 @@ static void handle_stripe_clean_event(struct r5conf *conf,
> WARN_ON(dev->page != dev->orig_page);
> }
>
> + r5l_stripe_write_finished(sh);
> +
> if (!discard_pending &&
> test_bit(R5_Discard, &sh->dev[sh->pd_idx].flags)) {
> clear_bit(R5_Discard, &sh->dev[sh->pd_idx].flags);
> @@ -5867,6 +5871,8 @@ static void raid5d(struct md_thread *thread)
> set_bit(R5_DID_ALLOC, &conf->cache_state);
> }
>
> + r5l_flush_stripe_to_raid(conf->log);
> +
> async_tx_issue_pending_all();
> blk_finish_plug(&plug);
>
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index a8daf39..23cc9c3 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -611,8 +611,11 @@ static inline int algorithm_is_DDF(int layout)
> extern void md_raid5_kick_device(struct r5conf *conf);
> extern int raid5_set_cache_size(struct mddev *mddev, int size);
> extern sector_t compute_blocknr(struct stripe_head *sh, int i, int previous);
> +extern void release_stripe(struct stripe_head *sh);
> extern int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev);
> extern void r5l_exit_log(struct r5l_log *log);
> extern int r5l_write_stripe(struct r5l_log *log, struct stripe_head *head_sh);
> extern void r5l_write_stripe_run(struct r5l_log *log);
> +extern void r5l_flush_stripe_to_raid(struct r5l_log *log);
> +extern void r5l_stripe_write_finished(struct stripe_head *sh);
> #endif
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH 3/9] raid5: add basic stripe log
From: NeilBrown @ 2015-08-05 3:07 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <29c5d697b86094fb7141b23875eb4da6abc6022c.1438215986.git.shli@fb.com>
On Wed, 29 Jul 2015 17:38:43 -0700 Shaohua Li <shli@fb.com> wrote:
> This introduces a simple log for raid5. Data/parity writting to raid
> array first writes to the log, then write to raid array disks. If crash
> happens, we can recovery data from the log. This can speed up raid
> resync and fix write hole issue.
>
> The log structure is pretty simple. Data/meta data is stored in block
> unit, which is 4k generally. It has only one type of meta data block.
> The meta data block can track 3 types of data, stripe data, stripe
> parity and flush block. MD superblock will point to the last valid meta
> data block. Each meta data block has checksum/seq number, so recovery
> can scan the log correctly. We store a checksum of stripe data/parity to
> the metadata block, so meta data and stripe data/parity can be written
> to log disk together. otherwise, meta data write must wait till stripe
> data/parity is finished.
>
> For stripe data, meta data block will record stripe data sector and
> size. Currently the size is always 4k. This meta data record can be made
> simpler if we just fix write hole (eg, we can record data of a stripe's
> different disks together), but this format can be extended to support
> caching in the future, which must record data address/size.
>
> For stripe parity, meta data block will record stripe sector. It's size
> should be 4k (for raid5) or 8k (for raid6). We always store p parity
> first. This format should work for caching too.
It feels a bit odd have a 8K parity block for RAID6 as it is really two
blocks: a parity block and a Q-syndrome block. What would you think of
introducing another block type for Q? So there are Data blocks, Parity
blocks, and Q blocks ???
Not a big issue, but I thought I would mention it.
>
> flush block indicates a stripe is in raid array disks. Fixing write hole
> doesn't need this type of meta data, it's for caching extention.
>
> We should be careful about deadlock. Appending stripe data/parity to log
> is done in raid5d. The append need log space, which can trigger log
> reclaim, which might wait for raid5d to run (to flush data from log to
> raid array disks). Here we always do the log write in a separate thread.
I'm not convinced about the need for a separate thread. As
raid5d/handle_stripe works as a state machine, and as all IO is async,
we should at most need an extra state, not an extra thread.
I think a key issue is that you don't call r5l_get_reserve() until you
are about to submit writes to the log.
If instead you reserved the space in r5l_write_stripe and delay the
stripe if space is not available, there there would be no deadlock.
Then when available space crosses some threshold, re-activate those
delayed stripes.
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/Makefile | 2 +-
> drivers/md/raid5-cache.c | 677 +++++++++++++++++++++++++++++++++++++++++
> drivers/md/raid5.c | 8 +-
> drivers/md/raid5.h | 10 +
> include/uapi/linux/raid/md_p.h | 48 +++
> 5 files changed, 741 insertions(+), 4 deletions(-)
> create mode 100644 drivers/md/raid5-cache.c
>
> diff --git a/drivers/md/Makefile b/drivers/md/Makefile
> index 462f443..f34979c 100644
> --- a/drivers/md/Makefile
> +++ b/drivers/md/Makefile
> @@ -17,7 +17,7 @@ dm-cache-smq-y += dm-cache-policy-smq.o
> dm-cache-cleaner-y += dm-cache-policy-cleaner.o
> dm-era-y += dm-era-target.o
> md-mod-y += md.o bitmap.o
> -raid456-y += raid5.o
> +raid456-y += raid5.o raid5-cache.o
>
> # Note: link order is important. All raid personalities
> # and must come before md.o, as they each initialise
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> new file mode 100644
> index 0000000..5c9bab6
> --- /dev/null
> +++ b/drivers/md/raid5-cache.c
> @@ -0,0 +1,677 @@
> +/*
> + * Copyright (C) 2015 Shaohua Li <shli@fb.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +#include <linux/kernel.h>
> +#include <linux/wait.h>
> +#include <linux/blkdev.h>
> +#include <linux/slab.h>
> +#include <linux/raid/md_p.h>
> +#include <linux/crc32.h>
> +#include <linux/random.h>
> +#include "md.h"
> +#include "raid5.h"
> +
> +typedef u64 r5blk_t; /* log blocks, 512B - 4k */
I would much rather use sector_t throughout and keep all addresses as
sector addresses. Much less room for confusion that way.
If we wanted to pack lots of addresses into limited space then using
block addresses might be justified (filesystems do that), but I don't
think it is called for here at all.
> +
> +struct r5l_log {
> + struct mddev *mddev;
> + struct md_rdev *rdev;
As the rdev contains a pointer to the mddev, storing the mddev as well
isn't strictly necessary - though it doesn't really hurt.
You don't seem to use it very much....
> +
> + u32 uuid_checksum;
> +
> + unsigned int block_size; /* bytes */
> + unsigned int block_sector_shift;
> + unsigned int page_block_shift; /* page to block */
These last two probably aren't needed if everything is in sectors.
> +
> + r5blk_t total_blocks;
> + r5blk_t first_block;
> + r5blk_t last_block;
> +
> + r5blk_t last_checkpoint; /* log tail */
> + u64 last_cp_seq; /* log tail sequence */
> +
> + u64 seq; /* current sequence */
> + r5blk_t log_start; /* current log position */
How is this different from first_block? Or maybe it is last_block.
It is good that you have some comments here, but a few more wouldn't
hurt.
Also it sometimes helps to say how something is used as well as what
it is.
e.g. is the "current sequence" number the sequence number of the last
metadata block written, or of the next metadata block to be written?
> +
> + r5blk_t reserved_blocks;
> + wait_queue_head_t space_waitq;
> +
> + struct mutex io_mutex;
> + struct r5l_io_unit *current_io;
> +
> + spinlock_t io_list_lock;
> + struct list_head running_ios; /* running io_unit list */
A few more words to make it clear what "running" means would be good.
An io_unit becomes "running" when .... and ceases to be "running"
when .......
> +
> + struct kmem_cache *io_kc;
> +
> + struct md_thread *log_thread;
> + struct list_head log_stripes;
> + spinlock_t log_stripes_lock;
> +};
> +
> +/*
> + * an IO range starts from a meta data block and end at the next meta data
> + * block. The io unit's the meta data block tracks data/parity followed it. io
> + * unit is written to log disk with normal write, as we always flush log disk
> + * first and then start move data to raid disks, there is no requirement to
> + * write io unit with FLUSH/FUA
> + * */
> +struct r5l_io_unit {
> + struct r5l_log *log;
> +
> + struct page *meta_page; /* store meta block */
> + int meta_offset; /* current offset in meta_page */
> +
> + struct bio_list bios;
> + struct bio *current_bio; /* current_bio accepting pages */
> + atomic_t pending_io; /* pending bios not writting to log */
"now writing" ?? (I often type 'now' and 'not' :-)
Actually it is often "1 more than the number of bios queued but which
have not yet completed".
In this case there is no need for that "one more than" as we first
queue all the bios, then we submit them.
You only need the "one more than" thing if you might keep incrementing
the counter after you start submitting bios.
> +
> + atomic_t pending_stripe; /* how many stripes not flushed to raid */
> + u64 seq;
> + r5blk_t log_start; /* where the io_unit starts */
> + r5blk_t log_end; /* where the io_unit ends */
> + struct list_head log_sibling; /* log->running_ios */
> + struct list_head stripe_list; /* stripes added to the io_unit */
> + int state;
> + wait_queue_head_t wait_state;
> +};
> +
> +/* r5l_io_unit state */
> +enum {
> + IO_UNIT_RUNNING = 0, /* accepting new IO */
> + IO_UNIT_IO_START = 1, /* io_unit is writting to log */
> + IO_UNIT_IO_END = 2, /* io_unit is in log */
> + IO_UNIT_STRIPE_START = 3, /* stripes of io_unit are running */
> + IO_UNIT_STRIPE_END = 4, /* stripes data are in raid disks */
> +};
0, 1, and 3 tell me what is currently happening to the stripe.
4 tells me that nothing is happening any more.
What does '2' tell me exactly? What is happening when state is 2?
Is that state needed?
> +
> +#define PAGE_SECTOR_SHIFT (PAGE_SHIFT - 9)
> +
> +static inline struct block_device *r5l_bdev(struct r5l_log *log)
> +{
> + return log->rdev->bdev;
> +}
> +
> +static inline sector_t r5l_block_to_sector(struct r5l_log *log, r5blk_t block)
> +{
> + return block << log->block_sector_shift;
> +}
> +
> +static inline r5blk_t r5l_sector_to_block(struct r5l_log *log, sector_t s)
> +{
> + return s >> log->block_sector_shift;
> +}
> +
> +static inline int r5l_page_blocks(struct r5l_log *log, int pages)
> +{
> + return pages << log->page_block_shift;
> +}
> +
> +static u32 r5l_calculate_checksum(struct r5l_log *log, u32 crc,
> + void *buf, size_t size)
> +{
> + return crc32_le(crc, buf, size);
> +}
> +
Tiny little inlines and defines like this worry me.
Do they really add useful abstractions, or do they just hide important
detail?
Putting this sort of thing in a header file and making it part of an
interface is quite different from putting them in a .c file and making
the code harder to read (because I keep having to check exactly what
the function does).
If r5l_calculate_checksum copied out the old checksum, stored a zero
there, performed the calculation, then put the old checksum back, then
that might be a useful separate function. But as it is there seems no
point.
PAGE_SECTOR_SHIFT is more typing than PAGE_SHIFT - 9 and isn't any more
clear. And you only use it once where I'm not at all sure that you
should.
> +static r5blk_t r5l_ring_add(struct r5l_log *log, r5blk_t block, int inc)
> +{
> + block += inc;
> + if (block >= log->last_block)
> + block = block - log->total_blocks;
> + return block;
> +}
> +
> +static void r5l_wake_reclaim(struct r5l_log *log, r5blk_t space);
> +static r5blk_t r5l_free_space(struct r5l_log *log)
> +{
> + r5blk_t used_size;
> +
> + if (log->log_start >= log->last_checkpoint)
> + used_size = log->log_start - log->last_checkpoint;
> + else
> + used_size = log->log_start + log->total_blocks -
> + log->last_checkpoint;
> +
> + if (log->total_blocks > used_size + log->reserved_blocks)
> + return log->total_blocks - used_size - log->reserved_blocks;
> + return 0;
Maybe I'm getting confused by terminology, but this looks wrong.
I would think that "last_checkpoint" was something written fairly
recently, and "log_start" was something written a long time ago.
So if the log hasn't wrapped, then the "used_size" would be
last_checkpoint - log_start
while it if had wrapped, it would be
last_checkpoint + total_blocks - log_start
???
> +}
> +
> +/* Make sure we have enough free space in log device */
> +static void r5l_get_reserve(struct r5l_log *log, unsigned int size)
> +{
> + r5blk_t free;
> +
> + BUG_ON(!mutex_is_locked(&log->io_mutex));
> + free = r5l_free_space(log);
> + if (free >= size) {
> + log->reserved_blocks += size;
> + return;
> + }
> +
> + log->reserved_blocks += size;
> + mutex_unlock(&log->io_mutex);
> +
> + r5l_wake_reclaim(log, size);
> +
> + mutex_lock(&log->io_mutex);
> + wait_event_cmd(log->space_waitq, r5l_free_space(log) > 0,
> + mutex_unlock(&log->io_mutex), mutex_lock(&log->io_mutex));
> +}
As I've already said I don't think that blocking when out of space is a
good idea, but couldn't this be much simpler?
log->reserved_blocks += size;
wait_event_cmd(log->space_waitq, r5l_free_space(log) > 0,
mutex_unlock(&log->io_mutex);
r5l_wake_reclaim(log, size),
mutex_lock(&log->io_mutex));
??
> +
> +static void r5l_put_reserve(struct r5l_log *log, unsigned int reserved_blocks)
> +{
> + BUG_ON(!mutex_is_locked(&log->io_mutex));
> +
> + log->reserved_blocks -= reserved_blocks;
> + if (r5l_free_space(log) > 0)
> + wake_up(&log->space_waitq);
> +}
> +
> +static struct r5l_io_unit *r5l_alloc_io_unit(struct r5l_log *log)
> +{
> + struct r5l_io_unit *io;
> + gfp_t gfp = GFP_NOIO | __GFP_NOFAIL;
> +
> + io = kmem_cache_zalloc(log->io_kc, gfp);
> + if (!io)
> + return NULL;
> + io->log = log;
> + io->meta_page = alloc_page(gfp | __GFP_ZERO);
> + if (!io->meta_page) {
> + kmem_cache_free(log->io_kc, io);
> + return NULL;
> + }
This can return NULL, but the one place where you call it you do not
check for NULL.
Maybe a mempool would be appropriate, maybe something else - I haven't
examine the issue closely.
> +
> + bio_list_init(&io->bios);
> + atomic_set(&io->pending_io, 1);
> + INIT_LIST_HEAD(&io->log_sibling);
> + INIT_LIST_HEAD(&io->stripe_list);
> + io->state = IO_UNIT_RUNNING;
> + init_waitqueue_head(&io->wait_state);
> + return io;
> +}
> +
> +static void r5l_free_io_unit(struct r5l_log *log, struct r5l_io_unit *io)
> +{
> + __free_page(io->meta_page);
> + kmem_cache_free(log->io_kc, io);
> +}
> +
> +static void r5l_wait_io_unit_state(struct r5l_io_unit *io, int state)
> +{
> + wait_event(io->wait_state, io->state >= state);
> +}
This is another of those tiny inlines which I think just make the code
harder to read.
> +
> +static void r5l_set_io_unit_state(struct r5l_io_unit *io, int state)
> +{
> + if (io->state >= state)
> + return;
Should this ever actually 'return'?
if (WARN_ON(..)) return; ???
Not sure - I haven't examined closely.
> + io->state = state;
> + wake_up(&io->wait_state);
> +}
> +
> +static void r5l_submit_bio(struct r5l_log *log, int rw, struct bio *bio)
> +{
> + /* all IO must start from rdev->data_offset */
> + bio->bi_iter.bi_sector += log->rdev->data_offset;
> + submit_bio(rw, bio);
> +}
This might almost be worthy of its own function if it was called more
than once. But as it is...
> +
> +static void r5l_io_unit_ioend(struct r5l_io_unit *io, int error)
> +{
> + struct r5l_log *log = io->log;
> + if (!atomic_dec_and_test(&io->pending_io))
> + return;
> +
> + r5l_set_io_unit_state(io, IO_UNIT_IO_END);
> + md_wakeup_thread(log->mddev->thread);
> +}
> +
> +static void r5l_log_endio(struct bio *bio, int error)
> +{
> + struct r5l_io_unit *io = bio->bi_private;
> +
> + bio_put(bio);
> + r5l_io_unit_ioend(io, error);
> +}
> +
> +static void r5l_submit_io(struct r5l_log *log, struct r5l_io_unit *io)
> +{
> + struct bio *bio;
> + while ((bio = bio_list_pop(&io->bios)))
> + r5l_submit_bio(log, WRITE, bio);
> +
> + r5l_io_unit_ioend(io, 0);
> +}
> +
> +static void r5l_submit_current_io(struct r5l_log *log)
> +{
> + struct r5l_io_unit *io = log->current_io;
> + struct r5l_meta_block *block;
> + u32 crc;
> +
> + if (!io)
> + return;
> +
> + block = page_address(io->meta_page);
> + block->meta_size = cpu_to_le32(io->meta_offset);
> + crc = r5l_calculate_checksum(log, log->uuid_checksum,
> + block, log->block_size);
> + block->checksum = cpu_to_le32(crc);
> +
> + log->current_io = NULL;
> + r5l_set_io_unit_state(io, IO_UNIT_IO_START);
> +
> + r5l_submit_io(log, io);
> +}
> +
> +static struct r5l_io_unit *r5l_new_meta(struct r5l_log *log)
> +{
> + struct r5l_io_unit *io;
> + struct r5l_meta_block *block;
> + struct bio *bio;
> +
> + io = r5l_alloc_io_unit(log);
> +
> + block = page_address(io->meta_page);
> + block->magic = cpu_to_le32(R5LOG_MAGIC);
> + block->version = R5LOG_VERSION;
> + block->block_size = cpu_to_le16(log->block_size);
> + block->seq = cpu_to_le64(log->seq);
> + block->position = cpu_to_le64(log->log_start);
> +
> + io->log_start = log->log_start;
> + io->meta_offset = sizeof(struct r5l_meta_block);
> + io->seq = log->seq;
> +
> + bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL,
> + bio_get_nr_vecs(r5l_bdev(log)));
bio_alloc uses a mempool so __GFP_NOFAIL is not relevant.
Maybe this should use the mddev's mempool?
> + io->current_bio = bio;
> + bio->bi_rw = WRITE;
> + bio->bi_bdev = r5l_bdev(log);
> + bio->bi_iter.bi_sector = r5l_block_to_sector(log, log->log_start);
> + bio_add_page(bio, io->meta_page, log->block_size, 0);
> + bio->bi_end_io = r5l_log_endio;
> + bio->bi_private = io;
> +
> + bio_list_add(&io->bios, bio);
> + atomic_inc(&io->pending_io);
> +
> + log->seq++;
> + log->log_start = r5l_ring_add(log, log->log_start, 1);
> + io->log_end = log->log_start;
> + /* current bio hit disk end */
> + if (log->log_start == log->first_block)
> + io->current_bio = NULL;
> +
> + spin_lock(&log->io_list_lock);
> + list_add_tail(&io->log_sibling, &log->running_ios);
> + spin_unlock(&log->io_list_lock);
> +
> + return io;
> +}
> +
> +static int r5l_get_meta(struct r5l_log *log, unsigned int payload_size)
> +{
> + struct r5l_io_unit *io;
> +
> + io = log->current_io;
> + if (io && io->meta_offset + payload_size > log->block_size)
> + r5l_submit_current_io(log);
> + io = log->current_io;
> + if (io)
> + return 0;
> +
> + log->current_io = r5l_new_meta(log);
> + return 0;
> +}
> +
> +static void r5l_log_pages(struct r5l_log *log, u16 type, sector_t location,
> + struct page *page1, u32 checksum1,
> + struct page *page2, u32 checksum2)
> +{
> + struct r5l_io_unit *io = log->current_io;
> + struct r5l_payload_data_parity *payload;
> +
> + payload = page_address(io->meta_page) + io->meta_offset;
> + payload->header.type = cpu_to_le16(type);
> + payload->header.flags = cpu_to_le16(0);
> + payload->blocks = cpu_to_le32(r5l_page_blocks(log, 1 + !!page2));
> + payload->location = cpu_to_le64(location);
> + payload->checksum[0] = cpu_to_le32(checksum1);
> + if (page2)
> + payload->checksum[1] = cpu_to_le32(checksum2);
> +
> +alloc_bio:
> + if (!io->current_bio) {
> + struct bio *bio;
> + bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL,
> + bio_get_nr_vecs(r5l_bdev(log)));
ditto - __GFP_NOFAIL not appropriate.
> + bio->bi_rw = WRITE;
> + bio->bi_bdev = r5l_bdev(log);
> + bio->bi_iter.bi_sector = r5l_block_to_sector(log, log->log_start);
> + bio->bi_end_io = r5l_log_endio;
> + bio->bi_private = io;
> + bio_list_add(&io->bios, bio);
> + atomic_inc(&io->pending_io);
> + io->current_bio = bio;
> + }
> + if (page1) {
> + if (!bio_add_page(io->current_bio, page1, PAGE_SIZE, 0)) {
> + io->current_bio = NULL;
> + goto alloc_bio;
> + }
> + log->log_start = r5l_ring_add(log, log->log_start,
> + r5l_page_blocks(log, 1));
> + /* current bio hit disk end */
> + if (log->log_start == log->first_block)
> + io->current_bio = NULL;
> + }
> +
> + page1 = NULL;
> + if (page2) {
> + if (io->current_bio == NULL)
> + goto alloc_bio;
> + if (!bio_add_page(io->current_bio, page2, PAGE_SIZE, 0)) {
> + io->current_bio = NULL;
> + goto alloc_bio;
> + }
> + log->log_start = r5l_ring_add(log, log->log_start,
> + r5l_page_blocks(log, 1));
> + /* current bio hit disk end */
> + if (log->log_start == log->first_block)
> + io->current_bio = NULL;
> + }
> +
> + io->meta_offset += sizeof(struct r5l_payload_data_parity) +
> + sizeof(__le32) * (1 + !!page2);
> + io->log_end = log->log_start;
> +}
Allowing either 1 or 2 pages to be passed to this function makes it
more complex than I would like.
It would be much nicer if you could just call r5l_log_pages() twice for
the P and Q blocks.
> +
> +static void r5l_log_stripe(struct r5l_log *log, struct stripe_head *sh)
> +{
> + int i;
> + int meta_size;
> + int write_disks = 0;
> + int data_pages, parity_pages;
> + struct r5l_io_unit *io;
> + int reserve;
> +
> + for (i = 0; i < sh->disks; i++) {
> + if (!test_bit(R5_Wantwrite, &sh->dev[i].flags))
> + continue;
> + write_disks++;
> + }
> + parity_pages = 1 + !!(sh->qd_idx >= 0);
> + data_pages = write_disks - parity_pages;
> +
> + meta_size = (sizeof(struct r5l_payload_data_parity) + sizeof(__le32)) *
> + data_pages + sizeof(struct r5l_payload_data_parity) +
> + sizeof(__le32) * parity_pages;
In quite a lot of places your indentation is not correct.
When you have any sort of bracket/brace/parenthesis, and
the opening bracket is not at the end of the line, then everything
within the brackets must be immediately to the right of the opening
bracket.
I'm happy to fix most of this up when I ultimately accept that patch
(emacs makes it easy). However this line is just wrong. A line break
must be at a low-precedence point in the line. So e.g. break after '+'
rather than after '*' etc.
This should be more like:
meta_size = (sizeof(struct r5l_payload_data_parity) + sizeof(__le32))
* data_pages +
sizeof(struct r5l_payload_data_parity) +
sizeof(__le32) * parity_pages;
which suddenly I can actually read and understand....
Of course if you didn't combine the two P+Q blocks as suggested
earlier, this would become even simpler.
> +
> + /* meta + data */
> + reserve = 1 + r5l_page_blocks(log, write_disks);
> + r5l_get_reserve(log, reserve);
> +
> + r5l_get_meta(log, meta_size);
> + io = log->current_io;
> +
> + for (i = 0; i < sh->disks; i++) {
> + if (!test_bit(R5_Wantwrite, &sh->dev[i].flags))
> + continue;
> + if (i == sh->pd_idx || i == sh->qd_idx)
> + continue;
> + r5l_log_pages(log, R5LOG_PAYLOAD_DATA,
> + compute_blocknr(sh, i, 0),
> + sh->dev[i].page, sh->dev[i].log_checksum,
> + NULL, 0);
> + }
> + r5l_log_pages(log, R5LOG_PAYLOAD_PARITY,
> + sh->sector, sh->dev[sh->pd_idx].page,
> + sh->dev[sh->pd_idx].log_checksum,
> + sh->qd_idx >= 0 ? sh->dev[sh->qd_idx].page : NULL,
> + sh->qd_idx >= 0 ? sh->dev[sh->qd_idx].log_checksum : 0);
> +
> + list_add_tail(&sh->log_list, &io->stripe_list);
> + atomic_inc(&io->pending_stripe);
> + sh->log_io = io;
> +
> + r5l_put_reserve(log, reserve);
> +}
> +
> +static void r5l_log_thread(struct md_thread *thread)
> +{
> + struct mddev *mddev = thread->mddev;
> + struct r5conf *conf = mddev->private;
> + struct r5l_log *log = conf->log;
> + struct stripe_head *sh;
> + LIST_HEAD(list);
> + struct blk_plug plug;
> +
> + if (!log)
> + return;
> +
> + spin_lock(&log->log_stripes_lock);
> + list_splice_init(&log->log_stripes, &list);
> + spin_unlock(&log->log_stripes_lock);
> +
> + if (list_empty(&list))
> + return;
> + mutex_lock(&log->io_mutex);
> + blk_start_plug(&plug);
> + while (!list_empty(&list)) {
> + sh = list_first_entry(&list, struct stripe_head, log_list);
> + list_del_init(&sh->log_list);
> + r5l_log_stripe(log, sh);
> + }
> + r5l_submit_current_io(log);
> + blk_finish_plug(&plug);
> + mutex_unlock(&log->io_mutex);
> +}
> +
> +/*
> + * running in raid5d, where reclaim could wait for raid5d too (when it flushes
> + * data from log to raid disks), so we shouldn't wait for reclaim here
> + * */
> +int r5l_write_stripe(struct r5l_log *log, struct stripe_head *sh)
> +{
> + int write_disks = 0;
> + int data_pages, parity_pages;
> + int meta_size;
> + int i;
> +
> + if (!log)
> + return -EAGAIN;
> + /* Don't support stripe batch */
> + if (sh->log_io ||!test_bit(R5_Wantwrite, &sh->dev[sh->pd_idx].flags) ||
> + test_bit(STRIPE_SYNCING, &sh->state))
> + return -EAGAIN;
> +
> + for (i = 0; i < sh->disks; i++) {
> + void *addr;
> + if (!test_bit(R5_Wantwrite, &sh->dev[i].flags))
> + continue;
> + write_disks++;
> + addr = kmap_atomic(sh->dev[i].page);
> + sh->dev[i].log_checksum = r5l_calculate_checksum(log,
> + log->uuid_checksum, addr, PAGE_SIZE);
> + kunmap_atomic(addr);
> + }
> + parity_pages = 1 + !!(sh->qd_idx >= 0);
> + data_pages = write_disks - parity_pages;
> +
> + meta_size = (sizeof(struct r5l_payload_data_parity) + sizeof(__le32)) *
> + data_pages + sizeof(struct r5l_payload_data_parity) +
> + sizeof(__le32) * parity_pages;
> + /* Doesn't work with very big raid array */
> + if (meta_size + sizeof(struct r5l_meta_block) >
> + log->block_size)
> + return -EINVAL;
> +
> + atomic_inc(&sh->count);
> +
> + spin_lock(&log->log_stripes_lock);
> + list_add_tail(&sh->log_list, &log->log_stripes);
> + spin_unlock(&log->log_stripes_lock);
> + return 0;
> +}
> +
> +void r5l_write_stripe_run(struct r5l_log *log)
> +{
> + if (!log)
> + return;
> + md_wakeup_thread(log->log_thread);
> +}
if (log)
md_wakeup_thread(log->log_thread);
??
Does it need a separate function?
> +
> +static void r5l_wake_reclaim(struct r5l_log *log, r5blk_t space)
> +{
> +}
> +
A comment that this will be fleshed out it subsequent patch wouldn't
hurt, but isn't entirely necessary.
> +static int r5l_recovery_log(struct r5l_log *log)
> +{
> + /* fake recovery */
> + log->seq = log->last_cp_seq + 1;
> + log->log_start = r5l_ring_add(log, log->last_checkpoint, 1);
> + return 0;
> +}
> +
> +static void r5l_write_super(struct r5l_log *log, sector_t cp)
> +{
> + log->rdev->recovery_offset = cp;
> + md_update_sb(log->mddev, 1);
> +}
This is only called from run() when the log is first initialised. At
this point there is nothing useful in the log, so recording it's
location is pointless. At most you could set the MD_SB_DIRTY flag (or
whatever it is).
So it really doesn't need to be a separate function.
> +
> +static int r5l_load_log(struct r5l_log *log)
> +{
> + struct md_rdev *rdev = log->rdev;
> + struct page *page;
> + struct r5l_meta_block *mb;
> + sector_t cp = log->rdev->recovery_offset;
> + u32 stored_crc, expected_crc;
> + bool create_super = false;
> + int ret;
> +
> + /* Make sure it's valid */
> + if (cp >= rdev->sectors)
> + cp = 0;
> + page = alloc_page(GFP_KERNEL);
> + if (!page)
> + return -ENOMEM;
> +
> + if (!sync_page_io(rdev, cp, PAGE_SIZE, page, READ, false)) {
> + ret = -EIO;
> + goto ioerr;
> + }
> + mb = page_address(page);
> +
> + if (le32_to_cpu(mb->magic) != R5LOG_MAGIC ||
> + mb->version != R5LOG_VERSION ||
> + le16_to_cpu(mb->block_size) > PAGE_SIZE) {
> + create_super = true;
> + goto create;
> + }
> + stored_crc = le32_to_cpu(mb->checksum);
> + mb->checksum = 0;
> + expected_crc = r5l_calculate_checksum(log, log->uuid_checksum,
> + mb, le16_to_cpu(mb->block_size));
> + if (stored_crc != expected_crc) {
> + create_super = true;
> + goto create;
> + }
> + if (le64_to_cpu(mb->position) * (le16_to_cpu(mb->block_size) >> 9) !=
> + cp) {
> + create_super = true;
> + goto create;
> + }
> +create:
> + if (create_super) {
> + log->block_size = PAGE_SIZE;
> + log->last_cp_seq = prandom_u32();
> + cp = (cp >> PAGE_SECTOR_SHIFT) << PAGE_SECTOR_SHIFT;
This isn't the way we normally round down - we have the "round_down()"
macro for that.
But if you are creating a new log, why not just set 'cp' to zero??
> + /* Make sure super points to correct address */
> + r5l_write_super(log, cp);
> + } else {
> + log->block_size = le16_to_cpu(mb->block_size);
> + log->last_cp_seq = le64_to_cpu(mb->seq);
> + }
> + log->block_sector_shift = ilog2(log->block_size >> 9);
> + log->page_block_shift = PAGE_SHIFT - ilog2(log->block_size);
> +
> + log->first_block = 0;
> + log->total_blocks = r5l_sector_to_block(log, rdev->sectors);
> + log->last_block = log->first_block + log->total_blocks;
> + log->last_checkpoint = r5l_sector_to_block(log, cp);
> +
> + __free_page(page);
> +
> + return r5l_recovery_log(log);
> +ioerr:
> + __free_page(page);
> + return ret;
> +}
> +
> +int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
> +{
> + struct r5l_log *log;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL);
> + if (!log)
> + return -ENOMEM;
> + log->mddev = rdev->mddev;
> + log->rdev = rdev;
> +
> + log->uuid_checksum = r5l_calculate_checksum(log, ~0, rdev->mddev->uuid,
> + sizeof(rdev->mddev->uuid));
> +
> + init_waitqueue_head(&log->space_waitq);
> + mutex_init(&log->io_mutex);
> +
> + spin_lock_init(&log->io_list_lock);
> + INIT_LIST_HEAD(&log->running_ios);
> +
> + log->io_kc = KMEM_CACHE(r5l_io_unit, 0);
> + if (!log->io_kc)
> + goto io_kc;
> +
> + INIT_LIST_HEAD(&log->log_stripes);
> + spin_lock_init(&log->log_stripes_lock);
> + log->log_thread = md_register_thread(r5l_log_thread,
> + log->mddev, "log");
> + if (!log->log_thread)
> + goto log_thread;
> +
> + if (r5l_load_log(log))
> + goto error;
> +
> + conf->log = log;
> + return 0;
> +error:
> + md_unregister_thread(&log->log_thread);
> +log_thread:
> + kmem_cache_destroy(log->io_kc);
> +io_kc:
> + kfree(log);
> + return -EINVAL;
> +}
> +
> +void r5l_exit_log(struct r5l_log *log)
> +{
> + md_unregister_thread(&log->log_thread);
> +
> + kmem_cache_destroy(log->io_kc);
> + kfree(log);
> +}
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 59e44e9..9608a44 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -899,6 +899,8 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
>
> might_sleep();
>
> + if (!r5l_write_stripe(conf->log, sh))
> + return;
If no log is configured, r5l_write_stripe will return -EAGAIN, and so
ops_run_io will never submit any IO....
> for (i = disks; i--; ) {
> int rw;
> int replace_only = 0;
> @@ -2478,8 +2480,6 @@ static void raid5_end_write_request(struct bio *bi, int error)
> release_stripe(sh->batch_head);
> }
>
> -static sector_t compute_blocknr(struct stripe_head *sh, int i, int previous);
> -
> static void raid5_build_block(struct stripe_head *sh, int i, int previous)
> {
> struct r5dev *dev = &sh->dev[i];
> @@ -2729,7 +2729,7 @@ static sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
> return new_sector;
> }
>
> -static sector_t compute_blocknr(struct stripe_head *sh, int i, int previous)
> +sector_t compute_blocknr(struct stripe_head *sh, int i, int previous)
> {
> struct r5conf *conf = sh->raid_conf;
> int raid_disks = sh->disks;
> @@ -3498,6 +3498,7 @@ static void handle_stripe_clean_event(struct r5conf *conf,
> WARN_ON(test_bit(R5_SkipCopy, &dev->flags));
> WARN_ON(dev->page != dev->orig_page);
> }
> +
> if (!discard_pending &&
> test_bit(R5_Discard, &sh->dev[sh->pd_idx].flags)) {
> clear_bit(R5_Discard, &sh->dev[sh->pd_idx].flags);
> @@ -5746,6 +5747,7 @@ static int handle_active_stripes(struct r5conf *conf, int group,
>
> for (i = 0; i < batch_size; i++)
> handle_stripe(batch[i]);
> + r5l_write_stripe_run(conf->log);
>
> cond_resched();
>
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 02c3bf8..a8daf39 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -223,6 +223,9 @@ struct stripe_head {
> struct stripe_head *batch_head; /* protected by stripe lock */
> spinlock_t batch_lock; /* only header's lock is useful */
> struct list_head batch_list; /* protected by head's batch lock*/
> +
> + struct r5l_io_unit *log_io;
> + struct list_head log_list;
> /**
> * struct stripe_operations
> * @target - STRIPE_OP_COMPUTE_BLK target
I wonder if we really need yet another 'list_head' in 'stripe_head'.
I guess one more is no great cost.
> @@ -244,6 +247,7 @@ struct stripe_head {
> struct bio *toread, *read, *towrite, *written;
> sector_t sector; /* sector of this page */
> unsigned long flags;
> + u32 log_checksum;
> } dev[1]; /* allocated with extra space depending of RAID geometry */
> };
>
> @@ -539,6 +543,7 @@ struct r5conf {
> struct r5worker_group *worker_groups;
> int group_cnt;
> int worker_cnt_per_group;
> + struct r5l_log *log;
> };
>
>
> @@ -605,4 +610,9 @@ static inline int algorithm_is_DDF(int layout)
>
> extern void md_raid5_kick_device(struct r5conf *conf);
> extern int raid5_set_cache_size(struct mddev *mddev, int size);
> +extern sector_t compute_blocknr(struct stripe_head *sh, int i, int previous);
When making static functions extern, you need to make sure the module
name is mentioned somehow. So raid5_compute_blocknr, or r5_.. or
md_...
> +extern int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev);
> +extern void r5l_exit_log(struct r5l_log *log);
> +extern int r5l_write_stripe(struct r5l_log *log, struct stripe_head *head_sh);
> +extern void r5l_write_stripe_run(struct r5l_log *log);
> #endif
> diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
> index 8c8e12c..418b1ba 100644
> --- a/include/uapi/linux/raid/md_p.h
> +++ b/include/uapi/linux/raid/md_p.h
> @@ -315,4 +315,52 @@ struct mdp_superblock_1 {
> |MD_FEATURE_WRITE_CACHE \
> )
>
> +struct r5l_payload_header {
> + __le16 type;
> + __le16 flags;
> +} __attribute__ ((__packed__));
> +
> +enum {
> + R5LOG_PAYLOAD_DATA = 0,
> + R5LOG_PAYLOAD_PARITY = 1,
> + R5LOG_PAYLOAD_FLUSH = 2,
> +};
I would really like it if this was
enum r5l_payload_type {
so I knew immediately where the numbers would appear.
Then have the two different 'flags' enums immediately afterwards also
with useful names.
> +
> +struct r5l_payload_data_parity {
> + struct r5l_payload_header header;
> + __le32 blocks; /* block. data/parity size. each 4k has a checksum */
> + __le64 location; /* sector. For data, it's raid sector. For
> + parity, it's stripe sector */
> + __le32 checksum[];
> +} __attribute__ ((__packed__));
> +
> +enum {
> + R5LOG_PAYLOAD_FLAG_DISCARD = 1,
> +};
> +
> +struct r5l_payload_flush {
> + struct r5l_payload_header header;
> + __le32 size; /* flush_stripes size, bytes */
> + __le64 flush_stripes[];
> +} __attribute__ ((__packed__));
> +
> +enum {
> + R5LOG_PAYLOAD_FLAG_FLUSH_STRIPE = 1, /* data represents whole stripe */
> +};
> +
> +struct r5l_meta_block {
> + __le32 magic;
> + __le32 checksum;
> + __u8 version;
> + __u8 __zero_pading;
> + __le16 block_size; /* 512B - 4k */
> + __le32 meta_size; /* whole size of the block */
> +
> + __le64 seq;
> + __le64 position; /* block, start from rdev->data_offset, current position */
> + struct r5l_payload_header payloads[];
> +} __attribute__ ((__packed__));
> +
> +#define R5LOG_VERSION 0x1
> +#define R5LOG_MAGIC 0x6433c509
> #endif
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH 2/9] md: override md superblock recovery_offset for cache device
From: NeilBrown @ 2015-08-05 1:08 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <42ac39ae4466dda63b1dee827c2bd3741d669860.1438215986.git.shli@fb.com>
On Wed, 29 Jul 2015 17:38:42 -0700 Shaohua Li <shli@fb.com> wrote:
> Cache device stores data in a log structure. We need record the log
> start. Here we override md superblock recovery_offset for this purpose.
> This field of a cache device is meaningless otherwise.
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/md.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index fd84f16..9861f34 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1664,6 +1664,7 @@ static int super_1_validate(struct mddev *mddev, struct md_rdev *rdev)
> "cache feature, ignoring the device\n");
> return -EINVAL;
> }
> + rdev->recovery_offset = le64_to_cpu(sb->recovery_offset);
> set_bit(WriteCache, &rdev->flags);
> break;
> default:
> @@ -1830,6 +1831,9 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)
> sb->dev_roles[i] = cpu_to_le16(0xffff);
> }
>
> + if (test_bit(WriteCache, &rdev->flags))
> + sb->recovery_offset = cpu_to_le64(rdev->recovery_offset);
> +
I would much rather this was a little earlier in the function,
near where recovery_offset is set for non-InSync devices.
i.e. immediately after:
if (rdev->raid_disk >= 0 &&
!test_bit(In_sync, &rdev->flags)) {
sb->feature_map |=
cpu_to_le32(MD_FEATURE_RECOVERY_OFFSET);
sb->recovery_offset =
cpu_to_le64(rdev->recovery_offset);
if (rdev->saved_raid_disk >= 0 && mddev->bitmap)
sb->feature_map |=
cpu_to_le32(MD_FEATURE_RECOVERY_BITMAP);
}
Maybe we could always copy recovery_offset from rdev to sb, and then
just set whichever feature flags are relevant - if any. Or maybe not.
Thanks,
NeilBrown
> sb->sb_csum = calc_sb_1_csum(sb);
> }
>
^ permalink raw reply
* Re: [PATCH 1/9] MD: add a new disk role to present cache device
From: NeilBrown @ 2015-08-05 1:05 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <e871cc4fc266c76f1d4b55225f2c0a6aacb18680.1438215986.git.shli@fb.com>
On Wed, 29 Jul 2015 17:38:41 -0700 Shaohua Li <shli@fb.com> wrote:
> From: Song Liu <songliubraving@fb.com>
>
> Next patches will use a disk as raid5/6 caching. We need a new disk role
> to present the cache device and add MD_FEATURE_WRITE_CACHE to
> feature_map for backward compability.
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/md.c | 24 ++++++++++++++++++++++--
> drivers/md/md.h | 4 ++++
> include/uapi/linux/raid/md_p.h | 3 +++
> 3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d429c30..fd84f16 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1656,6 +1656,16 @@ static int super_1_validate(struct mddev *mddev, struct md_rdev *rdev)
> case 0xfffe: /* faulty */
> set_bit(Faulty, &rdev->flags);
> break;
> + case 0xfffd: /* cache device */
> + if (!(sb->feature_map & MD_FEATURE_WRITE_CACHE)) {
> + /* cache device without cache feature */
> + printk(KERN_WARNING
> + "md: cache device provided without write "
> + "cache feature, ignoring the device\n");
> + return -EINVAL;
> + }
> + set_bit(WriteCache, &rdev->flags);
> + break;
> default:
> rdev->saved_raid_disk = role;
> if ((le32_to_cpu(sb->feature_map) &
> @@ -1811,7 +1821,10 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)
> sb->dev_roles[i] = cpu_to_le16(0xfffe);
> else if (test_bit(In_sync, &rdev2->flags))
> sb->dev_roles[i] = cpu_to_le16(rdev2->raid_disk);
> - else if (rdev2->raid_disk >= 0)
> + else if (test_bit(WriteCache, &rdev2->flags)) {
> + sb->dev_roles[i] = cpu_to_le16(0xfffd);
> + sb->feature_map |= cpu_to_le32(MD_FEATURE_WRITE_CACHE);
> + } else if (rdev2->raid_disk >= 0)
> sb->dev_roles[i] = cpu_to_le16(rdev2->raid_disk);
> else
> sb->dev_roles[i] = cpu_to_le16(0xffff);
> @@ -5803,7 +5816,8 @@ static int get_disk_info(struct mddev *mddev, void __user * arg)
> else if (test_bit(In_sync, &rdev->flags)) {
> info.state |= (1<<MD_DISK_ACTIVE);
> info.state |= (1<<MD_DISK_SYNC);
> - }
> + } else if (test_bit(WriteCache, &rdev->flags))
> + info.state |= (1<<MD_DISK_WRITECACHE);
> if (test_bit(WriteMostly, &rdev->flags))
> info.state |= (1<<MD_DISK_WRITEMOSTLY);
> } else {
> @@ -5918,6 +5932,8 @@ static int add_new_disk(struct mddev *mddev, mdu_disk_info_t *info)
> else
> clear_bit(WriteMostly, &rdev->flags);
>
> + if (info->state & (1<<MD_DISK_WRITECACHE))
> + set_bit(WriteCache, &rdev->flags);
> /*
> * check whether the device shows up in other nodes
> */
> @@ -7286,6 +7302,10 @@ static int md_seq_show(struct seq_file *seq, void *v)
> seq_printf(seq, "(F)");
> continue;
> }
> + if (test_bit(WriteCache, &rdev->flags)) {
> + seq_printf(seq, "(C)");
> + continue;
> + }
> if (rdev->raid_disk < 0)
> seq_printf(seq, "(S)"); /* spare */
> if (test_bit(Replacement, &rdev->flags))
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 7da6e9c..a9f27db 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -176,6 +176,10 @@ enum flag_bits {
> * This device is seen locally but not
> * by the whole cluster
> */
> + WriteCache, /* This device is used as write cache.
> + * Usually, this device should be faster
> + * than other devices in the array
> + */
> };
>
> #define BB_LEN_MASK (0x00000000000001FFULL)
> diff --git a/include/uapi/linux/raid/md_p.h b/include/uapi/linux/raid/md_p.h
> index 2ae6131..8c8e12c 100644
> --- a/include/uapi/linux/raid/md_p.h
> +++ b/include/uapi/linux/raid/md_p.h
> @@ -89,6 +89,7 @@
> * read requests will only be sent here in
> * dire need
> */
> +#define MD_DISK_WRITECACHE 18 /* disk is used as the write cache in RAID-5/6 */
>
> typedef struct mdp_device_descriptor_s {
> __u32 number; /* 0 Device number in the entire set */
> @@ -302,6 +303,7 @@ struct mdp_superblock_1 {
> #define MD_FEATURE_RECOVERY_BITMAP 128 /* recovery that is happening
> * is guided by bitmap.
> */
> +#define MD_FEATURE_WRITE_CACHE 256 /* support write cache */
> #define MD_FEATURE_ALL (MD_FEATURE_BITMAP_OFFSET \
> |MD_FEATURE_RECOVERY_OFFSET \
> |MD_FEATURE_RESHAPE_ACTIVE \
> @@ -310,6 +312,7 @@ struct mdp_superblock_1 {
> |MD_FEATURE_RESHAPE_BACKWARDS \
> |MD_FEATURE_NEW_OFFSET \
> |MD_FEATURE_RECOVERY_BITMAP \
> + |MD_FEATURE_WRITE_CACHE \
> )
>
> #endif
That last line is technically a bit premature. Once you add
MD_FEATURE_WRITE_CACHE to MD_FEATURE_ALL, super_1_load will start
accepting metadata with that bit set. We should really leave that
until the code is really ready to deal with it.
Otherwise this patch is OK.
Thanks,
NeilBrown
^ 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