* Re: sense handling improvements
From: Martin K. Petersen @ 2017-02-23 14:28 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Martin K. Petersen, axboe, linux-block, linux-scsi
In-Reply-To: <20170223094948.GA29551@lst.de>
>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:
Christoph,
>> I applied 1-4 to 4.11/scsi-fixes. Both 5 and 6 had problems so please
>> fix those up.
Christoph> What kind of problem? I didn't see anything on the list.
They didn't apply. I tried to fix them up by hand. 5 was easy but 6
caused a flurry of failures that I ran out of time to look into.
So please resubmit 5 and 6 against 4.11/scsi-fixes.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH 1/4] f2fs: avoid very large discard command
From: Christoph Hellwig @ 2017-02-23 10:16 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel, linux-block
In-Reply-To: <20170223042850.43456-1-jaegeuk@kernel.org>
On Wed, Feb 22, 2017 at 08:28:47PM -0800, Jaegeuk Kim wrote:
> This patch adds MAX_DISCARD_BLOCKS() to avoid issuing too much large single
> discard command.
Needs an explanation in the code on why this number was chosen.
In doubt I suspect it should be a quirk in the driver for the device,
and not something decided by the fs.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> fs/f2fs/f2fs.h | 3 ++-
> fs/f2fs/segment.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index d076b94530bc..5f3fe97df055 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -133,7 +133,8 @@ enum {
> (SM_I(sbi)->trim_sections * (sbi)->segs_per_sec)
> #define BATCHED_TRIM_BLOCKS(sbi) \
> (BATCHED_TRIM_SEGMENTS(sbi) << (sbi)->log_blocks_per_seg)
> -
> +#define MAX_DISCARD_BLOCKS(sbi) \
> + ((1 << (sbi)->log_blocks_per_seg) * (sbi)->segs_per_sec)
> #define DISCARD_ISSUE_RATE 8
> #define DEF_CP_INTERVAL 60 /* 60 secs */
> #define DEF_IDLE_INTERVAL 5 /* 5 secs */
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index fe434cd872b4..567019940e9b 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -886,7 +886,8 @@ static void __add_discard_entry(struct f2fs_sb_info *sbi,
> if (!list_empty(head)) {
> last = list_last_entry(head, struct discard_entry, list);
> if (START_BLOCK(sbi, cpc->trim_start) + start ==
> - last->blkaddr + last->len) {
> + last->blkaddr + last->len &&
> + last->len < MAX_DISCARD_BLOCKS(sbi)) {
> last->len += end - start;
> goto done;
> }
> --
> 2.11.0
>
---end quoted text---
^ permalink raw reply
* Re: [PATCH] block/sed: Embed function data into the function sequence
From: Christoph Hellwig @ 2017-02-23 9:54 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-block, linux-nvme, Scott Bauer, Rafael Antognolli,
Jens Axboe, Christoph Hellwig
In-Reply-To: <1487775313-10188-1-git-send-email-jonathan.derrick@intel.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: sense handling improvements
From: Christoph Hellwig @ 2017-02-23 9:49 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: Christoph Hellwig, axboe, linux-block, linux-scsi
In-Reply-To: <yq1shn57mqn.fsf@oracle.com>
On Wed, Feb 22, 2017 at 07:51:44PM -0500, Martin K. Petersen wrote:
> I applied 1-4 to 4.11/scsi-fixes. Both 5 and 6 had problems so please
> fix those up.
What kind of problem? I didn't see anything on the list.
^ permalink raw reply
* Re: sense handling improvements
From: Martin K. Petersen @ 2017-02-23 0:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Martin K. Petersen, axboe, linux-block, linux-scsi
In-Reply-To: <20170222071915.GA18084@lst.de>
>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:
Christoph,
>> I can bring them in after Linus' initial block pull.
Christoph> Both the block and SCSI trees are now merged by Linus, and
Christoph> Jens didn't pick up patch one from this series yet - maybe
Christoph> it's best to send the whole series through the SCSI tree in
Christoph> this case.
I applied 1-4 to 4.11/scsi-fixes. Both 5 and 6 had problems so please
fix those up.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Linus Torvalds @ 2017-02-23 0:16 UTC (permalink / raw)
To: Markus Trippelsdorf
Cc: Jens Axboe, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20170222215058.GA291@x4>
On Wed, Feb 22, 2017 at 1:50 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
>
> But what about e.g. SATA SSDs? Wouldn't they be better off without any
> scheduler?
> So perhaps setting "none" for queue/rotational==0 and mq-deadline for
> spinning drives automatically in the sq blk-mq case?
Jens already said that the merging advantage can outweigh the costs,
but he didn't actually talk much about it.
The scheduler advantage can outweigh the costs of running a scheduler
by an absolutely _huge_ amount.
An SSD isn't zero-cost, and each command tends to have some fixed
overhead on the controller, and pretty much all SSD's heavily prefer
fewer large request over lots of tiny ones.
There are also fairness/latency issues that tend to very heavily favor
having an actual scheduler, ie reads want to be scheduled before
writes on an SSD (within reason) in order to make latency better.
Ten years ago, there were lots of people who argued that you don't
want to do do scheduling for SSD's, because SSD's were so fast that
you only added overhead. Nobody really believes that fairytale any
more.
So you might have particular loads that look better with noop, but
they will be rare and far between. Try it, by all means, and if it
works for you, set it in your udev rules.
The main place where a noop scheduler currently might make sense is
likely for a ramdisk, but quite frankly, since the main real usecase
for a ram-disk tends to be to make it easy to profile and find the
bottlenecks for performance analysis (for emulating future "infinitely
fast" media), even that isn't true - using noop there defeats the
whole purpose.
Linus
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-22 21:55 UTC (permalink / raw)
To: Markus Trippelsdorf
Cc: Linus Torvalds, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20170222215058.GA291@x4>
On 02/22/2017 02:50 PM, Markus Trippelsdorf wrote:
> On 2017.02.22 at 11:44 -0700, Jens Axboe wrote:
>> On 02/22/2017 11:42 AM, Linus Torvalds wrote:
>>> On Wed, Feb 22, 2017 at 10:26 AM, Linus Torvalds
>>> <torvalds@linux-foundation.org> wrote:
>>>>
>>>> And dammit, IF YOU DON'T EVEN KNOW, WHY THE HELL ARE YOU ASKING THE POOR USER?
>>>
>>> Basically, I'm pushing back on config options that I can't personally
>>> even sanely answer.
>>
>> I got that much, and I don't disagree on that part.
>>
>>> If it's a config option about "do I have a particular piece of
>>> hardware", it makes sense. But these new ones were just complete
>>> garbage.
>>>
>>> The whole "default IO scheduler" thing is a disease. We should stop
>>> making up these shit schedulers and then say "we don't know which one
>>> works best for you".
>>>
>>> All it does is encourage developers to make shortcuts and create crap
>>> that isn't generically useful, and then blame the user and say "well,
>>> you should have picked a different scheduler" when they say "this does
>>> not work well for me".
>>>
>>> We have had too many of those kinds of broken choices. And when the
>>> new Kconfig options get so confusing and so esoteric that I go "Hmm, I
>>> have no idea if my hardware does a single queue or not", I put my foot
>>> down.
>>>
>>> When the IO scheduler questions were about a generic IO scheduler for
>>> everything, I can kind of understand them. I think it was still a
>>> mistake (for the reasons outline above), but at least it was a
>>> comprehensible question to ask.
>>>
>>> But when it gets to "what should I do about a single-queue version of
>>> a MQ scheduler", the question is no longer even remotely sensible. The
>>> question should simply NOT EXIST. There is no possible valid reason to
>>> ask that kind of crap.
>>
>> OK, so here's what I'll do:
>>
>> 1) We'll kill the default scheduler choices. sq blk-mq will default to
>> mq-deadline, mq blk-mq will default to "none" (at least for now, until
>> the new scheduler is done).
>
> But what about e.g. SATA SSDs? Wouldn't they be better off without any
> scheduler?
Marginal. If they are single queue, using a basic scheduler like
deadline isn't going to be a significant amount of overhead. In some
cases they are going to be better off, due to better merging. In the
worst case, overhead is slightly higher. Net result is positive, I'd
say.
> So perhaps setting "none" for queue/rotational==0 and mq-deadline for
> spinning drives automatically in the sq blk-mq case?
You can do that through a udev rule. The kernel doesn't know if the
device is rotational or not when we set up the scheduler. So we'd either
have to add code to do that, or simply just do it with a udev rule. I'd
prefer the latter.
--
Jens Axboe
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Markus Trippelsdorf @ 2017-02-22 21:50 UTC (permalink / raw)
To: Jens Axboe
Cc: Linus Torvalds, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <c1d07f31-40e7-0fb0-7251-24067564a0ad@kernel.dk>
On 2017.02.22 at 11:44 -0700, Jens Axboe wrote:
> On 02/22/2017 11:42 AM, Linus Torvalds wrote:
> > On Wed, Feb 22, 2017 at 10:26 AM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >>
> >> And dammit, IF YOU DON'T EVEN KNOW, WHY THE HELL ARE YOU ASKING THE POOR USER?
> >
> > Basically, I'm pushing back on config options that I can't personally
> > even sanely answer.
>
> I got that much, and I don't disagree on that part.
>
> > If it's a config option about "do I have a particular piece of
> > hardware", it makes sense. But these new ones were just complete
> > garbage.
> >
> > The whole "default IO scheduler" thing is a disease. We should stop
> > making up these shit schedulers and then say "we don't know which one
> > works best for you".
> >
> > All it does is encourage developers to make shortcuts and create crap
> > that isn't generically useful, and then blame the user and say "well,
> > you should have picked a different scheduler" when they say "this does
> > not work well for me".
> >
> > We have had too many of those kinds of broken choices. And when the
> > new Kconfig options get so confusing and so esoteric that I go "Hmm, I
> > have no idea if my hardware does a single queue or not", I put my foot
> > down.
> >
> > When the IO scheduler questions were about a generic IO scheduler for
> > everything, I can kind of understand them. I think it was still a
> > mistake (for the reasons outline above), but at least it was a
> > comprehensible question to ask.
> >
> > But when it gets to "what should I do about a single-queue version of
> > a MQ scheduler", the question is no longer even remotely sensible. The
> > question should simply NOT EXIST. There is no possible valid reason to
> > ask that kind of crap.
>
> OK, so here's what I'll do:
>
> 1) We'll kill the default scheduler choices. sq blk-mq will default to
> mq-deadline, mq blk-mq will default to "none" (at least for now, until
> the new scheduler is done).
But what about e.g. SATA SSDs? Wouldn't they be better off without any
scheduler?
So perhaps setting "none" for queue/rotational==0 and mq-deadline for
spinning drives automatically in the sq blk-mq case?
--
Markus
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-22 21:29 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CA+55aFzokxXsAJSnJb5n6k0haNW5_7SpVmq9EZ0ZXfAVpAmSow@mail.gmail.com>
On 02/22/2017 12:04 PM, Linus Torvalds wrote:
> On Wed, Feb 22, 2017 at 10:58 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 02/22/2017 11:56 AM, Linus Torvalds wrote:
>>
>> OK, so here's what I'll do:
>>
>> 1) We'll kill the default scheduler choices. sq blk-mq will default to
>> mq-deadline, mq blk-mq will default to "none" (at least for now, until
>> the new scheduler is done).
>> 2) The individual schedulers will be y/m/n selectable, just like any
>> other driver.
>
> Yes. That makes sense as options. I can (or, perhaps even more
> importantly, a distro can) answer those kinds of questions.
Someone misspelled pacman:
parman (PARMAN) [N/m/y] (NEW) ?
There is no help available for this option.
Or I think it's pacman, because I have no idea what else it could be. I'm
going to say N.
--
Jens Axboe
^ permalink raw reply
* Re: [WIP PATCHSET 0/4] WIP branch for bfq-mq
From: Paolo Valente @ 2017-02-22 21:29 UTC (permalink / raw)
To: Bart Van Assche
Cc: ulf.hansson@linaro.org, tj@kernel.org,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
broonie@kernel.org, linus.walleij@linaro.org, axboe@kernel.dk
In-Reply-To: <1487025524.2719.11.camel@sandisk.com>
> Il giorno 13 feb 2017, alle ore 23:38, Bart Van Assche =
<bart.vanassche@sandisk.com> ha scritto:
>=20
> On Mon, 2017-02-13 at 22:07 +0000, Bart Van Assche wrote:
>> On Mon, 2017-02-13 at 22:07 +0100, Paolo Valente wrote:
>>> but what do you think about trying this fix?
>>=20
>> Sorry but with ... the same server I used for the previous test still
>> didn't boot up properly. A screenshot is available at
>> https://goo.gl/photos/Za9QVGCNe2BJBwxVA.
>>=20
>>> Otherwise, if you have no news or suggestions, would you be willing =
to
>>> try my micro-logging proposal =
https://github.com/Algodev-github/bfq-mq?
>>=20
>> Sorry but it's not clear to me what logging mechanism you are =
referring
>> to and how to enable it? Are you perhaps referring to
>> CONFIG_BFQ_REDIRECT_TO_CONSOLE?
>=20
> Anyway, a second screenshot has been added to the same album after I =
had
> applied the following patch:
>=20
Hi Bart,
thanks for this second attempt of yours. Although, unfortunately, not
providing some clear indication of the exact cause of your hang (apart
from a possible deadlock), your log helped me notice another bug.
At any rate, as I have just written to Jens, I have pushed a new
version of the branch [1] (not just added new commits, but also
integrated some old commit with new changes, to make it more quickly).
The branch now contains both a fix for the above bug, and, more
importantly, a fix for the circular dependencies that were still
lurking around. Could you please test it?
Crossing my fingers,
Paolo
[1] https://github.com/Algodev-github/bfq-mq
> diff --git a/block/Makefile b/block/Makefile
> index 1c04fe19e825..bf472ac82c08 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -2,6 +2,8 @@
> # Makefile for the kernel block layer
> #
> =20
> +KBUILD_CFLAGS +=3D -DCONFIG_BFQ_REDIRECT_TO_CONSOLE
> +
>=20
> obj-$(CONFIG_BLOCK) :=3D bio.o elevator.o blk-core.o blk-tag.o =
blk-sysfs.o \
> blk-flush.o blk-settings.o blk-ioc.o blk-map.o =
\
> blk-exec.o blk-merge.o blk-softirq.o =
blk-timeout.o \
>=20
> Bart.
> Western Digital Corporation (and its subsidiaries) E-mail =
Confidentiality Notice & Disclaimer:
>=20
> This e-mail and any files transmitted with it may contain confidential =
or legally privileged information of WDC and/or its affiliates, and are =
intended solely for the use of the individual or entity to which they =
are addressed. If you are not the intended recipient, any disclosure, =
copying, distribution or any action taken or omitted to be taken in =
reliance on it, is prohibited. If you have received this e-mail in =
error, please notify the sender immediately and delete the e-mail in its =
entirety from your system.
>=20
^ permalink raw reply
* Re: [PATCH] blk-mq-sched: don't hold queue_lock when calling exit_icq
From: Paolo Valente @ 2017-02-22 21:21 UTC (permalink / raw)
To: Jens Axboe; +Cc: Omar Sandoval, linux-block, kernel-team
In-Reply-To: <F598CD85-0340-4189-AF45-D30C40DA4A07@linaro.org>
> Il giorno 17 feb 2017, alle ore 11:30, Paolo Valente =
<paolo.valente@linaro.org> ha scritto:
>=20
>=20
>> Il giorno 16 feb 2017, alle ore 11:31, Paolo Valente =
<paolo.valente@linaro.org> ha scritto:
>>=20
>>>=20
>>> Il giorno 15 feb 2017, alle ore 19:04, Jens Axboe <axboe@fb.com> ha =
scritto:
>>>=20
>>> On 02/15/2017 10:58 AM, Jens Axboe wrote:
>>>> On 02/15/2017 10:24 AM, Paolo Valente wrote:
>>>>>=20
>>>>>> Il giorno 10 feb 2017, alle ore 19:32, Omar Sandoval =
<osandov@osandov.com> ha scritto:
>>>>>>=20
>>>>>> From: Omar Sandoval <osandov@fb.com>
>>>>>>=20
>>>>>> None of the other blk-mq elevator hooks are called with this lock =
held.
>>>>>> Additionally, it can lead to circular locking dependencies =
between
>>>>>> queue_lock and the private scheduler lock.
>>>>>>=20
>>>>>=20
>>>>> Hi Omar,
>>>>> I'm sorry but it seems that a new potential deadlock has showed =
up.
>>>>> See lockdep splat below.
>>>>>=20
>>>>> I've tried to think about different solutions than turning back to
>>>>> deferring the body of exit_icq, but at no avail.
>>>>=20
>>>> Looks like a interaction between bfqd->lock and q->queue_lock. =
Since the
>>>> core has no notion of you bfqd->lock, the naturally dependency here
>>>> would be to nest bfqd->lock inside q->queue_lock. Is that possible =
for
>>>> you?
>>>>=20
>>>> Looking at the code a bit, maybe it'd just be simpler to get rid of
>>>> holding the queue lock for that spot. For the mq scheduler, we =
really
>>>> don't want places where we invoke with that held already. Does the =
below
>>>> work for you?
>>>=20
>>> Would need to remove one more lockdep assert. And only test this for
>>> the mq parts, we'd need to spread a bit of love on the classic
>>> scheduling icq exit path for this to work on that side.
>>>=20
>>=20
>> Sorry Jens, same splat. What confuses me is the second column
>> in the possible scenario:
>>=20
>> [ 139.368477] CPU0 CPU1
>> [ 139.369129] ---- ----
>> [ 139.369774] lock(&(&ioc->lock)->rlock);
>> [ 139.370339] =
lock(&(&q->__queue_lock)->rlock);
>> [ 139.390579] =
lock(&(&ioc->lock)->rlock);
>> [ 139.391522] lock(&(&bfqd->lock)->rlock);
>>=20
>> I could not find any code path, related to the reported call traces,
>> and taking first q->queue_lock and then ioc->lock.
>>=20
>> Any suggestion on how to go on, and hopefully help with this problem =
is
>> welcome.
>>=20
>=20
> Jens,
> this is just to tell you that I have found the link that still causes
> the circular dependency: an ioc->lock nested into a queue_lock in
> ioc_create_icq. I'll try to come back with a solution proposal.
>=20
Solution implemented and apparently working. If anyone wants to have
a look at or test it, it is in the usual branch [1]. I have found
other similar circular dependencies in the meanwhile, and this
solution covers them too. I'm pasting the message of the last commit
below. Actually, the branch now also contains a fix to another minor
bug (if useful to know, to not waste further time in preparing several
micro fixes, I have just merged some minor fixes into existing
commits). I'm starting to work on cgroups support.
Unnest request-queue and ioc locks from scheduler locks
=20
In some bio-merging functions, the request-queue lock needs to be
taken, to lookup for the bic associated with the process that issued
the bio that may need to be merged. In addition, put_io_context must
be invoked in some other functions, and put_io_context may cause the
lock of the involved ioc to be taken. In both cases, these extra
request-queue or ioc locks are taken, or might be taken, while the
scheduler lock is being held. In this respect, there are other code
paths, in part external to bfq-mq, in which the same locks are taken
(nested) in the opposite order, i.e., it is the scheduler lock to be
taken while the request-queue or the ioc lock is being held. This
leads to circular deadlocks.
=20
This commit addresses this issue by modifying the logic of the above
functions, so as to let the lookup and put_io_context be performed,
and thus the extra locks be taken, outside the critical sections
protected by the scheduler lock.
Thanks,
Paolo
[1] https://github.com/Algodev-github/bfq-mq
> Thanks,
> Paolo
>=20
>> Thanks,
>> Paolo
>>=20
>>> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
>>> index b12f9c87b4c3..546ff8f81ede 100644
>>> --- a/block/blk-ioc.c
>>> +++ b/block/blk-ioc.c
>>> @@ -54,7 +54,7 @@ static void ioc_exit_icq(struct io_cq *icq)
>>> icq->flags |=3D ICQ_EXITED;
>>> }
>>>=20
>>> -/* Release an icq. Called with both ioc and q locked. */
>>> +/* Release an icq. Called with ioc locked. */
>>> static void ioc_destroy_icq(struct io_cq *icq)
>>> {
>>> struct io_context *ioc =3D icq->ioc;
>>> @@ -62,7 +62,6 @@ static void ioc_destroy_icq(struct io_cq *icq)
>>> struct elevator_type *et =3D q->elevator->type;
>>>=20
>>> lockdep_assert_held(&ioc->lock);
>>> - lockdep_assert_held(q->queue_lock);
>>>=20
>>> radix_tree_delete(&ioc->icq_tree, icq->q->id);
>>> hlist_del_init(&icq->ioc_node);
>>> @@ -222,25 +221,34 @@ void exit_io_context(struct task_struct *task)
>>> put_io_context_active(ioc);
>>> }
>>>=20
>>> +static void __ioc_clear_queue(struct list_head *icq_list)
>>> +{
>>> + while (!list_empty(icq_list)) {
>>> + struct io_cq *icq =3D list_entry(icq_list->next,
>>> + struct io_cq, q_node);
>>> + struct io_context *ioc =3D icq->ioc;
>>> +
>>> + spin_lock_irq(&ioc->lock);
>>> + ioc_destroy_icq(icq);
>>> + spin_unlock_irq(&ioc->lock);
>>> + }
>>> +}
>>> +
>>> /**
>>> * ioc_clear_queue - break any ioc association with the specified =
queue
>>> * @q: request_queue being cleared
>>> *
>>> - * Walk @q->icq_list and exit all io_cq's. Must be called with @q =
locked.
>>> + * Walk @q->icq_list and exit all io_cq's.
>>> */
>>> void ioc_clear_queue(struct request_queue *q)
>>> {
>>> - lockdep_assert_held(q->queue_lock);
>>> + LIST_HEAD(icq_list);
>>>=20
>>> - while (!list_empty(&q->icq_list)) {
>>> - struct io_cq *icq =3D list_entry(q->icq_list.next,
>>> - struct io_cq, q_node);
>>> - struct io_context *ioc =3D icq->ioc;
>>> + spin_lock_irq(q->queue_lock);
>>> + list_splice_init(&q->icq_list, &icq_list);
>>> + spin_unlock_irq(q->queue_lock);
>>>=20
>>> - spin_lock(&ioc->lock);
>>> - ioc_destroy_icq(icq);
>>> - spin_unlock(&ioc->lock);
>>> - }
>>> + __ioc_clear_queue(&icq_list);
>>> }
>>>=20
>>> int create_task_io_context(struct task_struct *task, gfp_t =
gfp_flags, int node)
>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>> index 070d81bae1d5..1944aa1cb899 100644
>>> --- a/block/blk-sysfs.c
>>> +++ b/block/blk-sysfs.c
>>> @@ -815,9 +815,7 @@ static void blk_release_queue(struct kobject =
*kobj)
>>> blkcg_exit_queue(q);
>>>=20
>>> if (q->elevator) {
>>> - spin_lock_irq(q->queue_lock);
>>> ioc_clear_queue(q);
>>> - spin_unlock_irq(q->queue_lock);
>>> elevator_exit(q->elevator);
>>> }
>>>=20
>>> diff --git a/block/elevator.c b/block/elevator.c
>>> index a25bdd90b270..aaa1e9836512 100644
>>> --- a/block/elevator.c
>>> +++ b/block/elevator.c
>>> @@ -985,9 +985,7 @@ static int elevator_switch(struct request_queue =
*q, struct elevator_type *new_e)
>>> if (old_registered)
>>> elv_unregister_queue(q);
>>>=20
>>> - spin_lock_irq(q->queue_lock);
>>> ioc_clear_queue(q);
>>> - spin_unlock_irq(q->queue_lock);
>>> }
>>>=20
>>> /* allocate, init and register new elevator */
>>>=20
>>> --=20
>>> Jens Axboe
>=20
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Linus Torvalds @ 2017-02-22 19:04 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <9f6e0c1e-fe62-9898-d074-edc05310cebe@kernel.dk>
On Wed, Feb 22, 2017 at 10:58 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 02/22/2017 11:56 AM, Linus Torvalds wrote:
>
> OK, so here's what I'll do:
>
> 1) We'll kill the default scheduler choices. sq blk-mq will default to
> mq-deadline, mq blk-mq will default to "none" (at least for now, until
> the new scheduler is done).
> 2) The individual schedulers will be y/m/n selectable, just like any
> other driver.
Yes. That makes sense as options. I can (or, perhaps even more
importantly, a distro can) answer those kinds of questions.
Linus
^ permalink raw reply
* [PATCH v3 1/2] blk-mq: use sbq wait queues instead of restart for driver tags
From: Omar Sandoval @ 2017-02-22 18:58 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: kernel-team
From: Omar Sandoval <osandov@fb.com>
Commit 50e1dab86aa2 ("blk-mq-sched: fix starvation for multiple hardware
queues and shared tags") fixed one starvation issue for shared tags.
However, we can still get into a situation where we fail to allocate a
tag because all tags are allocated but we don't have any pending
requests on any hardware queue.
One solution for this would be to restart all queues that share a tag
map, but that really sucks. Ideally, we could just block and wait for a
tag, but that isn't always possible from blk_mq_dispatch_rq_list().
However, we can still use the struct sbitmap_queue wait queues with a
custom callback instead of blocking. This has a few benefits:
1. It avoids iterating over all hardware queues when completing an I/O,
which the current restart code has to do.
2. It benefits from the existing rolling wakeup code.
3. It avoids punting to another thread just to have it block.
Signed-off-by: Omar Sandoval <osandov@fb.com>
Signed-off-by: Jens Axboe <axboe@fb.com>
---
Changed from v2:
- Allow the hardware queue to be run while we're waiting for a tag. This
fixes a hang observed when running xfs/297. We still avoid busy
looping by moving the same check into blk_mq_dispatch_rq_list().
block/blk-mq.c | 64 +++++++++++++++++++++++++++++++++++++++++++-------
include/linux/blk-mq.h | 2 ++
2 files changed, 57 insertions(+), 9 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index b29e7dc7b309..9e6b064e5339 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -904,6 +904,44 @@ static bool reorder_tags_to_front(struct list_head *list)
return first != NULL;
}
+static int blk_mq_dispatch_wake(wait_queue_t *wait, unsigned mode, int flags,
+ void *key)
+{
+ struct blk_mq_hw_ctx *hctx;
+
+ hctx = container_of(wait, struct blk_mq_hw_ctx, dispatch_wait);
+
+ list_del(&wait->task_list);
+ clear_bit_unlock(BLK_MQ_S_TAG_WAITING, &hctx->state);
+ blk_mq_run_hw_queue(hctx, true);
+ return 1;
+}
+
+static bool blk_mq_dispatch_wait_add(struct blk_mq_hw_ctx *hctx)
+{
+ struct sbq_wait_state *ws;
+
+ /*
+ * The TAG_WAITING bit serves as a lock protecting hctx->dispatch_wait.
+ * The thread which wins the race to grab this bit adds the hardware
+ * queue to the wait queue.
+ */
+ if (test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state) ||
+ test_and_set_bit_lock(BLK_MQ_S_TAG_WAITING, &hctx->state))
+ return false;
+
+ init_waitqueue_func_entry(&hctx->dispatch_wait, blk_mq_dispatch_wake);
+ ws = bt_wait_ptr(&hctx->tags->bitmap_tags, hctx);
+
+ /*
+ * As soon as this returns, it's no longer safe to fiddle with
+ * hctx->dispatch_wait, since a completion can wake up the wait queue
+ * and unlock the bit.
+ */
+ add_wait_queue(&ws->wait, &hctx->dispatch_wait);
+ return true;
+}
+
bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list)
{
struct request_queue *q = hctx->queue;
@@ -931,15 +969,22 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list)
continue;
/*
- * We failed getting a driver tag. Mark the queue(s)
- * as needing a restart. Retry getting a tag again,
- * in case the needed IO completed right before we
- * marked the queue as needing a restart.
+ * The initial allocation attempt failed, so we need to
+ * rerun the hardware queue when a tag is freed.
*/
- blk_mq_sched_mark_restart(hctx);
- if (!blk_mq_get_driver_tag(rq, &hctx, false))
+ if (blk_mq_dispatch_wait_add(hctx)) {
+ /*
+ * It's possible that a tag was freed in the
+ * window between the allocation failure and
+ * adding the hardware queue to the wait queue.
+ */
+ if (!blk_mq_get_driver_tag(rq, &hctx, false))
+ break;
+ } else {
break;
+ }
}
+
list_del_init(&rq->queuelist);
bd.rq = rq;
@@ -995,10 +1040,11 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list)
*
* blk_mq_run_hw_queue() already checks the STOPPED bit
*
- * If RESTART is set, then let completion restart the queue
- * instead of potentially looping here.
+ * If RESTART or TAG_WAITING is set, then let completion restart
+ * the queue instead of potentially looping here.
*/
- if (!blk_mq_sched_needs_restart(hctx))
+ if (!blk_mq_sched_needs_restart(hctx) &&
+ !test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state))
blk_mq_run_hw_queue(hctx, true);
}
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 8e4df3d6c8cd..001d30d727c5 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -33,6 +33,7 @@ struct blk_mq_hw_ctx {
struct blk_mq_ctx **ctxs;
unsigned int nr_ctx;
+ wait_queue_t dispatch_wait;
atomic_t wait_index;
struct blk_mq_tags *tags;
@@ -160,6 +161,7 @@ enum {
BLK_MQ_S_STOPPED = 0,
BLK_MQ_S_TAG_ACTIVE = 1,
BLK_MQ_S_SCHED_RESTART = 2,
+ BLK_MQ_S_TAG_WAITING = 3,
BLK_MQ_MAX_DEPTH = 10240,
--
2.11.1
^ permalink raw reply related
* [PATCH v3 2/2] blk-mq-sched: separate mark hctx and queue restart operations
From: Omar Sandoval @ 2017-02-22 18:58 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: kernel-team
In-Reply-To: <ff44cc50b198ea38b4040fd66e8301249d6be69b.1487789478.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
In blk_mq_sched_dispatch_requests(), we call blk_mq_sched_mark_restart()
after we dispatch requests left over on our hardware queue dispatch
list. This is so we'll go back and dispatch requests from the scheduler.
In this case, it's only necessary to restart the hardware queue that we
are running; there's no reason to run other hardware queues just because
we are using shared tags.
So, split out blk_mq_sched_mark_restart() into two operations, one for
just the hardware queue and one for the whole request queue. The core
code only needs the hctx variant, but I/O schedulers will want to use
both.
This also requires adjusting blk_mq_sched_restart_queues() to always
check the queue restart flag, not just when using shared tags.
Signed-off-by: Omar Sandoval <osandov@fb.com>
Signed-off-by: Jens Axboe <axboe@fb.com>
---
block/blk-mq-sched.c | 20 ++++++++------------
block/blk-mq-sched.h | 26 ++++++++++++++++++--------
2 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 9e8d6795a8c1..16df0a5e7046 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -205,7 +205,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
* needing a restart in that case.
*/
if (!list_empty(&rq_list)) {
- blk_mq_sched_mark_restart(hctx);
+ blk_mq_sched_mark_restart_hctx(hctx);
did_work = blk_mq_dispatch_rq_list(hctx, &rq_list);
} else if (!has_sched_dispatch) {
blk_mq_flush_busy_ctxs(hctx, &rq_list);
@@ -331,20 +331,16 @@ static void blk_mq_sched_restart_hctx(struct blk_mq_hw_ctx *hctx)
void blk_mq_sched_restart_queues(struct blk_mq_hw_ctx *hctx)
{
+ struct request_queue *q = hctx->queue;
unsigned int i;
- if (!(hctx->flags & BLK_MQ_F_TAG_SHARED))
+ if (test_bit(QUEUE_FLAG_RESTART, &q->queue_flags)) {
+ if (test_and_clear_bit(QUEUE_FLAG_RESTART, &q->queue_flags)) {
+ queue_for_each_hw_ctx(q, hctx, i)
+ blk_mq_sched_restart_hctx(hctx);
+ }
+ } else {
blk_mq_sched_restart_hctx(hctx);
- else {
- struct request_queue *q = hctx->queue;
-
- if (!test_bit(QUEUE_FLAG_RESTART, &q->queue_flags))
- return;
-
- clear_bit(QUEUE_FLAG_RESTART, &q->queue_flags);
-
- queue_for_each_hw_ctx(q, hctx, i)
- blk_mq_sched_restart_hctx(hctx);
}
}
diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
index 7b5f3b95c78e..a75b16b123f7 100644
--- a/block/blk-mq-sched.h
+++ b/block/blk-mq-sched.h
@@ -122,17 +122,27 @@ static inline bool blk_mq_sched_has_work(struct blk_mq_hw_ctx *hctx)
return false;
}
-static inline void blk_mq_sched_mark_restart(struct blk_mq_hw_ctx *hctx)
+/*
+ * Mark a hardware queue as needing a restart.
+ */
+static inline void blk_mq_sched_mark_restart_hctx(struct blk_mq_hw_ctx *hctx)
{
- if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state)) {
+ if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
set_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
- if (hctx->flags & BLK_MQ_F_TAG_SHARED) {
- struct request_queue *q = hctx->queue;
+}
+
+/*
+ * Mark a hardware queue and the request queue it belongs to as needing a
+ * restart.
+ */
+static inline void blk_mq_sched_mark_restart_queue(struct blk_mq_hw_ctx *hctx)
+{
+ struct request_queue *q = hctx->queue;
- if (!test_bit(QUEUE_FLAG_RESTART, &q->queue_flags))
- set_bit(QUEUE_FLAG_RESTART, &q->queue_flags);
- }
- }
+ if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
+ set_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
+ if (!test_bit(QUEUE_FLAG_RESTART, &q->queue_flags))
+ set_bit(QUEUE_FLAG_RESTART, &q->queue_flags);
}
static inline bool blk_mq_sched_needs_restart(struct blk_mq_hw_ctx *hctx)
--
2.11.1
^ permalink raw reply related
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-22 18:58 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CA+55aFx+FUWAY9zDyULbsy-Xqf9YhK3kOBskurMPSainYbSQcw@mail.gmail.com>
On 02/22/2017 11:56 AM, Linus Torvalds wrote:
> On Wed, Feb 22, 2017 at 10:52 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> It's that simple.
>>
>> No, it's not that simple at all. Fact is, some optimizations make sense
>> for some workloads, and some do not.
>
> Are you even listening?
>
> I'm saying no user can ever give a sane answer to your question. The
> question is insane and wrong.
>
> I already said you can have a dynamic configuration (and maybe even an
> automatic heuristic - like saying that a ramdisk gets NOOP by default,
> real hardware does not).
>
> But asking a user at kernel config time for a default is insane. If
> *you* cannot answer it, then the user sure as hell cannot.
>
> Other configuration questions have problems too, but at least the
> question about "should I support ext4" is something a user (or distro)
> can sanely answer. So your comparisons are pure bullshit.
As per the previous email, this was my proposed solution:
OK, so here's what I'll do:
1) We'll kill the default scheduler choices. sq blk-mq will default to
mq-deadline, mq blk-mq will default to "none" (at least for now, until
the new scheduler is done).
2) The individual schedulers will be y/m/n selectable, just like any
other driver.
Any further settings on that can be done at runtime, through sysfs.
--
Jens Axboe
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Linus Torvalds @ 2017-02-22 18:56 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <2fa1c161-2893-438a-45f9-adbf5284ee71@kernel.dk>
On Wed, Feb 22, 2017 at 10:52 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> It's that simple.
>
> No, it's not that simple at all. Fact is, some optimizations make sense
> for some workloads, and some do not.
Are you even listening?
I'm saying no user can ever give a sane answer to your question. The
question is insane and wrong.
I already said you can have a dynamic configuration (and maybe even an
automatic heuristic - like saying that a ramdisk gets NOOP by default,
real hardware does not).
But asking a user at kernel config time for a default is insane. If
*you* cannot answer it, then the user sure as hell cannot.
Other configuration questions have problems too, but at least the
question about "should I support ext4" is something a user (or distro)
can sanely answer. So your comparisons are pure bullshit.
Linus
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-22 18:52 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CA+55aFzK6j+jM8eHcMirbe2huV1RKEmqwrKTYH9YuXWBYZxsug@mail.gmail.com>
On 02/22/2017 11:45 AM, Linus Torvalds wrote:
> On Wed, Feb 22, 2017 at 10:41 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> The fact is that we have two different sets, until we can yank
>> the old ones. So I can't just ask one question, since the sets
>> aren't identical.
>
> Bullshit.
>
> I'm, saying: rip out the question ENTIRELY. For *both* cases.
>
> If you cannot yourself give a good answer, then there's no f*cking way
> any user can give a good answer. So asking the question is totally and
> utterly pointless.
>
> All it means is that different people will try different (in random
> ways) configurations, and the end result is random crap.
>
> So get rid of those questions. Pick a default, and live with it. And
> if people complain about performance, fix the performance issue.
>
> It's that simple.
No, it's not that simple at all. Fact is, some optimizations make sense
for some workloads, and some do not. CFQ works great for some cases, and
it works poorly for others, even if we try to make heuristics that enable
it to work well for all cases. Some optimizations are costly, that's
fine on certain types of hardware or maybe that's a trade off you want
to make. Or we end up with tons of settings for a single driver, that
does not reduce the configuration matrix at all.
By that logic, why do we have ANY config options outside of what drivers
to build? What should I set HZ at? RCU options? Let's default to ext4,
and kill off xfs? Or btrfs? slab/slob/slub/whatever?
Yes, that's taking the argument a bit more to the extreme, but it's the
same damn thing.
I'm fine with getting rid of the default selections, but we're NOT
going to be able to have just one scheduler for everything. We can
make sane defaults based on the hardware type.
--
Jens Axboe
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Linus Torvalds @ 2017-02-22 18:45 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <316818ab-64ff-89b7-79e4-05a7b3159a38@kernel.dk>
On Wed, Feb 22, 2017 at 10:41 AM, Jens Axboe <axboe@kernel.dk> wrote:
>
> The fact is that we have two different sets, until we can yank
> the old ones. So I can't just ask one question, since the sets
> aren't identical.
Bullshit.
I'm, saying: rip out the question ENTIRELY. For *both* cases.
If you cannot yourself give a good answer, then there's no f*cking way
any user can give a good answer. So asking the question is totally and
utterly pointless.
All it means is that different people will try different (in random
ways) configurations, and the end result is random crap.
So get rid of those questions. Pick a default, and live with it. And
if people complain about performance, fix the performance issue.
It's that simple.
Linus
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-22 18:44 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CA+55aFwXmBsHBPJJLzxnY0H4ib-iro__TdYA6=rpUnoh+BbyGA@mail.gmail.com>
On 02/22/2017 11:42 AM, Linus Torvalds wrote:
> On Wed, Feb 22, 2017 at 10:26 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> And dammit, IF YOU DON'T EVEN KNOW, WHY THE HELL ARE YOU ASKING THE POOR USER?
>
> Basically, I'm pushing back on config options that I can't personally
> even sanely answer.
I got that much, and I don't disagree on that part.
> If it's a config option about "do I have a particular piece of
> hardware", it makes sense. But these new ones were just complete
> garbage.
>
> The whole "default IO scheduler" thing is a disease. We should stop
> making up these shit schedulers and then say "we don't know which one
> works best for you".
>
> All it does is encourage developers to make shortcuts and create crap
> that isn't generically useful, and then blame the user and say "well,
> you should have picked a different scheduler" when they say "this does
> not work well for me".
>
> We have had too many of those kinds of broken choices. And when the
> new Kconfig options get so confusing and so esoteric that I go "Hmm, I
> have no idea if my hardware does a single queue or not", I put my foot
> down.
>
> When the IO scheduler questions were about a generic IO scheduler for
> everything, I can kind of understand them. I think it was still a
> mistake (for the reasons outline above), but at least it was a
> comprehensible question to ask.
>
> But when it gets to "what should I do about a single-queue version of
> a MQ scheduler", the question is no longer even remotely sensible. The
> question should simply NOT EXIST. There is no possible valid reason to
> ask that kind of crap.
OK, so here's what I'll do:
1) We'll kill the default scheduler choices. sq blk-mq will default to
mq-deadline, mq blk-mq will default to "none" (at least for now, until
the new scheduler is done).
2) The individual schedulers will be y/m/n selectable, just like any
other driver.
I hope that works for everyone.
--
Jens Axboe
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Linus Torvalds @ 2017-02-22 18:42 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CA+55aFxFtiAz2nnczcBQLPS6sMwgRnDq-j6U-s1yPS=mN2VgAQ@mail.gmail.com>
On Wed, Feb 22, 2017 at 10:26 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And dammit, IF YOU DON'T EVEN KNOW, WHY THE HELL ARE YOU ASKING THE POOR USER?
Basically, I'm pushing back on config options that I can't personally
even sanely answer.
If it's a config option about "do I have a particular piece of
hardware", it makes sense. But these new ones were just complete
garbage.
The whole "default IO scheduler" thing is a disease. We should stop
making up these shit schedulers and then say "we don't know which one
works best for you".
All it does is encourage developers to make shortcuts and create crap
that isn't generically useful, and then blame the user and say "well,
you should have picked a different scheduler" when they say "this does
not work well for me".
We have had too many of those kinds of broken choices. And when the
new Kconfig options get so confusing and so esoteric that I go "Hmm, I
have no idea if my hardware does a single queue or not", I put my foot
down.
When the IO scheduler questions were about a generic IO scheduler for
everything, I can kind of understand them. I think it was still a
mistake (for the reasons outline above), but at least it was a
comprehensible question to ask.
But when it gets to "what should I do about a single-queue version of
a MQ scheduler", the question is no longer even remotely sensible. The
question should simply NOT EXIST. There is no possible valid reason to
ask that kind of crap.
Linus
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-22 18:41 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CA+55aFxFtiAz2nnczcBQLPS6sMwgRnDq-j6U-s1yPS=mN2VgAQ@mail.gmail.com>
On 02/22/2017 11:26 AM, Linus Torvalds wrote:
> On Wed, Feb 22, 2017 at 10:14 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> What do you mean by "the regular IO scheduler"? These are different
>> schedulers.
>
> Not to the user they aren't.
>
> If the user already answered once about the IO schedulers, we damn
> well shouldn't ask again abotu another small implementaiton detail.
>
> How hard is this to understand? You're asking users stupid things.
The fact is that we have two different sets, until we can yank
the old ones. So I can't just ask one question, since the sets
aren't identical.
This IS confusing to the user, and it's an artifact of the situation
that we have where we are phasing out the old IO path and switching
to blk-mq. I don't want to user to know about blk-mq, I just want
it to be what everything runs on. But until that happens, and it is
happening, we are going to be stuck with that situation.
We have this exposed in other places, too. Like for dm, and for
SCSI. Not a perfect situation, but something that WILL go away
eventually.
> It's not just about the wording. It's a fundamental issue. These
> questions are about internal implementation details. They make no
> sense to a user. They don't even make sense to a kernel developer, for
> chrissake!
>
> Don't make the kconfig mess worse. This "we can't make good defaults
> in the kernel, so ask users about random things that they cannot
> possibly answer" model is not an acceptable model.
There are good defaults! mq single-queue should default to mq-deadline,
and mq multi-queue should default to "none" for now. If you feel that
strongly about it (and I'm guessing you do, judging by the speed
typing and generally annoyed demeanor), then by all means, let's kill
the config entries and I'll just hardwire the defaults. The config
entries were implemented similarly to the old schedulers, and each
scheduler is selectable individually. I'd greatly prefer just
improving the wording so it makes more sense.
> If the new schedulers aren't better than NOOP, they shouldn't exist.
> And if you want people to be able to test, they should be dynamic.
They are dynamic! You can build them as modules, you can switch at
runtime. Just like we have always been able to. I can't make it more
dynamic than that. We're reusing the same internal infrastructure for
that, AND the user visible ABI for checking what is available, and
setting a new one.
> And dammit, IF YOU DON'T EVEN KNOW, WHY THE HELL ARE YOU ASKING THE POOR USER?
BECAUSE IT'S POLICY! Fact of that matter is, if I just default to what
we had before, it'd all be running with none. In a few years time, if
I'm lucky, someone will have shipped udev rules setting this appropriately.
If I ask the question, we'll get testing NOW. People will run with
the default set.
--
Jens Axboe
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Linus Torvalds @ 2017-02-22 18:26 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <6d5d4b2b-5343-09ff-72c3-4e9a7e591d68@kernel.dk>
On Wed, Feb 22, 2017 at 10:14 AM, Jens Axboe <axboe@kernel.dk> wrote:
>
> What do you mean by "the regular IO scheduler"? These are different
> schedulers.
Not to the user they aren't.
If the user already answered once about the IO schedulers, we damn
well shouldn't ask again abotu another small implementaiton detail.
How hard is this to understand? You're asking users stupid things.
It's not just about the wording. It's a fundamental issue. These
questions are about internal implementation details. They make no
sense to a user. They don't even make sense to a kernel developer, for
chrissake!
Don't make the kconfig mess worse. This "we can't make good defaults
in the kernel, so ask users about random things that they cannot
possibly answer" model is not an acceptable model.
If the new schedulers aren't better than NOOP, they shouldn't exist.
And if you want people to be able to test, they should be dynamic.
And dammit, IF YOU DON'T EVEN KNOW, WHY THE HELL ARE YOU ASKING THE POOR USER?
It's really that simple.
Linus
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-22 18:14 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CA+55aFwkdDmO0LPYv8g=VxCXuxF4_JRHdG7-tfF4QSRvQYJFKQ@mail.gmail.com>
On 02/21/2017 04:23 PM, Linus Torvalds wrote:
> On Tue, Feb 21, 2017 at 3:15 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> But under a device managed by blk-mq, that device exposes a number of
>> hardware queues. For older style devices, that number is typically 1
>> (single queue).
>
> ... but why would this ever be different from the normal IO scheduler?
Because we have a different set of schedulers for blk-mq, different
than the legacy path. mq-deadline is a basic port that will work
fine with rotational storage, but it's not going to be a good choice
for NVMe because of scalability issues.
We'll have BFQ on the blk-mq side, catering to the needs of those
folks that currently rely on the richer feature set that CFQ supports.
We've continually been working towards getting rid of the legacy
IO path, and its set of schedulers. So if it's any consolation,
those options will go away in the future.
> IOW, what makes single-queue mq scheduling so special that
>
> (a) it needs its own config option
>
> (b) it is different from just the regular IO scheduler in the first place?
>
> So the whole thing stinks. The fact that it then has an
> incomprehensible config option seems to be just gravy on top of the
> crap.
What do you mean by "the regular IO scheduler"? These are different
schedulers.
As explained above, single-queue mq devices generally DO want mq-deadline.
multi-queue mq devices, we don't have a good choice for them right now,
so we retain the current behavior (that we've had since blk-mq was
introduced in 3.13) of NOT doing any IO scheduling for them. If you
do want scheduling for them, set the option, or configure udev to
make the right choice for you.
I agree the wording isn't great, and we can improve that. But I do
think that the current choices make sense.
>> "none" just means that we don't have a scheduler attached.
>
> .. which makes no sense to me in the first place.
>
> People used to try to convince us that doing IO schedulers was a
> mistake, because modern disk hardware did a better job than we could
> do in software.
>
> Those people were full of crap. The regular IO scheduler used to have
> a "NONE" option too. Maybe it even still has one, but only insane
> people actually use it.
>
> Why is the MQ stuff magically so different that NONE would make sense at all>?
I was never one of those people, and I've always been a strong advocate
for imposing scheduling to keep devices in check. The regular IO scheduler
pool includes "noop", which is probably the one you are thinking of. That
one is a bit different than the new "none" option for blk-mq, in that it
does do insertion sorts and it does do merges. "none" does some merging,
but only where it happens to make sense. There's no insertion sorting.
> And equally importantly: why do we _ask_ people these issues? Is this
> some kind of sick "cover your ass" thing, where you can say "well, I
> asked about it", when inevitably the choice ends up being the wrong
> one?
>
> We have too damn many Kconfig options as-is, I'm trying to push back
> on them. These two options seem fundamentally broken and stupid.
>
> The "we have no good idea, so let's add a Kconfig option" seems like a
> broken excuse for these things existing.
>
> So why ask this question in the first place?
>
> Is there any possible reason why "NONE" is a good option at all? And
> if it is the _only_ option (because no other better choice exists), it
> damn well shouldn't be a kconfig option!
I'm all for NOT asking questions, and not providing tunables. That's
generally how I do write code. See the blk-wbt stuff, for instance, that
basically just has one tunable that's set sanely by default, and we
figure out the rest.
I don't want to regress performance of blk-mq devices by attaching
mq-deadline to them. When we do have a sane scheduler choice, we'll
make that the default. And yes, maybe we can remove the Kconfig option
at that point.
For single queue devices, we could kill the option. But we're expecting
bfq-mq for 4.12, and we'll want to have the option at that point unless
you want to rely solely on runtime setting of the scheduler through
udev or by the sysadmin.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 04/13] block: Move bdi_unregister() to del_gendisk()
From: Bart Van Assche @ 2017-02-22 17:42 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block@vger.kernel.org, Christoph Hellwig,
Dan Williams, Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo,
NeilBrown, Omar Sandoval
In-Reply-To: <20170222085107.GA23312@quack2.suse.cz>
On 02/22/2017 01:20 AM, Jan Kara wrote:=0A=
> On Tue 21-02-17 19:53:29, Bart Van Assche wrote:=0A=
>> This change looks suspicious to me. There are drivers that create a=0A=
>> block layer queue but neither call device_add_disk() nor del_gendisk(),=
=0A=
>> e.g. drivers/scsi/st.c. Although bdi_init() will be called for the=0A=
>> queues created by these drivers, this patch will cause the=0A=
>> bdi_unregister() call to be skipped for these drivers.=0A=
> =0A=
> Well, the thing is that bdi_unregister() is the counterpart to=0A=
> bdi_register(). Unless you call bdi_register(), which happens only in=0A=
> device_add_disk() (and some filesystems which create their private bdis),=
=0A=
> there's no point in calling bdi_unregister(). Counterpart to bdi_init() i=
s=0A=
> bdi_exit() and that gets called always once bdi reference count drops to =
0.=0A=
=0A=
That makes sense to me. Hence:=0A=
=0A=
Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>=0A=
=0A=
^ permalink raw reply
* [PATCH 3/3] block/sed-opal: Propagate original error message to userland.
From: Scott Bauer @ 2017-02-22 17:15 UTC (permalink / raw)
To: linux-block
Cc: keith.busch, linux-nvme, axboe, jonathan.derrick, hch,
Scott Bauer
In-Reply-To: <1487783708-16285-1-git-send-email-scott.bauer@intel.com>
During an error on a comannd, ex: user provides wrong pw to unlock
range, we will gracefully terminate the opal session. We want to
propagate the original error to userland instead of the result of
the session termination, which is almost always a success.
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
---
block/sed-opal.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index 020bf3e..1e18dca 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -396,8 +396,11 @@ static int next(struct opal_dev *dev)
* session. Therefore we shouldn't attempt to terminate
* a session, as one has not yet been created.
*/
- if (state > 1)
- return end_opal_session_error(dev);
+ if (state > 1) {
+ end_opal_session_error(dev);
+ return error;
+ }
+
}
state++;
} while (!error);
--
2.7.4
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related
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