* Re: Fault tolerance with badblocks
From: Nix @ 2017-05-09 9:58 UTC (permalink / raw)
To: David Brown; +Cc: Wols Lists, Ravi (Tom) Hale, linux-raid
In-Reply-To: <591171BD.3060707@hesbynett.no>
On 9 May 2017, David Brown spake thusly:
> On 08/05/17 16:50, Nix wrote:
>
>> I wonder... scrubbing is not very useful with md, particularly with RAID
>> 6, because it does no writes unless something mismatches, and on failure
>> there is no attempt to determine which of the N disks is bad and rewrite
>> its contents from the other devices (nor, as I understand it, does it
>> clearly say which drive gave the error, so even failing it out and
>> resyncing it is hard).
>
> Please read Neil Brown's article on this: "Smart or simple RAID
> recovery?" <http://neil.brown.name/blog/20100211050355>
I have. THe simple recovery is too simple. So you have a 40TiB RAID-6
array, say, and mismatch_cnt is consistently >0, but a low value, on
scrub. What can you do? The drive is probably not faulty or you'd have
many more mismatches from persistent misdirected reads or writes. md
doesn't repair the corruption, even though on RAID-6 it could. It
doesn't tell you which disk disagreed so you can fail it out. It doesn't
even tell you where the disagreement was so you can try to rebuild it by
hand. What on earth are you supposed to do in this case? Wipe the entire
array and restore from backup? For a *single* sector?
Right now I'm doing scrubs and ignoring the mismatch_cnt, because all it
can do is increase my worry level to no gain at all. I could just as
well do a dd over /dev/md*. It would have the same effect (only without
md's progress feedback and bandwidth throttling. You get progress
feedback, but you don't get told where errors are found?!)
> When the disk is asked to read a block, it pulls up the data and the ECC
> bits, and uses this to check and re-construct the 4K of data, and a
> measure of how many errors were corrected. On modern high-capacity
> drives, it is normal that some errors are corrected on a read. But if
> more than a certain level occur, then the firmware will trigger a
> re-write automatically to the same sector. This will then be re-read.
> If the error rate is low, fine. If it is high, then the sector will be
> remapped by the disk.
>
> So simply /reading/ the data, as far as the processor is concerned, will
> cause re-writes as and when needed.
Last time I asked a disk manufacturer about this, they said oh no we
never correct on read, we can't: if we needed to correct on read, the
data would already be unreadable: you have to trigger a write to get
sparing. Nice to see the drive firmware has improved in the last few
years... but one wonders how many disks actually *do* this. It's hard to
tell because sector sparing is so quiet: it's not always even reflected
in the SMART data, AIUI.
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Nix @ 2017-05-09 9:53 UTC (permalink / raw)
To: Anthony Youngman; +Cc: Phil Turmel, Ravi (Tom) Hale, linux-raid
In-Reply-To: <c5307694-034c-b610-8a27-3bf272cac380@youngman.org.uk>
On 8 May 2017, Anthony Youngman told this:
> If the scrub finds a mismatch, then the drives are reporting
> "everything's fine here". Something's gone wrong, but the question is
> what? If you've got a four-drive raid that reports a mismatch, how do
> you know which of the four drives is corrupt? Doing an auto-correct
> here risks doing even more damage. (I think a raid-6 could recover,
> but raid-5 is toast ...)
With a RAID-5 you are screwed: you can reconstruct the parity but cannot
tell if it was actually right. You can make things consistent, but not
correct.
But with a RAID-6 you *do* have enough data to make things correct, with
precisely the same probability as recovery of a RAID-5 "drive" of length
a single sector. It seems wrong that not only does md not do this but
doesn't even tell you which drive made the mistake so you could do the
millions-of-times-slower process of a manual fail and readdition of the
drive (or, if you suspect it of being wholly buggered, a manual fail and
replacement).
> And seeing as drives are pretty much guaranteed (unless something's
> gone BADLY wrong) to either (a) accurately return the data written, or
> (b) return a read error, that means a data mismatch indicates
> something is seriously wrong that is NOTHING to do with the drives.
This turns out not to be the case. See this ten-year-old paper:
<https://indico.cern.ch/event/13797/contributions/1362288/attachments/115080/163419/Data_integrity_v3.pdf>.
Five weeks of doing 2GiB writes on 3000 nodes once every two hours
found, they estimated, 50 errors possibly attributable to disk problems
(sector- or page-size regions of corrupted data) on 1/30th of their
nodes. This is *not* rare and it is hard to imagine that 1/30th of disks
used by CERN deserve discarding. It is better to assume that drives
misdirect writes now and then, and to provide a means of recovering from
them that does not take days of panic. RAID-6 gives you that means: md
should use it.
The page-sized regions of corrupted data were probably software -- but
the sector-sized regions were just as likely the drives, possibly
misdirected writes or misdirected reads.
Neil decided not to do any repair work in this case on the grounds that
if the drive is misdirecting one write it might misdirect the repair as
well -- but if the repair is *consistently* misdirected, that seems
relatively harmless (you had corruption before, you have it now, it just
moved), and if it was a sporadic error, the repair is worthwhile. The
only case in which a repair should not be attempted is if the drive is
misdirecting all or most writes -- but in that case, by the time you do
a scrub, on all but the quietest arrays you'll see millions of
mismatches and it'll be obvious that it's time to throw the drive out.
(Assuming md told you which drive it was.)
>> If a sector weakens purely because of neighbouring writes or temperature
>> or a vibrating housing or something (i.e. not because of actual damage),
>> so that a rewrite will strengthen it and relocation was never necessary,
>> surely you've just saved a pointless bit of sector sparing? (I don't
>> know: I'm not sure what the relative frequency of these things is. Read
>> and write errors in general are so rare that it's quite possible I'm
>> worrying about nothing at all. I do know I forgot to scrub my old
>> hardware RAID array for about three years and nothing bad happened...)
>>
> Yes you have saved a sector sparing. Note that a consumer 3TB drive
> can return, on average, one error every time it's read from end to end
> 3 times, and still be considered "within spec" ie "not faulty" by the
Yeah, that's why RAID-6 is a good idea. :)
> manufacturer. And that's a *brand* *new* drive. That's why building a
> large array using consumer drives is a stupid idea - 4 x 3TB drives
> and a *within* *spec* array must expect to handle at least one error
> every scrub.
That's just one reason why. The lack of control over URE timeouts is
just as bad.
> Okay - most drives are actually way over spec, and could probably be
> read end-to-end many times without a single error, but you'd be a fool
> to gamble on it.
I'm trying *not* to gamble on it -- but I don't want to end up in the
current situation we seem to have with md6, which is "oh, you have a
mismatch, it's not going away, but we're neither going to tell you where
it is nor what disk it's on nor repair it ourselves, even though we
could, just to make it as hard as possible for you to repair the problem
or even tell if it's a consistent one" (is the single mismatch an
expected, spurious read error because of the volume of data you're
reading, or one that's consistent and needs repair? All mismatch_cnt
tells you is that there's a mismatch).
--
NULL && (void)
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: David Brown @ 2017-05-09 7:37 UTC (permalink / raw)
To: Nix, Wols Lists; +Cc: Ravi (Tom) Hale, linux-raid
In-Reply-To: <87h90v8kt3.fsf@esperi.org.uk>
On 08/05/17 16:50, Nix wrote:
>
> I wonder... scrubbing is not very useful with md, particularly with RAID
> 6, because it does no writes unless something mismatches, and on failure
> there is no attempt to determine which of the N disks is bad and rewrite
> its contents from the other devices (nor, as I understand it, does it
> clearly say which drive gave the error, so even failing it out and
> resyncing it is hard).
>
Please read Neil Brown's article on this: "Smart or simple RAID
recovery?" <http://neil.brown.name/blog/20100211050355>
> If there was a way to get md to *rewrite* everything during scrub,
> rather than just checking, this might help (in addition to letting the
> drive refresh the magnetization of absolutely everything). "repair" mode
> appears to do no writes until an error is found, whereupon (on RAID 6)
> it proceeds to make a "repair" that is more likely than not to overwrite
> good data with bad. Optionally writing what's already there on non-error
> seems like it might be a worthwhile (and fairly simple) change.
>
Scrubbing /does/ rewrite disk blocks - when necessary. It does not do
it explicitly, but the disks handle this themselves.
To the processor, a disk block is 4K of data. But to the disk and its
controllers, it is 4K plus a sizeable amount of error checking and
correcting bits. Some are spread out within the block, some are
collected together at the end of the block. The ECC system can handle a
large number of failed bits, either in lumps caused by a physical defect
on the disk surface, or spread out due to the slow decay of the magnetic
orientation, or hits by cosmic rays.
When the disk is asked to read a block, it pulls up the data and the ECC
bits, and uses this to check and re-construct the 4K of data, and a
measure of how many errors were corrected. On modern high-capacity
drives, it is normal that some errors are corrected on a read. But if
more than a certain level occur, then the firmware will trigger a
re-write automatically to the same sector. This will then be re-read.
If the error rate is low, fine. If it is high, then the sector will be
remapped by the disk.
So simply /reading/ the data, as far as the processor is concerned, will
cause re-writes as and when needed.
^ permalink raw reply
* Re: [PATCH 25/27] block: remove the discard_zeroes_data flag
From: Nicholas A. Bellinger @ 2017-05-09 6:46 UTC (permalink / raw)
To: hch-jcswGhMUV9g@public.gmane.org
Cc: axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org,
linux-raid-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org,
snitzer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
philipp.reisner-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org,
linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
target-devel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Bart Van Assche,
lars.ellenberg-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org,
shli-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
agk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
drbd-dev-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org
In-Reply-To: <20170507092209.GA27370-jcswGhMUV9g@public.gmane.org>
On Sun, 2017-05-07 at 11:22 +0200, hch-jcswGhMUV9g@public.gmane.org wrote:
> On Tue, May 02, 2017 at 08:33:15PM -0700, Nicholas A. Bellinger wrote:
> > The larger target/iblock conversion patch looks like post v4.12 material
> > at this point, so to avoid breakage wrt to existing LBPRZ behavior, I'll
> > plan to push the following patch post -rc1.
>
> I don't think this is safe. If you want to do the aboe you also
> need to ensure ->execute_unmap always zeroes the data. For actual
> files in the file backend we should be all fine, but for the block
> device case [1] and iblock we'd need to use blkdev_issue_zeroout
> instead of blkdev_issue_discard when unmap_zeroes_data is set.
>
> [1] which btw already seems broken as it doesn't invalidate cached
> data when issuing a discard.
Mmm, for [1] that would appear to be true, but after a deeper look at
existing code I don't think this is the case.
The reason being is target backend attributes emulate_tpu and
emulate_tpws are strictly user configurable, and aren't automatically
set based upon the underlying IBLOCK block_device support for either
one.
According to pre v4.12-rc1 code, q->limits.discard_zeroes_data was only
enabled by drivers/scsi/sd.c:sd_config_discard() for
sdkp->provisioning_mode WRITE_SAME with LBPRZ = 1 or explicit ZERO, and
for NVME for devices that supported NVME_QUIRK_DISCARD_ZEROES. Eg: Only
real DISCARD + ZERO support.
In your changes to v4.12-rc1, this logic to signal real DISCARD + zero
support for SCSI and NVMe via q->limits.max_write_zeroes_sectors has not
changed..
So AFAICT, regardless if the user sets emulate_tpu or emulate_tpws for a
IBLOCK backend, SCSI host code will have to choose sdkp->zeroing_mode
WRITE_SAME with LBPRZ or explicit ZERO, and NVMe host code will have to
chose a ctrl NVME_QUIRK_DEALLOCATE_ZEROES before
q->limits.max_write_zeroes_sectors != 0 is propagated up to target code,
and LBPRZ = 1 is signaled via READ_CAPACITY_16 and EVPD = 0xb2.
That said, simply propagating up q->limits.max_write_zeroes_sectors as
dev_attrib->unmap_zeroes_data following existing code still looks like
the right thing to do.
^ permalink raw reply
* [PATCH] mdadm/md.4: set page-length as 1000 to avoid warnings from grotty
From: Zhilong Liu @ 2017-05-09 6:25 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: linux-raid, Zhilong Liu
"nroff" ("man") or ("tbl") needs a long page to avoid warnings
from "grotty", set 'pl' as 1000 to fix the following issue.
This command invokes by Makefile rule.
linux-tjrh:~/mdadm # man -l md.4 > test
grotty:<standard input> (<standard input>):5967:
character above first line discarded
... ...
Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
md.4 | 1 +
1 file changed, 1 insertion(+)
diff --git a/md.4 b/md.4
index 5bdf7a7..3a1d677 100644
--- a/md.4
+++ b/md.4
@@ -4,6 +4,7 @@
.\" the Free Software Foundation; either version 2 of the License, or
.\" (at your option) any later version.
.\" See file COPYING in distribution for details.
+.if n .pl 1000v
.TH MD 4
.SH NAME
md \- Multiple Device driver aka Linux Software RAID
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] md/md0: optimize raid0 discard handling
From: NeilBrown @ 2017-05-09 1:03 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, Coly Li
In-Reply-To: <1af0237c374a1e276663fa17824e8345165fcca6.1494203680.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 7062 bytes --]
On Sun, May 07 2017, Shaohua Li wrote:
> There are complaints that raid0 discard handling is slow. Currently we
> divide discard request into chunks and dispatch to underlayer disks. The
> block layer will do merge to form big requests. This causes a lot of
> request split/merge and uses significant CPU time.
>
> A simple idea is to calculate the range for each raid disk for an IO
> request and send a discard request to raid disks, which will avoid the
> split/merge completely. Previously Coly tried the approach, but the
> implementation was too complex because of raid0 zones. This patch always
> split bio in zone boundary and handle bio within one zone. It simplifies
> the implementation a lot.
>
> Cc: NeilBrown <neilb@suse.com>
> Cc: Coly Li <colyli@suse.de>
> Signed-off-by: Shaohua Li <shli@fb.com>
Reviewed-by: NeilBrown <neilb@suse.com>
I'm a little bit bothered by the use for __blkdev_issue_discard() which
uses bio_alloc(), which allocates from fs_bio_set. This code isn't in a
filesystem, so it feels wrong.
fs_bio_set has 4 entries. If 4 different threads call
__blkdev_issue_discard() on a raid0 device, they could get allocated all
of the entries from the pool. Then when raid0 calls
__blkdev_issue_discard(), the pool is empty.
I don't think this is actually a problem as discard (presumably) doesn't
happen on the write-out-for-memory-reclaim path, so the bio_alloc() will
eventually be able to get memory from kmalloc() rather than from the
pool.
Maybe next_bio() should use the bio_split pool from
bio->bi_bdev->queue. But it probably doesn't really matter.
Thanks,
NeilBrown
> ---
> drivers/md/raid0.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 102 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index 84e5859..d6c0bc7 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -385,7 +385,7 @@ static int raid0_run(struct mddev *mddev)
> blk_queue_max_hw_sectors(mddev->queue, mddev->chunk_sectors);
> blk_queue_max_write_same_sectors(mddev->queue, mddev->chunk_sectors);
> blk_queue_max_write_zeroes_sectors(mddev->queue, mddev->chunk_sectors);
> - blk_queue_max_discard_sectors(mddev->queue, mddev->chunk_sectors);
> + blk_queue_max_discard_sectors(mddev->queue, UINT_MAX);
>
> blk_queue_io_min(mddev->queue, mddev->chunk_sectors << 9);
> blk_queue_io_opt(mddev->queue,
> @@ -459,6 +459,95 @@ static inline int is_io_in_chunk_boundary(struct mddev *mddev,
> }
> }
>
> +static void raid0_handle_discard(struct mddev *mddev, struct bio *bio)
> +{
> + struct r0conf *conf = mddev->private;
> + struct strip_zone *zone;
> + sector_t start = bio->bi_iter.bi_sector;
> + sector_t end;
> + unsigned int stripe_size;
> + sector_t first_stripe_index, last_stripe_index;
> + sector_t start_disk_offset;
> + unsigned int start_disk_index;
> + sector_t end_disk_offset;
> + unsigned int end_disk_index;
> + unsigned int disk;
> +
> + zone = find_zone(conf, &start);
> +
> + if (bio_end_sector(bio) > zone->zone_end) {
> + struct bio *split = bio_split(bio,
> + zone->zone_end - bio->bi_iter.bi_sector, GFP_NOIO,
> + mddev->bio_set);
> + bio_chain(split, bio);
> + generic_make_request(bio);
> + bio = split;
> + end = zone->zone_end;
> + } else
> + end = bio_end_sector(bio);
> +
> + if (zone != conf->strip_zone)
> + end = end - zone[-1].zone_end;
> +
> + /* Now start and end is the offset in zone */
> + stripe_size = zone->nb_dev * mddev->chunk_sectors;
> +
> + first_stripe_index = start;
> + sector_div(first_stripe_index, stripe_size);
> + last_stripe_index = end;
> + sector_div(last_stripe_index, stripe_size);
> +
> + start_disk_index = (int)(start - first_stripe_index * stripe_size) /
> + mddev->chunk_sectors;
> + start_disk_offset = ((int)(start - first_stripe_index * stripe_size) %
> + mddev->chunk_sectors) +
> + first_stripe_index * mddev->chunk_sectors;
> + end_disk_index = (int)(end - last_stripe_index * stripe_size) /
> + mddev->chunk_sectors;
> + end_disk_offset = ((int)(end - last_stripe_index * stripe_size) %
> + mddev->chunk_sectors) +
> + last_stripe_index * mddev->chunk_sectors;
> +
> + for (disk = 0; disk < zone->nb_dev; disk++) {
> + sector_t dev_start, dev_end;
> + struct bio *discard_bio = NULL;
> + struct md_rdev *rdev;
> +
> + if (disk < start_disk_index)
> + dev_start = (first_stripe_index + 1) *
> + mddev->chunk_sectors;
> + else if (disk > start_disk_index)
> + dev_start = first_stripe_index * mddev->chunk_sectors;
> + else
> + dev_start = start_disk_offset;
> +
> + if (disk < end_disk_index)
> + dev_end = (last_stripe_index + 1) * mddev->chunk_sectors;
> + else if (disk > end_disk_index)
> + dev_end = last_stripe_index * mddev->chunk_sectors;
> + else
> + dev_end = end_disk_offset;
> +
> + if (dev_end <= dev_start)
> + continue;
> +
> + rdev = conf->devlist[(zone - conf->strip_zone) *
> + conf->strip_zone[0].nb_dev + disk];
> + if (__blkdev_issue_discard(rdev->bdev,
> + dev_start + zone->dev_start + rdev->data_offset,
> + dev_end - dev_start, GFP_NOIO, 0, &discard_bio) ||
> + !discard_bio)
> + continue;
> + bio_chain(discard_bio, bio);
> + if (mddev->gendisk)
> + trace_block_bio_remap(bdev_get_queue(rdev->bdev),
> + discard_bio, disk_devt(mddev->gendisk),
> + bio->bi_iter.bi_sector);
> + generic_make_request(discard_bio);
> + }
> + bio_endio(bio);
> +}
> +
> static void raid0_make_request(struct mddev *mddev, struct bio *bio)
> {
> struct strip_zone *zone;
> @@ -473,6 +562,11 @@ static void raid0_make_request(struct mddev *mddev, struct bio *bio)
> return;
> }
>
> + if (unlikely((bio_op(bio) == REQ_OP_DISCARD))) {
> + raid0_handle_discard(mddev, bio);
> + return;
> + }
> +
> bio_sector = bio->bi_iter.bi_sector;
> sector = bio_sector;
> chunk_sects = mddev->chunk_sectors;
> @@ -498,19 +592,13 @@ static void raid0_make_request(struct mddev *mddev, struct bio *bio)
> bio->bi_iter.bi_sector = sector + zone->dev_start +
> tmp_dev->data_offset;
>
> - if (unlikely((bio_op(bio) == REQ_OP_DISCARD) &&
> - !blk_queue_discard(bdev_get_queue(bio->bi_bdev)))) {
> - /* Just ignore it */
> - bio_endio(bio);
> - } else {
> - if (mddev->gendisk)
> - trace_block_bio_remap(bdev_get_queue(bio->bi_bdev),
> - bio, disk_devt(mddev->gendisk),
> - bio_sector);
> - mddev_check_writesame(mddev, bio);
> - mddev_check_write_zeroes(mddev, bio);
> - generic_make_request(bio);
> - }
> + if (mddev->gendisk)
> + trace_block_bio_remap(bdev_get_queue(bio->bi_bdev),
> + bio, disk_devt(mddev->gendisk),
> + bio_sector);
> + mddev_check_writesame(mddev, bio);
> + mddev_check_write_zeroes(mddev, bio);
> + generic_make_request(bio);
> }
>
> static void raid0_status(struct seq_file *seq, struct mddev *mddev)
> --
> 2.9.3
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [PATCH v4 2/2] md/r5cache: gracefully handle journal device errors for writeback mode
From: Song Liu @ 2017-05-09 0:39 UTC (permalink / raw)
To: linux-raid
Cc: shli, neilb, kernel-team, dan.j.williams, hch, jes.sorensen,
Song Liu
In-Reply-To: <20170509003925.3480693-1-songliubraving@fb.com>
For the raid456 with writeback cache, when journal device failed during
normal operation, it is still possible to persist all data, as all
pending data is still in stripe cache. However, it is necessary to handle
journal failure gracefully.
During journal failures, this patch makes the follow changes to land data
in cache to raid disks gracefully:
1. In handle_stripe(), allow stripes with data in journal (s.injournal > 0)
to make progress;
2. In delay_towrite(), only process data in the cache (skip dev->towrite);
3. In __get_priority_stripe(), set try_loprio to true, so no stripe stuck
in loprio_list
Signed-off-by: Song Liu <songliubraving@fb.com>
---
drivers/md/raid5-cache.c | 13 ++++++++++---
drivers/md/raid5-log.h | 3 ++-
drivers/md/raid5.c | 29 +++++++++++++++++++++++------
3 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index dc1dba6..e6032f6 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -24,6 +24,7 @@
#include "md.h"
#include "raid5.h"
#include "bitmap.h"
+#include "raid5-log.h"
/*
* metadata/data stored in disk with 4k size unit (a block) regardless
@@ -679,6 +680,7 @@ static void r5c_disable_writeback_async(struct work_struct *work)
return;
pr_info("md/raid:%s: Disabling writeback cache for degraded array.\n",
mdname(mddev));
+ md_update_sb(mddev, 1);
mddev_suspend(mddev);
log->r5c_journal_mode = R5C_JOURNAL_MODE_WRITE_THROUGH;
mddev_resume(mddev);
@@ -1557,6 +1559,8 @@ void r5l_wake_reclaim(struct r5l_log *log, sector_t space)
void r5l_quiesce(struct r5l_log *log, int state)
{
struct mddev *mddev;
+ struct r5conf *conf;
+
if (!log || state == 2)
return;
if (state == 0)
@@ -1564,10 +1568,12 @@ void r5l_quiesce(struct r5l_log *log, int state)
else if (state == 1) {
/* make sure r5l_write_super_and_discard_space exits */
mddev = log->rdev->mddev;
+ conf = mddev->private;
wake_up(&mddev->sb_wait);
kthread_park(log->reclaim_thread->tsk);
r5l_wake_reclaim(log, MaxSector);
- r5l_do_reclaim(log);
+ if (!r5l_log_disk_error(conf))
+ r5l_do_reclaim(log);
}
}
@@ -2982,7 +2988,7 @@ static int r5l_load_log(struct r5l_log *log)
return ret;
}
-void r5c_update_on_rdev_error(struct mddev *mddev)
+void r5c_update_on_rdev_error(struct mddev *mddev, struct md_rdev *rdev)
{
struct r5conf *conf = mddev->private;
struct r5l_log *log = conf->log;
@@ -2990,7 +2996,8 @@ void r5c_update_on_rdev_error(struct mddev *mddev)
if (!log)
return;
- if (raid5_calc_degraded(conf) > 0 &&
+ if ((raid5_calc_degraded(conf) > 0 ||
+ test_bit(Journal, &rdev->flags)) &&
conf->log->r5c_journal_mode == R5C_JOURNAL_MODE_WRITE_BACK)
schedule_work(&log->disable_writeback_work);
}
diff --git a/drivers/md/raid5-log.h b/drivers/md/raid5-log.h
index 2709710..328d67a 100644
--- a/drivers/md/raid5-log.h
+++ b/drivers/md/raid5-log.h
@@ -28,7 +28,8 @@ extern void r5c_flush_cache(struct r5conf *conf, int num);
extern void r5c_check_stripe_cache_usage(struct r5conf *conf);
extern void r5c_check_cached_full_stripe(struct r5conf *conf);
extern struct md_sysfs_entry r5c_journal_mode;
-extern void r5c_update_on_rdev_error(struct mddev *mddev);
+extern void r5c_update_on_rdev_error(struct mddev *mddev,
+ struct md_rdev *rdev);
extern bool r5c_big_stripe_cached(struct r5conf *conf, sector_t sect);
extern struct dma_async_tx_descriptor *
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index f8055a7..0ac57a9 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2689,7 +2689,7 @@ static void raid5_error(struct mddev *mddev, struct md_rdev *rdev)
bdevname(rdev->bdev, b),
mdname(mddev),
conf->raid_disks - mddev->degraded);
- r5c_update_on_rdev_error(mddev);
+ r5c_update_on_rdev_error(mddev, rdev);
}
/*
@@ -3050,6 +3050,11 @@ sector_t raid5_compute_blocknr(struct stripe_head *sh, int i, int previous)
* When LOG_CRITICAL, stripes with injournal == 0 will be sent to
* no_space_stripes list.
*
+ * 3. during journal failure
+ * In journal failure, we try to flush all cached data to raid disks
+ * based on data in stripe cache. The array is read-only to upper
+ * layers, so we would skip all pending writes.
+ *
*/
static inline bool delay_towrite(struct r5conf *conf,
struct r5dev *dev,
@@ -3063,6 +3068,9 @@ static inline bool delay_towrite(struct r5conf *conf,
if (test_bit(R5C_LOG_CRITICAL, &conf->cache_state) &&
s->injournal > 0)
return true;
+ /* case 3 above */
+ if (s->log_failed && s->injournal)
+ return true;
return false;
}
@@ -4696,10 +4704,15 @@ static void handle_stripe(struct stripe_head *sh)
" to_write=%d failed=%d failed_num=%d,%d\n",
s.locked, s.uptodate, s.to_read, s.to_write, s.failed,
s.failed_num[0], s.failed_num[1]);
- /* check if the array has lost more than max_degraded devices and,
+ /*
+ * check if the array has lost more than max_degraded devices and,
* if so, some requests might need to be failed.
+ *
+ * When journal device failed (log_failed), we will only process
+ * the stripe if there is data need write to raid disks
*/
- if (s.failed > conf->max_degraded || s.log_failed) {
+ if (s.failed > conf->max_degraded ||
+ (s.log_failed && s.injournal == 0)) {
sh->check_state = 0;
sh->reconstruct_state = 0;
break_stripe_batch_list(sh, 0);
@@ -5272,8 +5285,10 @@ static struct stripe_head *__get_priority_stripe(struct r5conf *conf, int group)
struct stripe_head *sh, *tmp;
struct list_head *handle_list = NULL;
struct r5worker_group *wg;
- bool second_try = !r5c_is_writeback(conf->log);
- bool try_loprio = test_bit(R5C_LOG_TIGHT, &conf->cache_state);
+ bool second_try = !r5c_is_writeback(conf->log) &&
+ !r5l_log_disk_error(conf);
+ bool try_loprio = test_bit(R5C_LOG_TIGHT, &conf->cache_state) ||
+ r5l_log_disk_error(conf);
again:
wg = NULL;
@@ -7521,7 +7536,9 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
* neilb: there is no locking about new writes here,
* so this cannot be safe.
*/
- if (atomic_read(&conf->active_stripes)) {
+ if (atomic_read(&conf->active_stripes) ||
+ atomic_read(&conf->r5c_cached_full_stripes) ||
+ atomic_read(&conf->r5c_cached_partial_stripes)) {
return -EBUSY;
}
log_exit(conf);
--
2.9.3
^ permalink raw reply related
* [PATCH v4 1/2] md:r5cache: in r5l_do_submit_io(), submit io->split_bio first.
From: Song Liu @ 2017-05-09 0:39 UTC (permalink / raw)
To: linux-raid
Cc: shli, neilb, kernel-team, dan.j.williams, hch, jes.sorensen,
Song Liu
In r5l_do_submit_io(), it is necessary to check io->split_bio before
submit io->current_bio. This is because, endio of current_bio may
free the whole IO unit, and thus change io->split_bio.
Signed-off-by: Song Liu <songliubraving@fb.com>
---
drivers/md/raid5-cache.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 26ba092..dc1dba6 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -622,20 +622,29 @@ static void r5l_do_submit_io(struct r5l_log *log, struct r5l_io_unit *io)
__r5l_set_io_unit_state(io, IO_UNIT_IO_START);
spin_unlock_irqrestore(&log->io_list_lock, flags);
+ /*
+ * In case of journal device failures, submit_bio will get error
+ * and calls endio, then active stripes will continue write
+ * process. Therefore, it is not necessary to check Faulty bit
+ * of journal device here.
+ *
+ * However, calling r5l_log_endio for current_bio may free the
+ * io_unit. Therefore, it is necessary to check io->split_bio
+ * before submitting io->current_bio.
+ */
+ if (io->split_bio) {
+ if (io->has_flush)
+ io->split_bio->bi_opf |= REQ_PREFLUSH;
+ if (io->has_fua)
+ io->split_bio->bi_opf |= REQ_FUA;
+ submit_bio(io->split_bio);
+ }
+
if (io->has_flush)
io->current_bio->bi_opf |= REQ_PREFLUSH;
if (io->has_fua)
io->current_bio->bi_opf |= REQ_FUA;
submit_bio(io->current_bio);
-
- if (!io->split_bio)
- return;
-
- if (io->has_flush)
- io->split_bio->bi_opf |= REQ_PREFLUSH;
- if (io->has_fua)
- io->split_bio->bi_opf |= REQ_FUA;
- submit_bio(io->split_bio);
}
/* deferred io_unit will be dispatched here */
--
2.9.3
^ permalink raw reply related
* Re: Fault tolerance with badblocks
From: Phil Turmel @ 2017-05-08 20:56 UTC (permalink / raw)
To: Nix; +Cc: Wols Lists, Ravi (Tom) Hale, linux-raid
In-Reply-To: <87vapb6s9h.fsf@esperi.org.uk>
On 05/08/2017 03:52 PM, Nix wrote:
> On 8 May 2017, Phil Turmel verbalised:
>
>> On 05/08/2017 10:50 AM, Nix wrote:
> And... then what do you do? On RAID-6, it appears the answer is "live
> with a high probability of inevitable corruption".
No, you investigate the quality of your data and the integrity of the
rest of the system, as something *other* than a drive problem caused the
mismatch. (Swap is a known exception, though.)
> That's not very good.
> (AIUI, if a check scrub finds a URE, it'll rewrite it, and when in the
> common case the drive spares it out and the write succeeds, this will
> not be reported as a mismatch: is this right?)
This is also wrong, because you are assuming sparing-out is the common
case. A read error does not automatically trigger relocation. It
triggers *verification* of the next *write*. In young drives,
successful rewrite in place is the common case. As the drive ages,
rewrites will begin relocating because there really is a new problem at
that spot, not simple thermal/magnetic decay.
But keep in mind that the firmware of the drive will start verification
of a sector only if it gets a *read* error. Such sectors get marked as
"pending" relocations until they are written again. If that write
verifies correct, the "pending" status simply goes away. Ordinary
writes to presumed-ok sectors are *not* verified. (There'd be a huge
difference between read and write speeds on rotating media if they were.)
{ Drive self tests might do some pre-emptive rewriting of marginal
sectors -- it's not something drive manufacturers are documenting. But
a drive self-test cannot fix an unreadable sector -- it doesn't know
what to write there. }
>> This is actually counterproductive. Rewriting everything may refresh
>> the magnetism on weakening sectors, but will also prevent the drive from
>> *finding* weakening sectors that really do need relocation.
>
> If a sector weakens purely because of neighbouring writes or temperature
> or a vibrating housing or something (i.e. not because of actual damage),
> so that a rewrite will strengthen it and relocation was never necessary,
> surely you've just saved a pointless bit of sector sparing? (I don't
> know: I'm not sure what the relative frequency of these things is. Read
> and write errors in general are so rare that it's quite possible I'm
> worrying about nothing at all. I do know I forgot to scrub my old
> hardware RAID array for about three years and nothing bad happened...)
Drives that are in applications that get *read* pretty often don't need
much if any scrubbing -- the application itself will expose problem
sectors. Hobbyists and home media servers can go months with specific
files unread, so developing problems can hit in clusters. Regular
scrubbing will catch these problems before they take your array down.
And you can't compare hardware array behavior to MD -- they have their
own algorithms to take care of attached disks without OS intervention.
Phil
^ permalink raw reply
* Re: [RFC 08/10] cifs: move to generic async completion
From: Pavel Shilovsky @ 2017-05-08 20:56 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Jonathan Corbet, David Howells,
Alasdair Kergon, Mike Snitzer, dm-devel, Shaohua Li, Steve French,
Theodore Y. Ts'o, Jaegeuk Kim, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, Ofir Drang, Gilad Ben-Yossef,
linux-crypto, linux-doc, Kernel Mailing List, keyrings
In-Reply-To: <1494075602-5061-9-git-send-email-gilad@benyossef.com>
2017-05-06 5:59 GMT-07:00 Gilad Ben-Yossef <gilad@benyossef.com>:
> cifs starts an async. crypto op and waits for their completion.
> Move it over to generic code doing the same.
>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> ---
> fs/cifs/smb2ops.c | 30 ++++--------------------------
> 1 file changed, 4 insertions(+), 26 deletions(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 152e37f..cee7bb3 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1706,22 +1706,6 @@ init_sg(struct smb_rqst *rqst, u8 *sign)
> return sg;
> }
>
> -struct cifs_crypt_result {
> - int err;
> - struct completion completion;
> -};
> -
> -static void cifs_crypt_complete(struct crypto_async_request *req, int err)
> -{
> - struct cifs_crypt_result *res = req->data;
> -
> - if (err == -EINPROGRESS)
> - return;
> -
> - res->err = err;
> - complete(&res->completion);
> -}
> -
> static int
> smb2_get_enc_key(struct TCP_Server_Info *server, __u64 ses_id, int enc, u8 *key)
> {
> @@ -1762,12 +1746,10 @@ crypt_message(struct TCP_Server_Info *server, struct smb_rqst *rqst, int enc)
> struct aead_request *req;
> char *iv;
> unsigned int iv_len;
> - struct cifs_crypt_result result = {0, };
> + DECLARE_CRYPTO_WAIT(wait);
> struct crypto_aead *tfm;
> unsigned int crypt_len = le32_to_cpu(tr_hdr->OriginalMessageSize);
>
> - init_completion(&result.completion);
> -
> rc = smb2_get_enc_key(server, tr_hdr->SessionId, enc, key);
> if (rc) {
> cifs_dbg(VFS, "%s: Could not get %scryption key\n", __func__,
> @@ -1825,14 +1807,10 @@ crypt_message(struct TCP_Server_Info *server, struct smb_rqst *rqst, int enc)
> aead_request_set_ad(req, assoc_data_len);
>
> aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> - cifs_crypt_complete, &result);
> + crypto_req_done, &wait);
>
> - rc = enc ? crypto_aead_encrypt(req) : crypto_aead_decrypt(req);
> -
> - if (rc == -EINPROGRESS || rc == -EBUSY) {
> - wait_for_completion(&result.completion);
> - rc = result.err;
> - }
> + rc = crypto_wait_req(enc ? crypto_aead_encrypt(req)
> + : crypto_aead_decrypt(req), &wait);
>
> if (!rc && enc)
> memcpy(&tr_hdr->Signature, sign, SMB2_SIGNATURE_SIZE);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Acked-by: Pavel Shilovsky <pshilov@microsoft.com>
Best regards,
Pavel Shilovsky
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Anthony Youngman @ 2017-05-08 20:27 UTC (permalink / raw)
To: Nix, Phil Turmel; +Cc: Ravi (Tom) Hale, linux-raid
In-Reply-To: <87vapb6s9h.fsf@esperi.org.uk>
On 08/05/17 20:52, Nix wrote:
> On 8 May 2017, Phil Turmel verbalised:
>
>> On 05/08/2017 10:50 AM, Nix wrote:
>>
>>> I wonder... scrubbing is not very useful with md, particularly with RAID
>>> 6, because it does no writes unless something mismatches,
>>
>> This is wrong. The purpose of scrubbing is to expose any sectors that
>> have degraded (as Wol describes) to the point of generating a read
>> error. A "check" scrub only writes back to the sectors that report a
>> URE, giving the drive firmware a chance to fix or relocate the sector.
>>
>> A check scrub will NOT write on mismatch, just increment the mismatch
>> counter. This is the recommended regular scrubbing operation. You want
>> to know when mismatches occur.
>
> And... then what do you do? On RAID-6, it appears the answer is "live
> with a high probability of inevitable corruption". That's not very good.
> (AIUI, if a check scrub finds a URE, it'll rewrite it, and when in the
> common case the drive spares it out and the write succeeds, this will
> not be reported as a mismatch: is this right?)
I think you're misunderstanding RAID here. IF the drive says "I can't
read this block", the RAID reconstructs the block, and rewrites it. No
corruption.
If the scrub finds a mismatch, then the drives are reporting
"everything's fine here". Something's gone wrong, but the question is
what? If you've got a four-drive raid that reports a mismatch, how do
you know which of the four drives is corrupt? Doing an auto-correct here
risks doing even more damage. (I think a raid-6 could recover, but
raid-5 is toast ...)
And seeing as drives are pretty much guaranteed (unless something's gone
BADLY wrong) to either (a) accurately return the data written, or (b)
return a read error, that means a data mismatch indicates something is
seriously wrong that is NOTHING to do with the drives.
<snip>
>
> If a sector weakens purely because of neighbouring writes or temperature
> or a vibrating housing or something (i.e. not because of actual damage),
> so that a rewrite will strengthen it and relocation was never necessary,
> surely you've just saved a pointless bit of sector sparing? (I don't
> know: I'm not sure what the relative frequency of these things is. Read
> and write errors in general are so rare that it's quite possible I'm
> worrying about nothing at all. I do know I forgot to scrub my old
> hardware RAID array for about three years and nothing bad happened...)
>
Yes you have saved a sector sparing. Note that a consumer 3TB drive can
return, on average, one error every time it's read from end to end 3
times, and still be considered "within spec" ie "not faulty" by the
manufacturer. And that's a *brand* *new* drive. That's why building a
large array using consumer drives is a stupid idea - 4 x 3TB drives and
a *within* *spec* array must expect to handle at least one error every
scrub.
Okay - most drives are actually way over spec, and could probably be
read end-to-end many times without a single error, but you'd be a fool
to gamble on it.
Cheers,
Wol
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Nix @ 2017-05-08 19:52 UTC (permalink / raw)
To: Phil Turmel; +Cc: Wols Lists, Ravi (Tom) Hale, linux-raid
In-Reply-To: <1533bba8-41cb-2c50-b28a-52786e463072@turmel.org>
On 8 May 2017, Phil Turmel verbalised:
> On 05/08/2017 10:50 AM, Nix wrote:
>
>> I wonder... scrubbing is not very useful with md, particularly with RAID
>> 6, because it does no writes unless something mismatches,
>
> This is wrong. The purpose of scrubbing is to expose any sectors that
> have degraded (as Wol describes) to the point of generating a read
> error. A "check" scrub only writes back to the sectors that report a
> URE, giving the drive firmware a chance to fix or relocate the sector.
>
> A check scrub will NOT write on mismatch, just increment the mismatch
> counter. This is the recommended regular scrubbing operation. You want
> to know when mismatches occur.
And... then what do you do? On RAID-6, it appears the answer is "live
with a high probability of inevitable corruption". That's not very good.
(AIUI, if a check scrub finds a URE, it'll rewrite it, and when in the
common case the drive spares it out and the write succeeds, this will
not be reported as a mismatch: is this right?)
>> If there was a way to get md to *rewrite* everything during scrub,
>> rather than just checking, this might help (in addition to letting the
>> drive refresh the magnetization of absolutely everything).
>
> This is actually counterproductive. Rewriting everything may refresh
> the magnetism on weakening sectors, but will also prevent the drive from
> *finding* weakening sectors that really do need relocation.
If a sector weakens purely because of neighbouring writes or temperature
or a vibrating housing or something (i.e. not because of actual damage),
so that a rewrite will strengthen it and relocation was never necessary,
surely you've just saved a pointless bit of sector sparing? (I don't
know: I'm not sure what the relative frequency of these things is. Read
and write errors in general are so rare that it's quite possible I'm
worrying about nothing at all. I do know I forgot to scrub my old
hardware RAID array for about three years and nothing bad happened...)
--
NULL && (void)
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Phil Turmel @ 2017-05-08 19:02 UTC (permalink / raw)
To: Nix, Wols Lists; +Cc: Ravi (Tom) Hale, linux-raid
In-Reply-To: <87h90v8kt3.fsf@esperi.org.uk>
On 05/08/2017 10:50 AM, Nix wrote:
> I wonder... scrubbing is not very useful with md, particularly with RAID
> 6, because it does no writes unless something mismatches,
This is wrong. The purpose of scrubbing is to expose any sectors that
have degraded (as Wol describes) to the point of generating a read
error. A "check" scrub only writes back to the sectors that report a
URE, giving the drive firmware a chance to fix or relocate the sector.
A check scrub will NOT write on mismatch, just increment the mismatch
counter. This is the recommended regular scrubbing operation. You want
to know when mismatches occur.
> If there was a way to get md to *rewrite* everything during scrub,
> rather than just checking, this might help (in addition to letting the
> drive refresh the magnetization of absolutely everything).
This is actually counterproductive. Rewriting everything may refresh
the magnetism on weakening sectors, but will also prevent the drive from
*finding* weakening sectors that really do need relocation.
Phil
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Anthony Youngman @ 2017-05-08 18:00 UTC (permalink / raw)
To: Nix; +Cc: Ravi (Tom) Hale, linux-raid
In-Reply-To: <87h90v8kt3.fsf@esperi.org.uk>
On 08/05/17 15:50, Nix wrote:
> On 6 May 2017, Wols Lists outgrape:
>
>> On 06/05/17 12:21, Ravi (Tom) Hale wrote:
>>>> Bear in mind also, that any *within* *spec* drive can have an "accident"
>>>>> every 10TB and still be considered perfectly okay. Which means that if
>>>>> you do what you are supposed to do (rewrite the block) you're risking
>>>>> the drive remapping the block - and getting closer to the drive bricking
>>>>> itself. But if you trap the error yourself and add it to the badblocks
>>>>> list, you are risking throwing away perfectly decent blocks that just
>>>>> hiccuped.
>>
>>> For hiccups, having a bad-read-count for each suspected-bad block could
>>> be sensible. If that number goes above <small-threshold> it's very
>>> likely that the block is indeed bad and should be avoided in future.
>>
>> Except you have the second law of thermodynamics in play - "what man
>> proposes, nature opposes". This could well screw up big time.
>>
>> DRAM memory needs to be refreshed by a read-write cycle every few
>> nanoseconds. Hard drives are the same, actually, except that the
>> interval is measured in years, not nanoseconds. Fill your brand new hard
>> drive with data, then hammer it gently over a few years. Especially if a
>> block's neighbours are repeatedly rewritten but this particular block is
>> never touched, it is likely to become unreadable.
>>
>> So it will fail your test - reads will repeatedly fail - but if the
>> firmware was given a look-in (by rewriting it) it wouldn't be remapped.
>
> You mean it *would* be remapped (and all would be well).
>
No. The data would be lost, the block would be overwritten successfully
and there would be no need to remap. Basically, the magnetism has
decayed (so it can't be reconstructed from the extra error recovery bits
on disk) and rewriting it fixes the problem. But the data's been lost ...
> I wonder... scrubbing is not very useful with md, particularly with RAID
> 6, because it does no writes unless something mismatches, and on failure
> there is no attempt to determine which of the N disks is bad and rewrite
> its contents from the other devices (nor, as I understand it, does it
> clearly say which drive gave the error, so even failing it out and
> resyncing it is hard).
With redundant raid (and that doesn't include a two-disk, or even
three-disk mirror), it SHOULD recalculate the failed block. If it
doesn't bother even though it can, I'd call that a bug in scrub. What I
thought happened was that it reads a stripe direct from disk, and if
that failed it read the same stripe via the raid code, to get the raid
error correction to fire, and then it rewrote the stripe.
What would be a nice touch, is that if we have a massive timeout for
non-SCT drives, if the scrub has to wait more than, say, 10 seconds for
a read to succeed it then assumes the block is failing and rewrites it.
Actually, scrub that (groan... :-) - if the drive takes longer than 1/3
of the timeout to respond, then the scrub assumes it's dodgy and
rewrites it.
>
> If there was a way to get md to *rewrite* everything during scrub,
> rather than just checking, this might help (in addition to letting the
> drive refresh the magnetization of absolutely everything). "repair" mode
> appears to do no writes until an error is found, whereupon (on RAID 6)
> it proceeds to make a "repair" that is more likely than not to overwrite
> good data with bad. Optionally writing what's already there on non-error
> seems like it might be a worthwhile (and fairly simple) change.
>
Agreed. But without some heuristic, it's actually going to make a scrub
much slower, and achieve very little apart from adding unnecessary wear
to the drive.
Cheers,
Wol
^ permalink raw reply
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked
From: Jes Sorensen @ 2017-05-08 17:54 UTC (permalink / raw)
To: Zhilong Liu; +Cc: linux-raid
In-Reply-To: <2b947e61-36b6-0c6c-c25c-a9d2831cca11@suse.com>
On 05/07/2017 09:50 PM, Zhilong Liu wrote:
>
>
> On 05/05/2017 11:31 AM, Liu Zhilong wrote:
>>
>>
>> On 05/04/2017 10:54 PM, Jes Sorensen wrote:
>>> On 05/04/2017 08:20 AM, Zhilong Liu wrote:
>>>> Hi Jes,
>>>>
>>>> apply for review, this is a bug I ever encountered.
>>>
>>> Zhilong,
>>>
>>> Under what circumstances do you see this?
>>>
>>
>> Issued the command:
>> linux-g0sr:/home/test # ./mdadm -CR /dev/md0 -l1 -n2 -b internal
>> /dev/loop[0-1] --size 63
>> ... ... ...
>> mdadm: Given bitmap chunk size not supported.
>> linux-g0sr:/home/test # ls /dev/md0
>> /dev/md0
>> linux-g0sr:/home/test # ls /sys/block/md0/md/
>> array_size bitmap component_size level metadata_version
>> raid_disks reshape_position safe_mode_delay
>> array_state chunk_size layout max_read_errors
>> new_dev reshape_direction resync_start
>>
>> create_mddev() writes the devnm to
>> /sys/module/md_mod/parameter/new_array,
>> then in md.c, module_param_call() called the 'set' function of
>> add_named_array(),
>> md_alloc() init_and_add the kobject for devm, finally the devnm device
>> has created
>> and sysfs has registered after create_mddev executed successfully.
>> Thus it's better
>> to STOP_ARRAY in any case after create_mddev() invoked.
>>
>
> this patch depends on the kernel commit:
> 039b7225e6e9 ("md: allow creation of mdNNN arrays via
> md_mod/parameters/new_array")
> Neil's patch has set "mddev->hold_active = UNTIL_STOP", thus the
> STOP_ARRAY can work
> well on this situation.
OK now I am confused - are you saying this change will only work after
Neil's kernel patch has been applied? That would be no good, mdadm needs
to work on older kernels too.
Jes
^ permalink raw reply
* Re: [mdadm PATCH] Mention "endian" in documentation for --update=byte-order
From: Jes Sorensen @ 2017-05-08 17:42 UTC (permalink / raw)
To: NeilBrown, Trevor Cordes
Cc: Continuation of Round Table discussion, Adam Thompson, linux-raid
In-Reply-To: <87a86onsme.fsf@notabene.neil.brown.name>
On 05/07/2017 07:40 PM, NeilBrown wrote:
>
> This makes it easier to find as "endian" is a commonly used term.
>
> Reported-by: Trevor Cordes <trevor@tecnopolis.ca>
> Signed-off-by: NeilBrown <neilb@suse.com>
Applied!
Thanks,
Jes
^ permalink raw reply
* Re: [PATCH] md: don't return -EAGAIN in md_allow_write for external metadata arrays
From: Shaohua Li @ 2017-05-08 17:32 UTC (permalink / raw)
To: Artur Paszkiewicz; +Cc: shli, linux-raid
In-Reply-To: <20170508095655.27350-1-artur.paszkiewicz@intel.com>
On Mon, May 08, 2017 at 11:56:55AM +0200, Artur Paszkiewicz wrote:
> This essentially reverts commit b5470dc5fc18 ("md: resolve external
> metadata handling deadlock in md_allow_write") with some adjustments.
>
> Since commit 6791875e2e53 ("md: make reconfig_mutex optional for writes
> to md sysfs files.") changing array_state to 'active' does not use
> mddev_lock() and will not cause a deadlock with md_allow_write(). This
> revert simplifies userspace tools that write to sysfs attributes like
> "stripe_cache_size" or "consistency_policy" because it removes the need
> for special handling for external metadata arrays, checking for EAGAIN
> and retrying the write.
applied, thanks!
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
> drivers/md/md.c | 20 ++++++++------------
> drivers/md/md.h | 2 +-
> drivers/md/raid1.c | 9 +++------
> drivers/md/raid5.c | 12 +++---------
> 4 files changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 82f798be964f..10367ffe92e3 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8022,18 +8022,15 @@ EXPORT_SYMBOL(md_write_end);
> * may proceed without blocking. It is important to call this before
> * attempting a GFP_KERNEL allocation while holding the mddev lock.
> * Must be called with mddev_lock held.
> - *
> - * In the ->external case MD_SB_CHANGE_PENDING can not be cleared until mddev->lock
> - * is dropped, so return -EAGAIN after notifying userspace.
> */
> -int md_allow_write(struct mddev *mddev)
> +void md_allow_write(struct mddev *mddev)
> {
> if (!mddev->pers)
> - return 0;
> + return;
> if (mddev->ro)
> - return 0;
> + return;
> if (!mddev->pers->sync_request)
> - return 0;
> + return;
>
> spin_lock(&mddev->lock);
> if (mddev->in_sync) {
> @@ -8046,13 +8043,12 @@ int md_allow_write(struct mddev *mddev)
> spin_unlock(&mddev->lock);
> md_update_sb(mddev, 0);
> sysfs_notify_dirent_safe(mddev->sysfs_state);
> + /* wait for the dirty state to be recorded in the metadata */
> + wait_event(mddev->sb_wait,
> + !test_bit(MD_SB_CHANGE_CLEAN, &mddev->sb_flags) &&
> + !test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags));
> } else
> spin_unlock(&mddev->lock);
> -
> - if (test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags))
> - return -EAGAIN;
> - else
> - return 0;
> }
> EXPORT_SYMBOL_GPL(md_allow_write);
>
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 4e75d121bfcc..11f15146ce51 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -665,7 +665,7 @@ extern int sync_page_io(struct md_rdev *rdev, sector_t sector, int size,
> bool metadata_op);
> extern void md_do_sync(struct md_thread *thread);
> extern void md_new_event(struct mddev *mddev);
> -extern int md_allow_write(struct mddev *mddev);
> +extern void md_allow_write(struct mddev *mddev);
> extern void md_wait_for_blocked_rdev(struct md_rdev *rdev, struct mddev *mddev);
> extern void md_set_array_sectors(struct mddev *mddev, sector_t array_sectors);
> extern int md_check_no_bitmap(struct mddev *mddev);
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 7ed59351fe97..7c1f73398800 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -3197,7 +3197,7 @@ static int raid1_reshape(struct mddev *mddev)
> struct r1conf *conf = mddev->private;
> int cnt, raid_disks;
> unsigned long flags;
> - int d, d2, err;
> + int d, d2;
>
> /* Cannot change chunk_size, layout, or level */
> if (mddev->chunk_sectors != mddev->new_chunk_sectors ||
> @@ -3209,11 +3209,8 @@ static int raid1_reshape(struct mddev *mddev)
> return -EINVAL;
> }
>
> - if (!mddev_is_clustered(mddev)) {
> - err = md_allow_write(mddev);
> - if (err)
> - return err;
> - }
> + if (!mddev_is_clustered(mddev))
> + md_allow_write(mddev);
>
> raid_disks = mddev->raid_disks + mddev->delta_disks;
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 3809a2192132..f8055a7abb4b 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2309,14 +2309,12 @@ static int resize_stripes(struct r5conf *conf, int newsize)
> struct stripe_head *osh, *nsh;
> LIST_HEAD(newstripes);
> struct disk_info *ndisks;
> - int err;
> + int err = 0;
> struct kmem_cache *sc;
> int i;
> int hash, cnt;
>
> - err = md_allow_write(conf->mddev);
> - if (err)
> - return err;
> + md_allow_write(conf->mddev);
>
> /* Step 1 */
> sc = kmem_cache_create(conf->cache_name[1-conf->active_name],
> @@ -6310,7 +6308,6 @@ int
> raid5_set_cache_size(struct mddev *mddev, int size)
> {
> struct r5conf *conf = mddev->private;
> - int err;
>
> if (size <= 16 || size > 32768)
> return -EINVAL;
> @@ -6322,10 +6319,7 @@ raid5_set_cache_size(struct mddev *mddev, int size)
> ;
> mutex_unlock(&conf->cache_size_mutex);
>
> -
> - err = md_allow_write(mddev);
> - if (err)
> - return err;
> + md_allow_write(mddev);
>
> mutex_lock(&conf->cache_size_mutex);
> while (size > conf->max_nr_stripes)
> --
> 2.11.0
>
> --
> 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
* BUG_ON(sh->batch_head) in init_stripe()
From: Stephane Thiell @ 2017-05-08 17:15 UTC (permalink / raw)
To: linux-raid@vger.kernel.org
Hi list,
I have a recurring issue using raid6 which results in a panic due to a BUG_ON(sh->batch_head). During last weekend, the issue occurred during a weekly raid-check. The raid volumes (12 total) are pretty new, no mismatch nor hardware errors have been detected.
[535089.369357] kernel BUG at drivers/md/raid5.c:527!
[535089.374700] invalid opcode: 0000 [#1] SMP
[535089.379384] Modules linked in: ...
[535089.503509] CPU: 34 PID: 280061 Comm: md0_resync Tainted: G OE ------------ 3.10.0-514.10.2.el7_lustre.x86_64 #1
This is the backtrace:
crash> bt 280061
PID: 280061 TASK: ffff8800757cde20 CPU: 34 COMMAND: "md0_resync"
#0 [ffff88024e217830] machine_kexec at ffffffff81059bdb
#1 [ffff88024e217890] __crash_kexec at ffffffff81105382
#2 [ffff88024e217960] crash_kexec at ffffffff81105470
#3 [ffff88024e217978] oops_end at ffffffff8168f508
#4 [ffff88024e2179a0] die at ffffffff8102e93b
#5 [ffff88024e2179d0] do_trap at ffffffff8168ebc0
#6 [ffff88024e217a20] do_invalid_op at ffffffff8102b144
#7 [ffff88024e217ad0] invalid_op at ffffffff816984de
[exception RIP: raid5_get_active_stripe+1809]
RIP: ffffffffa0e4ed71 RSP: ffff88024e217b88 RFLAGS: 00010086
RAX: 0000000000000000 RBX: ffff883fe5e40408 RCX: dead000000000200
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8823ebf45ca0
RBP: ffff88024e217c30 R8: ffff8823ebf45cb0 R9: 0000000000000080
R10: 0000000000000006 R11: 0000000000000000 R12: ffff883fe5e40400
R13: ffff8823ebf45ca0 R14: 0000000000000000 R15: 0000000000000000
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
#8 [ffff88024e217b80] raid5_get_active_stripe at ffffffffa0e4e996 [raid456]
#9 [ffff88024e217be0] raid5_release_stripe at ffffffffa0e48f24 [raid456]
#10 [ffff88024e217c38] raid5_sync_request at ffffffffa0e53c4b [raid456]
#11 [ffff88024e217ca8] md_do_sync at ffffffff814fef9b
#12 [ffff88024e217e50] md_thread at ffffffff814fb1b5
#13 [ffff88024e217ec8] kthread at ffffffff810b06ff
#14 [ffff88024e217f50] ret_from_fork at ffffffff81696b98
It appears to be triggered by BUG_ON(sh->batch_head) in init_stripe():
crash> l drivers/md/raid5.c:524
519 static void init_stripe(struct stripe_head *sh, sector_t sector, int previous)
520 {
521 struct r5conf *conf = sh->raid_conf;
522 int i, seq;
523
524 BUG_ON(atomic_read(&sh->count) != 0);
525 BUG_ON(test_bit(STRIPE_HANDLE, &sh->state));
526 BUG_ON(stripe_operations_active(sh));
527 BUG_ON(sh->batch_head); <<<
528
Other I/Os were processed at this time, but I am not sure how to check that they were actually on the same md:
crash> ps | grep ">"
<snip>
> 59684 2 28 ffff88407fdc0fb0 RU 0.0 0 0 [md0_raid6]
> 61479 2 17 ffff883e46e80000 UN 0.0 0 0 [ll_ost_io01_001]
> 220748 2 23 ffff881fb7ab8fb0 UN 0.0 0 0 [ll_ost_io01_011]
> 220750 2 19 ffff881fb7abce70 UN 0.0 0 0 [ll_ost_io01_013]
> 279158 2 14 ffff883ab46b4e70 RU 0.0 0 0 [md22_resync]
> 280061 2 34 ffff8800757cde20 RU 0.0 0 0 [md0_resync]
> 280829 2 6 ffff881c72296dd0 RU 0.0 0 0 [md6_resync]
example of possible concurrent writing thread:
crash> bt 61479
PID: 61479 TASK: ffff883e46e80000 CPU: 17 COMMAND: "ll_ost_io01_001"
#0 [ffff883ffc805e58] crash_nmi_callback at ffffffff8104d2e2
#1 [ffff883ffc805e68] nmi_handle at ffffffff8168f699
#2 [ffff883ffc805eb0] do_nmi at ffffffff8168f813
#3 [ffff883ffc805ef0] end_repeat_nmi at ffffffff8168ead3
[exception RIP: _raw_spin_lock_irq+63]
RIP: ffffffff8168e09f RSP: ffff883e46e3f588 RFLAGS: 00000002
RAX: 00000000000044c2 RBX: ffff883fe5e40408 RCX: 000000000000c464
RDX: 000000000000c468 RSI: 000000000000c468 RDI: ffff883fe5e40408
RBP: ffff883e46e3f588 R8: 0000000000000000 R9: 0000000000000080
R10: 0000000000000002 R11: 0000000000000000 R12: ffff883fe5e40400
R13: 0000000000000000 R14: ffff883fe0e61900 R15: 0000000000000000
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
--- <NMI exception stack> ---
#4 [ffff883e46e3f588] _raw_spin_lock_irq at ffffffff8168e09f
#5 [ffff883e46e3f590] raid5_get_active_stripe at ffffffffa0e4e6cb [raid456]
#6 [ffff883e46e3f648] raid5_make_request at ffffffffa0e4ef55 [raid456]
#7 [ffff883e46e3f738] md_make_request at ffffffff814f7dfc
#8 [ffff883e46e3f798] generic_make_request at ffffffff812ee939
#9 [ffff883e46e3f7e0] submit_bio at ffffffff812eea81
#10 [ffff883e46e3f838] osd_submit_bio at ffffffffa10a0bcc [osd_ldiskfs]
#11 [ffff883e46e3f848] osd_do_bio at ffffffffa10a3007 [osd_ldiskfs]
#12 [ffff883e46e3f968] osd_write_commit at ffffffffa10a3b94 [osd_ldiskfs]
#13 [ffff883e46e3fa08] ofd_commitrw_write at ffffffffa1113774 [ofd]
#14 [ffff883e46e3fa80] ofd_commitrw at ffffffffa1116f2d [ofd]
#15 [ffff883e46e3fb08] obd_commitrw at ffffffffa0c43c22 [ptlrpc]
#16 [ffff883e46e3fb70] tgt_brw_write at ffffffffa0c1bfc1 [ptlrpc]
#17 [ffff883e46e3fcd8] tgt_request_handle at ffffffffa0c18275 [ptlrpc]
#18 [ffff883e46e3fd20] ptlrpc_server_handle_request at ffffffffa0bc41fb [ptlrpc]
#19 [ffff883e46e3fde8] ptlrpc_main at ffffffffa0bc82b0 [ptlrpc]
#20 [ffff883e46e3fec8] kthread at ffffffff810b06ff
#21 [ffff883e46e3ff50] ret_from_fork at ffffffff81696b98
The same issue happened during a heavy IOR benchmark a few months ago and was described in https://jira.hpdd.intel.com/browse/LU-8917 (on a slightly older el7 kernel).
I also found out that other users have described similar issues, like on this thread: https://lkml.org/lkml/2016/12/23/205
Now, I have a crash dump and I’m trying to understand why sh->batch_head could be set in init_stripe(), which is called by raid5_get_active_stripe() when __find_stripe() failed BUT get_free_stripe() succeeded. If sh->batch_head is set in that case, that means the idle stripe found had it set…
Does someone have any idea of how to troubleshoot or solve this?
Thanks!
Stephan
^ permalink raw reply
* Re: [PATCH] md/md0: optimize raid0 discard handling
From: Coly Li @ 2017-05-08 16:52 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, NeilBrown
In-Reply-To: <20170508163743.xkct4eoylpjrg72c@kernel.org>
On 2017/5/9 上午12:37, Shaohua Li wrote:
> On Mon, May 08, 2017 at 03:31:47PM +0800, Coly Li wrote:
>> On 2017/5/8 上午8:36, Shaohua Li wrote:
>>> There are complaints that raid0 discard handling is slow. Currently we
>>> divide discard request into chunks and dispatch to underlayer disks. The
>>> block layer will do merge to form big requests. This causes a lot of
>>> request split/merge and uses significant CPU time.
>>>
>>> A simple idea is to calculate the range for each raid disk for an IO
>>> request and send a discard request to raid disks, which will avoid the
>>> split/merge completely. Previously Coly tried the approach, but the
>>> implementation was too complex because of raid0 zones. This patch always
>>> split bio in zone boundary and handle bio within one zone. It simplifies
>>> the implementation a lot.
>>>
>>> Cc: NeilBrown <neilb@suse.com>
>>> Cc: Coly Li <colyli@suse.de>
>>> Signed-off-by: Shaohua Li <shli@fb.com>
>>> ---
>>> drivers/md/raid0.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 102 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
>>> index 84e5859..d6c0bc7 100644
>>> --- a/drivers/md/raid0.c
>>> +++ b/drivers/md/raid0.c
>>> @@ -385,7 +385,7 @@ static int raid0_run(struct mddev *mddev)
>>> blk_queue_max_hw_sectors(mddev->queue, mddev->chunk_sectors);
>>> blk_queue_max_write_same_sectors(mddev->queue, mddev->chunk_sectors);
>>> blk_queue_max_write_zeroes_sectors(mddev->queue, mddev->chunk_sectors);
>>> - blk_queue_max_discard_sectors(mddev->queue, mddev->chunk_sectors);
>>> + blk_queue_max_discard_sectors(mddev->queue, UINT_MAX);
>>>
>>> blk_queue_io_min(mddev->queue, mddev->chunk_sectors << 9);
>>> blk_queue_io_opt(mddev->queue,
>>> @@ -459,6 +459,95 @@ static inline int is_io_in_chunk_boundary(struct mddev *mddev,
>>> }
>>> }
>>>
>>> +static void raid0_handle_discard(struct mddev *mddev, struct bio *bio)
>>> +{
>>> + struct r0conf *conf = mddev->private;
>>> + struct strip_zone *zone;
>>> + sector_t start = bio->bi_iter.bi_sector;
>>> + sector_t end;
>>> + unsigned int stripe_size;
>>> + sector_t first_stripe_index, last_stripe_index;
>>> + sector_t start_disk_offset;
>>> + unsigned int start_disk_index;
>>> + sector_t end_disk_offset;
>>> + unsigned int end_disk_index;
>>> + unsigned int disk;
>>> +
>>> + zone = find_zone(conf, &start);
>>> +
>>> + if (bio_end_sector(bio) > zone->zone_end) {
>>> + struct bio *split = bio_split(bio,
>>> + zone->zone_end - bio->bi_iter.bi_sector, GFP_NOIO,
>>> + mddev->bio_set);
>>> + bio_chain(split, bio);
>>> + generic_make_request(bio);
>>> + bio = split;
>>> + end = zone->zone_end;
>>> + } else
>>> + end = bio_end_sector(bio);
>>> +
>>
>> Nice, good idea. Indeed, I have a WIP patch which trys to follow your
>> idea, but no clean as yours.
>>
>>
>>> + if (zone != conf->strip_zone)
>>> + end = end - zone[-1].zone_end;
>>> +
>>> + /* Now start and end is the offset in zone */
>>> + stripe_size = zone->nb_dev * mddev->chunk_sectors;
>>> +
>>> + first_stripe_index = start;
>>> + sector_div(first_stripe_index, stripe_size);
>>> + last_stripe_index = end;
>>> + sector_div(last_stripe_index, stripe_size);
>>> +
>>> + start_disk_index = (int)(start - first_stripe_index * stripe_size) /
>>> + mddev->chunk_sectors;
>>> + start_disk_offset = ((int)(start - first_stripe_index * stripe_size) %
>>> + mddev->chunk_sectors) +
>>> + first_stripe_index * mddev->chunk_sectors;
>>> + end_disk_index = (int)(end - last_stripe_index * stripe_size) /
>>> + mddev->chunk_sectors;
>>> + end_disk_offset = ((int)(end - last_stripe_index * stripe_size) %
>>> + mddev->chunk_sectors) +
>>> + last_stripe_index * mddev->chunk_sectors;
>>> +
>>
>> Nice code, again! Much simpler. It is enjoyed experience when reading
>> these calculation :-)
>>
>>
>>> + for (disk = 0; disk < zone->nb_dev; disk++) {
>>> + sector_t dev_start, dev_end;
>>> + struct bio *discard_bio = NULL;
>>> + struct md_rdev *rdev;
>>> +
>>> + if (disk < start_disk_index)
>>> + dev_start = (first_stripe_index + 1) *
>>> + mddev->chunk_sectors;
>>> + else if (disk > start_disk_index)
>>> + dev_start = first_stripe_index * mddev->chunk_sectors;
>>> + else
>>> + dev_start = start_disk_offset;
>>> +
>>> + if (disk < end_disk_index)
>>> + dev_end = (last_stripe_index + 1) * mddev->chunk_sectors;
>>> + else if (disk > end_disk_index)
>>> + dev_end = last_stripe_index * mddev->chunk_sectors;
>>> + else
>>> + dev_end = end_disk_offset;
>>> +
>>> + if (dev_end <= dev_start)
>>> + continue;
>>> +
>>> + rdev = conf->devlist[(zone - conf->strip_zone) *
>>> + conf->strip_zone[0].nb_dev + disk];
>>> + if (__blkdev_issue_discard(rdev->bdev,
>>> + dev_start + zone->dev_start + rdev->data_offset,
>>> + dev_end - dev_start, GFP_NOIO, 0, &discard_bio) ||
>>> + !discard_bio)
>>> + continue;
>>
>> Could you please give me any hint ? Why __blkdev_issue_discard() is
>> called here. The following generic_make_request() will issue the bio to
>> underlying device and call __blkdev_issue_discard() in its code path
>> eventually, why call this function explicitly here?
>
> I was going to allocate a bio, init it and dispatch it to raid disks, but found
> __blkdev_issue_discard essentially does these, so reuse it. did you see any
> problem?
>
I don't see anything suspicious here. Just wondering is it mandatory to
call __blkdev_issue_discard() here. It's clear to me, thanks for your
explanation.
Coly
^ permalink raw reply
* Re: [PATCH] md/md0: optimize raid0 discard handling
From: Shaohua Li @ 2017-05-08 16:37 UTC (permalink / raw)
To: Coly Li; +Cc: Shaohua Li, linux-raid, Kernel-team, NeilBrown
In-Reply-To: <979e4da4-3d31-da5d-459f-0dceecc95a3b@suse.de>
On Mon, May 08, 2017 at 03:31:47PM +0800, Coly Li wrote:
> On 2017/5/8 上午8:36, Shaohua Li wrote:
> > There are complaints that raid0 discard handling is slow. Currently we
> > divide discard request into chunks and dispatch to underlayer disks. The
> > block layer will do merge to form big requests. This causes a lot of
> > request split/merge and uses significant CPU time.
> >
> > A simple idea is to calculate the range for each raid disk for an IO
> > request and send a discard request to raid disks, which will avoid the
> > split/merge completely. Previously Coly tried the approach, but the
> > implementation was too complex because of raid0 zones. This patch always
> > split bio in zone boundary and handle bio within one zone. It simplifies
> > the implementation a lot.
> >
> > Cc: NeilBrown <neilb@suse.com>
> > Cc: Coly Li <colyli@suse.de>
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> > drivers/md/raid0.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 102 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> > index 84e5859..d6c0bc7 100644
> > --- a/drivers/md/raid0.c
> > +++ b/drivers/md/raid0.c
> > @@ -385,7 +385,7 @@ static int raid0_run(struct mddev *mddev)
> > blk_queue_max_hw_sectors(mddev->queue, mddev->chunk_sectors);
> > blk_queue_max_write_same_sectors(mddev->queue, mddev->chunk_sectors);
> > blk_queue_max_write_zeroes_sectors(mddev->queue, mddev->chunk_sectors);
> > - blk_queue_max_discard_sectors(mddev->queue, mddev->chunk_sectors);
> > + blk_queue_max_discard_sectors(mddev->queue, UINT_MAX);
> >
> > blk_queue_io_min(mddev->queue, mddev->chunk_sectors << 9);
> > blk_queue_io_opt(mddev->queue,
> > @@ -459,6 +459,95 @@ static inline int is_io_in_chunk_boundary(struct mddev *mddev,
> > }
> > }
> >
> > +static void raid0_handle_discard(struct mddev *mddev, struct bio *bio)
> > +{
> > + struct r0conf *conf = mddev->private;
> > + struct strip_zone *zone;
> > + sector_t start = bio->bi_iter.bi_sector;
> > + sector_t end;
> > + unsigned int stripe_size;
> > + sector_t first_stripe_index, last_stripe_index;
> > + sector_t start_disk_offset;
> > + unsigned int start_disk_index;
> > + sector_t end_disk_offset;
> > + unsigned int end_disk_index;
> > + unsigned int disk;
> > +
> > + zone = find_zone(conf, &start);
> > +
> > + if (bio_end_sector(bio) > zone->zone_end) {
> > + struct bio *split = bio_split(bio,
> > + zone->zone_end - bio->bi_iter.bi_sector, GFP_NOIO,
> > + mddev->bio_set);
> > + bio_chain(split, bio);
> > + generic_make_request(bio);
> > + bio = split;
> > + end = zone->zone_end;
> > + } else
> > + end = bio_end_sector(bio);
> > +
>
> Nice, good idea. Indeed, I have a WIP patch which trys to follow your
> idea, but no clean as yours.
>
>
> > + if (zone != conf->strip_zone)
> > + end = end - zone[-1].zone_end;
> > +
> > + /* Now start and end is the offset in zone */
> > + stripe_size = zone->nb_dev * mddev->chunk_sectors;
> > +
> > + first_stripe_index = start;
> > + sector_div(first_stripe_index, stripe_size);
> > + last_stripe_index = end;
> > + sector_div(last_stripe_index, stripe_size);
> > +
> > + start_disk_index = (int)(start - first_stripe_index * stripe_size) /
> > + mddev->chunk_sectors;
> > + start_disk_offset = ((int)(start - first_stripe_index * stripe_size) %
> > + mddev->chunk_sectors) +
> > + first_stripe_index * mddev->chunk_sectors;
> > + end_disk_index = (int)(end - last_stripe_index * stripe_size) /
> > + mddev->chunk_sectors;
> > + end_disk_offset = ((int)(end - last_stripe_index * stripe_size) %
> > + mddev->chunk_sectors) +
> > + last_stripe_index * mddev->chunk_sectors;
> > +
>
> Nice code, again! Much simpler. It is enjoyed experience when reading
> these calculation :-)
>
>
> > + for (disk = 0; disk < zone->nb_dev; disk++) {
> > + sector_t dev_start, dev_end;
> > + struct bio *discard_bio = NULL;
> > + struct md_rdev *rdev;
> > +
> > + if (disk < start_disk_index)
> > + dev_start = (first_stripe_index + 1) *
> > + mddev->chunk_sectors;
> > + else if (disk > start_disk_index)
> > + dev_start = first_stripe_index * mddev->chunk_sectors;
> > + else
> > + dev_start = start_disk_offset;
> > +
> > + if (disk < end_disk_index)
> > + dev_end = (last_stripe_index + 1) * mddev->chunk_sectors;
> > + else if (disk > end_disk_index)
> > + dev_end = last_stripe_index * mddev->chunk_sectors;
> > + else
> > + dev_end = end_disk_offset;
> > +
> > + if (dev_end <= dev_start)
> > + continue;
> > +
> > + rdev = conf->devlist[(zone - conf->strip_zone) *
> > + conf->strip_zone[0].nb_dev + disk];
> > + if (__blkdev_issue_discard(rdev->bdev,
> > + dev_start + zone->dev_start + rdev->data_offset,
> > + dev_end - dev_start, GFP_NOIO, 0, &discard_bio) ||
> > + !discard_bio)
> > + continue;
>
> Could you please give me any hint ? Why __blkdev_issue_discard() is
> called here. The following generic_make_request() will issue the bio to
> underlying device and call __blkdev_issue_discard() in its code path
> eventually, why call this function explicitly here?
I was going to allocate a bio, init it and dispatch it to raid disks, but found
__blkdev_issue_discard essentially does these, so reuse it. did you see any
problem?
Thanks,
Shaohua
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Nix @ 2017-05-08 14:50 UTC (permalink / raw)
To: Wols Lists; +Cc: Ravi (Tom) Hale, linux-raid
In-Reply-To: <590DC905.60207@youngman.org.uk>
On 6 May 2017, Wols Lists outgrape:
> On 06/05/17 12:21, Ravi (Tom) Hale wrote:
>>> Bear in mind also, that any *within* *spec* drive can have an "accident"
>>> > every 10TB and still be considered perfectly okay. Which means that if
>>> > you do what you are supposed to do (rewrite the block) you're risking
>>> > the drive remapping the block - and getting closer to the drive bricking
>>> > itself. But if you trap the error yourself and add it to the badblocks
>>> > list, you are risking throwing away perfectly decent blocks that just
>>> > hiccuped.
>
>> For hiccups, having a bad-read-count for each suspected-bad block could
>> be sensible. If that number goes above <small-threshold> it's very
>> likely that the block is indeed bad and should be avoided in future.
>
> Except you have the second law of thermodynamics in play - "what man
> proposes, nature opposes". This could well screw up big time.
>
> DRAM memory needs to be refreshed by a read-write cycle every few
> nanoseconds. Hard drives are the same, actually, except that the
> interval is measured in years, not nanoseconds. Fill your brand new hard
> drive with data, then hammer it gently over a few years. Especially if a
> block's neighbours are repeatedly rewritten but this particular block is
> never touched, it is likely to become unreadable.
>
> So it will fail your test - reads will repeatedly fail - but if the
> firmware was given a look-in (by rewriting it) it wouldn't be remapped.
You mean it *would* be remapped (and all would be well).
I wonder... scrubbing is not very useful with md, particularly with RAID
6, because it does no writes unless something mismatches, and on failure
there is no attempt to determine which of the N disks is bad and rewrite
its contents from the other devices (nor, as I understand it, does it
clearly say which drive gave the error, so even failing it out and
resyncing it is hard).
If there was a way to get md to *rewrite* everything during scrub,
rather than just checking, this might help (in addition to letting the
drive refresh the magnetization of absolutely everything). "repair" mode
appears to do no writes until an error is found, whereupon (on RAID 6)
it proceeds to make a "repair" that is more likely than not to overwrite
good data with bad. Optionally writing what's already there on non-error
seems like it might be a worthwhile (and fairly simple) change.
--
NULL && (void)
^ permalink raw reply
* MD device tinning documentation
From: Vladimir Fedorkov @ 2017-05-08 10:22 UTC (permalink / raw)
To: linux-raid
Hello there!
I'm running benchmarks and it looks like that md device is only
utilizing single core which it limiting it's performance on concurrent
(4+ threads) writes with 8 SSD drives in software RAID10
Kernel: Linux host 4.10.12-1.el7.elrepo.x86_64 #1 SMP Fri Apr 21
09:58:15 EDT 2017 x86_64 x86_64 x86_64 GNU/Linux
Exact numbers are here: http://astellar.com/images/sysbench_rnd_writes.png
Could you please let me know if there is multi-threaded support on MD
devices in 4.x kernels and point me to the documentation (preferred)
or source code which describes tunable parts of /dev/mdXX devices.
Much appreciated!
Thank you!
---
Vlad
^ permalink raw reply
* [PATCH] md: don't return -EAGAIN in md_allow_write for external metadata arrays
From: Artur Paszkiewicz @ 2017-05-08 9:56 UTC (permalink / raw)
To: shli; +Cc: linux-raid, Artur Paszkiewicz
This essentially reverts commit b5470dc5fc18 ("md: resolve external
metadata handling deadlock in md_allow_write") with some adjustments.
Since commit 6791875e2e53 ("md: make reconfig_mutex optional for writes
to md sysfs files.") changing array_state to 'active' does not use
mddev_lock() and will not cause a deadlock with md_allow_write(). This
revert simplifies userspace tools that write to sysfs attributes like
"stripe_cache_size" or "consistency_policy" because it removes the need
for special handling for external metadata arrays, checking for EAGAIN
and retrying the write.
Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
drivers/md/md.c | 20 ++++++++------------
drivers/md/md.h | 2 +-
drivers/md/raid1.c | 9 +++------
drivers/md/raid5.c | 12 +++---------
4 files changed, 15 insertions(+), 28 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 82f798be964f..10367ffe92e3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8022,18 +8022,15 @@ EXPORT_SYMBOL(md_write_end);
* may proceed without blocking. It is important to call this before
* attempting a GFP_KERNEL allocation while holding the mddev lock.
* Must be called with mddev_lock held.
- *
- * In the ->external case MD_SB_CHANGE_PENDING can not be cleared until mddev->lock
- * is dropped, so return -EAGAIN after notifying userspace.
*/
-int md_allow_write(struct mddev *mddev)
+void md_allow_write(struct mddev *mddev)
{
if (!mddev->pers)
- return 0;
+ return;
if (mddev->ro)
- return 0;
+ return;
if (!mddev->pers->sync_request)
- return 0;
+ return;
spin_lock(&mddev->lock);
if (mddev->in_sync) {
@@ -8046,13 +8043,12 @@ int md_allow_write(struct mddev *mddev)
spin_unlock(&mddev->lock);
md_update_sb(mddev, 0);
sysfs_notify_dirent_safe(mddev->sysfs_state);
+ /* wait for the dirty state to be recorded in the metadata */
+ wait_event(mddev->sb_wait,
+ !test_bit(MD_SB_CHANGE_CLEAN, &mddev->sb_flags) &&
+ !test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags));
} else
spin_unlock(&mddev->lock);
-
- if (test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags))
- return -EAGAIN;
- else
- return 0;
}
EXPORT_SYMBOL_GPL(md_allow_write);
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 4e75d121bfcc..11f15146ce51 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -665,7 +665,7 @@ extern int sync_page_io(struct md_rdev *rdev, sector_t sector, int size,
bool metadata_op);
extern void md_do_sync(struct md_thread *thread);
extern void md_new_event(struct mddev *mddev);
-extern int md_allow_write(struct mddev *mddev);
+extern void md_allow_write(struct mddev *mddev);
extern void md_wait_for_blocked_rdev(struct md_rdev *rdev, struct mddev *mddev);
extern void md_set_array_sectors(struct mddev *mddev, sector_t array_sectors);
extern int md_check_no_bitmap(struct mddev *mddev);
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 7ed59351fe97..7c1f73398800 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -3197,7 +3197,7 @@ static int raid1_reshape(struct mddev *mddev)
struct r1conf *conf = mddev->private;
int cnt, raid_disks;
unsigned long flags;
- int d, d2, err;
+ int d, d2;
/* Cannot change chunk_size, layout, or level */
if (mddev->chunk_sectors != mddev->new_chunk_sectors ||
@@ -3209,11 +3209,8 @@ static int raid1_reshape(struct mddev *mddev)
return -EINVAL;
}
- if (!mddev_is_clustered(mddev)) {
- err = md_allow_write(mddev);
- if (err)
- return err;
- }
+ if (!mddev_is_clustered(mddev))
+ md_allow_write(mddev);
raid_disks = mddev->raid_disks + mddev->delta_disks;
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 3809a2192132..f8055a7abb4b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2309,14 +2309,12 @@ static int resize_stripes(struct r5conf *conf, int newsize)
struct stripe_head *osh, *nsh;
LIST_HEAD(newstripes);
struct disk_info *ndisks;
- int err;
+ int err = 0;
struct kmem_cache *sc;
int i;
int hash, cnt;
- err = md_allow_write(conf->mddev);
- if (err)
- return err;
+ md_allow_write(conf->mddev);
/* Step 1 */
sc = kmem_cache_create(conf->cache_name[1-conf->active_name],
@@ -6310,7 +6308,6 @@ int
raid5_set_cache_size(struct mddev *mddev, int size)
{
struct r5conf *conf = mddev->private;
- int err;
if (size <= 16 || size > 32768)
return -EINVAL;
@@ -6322,10 +6319,7 @@ raid5_set_cache_size(struct mddev *mddev, int size)
;
mutex_unlock(&conf->cache_size_mutex);
-
- err = md_allow_write(mddev);
- if (err)
- return err;
+ md_allow_write(mddev);
mutex_lock(&conf->cache_size_mutex);
while (size > conf->max_nr_stripes)
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] md/md0: optimize raid0 discard handling
From: Coly Li @ 2017-05-08 7:31 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, NeilBrown
In-Reply-To: <1af0237c374a1e276663fa17824e8345165fcca6.1494203680.git.shli@fb.com>
On 2017/5/8 上午8:36, Shaohua Li wrote:
> There are complaints that raid0 discard handling is slow. Currently we
> divide discard request into chunks and dispatch to underlayer disks. The
> block layer will do merge to form big requests. This causes a lot of
> request split/merge and uses significant CPU time.
>
> A simple idea is to calculate the range for each raid disk for an IO
> request and send a discard request to raid disks, which will avoid the
> split/merge completely. Previously Coly tried the approach, but the
> implementation was too complex because of raid0 zones. This patch always
> split bio in zone boundary and handle bio within one zone. It simplifies
> the implementation a lot.
>
> Cc: NeilBrown <neilb@suse.com>
> Cc: Coly Li <colyli@suse.de>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid0.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 102 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index 84e5859..d6c0bc7 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -385,7 +385,7 @@ static int raid0_run(struct mddev *mddev)
> blk_queue_max_hw_sectors(mddev->queue, mddev->chunk_sectors);
> blk_queue_max_write_same_sectors(mddev->queue, mddev->chunk_sectors);
> blk_queue_max_write_zeroes_sectors(mddev->queue, mddev->chunk_sectors);
> - blk_queue_max_discard_sectors(mddev->queue, mddev->chunk_sectors);
> + blk_queue_max_discard_sectors(mddev->queue, UINT_MAX);
>
> blk_queue_io_min(mddev->queue, mddev->chunk_sectors << 9);
> blk_queue_io_opt(mddev->queue,
> @@ -459,6 +459,95 @@ static inline int is_io_in_chunk_boundary(struct mddev *mddev,
> }
> }
>
> +static void raid0_handle_discard(struct mddev *mddev, struct bio *bio)
> +{
> + struct r0conf *conf = mddev->private;
> + struct strip_zone *zone;
> + sector_t start = bio->bi_iter.bi_sector;
> + sector_t end;
> + unsigned int stripe_size;
> + sector_t first_stripe_index, last_stripe_index;
> + sector_t start_disk_offset;
> + unsigned int start_disk_index;
> + sector_t end_disk_offset;
> + unsigned int end_disk_index;
> + unsigned int disk;
> +
> + zone = find_zone(conf, &start);
> +
> + if (bio_end_sector(bio) > zone->zone_end) {
> + struct bio *split = bio_split(bio,
> + zone->zone_end - bio->bi_iter.bi_sector, GFP_NOIO,
> + mddev->bio_set);
> + bio_chain(split, bio);
> + generic_make_request(bio);
> + bio = split;
> + end = zone->zone_end;
> + } else
> + end = bio_end_sector(bio);
> +
Nice, good idea. Indeed, I have a WIP patch which trys to follow your
idea, but no clean as yours.
> + if (zone != conf->strip_zone)
> + end = end - zone[-1].zone_end;
> +
> + /* Now start and end is the offset in zone */
> + stripe_size = zone->nb_dev * mddev->chunk_sectors;
> +
> + first_stripe_index = start;
> + sector_div(first_stripe_index, stripe_size);
> + last_stripe_index = end;
> + sector_div(last_stripe_index, stripe_size);
> +
> + start_disk_index = (int)(start - first_stripe_index * stripe_size) /
> + mddev->chunk_sectors;
> + start_disk_offset = ((int)(start - first_stripe_index * stripe_size) %
> + mddev->chunk_sectors) +
> + first_stripe_index * mddev->chunk_sectors;
> + end_disk_index = (int)(end - last_stripe_index * stripe_size) /
> + mddev->chunk_sectors;
> + end_disk_offset = ((int)(end - last_stripe_index * stripe_size) %
> + mddev->chunk_sectors) +
> + last_stripe_index * mddev->chunk_sectors;
> +
Nice code, again! Much simpler. It is enjoyed experience when reading
these calculation :-)
> + for (disk = 0; disk < zone->nb_dev; disk++) {
> + sector_t dev_start, dev_end;
> + struct bio *discard_bio = NULL;
> + struct md_rdev *rdev;
> +
> + if (disk < start_disk_index)
> + dev_start = (first_stripe_index + 1) *
> + mddev->chunk_sectors;
> + else if (disk > start_disk_index)
> + dev_start = first_stripe_index * mddev->chunk_sectors;
> + else
> + dev_start = start_disk_offset;
> +
> + if (disk < end_disk_index)
> + dev_end = (last_stripe_index + 1) * mddev->chunk_sectors;
> + else if (disk > end_disk_index)
> + dev_end = last_stripe_index * mddev->chunk_sectors;
> + else
> + dev_end = end_disk_offset;
> +
> + if (dev_end <= dev_start)
> + continue;
> +
> + rdev = conf->devlist[(zone - conf->strip_zone) *
> + conf->strip_zone[0].nb_dev + disk];
> + if (__blkdev_issue_discard(rdev->bdev,
> + dev_start + zone->dev_start + rdev->data_offset,
> + dev_end - dev_start, GFP_NOIO, 0, &discard_bio) ||
> + !discard_bio)
> + continue;
Could you please give me any hint ? Why __blkdev_issue_discard() is
called here. The following generic_make_request() will issue the bio to
underlying device and call __blkdev_issue_discard() in its code path
eventually, why call this function explicitly here?
> + bio_chain(discard_bio, bio);
> + if (mddev->gendisk)
> + trace_block_bio_remap(bdev_get_queue(rdev->bdev),
> + discard_bio, disk_devt(mddev->gendisk),
> + bio->bi_iter.bi_sector);
> + generic_make_request(discard_bio);
> + }
> + bio_endio(bio);
> +}
> +
> static void raid0_make_request(struct mddev *mddev, struct bio *bio)
> {
> struct strip_zone *zone;
> @@ -473,6 +562,11 @@ static void raid0_make_request(struct mddev *mddev, struct bio *bio)
> return;
> }
>
> + if (unlikely((bio_op(bio) == REQ_OP_DISCARD))) {
> + raid0_handle_discard(mddev, bio);
> + return;
> + }
> +
> bio_sector = bio->bi_iter.bi_sector;
> sector = bio_sector;
> chunk_sects = mddev->chunk_sectors;
> @@ -498,19 +592,13 @@ static void raid0_make_request(struct mddev *mddev, struct bio *bio)
> bio->bi_iter.bi_sector = sector + zone->dev_start +
> tmp_dev->data_offset;
>
> - if (unlikely((bio_op(bio) == REQ_OP_DISCARD) &&
> - !blk_queue_discard(bdev_get_queue(bio->bi_bdev)))) {
> - /* Just ignore it */
> - bio_endio(bio);
> - } else {
> - if (mddev->gendisk)
> - trace_block_bio_remap(bdev_get_queue(bio->bi_bdev),
> - bio, disk_devt(mddev->gendisk),
> - bio_sector);
> - mddev_check_writesame(mddev, bio);
> - mddev_check_write_zeroes(mddev, bio);
> - generic_make_request(bio);
> - }
> + if (mddev->gendisk)
> + trace_block_bio_remap(bdev_get_queue(bio->bi_bdev),
> + bio, disk_devt(mddev->gendisk),
> + bio_sector);
> + mddev_check_writesame(mddev, bio);
> + mddev_check_write_zeroes(mddev, bio);
> + generic_make_request(bio);
> }
>
> static void raid0_status(struct seq_file *seq, struct mddev *mddev)
>
In general I enjoy read this patch and learn nice idea from it. I'd like
to add an "Acked-by: Coly Li <colyli@suse.de", and waiting for your
response to my question.
Thanks.
Coly
^ permalink raw reply
* Re: [mdadm PATCH] Create: move STOP_ARRAY to abort_locked
From: Zhilong Liu @ 2017-05-08 1:50 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid
In-Reply-To: <be1df636-2bbb-2757-7a96-264d2d1a4b65@suse.com>
On 05/05/2017 11:31 AM, Liu Zhilong wrote:
>
>
> On 05/04/2017 10:54 PM, Jes Sorensen wrote:
>> On 05/04/2017 08:20 AM, Zhilong Liu wrote:
>>> Hi Jes,
>>>
>>> apply for review, this is a bug I ever encountered.
>>
>> Zhilong,
>>
>> Under what circumstances do you see this?
>>
>
> Issued the command:
> linux-g0sr:/home/test # ./mdadm -CR /dev/md0 -l1 -n2 -b internal
> /dev/loop[0-1] --size 63
> ... ... ...
> mdadm: Given bitmap chunk size not supported.
> linux-g0sr:/home/test # ls /dev/md0
> /dev/md0
> linux-g0sr:/home/test # ls /sys/block/md0/md/
> array_size bitmap component_size level metadata_version
> raid_disks reshape_position safe_mode_delay
> array_state chunk_size layout max_read_errors
> new_dev reshape_direction resync_start
>
> create_mddev() writes the devnm to
> /sys/module/md_mod/parameter/new_array,
> then in md.c, module_param_call() called the 'set' function of
> add_named_array(),
> md_alloc() init_and_add the kobject for devm, finally the devnm device
> has created
> and sysfs has registered after create_mddev executed successfully.
> Thus it's better
> to STOP_ARRAY in any case after create_mddev() invoked.
>
this patch depends on the kernel commit:
039b7225e6e9 ("md: allow creation of mdNNN arrays via
md_mod/parameters/new_array")
Neil's patch has set "mddev->hold_active = UNTIL_STOP", thus the
STOP_ARRAY can work
well on this situation.
Thanks,
-Zhilong
> Thanks,
> -Zhilong
>
>> Thanks,
>> Jes
>>
>>>
>>> On 04/26/2017 03:03 PM, Zhilong Liu wrote:
>>>> The sysfs entry and devnm would be created once create_mddev()
>>>> performed successfully, but the creating isn't completed here,
>>>> move STOP_ARRAY to abort_locked, the purpose is to cleanup the
>>>> partially created array.
>>>>
>>>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>>>> ---
>>>> Create.c | 11 ++++-------
>>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/Create.c b/Create.c
>>>> index 6ca0924..fe0ab7e 100644
>>>> --- a/Create.c
>>>> +++ b/Create.c
>>>> @@ -904,10 +904,8 @@ int Create(struct supertype *st, char *mddev,
>>>> remove_partitions(fd);
>>>> if (st->ss->add_to_super(st, &inf->disk,
>>>> fd, dv->devname,
>>>> - dv->data_offset)) {
>>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>>> + dv->data_offset))
>>>> goto abort_locked;
>>>> - }
>>>> st->ss->getinfo_super(st, inf, NULL);
>>>> safe_mode_delay = inf->safe_mode_delay;
>>>> @@ -1008,7 +1006,6 @@ int Create(struct supertype *st, char *mddev,
>>>> sysfs_set_safemode(&info, safe_mode_delay);
>>>> if (err) {
>>>> pr_err("failed to activate array.\n");
>>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>>> goto abort;
>>>> }
>>>> } else if (c->readonly &&
>>>> @@ -1018,7 +1015,6 @@ int Create(struct supertype *st, char *mddev,
>>>> "array_state", "readonly") < 0) {
>>>> pr_err("Failed to start array: %s\n",
>>>> strerror(errno));
>>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>>> goto abort;
>>>> }
>>>> } else {
>>>> @@ -1030,7 +1026,6 @@ int Create(struct supertype *st, char *mddev,
>>>> if (info.array.chunk_size &
>>>> (info.array.chunk_size-1)) {
>>>> cont_err("Problem may be that chunk size is not
>>>> a power of 2\n");
>>>> }
>>>> - ioctl(mdfd, STOP_ARRAY, NULL);
>>>> goto abort;
>>>> }
>>>> /* if start_ro module parameter is set, array is
>>>> @@ -1061,7 +1056,9 @@ int Create(struct supertype *st, char *mddev,
>>>> map_remove(&map, fd2devnm(mdfd));
>>>> map_unlock(&map);
>>>> - if (mdfd >= 0)
>>>> + if (mdfd >= 0) {
>>>> + ioctl(mdfd, STOP_ARRAY, NULL);
>>>> close(mdfd);
>>>> + }
>>>> return 1;
>>>> }
>>>
>>
>> --
>> 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
>>
>
> --
> 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
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