* [PATCH] block: fix bio_will_gap()
From: Ming Lei @ 2017-04-13 16:06 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Johannes Thumshirn, jth, Ming Lei
Now commit 729204ef49ec("block: relax check on sg gap")
allows us to merge bios if both are physically contiguous,
this change can merge huge of small bios in use case of mkfs,
for example, mkfs.ntfs running time can be decreased to ~1/10.
But if one rq starts with non-aligned buffer(the 1st bvec's
bv_offset isn't zero) and if we allow to merge, it is quite
difficult to respect sg gap limit, especially the segment
can't be at maximum segment size, otherwise the segment
ends in unaligned virt boundary. This patch trys to avoid the
issue by not allowing to merge if the req starts with non-aligned
buffer.
Also add comments to explain why the merged segment can't
end in unaligned virt boundary.
Fixes: 729204ef49ec ("block: relax check on sg gap")
Tested-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
include/linux/blkdev.h | 32 ++++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b75d6fe5a1b9..5eaf323de98b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1672,12 +1672,36 @@ static inline bool bios_segs_mergeable(struct request_queue *q,
return true;
}
-static inline bool bio_will_gap(struct request_queue *q, struct bio *prev,
- struct bio *next)
+static inline bool bio_will_gap(struct request_queue *q,
+ struct request *prev_rq,
+ struct bio *prev,
+ struct bio *next)
{
if (bio_has_data(prev) && queue_virt_boundary(q)) {
struct bio_vec pb, nb;
+ /*
+ * don't merge if the 1st bio starts with non-zero
+ * offset, otherwise it is quite difficult to respect
+ * sg gap limit. We work hard to merge huge of small
+ * single bios in case of mkfs.
+ */
+ if (prev_rq)
+ bio_get_first_bvec(prev_rq->bio, &pb);
+ else
+ bio_get_first_bvec(prev, &pb);
+ if (pb.bv_offset)
+ return true;
+
+ /*
+ * We don't need to worry about the situation that the
+ * merged segment ends in unaligned virt boundary:
+ *
+ * - if 'pb' ends aligned, the merged segment ends aligned
+ * - if 'pb' ends unaligned, the next bio must include
+ * one single bvec of 'nb', otherwise the 'nb' can't
+ * merge with 'pb'
+ */
bio_get_last_bvec(prev, &pb);
bio_get_first_bvec(next, &nb);
@@ -1690,12 +1714,12 @@ static inline bool bio_will_gap(struct request_queue *q, struct bio *prev,
static inline bool req_gap_back_merge(struct request *req, struct bio *bio)
{
- return bio_will_gap(req->q, req->biotail, bio);
+ return bio_will_gap(req->q, req, req->biotail, bio);
}
static inline bool req_gap_front_merge(struct request *req, struct bio *bio)
{
- return bio_will_gap(req->q, bio, req->bio);
+ return bio_will_gap(req->q, NULL, bio, req->bio);
}
static inline void blk_dump_rq(const struct request *req, const char *msg)
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] block: fix bio_will_gap()
From: Bart Van Assche @ 2017-04-13 16:22 UTC (permalink / raw)
To: axboe@fb.com, ming.lei@redhat.com
Cc: hch@infradead.org, linux-block@vger.kernel.org,
jthumshirn@suse.de, jth@kernel.org
In-Reply-To: <20170413160626.14635-1-ming.lei@redhat.com>
On Fri, 2017-04-14 at 00:06 +0800, Ming Lei wrote:
> But if one rq starts with non-aligned buffer(the 1st bvec's
> bv_offset isn't zero) and if we allow to merge, it is quite
> difficult to respect sg gap limit, especially the segment
> can't be at maximum segment size, otherwise the segment
> ends in unaligned virt boundary. This patch trys to avoid the
> issue by not allowing to merge if the req starts with non-aligned
> buffer.
Hello Ming,
Why is it considered difficult to detect whether or not a gap exists if
the offset of the first bio is non-zero? Please note that a thoroughly
tested version of gap detection code that supports non-zero offsets for
the first element is already upstream. See also ib_map_mr_sg() and
ib_sg_to_pages() in drivers/infiniband/core/verbs.c.
Thanks,
Bart.=
^ permalink raw reply
* Re: [PATCH v4 6/6] dm rq: Avoid that request processing stalls sporadically
From: Bart Van Assche @ 2017-04-13 16:59 UTC (permalink / raw)
To: Ming Lei
Cc: linux-scsi@vger.kernel.org, dm-devel@redhat.com,
linux-block@vger.kernel.org, snitzer@redhat.com, axboe@kernel.dk
In-Reply-To: <20170413022030.GD8835@ming.t460p>
On 04/12/17 19:20, Ming Lei wrote:
> On Wed, Apr 12, 2017 at 06:38:07PM +0000, Bart Van Assche wrote:
>> If the blk-mq core would always rerun a hardware queue if a block driver
>> returns BLK_MQ_RQ_QUEUE_BUSY then that would cause 100% of a single CPU core
>
> It won't casue 100% CPU utilization since we restart queue in completion
> path and at that time at least one tag is available, then progress can be
> made.
Hello Ming,
Sorry but you are wrong. If .queue_rq() returns BLK_MQ_RQ_QUEUE_BUSY
then it's likely that calling .queue_rq() again after only a few
microseconds will cause it to return BLK_MQ_RQ_QUEUE_BUSY again. If you
don't believe me, change "if (!blk_mq_sched_needs_restart(hctx) &&
!test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state)) blk_mq_run_hw_queue(hctx,
true);" into "blk_mq_run_hw_queue(hctx, true);", trigger a busy
condition for either a SCSI LLD or a dm-rq driver, run top and you will
see that the CPU usage of a kworker thread jumps up to 100%.
Bart.
^ permalink raw reply
* [PATCH] lightnvm: pblk-gc: fix an error pointer dereference in init
From: Dan Carpenter @ 2017-04-13 19:32 UTC (permalink / raw)
To: Matias Bjorling, Javier González; +Cc: linux-block, kernel-janitors
These labels are reversed so we could end up dereferencing an error
pointer or leaking.
Fixes: 7f347ba6bb3a ("lightnvm: physical block device (pblk) target")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c
index 9b147cfd8a41..f173fd4ea947 100644
--- a/drivers/lightnvm/pblk-gc.c
+++ b/drivers/lightnvm/pblk-gc.c
@@ -527,10 +527,10 @@ int pblk_gc_init(struct pblk *pblk)
return 0;
-fail_free_main_kthread:
- kthread_stop(gc->gc_ts);
fail_free_writer_kthread:
kthread_stop(gc->gc_writer_ts);
+fail_free_main_kthread:
+ kthread_stop(gc->gc_ts);
return ret;
}
^ permalink raw reply related
* [PATCH] lightnvm: fix some error code in pblk-init.c
From: Dan Carpenter @ 2017-04-13 19:35 UTC (permalink / raw)
To: Matias Bjorling, Javier González; +Cc: linux-block, kernel-janitors
There were a bunch of places in pblk_lines_init() where we didn't set an
error code. And in pblk_writer_init() we accidentally return 1 instead
of a correct error code, which would result in a Oops later.
Fixes: 11a5d6fdf919 ("lightnvm: physical block device (pblk) target")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 94653b1f1300..3996e4b8fb0e 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -543,7 +543,7 @@ static int pblk_lines_init(struct pblk *pblk)
long nr_bad_blks, nr_meta_blks, nr_free_blks;
int bb_distance;
int i;
- int ret = 0;
+ int ret;
lm->sec_per_line = geo->sec_per_blk * geo->nr_luns;
lm->blk_per_line = geo->nr_luns;
@@ -638,12 +638,16 @@ static int pblk_lines_init(struct pblk *pblk)
}
l_mg->bb_template = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
- if (!l_mg->bb_template)
+ if (!l_mg->bb_template) {
+ ret = -ENOMEM;
goto fail_free_meta;
+ }
l_mg->bb_aux = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
- if (!l_mg->bb_aux)
+ if (!l_mg->bb_aux) {
+ ret = -ENOMEM;
goto fail_free_bb_template;
+ }
bb_distance = (geo->nr_luns) * geo->sec_per_pl;
for (i = 0; i < lm->sec_per_line; i += bb_distance)
@@ -667,8 +671,10 @@ static int pblk_lines_init(struct pblk *pblk)
pblk->lines = kcalloc(l_mg->nr_lines, sizeof(struct pblk_line),
GFP_KERNEL);
- if (!pblk->lines)
+ if (!pblk->lines) {
+ ret = -ENOMEM;
goto fail_free_bb_aux;
+ }
nr_free_blks = 0;
for (i = 0; i < l_mg->nr_lines; i++) {
@@ -682,8 +688,10 @@ static int pblk_lines_init(struct pblk *pblk)
spin_lock_init(&line->lock);
nr_bad_blks = pblk_bb_line(pblk, line);
- if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line)
+ if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line) {
+ ret = -EINVAL;
goto fail_free_lines;
+ }
line->blk_in_line = lm->blk_per_line - nr_bad_blks;
if (line->blk_in_line < lm->min_blk_line) {
@@ -733,7 +741,7 @@ static int pblk_writer_init(struct pblk *pblk)
pblk->writer_ts = kthread_create(pblk_write_ts, pblk, "pblk-writer-t");
if (IS_ERR(pblk->writer_ts)) {
pr_err("pblk: could not allocate writer kthread\n");
- return 1;
+ return PTR_ERR(pblk->writer_ts);
}
return 0;
^ permalink raw reply related
* Re: [PATCH] lightnvm: pblk-gc: fix an error pointer dereference in init
From: Matias Bjørling @ 2017-04-13 19:35 UTC (permalink / raw)
To: Dan Carpenter, Matias Bjorling, Javier González
Cc: linux-block, kernel-janitors
In-Reply-To: <20170413193241.GA591@mwanda>
On 04/13/2017 09:32 PM, Dan Carpenter wrote:
> These labels are reversed so we could end up dereferencing an error
> pointer or leaking.
>
> Fixes: 7f347ba6bb3a ("lightnvm: physical block device (pblk) target")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c
> index 9b147cfd8a41..f173fd4ea947 100644
> --- a/drivers/lightnvm/pblk-gc.c
> +++ b/drivers/lightnvm/pblk-gc.c
> @@ -527,10 +527,10 @@ int pblk_gc_init(struct pblk *pblk)
>
> return 0;
>
> -fail_free_main_kthread:
> - kthread_stop(gc->gc_ts);
> fail_free_writer_kthread:
> kthread_stop(gc->gc_writer_ts);
> +fail_free_main_kthread:
> + kthread_stop(gc->gc_ts);
>
> return ret;
> }
>
Thanks Dan. I've applied it for 4.12.
^ permalink raw reply
* [PATCH] lightnvm: fix some WARN() messages
From: Dan Carpenter @ 2017-04-13 19:36 UTC (permalink / raw)
To: Matias Bjorling, Javier González; +Cc: linux-block, kernel-janitors
WARN_ON() takes a condition, not an error message. I slightly tweaked
some conditions so hopefully it's more clear.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index eff0982c076f..bce7ed5fc73f 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -49,8 +49,8 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
int i, j = 0;
/* logic error: lba out-of-bounds. Ignore read request */
- if (!(blba + nr_secs < pblk->rl.nr_secs)) {
- WARN_ON("pblk: read lbas out of bounds\n");
+ if (blba + nr_secs >= pblk->rl.nr_secs) {
+ WARN(1, "pblk: read lbas out of bounds\n");
return;
}
@@ -254,8 +254,8 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd,
sector_t lba = pblk_get_lba(bio);
/* logic error: lba out-of-bounds. Ignore read request */
- if (!(lba < pblk->rl.nr_secs)) {
- WARN_ON("pblk: read lba out of bounds\n");
+ if (lba >= pblk->rl.nr_secs) {
+ WARN(1, "pblk: read lba out of bounds\n");
return;
}
@@ -411,8 +411,8 @@ static int read_rq_gc(struct pblk *pblk, struct nvm_rq *rqd,
int valid_secs = 0;
/* logic error: lba out-of-bounds */
- if (!(lba < pblk->rl.nr_secs)) {
- WARN_ON("pblk: read lba out of bounds\n");
+ if (lba >= pblk->rl.nr_secs) {
+ WARN(1, "pblk: read lba out of bounds\n");
goto out;
}
diff --git a/drivers/lightnvm/pblk-write.c b/drivers/lightnvm/pblk-write.c
index 02c415b957d4..56547ca87926 100644
--- a/drivers/lightnvm/pblk-write.c
+++ b/drivers/lightnvm/pblk-write.c
@@ -141,7 +141,7 @@ static void pblk_end_w_fail(struct pblk *pblk, struct nvm_rq *rqd)
/* Logic error */
if (bit > c_ctx->nr_valid) {
- WARN_ON_ONCE("pblk: corrupted write request\n");
+ WARN_ONCE(1, "pblk: corrupted write request\n");
goto out;
}
diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index 0d50f415cfde..f8f85087cd3c 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -167,7 +167,7 @@ static int pblk_recov_l2p_from_emeta(struct pblk *pblk, struct pblk_line *line)
if (le64_to_cpu(lba_list[i]) == ADDR_EMPTY) {
spin_lock(&line->lock);
if (test_and_set_bit(i, line->invalid_bitmap))
- WARN_ON_ONCE("pblk: rec. double invalidate:\n");
+ WARN_ONCE(1, "pblk: rec. double invalidate:\n");
else
line->vsc--;
spin_unlock(&line->lock);
^ permalink raw reply related
* [PATCH block-tree] net: off by one in inet6_pton()
From: Dan Carpenter @ 2017-04-13 19:42 UTC (permalink / raw)
To: David S. Miller, Jens Axboe, Sagi Grimberg
Cc: Wei Tang, Alexey Dobriyan, netdev, linux-block, kernel-janitors
If "scope_len" is sizeof(scope_id) then we would put the NUL terminator
one space beyond the end of the buffer.
Fixes: b1a951fe469e ("net/utils: generic inet_pton_with_scope helper")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This one goes through Jens' tree not through net-dev.
diff --git a/net/core/utils.c b/net/core/utils.c
index da1089ea5389..93066bd0305a 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -339,7 +339,7 @@ static int inet6_pton(struct net *net, const char *src, u16 port_num,
src + srclen != scope_delim && *scope_delim == '%') {
struct net_device *dev;
char scope_id[16];
- size_t scope_len = min_t(size_t, sizeof(scope_id),
+ size_t scope_len = min_t(size_t, sizeof(scope_id) - 1,
src + srclen - scope_delim - 1);
memcpy(scope_id, scope_delim + 1, scope_len);
^ permalink raw reply related
* Re: [PATCH] lightnvm: fix some error code in pblk-init.c
From: Matias Bjørling @ 2017-04-13 19:43 UTC (permalink / raw)
To: Dan Carpenter, Javier González; +Cc: linux-block, kernel-janitors
In-Reply-To: <20170413193512.GB591@mwanda>
On 04/13/2017 09:35 PM, Dan Carpenter wrote:
> There were a bunch of places in pblk_lines_init() where we didn't set an
> error code. And in pblk_writer_init() we accidentally return 1 instead
> of a correct error code, which would result in a Oops later.
>
> Fixes: 11a5d6fdf919 ("lightnvm: physical block device (pblk) target")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
> index 94653b1f1300..3996e4b8fb0e 100644
> --- a/drivers/lightnvm/pblk-init.c
> +++ b/drivers/lightnvm/pblk-init.c
> @@ -543,7 +543,7 @@ static int pblk_lines_init(struct pblk *pblk)
> long nr_bad_blks, nr_meta_blks, nr_free_blks;
> int bb_distance;
> int i;
> - int ret = 0;
> + int ret;
>
> lm->sec_per_line = geo->sec_per_blk * geo->nr_luns;
> lm->blk_per_line = geo->nr_luns;
> @@ -638,12 +638,16 @@ static int pblk_lines_init(struct pblk *pblk)
> }
>
> l_mg->bb_template = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
> - if (!l_mg->bb_template)
> + if (!l_mg->bb_template) {
> + ret = -ENOMEM;
> goto fail_free_meta;
> + }
>
> l_mg->bb_aux = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
> - if (!l_mg->bb_aux)
> + if (!l_mg->bb_aux) {
> + ret = -ENOMEM;
> goto fail_free_bb_template;
> + }
>
> bb_distance = (geo->nr_luns) * geo->sec_per_pl;
> for (i = 0; i < lm->sec_per_line; i += bb_distance)
> @@ -667,8 +671,10 @@ static int pblk_lines_init(struct pblk *pblk)
>
> pblk->lines = kcalloc(l_mg->nr_lines, sizeof(struct pblk_line),
> GFP_KERNEL);
> - if (!pblk->lines)
> + if (!pblk->lines) {
> + ret = -ENOMEM;
> goto fail_free_bb_aux;
> + }
>
> nr_free_blks = 0;
> for (i = 0; i < l_mg->nr_lines; i++) {
> @@ -682,8 +688,10 @@ static int pblk_lines_init(struct pblk *pblk)
> spin_lock_init(&line->lock);
>
> nr_bad_blks = pblk_bb_line(pblk, line);
> - if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line)
> + if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line) {
> + ret = -EINVAL;
> goto fail_free_lines;
> + }
>
> line->blk_in_line = lm->blk_per_line - nr_bad_blks;
> if (line->blk_in_line < lm->min_blk_line) {
> @@ -733,7 +741,7 @@ static int pblk_writer_init(struct pblk *pblk)
> pblk->writer_ts = kthread_create(pblk_write_ts, pblk, "pblk-writer-t");
> if (IS_ERR(pblk->writer_ts)) {
> pr_err("pblk: could not allocate writer kthread\n");
> - return 1;
> + return PTR_ERR(pblk->writer_ts);
> }
>
> return 0;
>
Thanks Dan. Applied for 4.12.
^ permalink raw reply
* Re: [PATCH] lightnvm: fix some WARN() messages
From: Matias Bjørling @ 2017-04-13 19:44 UTC (permalink / raw)
To: Dan Carpenter, Javier González; +Cc: linux-block, kernel-janitors
In-Reply-To: <20170413193636.GC591@mwanda>
On 04/13/2017 09:36 PM, Dan Carpenter wrote:
> WARN_ON() takes a condition, not an error message. I slightly tweaked
> some conditions so hopefully it's more clear.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
> index eff0982c076f..bce7ed5fc73f 100644
> --- a/drivers/lightnvm/pblk-read.c
> +++ b/drivers/lightnvm/pblk-read.c
> @@ -49,8 +49,8 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
> int i, j = 0;
>
> /* logic error: lba out-of-bounds. Ignore read request */
> - if (!(blba + nr_secs < pblk->rl.nr_secs)) {
> - WARN_ON("pblk: read lbas out of bounds\n");
> + if (blba + nr_secs >= pblk->rl.nr_secs) {
> + WARN(1, "pblk: read lbas out of bounds\n");
> return;
> }
>
> @@ -254,8 +254,8 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd,
> sector_t lba = pblk_get_lba(bio);
>
> /* logic error: lba out-of-bounds. Ignore read request */
> - if (!(lba < pblk->rl.nr_secs)) {
> - WARN_ON("pblk: read lba out of bounds\n");
> + if (lba >= pblk->rl.nr_secs) {
> + WARN(1, "pblk: read lba out of bounds\n");
> return;
> }
>
> @@ -411,8 +411,8 @@ static int read_rq_gc(struct pblk *pblk, struct nvm_rq *rqd,
> int valid_secs = 0;
>
> /* logic error: lba out-of-bounds */
> - if (!(lba < pblk->rl.nr_secs)) {
> - WARN_ON("pblk: read lba out of bounds\n");
> + if (lba >= pblk->rl.nr_secs) {
> + WARN(1, "pblk: read lba out of bounds\n");
> goto out;
> }
>
> diff --git a/drivers/lightnvm/pblk-write.c b/drivers/lightnvm/pblk-write.c
> index 02c415b957d4..56547ca87926 100644
> --- a/drivers/lightnvm/pblk-write.c
> +++ b/drivers/lightnvm/pblk-write.c
> @@ -141,7 +141,7 @@ static void pblk_end_w_fail(struct pblk *pblk, struct nvm_rq *rqd)
>
> /* Logic error */
> if (bit > c_ctx->nr_valid) {
> - WARN_ON_ONCE("pblk: corrupted write request\n");
> + WARN_ONCE(1, "pblk: corrupted write request\n");
> goto out;
> }
>
> diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
> index 0d50f415cfde..f8f85087cd3c 100644
> --- a/drivers/lightnvm/pblk-recovery.c
> +++ b/drivers/lightnvm/pblk-recovery.c
> @@ -167,7 +167,7 @@ static int pblk_recov_l2p_from_emeta(struct pblk *pblk, struct pblk_line *line)
> if (le64_to_cpu(lba_list[i]) == ADDR_EMPTY) {
> spin_lock(&line->lock);
> if (test_and_set_bit(i, line->invalid_bitmap))
> - WARN_ON_ONCE("pblk: rec. double invalidate:\n");
> + WARN_ONCE(1, "pblk: rec. double invalidate:\n");
> else
> line->vsc--;
> spin_unlock(&line->lock);
>
Hehe, yep. That was slightly misused. Thanks for pointing it out. I've
applied it for 4.12.
^ permalink raw reply
* Re: [PATCH 01/25] remove the mg_disk driver
From: Bart Van Assche @ 2017-04-13 19:58 UTC (permalink / raw)
To: hch@lst.de, axboe@kernel.dk
Cc: linux-block@vger.kernel.org, konrad.wilk@oracle.com,
roger.pau@citrix.com, linux-scsi@vger.kernel.org,
linux-nvme@lists.infradead.org, jbacik@fb.com,
james.smart@broadcom.com, dm-devel@redhat.com
In-Reply-To: <20170406153944.10058-2-hch@lst.de>
On Thu, 2017-04-06 at 17:39 +0200, Christoph Hellwig wrote:
> This drivers was added in 2008, but as far as a I can tell we never had a
> single platform that actually registered resources for the platform drive=
r.
>=20
> It's also been unmaintained for a long time and apparently has a ATA mode
> that can be driven using the IDE/libata subsystem.
Hello Christoph,
Should the person who submitted this driver be CC-ed for this patch (unsik
Kim <donari75@gmail.com>)?
Bart.=
^ permalink raw reply
* Re: [PATCH 02/25] block: remove the blk_execute_rq return value
From: Bart Van Assche @ 2017-04-13 20:03 UTC (permalink / raw)
To: hch@lst.de, axboe@kernel.dk
Cc: linux-block@vger.kernel.org, konrad.wilk@oracle.com,
roger.pau@citrix.com, linux-scsi@vger.kernel.org,
linux-nvme@lists.infradead.org, jbacik@fb.com,
james.smart@broadcom.com, dm-devel@redhat.com
In-Reply-To: <20170406153944.10058-3-hch@lst.de>
On Thu, 2017-04-06 at 17:39 +0200, Christoph Hellwig wrote:
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index 92b4b41d19d2..4b72fdf67548 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -242,8 +242,8 @@ static int nfsd4_scsi_identify_device(struct block_de=
vice *bdev,
> req->cmd[4] =3D bufflen & 0xff;
> req->cmd_len =3D COMMAND_SIZE(INQUIRY);
> =20
> - error =3D blk_execute_rq(rq->q, NULL, rq, 1);
> - if (error) {
> + blk_execute_rq(rq->q, NULL, rq, 1);
> + if (rq->errors) {
> pr_err("pNFS: INQUIRY 0x83 failed with: %x\n",
> rq->errors);
> goto out_put_request;
Hello Christoph,
That blk_execute_rq() call can only be reached if a few lines above 0 was
assigned to the "error" variable. Since nfsd4_scsi_identify_device() return=
s
the value of the "error" variable I think -EIO should be assigned to that
variable before the "goto out_put_request" statement is reached.
Bart.=
^ permalink raw reply
* Re: [PATCH 06/25] virtio: fix spelling of virtblk_scsi_request_done
From: Bart Van Assche @ 2017-04-13 20:05 UTC (permalink / raw)
To: hch@lst.de, axboe@kernel.dk
Cc: linux-block@vger.kernel.org, konrad.wilk@oracle.com,
roger.pau@citrix.com, linux-scsi@vger.kernel.org,
linux-nvme@lists.infradead.org, jbacik@fb.com,
james.smart@broadcom.com, dm-devel@redhat.com
In-Reply-To: <20170406153944.10058-7-hch@lst.de>
On Thu, 2017-04-06 at 17:39 +0200, Christoph Hellwig wrote:
> [ ... ]
Reviewed-by: Bart Van Assche <Bart.VanAssche@sandisk.com>=
^ permalink raw reply
* Re: [PATCH 08/25] scsi: fix fast-fail for non-passthrough requests
From: Bart Van Assche @ 2017-04-13 20:41 UTC (permalink / raw)
To: hch@lst.de, axboe@kernel.dk
Cc: linux-block@vger.kernel.org, konrad.wilk@oracle.com,
roger.pau@citrix.com, linux-scsi@vger.kernel.org,
linux-nvme@lists.infradead.org, jbacik@fb.com,
james.smart@broadcom.com, dm-devel@redhat.com
In-Reply-To: <20170406153944.10058-9-hch@lst.de>
On Thu, 2017-04-06 at 17:39 +0200, Christoph Hellwig wrote:
> Currently error is always 0 for non-passthrough requests when reaching th=
e
> scsi_noretry_cmd check in scsi_io_completion, which effectively disables
> all fastfail logic. Fix this by having a single call to
> __scsi_error_from_host_byte at the beginning of the function and always
> having a valid error value.
>=20
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/scsi/scsi_lib.c | 28 +++++++---------------------
> 1 file changed, 7 insertions(+), 21 deletions(-)
>=20
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 11972d1075f1..89b4d9e69866 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -779,21 +779,17 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsi=
gned int good_bytes)
> sense_valid =3D scsi_command_normalize_sense(cmd, &sshdr);
> if (sense_valid)
> sense_deferred =3D scsi_sense_is_deferred(&sshdr);
> +
> + if (!sense_deferred)
> + error =3D __scsi_error_from_host_byte(cmd, result);
> }
Hello Christoph,
Sorry but this doesn't look correct to me. Further down a "error =3D
__scsi_error_from_host_byte(cmd, result)" statement is removed that does no=
t
depend on "if (!sense_deferred)" so I think that assignment should happen
independent of the value of "sense_deferred". Additionally, how can it make
sense to call __scsi_error_from_host_byte() only if sense_deferred =3D=3D f=
alse?
As you know the SCSI command result is generated by the LLD so I don't thin=
k
that it depends on whether or not the sense data has been deferred.
Bart.=
^ permalink raw reply
* Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue
From: Omar Sandoval @ 2017-04-13 23:01 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Omar Sandoval, Hannes Reinecke
In-Reply-To: <20170411205842.28137-2-bart.vanassche@sandisk.com>
On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote:
> The blk-mq debugfs attributes are removed after blk_cleanup_queue()
> has finished. Since running a queue after a queue has entered the
> "dead" state is not allowed, disallow this. This patch avoids that
> an attempt to run a dead queue triggers a kernel crash.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
> block/blk-mq-debugfs.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index df9b688b877c..a1ce823578c7 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -111,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
> struct request_queue *q = file_inode(file)->i_private;
> char op[16] = { }, *s;
>
> + /*
> + * The debugfs attributes are removed after blk_cleanup_queue() has
> + * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set
> + * to avoid triggering a use-after-free.
> + */
> + if (blk_queue_dead(q))
> + return -ENOENT;
> +
> len = min(len, sizeof(op) - 1);
> if (copy_from_user(op, ubuf, len))
> return -EFAULT;
> --
> 2.12.0
>
Hi, Bart,
Looking at this, I think we have similar issues with most of the other
debugfs files. Should we move the debugfs cleanup earlier?
^ permalink raw reply
* Re: [PATCH 2/6] blk-mq: Move the "state" debugfs attribute one level down
From: Omar Sandoval @ 2017-04-13 23:01 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Omar Sandoval, Hannes Reinecke
In-Reply-To: <20170411205842.28137-3-bart.vanassche@sandisk.com>
On Tue, Apr 11, 2017 at 01:58:38PM -0700, Bart Van Assche wrote:
> Move the "state" attribute from the top level to the "mq" directory
> as requested by Omar.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
Thanks!
Reviewed-by: Omar Sandoval <osandov@fb.com>
> ---
> block/blk-mq-debugfs.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index a1ce823578c7..564470d4af52 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -149,11 +149,6 @@ static const struct file_operations blk_queue_flags_fops = {
> .write = blk_queue_flags_store,
> };
>
> -static const struct blk_mq_debugfs_attr blk_queue_attrs[] = {
> - {"state", 0600, &blk_queue_flags_fops},
> - {},
> -};
> -
> static void print_stat(struct seq_file *m, struct blk_rq_stat *stat)
> {
> if (stat->nr_samples) {
> @@ -762,6 +757,7 @@ static const struct file_operations ctx_completed_fops = {
>
> static const struct blk_mq_debugfs_attr blk_mq_debugfs_queue_attrs[] = {
> {"poll_stat", 0400, &queue_poll_stat_fops},
> + {"state", 0600, &blk_queue_flags_fops},
> {},
> };
>
> @@ -877,9 +873,6 @@ int blk_mq_debugfs_register_hctxs(struct request_queue *q)
> if (!q->debugfs_dir)
> return -ENOENT;
>
> - if (!debugfs_create_files(q->debugfs_dir, q, blk_queue_attrs))
> - goto err;
> -
> q->mq_debugfs_dir = debugfs_create_dir("mq", q->debugfs_dir);
> if (!q->mq_debugfs_dir)
> goto err;
> --
> 2.12.0
>
^ permalink raw reply
* Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue
From: Omar Sandoval @ 2017-04-13 23:03 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Omar Sandoval, Hannes Reinecke
In-Reply-To: <20170413230102.GA1550@vader.DHCP.thefacebook.com>
On Thu, Apr 13, 2017 at 04:01:02PM -0700, Omar Sandoval wrote:
> On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote:
> > The blk-mq debugfs attributes are removed after blk_cleanup_queue()
> > has finished. Since running a queue after a queue has entered the
> > "dead" state is not allowed, disallow this. This patch avoids that
> > an attempt to run a dead queue triggers a kernel crash.
> >
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Omar Sandoval <osandov@fb.com>
> > Cc: Hannes Reinecke <hare@suse.com>
> > ---
> > block/blk-mq-debugfs.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> > index df9b688b877c..a1ce823578c7 100644
> > --- a/block/blk-mq-debugfs.c
> > +++ b/block/blk-mq-debugfs.c
> > @@ -111,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
> > struct request_queue *q = file_inode(file)->i_private;
> > char op[16] = { }, *s;
> >
> > + /*
> > + * The debugfs attributes are removed after blk_cleanup_queue() has
> > + * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set
> > + * to avoid triggering a use-after-free.
> > + */
> > + if (blk_queue_dead(q))
> > + return -ENOENT;
> > +
> > len = min(len, sizeof(op) - 1);
> > if (copy_from_user(op, ubuf, len))
> > return -EFAULT;
> > --
> > 2.12.0
> >
>
> Hi, Bart,
>
> Looking at this, I think we have similar issues with most of the other
> debugfs files. Should we move the debugfs cleanup earlier?
In particular, I think we can call blk_mq_debugfs_unregister_hctxs()
(which is somewhat poorly named, as it removes the whole mq directory)
before we call blk_mq_free_queue(). I was under the impression that
that's what it already did, but I think I was wrong.
^ permalink raw reply
* Re: [PATCH 1/6] blk-mq: Do not invoke queue operations on a dead queue
From: Bart Van Assche @ 2017-04-13 23:05 UTC (permalink / raw)
To: osandov@osandov.com
Cc: hare@suse.com, linux-block@vger.kernel.org, osandov@fb.com,
axboe@kernel.dk
In-Reply-To: <20170413230102.GA1550@vader.DHCP.thefacebook.com>
On Thu, 2017-04-13 at 16:01 -0700, Omar Sandoval wrote:
> On Tue, Apr 11, 2017 at 01:58:37PM -0700, Bart Van Assche wrote:
> > The blk-mq debugfs attributes are removed after blk_cleanup_queue()
> > has finished. Since running a queue after a queue has entered the
> > "dead" state is not allowed, disallow this. This patch avoids that
> > an attempt to run a dead queue triggers a kernel crash.
> >=20
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Omar Sandoval <osandov@fb.com>
> > Cc: Hannes Reinecke <hare@suse.com>
> > ---
> > block/blk-mq-debugfs.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >=20
> > diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> > index df9b688b877c..a1ce823578c7 100644
> > --- a/block/blk-mq-debugfs.c
> > +++ b/block/blk-mq-debugfs.c
> > @@ -111,6 +111,14 @@ static ssize_t blk_queue_flags_store(struct file *=
file, const char __user *ubuf,
> > struct request_queue *q =3D file_inode(file)->i_private;
> > char op[16] =3D { }, *s;
> > =20
> > + /*
> > + * The debugfs attributes are removed after blk_cleanup_queue() has
> > + * called blk_mq_free_queue(). Return if QUEUE_FLAG_DEAD has been set
> > + * to avoid triggering a use-after-free.
> > + */
> > + if (blk_queue_dead(q))
> > + return -ENOENT;
> > +
> > len =3D min(len, sizeof(op) - 1);
> > if (copy_from_user(op, ubuf, len))
> > return -EFAULT;
>=20
> Looking at this, I think we have similar issues with most of the other
> debugfs files. Should we move the debugfs cleanup earlier?
Hello Omar,
That's a good question. However, while I was debugging it was very convenie=
nt
to be able to access the queue state after it had reached the "dead" state.
Performing the cleanup earlier would be an alternative solution but would
make debugging a bit harder ...
Bart.=
^ permalink raw reply
* Re: [PATCH 3/6] blk-mq: Make blk_flags_show() callers append a newline character
From: Omar Sandoval @ 2017-04-13 23:08 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Omar Sandoval, Hannes Reinecke
In-Reply-To: <20170411205842.28137-4-bart.vanassche@sandisk.com>
On Tue, Apr 11, 2017 at 01:58:39PM -0700, Bart Van Assche wrote:
> This patch does not change any functionality but makes it possible
> to produce a single line of output with multiple flag-to-name
> translations.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
Reviewed-by: Omar Sandoval <osandov@fb.com>
^ permalink raw reply
* RE: [PATCH] block-mq: set both block queue and hardware queue restart bit for restart
From: Long Li @ 2017-04-13 23:12 UTC (permalink / raw)
To: Bart Van Assche, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, KY Srinivasan, axboe@kernel.dk
Cc: Stephen Hemminger
In-Reply-To: <1491868058.4199.28.camel@sandisk.com>
> -----Original Message-----
> From: Bart Van Assche [mailto:Bart.VanAssche@sandisk.com]
> Sent: Monday, April 10, 2017 4:48 PM
> To: linux-kernel@vger.kernel.org; linux-block@vger.kernel.org; KY Sriniva=
san
> <kys@microsoft.com>; Long Li <longli@microsoft.com>; axboe@kernel.dk
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Subject: Re: [PATCH] block-mq: set both block queue and hardware queue
> restart bit for restart
>=20
> On Thu, 2017-04-06 at 04:21 +0000, KY Srinivasan wrote:
> > > -----Original Message-----
> > > From: Bart Van Assche [mailto:Bart.VanAssche@sandisk.com]
> > > Sent: Wednesday, April 5, 2017 8:46 PM
> > > To: linux-kernel@vger.kernel.org; linux-block@vger.kernel.org; Long
> > > Li <longli@microsoft.com>; axboe@kernel.dk
> > > Cc: Stephen Hemminger <sthemmin@microsoft.com>; KY Srinivasan
> > > <kys@microsoft.com>
> > > Subject: Re: [PATCH] block-mq: set both block queue and hardware
> > > queue restart bit for restart
> > >
> > > On Thu, 2017-04-06 at 03:38 +0000, Long Li wrote:
> > > > > -----Original Message-----
> > > > > From: Bart Van Assche [mailto:Bart.VanAssche@sandisk.com]
> > > > >
> > > > > Please drop this patch. I'm working on a better solution.
> > > >
> > > > Thank you. Looking forward to your patch.
> > >
> > > Hello Long,
> > >
> > > It would help if you could share the name of the block or SCSI
> > > driver with which you ran into that lockup and also if you could
> > > share the name of the I/O scheduler used in your test.
> >
> > The tests that indicated the issue were run Hyper-V. The driver is
> > storvsc_drv.c The I/O scheduler was I think noop.
>=20
> Hello Long and K.Y.,
>=20
> Thank you for the feedback. Can you repeat your test with kernel v4.11-rc=
6?
> The patches that went into the block layer for v4.11-rc6 should be suffic=
ient
> to fix
> this:
>=20
> $ PAGER=3D git log --format=3Dshort v4.11-rc5..v4.11-rc6 block include/li=
nux/blk*
> commit 6d8c6c0f97ad8a3517c42b179c1dc8e77397d0e2
> Author: Bart Van Assche <bart.vanassche@sandisk.com>
>=20
> =A0=A0=A0=A0blk-mq: Restart a single queue if tag sets are shared
>=20
> commit 7587a5ae7eef0439f7be31f1b5959af062bbc5ec
> Author: Bart Van Assche <bart.vanassche@sandisk.com>
>=20
> =A0=A0=A0=A0blk-mq: Introduce blk_mq_delay_run_hw_queue()
>=20
> commit ebe8bddb6e30d7a02775b9972099271fc5910f37
> Author: Omar Sandoval <osandov@fb.com>
>=20
> =A0=A0=A0=A0blk-mq: remap queues when adding/removing hardware queues
>=20
> commit 54d5329d425650fafaf90660a139c771d2d49cae
> Author: Omar Sandoval <osandov@fb.com>
>=20
> =A0=A0=A0=A0blk-mq-sched: fix crash in switch error path
>=20
> commit 93252632e828da3e90241a1c0e766556abf71598
> Author: Omar Sandoval <osandov@fb.com>
>=20
> =A0=A0=A0=A0blk-mq-sched: set up scheduler tags when bringing up new queu=
es
>=20
> commit 6917ff0b5bd4139e08a3f3146529dcb3b95ba7a6
> Author: Omar Sandoval <osandov@fb.com>
>=20
> =A0=A0=A0=A0blk-mq-sched: refactor scheduler initialization
>=20
> commit 81380ca10778b99dce98940cfc993214712df335
> Author: Omar Sandoval <osandov@fb.com>
>=20
> =A0=A0=A0=A0blk-mq: use the right hctx when getting a driver tag fails
>=20
> commit ac77a0c463c1d7d659861f7b6d1261970dd3282a
> Author: Minchan Kim <minchan@kernel.org>
>=20
> =A0=A0=A0=A0block: do not put mq context in blk_mq_alloc_request_hctx
>=20
> Bart.
Thank you. We are doing tests. I will update on the results.
Long
^ permalink raw reply
* Re: [PATCH 4/6] blk-mq: Show operation, cmd_flags and rq_flags names
From: Omar Sandoval @ 2017-04-13 23:17 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Omar Sandoval, Hannes Reinecke
In-Reply-To: <20170411205842.28137-5-bart.vanassche@sandisk.com>
On Tue, Apr 11, 2017 at 01:58:40PM -0700, Bart Van Assche wrote:
> Show the operation name, .cmd_flags and .rq_flags as names instead
> of numbers.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
I like this :) one minor nit below.
Reviewed-by: Omar Sandoval <osandov@fb.com>
> ---
> block/blk-mq-debugfs.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 69 insertions(+), 3 deletions(-)
>
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index aae4b7c7b7b0..161f30fc236f 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
[snip]
> static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
> {
> struct request *rq = list_entry_rq(v);
> + const unsigned int op = rq->cmd_flags & REQ_OP_MASK;
>
> - seq_printf(m, "%p {.cmd_flags=0x%x, .rq_flags=0x%x, .tag=%d, .internal_tag=%d}\n",
> - rq, rq->cmd_flags, (__force unsigned int)rq->rq_flags,
> - rq->tag, rq->internal_tag);
> + seq_printf(m, "%p {.op=", rq);
> + if (op < ARRAY_SIZE(op_name) && op_name[op])
> + seq_printf(m, "%s", op_name[op]);
> + else
> + seq_printf(m, "%d", op);
> + seq_puts(m, ", .cmd_flags=");
> + blk_flags_show(m, rq->cmd_flags ^ op, cmd_flag_name,
^^^^^^^^^^^^^^^^^^
I think rq->cmd_flags & ~REQ_OP_MASK is slightly clearer here, but I
don't feel that strongly about it, it's up to you.
> + ARRAY_SIZE(cmd_flag_name));
> + seq_puts(m, ", .rq_flags=");
> + blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
> + ARRAY_SIZE(rqf_name));
> + seq_printf(m, ", .tag=%d, .internal_tag=%d}\n", rq->tag,
> + rq->internal_tag);
> return 0;
> }
>
> --
> 2.12.0
>
^ permalink raw reply
* Re: [PATCH 5/6] blk-mq: Add blk_mq_ops.show_rq()
From: Omar Sandoval @ 2017-04-13 23:21 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Omar Sandoval, Hannes Reinecke
In-Reply-To: <20170411205842.28137-6-bart.vanassche@sandisk.com>
On Tue, Apr 11, 2017 at 01:58:41PM -0700, Bart Van Assche wrote:
> This new callback function will be used in the next patch to show
> more information about SCSI requests.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
> block/blk-mq-debugfs.c | 10 ++++++++--
> include/linux/blk-mq.h | 6 ++++++
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 161f30fc236f..6b28d92d4c0e 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -316,7 +316,9 @@ static const char *const rqf_name[] = {
> static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
> {
> struct request *rq = list_entry_rq(v);
> + const struct blk_mq_ops *const mq_ops = rq->q->mq_ops;
> const unsigned int op = rq->cmd_flags & REQ_OP_MASK;
> + char drv_info[200];
>
> seq_printf(m, "%p {.op=", rq);
> if (op < ARRAY_SIZE(op_name) && op_name[op])
> @@ -329,8 +331,12 @@ static int blk_mq_debugfs_rq_show(struct seq_file *m, void *v)
> seq_puts(m, ", .rq_flags=");
> blk_flags_show(m, (__force unsigned int)rq->rq_flags, rqf_name,
> ARRAY_SIZE(rqf_name));
> - seq_printf(m, ", .tag=%d, .internal_tag=%d}\n", rq->tag,
> - rq->internal_tag);
> + if (mq_ops->show_rq)
> + mq_ops->show_rq(rq, drv_info, sizeof(drv_info));
How about passing the seq_file to the callback instead of this
arbitrarily-sized on-stack buffer?
^ permalink raw reply
* Re: [PATCH v4 6/6] dm rq: Avoid that request processing stalls sporadically
From: Ming Lei @ 2017-04-14 1:13 UTC (permalink / raw)
To: Bart Van Assche
Cc: linux-scsi@vger.kernel.org, dm-devel@redhat.com,
linux-block@vger.kernel.org, snitzer@redhat.com, axboe@kernel.dk
In-Reply-To: <25ebc19f-f6e0-2f71-f590-8416359d2fd1@sandisk.com>
On Thu, Apr 13, 2017 at 09:59:57AM -0700, Bart Van Assche wrote:
> On 04/12/17 19:20, Ming Lei wrote:
> > On Wed, Apr 12, 2017 at 06:38:07PM +0000, Bart Van Assche wrote:
> >> If the blk-mq core would always rerun a hardware queue if a block driver
> >> returns BLK_MQ_RQ_QUEUE_BUSY then that would cause 100% of a single CPU core
> >
> > It won't casue 100% CPU utilization since we restart queue in completion
> > path and at that time at least one tag is available, then progress can be
> > made.
>
> Hello Ming,
>
> Sorry but you are wrong. If .queue_rq() returns BLK_MQ_RQ_QUEUE_BUSY
> then it's likely that calling .queue_rq() again after only a few
> microseconds will cause it to return BLK_MQ_RQ_QUEUE_BUSY again. If you
> don't believe me, change "if (!blk_mq_sched_needs_restart(hctx) &&
> !test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state)) blk_mq_run_hw_queue(hctx,
> true);" into "blk_mq_run_hw_queue(hctx, true);", trigger a busy
Yes, that can be true, but I mean it is still OK to run the queue again
with
if (!blk_mq_sched_needs_restart(hctx) &&
!test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state))
blk_mq_run_hw_queue(hctx, true);
and restarting queue in __blk_mq_finish_request() when
BLK_MQ_RQ_QUEUE_BUSY is returned from .queue_rq(). And both are in current
blk-mq implementation.
Then why do we need blk_mq_delay_run_hw_queue(hctx, 100/*ms*/) in dm?
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH] block: fix bio_will_gap()
From: Ming Lei @ 2017-04-14 2:04 UTC (permalink / raw)
To: Bart Van Assche
Cc: axboe@fb.com, hch@infradead.org, linux-block@vger.kernel.org,
jthumshirn@suse.de, jth@kernel.org
In-Reply-To: <1492100540.5176.6.camel@sandisk.com>
On Thu, Apr 13, 2017 at 04:22:22PM +0000, Bart Van Assche wrote:
> On Fri, 2017-04-14 at 00:06 +0800, Ming Lei wrote:
> > But if one rq starts with non-aligned buffer(the 1st bvec's
> > bv_offset isn't zero) and if we allow to merge, it is quite
> > difficult to respect sg gap limit, especially the segment
> > can't be at maximum segment size, otherwise the segment
> > ends in unaligned virt boundary. This patch trys to avoid the
> > issue by not allowing to merge if the req starts with non-aligned
> > buffer.
>
> Hello Ming,
>
> Why is it considered difficult to detect whether or not a gap exists if
> the offset of the first bio is non-zero? Please note that a thoroughly
> tested version of gap detection code that supports non-zero offsets for
> the first element is already upstream. See also ib_map_mr_sg() and
> ib_sg_to_pages() in drivers/infiniband/core/verbs.c.
I don't know infiniband much, but from the code, it is just about
sg, and looks not same with block's sg gap limit.
The sg gap limit in block layer actually means:
- except for last segment, every segment has to end in aligned virt boundary
- from the 2nd segment, offset of the segment has to be zero
But we don't store main segment info(start, size) in bio/req,
all the segments are basically computed in the flight, so it isn't
easy to check/handle sg gap. Also IMO segment handling of block layer is
quite fragile, and hope multi-page bvec can improve the case much.
We relax check for sg gap for allowing merging tons of small bios, if
these bios are physically contiguous. But if offset of the 1st bio
in the merged req isn't zero, current segment compution in
__blk_segment_map_sg() doesn't consider sg gap, so one segment may
ends in unaligned virt boundary.
Given it is late in v4.11 cycle, this patch doesn't want to touch
__blk_segment_map_sg() for avoiding regression risk, instead just
changing bio_will_gap() for avoding the case with minimized risk,
because the change is quite simple.
Thanks,
Ming
^ permalink raw reply
* Re: remove REQ_OP_WRITE_SAME
From: Martin K. Petersen @ 2017-04-14 2:23 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, martin.petersen, philipp.reisner, lars.ellenberg,
target-devel, linux-block, linux-scsi, drbd-dev, dm-devel
In-Reply-To: <20170412084809.8245-1-hch@lst.de>
Christoph,
> Now that we are using REQ_OP_WRITE_ZEROES for all zeroing needs in the
> kernel there is very little use left for REQ_OP_WRITE_SAME. We only
> have two callers left, and both just export optional protocol features
> to remote systems: DRBD and the target code.
While I'm not particularly married to WRITE SAME, I do think it's a
shame that the RAID5/6 code never started using it. It does make a
difference when initializing RAID sets.
The other thing that keeps me a bit on the fence is that a bunch of the
plumbing to handle a bio with a payload different from bi_size is needed
for the copy offload token. I'm hoping to have those patches ready for
4.13. Right now there are a bunch of places where handling of
REQ_OP_COPY_IN and REQ_OP_COPY_OUT share conditionals with WRITE SAME.
So I suggest postponing the decision about whether to rip out WRITE SAME
until I finish the token stuff.
--
Martin K. Petersen Oracle Linux Engineering
^ 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