* [PATCH 1/3] block: Introduce blk_rq_accesses_medium()
From: Damien Le Moal @ 2017-02-28 10:25 UTC (permalink / raw)
To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi
Cc: MPT-FusionLinux.pdl, Hannes Reinecke, Christoph Hellwig,
Bart Van Assche
In-Reply-To: <20170228102535.15966-1-damien.lemoal@wdc.com>
From: Bart Van Assche <bart.vanassche@sandisk.com>
A medium access request is defined as an internal regular request that
operates on a whole number of logical blocks of the storage medium.
These include REQ_OP_READ, REQ_OP_WRITE, REQ_OP_FLUSH, REQ_OP_DISCARD,
REQ_OP_SECURE_ERASE, REQ_OP_WRITE_SAME and REQ_OP_WRITE_ZEROES.
Zoned block device requests (REQ_OP_ZONE_REPORT and REQ_OP_ZONE_RESET)
as well as SCSI passthrough and driver private commands are not
considered medium access request.
Reshuffle enum req_opf definitions grouping medium access request up
to REQ_OP_MEDIUM_LAST and introduce the helper function
blk_rq_accesses_medium() to test a request.
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
include/linux/blk_types.h | 17 ++++++++++-------
include/linux/blkdev.h | 12 ++++++++++++
2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index d703acb..6420057 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -151,16 +151,19 @@ enum req_opf {
REQ_OP_FLUSH = 2,
/* discard sectors */
REQ_OP_DISCARD = 3,
- /* get zone information */
- REQ_OP_ZONE_REPORT = 4,
/* securely erase sectors */
- REQ_OP_SECURE_ERASE = 5,
- /* seset a zone write pointer */
- REQ_OP_ZONE_RESET = 6,
+ REQ_OP_SECURE_ERASE = 4,
/* write the same sector many times */
- REQ_OP_WRITE_SAME = 7,
+ REQ_OP_WRITE_SAME = 5,
/* write the zero filled sector many times */
- REQ_OP_WRITE_ZEROES = 8,
+ REQ_OP_WRITE_ZEROES = 6,
+
+ REQ_OP_MEDIUM_LAST = REQ_OP_WRITE_ZEROES,
+
+ /* get zone information */
+ REQ_OP_ZONE_REPORT = 16,
+ /* reset a zone write pointer */
+ REQ_OP_ZONE_RESET = 17,
/* SCSI passthrough using struct scsi_request */
REQ_OP_SCSI_IN = 32,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index aecca0e..7d1ce2d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -248,6 +248,18 @@ static inline bool blk_rq_is_passthrough(struct request *rq)
return blk_rq_is_scsi(rq) || blk_rq_is_private(rq);
}
+/**
+ * blk_rq_accesses_medium - test if a request is a medium access request
+ * @rq: A block layer request.
+ *
+ * A medium access request is a regular internal request that operates on
+ * a whole number of logical blocks of the storage medium.
+ */
+static inline bool blk_rq_accesses_medium(const struct request *rq)
+{
+ return req_op(rq) <= REQ_OP_MEDIUM_LAST;
+}
+
static inline unsigned short req_get_ioprio(struct request *req)
{
return req->ioprio;
--
2.9.3
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:
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.
^ permalink raw reply related
* [PATCH 0/3] Separate zone requests from medium access requests
From: Damien Le Moal @ 2017-02-28 10:25 UTC (permalink / raw)
To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi
Cc: MPT-FusionLinux.pdl, Hannes Reinecke, Christoph Hellwig,
Bart Van Assche, Damien Le Moal
This series introduces blk_rq_accesses_medium(), which is equivalent to
!blk_rq_is_passthrough() minus the zone request operations REQ_OP_ZONE_REPORT
and REQ_OP_ZONE_RESET. This new helper allows avoiding problems due to the
non-standard nature of these commands (report zones does no operate on logical
block size units while reset zone operates on entire zones only).
Using blk_rq_accesses_medium(), patch 2 excludes the zone commands from
request accounting (these commands are not accessing the device medium).
Exclusion from request scheduling is also added.
Finally, patch 3 uses the blk_rq_accesses_medium() helper to fix improperly
unaligned resid values only and only for medium access commands. This
correctly excludes from the resid correction zone requests as well as
passthrough requests.
ALl 3 patches are originally from Bart.
Bart Van Assche (3):
block: Introduce blk_rq_accesses_medium()
block: Separate zone requests from medium access requests
mpt3sas: Do not check resid for non medium access commands
block/blk-core.c | 2 +-
block/blk.h | 2 +-
block/elevator.c | 12 ++++++++----
block/mq-deadline.c | 2 +-
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 4 ++--
include/linux/blk_types.h | 17 ++++++++++-------
include/linux/blkdev.h | 18 +++++++++++++++---
7 files changed, 38 insertions(+), 19 deletions(-)
--
2.9.3
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:
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.
^ permalink raw reply
* Re: [Lsf-pc] [LSF/MM TOPIC] do we really need PG_error at all?
From: Boaz Harrosh @ 2017-02-28 10:12 UTC (permalink / raw)
To: Jeff Layton, NeilBrown, Andreas Dilger
Cc: linux-block, linux-scsi, lsf-pc, Neil Brown, LKML,
James Bottomley, linux-mm, linux-fsdevel
In-Reply-To: <1488244308.7627.5.camel@redhat.com>
On 02/28/2017 03:11 AM, Jeff Layton wrote:
<>
>
> I'll probably have questions about the read side as well, but for now it
> looks like it's mostly used in an ad-hoc way to communicate errors
> across subsystems (block to fs layer, for instance).
If memory does not fail me it used to be checked long time ago in the
read-ahead case. On the buffered read case, the first page is read synchronous
and any error is returned to the caller, but then a read-ahead chunk is
read async all the while the original thread returned to the application.
So any errors are only recorded on the page-bit, since otherwise the uptodate
is off and the IO will be retransmitted. Then the move to read_iter changed
all that I think.
But again this is like 5-6 years ago, and maybe I didn't even understand
very well.
> --
> Jeff Layton <jlayton@redhat.com>
>
I would like a Documentation of all this as well please. Where are the
tests for this?
Thanks
Boaz
^ permalink raw reply
* Re: [PATCH 0/4] blk-mq: cleanup on all kinds of kobjects
From: Peter Zijlstra @ 2017-02-28 9:46 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-kernel, linux-block, Christoph Hellwig,
Omar Sandoval
In-Reply-To: <1487758442-5855-1-git-send-email-tom.leiming@gmail.com>
On Wed, Feb 22, 2017 at 06:13:58PM +0800, Ming Lei wrote:
> This patchset cleans up on kojects of request_queue.mq_kobj,
> sw queue's kobject and hw queue's kobject.
>
> The 1st patch initialized kobject of request_queue and sw queue
> in blk_mq_init_allocated_queue(), so we can avoid to initialize
> these kobjects several times, and this patch fixes one kerne
> warning reported from Omar Sandoval.
>
> The 2nd patch makes lifetime consitent between mq request queue/sw
> queue and their kobjects.
>
> The 3rd patch makes lifetime consitent between hw queue and their
> kobjects.
>
> The last patch is a followup of 3rd patch.
These do indeed make the kobject warnings, and the resulting refcount
warning, go away.
Tested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply
* Re: [blk_mq_register_hctx] 29dee3c03a WARNING: CPU: 0 PID: 5 at lib/refcount.c:114 refcount_inc
From: Peter Zijlstra @ 2017-02-28 8:38 UTC (permalink / raw)
To: Fengguang Wu
Cc: linux-block, Omar Sandoval, Jens Axboe, LKP, Greg Kroah-Hartman
In-Reply-To: <20170228081711.GW6515@twins.programming.kicks-ass.net>
On Tue, Feb 28, 2017 at 09:17:11AM +0100, Peter Zijlstra wrote:
> On Tue, Feb 28, 2017 at 12:11:17PM +0800, Fengguang Wu wrote:
> > Hello,
> >
> > FYI, an old blk_mq bug triggers new warnings on this commit. It's very
> > reproducible and you may try the attached reproduce-* script.
>
> > [ 4.447772] kobject (ffff88001c041f10): tried to init an initialized object, something is seriously wrong.
> > [ 4.453395] CPU: 0 PID: 5 Comm: kworker/u2:0 Not tainted 4.10.0-01216-g29dee3c #2
> > [ 4.455534] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
> > [ 4.458252] Workqueue: events_unbound async_run_entry_fn
> > [ 4.459708] Call Trace:
> > [ 4.460611] dump_stack+0x19/0x27
> > [ 4.461652] kobject_init+0xda/0xf0
> > [ 4.462731] blk_mq_register_dev+0x31/0x150
> > [ 4.463990] blk_register_queue+0x205/0x250
> > [ 4.465217] device_add_disk+0x1ab/0x710
> > [ 4.466384] sd_probe_async+0x11c/0x1e0
> > [ 4.467544] async_run_entry_fn+0xbd/0x220
> > [ 4.468760] process_one_work+0x4a7/0x990
> > [ 4.469938] ? process_one_work+0x348/0x990
> > [ 4.471168] worker_thread+0x342/0x8a0
> > [ 4.472300] ? process_one_work+0x990/0x990
> > [ 4.473540] kthread+0x188/0x190
> > [ 4.474557] ? kthread_create_on_node+0x40/0x40
> > [ 4.475850] ret_from_fork+0x31/0x40
>
> So this was pre-existing wreckage? If so, that needs to be sorted first.
> Because if the kobject stuff is broken, there's no way the refcount
> stuff can begin to work.
Google just found me:
https://lkml.kernel.org/r/1487758442-5855-2-git-send-email-tom.leiming@gmail.com
Let me see if that works.
^ permalink raw reply
* Re: [blk_mq_register_hctx] 29dee3c03a WARNING: CPU: 0 PID: 5 at lib/refcount.c:114 refcount_inc
From: Omar Sandoval @ 2017-02-28 8:52 UTC (permalink / raw)
To: Fengguang Wu
Cc: Peter Zijlstra, linux-block, Omar Sandoval, Jens Axboe, LKP,
Greg Kroah-Hartman
In-Reply-To: <20170228083508.fgcncfhksclb2dei@wfg-t540p.sh.intel.com>
On Tue, Feb 28, 2017 at 04:35:08PM +0800, Fengguang Wu wrote:
> On Tue, Feb 28, 2017 at 09:17:11AM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 28, 2017 at 12:11:17PM +0800, Fengguang Wu wrote:
> > > Hello,
> > >
> > > FYI, an old blk_mq bug triggers new warnings on this commit. It's very
> > > reproducible and you may try the attached reproduce-* script.
> >
> > > [ 4.447772] kobject (ffff88001c041f10): tried to init an initialized object, something is seriously wrong.
> > > [ 4.453395] CPU: 0 PID: 5 Comm: kworker/u2:0 Not tainted 4.10.0-01216-g29dee3c #2
> > > [ 4.455534] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
> > > [ 4.458252] Workqueue: events_unbound async_run_entry_fn
> > > [ 4.459708] Call Trace:
> > > [ 4.460611] dump_stack+0x19/0x27
> > > [ 4.461652] kobject_init+0xda/0xf0
> > > [ 4.462731] blk_mq_register_dev+0x31/0x150
> > > [ 4.463990] blk_register_queue+0x205/0x250
> > > [ 4.465217] device_add_disk+0x1ab/0x710
> > > [ 4.466384] sd_probe_async+0x11c/0x1e0
> > > [ 4.467544] async_run_entry_fn+0xbd/0x220
> > > [ 4.468760] process_one_work+0x4a7/0x990
> > > [ 4.469938] ? process_one_work+0x348/0x990
> > > [ 4.471168] worker_thread+0x342/0x8a0
> > > [ 4.472300] ? process_one_work+0x990/0x990
> > > [ 4.473540] kthread+0x188/0x190
> > > [ 4.474557] ? kthread_create_on_node+0x40/0x40
> > > [ 4.475850] ret_from_fork+0x31/0x40
> >
> > So this was pre-existing wreckage? If so, that needs to be sorted first.
> > Because if the kobject stuff is broken, there's no way the refcount
> > stuff can begin to work.
>
> Yeah it's old bug that should have existed for quite some time.
> It's not quite related to the refcount work, just hoping the new
> warning might serve as new clues to help debugging the blk_mq bug.
>
> Thanks,
> Fengguang
Ming Lei posted a series to fix this here [1]. I haven't gotten around
to testing it, but it'd be great if you could try it, too.
http://marc.info/?l=linux-block&m=148775846217069&w=2
^ permalink raw reply
* Re: [blk_mq_register_hctx] 29dee3c03a WARNING: CPU: 0 PID: 5 at lib/refcount.c:114 refcount_inc
From: Fengguang Wu @ 2017-02-28 8:35 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-block, Omar Sandoval, Jens Axboe, LKP, Greg Kroah-Hartman
In-Reply-To: <20170228081711.GW6515@twins.programming.kicks-ass.net>
On Tue, Feb 28, 2017 at 09:17:11AM +0100, Peter Zijlstra wrote:
>On Tue, Feb 28, 2017 at 12:11:17PM +0800, Fengguang Wu wrote:
>> Hello,
>>
>> FYI, an old blk_mq bug triggers new warnings on this commit. It's very
>> reproducible and you may try the attached reproduce-* script.
>
>> [ 4.447772] kobject (ffff88001c041f10): tried to init an initialized object, something is seriously wrong.
>> [ 4.453395] CPU: 0 PID: 5 Comm: kworker/u2:0 Not tainted 4.10.0-01216-g29dee3c #2
>> [ 4.455534] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
>> [ 4.458252] Workqueue: events_unbound async_run_entry_fn
>> [ 4.459708] Call Trace:
>> [ 4.460611] dump_stack+0x19/0x27
>> [ 4.461652] kobject_init+0xda/0xf0
>> [ 4.462731] blk_mq_register_dev+0x31/0x150
>> [ 4.463990] blk_register_queue+0x205/0x250
>> [ 4.465217] device_add_disk+0x1ab/0x710
>> [ 4.466384] sd_probe_async+0x11c/0x1e0
>> [ 4.467544] async_run_entry_fn+0xbd/0x220
>> [ 4.468760] process_one_work+0x4a7/0x990
>> [ 4.469938] ? process_one_work+0x348/0x990
>> [ 4.471168] worker_thread+0x342/0x8a0
>> [ 4.472300] ? process_one_work+0x990/0x990
>> [ 4.473540] kthread+0x188/0x190
>> [ 4.474557] ? kthread_create_on_node+0x40/0x40
>> [ 4.475850] ret_from_fork+0x31/0x40
>
>So this was pre-existing wreckage? If so, that needs to be sorted first.
>Because if the kobject stuff is broken, there's no way the refcount
>stuff can begin to work.
Yeah it's old bug that should have existed for quite some time.
It's not quite related to the refcount work, just hoping the new
warning might serve as new clues to help debugging the blk_mq bug.
Thanks,
Fengguang
^ permalink raw reply
* Re: blk_integrity_revalidate() clears BDI_CAP_STABLE_WRITES
From: Ilya Dryomov @ 2017-02-28 8:19 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Ceph Development, linux-block, Dan Williams, Christoph Hellwig,
Sagi Grimberg, Mike Snitzer, Jens Axboe, linux-nvme
In-Reply-To: <yq1fuj44geq.fsf@oracle.com>
On Fri, Feb 24, 2017 at 12:49 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Ilya" == Ilya Dryomov <idryomov@gmail.com> writes:
>
> Ilya,
>
> Ilya> Well, blk_integrity_revalidate() doesn't clear the profile, it
> Ilya> just clears the stable pages flag. Whoever calls
> Ilya> blk_integrity_unregister() to clear the profile can also clear the
> Ilya> stable pages flag -- why not let blk_integrity_unregister() clear
> Ilya> the flag like I suggested?
>
> That's what it used to do.
>
> blk_integrity_revalidate() was obviously introduced to overcome some
> problem. Unfortunately, I can't recall what that was and Google isn't
> being particularly helpful. I suspect it was either in the NVDIMM or
> NVMe camps since that's where the churn was.
Adding more people from 25520d55cdb6 -- does anyone remember the story
behind blk_integrity_revalidate()?
>
> I don't have a problem with your patch as long as we're sure there are
> no regressions. I would carry the gendisk check over, though.
I spent quite some time trying to do basic tests on nvme, since it uses
both nop_profile and real integrity profiles. Unfortunately I don't
have access to an nvme card with separate-metadata capability and
upstream qemu driver doesn't support any metadata capabilities at all,
so I turned to https://github.com/OpenChannelSSD/qemu-nvme. It lets
you configure mc and multiple lbafs, but
commit ff646a5841f743fdd9cfef138ac6be1f0ba4fbfb
Author: Keith Busch <keith.busch@intel.com> Thu Oct 23 20:02:44 2014
As part of this, I am removing all meta-data support. It's not well
supported in any immediatly available operating system, so the code
is not well tested.
In the end I got it into a semi-working state by checking out an old
version and dropping some sanity checks. I formatted the namespace in
a number of ways -- bdi/stable_pages_required and integrity/format
behaved as expected ("none", "nop", "T10-DIF-TYPE1-CRC").
Given the above, I'm not sure what the baseline is -- blk_integrity
code isn't invoked for data-only lbafs. Could nvme folks please look
at this? rbd regression caused by integrity revalidate change goes
back to 4.4 and I'd really like to get it fixed. The proposed patch
is attached to my earlier reply on linux-block.
Thanks,
Ilya
^ permalink raw reply
* Re: [blk_mq_register_hctx] 29dee3c03a WARNING: CPU: 0 PID: 5 at lib/refcount.c:114 refcount_inc
From: Peter Zijlstra @ 2017-02-28 8:17 UTC (permalink / raw)
To: Fengguang Wu
Cc: linux-block, Omar Sandoval, Jens Axboe, LKP, Greg Kroah-Hartman
In-Reply-To: <20170228041117.hbqqyczf5bnypqmc@wfg-t540p.sh.intel.com>
On Tue, Feb 28, 2017 at 12:11:17PM +0800, Fengguang Wu wrote:
> Hello,
>
> FYI, an old blk_mq bug triggers new warnings on this commit. It's very
> reproducible and you may try the attached reproduce-* script.
> [ 4.447772] kobject (ffff88001c041f10): tried to init an initialized object, something is seriously wrong.
> [ 4.453395] CPU: 0 PID: 5 Comm: kworker/u2:0 Not tainted 4.10.0-01216-g29dee3c #2
> [ 4.455534] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
> [ 4.458252] Workqueue: events_unbound async_run_entry_fn
> [ 4.459708] Call Trace:
> [ 4.460611] dump_stack+0x19/0x27
> [ 4.461652] kobject_init+0xda/0xf0
> [ 4.462731] blk_mq_register_dev+0x31/0x150
> [ 4.463990] blk_register_queue+0x205/0x250
> [ 4.465217] device_add_disk+0x1ab/0x710
> [ 4.466384] sd_probe_async+0x11c/0x1e0
> [ 4.467544] async_run_entry_fn+0xbd/0x220
> [ 4.468760] process_one_work+0x4a7/0x990
> [ 4.469938] ? process_one_work+0x348/0x990
> [ 4.471168] worker_thread+0x342/0x8a0
> [ 4.472300] ? process_one_work+0x990/0x990
> [ 4.473540] kthread+0x188/0x190
> [ 4.474557] ? kthread_create_on_node+0x40/0x40
> [ 4.475850] ret_from_fork+0x31/0x40
So this was pre-existing wreckage? If so, that needs to be sorted first.
Because if the kobject stuff is broken, there's no way the refcount
stuff can begin to work.
^ permalink raw reply
* Re: [blk_mq_register_hctx] 29dee3c03a WARNING: CPU: 0 PID: 5 at lib/refcount.c:114 refcount_inc
From: Peter Zijlstra @ 2017-02-28 7:48 UTC (permalink / raw)
To: Fengguang Wu; +Cc: linux-block, Omar Sandoval, Jens Axboe, LKP
In-Reply-To: <20170228041117.hbqqyczf5bnypqmc@wfg-t540p.sh.intel.com>
On Tue, Feb 28, 2017 at 12:11:17PM +0800, Fengguang Wu wrote:
> Hello,
>
> FYI, an old blk_mq bug triggers new warnings on this commit. It's very
> reproducible and you may try the attached reproduce-* script.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
>
> commit 29dee3c03abce04cd527878ef5f9e5f91b7b83f4
> Author: Peter Zijlstra <peterz@infradead.org>
> AuthorDate: Fri Feb 10 16:27:52 2017 +0100
> Commit: Ingo Molnar <mingo@kernel.org>
> CommitDate: Fri Feb 24 09:02:10 2017 +0100
>
> locking/refcounts: Out-of-line everything
>
> Linus asked to please make this real C code.
>
> And since size then isn't an issue what so ever anymore, remove the
> debug knob and make all WARN()s unconditional.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I suspect that if you revert, but enable CONFIG_DEBUG_REFCOUNT you'd
have gotten the same splats..
I'll go have a look though, something looks buggered.
^ permalink raw reply
* Re: [PATCHv2 2/2] nvme: Complete all stuck requests
From: Artur Paszkiewicz @ 2017-02-28 7:42 UTC (permalink / raw)
To: Keith Busch, Sagi Grimberg
Cc: linux-block, Jens Axboe, Marc MERLIN, Christoph Hellwig,
linux-nvme
In-Reply-To: <20170227191459.GA1170@localhost.localdomain>
On 02/27/2017 08:15 PM, Keith Busch wrote:
> On Mon, Feb 27, 2017 at 07:27:51PM +0200, Sagi Grimberg wrote:
>> OK, I think we can get it for fabrics too, need to figure out how to
>> handle it there too.
>>
>> Do you have a reproducer?
>
> To repro, I have to run a buffered writer workload then put the system into S3.
>
> This fio job seems to reproduce for me:
>
> fio --name=global --filename=/dev/nvme0n1 --bsrange=4k-128k --rw=randwrite --ioengine=libaio --iodepth=8 --numjobs=8 --name=foobar
>
> I use rtcwake to test suspend/resume:
>
> rtcwake -m mem -s 10
>
> Without the patch we'll get stuck after "Disabling non-boot CPUs ..."
> when blk-mq waits to freeze some entered queues after nvme was disabled.
I'm observing the same thing when hibernating during mdraid resync on
nvme - it hangs in blk_mq_freeze_queue_wait() after "Disabling non-boot
CPUs ...". This patch did not help but when I put nvme_wait_freeze()
right after nvme_start_freeze() it appeared to be working. Maybe the
difference here is that requests are submitted from a non-freezable
kernel thread (md sync_thread)?
Thanks,
Artur
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* [PATCH] blkcg: allocate struct blkcg_gq outside request queue spinlock
From: Tahsin Erdogan @ 2017-02-28 2:49 UTC (permalink / raw)
To: Tejun Heo, Jens Axboe
Cc: linux-block, David Rientjes, linux-kernel, Tahsin Erdogan
blkg_conf_prep() currently calls blkg_lookup_create() while holding
request queue spinlock. This means allocating memory for struct
blkcg_gq has to be made non-blocking. This causes occasional -ENOMEM
failures in call paths like below:
pcpu_alloc+0x68f/0x710
__alloc_percpu_gfp+0xd/0x10
__percpu_counter_init+0x55/0xc0
cfq_pd_alloc+0x3b2/0x4e0
blkg_alloc+0x187/0x230
blkg_create+0x489/0x670
blkg_lookup_create+0x9a/0x230
blkg_conf_prep+0x1fb/0x240
__cfqg_set_weight_device.isra.105+0x5c/0x180
cfq_set_weight_on_dfl+0x69/0xc0
cgroup_file_write+0x39/0x1c0
kernfs_fop_write+0x13f/0x1d0
__vfs_write+0x23/0x120
vfs_write+0xc2/0x1f0
SyS_write+0x44/0xb0
entry_SYSCALL_64_fastpath+0x18/0xad
In the code path above, percpu allocator cannot call vmalloc() due to
queue spinlock.
A failure in this call path gives grief to tools which are trying to
configure io weights. We see occasional failures happen shortly after
reboots even when system is not under any memory pressure. Machines
with a lot of cpus are more vulnerable to this condition.
Do struct blkcg_gq allocations outside the queue spinlock to allow blocking
during memory allocations.
Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Tahsin Erdogan <tahsin@google.com>
---
block/blk-cgroup.c | 108 ++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 82 insertions(+), 26 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 295e98c2c8cc..8ec95f333bc8 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -788,6 +788,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
__acquires(rcu) __acquires(disk->queue->queue_lock)
{
struct gendisk *disk;
+ struct request_queue *q;
struct blkcg_gq *blkg;
struct module *owner;
unsigned int major, minor;
@@ -806,44 +807,99 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
if (!disk)
return -ENODEV;
if (part) {
- owner = disk->fops->owner;
- put_disk(disk);
- module_put(owner);
- return -ENODEV;
+ ret = -ENODEV;
+ goto fail;
+ }
+
+ q = disk->queue;
+
+ if (!blkcg_policy_enabled(q, pol)) {
+ ret = -EOPNOTSUPP;
+ goto fail;
}
rcu_read_lock();
- spin_lock_irq(disk->queue->queue_lock);
+ spin_lock_irq(q->queue_lock);
- if (blkcg_policy_enabled(disk->queue, pol))
- blkg = blkg_lookup_create(blkcg, disk->queue);
- else
- blkg = ERR_PTR(-EOPNOTSUPP);
+ /*
+ * This could be the first entry point of blkcg implementation and
+ * we shouldn't allow anything to go through for a bypassing queue.
+ */
+ if (unlikely(blk_queue_bypass(q))) {
+ ret = blk_queue_dying(q) ? -ENODEV : -EBUSY;
+ goto fail_unlock;
+ }
- if (IS_ERR(blkg)) {
- ret = PTR_ERR(blkg);
+ blkg = __blkg_lookup(blkcg, q, true);
+ if (blkg)
+ goto success;
+
+ /*
+ * Create blkgs walking down from blkcg_root to @blkcg, so that all
+ * non-root blkgs have access to their parents.
+ */
+ while (true) {
+ struct blkcg *pos = blkcg;
+ struct blkcg *parent;
+ struct blkcg_gq *new_blkg;
+
+ parent = blkcg_parent(blkcg);
+ while (parent && !__blkg_lookup(parent, q, false)) {
+ pos = parent;
+ parent = blkcg_parent(parent);
+ }
+
+ spin_unlock_irq(q->queue_lock);
rcu_read_unlock();
- spin_unlock_irq(disk->queue->queue_lock);
- owner = disk->fops->owner;
- put_disk(disk);
- module_put(owner);
- /*
- * If queue was bypassing, we should retry. Do so after a
- * short msleep(). It isn't strictly necessary but queue
- * can be bypassing for some time and it's always nice to
- * avoid busy looping.
- */
- if (ret == -EBUSY) {
- msleep(10);
- ret = restart_syscall();
+
+ new_blkg = blkg_alloc(pos, q, GFP_KERNEL);
+ if (unlikely(!new_blkg)) {
+ ret = -ENOMEM;
+ goto fail;
+ }
+
+ rcu_read_lock();
+ spin_lock_irq(q->queue_lock);
+
+ /* Lookup again since we dropped the lock for blkg_alloc(). */
+ blkg = __blkg_lookup(pos, q, false);
+ if (blkg) {
+ blkg_free(new_blkg);
+ } else {
+ blkg = blkg_create(pos, q, new_blkg);
+ if (unlikely(IS_ERR(blkg))) {
+ ret = PTR_ERR(blkg);
+ goto fail_unlock;
+ }
}
- return ret;
- }
+ if (pos == blkcg)
+ goto success;
+ }
+success:
ctx->disk = disk;
ctx->blkg = blkg;
ctx->body = body;
return 0;
+
+fail_unlock:
+ spin_unlock_irq(q->queue_lock);
+ rcu_read_unlock();
+fail:
+ owner = disk->fops->owner;
+ put_disk(disk);
+ module_put(owner);
+ /*
+ * If queue was bypassing, we should retry. Do so after a
+ * short msleep(). It isn't strictly necessary but queue
+ * can be bypassing for some time and it's always nice to
+ * avoid busy looping.
+ */
+ if (ret == -EBUSY) {
+ msleep(10);
+ ret = restart_syscall();
+ }
+ return ret;
}
EXPORT_SYMBOL_GPL(blkg_conf_prep);
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: [Lsf-pc] [LSF/MM TOPIC] do we really need PG_error at all?
From: Jeff Layton @ 2017-02-28 1:11 UTC (permalink / raw)
To: NeilBrown, Andreas Dilger
Cc: linux-block, linux-scsi, lsf-pc, Neil Brown, LKML,
James Bottomley, linux-mm, linux-fsdevel
In-Reply-To: <87varvp5v1.fsf@notabene.neil.brown.name>
On Tue, 2017-02-28 at 10:32 +1100, NeilBrown wrote:
> On Mon, Feb 27 2017, Andreas Dilger wrote:
>
> >
> > My thought is that PG_error is definitely useful for applications to get
> > correct errors back when doing write()/sync_file_range() so that they know
> > there is an error in the data that _they_ wrote, rather than receiving an
> > error for data that may have been written by another thread, and in turn
> > clearing the error from another thread so it *doesn't* know it had a write
> > error.
>
> It might be useful in that way, but it is not currently used that way.
> Such usage would be a change in visible behaviour.
>
> sync_file_range() calls filemap_fdatawait_range(), which calls
> filemap_check_errors().
> If there have been any errors in the file recently, inside or outside
> the range, the latter will return an error which will propagate up.
>
> >
> > As for stray sync() clearing PG_error from underneath an application, that
> > shouldn't happen since filemap_fdatawait_keep_errors() doesn't clear errors
> > and is used by device flushing code (fdatawait_one_bdev(), wait_sb_inodes()).
>
> filemap_fdatawait_keep_errors() calls __filemap_fdatawait_range() which
> clears PG_error on every page.
> What it doesn't do is call filemap_check_errors(), and so doesn't clear
> AS_ENOSPC or AS_EIO.
>
>
I think it's helpful to get a clear idea of what happens now in the face
of errors and what we expect to happen, and I don't quite have that yet:
--------------------------8<-----------------------------
void page_endio(struct page *page, bool is_write, int err)
{
if (!is_write) {
if (!err) {
SetPageUptodate(page);
} else {
ClearPageUptodate(page);
SetPageError(page);
}
unlock_page(page);
} else {
if (err) {
SetPageError(page);
if (page->mapping)
mapping_set_error(page->mapping, err);
}
end_page_writeback(page);
}
}
--------------------------8<-----------------------------
...not everything uses page_endio, but it's a good place to look since
we have both flavors of error handling in one place.
On a write error, we SetPageError and set the error in the mapping.
What I'm not clear on is:
1) what happens to the page at that point when we get a writeback error?
Does it just remain in-core and is allowed to service reads (assuming
that it was uptodate before)?
Can I redirty it and have it retry the write? Is there standard behavior
for this or is it just up to the whim of the filesystem?
I'll probably have questions about the read side as well, but for now it
looks like it's mostly used in an ad-hoc way to communicate errors
across subsystems (block to fs layer, for instance).
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply
* Re: [LSF/MM TOPIC] do we really need PG_error at all?
From: NeilBrown @ 2017-02-27 23:32 UTC (permalink / raw)
To: Andreas Dilger, Jeff Layton
Cc: James Bottomley, linux-mm, linux-fsdevel, LKML, lsf-pc,
Neil Brown, linux-scsi, linux-block
In-Reply-To: <DC27F5BA-BCCA-41FF-8D41-7BB99AA4DB26@dilger.ca>
[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]
On Mon, Feb 27 2017, Andreas Dilger wrote:
>
> My thought is that PG_error is definitely useful for applications to get
> correct errors back when doing write()/sync_file_range() so that they know
> there is an error in the data that _they_ wrote, rather than receiving an
> error for data that may have been written by another thread, and in turn
> clearing the error from another thread so it *doesn't* know it had a write
> error.
It might be useful in that way, but it is not currently used that way.
Such usage would be a change in visible behaviour.
sync_file_range() calls filemap_fdatawait_range(), which calls
filemap_check_errors().
If there have been any errors in the file recently, inside or outside
the range, the latter will return an error which will propagate up.
>
> As for stray sync() clearing PG_error from underneath an application, that
> shouldn't happen since filemap_fdatawait_keep_errors() doesn't clear errors
> and is used by device flushing code (fdatawait_one_bdev(), wait_sb_inodes()).
filemap_fdatawait_keep_errors() calls __filemap_fdatawait_range() which
clears PG_error on every page.
What it doesn't do is call filemap_check_errors(), and so doesn't clear
AS_ENOSPC or AS_EIO.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [LSF/MM TOPIC] do we really need PG_error at all?
From: Jeff Layton @ 2017-02-27 23:02 UTC (permalink / raw)
To: Andreas Dilger
Cc: NeilBrown, James Bottomley, linux-mm, linux-fsdevel, LKML, lsf-pc,
Neil Brown, linux-scsi, linux-block
In-Reply-To: <DC27F5BA-BCCA-41FF-8D41-7BB99AA4DB26@dilger.ca>
On Mon, 2017-02-27 at 15:51 -0700, Andreas Dilger wrote:
> On Feb 27, 2017, at 8:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >
> > On Mon, 2017-02-27 at 11:27 +1100, NeilBrown wrote:
> > > On Sun, Feb 26 2017, James Bottomley wrote:
> > >
> > > > On Mon, 2017-02-27 at 08:03 +1100, NeilBrown wrote:
> > > > > On Sun, Feb 26 2017, James Bottomley wrote:
> > > > >
> > > > > > [added linux-scsi and linux-block because this is part of our error
> > > > > > handling as well]
> > > > > > On Sun, 2017-02-26 at 09:42 -0500, Jeff Layton wrote:
> > > > > > > Proposing this as a LSF/MM TOPIC, but it may turn out to be me
> > > > > > > just not understanding the semantics here.
> > > > > > >
> > > > > > > As I was looking into -ENOSPC handling in cephfs, I noticed that
> > > > > > > PG_error is only ever tested in one place [1]
> > > > > > > __filemap_fdatawait_range, which does this:
> > > > > > >
> > > > > > > if (TestClearPageError(page))
> > > > > > > ret = -EIO;
> > > > > > >
> > > > > > > This error code will override any AS_* error that was set in the
> > > > > > > mapping. Which makes me wonder...why don't we just set this error
> > > > > > > in the mapping and not bother with a per-page flag? Could we
> > > > > > > potentially free up a page flag by eliminating this?
> > > > > >
> > > > > > Note that currently the AS_* codes are only set for write errors
> > > > > > not for reads and we have no mapping error handling at all for swap
> > > > > > pages, but I'm sure this is fixable.
> > > > >
> > > > > How is a read error different from a failure to set PG_uptodate?
> > > > > Does PG_error suppress retries?
> > > >
> > > > We don't do any retries in the code above the block layer (or at least
> > > > we shouldn't).
> > >
> > > I was wondering about what would/should happen if a read request was
> > > re-issued for some reason. Should the error flag on the page cause an
> > > immediate failure, or should it try again.
> > > If read-ahead sees a read-error on some future page, is it necessary to
> > > record the error so subsequent read-aheads don't notice the page is
> > > missing and repeatedly try to re-load it?
> > > When the application eventually gets to the faulty page, should a read
> > > be tried then, or is the read-ahead failure permanent?
> > >
> > >
> > >
> > > >
> > > > > >
> > > > > > From the I/O layer point of view we take great pains to try to
> > > > > > pinpoint the error exactly to the sector. We reflect this up by
> > > > > > setting the PG_error flag on the page where the error occurred. If
> > > > > > we only set the error on the mapping, we lose that granularity,
> > > > > > because the mapping is mostly at the file level (or VMA level for
> > > > > > anon pages).
> > > > >
> > > > > Are you saying that the IO layer finds the page in the bi_io_vec and
> > > > > explicitly sets PG_error,
> > > >
> > > > I didn't say anything about the mechanism. I think the function you're
> > > > looking for is fs/mpage.c:mpage_end_io(). layers below block indicate
> > > > the position in the request. Block maps the position to bio and the
> > > > bio completion maps to page. So the actual granularity seen in the
> > > > upper layer depends on how the page to bio mapping is done.
> > >
> > > If the block layer is just returning the status at a per-bio level (which
> > > makes perfect sense), then this has nothing directly to do with the
> > > PG_error flag.
> > >
> > > The page cache needs to do something with bi_error, but it isn't
> > > immediately clear that it needs to set PG_error.
> > >
> > > > :q
> > > > > rather than just passing an error indication to bi_end_io ?? That
> > > > > would seem to be wrong as the page may not be in the page cache.
> > > >
> > > > Usually pages in the mpage_end_io path are pinned, I think.
> > > >
> > > > > So I guess I misunderstand you.
> > > > >
> > > > > >
> > > > > > So I think the question for filesystem people from us would be do
> > > > > > you care about this accuracy? If it's OK just to know an error
> > > > > > occurred somewhere in this file, then perhaps we don't need it.
> > > > >
> > > > > I had always assumed that a bio would either succeed or fail, and
> > > > > that no finer granularity could be available.
> > > >
> > > > It does ... but a bio can be as small as a single page.
> > > >
> > > > > I think the question here is: Do filesystems need the pagecache to
> > > > > record which pages have seen an IO error?
> > > >
> > > > It's not just filesystems. The partition code uses PageError() ... the
> > > > metadata code might as well (those are things with no mapping). I'm
> > > > not saying we can't remove PG_error; I am saying it's not going to be
> > > > quite as simple as using the AS_ flags.
> > >
> > > The partition code could use PageUptodate().
> > > mpage_end_io() calls page_endio() on each page, and on read error that
> > > calls:
> > >
> > > ClearPageUptodate(page);
> > > SetPageError(page);
> > >
> > > are both of these necessary?
> > >
> > > fs/buffer.c can use several bios to read a single page.
> > > If any one returns an error, PG_error is set. When all of them have
> > > completed, if PG_error is clear, PG_uptodate is then set.
> > > This is an opportunistic use of PG_error, rather than an essential use.
> > > It could be "fixed", and would need to be fixed if we were to deprecate
> > > use of PG_error for read errors.
> > > There are probably other usages like this.
> > >
> >
> > Ok, I think I get it (somewhat):
> >
> > The tricky part there is how to handle the PageError check in
> > read_dev_sector if you don't use SetPageError in the result handler.
> >
> > If we can count on read_pagecache_sector and read_dax_sector reliably
> > returning an error when the page is considered to be in the cache
> > (PageUpToDate) but had a read error, then that would work. I'm not sure
> > how you'd indicate that without something like PG_error though if you
> > don't want to retry on every attempt.
> >
> > OTOH, if we want to always retry to read in pages that have had read
> > errors when someone requests them, then we can simply not set
> > PageUpToDate when readahead fails.
> >
> > To chip away at the edges of this, what may make sense is to get this
> > flag out of the writeback code as much as we can. When a write fails and
> > SetPageError is called, we should also mark the mapping with an error.
> > Then, we should be able to stop overriding the mapping error with -EIO
> > in that codepath. Maybe call ClearPageError, or maybe leave it alone
> > there?
>
> My thought is that PG_error is definitely useful for applications to get
> correct errors back when doing write()/sync_file_range() so that they know
> there is an error in the data that _they_ wrote, rather than receiving an
> error for data that may have been written by another thread, and in turn
> clearing the error from another thread so it *doesn't* know it had a write
> error.
>
Right, that was my point about sync_file_range. Today I think you can
call sync_file_range and if your range didn't hit errors (none of you
PG_error bits are set), then you might get back 0 (iff the mapping had
no error flagged on it).
The question I have is: is that the semantics that sync_file_range is
supposed to have? It's not clear from the manpage whether errors are
supposed to be that granular or not.
It's also the case that not all writepage implementations set the
PG_error flag. There is a lot of variation here, and so we end up with
different semantics on different filesystems. That's less than ideal.
> As for stray sync() clearing PG_error from underneath an application, that
> shouldn't happen since filemap_fdatawait_keep_errors() doesn't clear errors
> and is used by device flushing code (fdatawait_one_bdev(), wait_sb_inodes()).
>
>
It sure looks like it does to me. Am I missing something?
filemap_fdatawait_keep_errors calls __filemap_fdatawait_range, which
finds pages and waits on their writeback bit to clear. Once it does
that, it calls TestClearPageError and sets the return to -EIO if the bit
was set. That should result in all of the PG_error bits being cleared on
a sync() call, right?
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply
* [PATCH v2] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Omar Sandoval @ 2017-02-27 18:28 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: kernel-team
In-Reply-To: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
blk_mq_alloc_request_hctx() allocates a driver request directly, unlike
its blk_mq_alloc_request() counterpart. It also crashes because it
doesn't update the tags->rqs map.
Fix it by making it allocate a scheduler request.
Reported-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-mq-sched.c | 11 +++++------
block/blk-mq.c | 33 +++++++++++++++------------------
2 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 46ca965fff5c..5697b23412a1 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -110,15 +110,14 @@ struct request *blk_mq_sched_get_request(struct request_queue *q,
struct blk_mq_alloc_data *data)
{
struct elevator_queue *e = q->elevator;
- struct blk_mq_hw_ctx *hctx;
- struct blk_mq_ctx *ctx;
struct request *rq;
blk_queue_enter_live(q);
- ctx = blk_mq_get_ctx(q);
- hctx = blk_mq_map_queue(q, ctx->cpu);
-
- blk_mq_set_alloc_data(data, q, data->flags, ctx, hctx);
+ data->q = q;
+ if (likely(!data->ctx))
+ data->ctx = blk_mq_get_ctx(q);
+ if (likely(!data->hctx))
+ data->hctx = blk_mq_map_queue(q, data->ctx->cpu);
if (e) {
data->flags |= BLK_MQ_REQ_INTERNAL;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4df5fb42c74f..85a7047d8b03 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -273,10 +273,9 @@ EXPORT_SYMBOL(blk_mq_alloc_request);
struct request *blk_mq_alloc_request_hctx(struct request_queue *q, int rw,
unsigned int flags, unsigned int hctx_idx)
{
- struct blk_mq_hw_ctx *hctx;
- struct blk_mq_ctx *ctx;
+ struct blk_mq_alloc_data alloc_data = { .flags = flags };
struct request *rq;
- struct blk_mq_alloc_data alloc_data;
+ unsigned int cpu;
int ret;
/*
@@ -299,25 +298,23 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, int rw,
* Check if the hardware context is actually mapped to anything.
* If not tell the caller that it should skip this queue.
*/
- hctx = q->queue_hw_ctx[hctx_idx];
- if (!blk_mq_hw_queue_mapped(hctx)) {
- ret = -EXDEV;
- goto out_queue_exit;
+ alloc_data.hctx = q->queue_hw_ctx[hctx_idx];
+ if (!blk_mq_hw_queue_mapped(alloc_data.hctx)) {
+ blk_queue_exit(q);
+ return ERR_PTR(-EXDEV);
}
- ctx = __blk_mq_get_ctx(q, cpumask_first(hctx->cpumask));
+ cpu = cpumask_first(alloc_data.hctx->cpumask);
+ alloc_data.ctx = __blk_mq_get_ctx(q, cpu);
- blk_mq_set_alloc_data(&alloc_data, q, flags, ctx, hctx);
- rq = __blk_mq_alloc_request(&alloc_data, rw);
- if (!rq) {
- ret = -EWOULDBLOCK;
- goto out_queue_exit;
- }
-
- return rq;
+ rq = blk_mq_sched_get_request(q, NULL, rw, &alloc_data);
-out_queue_exit:
+ blk_mq_put_ctx(alloc_data.ctx);
blk_queue_exit(q);
- return ERR_PTR(ret);
+
+ if (!rq)
+ return ERR_PTR(-EWOULDBLOCK);
+
+ return rq;
}
EXPORT_SYMBOL_GPL(blk_mq_alloc_request_hctx);
--
2.12.0
^ permalink raw reply related
* Re: [LSF/MM TOPIC] do we really need PG_error at all?
From: Andreas Dilger @ 2017-02-27 22:51 UTC (permalink / raw)
To: Jeff Layton
Cc: NeilBrown, James Bottomley, linux-mm, linux-fsdevel, LKML, lsf-pc,
Neil Brown, linux-scsi, linux-block
In-Reply-To: <1488208047.2876.6.camel@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 7794 bytes --]
On Feb 27, 2017, at 8:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
>
> On Mon, 2017-02-27 at 11:27 +1100, NeilBrown wrote:
>> On Sun, Feb 26 2017, James Bottomley wrote:
>>
>>> On Mon, 2017-02-27 at 08:03 +1100, NeilBrown wrote:
>>>> On Sun, Feb 26 2017, James Bottomley wrote:
>>>>
>>>>> [added linux-scsi and linux-block because this is part of our error
>>>>> handling as well]
>>>>> On Sun, 2017-02-26 at 09:42 -0500, Jeff Layton wrote:
>>>>>> Proposing this as a LSF/MM TOPIC, but it may turn out to be me
>>>>>> just not understanding the semantics here.
>>>>>>
>>>>>> As I was looking into -ENOSPC handling in cephfs, I noticed that
>>>>>> PG_error is only ever tested in one place [1]
>>>>>> __filemap_fdatawait_range, which does this:
>>>>>>
>>>>>> if (TestClearPageError(page))
>>>>>> ret = -EIO;
>>>>>>
>>>>>> This error code will override any AS_* error that was set in the
>>>>>> mapping. Which makes me wonder...why don't we just set this error
>>>>>> in the mapping and not bother with a per-page flag? Could we
>>>>>> potentially free up a page flag by eliminating this?
>>>>>
>>>>> Note that currently the AS_* codes are only set for write errors
>>>>> not for reads and we have no mapping error handling at all for swap
>>>>> pages, but I'm sure this is fixable.
>>>>
>>>> How is a read error different from a failure to set PG_uptodate?
>>>> Does PG_error suppress retries?
>>>
>>> We don't do any retries in the code above the block layer (or at least
>>> we shouldn't).
>>
>> I was wondering about what would/should happen if a read request was
>> re-issued for some reason. Should the error flag on the page cause an
>> immediate failure, or should it try again.
>> If read-ahead sees a read-error on some future page, is it necessary to
>> record the error so subsequent read-aheads don't notice the page is
>> missing and repeatedly try to re-load it?
>> When the application eventually gets to the faulty page, should a read
>> be tried then, or is the read-ahead failure permanent?
>>
>>
>>
>>>
>>>>>
>>>>> From the I/O layer point of view we take great pains to try to
>>>>> pinpoint the error exactly to the sector. We reflect this up by
>>>>> setting the PG_error flag on the page where the error occurred. If
>>>>> we only set the error on the mapping, we lose that granularity,
>>>>> because the mapping is mostly at the file level (or VMA level for
>>>>> anon pages).
>>>>
>>>> Are you saying that the IO layer finds the page in the bi_io_vec and
>>>> explicitly sets PG_error,
>>>
>>> I didn't say anything about the mechanism. I think the function you're
>>> looking for is fs/mpage.c:mpage_end_io(). layers below block indicate
>>> the position in the request. Block maps the position to bio and the
>>> bio completion maps to page. So the actual granularity seen in the
>>> upper layer depends on how the page to bio mapping is done.
>>
>> If the block layer is just returning the status at a per-bio level (which
>> makes perfect sense), then this has nothing directly to do with the
>> PG_error flag.
>>
>> The page cache needs to do something with bi_error, but it isn't
>> immediately clear that it needs to set PG_error.
>>
>>> :q
>>>> rather than just passing an error indication to bi_end_io ?? That
>>>> would seem to be wrong as the page may not be in the page cache.
>>>
>>> Usually pages in the mpage_end_io path are pinned, I think.
>>>
>>>> So I guess I misunderstand you.
>>>>
>>>>>
>>>>> So I think the question for filesystem people from us would be do
>>>>> you care about this accuracy? If it's OK just to know an error
>>>>> occurred somewhere in this file, then perhaps we don't need it.
>>>>
>>>> I had always assumed that a bio would either succeed or fail, and
>>>> that no finer granularity could be available.
>>>
>>> It does ... but a bio can be as small as a single page.
>>>
>>>> I think the question here is: Do filesystems need the pagecache to
>>>> record which pages have seen an IO error?
>>>
>>> It's not just filesystems. The partition code uses PageError() ... the
>>> metadata code might as well (those are things with no mapping). I'm
>>> not saying we can't remove PG_error; I am saying it's not going to be
>>> quite as simple as using the AS_ flags.
>>
>> The partition code could use PageUptodate().
>> mpage_end_io() calls page_endio() on each page, and on read error that
>> calls:
>>
>> ClearPageUptodate(page);
>> SetPageError(page);
>>
>> are both of these necessary?
>>
>
>> fs/buffer.c can use several bios to read a single page.
>> If any one returns an error, PG_error is set. When all of them have
>> completed, if PG_error is clear, PG_uptodate is then set.
>> This is an opportunistic use of PG_error, rather than an essential use.
>> It could be "fixed", and would need to be fixed if we were to deprecate
>> use of PG_error for read errors.
>> There are probably other usages like this.
>>
>
> Ok, I think I get it (somewhat):
>
> The tricky part there is how to handle the PageError check in
> read_dev_sector if you don't use SetPageError in the result handler.
>
> If we can count on read_pagecache_sector and read_dax_sector reliably
> returning an error when the page is considered to be in the cache
> (PageUpToDate) but had a read error, then that would work. I'm not sure
> how you'd indicate that without something like PG_error though if you
> don't want to retry on every attempt.
>
> OTOH, if we want to always retry to read in pages that have had read
> errors when someone requests them, then we can simply not set
> PageUpToDate when readahead fails.
>
> To chip away at the edges of this, what may make sense is to get this
> flag out of the writeback code as much as we can. When a write fails and
> SetPageError is called, we should also mark the mapping with an error.
> Then, we should be able to stop overriding the mapping error with -EIO
> in that codepath. Maybe call ClearPageError, or maybe leave it alone
> there?
My thought is that PG_error is definitely useful for applications to get
correct errors back when doing write()/sync_file_range() so that they know
there is an error in the data that _they_ wrote, rather than receiving an
error for data that may have been written by another thread, and in turn
clearing the error from another thread so it *doesn't* know it had a write
error.
As for stray sync() clearing PG_error from underneath an application, that
shouldn't happen since filemap_fdatawait_keep_errors() doesn't clear errors
and is used by device flushing code (fdatawait_one_bdev(), wait_sb_inodes()).
Cheers, Andreas
>>>
>>> James
>>>
>>>> I think that for write errors, there is no value in recording
>>>> block-oriented error status - only file-oriented status.
>>>> For read errors, it might if help to avoid indefinite read retries,
>>>> but I don't know the code well enough to be sure if this is an issue.
>>>>
>>>> NeilBrown
>>>>
>>>>
>>>>>
>>>>> James
>>>>>
>>>>>> The main argument I could see for keeping it is that removing it
>>>>>> might subtly change the behavior of sync_file_range if you have
>>>>>> tasks syncing different ranges in a file concurrently. I'm not
>>>>>> sure if that would break any guarantees though.
>>>>>>
>>>>>> Even if we do need it, I think we might need some cleanup here
>>>>>> anyway. A lot of readpage operations end up setting that flag
>>>>>> when they hit an error. Isn't it wrong to return an error on
>>>>>> fsync, just because we had a read error somewhere in the file in
>>>>>> a range that was never dirtied?
>>>>>>
>>>>>> --
>>>>>> [1]: there is another place in f2fs, but it's more or less
>>>>>> equivalent to the call site in __filemap_fdatawait_range.
>>>>>>
>
> --
> Jeff Layton <jlayton@redhat.com>
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Jens Axboe @ 2017-02-27 18:44 UTC (permalink / raw)
To: Sagi Grimberg, Omar Sandoval; +Cc: linux-block, kernel-team
In-Reply-To: <719e3d15-73e2-0b11-e205-e733b882c2ae@grimberg.me>
On 02/27/2017 11:41 AM, Sagi Grimberg wrote:
>
>>>>> blk_mq_alloc_request_hctx() allocates a driver request directly, unlike
>>>>> its blk_mq_alloc_request() counterpart. It also crashes because it
>>>>> doesn't update the tags->rqs map.
>>>>>
>>>>> Fix it by making it allocate a scheduler request.
>>>>
>>>> All three look good to me. Would you mind respinning #1, it doesn't
>>>> apply on top of the reserved tag patch.
>>>
>>> Yup, I had those based on Sagi's original patches for some reason. I
>>> fat-fingered send-email, sent as a reply to the original patch 1 instead
>>> of this email.
>>
>> I got it, applied all 3, thanks Omar!
>
> FWIW, you can add my:
>
> Tested-by: Sagi Grimberg <sagi@grmberg.me>
Done, I fixed up your typo, though :-)
> Although I didn't try to trigger reserved vs. normal tag
> competition which will never ever happen for nvmf btw, but
> I assume it can happen in drivers who use reserved for error
> handling.
As long as we verify they work independently, there should not
be a need to check that edge case separately. We should have
identical behavior there now to previously.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Jens Axboe @ 2017-02-27 18:34 UTC (permalink / raw)
To: Omar Sandoval; +Cc: Sagi Grimberg, linux-block, kernel-team
In-Reply-To: <20170227183021.GG10715@vader>
On 02/27/2017 11:30 AM, Omar Sandoval wrote:
> On Mon, Feb 27, 2017 at 11:07:27AM -0700, Jens Axboe wrote:
>> On 02/27/2017 10:47 AM, Omar Sandoval wrote:
>>> From: Omar Sandoval <osandov@fb.com>
>>>
>>> blk_mq_alloc_request_hctx() allocates a driver request directly, unlike
>>> its blk_mq_alloc_request() counterpart. It also crashes because it
>>> doesn't update the tags->rqs map.
>>>
>>> Fix it by making it allocate a scheduler request.
>>
>> All three look good to me. Would you mind respinning #1, it doesn't
>> apply on top of the reserved tag patch.
>
> Yup, I had those based on Sagi's original patches for some reason. I
> fat-fingered send-email, sent as a reply to the original patch 1 instead
> of this email.
I got it, applied all 3, thanks Omar!
--
Jens Axboe
^ permalink raw reply
* Re: [PATCHv2 2/2] nvme: Complete all stuck requests
From: Keith Busch @ 2017-02-27 19:15 UTC (permalink / raw)
To: Sagi Grimberg
Cc: linux-block, Jens Axboe, Marc MERLIN, Christoph Hellwig,
linux-nvme
In-Reply-To: <4c952c67-9871-249d-86b0-5f81acf00571@grimberg.me>
On Mon, Feb 27, 2017 at 07:27:51PM +0200, Sagi Grimberg wrote:
> OK, I think we can get it for fabrics too, need to figure out how to
> handle it there too.
>
> Do you have a reproducer?
To repro, I have to run a buffered writer workload then put the system into S3.
This fio job seems to reproduce for me:
fio --name=global --filename=/dev/nvme0n1 --bsrange=4k-128k --rw=randwrite --ioengine=libaio --iodepth=8 --numjobs=8 --name=foobar
I use rtcwake to test suspend/resume:
rtcwake -m mem -s 10
Without the patch we'll get stuck after "Disabling non-boot CPUs ..."
when blk-mq waits to freeze some entered queues after nvme was disabled.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: automatic IRQ affinity for virtio V3
From: Michael S. Tsirkin @ 2017-02-27 18:49 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Christoph Hellwig, jasowang, axboe, pbonzini, virtualization,
linux-block, linux-kernel
In-Reply-To: <20170227204456-mutt-send-email-mst@kernel.org>
On Mon, Feb 27, 2017 at 08:45:25PM +0200, Michael S. Tsirkin wrote:
> On Mon, Feb 27, 2017 at 09:48:32AM +0100, Christoph Hellwig wrote:
> > On Thu, Feb 09, 2017 at 06:01:57PM +0200, Michael S. Tsirkin wrote:
> > > > Any chance to get this in for 4.11 after I got reviews from Jason
> > > > for most of the patches?
> > >
> > > Absolutely, I intend to merge it.
> >
> > So, what is the plan for virtio this merge window? No changes seem
> > to have made it into linux-next before the merge window, never mind
> > a week into it.
>
> Sorry I've been a bit busy. Any chance you could rebase?
> This conflicts with your patch removing vq info.
Actually it doesn't, my bad. Applied now, thanks!
^ permalink raw reply
* Re: automatic IRQ affinity for virtio V3
From: Michael S. Tsirkin @ 2017-02-27 18:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Christoph Hellwig, jasowang, axboe, pbonzini, virtualization,
linux-block, linux-kernel
In-Reply-To: <20170227084832.GA22373@lst.de>
On Mon, Feb 27, 2017 at 09:48:32AM +0100, Christoph Hellwig wrote:
> On Thu, Feb 09, 2017 at 06:01:57PM +0200, Michael S. Tsirkin wrote:
> > > Any chance to get this in for 4.11 after I got reviews from Jason
> > > for most of the patches?
> >
> > Absolutely, I intend to merge it.
>
> So, what is the plan for virtio this merge window? No changes seem
> to have made it into linux-next before the merge window, never mind
> a week into it.
Sorry I've been a bit busy. Any chance you could rebase?
This conflicts with your patch removing vq info.
^ permalink raw reply
* Re: [PATCH 1/2] lightnvm: add generic ocssd detection
From: Keith Busch @ 2017-02-27 18:50 UTC (permalink / raw)
To: Sagi Grimberg
Cc: Christoph Hellwig, Matias Bjørling, linux-block,
linux-kernel, linux-nvme
In-Reply-To: <108882ac-9f55-e4f5-c04e-244848139db8@grimberg.me>
On Mon, Feb 27, 2017 at 08:35:06PM +0200, Sagi Grimberg wrote:
> > On Sat, Feb 25, 2017 at 08:16:04PM +0100, Matias Bj�rling wrote:
> > > On 02/25/2017 07:21 PM, Christoph Hellwig wrote:
> > > > No way in hell. vs is vendor specific and we absolutely can't overload
> > > > it with any sort of meaning. Get OCSSD support properly standardized and
> > > > add a class code for it. Until then it's individual PCI IDs.
> > > >
> > >
> > > You are right, that is the right way to go, and we are working on it. In the
> > > meantime, there are a couple of reasons I want to do a pragmatic solution:
> >
> > Reasonable reaosons, but that's just not how standard interfaces work.
> > Either you standardize the behaviour and have a standardized trigger
> > for it, or it is vendor specific and needs to be keyed off a specific
> > vendor/device identification.
>
> I agree, I don't see how we're allowed to use vs for that.
>From personal experience, some OEMs will put whatever they want in the
VS region for their rebranded device, making it an unreliable place to
check for a capability.
^ permalink raw reply
* Re: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Sagi Grimberg @ 2017-02-27 18:41 UTC (permalink / raw)
To: Jens Axboe, Omar Sandoval; +Cc: linux-block, kernel-team
In-Reply-To: <5d9bc14d-9a90-8915-f538-f61452420677@fb.com>
>>>> blk_mq_alloc_request_hctx() allocates a driver request directly, unlike
>>>> its blk_mq_alloc_request() counterpart. It also crashes because it
>>>> doesn't update the tags->rqs map.
>>>>
>>>> Fix it by making it allocate a scheduler request.
>>>
>>> All three look good to me. Would you mind respinning #1, it doesn't
>>> apply on top of the reserved tag patch.
>>
>> Yup, I had those based on Sagi's original patches for some reason. I
>> fat-fingered send-email, sent as a reply to the original patch 1 instead
>> of this email.
>
> I got it, applied all 3, thanks Omar!
FWIW, you can add my:
Tested-by: Sagi Grimberg <sagi@grmberg.me>
Although I didn't try to trigger reserved vs. normal tag
competition which will never ever happen for nvmf btw, but
I assume it can happen in drivers who use reserved for error
handling.
^ permalink raw reply
* Re: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Omar Sandoval @ 2017-02-27 18:30 UTC (permalink / raw)
To: Jens Axboe; +Cc: Sagi Grimberg, linux-block, kernel-team
In-Reply-To: <6137d2c6-816e-83b2-103c-cdf0840f49fd@fb.com>
On Mon, Feb 27, 2017 at 11:07:27AM -0700, Jens Axboe wrote:
> On 02/27/2017 10:47 AM, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> >
> > blk_mq_alloc_request_hctx() allocates a driver request directly, unlike
> > its blk_mq_alloc_request() counterpart. It also crashes because it
> > doesn't update the tags->rqs map.
> >
> > Fix it by making it allocate a scheduler request.
>
> All three look good to me. Would you mind respinning #1, it doesn't
> apply on top of the reserved tag patch.
Yup, I had those based on Sagi's original patches for some reason. I
fat-fingered send-email, sent as a reply to the original patch 1 instead
of this email.
^ 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