* Boot option raid=parttitionable and autodetection
From: Bostjan Skufca @ 2014-09-10 23:31 UTC (permalink / raw)
To: linux-raid
Hi all,
does kernel boot time parameter raid=partitionable ofer any additional
feature over not using it at all?
Besides changing autodetected device names from /dev/mdX to /dev/md_dX
I currently can not see any other difference, save lilo being unable
to work with them.
Normal raid devices can be partitioned without hassle, unfortunately
in-kernel autodetection of nested raid arrays does not work (talking
about raid0 over raid1 here, not raid10), but that is more or less
understandable.
Another quick question:
What is the status of in-kernel autodetection vs initramfs based one?
As I could gather from various sites it seems initramfs is preferable,
but what does that mean for in-kernel detection? Is it slowly being
phased out?
Tnx for info,
b.
^ permalink raw reply
* Raid 1 vs Raid 10 single thread performance
From: Bostjan Skufca @ 2014-09-10 21:24 UTC (permalink / raw)
To: linux-raid
Hi,
I have a simple question:
- Where is the code that is used for actual RAID 10 creation? In
kernel or in mdadm?
Explanation:
I was dissatisfied with single-threaded RAID 1 sequential read
performance (basically boils down to the speed of one disk). I figured
that instead of using level 1 I could create RAID level 10 and use two
equally-sized partitions on each drive (instead of one).
It turns out that if array is created properly, it is capable of
sequential reads at almost 2x single device speed, as expected (on
SSD!) and what would anyone expect from ordinary RAID 1.
What does "properly" actually mean?
I was doing some benchmarks with various raid configurations and
figured out that the order of devices submitted to creation command is
significant. It also makes raid10 created in such mode reliable or
unreliable to a device failure (not partition failure, device failure,
which means that two raid underlying devices fail at once).
Sum:
- if such array is created properly, it has redundancy in place and
performs as expected
- if not, it performs as raid1 and fails with one physical disk failure
I am trying to find the code responsible for creation of RAID 10 in
order to try and make it more inteligent about where to place RAID 10
parts if it gets a list of devices to use, and some of those devices
are on the same physical disks.
Thanks for hints,
b.
PS: More details about testing is available here, but be warned, it is
still a bit hectic to read:
http://blog.a2o.si/2014/09/07/linux-software-raid-why-you-should-always-use-raid-10-instead-of-raid-1/
^ permalink raw reply
* Add disks and convert level 0 to level 5
From: Michael Muratet @ 2014-09-10 16:15 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <20140901083137.4772c8c5@notabene.brown>
I have a two-disk RAID0 system that is working splendidly, thanks to the list for the help.
I managed to get my hands on more identical disks and since I have the disks and because I'm adding precious data, I'd like to add two more disks and grow to RAID5.
I have partitioned the two new drives to type 'fd', /dev/sde and /dev/sdf
I believe the command to accomplish the change is this:
mdadm /dev/md0 --grow --level=5 --add /dev/sde /dev/sdf
Following the old adage "measure twice, cut once", is this syntax correct? Is there any danger of data loss in such a conversion?
Thanks
Mike
^ permalink raw reply
* [patch v2 6/6] raid5: handle expansion/resync case with stripe batching
From: shli @ 2014-09-10 12:40 UTC (permalink / raw)
To: neilb, linux-raid
In-Reply-To: <20140910124009.810771309@kernel.org>
[-- Attachment #1: raid5-handle-resync-stripe-for-batching.patch --]
[-- Type: text/plain, Size: 3577 bytes --]
expansion/resync can grab a stripe when the stripe is in batch list. Since all
stripes in batch list must be in the same state, we can't allow some stripes
run into expansion/resync. So we delay expansion/resync for stripe in batch
list.
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
drivers/md/raid5.c | 22 +++++++++++++++-------
drivers/md/raid5.h | 5 +++++
2 files changed, 20 insertions(+), 7 deletions(-)
Index: linux/drivers/md/raid5.c
===================================================================
--- linux.orig/drivers/md/raid5.c 2014-09-10 19:21:16.759271053 +0800
+++ linux/drivers/md/raid5.c 2014-09-10 19:21:16.755271103 +0800
@@ -3371,8 +3371,10 @@ unhash:
struct stripe_head, batch_list);
list_del_init(&sh->batch_list);
- sh->state = head_sh->state & (~((1 << STRIPE_ACTIVE) |
- (1 << STRIPE_PREREAD_ACTIVE)));
+ set_mask_bits(&sh->state, ~STRIPE_EXPAND_SYNC_FLAG,
+ head_sh->state & (~((1 << STRIPE_ACTIVE) |
+ (1 << STRIPE_PREREAD_ACTIVE) |
+ STRIPE_EXPAND_SYNC_FLAG)));
sh->check_state = head_sh->check_state;
sh->reconstruct_state = head_sh->reconstruct_state;
for (i = 0; i < sh->disks; i++) {
@@ -3384,6 +3386,8 @@ unhash:
spin_lock_irq(&sh->stripe_lock);
sh->batch_head = NULL;
spin_unlock_irq(&sh->stripe_lock);
+ if (sh->state & STRIPE_EXPAND_SYNC_FLAG)
+ set_bit(STRIPE_HANDLE, &sh->state);
release_stripe(sh);
}
@@ -3391,6 +3395,8 @@ unhash:
head_sh->batch_head = NULL;
spin_unlock_irq(&head_sh->stripe_lock);
wake_up_nr(&conf->wait_for_overlap, wakeup_nr);
+ if (head_sh->state & STRIPE_EXPAND_SYNC_FLAG)
+ set_bit(STRIPE_HANDLE, &head_sh->state);
}
static void handle_stripe_dirtying(struct r5conf *conf,
@@ -3853,8 +3859,8 @@ static void analyse_stripe(struct stripe
memset(s, 0, sizeof(*s));
- s->expanding = test_bit(STRIPE_EXPAND_SOURCE, &sh->state);
- s->expanded = test_bit(STRIPE_EXPAND_READY, &sh->state);
+ s->expanding = test_bit(STRIPE_EXPAND_SOURCE, &sh->state) && !sh->batch_head;
+ s->expanded = test_bit(STRIPE_EXPAND_READY, &sh->state) && !sh->batch_head;
s->failed_num[0] = -1;
s->failed_num[1] = -1;
@@ -4076,8 +4082,10 @@ static void check_break_stripe_batch_lis
struct stripe_head, batch_list);
list_del_init(&sh->batch_list);
- sh->state = head_sh->state & (~((1 << STRIPE_ACTIVE) |
- (1 << STRIPE_PREREAD_ACTIVE) | (1 << STRIPE_DEGRADED)));
+ set_mask_bits(&sh->state, ~STRIPE_EXPAND_SYNC_FLAG,
+ head_sh->state & (~((1 << STRIPE_ACTIVE) |
+ (1 << STRIPE_PREREAD_ACTIVE) | (1 << STRIPE_DEGRADED) |
+ STRIPE_EXPAND_SYNC_FLAG)));
sh->check_state = head_sh->check_state;
sh->reconstruct_state = head_sh->reconstruct_state;
for (i = 0; i < sh->disks; i++)
@@ -4119,7 +4127,7 @@ static void handle_stripe(struct stripe_
check_break_stripe_batch_list(sh);
- if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state)) {
+ if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state) && !sh->batch_head) {
spin_lock(&sh->stripe_lock);
/* Cannot process 'sync' concurrently with 'discard' */
if (!test_bit(STRIPE_DISCARD, &sh->state) &&
Index: linux/drivers/md/raid5.h
===================================================================
--- linux.orig/drivers/md/raid5.h 2014-09-10 19:21:16.759271053 +0800
+++ linux/drivers/md/raid5.h 2014-09-10 19:21:16.755271103 +0800
@@ -339,6 +339,11 @@ enum {
STRIPE_BATCH_ERR,
};
+#define STRIPE_EXPAND_SYNC_FLAG \
+ ((1 << STRIPE_EXPAND_SOURCE) |\
+ (1 << STRIPE_EXPAND_READY) |\
+ (1 << STRIPE_EXPANDING) |\
+ (1 << STRIPE_SYNC_REQUESTED))
/*
* Operation request flags
*/
^ permalink raw reply
* [patch v2 5/6] raid5: handle io error of batch list
From: shli @ 2014-09-10 12:40 UTC (permalink / raw)
To: neilb, linux-raid
In-Reply-To: <20140910124009.810771309@kernel.org>
[-- Attachment #1: raid5-handle-batch-io-err.patch --]
[-- Type: text/plain, Size: 3070 bytes --]
If io error happens in any stripe of a batch list, the batch list will be
split, then normal process will run for the stripes in the list.
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
drivers/md/raid5.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/md/raid5.h | 1 +
2 files changed, 48 insertions(+)
Index: linux/drivers/md/raid5.c
===================================================================
--- linux.orig/drivers/md/raid5.c 2014-09-10 19:21:14.723296649 +0800
+++ linux/drivers/md/raid5.c 2014-09-10 19:21:14.715296750 +0800
@@ -1073,6 +1073,9 @@ again:
pr_debug("skip op %ld on disc %d for sector %llu\n",
bi->bi_rw, i, (unsigned long long)sh->sector);
clear_bit(R5_LOCKED, &sh->dev[i].flags);
+ if (sh->batch_head)
+ set_bit(STRIPE_BATCH_ERR,
+ &sh->batch_head->state);
set_bit(STRIPE_HANDLE, &sh->state);
}
@@ -2382,6 +2385,9 @@ static void raid5_end_write_request(stru
}
rdev_dec_pending(rdev, conf->mddev);
+ if (sh->batch_head && !uptodate)
+ set_bit(STRIPE_BATCH_ERR, &sh->batch_head->state);
+
if (!test_and_clear_bit(R5_DOUBLE_LOCKED, &sh->dev[i].flags))
clear_bit(R5_LOCKED, &sh->dev[i].flags);
set_bit(STRIPE_HANDLE, &sh->state);
@@ -4050,6 +4056,45 @@ static int clear_batch_ready(struct stri
return 0;
}
+static void check_break_stripe_batch_list(struct stripe_head *sh)
+{
+ struct stripe_head *head_sh, *next;
+ int i;
+
+ if (!test_and_clear_bit(STRIPE_BATCH_ERR, &sh->state))
+ return;
+
+ head_sh = sh;
+ do {
+ sh = list_first_entry(&sh->batch_list,
+ struct stripe_head, batch_list);
+ BUG_ON(sh == head_sh);
+ } while (!test_bit(STRIPE_DEGRADED, &sh->state));
+
+ while (sh != head_sh) {
+ next = list_first_entry(&sh->batch_list,
+ struct stripe_head, batch_list);
+ list_del_init(&sh->batch_list);
+
+ sh->state = head_sh->state & (~((1 << STRIPE_ACTIVE) |
+ (1 << STRIPE_PREREAD_ACTIVE) | (1 << STRIPE_DEGRADED)));
+ sh->check_state = head_sh->check_state;
+ sh->reconstruct_state = head_sh->reconstruct_state;
+ for (i = 0; i < sh->disks; i++)
+ sh->dev[i].flags = head_sh->dev[i].flags &
+ (~((1 << R5_WriteError) | (1 << R5_Overlap)));
+
+ spin_lock_irq(&sh->stripe_lock);
+ sh->batch_head = NULL;
+ spin_unlock_irq(&sh->stripe_lock);
+
+ set_bit(STRIPE_HANDLE, &sh->state);
+ release_stripe(sh);
+
+ sh = next;
+ }
+}
+
static void handle_stripe(struct stripe_head *sh)
{
struct stripe_head_state s;
@@ -4072,6 +4117,8 @@ static void handle_stripe(struct stripe_
return;
}
+ check_break_stripe_batch_list(sh);
+
if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state)) {
spin_lock(&sh->stripe_lock);
/* Cannot process 'sync' concurrently with 'discard' */
Index: linux/drivers/md/raid5.h
===================================================================
--- linux.orig/drivers/md/raid5.h 2014-09-10 19:21:14.723296649 +0800
+++ linux/drivers/md/raid5.h 2014-09-10 19:21:14.719296699 +0800
@@ -336,6 +336,7 @@ enum {
STRIPE_DISCARD,
STRIPE_ON_RELEASE_LIST,
STRIPE_BATCH_READY,
+ STRIPE_BATCH_ERR,
};
/*
^ permalink raw reply
* [patch v2 4/6] RAID5: batch adjacent full stripe write
From: shli @ 2014-09-10 12:40 UTC (permalink / raw)
To: neilb, linux-raid
In-Reply-To: <20140910124009.810771309@kernel.org>
[-- Attachment #1: raid5-batch-stripe.patch --]
[-- Type: text/plain, Size: 22844 bytes --]
stripe cache is 4k size. Even adjacent full stripe writes are handled in 4k
unit. Idealy we should use big size for adjacent full stripe writes. Bigger
stripe cache size means less stripes runing in the state machine so can reduce
cpu overhead. And also bigger size can cause bigger IO size dispatched to under
layer disks.
With below patch, we will automatically batch adjacent full stripe write
together. Such stripes will be added to the batch list. Only the first stripe
of the list will be put to handle_list and so run handle_stripe(). Some steps
of handle_stripe() are extended to cover all stripes of the list, including
ops_run_io, ops_run_biodrain and so on. With this patch, we have less stripes
running in handle_stripe() and we send IO of whole stripe list together to
increase IO size.
Stripes added to a batch list have some limitations. A batch list can only
include full stripe write and can't cross chunk boundary to make sure stripes
have the same parity disks. Stripes in a batch list must be in the same state
(no written, toread and so on). If a stripe is in a batch list, all new
read/write to add_stripe_bio will be blocked to overlap conflict till the batch
list is handled. The limitations will make sure stripes in a batch list be in
exactly the same state in the life circly.
I did test running 160k randwrite in a RAID5 array with 32k chunk size and 6
PCIe SSD. This patch improves around 30% performance and IO size to under layer
disk is exactly 32k. I also run a 4k randwrite test in the same array to make
sure the performance isn't changed with the patch.
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
drivers/md/raid5.c | 351 +++++++++++++++++++++++++++++++++++++++++++++++++----
drivers/md/raid5.h | 4
2 files changed, 331 insertions(+), 24 deletions(-)
Index: linux/drivers/md/raid5.c
===================================================================
--- linux.orig/drivers/md/raid5.c 2014-09-10 19:21:12.595323404 +0800
+++ linux/drivers/md/raid5.c 2014-09-10 19:21:12.591323456 +0800
@@ -526,6 +526,7 @@ static void init_stripe(struct stripe_he
BUG_ON(atomic_read(&sh->count) != 0);
BUG_ON(test_bit(STRIPE_HANDLE, &sh->state));
BUG_ON(stripe_operations_active(sh));
+ BUG_ON(sh->batch_head);
pr_debug("init_stripe called, stripe %llu\n",
(unsigned long long)sh->sector);
@@ -720,6 +721,124 @@ static bool is_full_stripe_write(struct
return sh->overwrite_disks == (sh->disks - sh->raid_conf->max_degraded);
}
+static void lock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
+{
+ local_irq_disable();
+ if (sh1 > sh2) {
+ spin_lock(&sh2->stripe_lock);
+ spin_lock_nested(&sh1->stripe_lock, 1);
+ } else {
+ spin_lock(&sh1->stripe_lock);
+ spin_lock_nested(&sh2->stripe_lock, 1);
+ }
+}
+
+static void unlock_two_stripes(struct stripe_head *sh1, struct stripe_head *sh2)
+{
+ spin_unlock(&sh1->stripe_lock);
+ spin_unlock(&sh2->stripe_lock);
+ local_irq_enable();
+}
+
+/* Only freshly new full stripe normal write stripe can be added to a batch list */
+static bool stripe_can_batch(struct stripe_head *sh)
+{
+ return test_bit(STRIPE_BATCH_READY, &sh->state) &&
+ is_full_stripe_write(sh);
+}
+
+/* we only do back search */
+static void stripe_add_to_batch_list(struct r5conf *conf, struct stripe_head *sh)
+{
+ struct stripe_head *head;
+ sector_t head_sector;
+ int hash;
+ int dd_idx;
+
+ if (!stripe_can_batch(sh))
+ return;
+ /* Don't cross chunks, so stripe pd_idx/qd_idx is the same */
+ if (!(sh->sector % conf->chunk_sectors))
+ return;
+ head_sector = sh->sector - STRIPE_SECTORS;
+
+ hash = stripe_hash_locks_hash(head_sector);
+ spin_lock_irq(conf->hash_locks + hash);
+ head = __find_stripe(conf, head_sector, conf->generation);
+ if (head && !atomic_inc_not_zero(&head->count)) {
+ spin_lock(&conf->device_lock);
+ if (!atomic_read(&head->count)) {
+ if (!test_bit(STRIPE_HANDLE, &head->state))
+ atomic_inc(&conf->active_stripes);
+ BUG_ON(list_empty(&head->lru) &&
+ !test_bit(STRIPE_EXPANDING, &head->state));
+ list_del_init(&head->lru);
+ if (head->group) {
+ head->group->stripes_cnt--;
+ head->group = NULL;
+ }
+ }
+ atomic_inc(&head->count);
+ spin_unlock(&conf->device_lock);
+ }
+ spin_unlock_irq(conf->hash_locks + hash);
+
+ if (!head)
+ return;
+ if (!stripe_can_batch(head))
+ goto out;
+
+ lock_two_stripes(head, sh);
+ /* clear_batch_ready clear the flag */
+ if (!stripe_can_batch(head) || !stripe_can_batch(sh))
+ goto unlock_out;
+
+ if (sh->batch_head)
+ goto unlock_out;
+
+ dd_idx = 0;
+ while (dd_idx == sh->pd_idx || dd_idx == sh->qd_idx)
+ dd_idx++;
+ if (head->dev[dd_idx].towrite->bi_rw != sh->dev[dd_idx].towrite->bi_rw)
+ goto unlock_out;
+
+ if (head->batch_head) {
+ spin_lock(&head->batch_head->batch_lock);
+ /* This batch list is already running */
+ if (!stripe_can_batch(head)) {
+ spin_unlock(&head->batch_head->batch_lock);
+ goto unlock_out;
+ }
+
+ /*
+ * at this point, head's BATCH_READY could be cleared, but we
+ * can still add the stripe to batch list
+ */
+ list_add(&sh->batch_list, &head->batch_list);
+ spin_unlock(&head->batch_head->batch_lock);
+
+ sh->batch_head = head->batch_head;
+ } else {
+ head->batch_head = head;
+ sh->batch_head = head->batch_head;
+ spin_lock(&head->batch_lock);
+ list_add_tail(&sh->batch_list, &head->batch_list);
+ spin_unlock(&head->batch_lock);
+ }
+
+ if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+ if (atomic_dec_return(&conf->preread_active_stripes)
+ < IO_THRESHOLD)
+ md_wakeup_thread(conf->mddev->thread);
+
+ atomic_inc(&sh->count);
+unlock_out:
+ unlock_two_stripes(head, sh);
+out:
+ release_stripe(head);
+}
+
+
/* Determine if 'data_offset' or 'new_data_offset' should be used
* in this stripe_head.
*/
@@ -750,6 +869,7 @@ static void ops_run_io(struct stripe_hea
{
struct r5conf *conf = sh->raid_conf;
int i, disks = sh->disks;
+ struct stripe_head *head_sh = sh;
might_sleep();
@@ -758,6 +878,8 @@ static void ops_run_io(struct stripe_hea
int replace_only = 0;
struct bio *bi, *rbi;
struct md_rdev *rdev, *rrdev = NULL;
+
+ sh = head_sh;
if (test_and_clear_bit(R5_Wantwrite, &sh->dev[i].flags)) {
if (test_and_clear_bit(R5_WantFUA, &sh->dev[i].flags))
rw = WRITE_FUA;
@@ -776,6 +898,7 @@ static void ops_run_io(struct stripe_hea
if (test_and_clear_bit(R5_SyncIO, &sh->dev[i].flags))
rw |= REQ_SYNC;
+again:
bi = &sh->dev[i].req;
rbi = &sh->dev[i].rreq; /* For writing to replacement */
@@ -794,7 +917,7 @@ static void ops_run_io(struct stripe_hea
/* We raced and saw duplicates */
rrdev = NULL;
} else {
- if (test_bit(R5_ReadRepl, &sh->dev[i].flags) && rrdev)
+ if (test_bit(R5_ReadRepl, &head_sh->dev[i].flags) && rrdev)
rdev = rrdev;
rrdev = NULL;
}
@@ -865,13 +988,15 @@ static void ops_run_io(struct stripe_hea
__func__, (unsigned long long)sh->sector,
bi->bi_rw, i);
atomic_inc(&sh->count);
+ if (sh != head_sh)
+ atomic_inc(&head_sh->count);
if (use_new_offset(conf, sh))
bi->bi_iter.bi_sector = (sh->sector
+ rdev->new_data_offset);
else
bi->bi_iter.bi_sector = (sh->sector
+ rdev->data_offset);
- if (test_bit(R5_ReadNoMerge, &sh->dev[i].flags))
+ if (test_bit(R5_ReadNoMerge, &head_sh->dev[i].flags))
bi->bi_rw |= REQ_NOMERGE;
if (test_bit(R5_SkipCopy, &sh->dev[i].flags))
@@ -915,6 +1040,8 @@ static void ops_run_io(struct stripe_hea
__func__, (unsigned long long)sh->sector,
rbi->bi_rw, i);
atomic_inc(&sh->count);
+ if (sh != head_sh)
+ atomic_inc(&head_sh->count);
if (use_new_offset(conf, sh))
rbi->bi_iter.bi_sector = (sh->sector
+ rrdev->new_data_offset);
@@ -948,6 +1075,13 @@ static void ops_run_io(struct stripe_hea
clear_bit(R5_LOCKED, &sh->dev[i].flags);
set_bit(STRIPE_HANDLE, &sh->state);
}
+
+ if (!head_sh->batch_head)
+ continue;
+ sh = list_first_entry(&sh->batch_list, struct stripe_head,
+ batch_list);
+ if (sh != head_sh)
+ goto again;
}
}
@@ -1063,6 +1197,7 @@ static void ops_run_biofill(struct strip
struct async_submit_ctl submit;
int i;
+ BUG_ON(sh->batch_head);
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
@@ -1152,6 +1287,8 @@ ops_run_compute5(struct stripe_head *sh,
struct async_submit_ctl submit;
int i;
+ BUG_ON(sh->batch_head);
+
pr_debug("%s: stripe %llu block: %d\n",
__func__, (unsigned long long)sh->sector, target);
BUG_ON(!test_bit(R5_Wantcompute, &tgt->flags));
@@ -1218,6 +1355,7 @@ ops_run_compute6_1(struct stripe_head *s
int i;
int count;
+ BUG_ON(sh->batch_head);
if (sh->ops.target < 0)
target = sh->ops.target2;
else if (sh->ops.target2 < 0)
@@ -1276,6 +1414,7 @@ ops_run_compute6_2(struct stripe_head *s
struct page **blocks = to_addr_page(sh, percpu, 0);
struct async_submit_ctl submit;
+ BUG_ON(sh->batch_head);
pr_debug("%s: stripe %llu block1: %d block2: %d\n",
__func__, (unsigned long long)sh->sector, target, target2);
BUG_ON(target < 0 || target2 < 0);
@@ -1389,6 +1528,7 @@ ops_run_prexor(struct stripe_head *sh, s
/* existing parity data subtracted */
struct page *xor_dest = xor_srcs[count++] = sh->dev[pd_idx].page;
+ BUG_ON(sh->batch_head);
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
@@ -1411,17 +1551,21 @@ ops_run_biodrain(struct stripe_head *sh,
{
int disks = sh->disks;
int i;
+ struct stripe_head *head_sh = sh;
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
for (i = disks; i--; ) {
- struct r5dev *dev = &sh->dev[i];
+ struct r5dev *dev;
struct bio *chosen;
- if (test_and_clear_bit(R5_Wantdrain, &dev->flags)) {
+ sh = head_sh;
+ if (test_and_clear_bit(R5_Wantdrain, &head_sh->dev[i].flags)) {
struct bio *wbi;
+again:
+ dev = &sh->dev[i];
spin_lock_irq(&sh->stripe_lock);
chosen = dev->towrite;
dev->towrite = NULL;
@@ -1450,6 +1594,14 @@ ops_run_biodrain(struct stripe_head *sh,
}
wbi = r5_next_bio(wbi, dev->sector);
}
+
+ if (head_sh->batch_head) {
+ sh = list_first_entry(&sh->batch_list,
+ struct stripe_head, batch_list);
+ if (sh == head_sh)
+ continue;
+ goto again;
+ }
}
}
@@ -1505,12 +1657,15 @@ ops_run_reconstruct5(struct stripe_head
struct dma_async_tx_descriptor *tx)
{
int disks = sh->disks;
- struct page **xor_srcs = to_addr_page(sh, percpu, 0);
+ struct page **xor_srcs;
struct async_submit_ctl submit;
- int count = 0, pd_idx = sh->pd_idx, i;
+ int count, pd_idx = sh->pd_idx, i;
struct page *xor_dest;
int prexor = 0;
unsigned long flags;
+ int j = 0;
+ struct stripe_head *head_sh = sh;
+ int last_stripe;
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
@@ -1527,15 +1682,18 @@ ops_run_reconstruct5(struct stripe_head
ops_complete_reconstruct(sh);
return;
}
+again:
+ count = 0;
+ xor_srcs = to_addr_page(sh, percpu, j);
/* check if prexor is active which means only process blocks
* that are part of a read-modify-write (written)
*/
- if (sh->reconstruct_state == reconstruct_state_prexor_drain_run) {
+ if (head_sh->reconstruct_state == reconstruct_state_prexor_drain_run) {
prexor = 1;
xor_dest = xor_srcs[count++] = sh->dev[pd_idx].page;
for (i = disks; i--; ) {
struct r5dev *dev = &sh->dev[i];
- if (dev->written)
+ if (head_sh->dev[i].written)
xor_srcs[count++] = dev->page;
}
} else {
@@ -1552,17 +1710,31 @@ ops_run_reconstruct5(struct stripe_head
* set ASYNC_TX_XOR_DROP_DST and ASYNC_TX_XOR_ZERO_DST
* for the synchronous xor case
*/
- flags = ASYNC_TX_ACK |
- (prexor ? ASYNC_TX_XOR_DROP_DST : ASYNC_TX_XOR_ZERO_DST);
-
- atomic_inc(&sh->count);
+ last_stripe = !head_sh->batch_head || list_first_entry(&sh->batch_list,
+ struct stripe_head, batch_list) == head_sh;
+ if (last_stripe) {
+ flags = ASYNC_TX_ACK |
+ (prexor ? ASYNC_TX_XOR_DROP_DST : ASYNC_TX_XOR_ZERO_DST);
+
+ atomic_inc(&head_sh->count);
+ init_async_submit(&submit, flags, tx, ops_complete_reconstruct, head_sh,
+ to_addr_conv(sh, percpu, j));
+ } else {
+ flags = prexor ? ASYNC_TX_XOR_DROP_DST : ASYNC_TX_XOR_ZERO_DST;
+ init_async_submit(&submit, flags, tx, NULL, NULL,
+ to_addr_conv(sh, percpu, j));
+ }
- init_async_submit(&submit, flags, tx, ops_complete_reconstruct, sh,
- to_addr_conv(sh, percpu, 0));
if (unlikely(count == 1))
tx = async_memcpy(xor_dest, xor_srcs[0], 0, 0, STRIPE_SIZE, &submit);
else
tx = async_xor(xor_dest, xor_srcs, 0, count, STRIPE_SIZE, &submit);
+ if (!last_stripe) {
+ j++;
+ sh = list_first_entry(&sh->batch_list, struct stripe_head,
+ batch_list);
+ goto again;
+ }
}
static void
@@ -1570,8 +1742,10 @@ ops_run_reconstruct6(struct stripe_head
struct dma_async_tx_descriptor *tx)
{
struct async_submit_ctl submit;
- struct page **blocks = to_addr_page(sh, percpu, 0);
- int count, i;
+ struct page **blocks;
+ int count, i, j = 0;
+ struct stripe_head *head_sh = sh;
+ int last_stripe;
pr_debug("%s: stripe %llu\n", __func__, (unsigned long long)sh->sector);
@@ -1589,13 +1763,26 @@ ops_run_reconstruct6(struct stripe_head
return;
}
+again:
+ blocks = to_addr_page(sh, percpu, j);
count = set_syndrome_sources(blocks, sh);
+ last_stripe = !head_sh->batch_head || list_first_entry(&sh->batch_list,
+ struct stripe_head, batch_list) == head_sh;
- atomic_inc(&sh->count);
-
- init_async_submit(&submit, ASYNC_TX_ACK, tx, ops_complete_reconstruct,
- sh, to_addr_conv(sh, percpu, 0));
+ if (last_stripe) {
+ atomic_inc(&head_sh->count);
+ init_async_submit(&submit, ASYNC_TX_ACK, tx, ops_complete_reconstruct,
+ head_sh, to_addr_conv(sh, percpu, j));
+ } else
+ init_async_submit(&submit, 0, tx, NULL, NULL,
+ to_addr_conv(sh, percpu, j));
async_gen_syndrome(blocks, 0, count+2, STRIPE_SIZE, &submit);
+ if (!last_stripe) {
+ j++;
+ sh = list_first_entry(&sh->batch_list, struct stripe_head,
+ batch_list);
+ goto again;
+ }
}
static void ops_complete_check(void *stripe_head_ref)
@@ -1625,6 +1812,7 @@ static void ops_run_check_p(struct strip
pr_debug("%s: stripe %llu\n", __func__,
(unsigned long long)sh->sector);
+ BUG_ON(sh->batch_head);
count = 0;
xor_dest = sh->dev[pd_idx].page;
xor_srcs[count++] = xor_dest;
@@ -1653,6 +1841,7 @@ static void ops_run_check_pq(struct stri
pr_debug("%s: stripe %llu checkp: %d\n", __func__,
(unsigned long long)sh->sector, checkp);
+ BUG_ON(sh->batch_head);
count = set_syndrome_sources(srcs, sh);
if (!checkp)
srcs[count] = NULL;
@@ -1720,7 +1909,7 @@ static void raid_run_ops(struct stripe_h
BUG();
}
- if (overlap_clear)
+ if (overlap_clear && !sh->batch_head)
for (i = disks; i--; ) {
struct r5dev *dev = &sh->dev[i];
if (test_and_clear_bit(R5_Overlap, &dev->flags))
@@ -1750,6 +1939,10 @@ static int grow_one_stripe(struct r5conf
atomic_set(&sh->count, 1);
atomic_inc(&conf->active_stripes);
INIT_LIST_HEAD(&sh->lru);
+
+ spin_lock_init(&sh->batch_lock);
+ INIT_LIST_HEAD(&sh->batch_list);
+ sh->batch_head = NULL;
release_stripe(sh);
return 1;
}
@@ -2193,6 +2386,9 @@ static void raid5_end_write_request(stru
clear_bit(R5_LOCKED, &sh->dev[i].flags);
set_bit(STRIPE_HANDLE, &sh->state);
release_stripe(sh);
+
+ if (sh->batch_head && sh != sh->batch_head)
+ release_stripe(sh->batch_head);
}
static sector_t compute_blocknr(struct stripe_head *sh, int i, int previous);
@@ -2682,6 +2878,9 @@ static int add_stripe_bio(struct stripe_
* protect it.
*/
spin_lock_irq(&sh->stripe_lock);
+ /* Don't allow new IO added to stripes in batch list */
+ if (sh->batch_head)
+ goto overlap;
if (forwrite) {
bip = &sh->dev[dd_idx].towrite;
if (*bip == NULL)
@@ -2731,6 +2930,9 @@ static int add_stripe_bio(struct stripe_
sh->bm_seq = conf->seq_flush+1;
set_bit(STRIPE_BIT_DELAY, &sh->state);
}
+
+ if (stripe_can_batch(sh))
+ stripe_add_to_batch_list(conf, sh);
return 1;
overlap:
@@ -2763,6 +2965,7 @@ handle_failed_stripe(struct r5conf *conf
struct bio **return_bi)
{
int i;
+ BUG_ON(sh->batch_head);
for (i = disks; i--; ) {
struct bio *bi;
int bitmap_end = 0;
@@ -2878,6 +3081,7 @@ handle_failed_sync(struct r5conf *conf,
int abort = 0;
int i;
+ BUG_ON(sh->batch_head);
clear_bit(STRIPE_SYNCING, &sh->state);
if (test_and_clear_bit(R5_Overlap, &sh->dev[sh->pd_idx].flags))
wake_up(&conf->wait_for_overlap);
@@ -3034,6 +3238,7 @@ static void handle_stripe_fill(struct st
{
int i;
+ BUG_ON(sh->batch_head);
/* look for blocks to read/compute, skip this if a compute
* is already in flight, or if the stripe contents are in the
* midst of changing due to a write
@@ -3058,6 +3263,9 @@ static void handle_stripe_clean_event(st
int i;
struct r5dev *dev;
int discard_pending = 0;
+ struct stripe_head *head_sh = sh;
+ bool do_endio = false;
+ int wakeup_nr = 0;
for (i = disks; i--; )
if (sh->dev[i].written) {
@@ -3073,8 +3281,11 @@ static void handle_stripe_clean_event(st
clear_bit(R5_UPTODATE, &dev->flags);
if (test_and_clear_bit(R5_SkipCopy, &dev->flags)) {
WARN_ON(test_bit(R5_UPTODATE, &dev->flags));
- dev->page = dev->orig_page;
}
+ do_endio = true;
+
+returnbi:
+ dev->page = dev->orig_page;
wbi = dev->written;
dev->written = NULL;
while (wbi && wbi->bi_iter.bi_sector <
@@ -3091,6 +3302,16 @@ static void handle_stripe_clean_event(st
STRIPE_SECTORS,
!test_bit(STRIPE_DEGRADED, &sh->state),
0);
+ if (head_sh->batch_head) {
+ sh = list_first_entry(&sh->batch_list,
+ struct stripe_head, batch_list);
+ if (sh != head_sh) {
+ dev = &sh->dev[i];
+ goto returnbi;
+ }
+ }
+ sh = head_sh;
+ dev = &sh->dev[i];
} else if (test_bit(R5_Discard, &dev->flags))
discard_pending = 1;
WARN_ON(test_bit(R5_SkipCopy, &dev->flags));
@@ -3112,8 +3333,17 @@ static void handle_stripe_clean_event(st
* will be reinitialized
*/
spin_lock_irq(&conf->device_lock);
+unhash:
remove_hash(sh);
+ if (head_sh->batch_head) {
+ sh = list_first_entry(&sh->batch_list,
+ struct stripe_head, batch_list);
+ if (sh != head_sh)
+ goto unhash;
+ }
spin_unlock_irq(&conf->device_lock);
+ sh = head_sh;
+
if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state))
set_bit(STRIPE_HANDLE, &sh->state);
@@ -3122,6 +3352,39 @@ static void handle_stripe_clean_event(st
if (test_and_clear_bit(STRIPE_FULL_WRITE, &sh->state))
if (atomic_dec_and_test(&conf->pending_full_writes))
md_wakeup_thread(conf->mddev->thread);
+
+ if (!head_sh->batch_head || !do_endio)
+ return;
+ for (i = 0; i < head_sh->disks; i++) {
+ if (test_and_clear_bit(R5_Overlap, &head_sh->dev[i].flags))
+ wakeup_nr++;
+ }
+ while (!list_empty(&head_sh->batch_list)) {
+ int i;
+ sh = list_first_entry(&head_sh->batch_list,
+ struct stripe_head, batch_list);
+ list_del_init(&sh->batch_list);
+
+ sh->state = head_sh->state & (~((1 << STRIPE_ACTIVE) |
+ (1 << STRIPE_PREREAD_ACTIVE)));
+ sh->check_state = head_sh->check_state;
+ sh->reconstruct_state = head_sh->reconstruct_state;
+ for (i = 0; i < sh->disks; i++) {
+ if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
+ wakeup_nr++;
+ sh->dev[i].flags = head_sh->dev[i].flags;
+ }
+
+ spin_lock_irq(&sh->stripe_lock);
+ sh->batch_head = NULL;
+ spin_unlock_irq(&sh->stripe_lock);
+ release_stripe(sh);
+ }
+
+ spin_lock_irq(&head_sh->stripe_lock);
+ head_sh->batch_head = NULL;
+ spin_unlock_irq(&head_sh->stripe_lock);
+ wake_up_nr(&conf->wait_for_overlap, wakeup_nr);
}
static void handle_stripe_dirtying(struct r5conf *conf,
@@ -3255,6 +3518,7 @@ static void handle_parity_checks5(struct
{
struct r5dev *dev = NULL;
+ BUG_ON(sh->batch_head);
set_bit(STRIPE_HANDLE, &sh->state);
switch (sh->check_state) {
@@ -3346,6 +3610,7 @@ static void handle_parity_checks6(struct
int qd_idx = sh->qd_idx;
struct r5dev *dev;
+ BUG_ON(sh->batch_head);
set_bit(STRIPE_HANDLE, &sh->state);
BUG_ON(s->failed > 2);
@@ -3509,6 +3774,7 @@ static void handle_stripe_expansion(stru
* copy some of them into a target stripe for expand.
*/
struct dma_async_tx_descriptor *tx = NULL;
+ BUG_ON(sh->batch_head);
clear_bit(STRIPE_EXPAND_SOURCE, &sh->state);
for (i = 0; i < sh->disks; i++)
if (i != sh->pd_idx && i != sh->qd_idx) {
@@ -3752,6 +4018,38 @@ static void analyse_stripe(struct stripe
rcu_read_unlock();
}
+static int clear_batch_ready(struct stripe_head *sh)
+{
+ struct stripe_head *tmp;
+ if (!test_and_clear_bit(STRIPE_BATCH_READY, &sh->state))
+ return 0;
+ spin_lock(&sh->stripe_lock);
+ if (!sh->batch_head) {
+ spin_unlock(&sh->stripe_lock);
+ return 0;
+ }
+
+ /*
+ * this stripe could be added to a batch list before we check
+ * BATCH_READY, skips it
+ */
+ if (sh->batch_head != sh) {
+ spin_unlock(&sh->stripe_lock);
+ return 1;
+ }
+ spin_lock(&sh->batch_lock);
+ list_for_each_entry(tmp, &sh->batch_list, batch_list)
+ clear_bit(STRIPE_BATCH_READY, &tmp->state);
+ spin_unlock(&sh->batch_lock);
+ spin_unlock(&sh->stripe_lock);
+
+ /*
+ * BATCH_READY is cleared, no new stripes can be added.
+ * batch_list can be accessed without lock
+ */
+ return 0;
+}
+
static void handle_stripe(struct stripe_head *sh)
{
struct stripe_head_state s;
@@ -3769,7 +4067,11 @@ static void handle_stripe(struct stripe_
return;
}
- clear_bit(STRIPE_BATCH_READY, &sh->state);
+ if (clear_batch_ready(sh) ) {
+ clear_bit_unlock(STRIPE_ACTIVE, &sh->state);
+ return;
+ }
+
if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state)) {
spin_lock(&sh->stripe_lock);
/* Cannot process 'sync' concurrently with 'discard' */
@@ -4770,7 +5072,8 @@ static void make_request(struct mddev *m
}
set_bit(STRIPE_HANDLE, &sh->state);
clear_bit(STRIPE_DELAYED, &sh->state);
- if ((bi->bi_rw & REQ_SYNC) &&
+ if ((!sh->batch_head || sh == sh->batch_head) &&
+ (bi->bi_rw & REQ_SYNC) &&
!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
atomic_inc(&conf->preread_active_stripes);
release_stripe_plug(mddev, sh);
Index: linux/drivers/md/raid5.h
===================================================================
--- linux.orig/drivers/md/raid5.h 2014-09-10 19:21:12.595323404 +0800
+++ linux/drivers/md/raid5.h 2014-09-10 19:21:12.591323456 +0800
@@ -219,6 +219,10 @@ struct stripe_head {
spinlock_t stripe_lock;
int cpu;
struct r5worker_group *group;
+
+ 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 stripe_operations
* @target - STRIPE_OP_COMPUTE_BLK target
^ permalink raw reply
* [patch v2 3/6] raid5: track overwrite disk count
From: shli @ 2014-09-10 12:40 UTC (permalink / raw)
To: neilb, linux-raid
In-Reply-To: <20140910124009.810771309@kernel.org>
[-- Attachment #1: raid5-track-full-stripe-write.patch --]
[-- Type: text/plain, Size: 3188 bytes --]
Track overwrite disk count, so we can know if a stripe is a full stripe write.
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
drivers/md/raid5.c | 14 +++++++++++++-
drivers/md/raid5.h | 4 ++++
2 files changed, 17 insertions(+), 1 deletion(-)
Index: linux/drivers/md/raid5.c
===================================================================
--- linux.orig/drivers/md/raid5.c 2014-09-10 19:21:10.495349803 +0800
+++ linux/drivers/md/raid5.c 2014-09-10 19:21:10.491349853 +0800
@@ -556,6 +556,7 @@ retry:
}
if (read_seqcount_retry(&conf->gen_lock, seq))
goto retry;
+ sh->overwrite_disks = 0;
insert_hash(conf, sh);
sh->cpu = smp_processor_id();
set_bit(STRIPE_BATCH_READY, &sh->state);
@@ -713,6 +714,12 @@ get_active_stripe(struct r5conf *conf, s
return sh;
}
+static bool is_full_stripe_write(struct stripe_head *sh)
+{
+ BUG_ON(sh->overwrite_disks > (sh->disks - sh->raid_conf->max_degraded));
+ return sh->overwrite_disks == (sh->disks - sh->raid_conf->max_degraded);
+}
+
/* Determine if 'data_offset' or 'new_data_offset' should be used
* in this stripe_head.
*/
@@ -1418,6 +1425,7 @@ ops_run_biodrain(struct stripe_head *sh,
spin_lock_irq(&sh->stripe_lock);
chosen = dev->towrite;
dev->towrite = NULL;
+ sh->overwrite_disks = 0;
BUG_ON(dev->written);
wbi = dev->written = chosen;
spin_unlock_irq(&sh->stripe_lock);
@@ -2708,7 +2716,8 @@ static int add_stripe_bio(struct stripe_
sector = bio_end_sector(bi);
}
if (sector >= sh->dev[dd_idx].sector + STRIPE_SECTORS)
- set_bit(R5_OVERWRITE, &sh->dev[dd_idx].flags);
+ if (!test_and_set_bit(R5_OVERWRITE, &sh->dev[dd_idx].flags))
+ sh->overwrite_disks++;
}
pr_debug("added bi b#%llu to stripe s#%llu, disk %d.\n",
@@ -2780,6 +2789,7 @@ handle_failed_stripe(struct r5conf *conf
/* fail all writes first */
bi = sh->dev[i].towrite;
sh->dev[i].towrite = NULL;
+ sh->overwrite_disks = 0;
spin_unlock_irq(&sh->stripe_lock);
if (bi)
bitmap_end = 1;
@@ -4576,12 +4586,14 @@ static void make_discard_request(struct
}
set_bit(STRIPE_DISCARD, &sh->state);
finish_wait(&conf->wait_for_overlap, &w);
+ sh->overwrite_disks = 0;
for (d = 0; d < conf->raid_disks; d++) {
if (d == sh->pd_idx || d == sh->qd_idx)
continue;
sh->dev[d].towrite = bi;
set_bit(R5_OVERWRITE, &sh->dev[d].flags);
raid5_inc_bi_active_stripes(bi);
+ sh->overwrite_disks++;
}
spin_unlock_irq(&sh->stripe_lock);
if (conf->mddev->bitmap) {
Index: linux/drivers/md/raid5.h
===================================================================
--- linux.orig/drivers/md/raid5.h 2014-09-10 19:21:10.495349803 +0800
+++ linux/drivers/md/raid5.h 2014-09-10 19:21:10.491349853 +0800
@@ -210,6 +210,10 @@ struct stripe_head {
atomic_t count; /* nr of active thread/requests */
int bm_seq; /* sequence number for bitmap flushes */
int disks; /* disks in stripe */
+ int overwrite_disks; /* total overwrite disks in stripe,
+ * this is only checked when stripe
+ * has STRIPE_BATCH_READY
+ */
enum check_states check_state;
enum reconstruct_states reconstruct_state;
spinlock_t stripe_lock;
^ permalink raw reply
* [patch v2 2/6] raid5: add a new flag to track if a stripe can be batched
From: shli @ 2014-09-10 12:40 UTC (permalink / raw)
To: neilb, linux-raid
In-Reply-To: <20140910124009.810771309@kernel.org>
[-- Attachment #1: raid5-add-new-flag-for-batching.patch --]
[-- Type: text/plain, Size: 2788 bytes --]
A freshly new stripe with write request can be batched. Any time the stripe is
handled or new read is queued, the flag will be cleared.
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
drivers/md/raid5.c | 12 +++++++++---
drivers/md/raid5.h | 1 +
2 files changed, 10 insertions(+), 3 deletions(-)
Index: linux/drivers/md/raid5.c
===================================================================
--- linux.orig/drivers/md/raid5.c 2014-09-10 19:21:08.183378868 +0800
+++ linux/drivers/md/raid5.c 2014-09-10 19:21:08.179378919 +0800
@@ -558,6 +558,7 @@ retry:
goto retry;
insert_hash(conf, sh);
sh->cpu = smp_processor_id();
+ set_bit(STRIPE_BATCH_READY, &sh->state);
}
static struct stripe_head *__find_stripe(struct r5conf *conf, sector_t sector,
@@ -2653,7 +2654,8 @@ schedule_reconstruction(struct stripe_he
* toread/towrite point to the first in a chain.
* The bi_next chain must be in order.
*/
-static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx, int forwrite)
+static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx,
+ int forwrite, int previous)
{
struct bio **bip;
struct r5conf *conf = sh->raid_conf;
@@ -2686,6 +2688,9 @@ static int add_stripe_bio(struct stripe_
if (*bip && (*bip)->bi_iter.bi_sector < bio_end_sector(bi))
goto overlap;
+ if (!forwrite || previous)
+ clear_bit(STRIPE_BATCH_READY, &sh->state);
+
BUG_ON(*bip && bi->bi_next && (*bip) != bi->bi_next);
if (*bip)
bi->bi_next = *bip;
@@ -3754,6 +3759,7 @@ static void handle_stripe(struct stripe_
return;
}
+ clear_bit(STRIPE_BATCH_READY, &sh->state);
if (test_bit(STRIPE_SYNC_REQUESTED, &sh->state)) {
spin_lock(&sh->stripe_lock);
/* Cannot process 'sync' concurrently with 'discard' */
@@ -4739,7 +4745,7 @@ static void make_request(struct mddev *m
}
if (test_bit(STRIPE_EXPANDING, &sh->state) ||
- !add_stripe_bio(sh, bi, dd_idx, rw)) {
+ !add_stripe_bio(sh, bi, dd_idx, rw, previous)) {
/* Stripe is busy expanding or
* add failed due to overlap. Flush everything
* and wait a while
@@ -5148,7 +5154,7 @@ static int retry_aligned_read(struct r5
return handled;
}
- if (!add_stripe_bio(sh, raid_bio, dd_idx, 0)) {
+ if (!add_stripe_bio(sh, raid_bio, dd_idx, 0, 0)) {
release_stripe(sh);
raid5_set_bi_processed_stripes(raid_bio, scnt);
conf->retry_read_aligned = raid_bio;
Index: linux/drivers/md/raid5.h
===================================================================
--- linux.orig/drivers/md/raid5.h 2014-09-10 19:21:08.183378868 +0800
+++ linux/drivers/md/raid5.h 2014-09-10 19:21:08.179378919 +0800
@@ -327,6 +327,7 @@ enum {
STRIPE_ON_UNPLUG_LIST,
STRIPE_DISCARD,
STRIPE_ON_RELEASE_LIST,
+ STRIPE_BATCH_READY,
};
/*
^ permalink raw reply
* [patch v2 1/6] raid5: use flex_array for scribble data
From: shli @ 2014-09-10 12:40 UTC (permalink / raw)
To: neilb, linux-raid
In-Reply-To: <20140910124009.810771309@kernel.org>
[-- Attachment #1: raid5-scribble-use-flex-array.patch --]
[-- Type: text/plain, Size: 12108 bytes --]
Use flex_array for scribble data. Next patch will batch several stripes
together, so scribble data should be able to cover several stripes, so this
patch also allocates scribble data for stripes across a chunk.
Signed-off-by: Shaohua Li <shli@fusionio.com>
---
drivers/md/raid5.c | 89 +++++++++++++++++++++++++++++++++--------------------
drivers/md/raid5.h | 6 ---
2 files changed, 57 insertions(+), 38 deletions(-)
Index: linux/drivers/md/raid5.c
===================================================================
--- linux.orig/drivers/md/raid5.c 2014-09-10 19:21:00.503475395 +0800
+++ linux/drivers/md/raid5.c 2014-09-10 19:21:00.499475454 +0800
@@ -54,6 +54,7 @@
#include <linux/slab.h>
#include <linux/ratelimit.h>
#include <linux/nodemask.h>
+#include <linux/flex_array.h>
#include <trace/events/block.h>
#include "md.h"
@@ -1112,16 +1113,29 @@ static void ops_complete_compute(void *s
/* return a pointer to the address conversion region of the scribble buffer */
static addr_conv_t *to_addr_conv(struct stripe_head *sh,
- struct raid5_percpu *percpu)
+ struct raid5_percpu *percpu, int i)
{
- return percpu->scribble + sizeof(struct page *) * (sh->disks + 2);
+ void *addr;
+
+ addr = flex_array_get(percpu->scribble, i);
+ return addr + sizeof(struct page *) * (sh->disks + 2);
+}
+
+/* return a pointer to the address conversion region of the scribble buffer */
+static struct page **to_addr_page(struct stripe_head *sh,
+ struct raid5_percpu *percpu, int i)
+{
+ void *addr;
+
+ addr = flex_array_get(percpu->scribble, i);
+ return addr;
}
static struct dma_async_tx_descriptor *
ops_run_compute5(struct stripe_head *sh, struct raid5_percpu *percpu)
{
int disks = sh->disks;
- struct page **xor_srcs = percpu->scribble;
+ struct page **xor_srcs = to_addr_page(sh, percpu, 0);
int target = sh->ops.target;
struct r5dev *tgt = &sh->dev[target];
struct page *xor_dest = tgt->page;
@@ -1141,7 +1155,7 @@ ops_run_compute5(struct stripe_head *sh,
atomic_inc(&sh->count);
init_async_submit(&submit, ASYNC_TX_FENCE|ASYNC_TX_XOR_ZERO_DST, NULL,
- ops_complete_compute, sh, to_addr_conv(sh, percpu));
+ ops_complete_compute, sh, to_addr_conv(sh, percpu, 0));
if (unlikely(count == 1))
tx = async_memcpy(xor_dest, xor_srcs[0], 0, 0, STRIPE_SIZE, &submit);
else
@@ -1186,7 +1200,7 @@ static struct dma_async_tx_descriptor *
ops_run_compute6_1(struct stripe_head *sh, struct raid5_percpu *percpu)
{
int disks = sh->disks;
- struct page **blocks = percpu->scribble;
+ struct page **blocks = to_addr_page(sh, percpu, 0);
int target;
int qd_idx = sh->qd_idx;
struct dma_async_tx_descriptor *tx;
@@ -1219,7 +1233,7 @@ ops_run_compute6_1(struct stripe_head *s
BUG_ON(blocks[count+1] != dest); /* q should already be set */
init_async_submit(&submit, ASYNC_TX_FENCE, NULL,
ops_complete_compute, sh,
- to_addr_conv(sh, percpu));
+ to_addr_conv(sh, percpu, 0));
tx = async_gen_syndrome(blocks, 0, count+2, STRIPE_SIZE, &submit);
} else {
/* Compute any data- or p-drive using XOR */
@@ -1232,7 +1246,7 @@ ops_run_compute6_1(struct stripe_head *s
init_async_submit(&submit, ASYNC_TX_FENCE|ASYNC_TX_XOR_ZERO_DST,
NULL, ops_complete_compute, sh,
- to_addr_conv(sh, percpu));
+ to_addr_conv(sh, percpu, 0));
tx = async_xor(dest, blocks, 0, count, STRIPE_SIZE, &submit);
}
@@ -1251,7 +1265,7 @@ ops_run_compute6_2(struct stripe_head *s
struct r5dev *tgt = &sh->dev[target];
struct r5dev *tgt2 = &sh->dev[target2];
struct dma_async_tx_descriptor *tx;
- struct page **blocks = percpu->scribble;
+ struct page **blocks = to_addr_page(sh, percpu, 0);
struct async_submit_ctl submit;
pr_debug("%s: stripe %llu block1: %d block2: %d\n",
@@ -1293,7 +1307,7 @@ ops_run_compute6_2(struct stripe_head *s
/* Missing P+Q, just recompute */
init_async_submit(&submit, ASYNC_TX_FENCE, NULL,
ops_complete_compute, sh,
- to_addr_conv(sh, percpu));
+ to_addr_conv(sh, percpu, 0));
return async_gen_syndrome(blocks, 0, syndrome_disks+2,
STRIPE_SIZE, &submit);
} else {
@@ -1317,21 +1331,21 @@ ops_run_compute6_2(struct stripe_head *s
init_async_submit(&submit,
ASYNC_TX_FENCE|ASYNC_TX_XOR_ZERO_DST,
NULL, NULL, NULL,
- to_addr_conv(sh, percpu));
+ to_addr_conv(sh, percpu, 0));
tx = async_xor(dest, blocks, 0, count, STRIPE_SIZE,
&submit);
count = set_syndrome_sources(blocks, sh);
init_async_submit(&submit, ASYNC_TX_FENCE, tx,
ops_complete_compute, sh,
- to_addr_conv(sh, percpu));
+ to_addr_conv(sh, percpu, 0));
return async_gen_syndrome(blocks, 0, count+2,
STRIPE_SIZE, &submit);
}
} else {
init_async_submit(&submit, ASYNC_TX_FENCE, NULL,
ops_complete_compute, sh,
- to_addr_conv(sh, percpu));
+ to_addr_conv(sh, percpu, 0));
if (failb == syndrome_disks) {
/* We're missing D+P. */
return async_raid6_datap_recov(syndrome_disks+2,
@@ -1360,7 +1374,7 @@ ops_run_prexor(struct stripe_head *sh, s
struct dma_async_tx_descriptor *tx)
{
int disks = sh->disks;
- struct page **xor_srcs = percpu->scribble;
+ struct page **xor_srcs = to_addr_page(sh, percpu, 0);
int count = 0, pd_idx = sh->pd_idx, i;
struct async_submit_ctl submit;
@@ -1378,7 +1392,7 @@ ops_run_prexor(struct stripe_head *sh, s
}
init_async_submit(&submit, ASYNC_TX_FENCE|ASYNC_TX_XOR_DROP_DST, tx,
- ops_complete_prexor, sh, to_addr_conv(sh, percpu));
+ ops_complete_prexor, sh, to_addr_conv(sh, percpu, 0));
tx = async_xor(xor_dest, xor_srcs, 0, count, STRIPE_SIZE, &submit);
return tx;
@@ -1482,7 +1496,7 @@ ops_run_reconstruct5(struct stripe_head
struct dma_async_tx_descriptor *tx)
{
int disks = sh->disks;
- struct page **xor_srcs = percpu->scribble;
+ struct page **xor_srcs = to_addr_page(sh, percpu, 0);
struct async_submit_ctl submit;
int count = 0, pd_idx = sh->pd_idx, i;
struct page *xor_dest;
@@ -1535,7 +1549,7 @@ ops_run_reconstruct5(struct stripe_head
atomic_inc(&sh->count);
init_async_submit(&submit, flags, tx, ops_complete_reconstruct, sh,
- to_addr_conv(sh, percpu));
+ to_addr_conv(sh, percpu, 0));
if (unlikely(count == 1))
tx = async_memcpy(xor_dest, xor_srcs[0], 0, 0, STRIPE_SIZE, &submit);
else
@@ -1547,7 +1561,7 @@ ops_run_reconstruct6(struct stripe_head
struct dma_async_tx_descriptor *tx)
{
struct async_submit_ctl submit;
- struct page **blocks = percpu->scribble;
+ struct page **blocks = to_addr_page(sh, percpu, 0);
int count, i;
pr_debug("%s: stripe %llu\n", __func__, (unsigned long long)sh->sector);
@@ -1571,7 +1585,7 @@ ops_run_reconstruct6(struct stripe_head
atomic_inc(&sh->count);
init_async_submit(&submit, ASYNC_TX_ACK, tx, ops_complete_reconstruct,
- sh, to_addr_conv(sh, percpu));
+ sh, to_addr_conv(sh, percpu, 0));
async_gen_syndrome(blocks, 0, count+2, STRIPE_SIZE, &submit);
}
@@ -1593,7 +1607,7 @@ static void ops_run_check_p(struct strip
int pd_idx = sh->pd_idx;
int qd_idx = sh->qd_idx;
struct page *xor_dest;
- struct page **xor_srcs = percpu->scribble;
+ struct page **xor_srcs = to_addr_page(sh, percpu, 0);
struct dma_async_tx_descriptor *tx;
struct async_submit_ctl submit;
int count;
@@ -1612,7 +1626,7 @@ static void ops_run_check_p(struct strip
}
init_async_submit(&submit, 0, NULL, NULL, NULL,
- to_addr_conv(sh, percpu));
+ to_addr_conv(sh, percpu, 0));
tx = async_xor_val(xor_dest, xor_srcs, 0, count, STRIPE_SIZE,
&sh->ops.zero_sum_result, &submit);
@@ -1623,7 +1637,7 @@ static void ops_run_check_p(struct strip
static void ops_run_check_pq(struct stripe_head *sh, struct raid5_percpu *percpu, int checkp)
{
- struct page **srcs = percpu->scribble;
+ struct page **srcs = to_addr_page(sh, percpu, 0);
struct async_submit_ctl submit;
int count;
@@ -1636,7 +1650,7 @@ static void ops_run_check_pq(struct stri
atomic_inc(&sh->count);
init_async_submit(&submit, ASYNC_TX_ACK, NULL, ops_complete_check,
- sh, to_addr_conv(sh, percpu));
+ sh, to_addr_conv(sh, percpu, 0));
async_syndrome_val(srcs, 0, count+2, STRIPE_SIZE,
&sh->ops.zero_sum_result, percpu->spare_page, &submit);
}
@@ -1776,13 +1790,21 @@ static int grow_stripes(struct r5conf *c
* calculate over all devices (not just the data blocks), using zeros in place
* of the P and Q blocks.
*/
-static size_t scribble_len(int num)
+static struct flex_array *scribble_alloc(int num, int cnt, gfp_t flags)
{
+ struct flex_array *ret;
size_t len;
len = sizeof(struct page *) * (num+2) + sizeof(addr_conv_t) * (num+2);
-
- return len;
+ ret = flex_array_alloc(len, cnt, flags);
+ if (!ret)
+ return NULL;
+ /* always prealloc all elements, so no locking is required */
+ if (flex_array_prealloc(ret, 0, cnt, flags)) {
+ flex_array_free(ret);
+ return NULL;
+ }
+ return ret;
}
static int resize_stripes(struct r5conf *conf, int newsize)
@@ -1900,16 +1922,16 @@ static int resize_stripes(struct r5conf
err = -ENOMEM;
get_online_cpus();
- conf->scribble_len = scribble_len(newsize);
for_each_present_cpu(cpu) {
struct raid5_percpu *percpu;
- void *scribble;
+ struct flex_array *scribble;
percpu = per_cpu_ptr(conf->percpu, cpu);
- scribble = kmalloc(conf->scribble_len, GFP_NOIO);
+ scribble = scribble_alloc(newsize, conf->chunk_sectors /
+ STRIPE_SECTORS, GFP_NOIO);
if (scribble) {
- kfree(percpu->scribble);
+ flex_array_free(percpu->scribble);
percpu->scribble = scribble;
} else {
err = -ENOMEM;
@@ -5610,7 +5632,7 @@ raid5_size(struct mddev *mddev, sector_t
static void free_scratch_buffer(struct r5conf *conf, struct raid5_percpu *percpu)
{
safe_put_page(percpu->spare_page);
- kfree(percpu->scribble);
+ flex_array_free(percpu->scribble);
percpu->spare_page = NULL;
percpu->scribble = NULL;
}
@@ -5620,7 +5642,9 @@ static int alloc_scratch_buffer(struct r
if (conf->level == 6 && !percpu->spare_page)
percpu->spare_page = alloc_page(GFP_KERNEL);
if (!percpu->scribble)
- percpu->scribble = kmalloc(conf->scribble_len, GFP_KERNEL);
+ percpu->scribble = scribble_alloc(max(conf->raid_disks,
+ conf->previous_raid_disks), conf->chunk_sectors /
+ STRIPE_SECTORS, GFP_KERNEL);
if (!percpu->scribble || (conf->level == 6 && !percpu->spare_page)) {
free_scratch_buffer(conf, percpu);
@@ -5790,7 +5814,6 @@ static struct r5conf *setup_conf(struct
else
conf->previous_raid_disks = mddev->raid_disks - mddev->delta_disks;
max_disks = max(conf->raid_disks, conf->previous_raid_disks);
- conf->scribble_len = scribble_len(max_disks);
conf->disks = kzalloc(max_disks * sizeof(struct disk_info),
GFP_KERNEL);
@@ -5818,6 +5841,7 @@ static struct r5conf *setup_conf(struct
INIT_LIST_HEAD(conf->temp_inactive_list + i);
conf->level = mddev->new_level;
+ conf->chunk_sectors = mddev->new_chunk_sectors;
if (raid5_alloc_percpu(conf) != 0)
goto abort;
@@ -5850,7 +5874,6 @@ static struct r5conf *setup_conf(struct
conf->fullsync = 1;
}
- conf->chunk_sectors = mddev->new_chunk_sectors;
conf->level = mddev->new_level;
if (conf->level == 6)
conf->max_degraded = 2;
Index: linux/drivers/md/raid5.h
===================================================================
--- linux.orig/drivers/md/raid5.h 2014-09-10 19:21:00.503475395 +0800
+++ linux/drivers/md/raid5.h 2014-09-10 19:21:00.499475454 +0800
@@ -459,15 +459,11 @@ struct r5conf {
/* per cpu variables */
struct raid5_percpu {
struct page *spare_page; /* Used when checking P/Q in raid6 */
- void *scribble; /* space for constructing buffer
+ struct flex_array *scribble; /* space for constructing buffer
* lists and performing address
* conversions
*/
} __percpu *percpu;
- size_t scribble_len; /* size of scribble region must be
- * associated with conf to handle
- * cpu hotplug while reshaping
- */
#ifdef CONFIG_HOTPLUG_CPU
struct notifier_block cpu_notify;
#endif
^ permalink raw reply
* [patch v2 0/6] raid5: automatically batch adjacent full stripe write
From: shli @ 2014-09-10 12:40 UTC (permalink / raw)
To: neilb, linux-raid
This is the 2nd attempt to make adjacent full stripe write together. The main
change against v1 is how to detect if stripes can be batched. Also some bugs
are fixed.
Thanks,
Shaohua
^ permalink raw reply
* Re: Question about RAID1 plug/unplug code
From: NeilBrown @ 2014-09-10 9:36 UTC (permalink / raw)
To: Alexander Lyakas; +Cc: linux-raid, Yair Hershko
In-Reply-To: <CAGRgLy6iLnxY-gHELBXhd4uymM3CoPVFM5yWdRpQC9Hk7vnf4g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7098 bytes --]
On Wed, 10 Sep 2014 11:01:30 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
wrote:
> Hello Neil,
>
> On Tue, Sep 9, 2014 at 12:45 PM, NeilBrown <neilb@suse.de> wrote:
> > On Tue, 9 Sep 2014 11:33:13 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> > wrote:
> >
> >> Hi Neil,
> >>
> >>
> >> On Tue, Sep 9, 2014 at 4:45 AM, NeilBrown <neilb@suse.de> wrote:
> >> > On Mon, 8 Sep 2014 16:55:52 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> >> > wrote:
> >> >
> >> >> Hi Neil,
> >> >> We have been seeing high latency on the md/raid1 block device, due to
> >> >> the fact that all WRITEs are handed off to raid1d thread. This thread
> >> >> also calls bitmap_unplug(), which writes the bitmap synchronously.
> >> >> While it waits for the bitmap, it cannot trigger other WRITEs waiting
> >> >> in its pending_bio_list. This is especially seen with SSDs: MD's
> >> >> latency is much higher that SSD latency (I have been stoned by Peter
> >> >> Grandi when I brought up this issue previously for raid5).
> >> >>
> >> >> Then I have noticed the commit:
> >> >>
> >> >> commit f54a9d0e59c4bea3db733921ca9147612a6f292c
> >> >> Author: NeilBrown <neilb@suse.de>
> >> >> Date: Thu Aug 2 08:33:20 2012 +1000
> >> >>
> >> >> md/raid1: submit IO from originating thread instead of md thread.
> >> >>
> >> >> Looking at the code, I learned that to avoid switching into raid1d,
> >> >> the caller has to use blk_start_plug/blk_finish_plug. So I added these
> >> >> calls in our kernel module, which submits bios to MD. Results were
> >> >> awesome, MD latency got down significantly.
> >> >
> >> > That's good to hear.
> >> >
> >> >>
> >> >> So I have several questions about this plug/unplug thing.
> >> >>
> >> >> 1/ Originally this infrastructure was supposed to help IO schedulers
> >> >> in merging requests. It is useful when one has a bunch of requests to
> >> >> submit in one shot.
> >> >
> >> > That is exactly the whole point of plugging: allow the device to handle a
> >> > batch of requests together instead of one at a time.
> >> >
> >> >> But in MD case, thus infrastructure is used for a different purpose:
> >> >> not to merge requests (which may help bandwidth, but probably not
> >> >> latency), but to avoid making raid1d a bottleneck, to be able to
> >> >> submit requests from multiple threads in parallel, which brings down
> >> >> latency significantly in our case. Indeed "struct blk_plug" has a
> >> >> special "cb_list", which is used only by MD.
> >> >
> >> > I don't think the way md uses plugging is conceptually different from any
> >> > other use: it is always about gathering a batch together.
> >> > "cb_list" is handled by blk_check_plugged() which is also used by
> >> > block/umem.c and btrfs.
> >> >
> >> > The base plugging code assumes that it is only gathering a batch of requests
> >> > for a single device - if the target device changes then the batch is flushed.
> >> > It also assumed that it was "struct request" that was batched.
> >> > Devices like md that want to queue 'struct bio', something else was needed.
> >> > Also with layered devices it can be useful to gather multiple batches for
> >> > multiple layers.
> >> > So I created "cb_list" etc and a more generic interface.
> >> >
> >> >> In my case I have only individual bios (not a bunch of bios), and I
> >> >> after wrap them with plug/unplug, MD latency gets better. So we are
> >> >> using the plug infrastructure for a different purpose.
> >> >> Is my understanding correct? Was this your intention?
> >> >
> >> > I don't really understand what you are doing. There is no point in using
> >> > plugging for individual bios. The main point for raid1 writes is to gather
> >> > a lot of writes together so that all multiple bitmap bits can be set all at
> >> > once.
> >> > It should be possible to submit individual bios directly from make_request
> >> > without passing them to raid1d and without using plugging.
> >> Can you pls explain how it is possible?
> >> You have this code for WRITEs:
> >> cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug));
> >> if (cb)
> >> plug = container_of(cb, struct raid1_plug_cb, cb);
> >> else
> >> plug = NULL;
> >> spin_lock_irqsave(&conf->device_lock, flags);
> >> if (plug) {
> >> bio_list_add(&plug->pending, mbio);
> >> plug->pending_cnt++;
> >> } else {
> >> bio_list_add(&conf->pending_bio_list, mbio);
> >> conf->pending_count++;
> >> }
> >> spin_unlock_irqrestore(&conf->device_lock, flags);
> >>
> >> If the thread blk_check_plugged returns NULL, then you always hand the
> >> WRITE to raid1d. So the only option to avoid handoff to raid1d is for
> >> the caller to plug. Otherwise, all WRITEs are handed off to raid1d and
> >> latency becomes terrible.
> >> So in my case, I use plug/unplug for individual bios only to avoid the
> >> handoff to raid1d.
> >> What am I missing in this analysis?
> >
> > if blk_check_plugged succeeds then it has arranged for raid1_unplug to be
> > called a little later by that same process.
> > So there is nothing to stop you calling raid1_unplug immediately.
> >
> > raid1_unplug essentially does:
> > bitmap_unplug()
> > generic_make_request()
> >
> > so you can very nearly just do that, without any plugging.
> I am sorry, but I did not understand your reply. Maybe I did not
> explain myself, I will try again.
>
> I am not changing raid1.c code. I just want to avoid the handoff to
> raid1d on WRITEs. According to your code, there are only two possible
> flows:
>
> Flow 1 - with plugging
> # caller calls blk_start_plug
> # caller calls submit_bio
> # blk_check_plugged succeeds, and bio is put onto plug->pending list
> # caller calls blk_finish_plug
> # raid1_unplug is called in the same caller's thread, so it does
> bitmap_unplug and generic_make_request
>
> Flow 2 - without plugging
> # caller calls submit_bio
> # blk_check_plugged fails, and bio is put onto conf->pending_bio_list,
> which means it will be submitted by raid1d
>
> My conclusion from that: to avoid the handoff to raid1, caller always
> need to plug, even if it has a single bio to submit. But you said "it
> should be possible to submit individual bios directly from
> make_request without passing them to raid1d and without using
> plugging". So can you explain how it is possible? I prefer not to
> change raid1.c code.
>
> >
> > There is a bit of extra subtlety but I can't really know how relevant that
> > might be to you without actually seeing you code.
> My code (in a different kernel module, not in raid1.c) is simply doing
> submit_bio. I want to wrap this with plug/unplug to avoid the handoff
> to raid1d and improve raid1 latency.
>
I think I need to see the code you are working with to be able to suggest
anything used.
But if it works with plugging, then just do it that way(?).
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: Question about RAID1 plug/unplug code
From: Alexander Lyakas @ 2014-09-10 8:01 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, Yair Hershko
In-Reply-To: <20140909194532.621f4500@notabene.brown>
Hello Neil,
On Tue, Sep 9, 2014 at 12:45 PM, NeilBrown <neilb@suse.de> wrote:
> On Tue, 9 Sep 2014 11:33:13 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> wrote:
>
>> Hi Neil,
>>
>>
>> On Tue, Sep 9, 2014 at 4:45 AM, NeilBrown <neilb@suse.de> wrote:
>> > On Mon, 8 Sep 2014 16:55:52 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
>> > wrote:
>> >
>> >> Hi Neil,
>> >> We have been seeing high latency on the md/raid1 block device, due to
>> >> the fact that all WRITEs are handed off to raid1d thread. This thread
>> >> also calls bitmap_unplug(), which writes the bitmap synchronously.
>> >> While it waits for the bitmap, it cannot trigger other WRITEs waiting
>> >> in its pending_bio_list. This is especially seen with SSDs: MD's
>> >> latency is much higher that SSD latency (I have been stoned by Peter
>> >> Grandi when I brought up this issue previously for raid5).
>> >>
>> >> Then I have noticed the commit:
>> >>
>> >> commit f54a9d0e59c4bea3db733921ca9147612a6f292c
>> >> Author: NeilBrown <neilb@suse.de>
>> >> Date: Thu Aug 2 08:33:20 2012 +1000
>> >>
>> >> md/raid1: submit IO from originating thread instead of md thread.
>> >>
>> >> Looking at the code, I learned that to avoid switching into raid1d,
>> >> the caller has to use blk_start_plug/blk_finish_plug. So I added these
>> >> calls in our kernel module, which submits bios to MD. Results were
>> >> awesome, MD latency got down significantly.
>> >
>> > That's good to hear.
>> >
>> >>
>> >> So I have several questions about this plug/unplug thing.
>> >>
>> >> 1/ Originally this infrastructure was supposed to help IO schedulers
>> >> in merging requests. It is useful when one has a bunch of requests to
>> >> submit in one shot.
>> >
>> > That is exactly the whole point of plugging: allow the device to handle a
>> > batch of requests together instead of one at a time.
>> >
>> >> But in MD case, thus infrastructure is used for a different purpose:
>> >> not to merge requests (which may help bandwidth, but probably not
>> >> latency), but to avoid making raid1d a bottleneck, to be able to
>> >> submit requests from multiple threads in parallel, which brings down
>> >> latency significantly in our case. Indeed "struct blk_plug" has a
>> >> special "cb_list", which is used only by MD.
>> >
>> > I don't think the way md uses plugging is conceptually different from any
>> > other use: it is always about gathering a batch together.
>> > "cb_list" is handled by blk_check_plugged() which is also used by
>> > block/umem.c and btrfs.
>> >
>> > The base plugging code assumes that it is only gathering a batch of requests
>> > for a single device - if the target device changes then the batch is flushed.
>> > It also assumed that it was "struct request" that was batched.
>> > Devices like md that want to queue 'struct bio', something else was needed.
>> > Also with layered devices it can be useful to gather multiple batches for
>> > multiple layers.
>> > So I created "cb_list" etc and a more generic interface.
>> >
>> >> In my case I have only individual bios (not a bunch of bios), and I
>> >> after wrap them with plug/unplug, MD latency gets better. So we are
>> >> using the plug infrastructure for a different purpose.
>> >> Is my understanding correct? Was this your intention?
>> >
>> > I don't really understand what you are doing. There is no point in using
>> > plugging for individual bios. The main point for raid1 writes is to gather
>> > a lot of writes together so that all multiple bitmap bits can be set all at
>> > once.
>> > It should be possible to submit individual bios directly from make_request
>> > without passing them to raid1d and without using plugging.
>> Can you pls explain how it is possible?
>> You have this code for WRITEs:
>> cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug));
>> if (cb)
>> plug = container_of(cb, struct raid1_plug_cb, cb);
>> else
>> plug = NULL;
>> spin_lock_irqsave(&conf->device_lock, flags);
>> if (plug) {
>> bio_list_add(&plug->pending, mbio);
>> plug->pending_cnt++;
>> } else {
>> bio_list_add(&conf->pending_bio_list, mbio);
>> conf->pending_count++;
>> }
>> spin_unlock_irqrestore(&conf->device_lock, flags);
>>
>> If the thread blk_check_plugged returns NULL, then you always hand the
>> WRITE to raid1d. So the only option to avoid handoff to raid1d is for
>> the caller to plug. Otherwise, all WRITEs are handed off to raid1d and
>> latency becomes terrible.
>> So in my case, I use plug/unplug for individual bios only to avoid the
>> handoff to raid1d.
>> What am I missing in this analysis?
>
> if blk_check_plugged succeeds then it has arranged for raid1_unplug to be
> called a little later by that same process.
> So there is nothing to stop you calling raid1_unplug immediately.
>
> raid1_unplug essentially does:
> bitmap_unplug()
> generic_make_request()
>
> so you can very nearly just do that, without any plugging.
I am sorry, but I did not understand your reply. Maybe I did not
explain myself, I will try again.
I am not changing raid1.c code. I just want to avoid the handoff to
raid1d on WRITEs. According to your code, there are only two possible
flows:
Flow 1 - with plugging
# caller calls blk_start_plug
# caller calls submit_bio
# blk_check_plugged succeeds, and bio is put onto plug->pending list
# caller calls blk_finish_plug
# raid1_unplug is called in the same caller's thread, so it does
bitmap_unplug and generic_make_request
Flow 2 - without plugging
# caller calls submit_bio
# blk_check_plugged fails, and bio is put onto conf->pending_bio_list,
which means it will be submitted by raid1d
My conclusion from that: to avoid the handoff to raid1, caller always
need to plug, even if it has a single bio to submit. But you said "it
should be possible to submit individual bios directly from
make_request without passing them to raid1d and without using
plugging". So can you explain how it is possible? I prefer not to
change raid1.c code.
>
> There is a bit of extra subtlety but I can't really know how relevant that
> might be to you without actually seeing you code.
My code (in a different kernel module, not in raid1.c) is simply doing
submit_bio. I want to wrap this with plug/unplug to avoid the handoff
to raid1d and improve raid1 latency.
Thanks,
Alex.
>
> NeilBrown
^ permalink raw reply
* [PATCH 5/5] md/raid1: update next_resync under resync_lock.
From: NeilBrown @ 2014-09-10 6:20 UTC (permalink / raw)
To: Eivind Sarto, Brassow Jonathan; +Cc: linux-raid, majianpeng
In-Reply-To: <20140910062039.26400.36745.stgit@notabene.brown>
raise_barrier() uses next_resync as part of its calculations, so it
really should be updated first, instead of afterwards.
next_resync is always used under resync_lock so update it under
resync lock to, just before it is used. That is safest.
Signed-off-by: NeilBrown <neilb@suse.de>
---
drivers/md/raid1.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 08a6989fcff6..53e163d31750 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -827,7 +827,7 @@ static void flush_pending_writes(struct r1conf *conf)
* there is no normal IO happeing. It must arrange to call
* lower_barrier when the particular background IO completes.
*/
-static void raise_barrier(struct r1conf *conf)
+static void raise_barrier(struct r1conf *conf, sector_t sector_nr)
{
spin_lock_irq(&conf->resync_lock);
@@ -837,6 +837,7 @@ static void raise_barrier(struct r1conf *conf)
/* block any new IO from starting */
conf->barrier++;
+ conf->next_resync = sector_nr;
/* For these conditions we must wait:
* A: while the array is in frozen state
@@ -2542,9 +2543,8 @@ static sector_t sync_request(struct mddev *mddev, sector_t sector_nr, int *skipp
bitmap_cond_end_sync(mddev->bitmap, sector_nr);
r1_bio = mempool_alloc(conf->r1buf_pool, GFP_NOIO);
- raise_barrier(conf);
- conf->next_resync = sector_nr;
+ raise_barrier(conf, sector_nr);
rcu_read_lock();
/*
^ permalink raw reply related
* [PATCH 4/5] md/raid1: Don't use next_resync to determine how far resync has progressed
From: NeilBrown @ 2014-09-10 6:20 UTC (permalink / raw)
To: Eivind Sarto, Brassow Jonathan; +Cc: linux-raid, majianpeng
In-Reply-To: <20140910062039.26400.36745.stgit@notabene.brown>
next_resync is (approximately) the locate for the next resync request.
However it does *not* reliably determine the earliest location
at which resync might be happening.
This is because resync requests can complete out of order, and
or only limit the number of current requests, not the distance
from the earliest pending request to the latest.
mddev->curr_resync_completed is a reliable indicator of the earliest
position at which resync could be happening. It is updated less
frequently, but is actually reliable which is more important.
So use it to determine if a write request is before the region
being resynced and so safe from conflict.
Signed-off-by: NeilBrown <neilb@suse.de>
---
drivers/md/raid1.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 86753358ea46..08a6989fcff6 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -875,12 +875,10 @@ static bool need_to_wait_for_sync(struct r1conf *conf, struct bio *bio)
if (conf->array_frozen || !bio)
wait = true;
else if (conf->barrier && bio_data_dir(bio) == WRITE) {
- if (conf->next_resync < RESYNC_WINDOW_SECTORS)
- wait = true;
- else if ((conf->next_resync - RESYNC_WINDOW_SECTORS
- >= bio_end_sector(bio)) ||
- (conf->next_resync + NEXT_NORMALIO_DISTANCE
- <= bio->bi_iter.bi_sector))
+ if ((conf->mddev->curr_resync_completed
+ >= bio_end_sector(bio)) ||
+ (conf->next_resync + NEXT_NORMALIO_DISTANCE
+ <= bio->bi_iter.bi_sector))
wait = false;
else
wait = true;
@@ -918,7 +916,7 @@ static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
if (bio && bio_data_dir(bio) == WRITE) {
if (bio->bi_iter.bi_sector >=
- conf->next_resync) {
+ conf->mddev->curr_resync_completed) {
if (conf->start_next_window == MaxSector)
conf->start_next_window =
conf->next_resync +
^ permalink raw reply related
* [PATCH 3/5] md/raid1: make sure resync waits for conflicting writes to complete.
From: NeilBrown @ 2014-09-10 6:20 UTC (permalink / raw)
To: Eivind Sarto, Brassow Jonathan; +Cc: linux-raid, majianpeng
In-Reply-To: <20140910062039.26400.36745.stgit@notabene.brown>
The resync/recovery process for raid1 was recently changed
so that writes could happen in parallel with resync providing
they were in different regions of the device.
There is a problem though: While a write request will always
wait for conflicting resync to complete, a resync request
will *not* always wait for conflicting writes to complete.
Two changes are needed to fix this:
1/ raise_barrier (which waits until it is safe to do resync)
must wait until current_window_requests is zero
2/ wait_battier (which wait at the start of a new write request)
must update current_window_requests if the request could
possible conflict with a concurrent resync.
As concurrent writes and resync can lead to data loss,
this patch is suitable for -stable.
Fixes: 79ef3a8aa1cb1523cc231c9a90a278333c21f761
Cc: stable@vger.kernel.org (v3.13+)
Cc: majianpeng <majianpeng@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---
drivers/md/raid1.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 65afd8aa6608..86753358ea46 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -845,10 +845,12 @@ static void raise_barrier(struct r1conf *conf)
* C: next_resync + RESYNC_SECTORS > start_next_window, meaning
* next resync will reach to the window which normal bios are
* handling.
+ * D: while there are any active requests in the current window.
*/
wait_event_lock_irq(conf->wait_barrier,
!conf->array_frozen &&
conf->barrier < RESYNC_DEPTH &&
+ conf->current_window_requests == 0 &&
(conf->start_next_window >=
conf->next_resync + RESYNC_SECTORS),
conf->resync_lock);
@@ -915,8 +917,8 @@ static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
}
if (bio && bio_data_dir(bio) == WRITE) {
- if (conf->next_resync + NEXT_NORMALIO_DISTANCE
- <= bio->bi_iter.bi_sector) {
+ if (bio->bi_iter.bi_sector >=
+ conf->next_resync) {
if (conf->start_next_window == MaxSector)
conf->start_next_window =
conf->next_resync +
^ permalink raw reply related
* [PATCH 2/5] md/raid1: clean up request counts properly in close_sync()
From: NeilBrown @ 2014-09-10 6:20 UTC (permalink / raw)
To: Eivind Sarto, Brassow Jonathan; +Cc: linux-raid, majianpeng
In-Reply-To: <20140910062039.26400.36745.stgit@notabene.brown>
If there are outstanding writes when close_sync is called,
the change to ->start_next_window might cause them to
decrement the wrong counter when they complete. Fix this
by merging the two counters into the one that will be decremented.
Signed-off-by: NeilBrown <neilb@suse.de>
---
drivers/md/raid1.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index f66f67e5a423..65afd8aa6608 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1544,8 +1544,13 @@ static void close_sync(struct r1conf *conf)
mempool_destroy(conf->r1buf_pool);
conf->r1buf_pool = NULL;
+ spin_lock_irq(&conf->resync_lock);
conf->next_resync = 0;
conf->start_next_window = MaxSector;
+ conf->current_window_requests +=
+ conf->next_window_requests;
+ conf->next_window_requests = 0;
+ spin_unlock_irq(&conf->resync_lock);
}
static int raid1_spare_active(struct mddev *mddev)
^ permalink raw reply related
* [PATCH 1/5] md/raid1: be more cautious where we read-balance during resync.
From: NeilBrown @ 2014-09-10 6:20 UTC (permalink / raw)
To: Eivind Sarto, Brassow Jonathan; +Cc: linux-raid, majianpeng
In-Reply-To: <20140910062039.26400.36745.stgit@notabene.brown>
commit 79ef3a8aa1cb1523cc231c9a90a278333c21f761 made
it possible for reads to happen concurrently with resync.
This means that we need to be more careful where read_balancing
is allowed during resync - we can no longer be sure that any
resync that has already started will definitely finish.
So keep read_balancing to before recovery_cp, which is conservative
but safe.
This bug makes it possible to read from a device that doesn't
have up-to-date data, so it can cause data corruption.
So it is suitable for any kernel since 3.11.
Fixes: 79ef3a8aa1cb1523cc231c9a90a278333c21f761
cc: stable@vger.kernel.org (v3.11+)
Signed-off-by: NeilBrown <neilb@suse.de>
---
drivers/md/raid1.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index d7690f86fdb9..f66f67e5a423 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -540,11 +540,7 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
has_nonrot_disk = 0;
choose_next_idle = 0;
- if (conf->mddev->recovery_cp < MaxSector &&
- (this_sector + sectors >= conf->next_resync))
- choose_first = 1;
- else
- choose_first = 0;
+ choose_first = (conf->mddev->recovery_cp < this_sector + sectors);
for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
sector_t dist;
^ permalink raw reply related
* [PATCH 0/5] Fixes for RAID1 resync
From: NeilBrown @ 2014-09-10 6:20 UTC (permalink / raw)
To: Eivind Sarto, Brassow Jonathan; +Cc: linux-raid, majianpeng
This set of patches fixes all the issues that I could
find with the new raid1 resync code.
I didn't find anything that would obviously fix the hang
that Jon Brassow reported.
Jon: could you test with these patches on top of what you
have just in case something happens to fix the problem without
me realising it?
Eivind: if you could test too, I'd really appreciate it.
Thanks,
NeilBrown
---
NeilBrown (5):
md/raid1: be more cautious where we read-balance during resync.
md/raid1: clean up request counts properly in close_sync()
md/raid1: make sure resync waits for conflicting writes to complete.
md/raid1: Don't use next_resync to determine how far resync has progressed
md/raid1: update next_resync under resync_lock.
drivers/md/raid1.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
--
Signature
^ permalink raw reply
* AW: raid5: direct IO and md_wakeup_thread
From: Markus Stockhausen @ 2014-09-09 18:48 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid@vger.kernel.org
In-Reply-To: <12EF8D94C6F8734FB2FF37B9FBEDD17358644B61@EXCHANGE.collogia.de>
[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]
> HI Neil,
>
> thanks for the explanations. I tried to understand the reason for the
> "bad" benchmark numbers during my direct IO write tests for my rmw
> patch. I only choose it to avoid caching side effects. Even with 64
> threads there is still a large gap to the theoretical maximum. So I
> reduced the test to 1 writer and took some time to have a look how
> raid5.c handles sync/direct writes. See my other post with only 5MB/s
> 4K direct writes to ramdisk backed raid5 md.
>
> With that insight I started to compare it to raid1.c. After some
> iterations it came to my mind that the empty raid5d() runs between
> two sync writes might give extra latency and so I looked for corners
> where md_wakeup_thread() might be unnecessary. The result are
> the questions above.
>
> I'll give the clear_bit(THREAD_WAKEUP,...) a try.
>
> Markus
Just an all-clear response. The error I was analyzing came from my
Virutalbox test environment. With more than one CPU activated
process synchronzation seems to face high synchonization penalties
between make_request() and raid5d(). Switching back to real
hardware with /dev/ramX shows normal throughput.
A lot to wonder about when testing the kernel ...
Markus
=
[-- 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: [PATCH] Create.c: Try few more times to stop array after failed creation
From: NeilBrown @ 2014-09-09 10:16 UTC (permalink / raw)
To: Baldysiak, Pawel; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur
In-Reply-To: <84A53BEA6EAC69439B7E311E9B17A76F1A69724E@IRSMSX110.ger.corp.intel.com>
[-- Attachment #1: Type: text/plain, Size: 3514 bytes --]
On Tue, 9 Sep 2014 10:04:38 +0000 "Baldysiak, Pawel"
<pawel.baldysiak@intel.com> wrote:
> > On: Monday, September 08, 2014 8:34 AM NeilBrown wrote:
> > To: Baldysiak, Pawel
> > Cc: linux-raid@vger.kernel.org; Paszkiewicz, Artur
> > Subject: Re: [PATCH] Create.c: Try few more times to stop array after failed
> > creation
> >
> > On Fri, 05 Sep 2014 16:26:13 +0200 Pawel Baldysiak
> > <pawel.baldysiak@intel.com> wrote:
> >
> > > Sometimes after failure in creation (exp. due to duplicate devices in
> > > create command) newly created empty md array will not be stopped due
> > > to openers>1 (create_mddev will not manage to drop lock).
> > > In this case ioctl() will return error - this needs to be checked and
> > > if occurs - sending STOP_ARRAY should be repeat after delay to make
> > > sure that mddev is stopped correctly.
> > >
> > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> > > ---
> > > Create.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Create.c b/Create.c
> > > index 330c5b4..7c8e53e 100644
> > > --- a/Create.c
> > > +++ b/Create.c
> > > @@ -904,7 +904,12 @@ int Create(struct supertype *st, char *mddev,
> > > if (st->ss->add_to_super(st, &inf->disk,
> > > fd, dv->devname,
> > > dv->data_offset)) {
> > > - ioctl(mdfd, STOP_ARRAY, NULL);
> > > + int count = 5;
> > > + while (count &&
> > > + (ioctl(mdfd, STOP_ARRAY, NULL)
> > < 0)) {
> > > + usleep(100000);
> > > + count--;
> > > + }
> > > goto abort_locked;
> > > }
> > > st->ss->getinfo_super(st, inf, NULL);
> >
> > I don't like this. I don't really like any of the other loops like this that are
> > already in the code either. I wonder if we can avoid the need for it.
> >
> > Given that the array hasn't been started yet, no other process can actually be
> > *using* the array. And given that we have an O_EXCL open at this point, no
> > other process can be trying to stop/start the array.
> > So it should be safe to change the kernel to not fail in this situation.
> >
> > If you apply this kernel patch:
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c index
> > 1294238610df..1bf3fe1ecc79 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -5362,7 +5362,7 @@ static int do_md_stop(struct mddev * mddev, int
> > mode,
> > mddev_lock_nointr(mddev);
> >
> > mutex_lock(&mddev->open_mutex);
> > - if (atomic_read(&mddev->openers) > !!bdev ||
> > + if ((mddev->pers && atomic_read(&mddev->openers) > !!bdev) ||
> > mddev->sysfs_active ||
> > mddev->sync_thread ||
> > (bdev && !test_bit(MD_STILL_CLOSED, &mddev->flags))) {
> >
> >
> > does that fir your problem? Can you see any reason not to allow
> > STOP_ARRAY to succeed in this situation?
> >
> Hi Neil
> Thanks for your answer.
> To fix this problem same thing needs to be added in one more place in kernel:
>
> @@ -6454,7 +6454,7 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
> * and writes
> */
> mutex_lock(&mddev->open_mutex);
> - if (atomic_read(&mddev->openers) > 1) {
> + if (mddev->pers && (atomic_read(&mddev->openers) > 1)) {
> mutex_unlock(&mddev->open_mutex);
> err = -EBUSY;
> goto abort;
>
> Should I prepare the patch, or you can do it?
I'll do it thanks - I have it half done already.
Thanks for testing.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* RE: [PATCH] Create.c: Try few more times to stop array after failed creation
From: Baldysiak, Pawel @ 2014-09-09 10:04 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid@vger.kernel.org, Paszkiewicz, Artur
In-Reply-To: <20140908163337.6129ef3e@notabene.brown>
> On: Monday, September 08, 2014 8:34 AM NeilBrown wrote:
> To: Baldysiak, Pawel
> Cc: linux-raid@vger.kernel.org; Paszkiewicz, Artur
> Subject: Re: [PATCH] Create.c: Try few more times to stop array after failed
> creation
>
> On Fri, 05 Sep 2014 16:26:13 +0200 Pawel Baldysiak
> <pawel.baldysiak@intel.com> wrote:
>
> > Sometimes after failure in creation (exp. due to duplicate devices in
> > create command) newly created empty md array will not be stopped due
> > to openers>1 (create_mddev will not manage to drop lock).
> > In this case ioctl() will return error - this needs to be checked and
> > if occurs - sending STOP_ARRAY should be repeat after delay to make
> > sure that mddev is stopped correctly.
> >
> > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> > ---
> > Create.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/Create.c b/Create.c
> > index 330c5b4..7c8e53e 100644
> > --- a/Create.c
> > +++ b/Create.c
> > @@ -904,7 +904,12 @@ int Create(struct supertype *st, char *mddev,
> > if (st->ss->add_to_super(st, &inf->disk,
> > fd, dv->devname,
> > dv->data_offset)) {
> > - ioctl(mdfd, STOP_ARRAY, NULL);
> > + int count = 5;
> > + while (count &&
> > + (ioctl(mdfd, STOP_ARRAY, NULL)
> < 0)) {
> > + usleep(100000);
> > + count--;
> > + }
> > goto abort_locked;
> > }
> > st->ss->getinfo_super(st, inf, NULL);
>
> I don't like this. I don't really like any of the other loops like this that are
> already in the code either. I wonder if we can avoid the need for it.
>
> Given that the array hasn't been started yet, no other process can actually be
> *using* the array. And given that we have an O_EXCL open at this point, no
> other process can be trying to stop/start the array.
> So it should be safe to change the kernel to not fail in this situation.
>
> If you apply this kernel patch:
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c index
> 1294238610df..1bf3fe1ecc79 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5362,7 +5362,7 @@ static int do_md_stop(struct mddev * mddev, int
> mode,
> mddev_lock_nointr(mddev);
>
> mutex_lock(&mddev->open_mutex);
> - if (atomic_read(&mddev->openers) > !!bdev ||
> + if ((mddev->pers && atomic_read(&mddev->openers) > !!bdev) ||
> mddev->sysfs_active ||
> mddev->sync_thread ||
> (bdev && !test_bit(MD_STILL_CLOSED, &mddev->flags))) {
>
>
> does that fir your problem? Can you see any reason not to allow
> STOP_ARRAY to succeed in this situation?
>
Hi Neil
Thanks for your answer.
To fix this problem same thing needs to be added in one more place in kernel:
@@ -6454,7 +6454,7 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
* and writes
*/
mutex_lock(&mddev->open_mutex);
- if (atomic_read(&mddev->openers) > 1) {
+ if (mddev->pers && (atomic_read(&mddev->openers) > 1)) {
mutex_unlock(&mddev->open_mutex);
err = -EBUSY;
goto abort;
Should I prepare the patch, or you can do it?
Thanks,
Pawel Baldysiak
> Thanks,
> NeilBrown
^ permalink raw reply
* Re: Question about RAID1 plug/unplug code
From: NeilBrown @ 2014-09-09 9:45 UTC (permalink / raw)
To: Alexander Lyakas; +Cc: linux-raid, Yair Hershko
In-Reply-To: <CAGRgLy4QAz6KVncLsVypsOD85jMUNaOgrFc13=knHMXb92SvXw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5106 bytes --]
On Tue, 9 Sep 2014 11:33:13 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
wrote:
> Hi Neil,
>
>
> On Tue, Sep 9, 2014 at 4:45 AM, NeilBrown <neilb@suse.de> wrote:
> > On Mon, 8 Sep 2014 16:55:52 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> > wrote:
> >
> >> Hi Neil,
> >> We have been seeing high latency on the md/raid1 block device, due to
> >> the fact that all WRITEs are handed off to raid1d thread. This thread
> >> also calls bitmap_unplug(), which writes the bitmap synchronously.
> >> While it waits for the bitmap, it cannot trigger other WRITEs waiting
> >> in its pending_bio_list. This is especially seen with SSDs: MD's
> >> latency is much higher that SSD latency (I have been stoned by Peter
> >> Grandi when I brought up this issue previously for raid5).
> >>
> >> Then I have noticed the commit:
> >>
> >> commit f54a9d0e59c4bea3db733921ca9147612a6f292c
> >> Author: NeilBrown <neilb@suse.de>
> >> Date: Thu Aug 2 08:33:20 2012 +1000
> >>
> >> md/raid1: submit IO from originating thread instead of md thread.
> >>
> >> Looking at the code, I learned that to avoid switching into raid1d,
> >> the caller has to use blk_start_plug/blk_finish_plug. So I added these
> >> calls in our kernel module, which submits bios to MD. Results were
> >> awesome, MD latency got down significantly.
> >
> > That's good to hear.
> >
> >>
> >> So I have several questions about this plug/unplug thing.
> >>
> >> 1/ Originally this infrastructure was supposed to help IO schedulers
> >> in merging requests. It is useful when one has a bunch of requests to
> >> submit in one shot.
> >
> > That is exactly the whole point of plugging: allow the device to handle a
> > batch of requests together instead of one at a time.
> >
> >> But in MD case, thus infrastructure is used for a different purpose:
> >> not to merge requests (which may help bandwidth, but probably not
> >> latency), but to avoid making raid1d a bottleneck, to be able to
> >> submit requests from multiple threads in parallel, which brings down
> >> latency significantly in our case. Indeed "struct blk_plug" has a
> >> special "cb_list", which is used only by MD.
> >
> > I don't think the way md uses plugging is conceptually different from any
> > other use: it is always about gathering a batch together.
> > "cb_list" is handled by blk_check_plugged() which is also used by
> > block/umem.c and btrfs.
> >
> > The base plugging code assumes that it is only gathering a batch of requests
> > for a single device - if the target device changes then the batch is flushed.
> > It also assumed that it was "struct request" that was batched.
> > Devices like md that want to queue 'struct bio', something else was needed.
> > Also with layered devices it can be useful to gather multiple batches for
> > multiple layers.
> > So I created "cb_list" etc and a more generic interface.
> >
> >> In my case I have only individual bios (not a bunch of bios), and I
> >> after wrap them with plug/unplug, MD latency gets better. So we are
> >> using the plug infrastructure for a different purpose.
> >> Is my understanding correct? Was this your intention?
> >
> > I don't really understand what you are doing. There is no point in using
> > plugging for individual bios. The main point for raid1 writes is to gather
> > a lot of writes together so that all multiple bitmap bits can be set all at
> > once.
> > It should be possible to submit individual bios directly from make_request
> > without passing them to raid1d and without using plugging.
> Can you pls explain how it is possible?
> You have this code for WRITEs:
> cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug));
> if (cb)
> plug = container_of(cb, struct raid1_plug_cb, cb);
> else
> plug = NULL;
> spin_lock_irqsave(&conf->device_lock, flags);
> if (plug) {
> bio_list_add(&plug->pending, mbio);
> plug->pending_cnt++;
> } else {
> bio_list_add(&conf->pending_bio_list, mbio);
> conf->pending_count++;
> }
> spin_unlock_irqrestore(&conf->device_lock, flags);
>
> If the thread blk_check_plugged returns NULL, then you always hand the
> WRITE to raid1d. So the only option to avoid handoff to raid1d is for
> the caller to plug. Otherwise, all WRITEs are handed off to raid1d and
> latency becomes terrible.
> So in my case, I use plug/unplug for individual bios only to avoid the
> handoff to raid1d.
> What am I missing in this analysis?
if blk_check_plugged succeeds then it has arranged for raid1_unplug to be
called a little later by that same process.
So there is nothing to stop you calling raid1_unplug immediately.
raid1_unplug essentially does:
bitmap_unplug()
generic_make_request()
so you can very nearly just do that, without any plugging.
There is a bit of extra subtlety but I can't really know how relevant that
might be to you without actually seeing you code.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: Question about RAID1 plug/unplug code
From: Alexander Lyakas @ 2014-09-09 8:33 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, Yair Hershko
In-Reply-To: <20140909114538.552a4a7d@notabene.brown>
Hi Neil,
On Tue, Sep 9, 2014 at 4:45 AM, NeilBrown <neilb@suse.de> wrote:
> On Mon, 8 Sep 2014 16:55:52 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
> wrote:
>
>> Hi Neil,
>> We have been seeing high latency on the md/raid1 block device, due to
>> the fact that all WRITEs are handed off to raid1d thread. This thread
>> also calls bitmap_unplug(), which writes the bitmap synchronously.
>> While it waits for the bitmap, it cannot trigger other WRITEs waiting
>> in its pending_bio_list. This is especially seen with SSDs: MD's
>> latency is much higher that SSD latency (I have been stoned by Peter
>> Grandi when I brought up this issue previously for raid5).
>>
>> Then I have noticed the commit:
>>
>> commit f54a9d0e59c4bea3db733921ca9147612a6f292c
>> Author: NeilBrown <neilb@suse.de>
>> Date: Thu Aug 2 08:33:20 2012 +1000
>>
>> md/raid1: submit IO from originating thread instead of md thread.
>>
>> Looking at the code, I learned that to avoid switching into raid1d,
>> the caller has to use blk_start_plug/blk_finish_plug. So I added these
>> calls in our kernel module, which submits bios to MD. Results were
>> awesome, MD latency got down significantly.
>
> That's good to hear.
>
>>
>> So I have several questions about this plug/unplug thing.
>>
>> 1/ Originally this infrastructure was supposed to help IO schedulers
>> in merging requests. It is useful when one has a bunch of requests to
>> submit in one shot.
>
> That is exactly the whole point of plugging: allow the device to handle a
> batch of requests together instead of one at a time.
>
>> But in MD case, thus infrastructure is used for a different purpose:
>> not to merge requests (which may help bandwidth, but probably not
>> latency), but to avoid making raid1d a bottleneck, to be able to
>> submit requests from multiple threads in parallel, which brings down
>> latency significantly in our case. Indeed "struct blk_plug" has a
>> special "cb_list", which is used only by MD.
>
> I don't think the way md uses plugging is conceptually different from any
> other use: it is always about gathering a batch together.
> "cb_list" is handled by blk_check_plugged() which is also used by
> block/umem.c and btrfs.
>
> The base plugging code assumes that it is only gathering a batch of requests
> for a single device - if the target device changes then the batch is flushed.
> It also assumed that it was "struct request" that was batched.
> Devices like md that want to queue 'struct bio', something else was needed.
> Also with layered devices it can be useful to gather multiple batches for
> multiple layers.
> So I created "cb_list" etc and a more generic interface.
>
>> In my case I have only individual bios (not a bunch of bios), and I
>> after wrap them with plug/unplug, MD latency gets better. So we are
>> using the plug infrastructure for a different purpose.
>> Is my understanding correct? Was this your intention?
>
> I don't really understand what you are doing. There is no point in using
> plugging for individual bios. The main point for raid1 writes is to gather
> a lot of writes together so that all multiple bitmap bits can be set all at
> once.
> It should be possible to submit individual bios directly from make_request
> without passing them to raid1d and without using plugging.
Can you pls explain how it is possible?
You have this code for WRITEs:
cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug));
if (cb)
plug = container_of(cb, struct raid1_plug_cb, cb);
else
plug = NULL;
spin_lock_irqsave(&conf->device_lock, flags);
if (plug) {
bio_list_add(&plug->pending, mbio);
plug->pending_cnt++;
} else {
bio_list_add(&conf->pending_bio_list, mbio);
conf->pending_count++;
}
spin_unlock_irqrestore(&conf->device_lock, flags);
If the thread blk_check_plugged returns NULL, then you always hand the
WRITE to raid1d. So the only option to avoid handoff to raid1d is for
the caller to plug. Otherwise, all WRITEs are handed off to raid1d and
latency becomes terrible.
So in my case, I use plug/unplug for individual bios only to avoid the
handoff to raid1d.
What am I missing in this analysis?
> You might end up
> making more bitmap updates, but you might not.
>
>
>>
>> 2/ Now that md/raid1 submits WRITEs from several threads in parallel,
>> is there any issue you can think of? Like for example, multiple
>> threads now call bitmap_unplug() in parallel. Is this alright?
>
> Hmmm... there could be an issue there. It is possible that some callers of
> bitmap_unplug won't block when they should. bitmap_unplug should probably
> wait unconditionally.
Ok, so you say there is a bug with bitmap updates when WRITE is
submitted directly from make_request. Is this something we should fix?
Alex.
>
> NeilBrown
>
>
>>
>> Thanks!
>> Alex.
>
^ permalink raw reply
* Re: RAID456 direct I/O write performance
From: NeilBrown @ 2014-09-09 3:24 UTC (permalink / raw)
To: Ethan Wilson; +Cc: linux-raid@vger.kernel.org
In-Reply-To: <5408D5CC.101@shiftmail.org>
[-- Attachment #1: Type: text/plain, Size: 899 bytes --]
On Thu, 04 Sep 2014 23:12:44 +0200 Ethan Wilson <ethan.wilson@shiftmail.org>
wrote:
> OTOH I would like to ask kernel experts one thing if I may: does anybody
> know a way to get a stack trace for a process which is currently running
> in kernel mode and is running NOW on a CPU and it is not stopped waiting
> in a queue? I know about /proc/pid/stack but that one shows
> 0xffffffffffffffff for such a case. Being able to do that would help to
> answer the above question too...
The contents of the stack would change while it was being inspected, so it
would be impossible to get a meaningful trace.
Maybe you could disable all but one CPU. Then whenever you try to look at
the stack of another process it must have scheduled and so will have a
stable visible stack...
Or maybe you would use "perf record -g" to get some stack information, I'm
not sure.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: Mirroring seek optimization resend
From: NeilBrown @ 2014-09-09 2:21 UTC (permalink / raw)
To: Robert Long; +Cc: linux-raid
In-Reply-To: <9300C887-1255-4AE6-806B-390332D10382@k9heritage.com>
[-- Attachment #1: Type: text/plain, Size: 3308 bytes --]
On Mon, 8 Sep 2014 10:13:12 -0700 Robert Long <boblong@k9heritage.com> wrote:
> Hi Again,
>
> First, the code base that I was working from is now 2 years old. While I doubt that the raid1.c code has changed significantly I could be very wrong. At that time the load balancing algorithm was simply to attempt to split the actively seeked area of the disk into two parts, and have one disk take the low half and one take the high half. The effect of that, in read intensive applications, is to cut in half the overall seek length for each disk in the mirror, resulting in considerable seek time savings and overall higher throughput. A moving average of sector addresses needs to be maintained to make this work, but this is quite easily done. The more writing happens, the more this advantage is lost. The problem with this approach, though not a large one, is that is that a numerical average is not actually representative of the middle of the work area due to outliers, clusters of reads, etc.
>
> A solution to this is to maintain a running median of the sector address. When this is done you get a measurable increase in throughput, at the cost of greater computational overhead. The method that I used to maintain a moving median involves the creation of an equal sized min heap and max heap connected together at the head of each. This connection point represents the current median sector of access requests to the mirror.
>
> Today I fired up the machine that I used to develop and test this modification. It took some time just to locate the modified raid1.c file :) The machine, in addition to the boot drive, is equipped with a couple of 1 TB Hitachi disk drives that were used for test purposes. To be honest I don’t remember the name of the test software that I used, but it seems that it was a pretty performance standard utility, not something I put together myself. Perhaps you could jog my memory. As I mentioned previously the improvement was around 7% with read only testing of random seeks.
>
> If this is of interest I will gladly post the NOT PRODUCTION READY code. (I took some shortcuts in the code for testing purposes, primarily in the assignment of drives to the array so that I didn’t have to figure out how the existing code does such a nice job of drive allocation.)
>
> On the other hand, if the improvement is deemed to be insufficient then I will chuck the code into the dust bin of history. Given the continued rise in the use of SSDs, seek optimization becomes less relevant every day.
>
> Please give me some feedback, even if you are not interested let me know why.
>
Hi Bob,
If you post the code I'll try to find time to look at it. If you don't, I
won't :-)
I personally have no pressing desire to improve RAID1 read performance, but
others have occasionally shown an interest in the past. Maybe they aren't
paying attention or have lost interest. Maybe they don't think their
interest matters (which wouldn't be true).
To be honest, "I've got some code that once did something good with some
tests that I don't remember and probably isn't production ready and may not
even work" just doesn't sound exciting.
On the other hand "here is some code" at least provides something concrete
to look at.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ 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