* Re: [PATCH] block: do not put mq context in blk_mq_alloc_request_hctx
From: Sagi Grimberg @ 2017-03-30 14:02 UTC (permalink / raw)
To: Minchan Kim, Jens Axboe
Cc: kernel-team, linux-block, linux-kernel, Omar Sandoval
In-Reply-To: <1490851245-32245-1-git-send-email-minchan@kernel.org>
Looks good,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [PATCH] block: do not put mq context in blk_mq_alloc_request_hctx
From: Jens Axboe @ 2017-03-30 14:12 UTC (permalink / raw)
To: Minchan Kim
Cc: kernel-team, linux-block, linux-kernel, Sagi Grimberg,
Omar Sandoval
In-Reply-To: <1490851245-32245-1-git-send-email-minchan@kernel.org>
On 03/29/2017 11:20 PM, Minchan Kim wrote:
> In blk_mq_alloc_request_hctx, blk_mq_sched_get_request doesn't
> get sw context so we don't need to put the context with
> blk_mq_put_ctx. Unless, we will see preempt counter underflow.
Good catch, that's definitely a bug. I have applied your patch
for 4.11.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
From: Minchan Kim @ 2017-03-30 15:08 UTC (permalink / raw)
To: Johannes Thumshirn, Jens Axboe
Cc: Hannes Reinecke, Nitin Gupta, Christoph Hellwig,
Sergey Senozhatsky, yizhan, Linux Block Layer Mailinglist,
Linux Kernel Mailinglist, Andrew Morton
In-Reply-To: <20170309052829.GA854@bbox>
Hi Jens,
It seems you miss this.
Could you handle this?
Thanks.
On Thu, Mar 9, 2017 at 2:28 PM, Minchan Kim <minchan@kernel.org> wrote:
< snip>
> Jens, Could you replace the one merged with this? And I don't want
> to add stable mark in this patch because I feel it need enough
> testing in 64K page system I don't have. ;(
>
> From bb73e75ab0e21016f60858fd61e7dc6a6813e359 Mon Sep 17 00:00:00 2001
> From: Minchan Kim <minchan@kernel.org>
> Date: Thu, 9 Mar 2017 14:00:40 +0900
> Subject: [PATCH] zram: handle multiple pages attached bio's bvec
>
> Johannes Thumshirn reported system goes the panic when using NVMe over
> Fabrics loopback target with zram.
>
> The reason is zram expects each bvec in bio contains a single page
> but nvme can attach a huge bulk of pages attached to the bio's bvec
> so that zram's index arithmetic could be wrong so that out-of-bound
> access makes panic.
>
> This patch solves the problem via removing the limit(a bvec should
> contains a only single page).
>
> Cc: Hannes Reinecke <hare@suse.com>
> Reported-by: Johannes Thumshirn <jthumshirn@suse.de>
> Tested-by: Johannes Thumshirn <jthumshirn@suse.de>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> I don't add stable mark intentionally because I think it's rather risky
> without enough testing on 64K page system(ie, partial IO part).
>
> Thanks for the help, Johannes and Hannes!!
>
> drivers/block/zram/zram_drv.c | 37 ++++++++++---------------------------
> 1 file changed, 10 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 01944419b1f3..fefdf260503a 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -137,8 +137,7 @@ static inline bool valid_io_request(struct zram *zram,
>
> static void update_position(u32 *index, int *offset, struct bio_vec *bvec)
> {
> - if (*offset + bvec->bv_len >= PAGE_SIZE)
> - (*index)++;
> + *index += (*offset + bvec->bv_len) / PAGE_SIZE;
> *offset = (*offset + bvec->bv_len) % PAGE_SIZE;
> }
>
> @@ -838,34 +837,20 @@ static void __zram_make_request(struct zram *zram, struct bio *bio)
> }
>
> bio_for_each_segment(bvec, bio, iter) {
> - int max_transfer_size = PAGE_SIZE - offset;
> -
> - if (bvec.bv_len > max_transfer_size) {
> - /*
> - * zram_bvec_rw() can only make operation on a single
> - * zram page. Split the bio vector.
> - */
> - struct bio_vec bv;
> -
> - bv.bv_page = bvec.bv_page;
> - bv.bv_len = max_transfer_size;
> - bv.bv_offset = bvec.bv_offset;
> + struct bio_vec bv = bvec;
> + unsigned int remained = bvec.bv_len;
>
> + do {
> + bv.bv_len = min_t(unsigned int, PAGE_SIZE, remained);
> if (zram_bvec_rw(zram, &bv, index, offset,
> - op_is_write(bio_op(bio))) < 0)
> + op_is_write(bio_op(bio))) < 0)
> goto out;
>
> - bv.bv_len = bvec.bv_len - max_transfer_size;
> - bv.bv_offset += max_transfer_size;
> - if (zram_bvec_rw(zram, &bv, index + 1, 0,
> - op_is_write(bio_op(bio))) < 0)
> - goto out;
> - } else
> - if (zram_bvec_rw(zram, &bvec, index, offset,
> - op_is_write(bio_op(bio))) < 0)
> - goto out;
> + bv.bv_offset += bv.bv_len;
> + remained -= bv.bv_len;
>
> - update_position(&index, &offset, &bvec);
> + update_position(&index, &offset, &bv);
> + } while (remained);
> }
>
> bio_endio(bio);
> @@ -882,8 +867,6 @@ static blk_qc_t zram_make_request(struct request_queue *queue, struct bio *bio)
> {
> struct zram *zram = queue->queuedata;
>
> - blk_queue_split(queue, &bio, queue->bio_split);
> -
> if (!valid_io_request(zram, bio->bi_iter.bi_sector,
> bio->bi_iter.bi_size)) {
> atomic64_inc(&zram->stats.invalid_io);
> --
> 2.7.4
>
>
--
Kind regards,
Minchan Kim
^ permalink raw reply
* Re: RFC: always use REQ_OP_WRITE_ZEROES for zeroing offload
From: Mike Snitzer @ 2017-03-30 15:12 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, martin.petersen, agk, shli, philipp.reisner,
lars.ellenberg, linux-block, linux-raid, dm-devel, linux-scsi,
drbd-dev
In-Reply-To: <20170323143341.31549-1-hch@lst.de>
Would be very useful, particularly for testing, if
drivers/scsi/scsi_debug.c were updated to support WRITE ZEROES.
^ permalink raw reply
* Re: [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state
From: Bart Van Assche @ 2017-03-30 15:16 UTC (permalink / raw)
To: hare@suse.de, axboe@fb.com; +Cc: osandov@fb.com, linux-block@vger.kernel.org
In-Reply-To: <824697ce-3fe4-f7ab-e23b-237b280196fb@suse.de>
On Thu, 2017-03-30 at 07:50 +0200, Hannes Reinecke wrote:
> On 03/29/2017 10:20 PM, Bart Van Assche wrote:
> > Make it possible to check whether or not a block layer queue has
> > been stopped. Make it possible to run a blk-mq queue from user
> > space.
> >=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 | 84 ++++++++++++++++++++++++++++++++++++++++++=
++++++++
> > 1 file changed, 84 insertions(+)
> >=20
>=20
> About bloody time :-)
>=20
> Reviewed-by: Hannes Reinecke <hare@suse.com>
Hello Hannes,
Thanks for the review :-) However, had you noticed that I had already
posted a v2 of this patch? Anyway, since I have improved v2 further
after I had posted it, I will post a v3 today.
Bart.=
^ permalink raw reply
* Re: [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state
From: Jens Axboe @ 2017-03-30 15:19 UTC (permalink / raw)
To: Bart Van Assche, hare@suse.de; +Cc: osandov@fb.com, linux-block@vger.kernel.org
In-Reply-To: <1490886955.2753.3.camel@sandisk.com>
On 03/30/2017 09:16 AM, Bart Van Assche wrote:
> On Thu, 2017-03-30 at 07:50 +0200, Hannes Reinecke wrote:
>> On 03/29/2017 10:20 PM, Bart Van Assche wrote:
>>> Make it possible to check whether or not a block layer queue has
>>> been stopped. Make it possible to run a blk-mq queue from user
>>> space.
>>>
>>> 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 | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 84 insertions(+)
>>>
>>
>> About bloody time :-)
>>
>> Reviewed-by: Hannes Reinecke <hare@suse.com>
>
> Hello Hannes,
>
> Thanks for the review :-) However, had you noticed that I had already
> posted a v2 of this patch? Anyway, since I have improved v2 further
> after I had posted it, I will post a v3 today.
I didn't see a v2 posting?
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 22/23] drbd: implement REQ_OP_WRITE_ZEROES
From: Martin K. Petersen @ 2017-03-30 15:20 UTC (permalink / raw)
To: Mike Snitzer
Cc: Christoph Hellwig, axboe, martin.petersen, agk, shli,
philipp.reisner, linux-block, linux-scsi, drbd-dev, dm-devel,
linux-raid
In-Reply-To: <20170330134957.GA508@redhat.com>
Mike Snitzer <snitzer@redhat.com> writes:
> I can work on this now. Only question I have is: should DM thinp take
> care to zero any misaligned head and tail? (I assume so but with all
> the back and forth between Bart, Paolo and Martin I figured I'd ask
> explicitly).
Yep, let's make sure our semantics match the hardware ditto.
- So write zeroes should behave deterministically and explicitly handle
any blocks that can't be cleared via deprovisioning.
- And discard can work at the discard granularity in a
non-deterministic fashion.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: RFC: always use REQ_OP_WRITE_ZEROES for zeroing offload
From: Martin K. Petersen @ 2017-03-30 15:22 UTC (permalink / raw)
To: Mike Snitzer
Cc: Christoph Hellwig, axboe, martin.petersen, agk, shli,
philipp.reisner, lars.ellenberg, linux-block, linux-raid,
dm-devel, linux-scsi, drbd-dev
In-Reply-To: <20170330151257.GA910@redhat.com>
Mike Snitzer <snitzer@redhat.com> writes:
> Would be very useful, particularly for testing, if
> drivers/scsi/scsi_debug.c were updated to support WRITE ZEROES.
There is no WRITE ZEROES in SCSI. You should be able to get the right
behavior with lbpws=1 lbprz=1.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH] block/sed-opal: fix spelling mistake: "Lifcycle" -> "Lifecycle"
From: Jens Axboe @ 2017-03-30 15:23 UTC (permalink / raw)
To: Colin King, Scott Bauer, Jonathan Derrick, Rafael Antognolli,
linux-block
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20170330095808.19574-1-colin.king@canonical.com>
On 03/30/2017 03:58 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> trivial fix to spelling mistake in pr_err error message
Added for 4.12, thanks.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state
From: Bart Van Assche @ 2017-03-30 15:23 UTC (permalink / raw)
To: hare@suse.de, axboe@fb.com; +Cc: osandov@fb.com, linux-block@vger.kernel.org
In-Reply-To: <fe04eba2-a67d-9e02-83f4-1f519f79ed4b@fb.com>
On Thu, 2017-03-30 at 09:19 -0600, Jens Axboe wrote:
> On 03/30/2017 09:16 AM, Bart Van Assche wrote:
> > On Thu, 2017-03-30 at 07:50 +0200, Hannes Reinecke wrote:
> > > On 03/29/2017 10:20 PM, Bart Van Assche wrote:
> > > > Make it possible to check whether or not a block layer queue has
> > > > been stopped. Make it possible to run a blk-mq queue from user
> > > > space.
> > > >=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 | 84 ++++++++++++++++++++++++++++++++++++++=
++++++++++++
> > > > 1 file changed, 84 insertions(+)
> > > >=20
> > >=20
> > > About bloody time :-)
> > >=20
> > > Reviewed-by: Hannes Reinecke <hare@suse.com>
> >=20
> > Hello Hannes,
> >=20
> > Thanks for the review :-) However, had you noticed that I had already
> > posted a v2 of this patch? Anyway, since I have improved v2 further
> > after I had posted it, I will post a v3 today.
>=20
> I didn't see a v2 posting?
Hello Jens,
The label "v2" was missing from the subject, maybe that's why it didn't get
noticed. Anyway, v2 is available e.g. at URL
http://marc.info/?l=3Dlinux-block&m=3D149082319230648.
Bart.=
^ permalink raw reply
* Re: [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state
From: Jens Axboe @ 2017-03-30 15:27 UTC (permalink / raw)
To: Bart Van Assche
Cc: Omar Sandoval, Hannes Reinecke, linux-block@vger.kernel.org
In-Reply-To: <1D08B61A9CF0974AA09887BE32D889DA12BA96@ULS-OP-MBXIP03.sdcorp.global.sandisk.com>
On 03/29/2017 03:32 PM, Bart Van Assche wrote:
> Make it possible to check whether or not a block layer queue has
> been stopped. Make it possible to start and to run a blk-mq queue
> from user space.
Looks good, minor nitpicking below.
> +static int blk_queue_flags_show(struct seq_file *m, void *v)
> +{
> + struct request_queue *q = m->private;
> + bool sep = false;
> + int i;
> +
> + for (i = 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
> + if (!(q->queue_flags & BIT(i)))
> + continue;
> + if (sep)
> + seq_puts(m, " ");
> + sep = true;
Put that sep = true in the else branch?
> + if (strcmp(op, "run") == 0) {
> + blk_mq_run_hw_queues(q, true);
> + } else if (strcmp(op, "start") == 0) {
> + blk_mq_start_stopped_hw_queues(q, true);
> + } else {
> + pr_err("%s: unsupported operation %s\n", __func__, op);
> + return -EINVAL;
> + }
You are inconsistent with your braces. I'd prefer no braces for single
lines.
Should the error case include valid commands? It'd be nice to have this
available, and not have to resort to looking at the source to figure out
what commands are available.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 12/23] sd: handle REQ_UNMAP
From: Martin K. Petersen @ 2017-03-30 15:28 UTC (permalink / raw)
To: hch@lst.de
Cc: Bart Van Assche, agk@redhat.com, lars.ellenberg@linbit.com,
snitzer@redhat.com, martin.petersen@oracle.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org,
linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
linux-raid@vger.kernel.org
In-Reply-To: <20170330090201.GD12015@lst.de>
"hch@lst.de" <hch@lst.de> writes:
Christoph,
> On Tue, Mar 28, 2017 at 04:48:55PM +0000, Bart Van Assche wrote:
>> > if (sdp->no_write_same)
>> > return BLKPREP_INVALID;
>> > if (sdkp->ws16 || sector > 0xffffffff || nr_sectors > 0xffff)
>>=20
>> Users can change the provisioning mode from user space from=C2=A0SD_LBP_=
WS16 into
>> SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector >
>> 0xffffffff || nr_sectors > 0xffff) check if REQ_UNMAP is set.
>
> They can, and if the device has too many sectors that will already cause
> discard to fail,
I'm not sure I understand what you mean by that?
--=20
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH 23/23] block: remove the discard_zeroes_data flag
From: Martin K. Petersen @ 2017-03-30 15:29 UTC (permalink / raw)
To: hch@lst.de
Cc: Bart Van Assche, agk@redhat.com, lars.ellenberg@linbit.com,
snitzer@redhat.com, martin.petersen@oracle.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org,
linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
linux-raid@vger.kernel.org
In-Reply-To: <20170330090655.GF12015@lst.de>
"hch@lst.de" <hch@lst.de> writes:
> Jens, any opinion? I'd like to remove it too, but I fear it might
> break things. We could deprecate it first with a warning when read
> and then remove it a few releases down the road.
I know of several apps that check this variable (as opposed to the
ioctl).
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
From: Jens Axboe @ 2017-03-30 15:35 UTC (permalink / raw)
To: Minchan Kim, Johannes Thumshirn
Cc: Hannes Reinecke, Nitin Gupta, Christoph Hellwig,
Sergey Senozhatsky, yizhan, Linux Block Layer Mailinglist,
Linux Kernel Mailinglist, Andrew Morton
In-Reply-To: <CAEwNFnBnEObVdjqNdvh_-Dc0ryDjeW3c1qVdfOCbbyQREaDdqA@mail.gmail.com>
On 03/30/2017 09:08 AM, Minchan Kim wrote:
> Hi Jens,
>
> It seems you miss this.
> Could you handle this?
I can, but I'm a little confused. The comment talks about replacing
the one I merged with this one, I can't do that. I'm assuming you
are talking about this commit:
commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f
Author: Johannes Thumshirn <jthumshirn@suse.de>
Date: Mon Mar 6 11:23:35 2017 +0100
zram: set physical queue limits to avoid array out of bounds accesses
which is in mainline. The patch still applies, though.
Do we really REALLY need this for 4.11, or can we queue for 4.12 and
mark it stable?
--
Jens Axboe
^ permalink raw reply
* Re: RFC: always use REQ_OP_WRITE_ZEROES for zeroing offload
From: Mike Snitzer @ 2017-03-30 15:38 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, axboe, agk, shli, philipp.reisner,
lars.ellenberg, linux-block, linux-raid, dm-devel, linux-scsi,
drbd-dev
In-Reply-To: <yq1lgrm3i36.fsf@oracle.com>
On Thu, Mar 30 2017 at 11:22am -0400,
Martin K. Petersen <martin.petersen@oracle.com> wrote:
> Mike Snitzer <snitzer@redhat.com> writes:
>
> > Would be very useful, particularly for testing, if
> > drivers/scsi/scsi_debug.c were updated to support WRITE ZEROES.
>
> There is no WRITE ZEROES in SCSI. You should be able to get the right
> behavior with lbpws=1 lbprz=1.
OK, thanks.
^ permalink raw reply
* Re: Outstanding MQ questions from MMC
From: Ulf Hansson @ 2017-03-30 16:39 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Walleij, linux-mmc@vger.kernel.org, linux-block, Jens Axboe,
Christoph Hellwig, Adrian Hunter, Paolo Valente
In-Reply-To: <CAK8P3a1CGtj5VSVoMM34VWn781WpS8ZcHpq6n0UfNOWWYEq=QQ@mail.gmail.com>
On 30 March 2017 at 14:42, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Mar 29, 2017 at 5:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Hi folks,
>>
>> I earlier left some unanswered questions in my MMC to MQ conversion series
>> but I figured it is better if I collect them and ask the blk-mq
>> maintainers directly
>> how to deal with the following situations that occur in the MMC block layer:
>>
>>
>> 1. The current MMC code locks the host when the first request comes in
>> from blk_fetch_request() and unlocks it when blk_fetch_request() returns
>> NULL twice in a row. Then the polling thread terminated and is not restarted
>> until we get called by the mmc_request_fn.
>>
>> Host locking means that we will not send other commands to the MMC
>> card from i.e. userspace, which sometimes can send spurious stuff orthogonal
>> to the block layer. If the block layer has locked the host, userspace
>> has to wait
>> and vice versa. It is not a common contention point but it still happens.
>>
>> In MQ, I have simply locked the host on the first request and then I never
>> release it. Clearly this does not work. I am uncertain on how to handle this
>> and whether MQ has a way to tell us that the queue is empty so we may release
>> the host. I toyed with the idea to just set up a timer, but a "queue
>> empty" callback
>> from the block layer is what would be ideal.
>
> Would it be possible to change the userspace code to go through
> the block layer instead and queue a request there, to avoid having
> to lock the card at all?
That would be good from an I/O scheduling point of view, as it would
avoid one side being able to starve the other.
However, we would still need a lock, as we also have card detect work
queue, which also needs to claim the host when it polls for removable
cards.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 23/23] block: remove the discard_zeroes_data flag
From: hch @ 2017-03-30 17:29 UTC (permalink / raw)
To: Martin K. Petersen
Cc: hch@lst.de, Bart Van Assche, agk@redhat.com,
lars.ellenberg@linbit.com, snitzer@redhat.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org,
linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
linux-raid@vger.kernel.org
In-Reply-To: <yq1d1cy3hra.fsf@oracle.com>
On Thu, Mar 30, 2017 at 11:29:29AM -0400, Martin K. Petersen wrote:
> "hch@lst.de" <hch@lst.de> writes:
>
> > Jens, any opinion? I'd like to remove it too, but I fear it might
> > break things. We could deprecate it first with a warning when read
> > and then remove it a few releases down the road.
>
> I know of several apps that check this variable (as opposed to the
> ioctl).
The above was in reference to both methods..
^ permalink raw reply
* Re: [PATCH 12/23] sd: handle REQ_UNMAP
From: hch @ 2017-03-30 17:30 UTC (permalink / raw)
To: Martin K. Petersen
Cc: hch@lst.de, Bart Van Assche, agk@redhat.com,
lars.ellenberg@linbit.com, snitzer@redhat.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org,
linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
linux-raid@vger.kernel.org
In-Reply-To: <yq1h92a3hsv.fsf@oracle.com>
On Thu, Mar 30, 2017 at 11:28:32AM -0400, Martin K. Petersen wrote:
> "hch@lst.de" <hch@lst.de> writes:
>
> Christoph,
>
> > On Tue, Mar 28, 2017 at 04:48:55PM +0000, Bart Van Assche wrote:
> >> > if (sdp->no_write_same)
> >> > return BLKPREP_INVALID;
> >> > if (sdkp->ws16 || sector > 0xffffffff || nr_sectors > 0xffff)
> >>
> >> Users can change the provisioning mode from user space from�SD_LBP_WS16 into
> >> SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector >
> >> 0xffffffff || nr_sectors > 0xffff) check if REQ_UNMAP is set.
> >
> > They can, and if the device has too many sectors that will already cause
> > discard to fail,
>
> I'm not sure I understand what you mean by that?
If you manually change the provisioning mode to WS10 on a device that
must use WRITE SAME (16) to be able to address all blocks you're already
screwed right now, and with this patch you can screw yourself through
the WRITE_ZEROES path in addition to the DISCARD path.
^ permalink raw reply
* [PATCH] blk-mq: fix schedule-under-preempt for blocking drivers
From: Jens Axboe @ 2017-03-30 18:00 UTC (permalink / raw)
To: linux-block@vger.kernel.org; +Cc: Christoph Hellwig, Josef Bacik
Commit a4d907b6a33b unified the single and multi queue request handlers,
but in the process, it also screwed up the locking balance and calls
blk_mq_try_issue_directly() with the ctx preempt lock held. This is a
problem for drivers that have set BLK_MQ_F_BLOCKING, since now they
can't reliably sleep.
While in there, protect against similar issues in the future, by adding
a might_sleep() trigger in the BLOCKING path for direct issue or queue
run.
Reported-by: Josef Bacik <josef@toxicpanda.com>
Tested-by: Josef Bacik <josef@toxicpanda.com>
Fixes: a4d907b6a33b ("blk-mq: streamline blk_mq_make_request")
Signed-off-by: Jens Axboe <axboe@fb.com>
diff --git a/block/blk-mq.c b/block/blk-mq.c
index e6ac774b0e41..d4d6ed4e7250 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1122,6 +1122,8 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
blk_mq_sched_dispatch_requests(hctx);
rcu_read_unlock();
} else {
+ might_sleep();
+
srcu_idx = srcu_read_lock(&hctx->queue_rq_srcu);
blk_mq_sched_dispatch_requests(hctx);
srcu_read_unlock(&hctx->queue_rq_srcu, srcu_idx);
@@ -1496,7 +1498,11 @@ static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
__blk_mq_try_issue_directly(rq, cookie, false);
rcu_read_unlock();
} else {
- unsigned int srcu_idx = srcu_read_lock(&hctx->queue_rq_srcu);
+ unsigned int srcu_idx;
+
+ might_sleep();
+
+ srcu_idx = srcu_read_lock(&hctx->queue_rq_srcu);
__blk_mq_try_issue_directly(rq, cookie, true);
srcu_read_unlock(&hctx->queue_rq_srcu, srcu_idx);
}
@@ -1596,18 +1602,23 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
list_del_init(&same_queue_rq->queuelist);
list_add_tail(&rq->queuelist, &plug->mq_list);
+ blk_mq_put_ctx(data.ctx);
+
if (same_queue_rq)
blk_mq_try_issue_directly(data.hctx, same_queue_rq,
&cookie);
+
+ return cookie;
} else if (q->nr_hw_queues > 1 && is_sync) {
+ blk_mq_put_ctx(data.ctx);
blk_mq_bio_to_request(rq, bio);
blk_mq_try_issue_directly(data.hctx, rq, &cookie);
+ return cookie;
} else if (q->elevator) {
blk_mq_bio_to_request(rq, bio);
blk_mq_sched_insert_request(rq, false, true, true, true);
- } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio)) {
+ } else if (!blk_mq_merge_queue_io(data.hctx, data.ctx, rq, bio))
blk_mq_run_hw_queue(data.hctx, true);
- }
blk_mq_put_ctx(data.ctx);
return cookie;
^ permalink raw reply related
* Re: [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state
From: Bart Van Assche @ 2017-03-30 18:10 UTC (permalink / raw)
To: axboe@fb.com; +Cc: osandov@fb.com, hare@suse.de, linux-block@vger.kernel.org
In-Reply-To: <f80153aa-3e7d-0a20-e426-7e58cd7c249d@fb.com>
On Thu, 2017-03-30 at 09:27 -0600, Jens Axboe wrote:
> On 03/29/2017 03:32 PM, Bart Van Assche wrote:
> > Make it possible to check whether or not a block layer queue has
> > been stopped. Make it possible to start and to run a blk-mq queue
> > from user space.
>=20
> Looks good, minor nitpicking below.
>=20
> > +static int blk_queue_flags_show(struct seq_file *m, void *v)
> > +{
> > + struct request_queue *q =3D m->private;
> > + bool sep =3D false;
> > + int i;
> > +
> > + for (i =3D 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
> > + if (!(q->queue_flags & BIT(i)))
> > + continue;
> > + if (sep)
> > + seq_puts(m, " ");
> > + sep =3D true;
>=20
> Put that sep =3D true in the else branch?
I can do that, but that would result in slightly less efficient assembler
code (additional jump) while stores can be pipelined easily.
> > + if (strcmp(op, "run") =3D=3D 0) {
> > + blk_mq_run_hw_queues(q, true);
> > + } else if (strcmp(op, "start") =3D=3D 0) {
> > + blk_mq_start_stopped_hw_queues(q, true);
> > + } else {
> > + pr_err("%s: unsupported operation %s\n", __func__, op);
> > + return -EINVAL;
> > + }
>=20
> You are inconsistent with your braces. I'd prefer no braces for single
> lines.
Sorry but if braces are used for one side of an if-then-else statement then
checkpatch wants braces to be used for the other side too, even if that oth=
er
side is only a single line. From the checkpatch source code:
# check for single line unbalanced braces
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0if ($sline =3D~ /^.\s*\}\s*=
else\s*$/ ||
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0$sline =3D~ /^.=
\s*else\s*\{\s*$/) {
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0CHK=
("BRACES", "Unbalanced braces around else statement\n" . $herecurr);
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0}
> Should the error case include valid commands? It'd be nice to have this
> available, and not have to resort to looking at the source to figure out
> what commands are available.
OK, I will update the error message.
Thanks for the review.
Bart.=
^ permalink raw reply
* Re: [PATCH] blk-mq: Export queue state through /sys/kernel/debug/block/*/state
From: Jens Axboe @ 2017-03-30 18:14 UTC (permalink / raw)
To: Bart Van Assche; +Cc: osandov@fb.com, hare@suse.de, linux-block@vger.kernel.org
In-Reply-To: <1490897421.2753.12.camel@sandisk.com>
On 03/30/2017 12:10 PM, Bart Van Assche wrote:
> On Thu, 2017-03-30 at 09:27 -0600, Jens Axboe wrote:
>> On 03/29/2017 03:32 PM, Bart Van Assche wrote:
>>> Make it possible to check whether or not a block layer queue has
>>> been stopped. Make it possible to start and to run a blk-mq queue
>>> from user space.
>>
>> Looks good, minor nitpicking below.
>>
>>> +static int blk_queue_flags_show(struct seq_file *m, void *v)
>>> +{
>>> + struct request_queue *q = m->private;
>>> + bool sep = false;
>>> + int i;
>>> +
>>> + for (i = 0; i < sizeof(q->queue_flags) * BITS_PER_BYTE; i++) {
>>> + if (!(q->queue_flags & BIT(i)))
>>> + continue;
>>> + if (sep)
>>> + seq_puts(m, " ");
>>> + sep = true;
>>
>> Put that sep = true in the else branch?
>
> I can do that, but that would result in slightly less efficient assembler
> code (additional jump) while stores can be pipelined easily.
It's a sysfs show function, it's not like we care about branching. But we
can leave it as-is, I don't feel that strongly about it at all.
>>> + if (strcmp(op, "run") == 0) {
>>> + blk_mq_run_hw_queues(q, true);
>>> + } else if (strcmp(op, "start") == 0) {
>>> + blk_mq_start_stopped_hw_queues(q, true);
>>> + } else {
>>> + pr_err("%s: unsupported operation %s\n", __func__, op);
>>> + return -EINVAL;
>>> + }
>>
>> You are inconsistent with your braces. I'd prefer no braces for single
>> lines.
>
> Sorry but if braces are used for one side of an if-then-else statement then
> checkpatch wants braces to be used for the other side too, even if that other
> side is only a single line. From the checkpatch source code:
Omar informs me that it's coding style mandated. The block layer generally
does NOT do braces, so I'd prefer to keep it consistent at least. Again, not
really a huge problem, and as I prefaced the original email with, totally
nitpicking.
--
Jens Axboe
^ permalink raw reply
* [PATCH 0/2] Export more queue state information through debugfs
From: Bart Van Assche @ 2017-03-30 18:21 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Bart Van Assche
Hello Jens,
This is a short patch series with one patch that exports the queue state
and another patch that shows symbolic names for hctx state and flags
instead of a numerical bitmask.
Please consider these patches for kernel v4.12.
Thanks,
Bart.
Changes compared to v2:
- Introduced function blk_flags_show() in patch 1.
- Modified error message in patch 1 such that it shows the names of the
valid commands.
- Added patch "blk-mq: Show symbolic names for hctx state and flags"
Changes compared to v1:
- Constified blk_queue_flag_name.
- Left out QUEUE_FLAG_VIRT because it is a synonym of QUEUE_FLAG_NONROT.
- Check array size before reading from blk_queue_flag_name[].
- Add functionality to restart a block layer queue.
Bart Van Assche (2):
blk-mq: Export queue state through /sys/kernel/debug/block/*/state
blk-mq: Show symbolic names for hctx state and flags
block/blk-mq-debugfs.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 140 insertions(+), 3 deletions(-)
--
2.12.0
^ permalink raw reply
* [PATCH 1/2] blk-mq: Export queue state through /sys/kernel/debug/block/*/state
From: Bart Van Assche @ 2017-03-30 18:21 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Bart Van Assche, Omar Sandoval, Hannes Reinecke
In-Reply-To: <20170330182127.24288-1-bart.vanassche@sandisk.com>
Make it possible to check whether or not a block layer queue has
been stopped. Make it possible to start and to run a blk-mq queue
from user space.
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 | 107 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 4b3f962a9c7a..9e86056d47cd 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -43,6 +43,110 @@ static int blk_mq_debugfs_seq_open(struct inode *inode, struct file *file,
return ret;
}
+static int blk_flags_show(struct seq_file *m, const unsigned long flags,
+ const char *const *flag_name, int flag_name_count)
+{
+ bool sep = false;
+ int i;
+
+ for (i = 0; i < sizeof(flags) * BITS_PER_BYTE; i++) {
+ if (!(flags & BIT(i)))
+ continue;
+ if (sep)
+ seq_puts(m, " ");
+ sep = true;
+ if (i < flag_name_count && flag_name[i])
+ seq_puts(m, flag_name[i]);
+ else
+ seq_printf(m, "%d", i);
+ }
+ seq_puts(m, "\n");
+ return 0;
+}
+
+static const char *const blk_queue_flag_name[] = {
+ [QUEUE_FLAG_QUEUED] = "QUEUED",
+ [QUEUE_FLAG_STOPPED] = "STOPPED",
+ [QUEUE_FLAG_SYNCFULL] = "SYNCFULL",
+ [QUEUE_FLAG_ASYNCFULL] = "ASYNCFULL",
+ [QUEUE_FLAG_DYING] = "DYING",
+ [QUEUE_FLAG_BYPASS] = "BYPASS",
+ [QUEUE_FLAG_BIDI] = "BIDI",
+ [QUEUE_FLAG_NOMERGES] = "NOMERGES",
+ [QUEUE_FLAG_SAME_COMP] = "SAME_COMP",
+ [QUEUE_FLAG_FAIL_IO] = "FAIL_IO",
+ [QUEUE_FLAG_STACKABLE] = "STACKABLE",
+ [QUEUE_FLAG_NONROT] = "NONROT",
+ [QUEUE_FLAG_IO_STAT] = "IO_STAT",
+ [QUEUE_FLAG_DISCARD] = "DISCARD",
+ [QUEUE_FLAG_NOXMERGES] = "NOXMERGES",
+ [QUEUE_FLAG_ADD_RANDOM] = "ADD_RANDOM",
+ [QUEUE_FLAG_SECERASE] = "SECERASE",
+ [QUEUE_FLAG_SAME_FORCE] = "SAME_FORCE",
+ [QUEUE_FLAG_DEAD] = "DEAD",
+ [QUEUE_FLAG_INIT_DONE] = "INIT_DONE",
+ [QUEUE_FLAG_NO_SG_MERGE] = "NO_SG_MERGE",
+ [QUEUE_FLAG_POLL] = "POLL",
+ [QUEUE_FLAG_WC] = "WC",
+ [QUEUE_FLAG_FUA] = "FUA",
+ [QUEUE_FLAG_FLUSH_NQ] = "FLUSH_NQ",
+ [QUEUE_FLAG_DAX] = "DAX",
+ [QUEUE_FLAG_STATS] = "STATS",
+ [QUEUE_FLAG_RESTART] = "RESTART",
+ [QUEUE_FLAG_POLL_STATS] = "POLL_STATS",
+ [QUEUE_FLAG_REGISTERED] = "REGISTERED",
+};
+
+static int blk_queue_flags_show(struct seq_file *m, void *v)
+{
+ struct request_queue *q = m->private;
+
+ blk_flags_show(m, q->queue_flags, blk_queue_flag_name,
+ ARRAY_SIZE(blk_queue_flag_name));
+ return 0;
+}
+
+static ssize_t blk_queue_flags_store(struct file *file, const char __user *ubuf,
+ size_t len, loff_t *offp)
+{
+ struct request_queue *q = file_inode(file)->i_private;
+ char op[16] = { }, *s;
+
+ len = min(len, sizeof(op) - 1);
+ if (copy_from_user(op, ubuf, len))
+ return -EFAULT;
+ s = op;
+ strsep(&s, " \t\n"); /* strip trailing whitespace */
+ if (strcmp(op, "run") == 0) {
+ blk_mq_run_hw_queues(q, true);
+ } else if (strcmp(op, "start") == 0) {
+ blk_mq_start_stopped_hw_queues(q, true);
+ } else {
+ pr_err("%s: unsupported operation %s. Use either 'run' or 'start'\n",
+ __func__, op);
+ return -EINVAL;
+ }
+ return len;
+}
+
+static int blk_queue_flags_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, blk_queue_flags_show, inode->i_private);
+}
+
+static const struct file_operations blk_queue_flags_fops = {
+ .open = blk_queue_flags_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+ .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) {
@@ -735,6 +839,9 @@ 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 related
* [PATCH 2/2] blk-mq: Show symbolic names for hctx state and flags
From: Bart Van Assche @ 2017-03-30 18:21 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Bart Van Assche, Omar Sandoval, Hannes Reinecke
In-Reply-To: <20170330182127.24288-1-bart.vanassche@sandisk.com>
Instead of showing the hctx state and flags as numbers, show the
names of the flags.
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 | 37 ++++++++++++++++++++++++++++++++++---
1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 9e86056d47cd..91d09f58a596 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -183,11 +183,19 @@ static const struct file_operations queue_poll_stat_fops = {
.release = single_release,
};
+static const char *const hctx_state_name[] = {
+ [BLK_MQ_S_STOPPED] = "STOPPED",
+ [BLK_MQ_S_TAG_ACTIVE] = "TAG_ACTIVE",
+ [BLK_MQ_S_SCHED_RESTART] = "SCHED_RESTART",
+ [BLK_MQ_S_TAG_WAITING] = "TAG_WAITING",
+
+};
static int hctx_state_show(struct seq_file *m, void *v)
{
struct blk_mq_hw_ctx *hctx = m->private;
- seq_printf(m, "0x%lx\n", hctx->state);
+ blk_flags_show(m, hctx->state, hctx_state_name,
+ ARRAY_SIZE(hctx_state_name));
return 0;
}
@@ -203,11 +211,34 @@ static const struct file_operations hctx_state_fops = {
.release = single_release,
};
+static const char *const alloc_policy_name[] = {
+ [BLK_TAG_ALLOC_FIFO] = "fifo",
+ [BLK_TAG_ALLOC_RR] = "rr",
+};
+
+static const char *const hctx_flag_name[] = {
+ [ilog2(BLK_MQ_F_SHOULD_MERGE)] = "SHOULD_MERGE",
+ [ilog2(BLK_MQ_F_TAG_SHARED)] = "TAG_SHARED",
+ [ilog2(BLK_MQ_F_SG_MERGE)] = "SG_MERGE",
+ [ilog2(BLK_MQ_F_BLOCKING)] = "BLOCKING",
+ [ilog2(BLK_MQ_F_NO_SCHED)] = "NO_SCHED",
+};
+
static int hctx_flags_show(struct seq_file *m, void *v)
{
struct blk_mq_hw_ctx *hctx = m->private;
-
- seq_printf(m, "0x%lx\n", hctx->flags);
+ const int alloc_policy = BLK_MQ_FLAG_TO_ALLOC_POLICY(hctx->flags);
+
+ seq_puts(m, "alloc_policy=");
+ if (alloc_policy < ARRAY_SIZE(alloc_policy_name) &&
+ alloc_policy_name[alloc_policy])
+ seq_puts(m, alloc_policy_name[alloc_policy]);
+ else
+ seq_printf(m, "%d", alloc_policy);
+ seq_puts(m, " ");
+ blk_flags_show(m,
+ hctx->flags ^ BLK_ALLOC_POLICY_TO_MQ_FLAG(alloc_policy),
+ hctx_flag_name, ARRAY_SIZE(hctx_flag_name));
return 0;
}
--
2.12.0
^ permalink raw reply related
* Re: [PATCH] blk-mq: fix schedule-under-preempt for blocking drivers
From: Christoph Hellwig @ 2017-03-30 18:28 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block@vger.kernel.org, Christoph Hellwig, Josef Bacik
In-Reply-To: <b298fcf4-47bc-b2e1-7c8f-b4affe347501@kernel.dk>
Oh, I need to add one of the blocking drivers to my test runs..
Looks fine:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ 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