* [PATCH 2/2] blk-mq: make sure to back-assign the request to rq_map in blk_mq_alloc_request_hctx
From: Sagi Grimberg @ 2017-02-27 15:36 UTC (permalink / raw)
To: Jens Axboe, linux-block, linux-nvme
In-Reply-To: <1488209781-1084-1-git-send-email-sagi@grimberg.me>
Otherwise we won't be able to retrieve the request from
the tag.
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
block/blk-mq.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index d84c66fb37b7..9611cd9920e9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -312,6 +312,7 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, int rw,
ret = -EWOULDBLOCK;
goto out_queue_exit;
}
+ alloc_data.hctx->tags->rqs[rq->tag] = rq;
return rq;
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Sagi Grimberg @ 2017-02-27 15:36 UTC (permalink / raw)
To: Jens Axboe, linux-block, linux-nvme
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
block/blk-mq-sched.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 98c7b061781e..46ca965fff5c 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -454,7 +454,8 @@ int blk_mq_sched_setup(struct request_queue *q)
*/
ret = 0;
queue_for_each_hw_ctx(q, hctx, i) {
- hctx->sched_tags = blk_mq_alloc_rq_map(set, i, q->nr_requests, 0);
+ hctx->sched_tags = blk_mq_alloc_rq_map(set, i,
+ q->nr_requests, set->reserved_tags);
if (!hctx->sched_tags) {
ret = -ENOMEM;
break;
--
2.7.4
^ permalink raw reply related
* Re: nvmf regression with mq-deadline
From: Jens Axboe @ 2017-02-27 15:33 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: linux-block@vger.kernel.org, linux-nvme
In-Reply-To: <0f06d732-fe32-bca0-8a4d-299a6cce7a6e@grimberg.me>
On Mon, Feb 27 2017, Sagi Grimberg wrote:
>
> >Now I'm getting a NULL deref with nvme-rdma [1].
> >
> >For some reason blk_mq_tag_to_rq() is returning NULL on
> >tag 0x0 which is io queue connect.
> >
> >I'll try to see where this is coming from.
> >This does not happen with loop though...
>
> That's because the loop driver does not rely on the
> cqe.command_id to resolve the submitted request (I'll
> fix that).
>
> Looks like blk_mq_alloc_request_hctx was overlooked when
> the back assignment of the request to the rq_map...
>
> This patch solves the issue for fabrics:
> --
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index d84c66fb37b7..9611cd9920e9 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -312,6 +312,7 @@ struct request *blk_mq_alloc_request_hctx(struct
> request_queue *q, int rw,
> ret = -EWOULDBLOCK;
> goto out_queue_exit;
> }
> + alloc_data.hctx->tags->rqs[rq->tag] = rq;
>
> return rq;
> --
>
> If its agreed with everyone I'll send a proper patch
> for this and the blk_mq_sched_setup fix?
Thanks Sagi, yes please send a proper patch for those two conditions!
--
Jens Axboe
^ permalink raw reply
* Re: [LSF/MM TOPIC] do we really need PG_error at all?
From: Jeff Layton @ 2017-02-27 15:07 UTC (permalink / raw)
To: NeilBrown, James Bottomley, linux-mm, linux-fsdevel, LKML
Cc: lsf-pc, Neil Brown, linux-scsi, linux-block
In-Reply-To: <874lzgqy06.fsf@notabene.neil.brown.name>
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?
> >
> > 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>
^ permalink raw reply
* Re: [PATCHv2 2/2] nvme: Complete all stuck requests
From: Keith Busch @ 2017-02-27 15:01 UTC (permalink / raw)
To: Sagi Grimberg
Cc: linux-block, Jens Axboe, Marc MERLIN, Christoph Hellwig,
linux-nvme
In-Reply-To: <b66a614e-140e-86e9-56bf-d3eafb750aa5@grimberg.me>
On Mon, Feb 27, 2017 at 03:46:09PM +0200, Sagi Grimberg wrote:
> On 24/02/17 02:36, Keith Busch wrote:
> > If the block layer has entered requests and gets a CPU hot plug event
> > prior to the resume event, it will wait for those requests to exit. If
> > the nvme driver is shutting down, it will not start the queues back up,
> > preventing forward progress.
> >
> > To fix that, this patch freezes the request queues when the driver intends
> > to shut down the controller so that no new requests may enter. After the
> > controller has been disabled, the queues will be restarted to force all
> > entered requests to end in failure so that blk-mq's hot cpu notifier may
> > progress. To ensure the queue usage count is 0 on a shutdown, the driver
> > waits for freeze to complete before completing the controller shutdown.
>
> Keith, can you explain (again) for me why is the freeze_wait must happen
> after the controller has been disabled, instead of starting the queues
> and waiting right after freeze start?
Yeah, the driver needs to make forward progress even if the controller
isn't functioning. If we do the freeze wait before disabling the
controller, there's no way to reclaim missing completions. If the
controller is working perfectly, it'd be okay, but the driver would be
stuck if there's a problem.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: nvmf regression with mq-deadline
From: Sagi Grimberg @ 2017-02-27 14:39 UTC (permalink / raw)
To: Jens Axboe, linux-block@vger.kernel.org, linux-nvme
In-Reply-To: <34fb43d6-37e7-feb3-73c6-63140993ba7c@grimberg.me>
> Now I'm getting a NULL deref with nvme-rdma [1].
>
> For some reason blk_mq_tag_to_rq() is returning NULL on
> tag 0x0 which is io queue connect.
>
> I'll try to see where this is coming from.
> This does not happen with loop though...
That's because the loop driver does not rely on the
cqe.command_id to resolve the submitted request (I'll
fix that).
Looks like blk_mq_alloc_request_hctx was overlooked when
the back assignment of the request to the rq_map...
This patch solves the issue for fabrics:
--
diff --git a/block/blk-mq.c b/block/blk-mq.c
index d84c66fb37b7..9611cd9920e9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -312,6 +312,7 @@ struct request *blk_mq_alloc_request_hctx(struct
request_queue *q, int rw,
ret = -EWOULDBLOCK;
goto out_queue_exit;
}
+ alloc_data.hctx->tags->rqs[rq->tag] = rq;
return rq;
--
If its agreed with everyone I'll send a proper patch
for this and the blk_mq_sched_setup fix?
^ permalink raw reply related
* Re: [PATCHv2 2/2] nvme: Complete all stuck requests
From: Sagi Grimberg @ 2017-02-27 13:46 UTC (permalink / raw)
To: Keith Busch, linux-block, linux-nvme, Jens Axboe,
Christoph Hellwig
Cc: Marc MERLIN
In-Reply-To: <1487896561-10454-2-git-send-email-keith.busch@intel.com>
On 24/02/17 02:36, Keith Busch wrote:
> If the block layer has entered requests and gets a CPU hot plug event
> prior to the resume event, it will wait for those requests to exit. If
> the nvme driver is shutting down, it will not start the queues back up,
> preventing forward progress.
>
> To fix that, this patch freezes the request queues when the driver intends
> to shut down the controller so that no new requests may enter. After the
> controller has been disabled, the queues will be restarted to force all
> entered requests to end in failure so that blk-mq's hot cpu notifier may
> progress. To ensure the queue usage count is 0 on a shutdown, the driver
> waits for freeze to complete before completing the controller shutdown.
Keith, can you explain (again) for me why is the freeze_wait must happen
after the controller has been disabled, instead of starting the queues
and waiting right after freeze start?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCHv2 1/2] blk-mq: Export blk_mq_freeze_queue_wait
From: Sagi Grimberg @ 2017-02-27 13:38 UTC (permalink / raw)
To: Keith Busch, linux-block, linux-nvme, Jens Axboe,
Christoph Hellwig
Cc: Marc MERLIN
In-Reply-To: <1487896561-10454-1-git-send-email-keith.busch@intel.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: nvmf regression with mq-deadline
From: Sagi Grimberg @ 2017-02-27 13:31 UTC (permalink / raw)
To: Jens Axboe, linux-block@vger.kernel.org, linux-nvme
In-Reply-To: <8384a5c8-c8e6-4e46-65d6-208b802f6957@grimberg.me>
> Hey Jens,
>
> I'm getting a regression in nvme-rdma/nvme-loop with for-linus [1]
> with a small script to trigger it.
>
> The reason seems to be that the sched_tags does not take into account
> the tag_set reserved tags.
>
> This solves it for me, any objections on this?
> --
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 98c7b061781e..46ca965fff5c 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -454,7 +454,8 @@ int blk_mq_sched_setup(struct request_queue *q)
> */
> ret = 0;
> queue_for_each_hw_ctx(q, hctx, i) {
> - hctx->sched_tags = blk_mq_alloc_rq_map(set, i,
> q->nr_requests, 0);
> + hctx->sched_tags = blk_mq_alloc_rq_map(set, i,
> + q->nr_requests, set->reserved_tags);
> if (!hctx->sched_tags) {
> ret = -ENOMEM;
> break;
> --
Now I'm getting a NULL deref with nvme-rdma [1].
For some reason blk_mq_tag_to_rq() is returning NULL on
tag 0x0 which is io queue connect.
I'll try to see where this is coming from.
This does not happen with loop though...
--
[ 30.431889] nvme nvme0: creating 2 I/O queues.
[ 30.465458] nvme nvme0: tag 0x0 on QP 0x84 not found
[ 36.060168] BUG: unable to handle kernel NULL pointer dereference at
0000000000000030
[ 36.063277] IP: bt_iter+0x31/0x50
[ 36.064088] PGD 0
[ 36.064088] Oops: 0000 [#1] SMP
[ 36.064088] Modules linked in: nvme_rdma nvme_fabrics nvme_core
mlx5_ib ppdev kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul
ghash_clmulni_intel pcbc aesni_intel aes_x86_64 crypto_simd glue_helper
cryptd i2c_piix4 joydev input_leds serio_raw parport_pc parport mac_hid
ib_iser rdma_cm iw_cm ib_cm ib_core configfs iscsi_tcp libiscsi_tcp
libiscsi sunrpc scsi_transport_iscsi autofs4 cirrus ttm drm_kms_helper
syscopyarea sysfillrect mlx5_core sysimgblt fb_sys_fops psmouse drm
floppy ptp pata_acpi pps_core
[ 36.064088] CPU: 0 PID: 186 Comm: kworker/0:1H Not tainted 4.10.0+ #115
[ 36.064088] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 36.064088] Workqueue: kblockd blk_mq_timeout_work
[ 36.064088] task: ffff95f6393a0080 task.stack: ffffb826803ac000
[ 36.064088] RIP: 0010:bt_iter+0x31/0x50
[ 36.064088] RSP: 0018:ffffb826803afda0 EFLAGS: 00010202
[ 36.064088] RAX: ffffb826803afdd0 RBX: ffff95f63c036800 RCX:
0000000000000001
[ 36.064088] RDX: ffff95f635ff0798 RSI: 0000000000000000 RDI:
ffff95f63c036800
[ 36.064088] RBP: ffffb826803afe18 R08: 0000000000000000 R09:
0000000000000001
[ 36.064088] R10: 0000000000000000 R11: 0000000000000000 R12:
0000000000000000
[ 36.064088] R13: ffff95f635d7c240 R14: 0000000000000000 R15:
ffff95f63c47ff00
[ 36.064088] FS: 0000000000000000(0000) GS:ffff95f63fc00000(0000)
knlGS:0000000000000000
[ 36.064088] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 36.064088] CR2: 0000000000000030 CR3: 000000003c8db000 CR4:
00000000003406f0
[ 36.064088] Call Trace:
[ 36.064088] ? blk_mq_queue_tag_busy_iter+0x191/0x1d0
[ 36.064088] ? blk_mq_rq_timed_out+0x70/0x70
[ 36.064088] ? blk_mq_rq_timed_out+0x70/0x70
[ 36.064088] blk_mq_timeout_work+0xba/0x160
[ 36.064088] process_one_work+0x16b/0x480
[ 36.064088] worker_thread+0x4b/0x500
[ 36.064088] kthread+0x101/0x140
[ 36.064088] ? process_one_work+0x480/0x480
[ 36.064088] ? kthread_create_on_node+0x40/0x40
[ 36.064088] ret_from_fork+0x2c/0x40
[ 36.064088] Code: 89 d0 48 8b 3a 0f b6 48 18 48 8b 97 08 01 00 00 84
c9 75 03 03 72 04 48 8b 92 80 00 00 00 89 f6 48 8b 34 f2 48 8b 97 98 00
00 00 <48> 39 56 30 74 06 b8 01 00 00 00 c3 55 48 8b 50 10 48 89 e5 ff
[ 36.064088] RIP: bt_iter+0x31/0x50 RSP: ffffb826803afda0
[ 36.064088] CR2: 0000000000000030
[ 36.064088] ---[ end trace 469df54df5f3cd87 ]---
--
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* nvmf regression with mq-deadline
From: Sagi Grimberg @ 2017-02-27 13:00 UTC (permalink / raw)
To: Jens Axboe, linux-block@vger.kernel.org, linux-nvme
Hey Jens,
I'm getting a regression in nvme-rdma/nvme-loop with for-linus [1]
with a small script to trigger it.
The reason seems to be that the sched_tags does not take into account
the tag_set reserved tags.
This solves it for me, any objections on this?
--
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 98c7b061781e..46ca965fff5c 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -454,7 +454,8 @@ int blk_mq_sched_setup(struct request_queue *q)
*/
ret = 0;
queue_for_each_hw_ctx(q, hctx, i) {
- hctx->sched_tags = blk_mq_alloc_rq_map(set, i,
q->nr_requests, 0);
+ hctx->sched_tags = blk_mq_alloc_rq_map(set, i,
+ q->nr_requests, set->reserved_tags);
if (!hctx->sched_tags) {
ret = -ENOMEM;
break;
--
[1]:
--
[ 94.819701] ------------[ cut here ]------------
[ 94.821639] WARNING: CPU: 0 PID: 729 at block/blk-mq-tag.c:114
blk_mq_get_tag+0x21e/0x260
[ 94.825201] Modules linked in: nvme_loop nvme_fabrics nvme_core
nvmet_rdma nvmet rdma_cm iw_cm null_blk mlx5_ib iscsi_target_mod ib_srpt
ib_cm ib_core tcm_loop tcm_fc libfc tcm_qla2xxx qla2xxx
scsi_transport_fc usb_f_tcm tcm_usb_gadget libcomposite udc_core
vhost_scsi vhost target_core_file target_core_iblock target_core_pscsi
target_core_mod configfs ppdev kvm_intel kvm irqbypass crct10dif_pclmul
crc32_pclmul ghash_clmulni_intel pcbc aesni_intel aes_x86_64 crypto_simd
glue_helper cryptd input_leds joydev serio_raw parport_pc i2c_piix4
parport mac_hid sunrpc autofs4 8139too cirrus ttm psmouse drm_kms_helper
syscopyarea floppy sysfillrect mlx5_core ptp pps_core sysimgblt
fb_sys_fops 8139cp drm mii pata_acpi
[ 94.849215] CPU: 0 PID: 729 Comm: bash Not tainted 4.10.0+ #114
[ 94.850761] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 94.853183] Call Trace:
[ 94.853183] dump_stack+0x63/0x90
[ 94.853183] __warn+0xcb/0xf0
[ 94.853183] warn_slowpath_null+0x1d/0x20
[ 94.853183] blk_mq_get_tag+0x21e/0x260
[ 94.853183] ? wake_atomic_t_function+0x60/0x60
[ 94.853183] __blk_mq_alloc_request+0x1b/0xc0
[ 94.853183] blk_mq_sched_get_request+0x1d4/0x290
[ 94.853183] blk_mq_alloc_request+0x63/0xb0
[ 94.853183] nvme_alloc_request+0x53/0x60 [nvme_core]
[ 94.853183] __nvme_submit_sync_cmd+0x31/0xd0 [nvme_core]
[ 94.853183] nvmf_connect_admin_queue+0x11d/0x180 [nvme_fabrics]
[ 94.853183] ? blk_mq_init_allocated_queue+0x472/0x4a0
[ 94.853183] nvme_loop_configure_admin_queue+0xf5/0x1c0 [nvme_loop]
[ 94.853183] nvme_loop_create_ctrl+0x13c/0x550 [nvme_loop]
[ 94.853183] ? nvmf_dev_write+0x50c/0x8de [nvme_fabrics]
[ 94.853183] nvmf_dev_write+0x75a/0x8de [nvme_fabrics]
[ 94.853183] __vfs_write+0x28/0x140
[ 94.853183] ? apparmor_file_permission+0x1a/0x20
[ 94.853183] ? security_file_permission+0x3b/0xc0
[ 94.853183] ? rw_verify_area+0x4e/0xb0
[ 94.853183] vfs_write+0xb8/0x1b0
[ 94.853183] SyS_write+0x46/0xa0
[ 94.853183] ? __close_fd+0x96/0xc0
[ 94.853183] entry_SYSCALL_64_fastpath+0x1e/0xad
[ 94.853183] RIP: 0033:0x7f7be3e74a10
[ 94.853183] RSP: 002b:00007ffca6ac59c8 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 94.853183] RAX: ffffffffffffffda RBX: 0000000000000000 RCX:
00007f7be3e74a10
[ 94.853183] RDX: 000000000000001c RSI: 0000000001b90808 RDI:
0000000000000001
[ 94.853183] RBP: 0000000000000001 R08: 00007f7be4143780 R09:
00007f7be478b700
[ 94.853183] R10: 000000000000001b R11: 0000000000000246 R12:
0000000000000000
[ 94.853183] R13: 0000000000000000 R14: 0000000000000000 R15:
0000000000000000
[ 94.875340] ---[ end trace b820e053982d7057 ]---
--
[2]:
--
#!/bin/bash
CFGFS=/sys/kernel/config/nvmet
NQN=test
modprobe nvme_loop
mkdir $CFGFS/ports/1
echo "loop" > $CFGFS/ports/1/addr_trtype
mkdir $CFGFS/subsystems/$NQN
echo 1 > $CFGFS/subsystems/$NQN/attr_allow_any_host
ln -s $CFGFS/subsystems/$NQN $CFGFS/ports/1/subsystems/
echo "transport=loop,nqn=test" > /dev/nvme-fabrics
--
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related
* Re: automatic IRQ affinity for virtio V3
From: Christoph Hellwig @ 2017-02-27 8:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Christoph Hellwig, Christoph Hellwig, jasowang, axboe, pbonzini,
virtualization, linux-block, linux-kernel
In-Reply-To: <20170209180130-mutt-send-email-mst@kernel.org>
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.
^ permalink raw reply
* Re: [LSF/MM TOPIC] do we really need PG_error at all?
From: NeilBrown @ 2017-02-27 0:27 UTC (permalink / raw)
To: James Bottomley, Jeff Layton, linux-mm, linux-fsdevel, LKML
Cc: lsf-pc, Neil Brown, linux-scsi, linux-block
In-Reply-To: <1488151856.4157.50.camel@HansenPartnership.com>
[-- Attachment #1: Type: text/plain, Size: 5847 bytes --]
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.
>
>> 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.
Thanks,
NeilBrown
>
> 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.
>> > >
[-- 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-26 23:57 UTC (permalink / raw)
To: James Bottomley, NeilBrown, linux-mm, linux-fsdevel, LKML
Cc: lsf-pc, Neil Brown, linux-scsi, linux-block
In-Reply-To: <1488151856.4157.50.camel@HansenPartnership.com>
On Sun, 2017-02-26 at 15:30 -0800, 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).
>
> > >
> > > 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.
>
> > 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.
>
> James
>
Ok, I see what you mean about the partition code. We may very well need
to keep PG_error for things like that.
If we do, then it'd be good to spell out when and how filesystems should
use it. Most of the usage seems to be the result of cargo-cult copying
all over the place.
In particular, I think we might be best off not using PG_error for
writeback errors in filesystems. It complicates the error path there and
I don't see how it adds much benefit. It's also inconsistent as a stray
sync() syscall can wipe the flag in most cases without reporting
anything.
For filesystem read errors, it might make sense to keep it, but it'd be
nice to see some guidelines about how it should be used there.
> > 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>
^ permalink raw reply
* Re: [LSF/MM TOPIC] do we really need PG_error at all?
From: James Bottomley @ 2017-02-26 23:30 UTC (permalink / raw)
To: NeilBrown, Jeff Layton, linux-mm, linux-fsdevel, LKML
Cc: lsf-pc, Neil Brown, linux-scsi, linux-block
In-Reply-To: <877f4cr7ew.fsf@notabene.neil.brown.name>
[-- Attachment #1: Type: text/plain, Size: 4244 bytes --]
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).
> >
> > 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.
> 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.
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.
> > >
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [LSF/MM TOPIC] do we really need PG_error at all?
From: Jeff Layton @ 2017-02-26 22:43 UTC (permalink / raw)
To: NeilBrown, James Bottomley, linux-mm, linux-fsdevel, LKML
Cc: lsf-pc, Neil Brown, linux-scsi, linux-block
In-Reply-To: <877f4cr7ew.fsf@notabene.neil.brown.name>
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?
>
The thing is that we only ever TestClearError in write and fsync type
codepaths. Does it make sense to report read errors _at_all_ in fsync?
> >
> > 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, 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. 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.
>
> I think the question here is: Do filesystems need the pagecache to
> record which pages have seen an IO error?
> 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.
>
Yeah, it might be useful for preventing failing read retries, but I
don't see that it's being used in that way today, unless I'm missing
something.
If PG_error is ultimately needed, I'd like to have some more clearly
defined semantics for it. It's sprinkled all over the place today, and
it's not clear to me that it's being used correctly everywhere.
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply
* Re: [LSF/MM TOPIC] do we really need PG_error at all?
From: NeilBrown @ 2017-02-26 21:03 UTC (permalink / raw)
To: James Bottomley, Jeff Layton, linux-mm, linux-fsdevel, LKML
Cc: lsf-pc, Neil Brown, linux-scsi, linux-block
In-Reply-To: <1488129033.4157.8.camel@HansenPartnership.com>
[-- Attachment #1: Type: text/plain, Size: 3158 bytes --]
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?
>
> 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, 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. 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.
I think the question here is: Do filesystems need the pagecache to
record which pages have seen an IO error?
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.
>>
[-- 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: James Bottomley @ 2017-02-26 17:10 UTC (permalink / raw)
To: Jeff Layton, linux-mm, linux-fsdevel, LKML
Cc: lsf-pc, Neil Brown, linux-scsi, linux-block
In-Reply-To: <1488120164.2948.4.camel@redhat.com>
[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.
>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).
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.
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.
>
^ permalink raw reply
* Re: [PATCH 1/2] lightnvm: add generic ocssd detection
From: Christoph Hellwig @ 2017-02-26 6:47 UTC (permalink / raw)
To: Matias Bjørling
Cc: Christoph Hellwig, linux-block, linux-kernel, linux-nvme
In-Reply-To: <8cce0bfd-a153-ba4e-0d84-bea29764982e@cnexlabs.com>
[adding linux-nvme to Cc as the patch changes the nvme driver, despite
the subject line]
On Sat, Feb 25, 2017 at 08:16:04PM +0100, Matias Bj=F8rling wrote:
> On 02/25/2017 07:21 PM, Christoph Hellwig wrote:
> > On Fri, Feb 24, 2017 at 06:16:48PM +0100, Matias Bj=F8rling wrote:
> > > More implementations of OCSSDs are becoming available. Adding each us=
ing
> > > pci ids are becoming a hassle. Instead, use a 16 byte string in the
> > > vendor-specific area of the identification command to identify an
> > > Open-Channel SSD.
> > > =
> > > The large string should make the collision probability with other
> > > vendor-specific strings to be near nil.
> > =
> > 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 a=
nd
> > 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.
> 1. Enabling open-channel SSDs on NVMeoF. Customers are asking to use OCSS=
Ds
> with NVMoeF. I do not think detection of PCI ids works with that.
To use NVMoeF your protocol needs to be NVMe. Get it standardized.
> 2. Some vendors are circumventing the OCSSD detection by utilizing the CN=
EX
> Labs PCI ids. That is not very helpful and shows that there is a need for=
a
> generic approach. When they become public and will use their PCI id (if t=
hey
> will do that...), it is cumbersome to backport their PCI ids back to
> previous kernel versions to detect support.
Sue them.
> 3. Things are not a technical issue for why this is not adopted today. It
> will be soon enough one way or another, but until then, a pragmatic appro=
ach
> would go a long way.
It's not a pragmatic approach, it's broken so please don't use these
whitewashing words.
> If identify VS is too specific, is there another combination that solves =
the
> above in a generic and practical way that would satisfy you and the above?
Standardize your interface and get a I/O command set bit for it
standardized in the NVMe spec. You've had a year and a half since
the lightnvm code hit the kernel tree to do this.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCH 1/2] lightnvm: add generic ocssd detection
From: Matias Bjørling @ 2017-02-25 19:16 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block, linux-kernel
In-Reply-To: <20170225182117.GA26447@infradead.org>
On 02/25/2017 07:21 PM, Christoph Hellwig wrote:
> On Fri, Feb 24, 2017 at 06:16:48PM +0100, Matias Bj�rling wrote:
>> More implementations of OCSSDs are becoming available. Adding each using
>> pci ids are becoming a hassle. Instead, use a 16 byte string in the
>> vendor-specific area of the identification command to identify an
>> Open-Channel SSD.
>>
>> The large string should make the collision probability with other
>> vendor-specific strings to be near nil.
>
> 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:
1. Enabling open-channel SSDs on NVMeoF. Customers are asking to use
OCSSDs with NVMoeF. I do not think detection of PCI ids works with that.
2. Some vendors are circumventing the OCSSD detection by utilizing the
CNEX Labs PCI ids. That is not very helpful and shows that there is a
need for a generic approach. When they become public and will use their
PCI id (if they will do that...), it is cumbersome to backport their PCI
ids back to previous kernel versions to detect support.
3. Things are not a technical issue for why this is not adopted today.
It will be soon enough one way or another, but until then, a pragmatic
approach would go a long way.
If identify VS is too specific, is there another combination that solves
the above in a generic and practical way that would satisfy you and the
above?
-Matias
^ permalink raw reply
* Re: [WIP BRANCH] cgroups support in bfq-mq WIP branch
From: Jens Axboe @ 2017-02-25 18:52 UTC (permalink / raw)
To: Paolo Valente, ulf.hansson@linaro.org, tj@kernel.org,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
broonie@kernel.org, linus.walleij@linaro.org, Bart Van Assche
In-Reply-To: <CBE2870C-EEB4-4C2E-A5A3-20C37E16B6AE@linaro.org>
On 02/25/2017 10:44 AM, Paolo Valente wrote:
> Hi,
> I've just completed cgroups support, and I'd like to highlight the
> main blk-mq issue that I have found along the way. I have pushed the
> commit that completes the support for cgroups to the usual WIP branch
> [1]. Before moving to this issue, I have preliminary question about
> the scheduler name, since I'm about to start preparing the patch
> series for submission. So far, I have used bfq-mq as a temporary
> name. Are we fine with it, or should I change it, for example, to
> just bfq? Jens?
Just call it 'bfq', that doesn't conflict with anything that's
in the kernel already.
> I've found a sort of circular dependency in blk-mq, related to
> scheduler initialization. To describe both the issue and how I've
> addressed it, I'm pasting the message of the new commit.
Rebase your patches on top of Linus current master, some of them
will need to change and some can be dropped.
And disentangle it completely from the old bfq, I don't want to see
nasty stuff like includes of .c files with prior defines modifying
behavior of functions.
When that's done, get it posted for review asap. I would imagine
we will go through a few postings and review cycles, and if we're
targeting 4.12 with this, then we should get the ball rolling
on that side.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH v1 02/14] block: introduce bio_remove_last_page()
From: Christoph Hellwig @ 2017-02-25 18:23 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig
In-Reply-To: <1487950971-1131-3-git-send-email-tom.leiming@gmail.com>
On Fri, Feb 24, 2017 at 11:42:39PM +0800, Ming Lei wrote:
> MD need this helper to remove the last added page, so introduce
> it.
If MD really has a valid use case for this it should open code the
operation. The semantics look deeply fishy to me.
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-25 18:22 UTC (permalink / raw)
To: hch@lst.de
Cc: Linus Torvalds, Bart Van Assche, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, snitzer@redhat.com
In-Reply-To: <20170225181704.GB27774@lst.de>
On 02/25/2017 11:17 AM, hch@lst.de wrote:
> On Fri, Feb 24, 2017 at 01:22:42PM -0700, Jens Axboe wrote:
>> Bart, I pushed a fix here:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=61febef40bfe8ab68259d8545257686e8a0d91d1
>
> Yeah, this looks fine to me. It was broken on blk-mq before, but
> basically impossible to hit. I wonder if we should have a debug mode
> where we set requests to a known pattern after they are freed to catch
> these sort of bugs.
tio/md would probably never change for the same set of requests, so
it worked by happy accident before. Some of the blk-mq state remains
valid after "freeing" a request, but we could selectively fill and
definitely fill the entire PDU area with free poison.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH v1 01/14] block: introduce bio_segments_all()
From: Christoph Hellwig @ 2017-02-25 18:22 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig
In-Reply-To: <1487950971-1131-2-git-send-email-tom.leiming@gmail.com>
> +static inline unsigned bio_segments_all(struct bio *bio)
> +{
> + WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED));
> +
> + return bio->bi_vcnt;
> +}
I don't think this helpers really adds any benefit.
^ permalink raw reply
* Re: [PATCH 1/2] lightnvm: add generic ocssd detection
From: Christoph Hellwig @ 2017-02-25 18:21 UTC (permalink / raw)
To: Matias Bjørling; +Cc: linux-block, linux-kernel
In-Reply-To: <20170224171649.27409-1-matias@cnexlabs.com>
On Fri, Feb 24, 2017 at 06:16:48PM +0100, Matias Bj�rling wrote:
> More implementations of OCSSDs are becoming available. Adding each using
> pci ids are becoming a hassle. Instead, use a 16 byte string in the
> vendor-specific area of the identification command to identify an
> Open-Channel SSD.
>
> The large string should make the collision probability with other
> vendor-specific strings to be near nil.
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.
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: hch @ 2017-02-25 18:17 UTC (permalink / raw)
To: Jens Axboe
Cc: Linus Torvalds, Bart Van Assche, hch@lst.de,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
snitzer@redhat.com
In-Reply-To: <0dc039e5-58c5-d328-0b81-bf9e7d782675@kernel.dk>
On Fri, Feb 24, 2017 at 01:22:42PM -0700, Jens Axboe wrote:
> Bart, I pushed a fix here:
>
> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=61febef40bfe8ab68259d8545257686e8a0d91d1
Yeah, this looks fine to me. It was broken on blk-mq before, but
basically impossible to hit. I wonder if we should have a debug mode
where we set requests to a known pattern after they are freed to catch
these sort of bugs.
^ 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