* Re: [RFC] block: fix blk_queue_split() resource exhaustion
From: Ming Lei @ 2016-07-02 10:28 UTC (permalink / raw)
To: Lars Ellenberg
Cc: linux-block, Roland Kammerer, Jens Axboe, NeilBrown,
Kent Overstreet, Shaohua Li, Alasdair Kergon, Mike Snitzer,
open list:DEVICE-MAPPER (LVM), Ingo Molnar, Peter Zijlstra,
Takashi Iwai, Jiri Kosina, Zheng Liu, Keith Busch,
Martin K. Petersen, Kirill A. Shutemov, Linux Kernel Mailing List,
open list:BCACHE (BLOCK LAYER CACHE),
open list:SOFTWARE RAID (Multiple Disks) SUPPORT
In-Reply-To: <1466583730-28595-1-git-send-email-lars.ellenberg@linbit.com>
On Wed, Jun 22, 2016 at 4:22 PM, Lars Ellenberg
<lars.ellenberg@linbit.com> wrote:
> For a long time, generic_make_request() converts recursion into
> iteration by queuing recursive arguments on current->bio_list.
>
> This is convenient for stacking drivers,
> the top-most driver would take the originally submitted bio,
> and re-submit a re-mapped version of it, or one or more clones,
> or one or more new allocated bios to its backend(s). Which
> are then simply processed in turn, and each can again queue
> more "backend-bios" until we reach the bottom of the driver stack,
> and actually dispatch to the real backend device.
>
> Any stacking driver ->make_request_fn() could expect that,
> once it returns, any backend-bios it submitted via recursive calls
> to generic_make_request() would now be processed and dispatched, before
> the current task would call into this driver again.
>
> This is changed by commit
> 54efd50 block: make generic_make_request handle arbitrarily sized bios
>
> Drivers may call blk_queue_split() inside their ->make_request_fn(),
> which may split the current bio into a front-part to be dealt with
> immediately, and a remainder-part, which may need to be split even
> further. That remainder-part will simply also be pushed to
> current->bio_list, and would end up being head-of-queue, in front
> of any backend-bios the current make_request_fn() might submit during
> processing of the fron-part.
>
> Which means the current task would immediately end up back in the same
> make_request_fn() of the same driver again, before any of its backend
> bios have even been processed.
>
> This can lead to resource starvation deadlock.
> Drivers could avoid this by learning to not need blk_queue_split(),
> or by submitting their backend bios in a different context (dedicated
> kernel thread, work_queue context, ...). Or by playing funny re-ordering
> games with entries on current->bio_list.
>
> Instead, I suggest to distinguish between recursive calls to
> generic_make_request(), and pushing back the remainder part in
> blk_queue_split(), by pointing current->bio_lists to a
> struct recursion_to_iteration_bio_lists {
> struct bio_list recursion;
> struct bio_list remainder;
> }
>
> To have all bios targeted to drivers lower in the stack processed before
> processing the next piece of a bio targeted at the higher levels,
> as long as queued bios resulting from recursion are available,
> they will continue to be processed in FIFO order.
> Pushed back bio-parts resulting from blk_queue_split() will be processed
> in LIFO order, one-by-one, whenever the recursion list becomes empty.
>
> Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
> Signed-off-by: Roland Kammerer <roland.kammerer@linbit.com>
> ---
>
> This is not a theoretical problem.
> At least int DRBD, and an unfortunately high IO concurrency wrt. the
> "max-buffers" setting, without this patch we have a reproducible deadlock.
>
> I'm unsure if it could cause problems in md raid in some corner cases
> or when a rebuild/scrub/... starts in a "bad" moment.
>
> It also may increase "stress" on any involved bio_set,
> because it may increase the number of bios that are allocated
> before the first of them is actually processed, causing more
> frequent calls of punt_bios_to_rescuer().
>
> Alternatively,
> a simple one-line change to generic_make_request() would also "fix" it,
> by processing all recursive bios in LIFO order.
> But that may change the order in which bios reach the "physical" layer,
> in case they are in fact split into many smaller pieces, for example in
> a striping setup, which may have other performance implications.
>
> | diff --git a/block/blk-core.c b/block/blk-core.c
> | index 2475b1c7..a5623f6 100644
> | --- a/block/blk-core.c
> | +++ b/block/blk-core.c
> | @@ -2048,7 +2048,7 @@ blk_qc_t generic_make_request(struct bio *bio)
> | * should be added at the tail
> | */
> | if (current->bio_list) {
> | - bio_list_add(current->bio_list, bio);
> | + bio_list_add_head(current->bio_list, bio);
> | goto out;
> | }
>
> Any fix would need to go to stable 4.3.x and up.
> This patch does apply as is to 4.5 and up,
> with a trivial fixup (or a cherry-pick of cda2264) to 4.4,
> and with some more (also trivial) fixup to 4.3 because of
> GFP_WAIT -> GFP_RECLAIM and comment rewrap.
>
> Any input?
The idea looks good, but not sure it can cover all cases of
dm over brbd or brbd over dm and maintaining two lists
becomes too complicated.
One clean solution may be to convert the loop of generic_make_request()
into the following way:
do {
struct bio *splitted, *remainder = NULL;
struct request_queue *q = bdev_get_queue(bio->bi_bdev);
blk_queue_split(q, &bio, &remainder, q->bio_split);
ret = q->make_request_fn(q, bio);
if (remainder)
bio_list_add(current->bio_list, remainder);
bio = bio_list_pop(current->bio_list);
} while (bio)
And blk_queue_split() need a bit change and all its external calling
have to be removed.
Of cource we also need to fix blk_queue_bounce() first, but that can
be done easily by handling bounced bio via generic_make_request().
> How should I proceed?
I think the issue need to be fixed.
Thanks,
Ming
>
> ---
> block/bio.c | 14 +++++++-------
> block/blk-core.c | 32 +++++++++++++++++---------------
> block/blk-merge.c | 3 ++-
> drivers/md/bcache/btree.c | 12 ++++++------
> drivers/md/dm-bufio.c | 2 +-
> drivers/md/md.h | 7 +++++++
> drivers/md/raid1.c | 5 ++---
> drivers/md/raid10.c | 5 ++---
> include/linux/bio.h | 11 +++++++++++
> include/linux/sched.h | 4 ++--
> 10 files changed, 57 insertions(+), 38 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 0e4aa42..2ffcea0 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -368,10 +368,10 @@ static void punt_bios_to_rescuer(struct bio_set *bs)
> bio_list_init(&punt);
> bio_list_init(&nopunt);
>
> - while ((bio = bio_list_pop(current->bio_list)))
> + while ((bio = bio_list_pop(¤t->bio_lists->recursion)))
> bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
>
> - *current->bio_list = nopunt;
> + current->bio_lists->recursion = nopunt;
>
> spin_lock(&bs->rescue_lock);
> bio_list_merge(&bs->rescue_list, &punt);
> @@ -453,13 +453,13 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
> *
> * We solve this, and guarantee forward progress, with a rescuer
> * workqueue per bio_set. If we go to allocate and there are
> - * bios on current->bio_list, we first try the allocation
> - * without __GFP_DIRECT_RECLAIM; if that fails, we punt those
> - * bios we would be blocking to the rescuer workqueue before
> - * we retry with the original gfp_flags.
> + * bios on current->bio_lists->recursion, we first try the
> + * allocation without __GFP_DIRECT_RECLAIM; if that fails, we
> + * punt those bios we would be blocking to the rescuer
> + * workqueue before we retry with the original gfp_flags.
> */
>
> - if (current->bio_list && !bio_list_empty(current->bio_list))
> + if (current->bio_lists && !bio_list_empty(¤t->bio_lists->recursion))
> gfp_mask &= ~__GFP_DIRECT_RECLAIM;
>
> p = mempool_alloc(bs->bio_pool, gfp_mask);
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2475b1c7..f03ff4c 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2031,7 +2031,7 @@ end_io:
> */
> blk_qc_t generic_make_request(struct bio *bio)
> {
> - struct bio_list bio_list_on_stack;
> + struct recursion_to_iteration_bio_lists bio_lists_on_stack;
> blk_qc_t ret = BLK_QC_T_NONE;
>
> if (!generic_make_request_checks(bio))
> @@ -2040,15 +2040,18 @@ blk_qc_t generic_make_request(struct bio *bio)
> /*
> * We only want one ->make_request_fn to be active at a time, else
> * stack usage with stacked devices could be a problem. So use
> - * current->bio_list to keep a list of requests submited by a
> - * make_request_fn function. current->bio_list is also used as a
> + * current->bio_lists to keep a list of requests submited by a
> + * make_request_fn function. current->bio_lists is also used as a
> * flag to say if generic_make_request is currently active in this
> * task or not. If it is NULL, then no make_request is active. If
> * it is non-NULL, then a make_request is active, and new requests
> - * should be added at the tail
> + * should be added at the tail of current->bio_lists->recursion;
> + * remainder bios resulting from a call to bio_queue_split() from
> + * within ->make_request_fn() should be added to the head of
> + * current->bio_lists->remainder.
> */
> - if (current->bio_list) {
> - bio_list_add(current->bio_list, bio);
> + if (current->bio_lists) {
> + bio_list_add(¤t->bio_lists->recursion, bio);
> goto out;
> }
>
> @@ -2057,7 +2060,7 @@ blk_qc_t generic_make_request(struct bio *bio)
> * Before entering the loop, bio->bi_next is NULL (as all callers
> * ensure that) so we have a list with a single bio.
> * We pretend that we have just taken it off a longer list, so
> - * we assign bio_list to a pointer to the bio_list_on_stack,
> + * we assign bio_list to a pointer to the bio_lists_on_stack,
> * thus initialising the bio_list of new bios to be
> * added. ->make_request() may indeed add some more bios
> * through a recursive call to generic_make_request. If it
> @@ -2067,8 +2070,9 @@ blk_qc_t generic_make_request(struct bio *bio)
> * bio_list, and call into ->make_request() again.
> */
> BUG_ON(bio->bi_next);
> - bio_list_init(&bio_list_on_stack);
> - current->bio_list = &bio_list_on_stack;
> + bio_list_init(&bio_lists_on_stack.recursion);
> + bio_list_init(&bio_lists_on_stack.remainder);
> + current->bio_lists = &bio_lists_on_stack;
> do {
> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>
> @@ -2076,16 +2080,14 @@ blk_qc_t generic_make_request(struct bio *bio)
> ret = q->make_request_fn(q, bio);
>
> blk_queue_exit(q);
> -
> - bio = bio_list_pop(current->bio_list);
> } else {
> - struct bio *bio_next = bio_list_pop(current->bio_list);
> -
> bio_io_error(bio);
> - bio = bio_next;
> }
> + bio = bio_list_pop(¤t->bio_lists->recursion);
> + if (!bio)
> + bio = bio_list_pop(¤t->bio_lists->remainder);
> } while (bio);
> - current->bio_list = NULL; /* deactivate */
> + current->bio_lists = NULL; /* deactivate */
>
> out:
> return ret;
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 2613531..8da0c22 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -172,6 +172,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
> struct bio *split, *res;
> unsigned nsegs;
>
> + BUG_ON(!current->bio_lists);
> if ((*bio)->bi_rw & REQ_DISCARD)
> split = blk_bio_discard_split(q, *bio, bs, &nsegs);
> else if ((*bio)->bi_rw & REQ_WRITE_SAME)
> @@ -190,7 +191,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
>
> bio_chain(split, *bio);
> trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
> - generic_make_request(*bio);
> + bio_list_add_head(¤t->bio_lists->remainder, *bio);
> *bio = split;
> }
> }
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index eab505e..eb0b41a 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -450,7 +450,7 @@ void __bch_btree_node_write(struct btree *b, struct closure *parent)
>
> trace_bcache_btree_write(b);
>
> - BUG_ON(current->bio_list);
> + BUG_ON(current->bio_lists);
> BUG_ON(b->written >= btree_blocks(b));
> BUG_ON(b->written && !i->keys);
> BUG_ON(btree_bset_first(b)->seq != i->seq);
> @@ -544,7 +544,7 @@ static void bch_btree_leaf_dirty(struct btree *b, atomic_t *journal_ref)
>
> /* Force write if set is too big */
> if (set_bytes(i) > PAGE_SIZE - 48 &&
> - !current->bio_list)
> + !current->bio_lists)
> bch_btree_node_write(b, NULL);
> }
>
> @@ -889,7 +889,7 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
> {
> struct btree *b;
>
> - BUG_ON(current->bio_list);
> + BUG_ON(current->bio_lists);
>
> lockdep_assert_held(&c->bucket_lock);
>
> @@ -976,7 +976,7 @@ retry:
> b = mca_find(c, k);
>
> if (!b) {
> - if (current->bio_list)
> + if (current->bio_lists)
> return ERR_PTR(-EAGAIN);
>
> mutex_lock(&c->bucket_lock);
> @@ -2127,7 +2127,7 @@ static int bch_btree_insert_node(struct btree *b, struct btree_op *op,
>
> return 0;
> split:
> - if (current->bio_list) {
> + if (current->bio_lists) {
> op->lock = b->c->root->level + 1;
> return -EAGAIN;
> } else if (op->lock <= b->c->root->level) {
> @@ -2209,7 +2209,7 @@ int bch_btree_insert(struct cache_set *c, struct keylist *keys,
> struct btree_insert_op op;
> int ret = 0;
>
> - BUG_ON(current->bio_list);
> + BUG_ON(current->bio_lists);
> BUG_ON(bch_keylist_empty(keys));
>
> bch_btree_op_init(&op.op, 0);
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index cd77216..b64d4f0 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -174,7 +174,7 @@ static inline int dm_bufio_cache_index(struct dm_bufio_client *c)
> #define DM_BUFIO_CACHE(c) (dm_bufio_caches[dm_bufio_cache_index(c)])
> #define DM_BUFIO_CACHE_NAME(c) (dm_bufio_cache_names[dm_bufio_cache_index(c)])
>
> -#define dm_bufio_in_request() (!!current->bio_list)
> +#define dm_bufio_in_request() (!!current->bio_lists)
>
> static void dm_bufio_lock(struct dm_bufio_client *c)
> {
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index b5c4be7..7afc71c 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -663,6 +663,13 @@ static inline void rdev_dec_pending(struct md_rdev *rdev, struct mddev *mddev)
> }
> }
>
> +static inline bool current_has_pending_bios(void)
> +{
> + return current->bio_lists && (
> + !bio_list_empty(¤t->bio_lists->recursion) ||
> + !bio_list_empty(¤t->bio_lists->remainder));
> +}
> +
> extern struct md_cluster_operations *md_cluster_ops;
> static inline int mddev_is_clustered(struct mddev *mddev)
> {
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index c7c8cde..811f2c0 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -876,8 +876,7 @@ static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
> (!conf->barrier ||
> ((conf->start_next_window <
> conf->next_resync + RESYNC_SECTORS) &&
> - current->bio_list &&
> - !bio_list_empty(current->bio_list))),
> + current_has_pending_bios())),
> conf->resync_lock);
> conf->nr_waiting--;
> }
> @@ -1014,7 +1013,7 @@ static void raid1_unplug(struct blk_plug_cb *cb, bool from_schedule)
> struct r1conf *conf = mddev->private;
> struct bio *bio;
>
> - if (from_schedule || current->bio_list) {
> + if (from_schedule || current->bio_lists) {
> spin_lock_irq(&conf->device_lock);
> bio_list_merge(&conf->pending_bio_list, &plug->pending);
> conf->pending_count += plug->pending_cnt;
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index c7de2a5..2c70717 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -945,8 +945,7 @@ static void wait_barrier(struct r10conf *conf)
> wait_event_lock_irq(conf->wait_barrier,
> !conf->barrier ||
> (conf->nr_pending &&
> - current->bio_list &&
> - !bio_list_empty(current->bio_list)),
> + current_has_pending_bios()),
> conf->resync_lock);
> conf->nr_waiting--;
> }
> @@ -1022,7 +1021,7 @@ static void raid10_unplug(struct blk_plug_cb *cb, bool from_schedule)
> struct r10conf *conf = mddev->private;
> struct bio *bio;
>
> - if (from_schedule || current->bio_list) {
> + if (from_schedule || current->bio_lists) {
> spin_lock_irq(&conf->device_lock);
> bio_list_merge(&conf->pending_bio_list, &plug->pending);
> conf->pending_count += plug->pending_cnt;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 9faebf7..57e45a0 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -598,6 +598,17 @@ struct bio_list {
> struct bio *tail;
> };
>
> +/* for generic_make_request() */
> +struct recursion_to_iteration_bio_lists {
> + /* For stacking drivers submitting to their respective backend.
> + * Added to tail, processed in FIFO order. */
> + struct bio_list recursion;
> + /* For pushing back the "remainder" part resulting from calling
> + * blk_queue_split(). Added to head, processed in LIFO order,
> + * once the "recursion" list has been drained. */
> + struct bio_list remainder;
> +};
> +
> static inline int bio_list_empty(const struct bio_list *bl)
> {
> return bl->head == NULL;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 6e42ada..146eedc 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -128,7 +128,7 @@ struct sched_attr {
>
> struct futex_pi_state;
> struct robust_list_head;
> -struct bio_list;
> +struct recursion_to_iteration_bio_lists;
> struct fs_struct;
> struct perf_event_context;
> struct blk_plug;
> @@ -1727,7 +1727,7 @@ struct task_struct {
> void *journal_info;
>
> /* stacked block device info */
> - struct bio_list *bio_list;
> + struct recursion_to_iteration_bio_lists *bio_lists;
>
> #ifdef CONFIG_BLOCK
> /* stack plugging */
> --
> 1.9.1
>
^ permalink raw reply
* [PATCH v3 1/3] bcache: Remove redundant parameter for cache_alloc()
From: Yijing Wang @ 2016-07-04 1:23 UTC (permalink / raw)
To: axboe, Kent Overstreet
Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel,
Yijing Wang
Cache_sb is not used in cache_alloc, and we have copied
sb info to cache->sb already, remove it.
Reviewed-by: Coly Li <colyli@suse.de>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
drivers/md/bcache/super.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index f5dbb4e..aecaace 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1803,7 +1803,7 @@ void bch_cache_release(struct kobject *kobj)
module_put(THIS_MODULE);
}
-static int cache_alloc(struct cache_sb *sb, struct cache *ca)
+static int cache_alloc(struct cache *ca)
{
size_t free;
struct bucket *b;
@@ -1858,7 +1858,7 @@ static int register_cache(struct cache_sb *sb, struct page *sb_page,
if (blk_queue_discard(bdev_get_queue(ca->bdev)))
ca->discard = CACHE_DISCARD(&ca->sb);
- ret = cache_alloc(sb, ca);
+ ret = cache_alloc(ca);
if (ret != 0)
goto err;
--
1.7.1
^ permalink raw reply related
* [PATCH v3 2/3] bcache: update document info
From: Yijing Wang @ 2016-07-04 1:23 UTC (permalink / raw)
To: axboe, Kent Overstreet
Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel,
Yijing Wang
There is no return in continue_at(), update the documentation.
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
drivers/md/bcache/closure.c | 2 +-
drivers/md/bcache/closure.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
index 9eaf1d6..864e673 100644
--- a/drivers/md/bcache/closure.c
+++ b/drivers/md/bcache/closure.c
@@ -112,7 +112,7 @@ bool closure_wait(struct closure_waitlist *waitlist, struct closure *cl)
EXPORT_SYMBOL(closure_wait);
/**
- * closure_sync - sleep until a closure a closure has nothing left to wait on
+ * closure_sync - sleep until a closure has nothing left to wait on
*
* Sleeps until the refcount hits 1 - the thread that's running the closure owns
* the last refcount.
diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
index 782cc2c..9b2fe2d 100644
--- a/drivers/md/bcache/closure.h
+++ b/drivers/md/bcache/closure.h
@@ -31,7 +31,8 @@
* passing it, as you might expect, the function to run when nothing is pending
* and the workqueue to run that function out of.
*
- * continue_at() also, critically, is a macro that returns the calling function.
+ * continue_at() also, critically, requires a 'return' immediately following the
+ * location where this macro is referenced, to return to the calling function.
* There's good reason for this.
*
* To use safely closures asynchronously, they must always have a refcount while
--
1.7.1
^ permalink raw reply related
* [PATCH v3 3/3] bcache: Remove redundant block_size assignment
From: Yijing Wang @ 2016-07-04 1:23 UTC (permalink / raw)
To: axboe, Kent Overstreet
Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel,
Yijing Wang
We have assigned sb->block_size before the switch,
so remove the redundant one.
Reviewed-by: Coly Li <colyli@suse.de>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Acked-by: Eric Wheeler <bcache@lists.ewheeler.net>
---
drivers/md/bcache/super.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index aecaace..bf4b100 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -134,7 +134,6 @@ static const char *read_super(struct cache_sb *sb, struct block_device *bdev,
case BCACHE_SB_VERSION_CDEV:
case BCACHE_SB_VERSION_CDEV_WITH_UUID:
sb->nbuckets = le64_to_cpu(s->nbuckets);
- sb->block_size = le16_to_cpu(s->block_size);
sb->bucket_size = le16_to_cpu(s->bucket_size);
sb->nr_in_set = le16_to_cpu(s->nr_in_set);
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v3 2/3] bcache: update document info
From: Coly Li @ 2016-07-04 5:49 UTC (permalink / raw)
To: Yijing Wang, axboe, Kent Overstreet
Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel
In-Reply-To: <1467595414-20844-1-git-send-email-wangyijing@huawei.com>
在 16/7/4 上午9:23, Yijing Wang 写道:
> There is no return in continue_at(), update the documentation.
>
Thanks.
Acked-by: Coly Li <colyli@suse.de>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
> drivers/md/bcache/closure.c | 2 +-
> drivers/md/bcache/closure.h | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
> index 9eaf1d6..864e673 100644
> --- a/drivers/md/bcache/closure.c
> +++ b/drivers/md/bcache/closure.c
> @@ -112,7 +112,7 @@ bool closure_wait(struct closure_waitlist *waitlist, struct closure *cl)
> EXPORT_SYMBOL(closure_wait);
>
> /**
> - * closure_sync - sleep until a closure a closure has nothing left to wait on
> + * closure_sync - sleep until a closure has nothing left to wait on
> *
> * Sleeps until the refcount hits 1 - the thread that's running the closure owns
> * the last refcount.
> diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
> index 782cc2c..9b2fe2d 100644
> --- a/drivers/md/bcache/closure.h
> +++ b/drivers/md/bcache/closure.h
> @@ -31,7 +31,8 @@
> * passing it, as you might expect, the function to run when nothing is pending
> * and the workqueue to run that function out of.
> *
> - * continue_at() also, critically, is a macro that returns the calling function.
> + * continue_at() also, critically, requires a 'return' immediately following the
> + * location where this macro is referenced, to return to the calling function.
> * There's good reason for this.
> *
> * To use safely closures asynchronously, they must always have a refcount while
>
--
Coly Li
^ permalink raw reply
* Re: URE, link resets, user hostile defaults
From: Hannes Reinecke @ 2016-07-04 6:00 UTC (permalink / raw)
To: Chris Murphy, Edward Kuns; +Cc: Zygo Blaxell, Linux-RAID
In-Reply-To: <CAJCQCtT2Sxi1GgmNaO8i46LuELpA_nrWLnVzAQMz7Qx_mdwXvw@mail.gmail.com>
On 07/01/2016 10:43 PM, Chris Murphy wrote:
> Here's a fun one of these I just got off the Fedora users mailing list
> with a laptop drive that's apparently hanging on *write*. This I would
> not expect to take a long time for a drive to figure out, but... there
> are more resets than there are write errors, and in fact there's no
> discrete write error from the drive, all we know is the failed command
> is a WRITE command.
>
> What seems to happen is, everything in the queue gets obliterated in
> the reset, and when ext4 finds out everything failed, not just one
> write, it barfs and goes read only.
>
> http://pastebin.com/3JAL297z
>
> How might this turn out differently if the drive reported a single
> discrete write error? I don't know how any file system tolerates this
> because it's so rare. Would ext4 just try to write again? Would it try
> to write to the same sector or another one? Or maybe the write finally
> succeeds by resulting in a remap (?) But this sure is dang slow to
> recover from a bad write. I don't understand the engineering rational
> for this. Maybe it's a firmware bug?
>
>
Could be. At the very least it's an issue with EH interaction.
ATA COMRESET fails, ie libata EH fails to reset the SATA link.
Which is pretty terminal, so the device is set to offline afterwards.
This is most definitely an ATA issue, and doesn't really belong in this
context.
(Have you reported it on linux-ide?)
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/3] bcache: update document info
From: wangyijing @ 2016-07-04 6:43 UTC (permalink / raw)
To: Coly Li, axboe, Kent Overstreet
Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel
In-Reply-To: <489c5709-29d0-077c-d8b7-92f12447fc08@coly.li>
在 2016/7/4 13:49, Coly Li 写道:
> 在 16/7/4 上午9:23, Yijing Wang 写道:
>> There is no return in continue_at(), update the documentation.
>>
>
> Thanks.
>
> Acked-by: Coly Li <colyli@suse.de>
Thanks very much!
>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>> drivers/md/bcache/closure.c | 2 +-
>> drivers/md/bcache/closure.h | 3 ++-
>> 2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
>> index 9eaf1d6..864e673 100644
>> --- a/drivers/md/bcache/closure.c
>> +++ b/drivers/md/bcache/closure.c
>> @@ -112,7 +112,7 @@ bool closure_wait(struct closure_waitlist *waitlist, struct closure *cl)
>> EXPORT_SYMBOL(closure_wait);
>>
>> /**
>> - * closure_sync - sleep until a closure a closure has nothing left to wait on
>> + * closure_sync - sleep until a closure has nothing left to wait on
>> *
>> * Sleeps until the refcount hits 1 - the thread that's running the closure owns
>> * the last refcount.
>> diff --git a/drivers/md/bcache/closure.h b/drivers/md/bcache/closure.h
>> index 782cc2c..9b2fe2d 100644
>> --- a/drivers/md/bcache/closure.h
>> +++ b/drivers/md/bcache/closure.h
>> @@ -31,7 +31,8 @@
>> * passing it, as you might expect, the function to run when nothing is pending
>> * and the workqueue to run that function out of.
>> *
>> - * continue_at() also, critically, is a macro that returns the calling function.
>> + * continue_at() also, critically, requires a 'return' immediately following the
>> + * location where this macro is referenced, to return to the calling function.
>> * There's good reason for this.
>> *
>> * To use safely closures asynchronously, they must always have a refcount while
>>
>
>
^ permalink raw reply
* raid5/6: general protection fault in async_copy_data
From: Joey Liao @ 2016-07-04 8:07 UTC (permalink / raw)
To: linux-raid
Hi,
We meet a general protection fault issue when doing the I/O stress
test. (Please refer the following call trace.)
The linux version what we use is 3.19.8.
Besides, it will not only happen in raid5 but also raid6, and our raid
is normal and clean (not in resync or rebuild or degraded state) in
this case.
The general protection fault logs just show-up suddenly during the I/O
stress test, and no other logs else are before it.
It seems that it is an old issue that someone has been meet in linux 3.8.13.
http://comments.gmane.org/gmane.linux.raid/48737
However, it did not come to an exact conclusion then.
Does anyone have any idea about this situation?
<4>[ 8415.258965] general protection fault: 0000 [#1] SMP
<4>[ 8415.263946] Modules linked in: vfio_pci vfio_iommu_type1 vfio
vringh virtio_scsi virtio_pci virtio_mmio virtio_console
virtio_balloon virtio_rng virtio_blk virtio_net virtio_ring virtio
vhost_net vhost tun macvtap macvlan kvm_intel kvm fbdisk(O) xt_mark
ipt_MASQUERADE iptable_nat nf_nat_masquerade_ipv4 nf_nat_ipv4 nf_nat
ppp_deflate bsd_comp ppp_mppe ppp_async ppp_generic slhc iscsi_tcp(O)
libiscsi_tcp(O) libiscsi(O) scsi_transport_iscsi(O) btusb bluetooth
bonding bridge stp ipv6 uvcvideo videobuf2_vmalloc videobuf2_memops
videobuf2_core snd_usb_caiaq snd_usb_audio snd_usbmidi_lib
snd_seq_midi snd_rawmidi fnotify(PO) udf isofs iTCO_wdt psnap llc
ufsd(PO) jnl(O) pl2303 usbserial intel_ips drbd(O) flashcache(O)
dm_thin_pool dm_bio_prison dm_persistent_data hal_netlink(O) coretemp
r8152 usbnet mii igb e1000e(O) mpt3sas mpt2sas scsi_transport_sas
raid_class uas usb_storage xhci_pci xhci_hcd usblp uhci_hcd ehci_pci
ehci_hcd [last unloaded: fbdisk]
<4>[ 8415.348888] CPU: 0 PID: 4611 Comm: md1_raid5 Tainted: P U
O 3.19.8 #1
<4>[ 8415.356137] Hardware name: Default string Default string/SKYBAY,
BIOS QX80AR20 06/07/2016
<4>[ 8415.364249] task: ffff880847616210 ti: ffff880831950000 task.ti:
ffff880831950000
<4>[ 8415.371667] RIP: 0010:[<ffffffff813c4446>] [<ffffffff813c4446>]
memcpy+0x6/0x110
<4>[ 8415.379126] RSP: 0000:ffff880831953990 EFLAGS: 00210206
<4>[ 8415.384398] RAX: ffff880832e87000 RBX: ffff880831953a18 RCX:
0000000000001000
<4>[ 8415.391475] RDX: 0000000000001000 RSI: 0845080000067000 RDI:
ffff880832e87000
<4>[ 8415.398553] RBP: ffff8808319539c8 R08: 0000000000001000 R09:
ffff880831953a18
<4>[ 8415.405660] R10: 0000000000000001 R11: 0000000000000001 R12:
ffffea0020cba1c0
<4>[ 8415.412750] R13: 0000000000000000 R14: 002100000000000e R15:
0000000000067000
<4>[ 8415.419829] FS: 0000000000000000(0000)
GS:ffff88086dc00000(0000) knlGS:0000000000000000
<4>[ 8415.427852] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[ 8415.433550] CR2: 0000000008120480 CR3: 0000000001c7d000 CR4:
00000000003407f0
<4>[ 8415.440648] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
<4>[ 8415.447748] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
<4>[ 8415.454823] Stack:
<4>[ 8415.456849] ffffffff8138408f 0000000000001000 0000000000080000
0000000000080000
<4>[ 8415.464283] 0000000000000000 0000000000000000 0000000000001000
ffff880831953a78
<4>[ 8415.471720] ffffffff8169911e ffff880831953a18 ffffffff8138a5b7
ffff880831953a18
<4>[ 8415.479144] Call Trace:
<4>[ 8415.481578] [<ffffffff8138408f>] ? async_memcpy+0x9f/0x100
<4>[ 8415.487122] [<ffffffff8169911e>] async_copy_data+0x12e/0x260
<4>[ 8415.492824] [<ffffffff8138a5b7>] ? bio_attempt_front_merge+0xb7/0xf0
<4>[ 8415.499214] [<ffffffff81699bb6>] raid_run_ops+0x876/0x1190
<4>[ 8415.504741] [<ffffffff8138c5b3>] ? generic_make_request+0xa3/0xf0
<4>[ 8415.510872] [<ffffffff8169f716>] ? ops_run_io+0x36/0x820
<4>[ 8415.516233] [<ffffffff81085763>] ? __wake_up+0x53/0x70
<4>[ 8415.521417] [<ffffffff816a0e7b>] handle_stripe+0xa8b/0x1b50
<4>[ 8415.527031] [<ffffffff81085206>] ? __wake_up_common+0x16/0x90
<4>[ 8415.532816] [<ffffffff816a22da>] handle_active_stripes+0x39a/0x450
<4>[ 8415.539035] [<ffffffff816a2849>] raid5d+0x399/0x630
<4>[ 8415.543964] [<ffffffff816ac4ed>] md_thread+0x7d/0x130
<4>[ 8415.549064] [<ffffffff81085370>] ? woken_wake_function+0x20/0x20
<4>[ 8415.555107] [<ffffffff816ac470>] ? errors_store+0x70/0x70
<4>[ 8415.560552] [<ffffffff8106a263>] kthread+0xe3/0xf0
<4>[ 8415.565411] [<ffffffff8106a180>] ? kthreadd+0x160/0x160
<4>[ 8415.570682] [<ffffffff8192d048>] ret_from_fork+0x58/0x90
<4>[ 8415.576044] [<ffffffff8106a180>] ? kthreadd+0x160/0x160
<4>[ 8415.581315] Code: 24 4c 8b 64 24 08 c9 c3 e8 68 f9 ff ff 41 80
7c 24 05 00 75 d3 eb e4 90 90 90 90 90 90 90 90 90 90 90 90 90 90 48
89 f8 48 89 d1 <f3> a4 c3 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 20 4c 8b
06 4c 8b
<1>[ 8415.601660] RIP [<ffffffff813c4446>] memcpy+0x6/0x110
<4>[ 8415.606788] RSP <ffff880831953990>
<4>[ 8415.612291] ---[ end trace 962cfd98d43b82fa ]---
<4>[ 8415.620528] ------------[ cut here ]------------
^ permalink raw reply
* Re: [RFC] block: fix blk_queue_split() resource exhaustion
From: Lars Ellenberg @ 2016-07-04 8:20 UTC (permalink / raw)
To: Ming Lei
Cc: linux-block, Roland Kammerer, Jens Axboe, NeilBrown,
Kent Overstreet, Shaohua Li, Alasdair Kergon, Mike Snitzer,
open list:DEVICE-MAPPER (LVM), Ingo Molnar, Peter Zijlstra,
Takashi Iwai, Jiri Kosina, Zheng Liu, Keith Busch,
Martin K. Petersen, Kirill A. Shutemov, Linux Kernel Mailing List,
open list:BCACHE (BLOCK LAYER CACHE),
open list:SOFTWARE RAID (Multiple Disks) SUPPORT
In-Reply-To: <CACVXFVMTp17gcM8HEMrZ+0TJWgpGk+VWDgC7t+rMAancWZEFFw@mail.gmail.com>
On Sat, Jul 02, 2016 at 06:28:29PM +0800, Ming Lei wrote:
> The idea looks good, but not sure it can cover all cases of
> dm over brbd or brbd over dm and maintaining two lists
> becomes too complicated.
>
> One clean solution may be to convert the loop of generic_make_request()
> into the following way:
>
> do {
> struct bio *splitted, *remainder = NULL;
> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>
> blk_queue_split(q, &bio, &remainder, q->bio_split);
>
> ret = q->make_request_fn(q, bio);
>
> if (remainder)
> bio_list_add(current->bio_list, remainder);
> bio = bio_list_pop(current->bio_list);
> } while (bio)
Not good enough.
Consider DRBD on device-mapper on device-mapper on scsi,
or insert multipath and / or md raid into the stack.
The iterative call to q->make_request_fn() in the second iteration
may queue bios after the remainder.
Goal was to first process all "deeper level" bios
before processing the remainder.
You can achieve this by doing last-in-first-out on bio_list,
or by using two lists, as I suggested in the original post.
Lars
^ permalink raw reply
* Re: [RFC] block: fix blk_queue_split() resource exhaustion
From: Ming Lei @ 2016-07-04 10:47 UTC (permalink / raw)
To: Lars Ellenberg
Cc: linux-block, Roland Kammerer, Jens Axboe, NeilBrown,
Kent Overstreet, Shaohua Li, Alasdair Kergon, Mike Snitzer,
open list:DEVICE-MAPPER (LVM), Ingo Molnar, Peter Zijlstra,
Takashi Iwai, Jiri Kosina, Zheng Liu, Keith Busch,
Martin K. Petersen, Kirill A. Shutemov, Linux Kernel Mailing List,
open list:BCACHE (BLOCK LAYER CACHE),
open list:SOFTWARE RAID (Multiple Disks) SUPPORT
In-Reply-To: <20160704082006.GN3239@soda.linbit>
On Mon, Jul 4, 2016 at 4:20 PM, Lars Ellenberg
<lars.ellenberg@linbit.com> wrote:
> On Sat, Jul 02, 2016 at 06:28:29PM +0800, Ming Lei wrote:
>> The idea looks good, but not sure it can cover all cases of
>> dm over brbd or brbd over dm and maintaining two lists
>> becomes too complicated.
>>
>> One clean solution may be to convert the loop of generic_make_request()
>> into the following way:
>>
>> do {
>> struct bio *splitted, *remainder = NULL;
>> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>>
>> blk_queue_split(q, &bio, &remainder, q->bio_split);
>>
>> ret = q->make_request_fn(q, bio);
>>
>> if (remainder)
>> bio_list_add(current->bio_list, remainder);
>> bio = bio_list_pop(current->bio_list);
>> } while (bio)
>
> Not good enough.
>
> Consider DRBD on device-mapper on device-mapper on scsi,
> or insert multipath and / or md raid into the stack.
> The iterative call to q->make_request_fn() in the second iteration
> may queue bios after the remainder.
But this remainder is not the top remainder any more, I guess you mean
the following situation:
- drbd_make_request(bio)
->generic_make_request(bio)
->bio is added into current->bio_list
- bio is splitted as bio_a and bio_b in generic_make_request()
- dm_make_request(bio_a)
->generic_make_request(bio_a)
->bio_a is add into current_list
- bio_a is splitted as bio_a_a and bio_a_b in generic_make_request()
- dm_make_request(bio_a_a)
->.....
- bio_a_b is added into current->bio_list
- dm_make_request(bio_a_b)
But it is correct because bio_a depends on both bio_a_a and bio_a_b.
Or I understand you wrong?
>
> Goal was to first process all "deeper level" bios
> before processing the remainder.
For the reported bio splitting issue, I think the goal is to make sure all
BIOs generated from 'bio' inside .make_request_fn(bio) are queued
before the 'current' remainder. Cause it is the issue introduced by
blk_split_bio().
Thanks,
Ming
>
> You can achieve this by doing last-in-first-out on bio_list,
> or by using two lists, as I suggested in the original post.
>
> Lars
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH RESENT] dm: Check kthread_run's return value
From: Minfei Huang @ 2016-07-04 15:25 UTC (permalink / raw)
To: agk, snitzer, shli; +Cc: dm-devel, linux-raid, linux-kernel, Minfei Huang
kthread function is used to process kthread_work. And there is no return
value checking during create this thread. Add this checking to fix this
issue.
Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
drivers/md/dm.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 1b2f962..d68b9d2 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2654,12 +2654,15 @@ struct queue_limits *dm_get_queue_limits(struct mapped_device *md)
}
EXPORT_SYMBOL_GPL(dm_get_queue_limits);
-static void dm_old_init_rq_based_worker_thread(struct mapped_device *md)
+static int dm_old_init_rq_based_worker_thread(struct mapped_device *md)
{
/* Initialize the request-based DM worker thread */
init_kthread_worker(&md->kworker);
md->kworker_task = kthread_run(kthread_worker_fn, &md->kworker,
"kdmwork-%s", dm_device_name(md));
+ if (IS_ERR(md->kworker_task))
+ return -ENOMEM;
+ return 0;
}
/*
@@ -2667,6 +2670,8 @@ static void dm_old_init_rq_based_worker_thread(struct mapped_device *md)
*/
static int dm_old_init_request_queue(struct mapped_device *md)
{
+ int ret;
+
/* Fully initialize the queue */
if (!blk_init_allocated_queue(md->queue, dm_request_fn, NULL))
return -EINVAL;
@@ -2678,7 +2683,9 @@ static int dm_old_init_request_queue(struct mapped_device *md)
blk_queue_softirq_done(md->queue, dm_softirq_done);
blk_queue_prep_rq(md->queue, dm_old_prep_fn);
- dm_old_init_rq_based_worker_thread(md);
+ ret = dm_old_init_rq_based_worker_thread(md);
+ if (ret < 0)
+ return ret;
elv_register_queue(md->queue);
--
2.6.3
^ permalink raw reply related
* Re: URE, link resets, user hostile defaults
From: Pasi Kärkkäinen @ 2016-07-04 21:43 UTC (permalink / raw)
To: Zygo Blaxell; +Cc: Chris Murphy, Hannes Reinecke, linux-raid
In-Reply-To: <20160629121751.GT15597@hungrycats.org>
On Wed, Jun 29, 2016 at 08:17:51AM -0400, Zygo Blaxell wrote:
> On Tue, Jun 28, 2016 at 11:33:36AM -0600, Chris Murphy wrote:
> > On Tue, Jun 28, 2016 at 12:33 AM, Hannes Reinecke <hare@suse.de> wrote:
> > > Can you post a message log detailing this problem?
> >
> > Just over the weekend Phil Turmel posted an email with a bunch of back
> > reading on the subject of timeout mismatches for someone to read. I've
> > lost track of how many user emails he's replied to, discovering this
> > common misconfiguration, and get it straightened out and more often
> > than not helping the user recover data that otherwise would have been
> > lost *because* of hard link resetting instead of explicit read errors.
>
> OK, but the two links you provided are not examples of these.
>
Here's one of the threads where Phil explains the issue:
http://marc.info/?l=linux-raid&m=133665797115876&w=2
quote:
"A very common report I see on this mailing list is people who have lost arrays
where the drives all appear to be healthy.
Given the large size of today's hard drives, even healthy drives will occasionally
have an unrecoverable read error.
When this happens in a raid array with a desktop drive without SCTERC,
the driver times out and reports an error to MD. MD proceeds to
reconstruct the missing data and tries to write it back to the bad
sector. However, that drive is still trying to read the bad sector and
ignores the controller. The write is immediately rejected. BOOM! The
*write* error ejects that member from the array. And you are now
degraded.
If you don't notice the degraded array right away, you probably won't
notice until a URE on another drive pops up. Once that happens, you
can't complete a resync to revive the array.
Running a "check" or "repair" on an array without TLER will have the
opposite of the intended effect: any URE will kick a drive out instead
of fixing it.
In the same scenario with an enterprise drive, or a drive with SCTERC
turned on, the drive read times out before the controller driver, the
controller never resets the link to the drive, and the followup write
succeeds. (The sector is either successfully corrected in place, or
it is relocated by the drive.) No BOOM."
-- Pasi
^ permalink raw reply
* [PATCH] Monitor: release /proc/mdstat fd when no arrays present
From: Tomasz Majchrzak @ 2016-07-05 7:12 UTC (permalink / raw)
To: linux-raid
Cc: Jes.Sorensen, aleksey.obitotskiy, pawel.baldysiak,
artur.paszkiewicz
If md kernel module is reloaded, /proc/mdstat cannot be accessed ("cat:
/proc/mdstat: No such file or directory"). The reason is mdadm monitor
still holds a file descriptor to previous /proc/mdstat instance. It
leads to really confusing outcome of the following operations - mdadm
seems to run without errors, however some udev rules don't get executed
and new array doesn't work.
Add a check if lseek was successful as it fails if md kernel module has
been unloaded - close a file descriptor then. The problem is mdadm
monitor doesn't always do it before next operation takes place. To
prevent it monitor always releases /proc/mdstat descriptor when there
are no arrays to be monitored, just in case driver unload happens in a
moment.
Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
Monitor.c | 2 ++
mdstat.c | 6 +++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/Monitor.c b/Monitor.c
index 4adc237..802a9d9 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -213,6 +213,8 @@ int Monitor(struct mddev_dev *devlist,
if (mdstat)
free_mdstat(mdstat);
mdstat = mdstat_read(oneshot?0:1, 0);
+ if (!mdstat)
+ mdstat_close();
for (st=statelist; st; st=st->next)
if (check_array(st, mdstat, c->test, &info,
diff --git a/mdstat.c b/mdstat.c
index 2972cdf..3962896 100644
--- a/mdstat.c
+++ b/mdstat.c
@@ -133,7 +133,11 @@ struct mdstat_ent *mdstat_read(int hold, int start)
int fd;
if (hold && mdstat_fd != -1) {
- lseek(mdstat_fd, 0L, 0);
+ off_t offset = lseek(mdstat_fd, 0L, 0);
+ if (offset == (off_t)-1) {
+ mdstat_close();
+ return NULL;
+ }
fd = dup(mdstat_fd);
if (fd >= 0)
f = fdopen(fd, "r");
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v3 1/3] bcache: Remove redundant parameter for cache_alloc()
From: Jens Axboe @ 2016-07-05 17:35 UTC (permalink / raw)
To: Yijing Wang, Kent Overstreet
Cc: Eric Wheeler, Coly Li, linux-bcache, linux-raid, linux-kernel
In-Reply-To: <1467595405-20805-1-git-send-email-wangyijing@huawei.com>
On 07/03/2016 07:23 PM, Yijing Wang wrote:
> Cache_sb is not used in cache_alloc, and we have copied
> sb info to cache->sb already, remove it.
Added this, and 2-3/3 for 4.8, thanks.
--
Jens Axboe
^ permalink raw reply
* Request for assistance
From: o1bigtenor @ 2016-07-06 0:13 UTC (permalink / raw)
To: Linux-RAID
Greetings
Running a Raid 10 array with 4 - 3 TB drives. Have a UPS but this area
gets significant lightning and also brownout (rural power) events.
Found the array was read only this morning. Thought that rebooting the
system might correct things - - - it did not as the array did not
load.
commands used followed by system response
mdadm --detail /dev/md0
mdadm: md device /dev/md0 does not appear to be active.
cat /proc/mdstat
md0 : inactive sdc1[5](S) sdf1[8](S) sde1[7](S) sdb1[4](S)
mdadm -E /dev/sdb1
sdc1
sde1
sdf1
everything is the same except for 2 items
sde and sdf have uptime listed from July 04 05:50:46
events 64841
array state of AAAA
sdb and sdc have uptime listed from July 05 01:57:38
events 64844
array state of AAA.
Do I just re-create the array?
TIA
Dee
^ permalink raw reply
* Re: Request for assistance
From: Adam Goryachev @ 2016-07-06 1:55 UTC (permalink / raw)
To: o1bigtenor, Linux-RAID
In-Reply-To: <CAPpdf58z4JagWmP=xiUj9qOU-bdyN7W=EvcM9evjCT4ZWp+VxQ@mail.gmail.com>
On 06/07/16 10:13, o1bigtenor wrote:
> Greetings
>
> Running a Raid 10 array with 4 - 3 TB drives. Have a UPS but this area
> gets significant lightning and also brownout (rural power) events.
>
> Found the array was read only this morning. Thought that rebooting the
> system might correct things - - - it did not as the array did not
> load.
>
> commands used followed by system response
>
> mdadm --detail /dev/md0
> mdadm: md device /dev/md0 does not appear to be active.
>
> cat /proc/mdstat
> md0 : inactive sdc1[5](S) sdf1[8](S) sde1[7](S) sdb1[4](S)
>
> mdadm -E /dev/sdb1
> sdc1
> sde1
> sdf1
>
> everything is the same except for 2 items
>
> sde and sdf have uptime listed from July 04 05:50:46
> events 64841
> array state of AAAA
>
> sdb and sdc have uptime listed from July 05 01:57:38
> events 64844
> array state of AAA.
>
>
>
> Do I just re-create the array?
>
No, not if you value your data. Only re-create the array if you are told
to by someone (knowledgeable) on the list.
In your case, I think you should stop the array.
mdadm --stop /dev/md0
Make sure there is nothing listed in /proc/mdstat
Then try to assemble the array, but force the events to match:
mdadm --assemble /dev/md0 --force /dev/sd[bcef]1
If that doesn't work, then include the output from dmesg as well as
/proc/mdstat and any commandline output generated.
You might also want to examine why two drives dropped, referring to logs
or similar might assist.
Regards,
Adam
--
Adam Goryachev Website Managers www.websitemanagers.com.au
^ permalink raw reply
* Re: Request for assistance
From: keld @ 2016-07-06 7:39 UTC (permalink / raw)
To: o1bigtenor; +Cc: Linux-RAID
In-Reply-To: <CAPpdf58z4JagWmP=xiUj9qOU-bdyN7W=EvcM9evjCT4ZWp+VxQ@mail.gmail.com>
What operating system and version are you running?
Best regards
keld
On Tue, Jul 05, 2016 at 07:13:23PM -0500, o1bigtenor wrote:
> Greetings
>
> Running a Raid 10 array with 4 - 3 TB drives. Have a UPS but this area
> gets significant lightning and also brownout (rural power) events.
>
> Found the array was read only this morning. Thought that rebooting the
> system might correct things - - - it did not as the array did not
> load.
>
> commands used followed by system response
>
> mdadm --detail /dev/md0
> mdadm: md device /dev/md0 does not appear to be active.
>
> cat /proc/mdstat
> md0 : inactive sdc1[5](S) sdf1[8](S) sde1[7](S) sdb1[4](S)
>
> mdadm -E /dev/sdb1
> sdc1
> sde1
> sdf1
>
> everything is the same except for 2 items
>
> sde and sdf have uptime listed from July 04 05:50:46
> events 64841
> array state of AAAA
>
> sdb and sdc have uptime listed from July 05 01:57:38
> events 64844
> array state of AAA.
>
>
>
> Do I just re-create the array?
>
> TIA
>
> Dee
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Request for assistance
From: o1bigtenor @ 2016-07-06 12:14 UTC (permalink / raw)
To: Adam Goryachev; +Cc: Linux-RAID
In-Reply-To: <577C64FA.6050504@websitemanagers.com.au>
On Tue, Jul 5, 2016 at 8:55 PM, Adam Goryachev
<mailinglists@websitemanagers.com.au> wrote:
> On 06/07/16 10:13, o1bigtenor wrote:
>>
>> Greetings
>>
>> Running a Raid 10 array with 4 - 3 TB drives. Have a UPS but this area
>> gets significant lightning and also brownout (rural power) events.
>>
snip
>>
>> Do I just re-create the array?
>>
> No, not if you value your data. Only re-create the array if you are told to
> by someone (knowledgeable) on the list.
>
> In your case, I think you should stop the array.
> mdadm --stop /dev/md0
> Make sure there is nothing listed in /proc/mdstat
> Then try to assemble the array, but force the events to match:
> mdadm --assemble /dev/md0 --force /dev/sd[bcef]1
>
> If that doesn't work, then include the output from dmesg as well as
> /proc/mdstat and any commandline output generated.
>
> You might also want to examine why two drives dropped, referring to logs or
> similar might assist.
>
mdadm --stop /dev/md0
cat /proc/mdstat
indicated no md (can't remember the exact response but it said
nothing there)
mdadm --assemble /dev/md0 --force /dev/sd[bcef]1 to
mdadm :forcing event count in /dev/sde1(2) from 64841 to 64844
mdadm :forcing event count in /dev/sdf1(3) from 64841 to 64844
mdadm: clearing FAULTY flag for device 3 in /dev/md0 for /dev/sdf1
mdadm: Marking array /dev/md0 as 'clean'
mdadm: /dev/md0 has been started with 4 drives
So my array is back up - - - thank you very much for your assistance!!!
What does the 'clearing FAULTY flag . . ' mean?
Regards
Dee
^ permalink raw reply
* Re: Request for assistance
From: o1bigtenor @ 2016-07-06 12:15 UTC (permalink / raw)
To: keld; +Cc: Linux-RAID
In-Reply-To: <20160706073934.GA8404@www5.open-std.org>
On Wed, Jul 6, 2016 at 2:39 AM, <keld@keldix.com> wrote:
> What operating system and version are you running?
>
Running Debian testing.
Thanks for the assistance.
Dee
^ permalink raw reply
* Re: [RFC] block: fix blk_queue_split() resource exhaustion
From: Lars Ellenberg @ 2016-07-06 12:38 UTC (permalink / raw)
To: Ming Lei
Cc: linux-block, Roland Kammerer, Jens Axboe, NeilBrown,
Kent Overstreet, Shaohua Li, Alasdair Kergon, Mike Snitzer,
open list:DEVICE-MAPPER (LVM), Ingo Molnar, Peter Zijlstra,
Takashi Iwai, Jiri Kosina, Zheng Liu, Keith Busch,
Martin K. Petersen, Kirill A. Shutemov, Linux Kernel Mailing List,
open list:BCACHE (BLOCK LAYER CACHE),
open list:SOFTWARE RAID (Multiple Disks) SUPPORT
In-Reply-To: <CACVXFVNTKsRQ0PQTL47-ceP-KqMR-zR4DO1v-ey5NTUYv8bUog@mail.gmail.com>
On Mon, Jul 04, 2016 at 06:47:29PM +0800, Ming Lei wrote:
> >> One clean solution may be to convert the loop of generic_make_request()
> >> into the following way:
> >>
> >> do {
> >> struct bio *splitted, *remainder = NULL;
> >> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> >>
> >> blk_queue_split(q, &bio, &remainder, q->bio_split);
> >>
> >> ret = q->make_request_fn(q, bio);
> >>
> >> if (remainder)
> >> bio_list_add(current->bio_list, remainder);
> >> bio = bio_list_pop(current->bio_list);
> >> } while (bio)
> >
> > Not good enough.
> > Goal was to first process all "deeper level" bios
> > before processing the remainder.
>
> For the reported bio splitting issue, I think the goal is to make sure all
> BIOs generated from 'bio' inside .make_request_fn(bio) are queued
> before the 'current' remainder. Cause it is the issue introduced by
> blk_split_bio().
Stacking:
qA (limitting layer)
-> qB (remapping)
-> qC (remapping)
-> qD (hardware)
[in fact you don't even need four layers,
this is just to clarify that the stack may be more complex than you
assume it would be]
Columns below:
1) argument to generic_make_request() and its target queue.
2) current->bio_list
3) "in-flight" counter of qA.
==== In your new picture, iiuc:
generic_make_request(bio_orig)
NULL in-flight=0
bio_orig empty
blk_queue_split()
result:
bio_s, bio_r
qA->make_request_fn(bio_s)
in-flight=1
bio_c = bio_clone(bio_s)
generic_make_request(bio_c to qB)
bio_c
<-return
bio_list_add(bio_r)
bio_c, bio_r
bio_list_pop()
bio_r
qB->make_request_fn(bio_c)
(Assume it does not clone, but only remap.
But it may also be a striping layer,
and queue more than one bio here.)
generic_make_request(bio_c to qC)
bio_r, bio_c
<-return
bio_list_pop()
bio_c
qA->make_request_fn(bio_r) in-flight still 1
BLOCKS, because bio_c has not been processed to its final
destination qD yet, and not dispatched to hardware.
==== my suggestion
generic_make_request(bio_orig)
NULL in-flight=0
bio_orig empty in-flight=0
qA->make_request_fn(bio_orig)
blk_queue_split()
result:
bio_s, and bio_r stuffed away to head of remainder list.
in-flight=1
bio_c = bio_clone(bio_s)
generic_make_request(bio_c to qB)
bio_c
<-return
bio_c
bio_list_pop()
empty
qB->make_request_fn(bio_c)
(Assume it does not clone, but only remap.
But it may also be a striping layer,
and queue more than one bio here.)
generic_make_request(bio_c to qC)
bio_c
<-return
bio_list_pop()
empty
qC->make_request_fn(bio_c)
generic_make_request(bio_c to qD)
bio_c
<-return
bio_list_pop()
empty
qD->make_request_fn(bio_c)
dispatches to hardware
<-return
empty
bio_list_pop()
NULL, great, lets pop from remainder list
qA->make_request_fn(bio_r) in-flight=?
May block, but only until completion of bio_c.
Which may already have happened.
*makes progress*
Thanks,
Lars
^ permalink raw reply
* Re: Request for assistance
From: Wols Lists @ 2016-07-06 12:51 UTC (permalink / raw)
To: o1bigtenor, Adam Goryachev; +Cc: Linux-RAID
In-Reply-To: <CAPpdf59gF6BMpmCY5SNzNH2O_WFn17hMVnrjHAyA0pU_i+--uA@mail.gmail.com>
On 06/07/16 13:14, o1bigtenor wrote:
> On Tue, Jul 5, 2016 at 8:55 PM, Adam Goryachev
> <mailinglists@websitemanagers.com.au> wrote:
>> On 06/07/16 10:13, o1bigtenor wrote:
>>>
>>> Greetings
>>>
>>> Running a Raid 10 array with 4 - 3 TB drives. Have a UPS but this area
>>> gets significant lightning and also brownout (rural power) events.
>>>
> snip
>>>
>>> Do I just re-create the array?
>>>
>> No, not if you value your data. Only re-create the array if you are told to
>> by someone (knowledgeable) on the list.
>>
>> In your case, I think you should stop the array.
>> mdadm --stop /dev/md0
>> Make sure there is nothing listed in /proc/mdstat
>> Then try to assemble the array, but force the events to match:
>> mdadm --assemble /dev/md0 --force /dev/sd[bcef]1
>>
>> If that doesn't work, then include the output from dmesg as well as
>> /proc/mdstat and any commandline output generated.
>>
>> You might also want to examine why two drives dropped, referring to logs or
>> similar might assist.
>>
> mdadm --stop /dev/md0
> cat /proc/mdstat
> indicated no md (can't remember the exact response but it said
> nothing there)
> mdadm --assemble /dev/md0 --force /dev/sd[bcef]1 to
>
> mdadm :forcing event count in /dev/sde1(2) from 64841 to 64844
> mdadm :forcing event count in /dev/sdf1(3) from 64841 to 64844
> mdadm: clearing FAULTY flag for device 3 in /dev/md0 for /dev/sdf1
> mdadm: Marking array /dev/md0 as 'clean'
> mdadm: /dev/md0 has been started with 4 drives
>
> So my array is back up - - - thank you very much for your assistance!!!
>
But why did they drop ... are you using desktop drives? I use Seagate
Barracudas - NOT a particularly good idea. You should be using WD Red,
Seagate NAS, or similar.
"smartctl -x /dev/sdx" will give you an idea of what's going on. Search
the list for "timeout error" for an idea of the grief you'll get if
you're using desktop drives ...
If smartctl says smart is disabled, enable it. When I do, my drive comes
back (using the -x option again) saying "SCT Error Recovery not
supported". This is a no-no for a decent raid drive. I think the other
acronyms are ETL or TLS - either way you can control how the drive
reports an error back to the OS. Which is why you need proper raid
drives (the manufacturers have downgraded the firmware on desktop drives :-(
You need to fix the WHY or it could easily happen again. And this could
well be why ... (if you've had a problem on a desktop drive, it WILL
happen again, and data loss is quite likely ... even if you recover the
bulk of the drive).
Cheers,
Wol
^ permalink raw reply
* Re: [PATCH RESENT] dm: Check kthread_run's return value
From: Mike Snitzer @ 2016-07-06 13:16 UTC (permalink / raw)
To: Minfei Huang; +Cc: agk, shli, dm-devel, linux-raid, linux-kernel
In-Reply-To: <1467645927-4143-1-git-send-email-mnghuan@gmail.com>
On Mon, Jul 04 2016 at 11:25am -0400,
Minfei Huang <mnghuan@gmail.com> wrote:
> kthread function is used to process kthread_work. And there is no return
> value checking during create this thread. Add this checking to fix this
> issue.
>
> Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> ---
> drivers/md/dm.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 1b2f962..d68b9d2 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -2654,12 +2654,15 @@ struct queue_limits *dm_get_queue_limits(struct mapped_device *md)
> }
> EXPORT_SYMBOL_GPL(dm_get_queue_limits);
>
> -static void dm_old_init_rq_based_worker_thread(struct mapped_device *md)
> +static int dm_old_init_rq_based_worker_thread(struct mapped_device *md)
> {
> /* Initialize the request-based DM worker thread */
> init_kthread_worker(&md->kworker);
> md->kworker_task = kthread_run(kthread_worker_fn, &md->kworker,
> "kdmwork-%s", dm_device_name(md));
> + if (IS_ERR(md->kworker_task))
> + return -ENOMEM;
> + return 0;
> }
>
> /*
> @@ -2667,6 +2670,8 @@ static void dm_old_init_rq_based_worker_thread(struct mapped_device *md)
> */
> static int dm_old_init_request_queue(struct mapped_device *md)
> {
> + int ret;
> +
> /* Fully initialize the queue */
> if (!blk_init_allocated_queue(md->queue, dm_request_fn, NULL))
> return -EINVAL;
> @@ -2678,7 +2683,9 @@ static int dm_old_init_request_queue(struct mapped_device *md)
> blk_queue_softirq_done(md->queue, dm_softirq_done);
> blk_queue_prep_rq(md->queue, dm_old_prep_fn);
>
> - dm_old_init_rq_based_worker_thread(md);
> + ret = dm_old_init_rq_based_worker_thread(md);
> + if (ret < 0)
> + return ret;
>
> elv_register_queue(md->queue);
>
This needed rebasing against linux-dm.git's 'for-next'. I've now staged
this fix for 4.8 inclusion, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=7193a9defcab6f3d3f1eb64c68bad7534e5a39ad
^ permalink raw reply
* Re: [PATCH RESENT] dm: Check kthread_run's return value
From: Minfei Huang @ 2016-07-06 13:27 UTC (permalink / raw)
To: Mike Snitzer; +Cc: agk, shli, dm-devel, linux-raid, linux-kernel
In-Reply-To: <20160706131601.GA28379@redhat.com>
On 07/06/16 at 09:16P, Mike Snitzer wrote:
> On Mon, Jul 04 2016 at 11:25am -0400,
> Minfei Huang <mnghuan@gmail.com> wrote:
>
> > kthread function is used to process kthread_work. And there is no return
> > value checking during create this thread. Add this checking to fix this
> > issue.
> >
> > Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> This needed rebasing against linux-dm.git's 'for-next'. I've now staged
> this fix for 4.8 inclusion, see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=7193a9defcab6f3d3f1eb64c68bad7534e5a39ad
Seems that we should fix it in stable as well.
Thanks
Minfei
^ permalink raw reply
* Re: [PATCH RESENT] dm: Check kthread_run's return value
From: Mike Snitzer @ 2016-07-06 13:31 UTC (permalink / raw)
To: Minfei Huang; +Cc: agk, shli, dm-devel, linux-raid, linux-kernel
In-Reply-To: <20160706132755.GA1712@MinfeideMacBook-Pro.local>
On Wed, Jul 06 2016 at 9:27am -0400,
Minfei Huang <mnghuan@gmail.com> wrote:
> On 07/06/16 at 09:16P, Mike Snitzer wrote:
> > On Mon, Jul 04 2016 at 11:25am -0400,
> > Minfei Huang <mnghuan@gmail.com> wrote:
> >
> > > kthread function is used to process kthread_work. And there is no return
> > > value checking during create this thread. Add this checking to fix this
> > > issue.
> > >
> > > Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> > This needed rebasing against linux-dm.git's 'for-next'. I've now staged
> > this fix for 4.8 inclusion, see:
> > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=7193a9defcab6f3d3f1eb64c68bad7534e5a39ad
>
> Seems that we should fix it in stable as well.
Given the code movement it isn't easy to do (by simply adding a stable@
cc). I've not seen a single report of an ignored kthread_run() failure
for multipath using .request_fn interface.
^ permalink raw reply
* Re: [PATCH RESENT] dm: Check kthread_run's return value
From: Minfei Huang @ 2016-07-06 13:38 UTC (permalink / raw)
To: Mike Snitzer; +Cc: agk, shli, dm-devel, linux-raid, linux-kernel
In-Reply-To: <20160706133129.GB28379@redhat.com>
On 07/06/16 at 09:31P, Mike Snitzer wrote:
> On Wed, Jul 06 2016 at 9:27am -0400,
> Minfei Huang <mnghuan@gmail.com> wrote:
>
> > On 07/06/16 at 09:16P, Mike Snitzer wrote:
> > > On Mon, Jul 04 2016 at 11:25am -0400,
> > > Minfei Huang <mnghuan@gmail.com> wrote:
> > >
> > > > kthread function is used to process kthread_work. And there is no return
> > > > value checking during create this thread. Add this checking to fix this
> > > > issue.
> > > >
> > > > Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> > > This needed rebasing against linux-dm.git's 'for-next'. I've now staged
> > > this fix for 4.8 inclusion, see:
> > > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=7193a9defcab6f3d3f1eb64c68bad7534e5a39ad
> >
> > Seems that we should fix it in stable as well.
>
> Given the code movement it isn't easy to do (by simply adding a stable@
> cc). I've not seen a single report of an ignored kthread_run() failure
> for multipath using .request_fn interface.
Yep, this bug will be trigged in very restrict condition, also dm is
installed in boot time when there are a lot of memory avaiable.
Thanks
Minfei
^ 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