* [PATCH 1/4] block: move req_set_nomerge to blk.h
From: Christoph Hellwig @ 2017-02-08 13:13 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208131343.4003-1-hch@lst.de>
This makes it available outside of blk-merge.c, and inlining such a trivial
helper seems pretty useful to start with.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-merge.c | 7 -------
block/blk.h | 7 +++++++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index a373416dbc9a..c956d9e7aafd 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -482,13 +482,6 @@ int blk_rq_map_sg(struct request_queue *q, struct request *rq,
}
EXPORT_SYMBOL(blk_rq_map_sg);
-static void req_set_nomerge(struct request_queue *q, struct request *req)
-{
- req->cmd_flags |= REQ_NOMERGE;
- if (req == q->last_merge)
- q->last_merge = NULL;
-}
-
static inline int ll_new_hw_segment(struct request_queue *q,
struct request *req,
struct bio *bio)
diff --git a/block/blk.h b/block/blk.h
index 4972b98d47e1..3e08703902a9 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -256,6 +256,13 @@ static inline int blk_do_io_stat(struct request *rq)
!blk_rq_is_passthrough(rq);
}
+static inline void req_set_nomerge(struct request_queue *q, struct request *req)
+{
+ req->cmd_flags |= REQ_NOMERGE;
+ if (req == q->last_merge)
+ q->last_merge = NULL;
+}
+
/*
* Internal io_context interface
*/
--
2.11.0
^ permalink raw reply related
* support for multi-range discard requests V2
From: Christoph Hellwig @ 2017-02-08 13:13 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
Hi all,
this series adds support for merging discontiguous discard bios into a
single request if the driver supports it. This reduces the number of
discards sent to the device by about a factor of 5-6 for typical
workloads on NVMe, and for slower devices that use I/O scheduling
the number will probably be even bigger but I've not implemented
that support yet.
Changes since V1:
- hardcoded max ranges for NVMe to 255
- minor cleanups
^ permalink raw reply
* Re: 4.10 lockdep: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
From: Konstantin Khlebnikov @ 2017-02-08 13:10 UTC (permalink / raw)
To: linux-kernel, Jens Axboe, linux-block
In-Reply-To: <f84551e2-a837-8c39-bbae-e22fc1429a75@yandex-team.ru>
Looks like this caused by del_timer_sync() in wbt_disable_default() under irq-safe lock.
Here
void wbt_disable_default(struct request_queue *q)
{
struct rq_wb *rwb = q->rq_wb;
if (rwb && rwb->enable_state == WBT_STATE_ON_DEFAULT) {
del_timer_sync(&rwb->window_timer);
rwb->win_nsec = rwb->min_lat_nsec = 0;
wbt_update_limits(rwb);
}
}
EXPORT_SYMBOL_GPL(wbt_disable_default);
On 08.02.2017 12:51, Konstantin Khlebnikov wrote:
> [ 41.715309] ======================================================
> [ 41.715410] [ INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected ]
> [ 41.715513] 4.10.0-rc7+ #2 Not tainted
> [ 41.715607] ------------------------------------------------------
> [ 41.715708] Starter.procman/1467 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> [ 41.715833] (((&rwb->window_timer))){+.-...}, at: [<ffffffff812fe7c5>] del_timer_sync+0x5/0x100
> [ 41.715974]
> and this task is already holding:
> [ 41.716094] (&(&q->__queue_lock)->rlock){-.-...}, at: [<ffffffff81c192ff>] cfq_set_request+0x18f/0xd50
> [ 41.716230] which would create a new lock dependency:
> [ 41.716327] (&(&q->__queue_lock)->rlock){-.-...} -> (((&rwb->window_timer))){+.-...}
> [ 41.716457]
> but this new dependency connects a HARDIRQ-irq-safe lock:
> [ 41.716582] (&(&q->__queue_lock)->rlock){-.-...}
> [ 41.716585]
> ... which became HARDIRQ-irq-safe at:
> [ 41.716801]
> [ 41.716807] [<ffffffff812a457a>] __lock_acquire+0x14ba/0x3a20
> [ 41.716994]
> [ 41.716997] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.717182]
> [ 41.717186] [<ffffffff82b0a700>] _raw_spin_lock_irqsave+0x50/0x70
> [ 41.717374]
> [ 41.717376] [<ffffffff81c17ce4>] cfq_idle_slice_timer+0xa4/0x3b0
> [ 41.717563]
> [ 41.717567] [<ffffffff81307704>] __hrtimer_run_queues+0x294/0xb50
> [ 41.717754]
> [ 41.717756] [<ffffffff81309827>] hrtimer_interrupt+0x187/0x5e0
> [ 41.717943]
> [ 41.717948] [<ffffffff810f50d4>] local_apic_timer_interrupt+0x74/0xe0
> [ 41.718136]
> [ 41.718140] [<ffffffff82b0d576>] smp_apic_timer_interrupt+0x76/0xa0
> [ 41.718327]
> [ 41.718330] [<ffffffff82b0c633>] apic_timer_interrupt+0x93/0xa0
> [ 41.718516]
> [ 41.718520] [<ffffffff825b781b>] cpuidle_enter_state+0x11b/0x820
> [ 41.718707]
> [ 41.718709] [<ffffffff825b7fb7>] cpuidle_enter+0x17/0x20
> [ 41.718894]
> [ 41.718897] [<ffffffff81288147>] call_cpuidle+0x47/0xc0
> [ 41.719082]
> [ 41.719084] [<ffffffff8128854e>] do_idle+0x21e/0x2e0
> [ 41.719269]
> [ 41.719272] [<ffffffff81288b1d>] cpu_startup_entry+0x1d/0x20
> [ 41.719458]
> [ 41.719461] [<ffffffff810f110d>] start_secondary+0x29d/0x370
> [ 41.719647]
> [ 41.719651] [<ffffffff810001c4>] verify_cpu+0x0/0xfc
> [ 41.719835]
> to a HARDIRQ-irq-unsafe lock:
> [ 41.719954] (((&rwb->window_timer))){+.-...}
> [ 41.719957]
> ... which became HARDIRQ-irq-unsafe at:
> [ 41.720172] ...
> [ 41.720172]
> [ 41.720263] [<ffffffff812a4289>] __lock_acquire+0x11c9/0x3a20
> [ 41.720450]
> [ 41.720452] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.720638]
> [ 41.720641] [<ffffffff812fe9e8>] call_timer_fn+0x118/0x520
> [ 41.720827]
> [ 41.720829] [<ffffffff812ff6d7>] run_timer_softirq+0x537/0x1450
> [ 41.721017]
> [ 41.721020] [<ffffffff82b0dc2e>] __do_softirq+0x21e/0x8a9
> [ 41.721205]
> [ 41.721209] [<ffffffff811b5093>] irq_exit+0x1a3/0x1d0
> [ 41.721394]
> [ 41.721397] [<ffffffff82b0d57b>] smp_apic_timer_interrupt+0x7b/0xa0
> [ 41.721584]
> [ 41.721586] [<ffffffff82b0c633>] apic_timer_interrupt+0x93/0xa0
> [ 41.721772]
> [ 41.721774] [<ffffffff825b781b>] cpuidle_enter_state+0x11b/0x820
> [ 41.721961]
> [ 41.721963] [<ffffffff825b7fb7>] cpuidle_enter+0x17/0x20
> [ 41.722148]
> [ 41.722151] [<ffffffff81288147>] call_cpuidle+0x47/0xc0
> [ 41.722336]
> [ 41.722338] [<ffffffff8128854e>] do_idle+0x21e/0x2e0
> [ 41.722522]
> [ 41.722524] [<ffffffff81288b1d>] cpu_startup_entry+0x1d/0x20
> [ 41.722709]
> [ 41.722712] [<ffffffff810f110d>] start_secondary+0x29d/0x370
> [ 41.722897]
> [ 41.722899] [<ffffffff810001c4>] verify_cpu+0x0/0xfc
> [ 41.723083]
> other info that might help us debug this:
>
> [ 41.723320] Possible interrupt unsafe locking scenario:
>
> [ 41.723443] CPU0 CPU1
> [ 41.723538] ---- ----
> [ 41.723634] lock(((&rwb->window_timer)));
> [ 41.723730] local_irq_disable();
> [ 41.723829] lock(&(&q->__queue_lock)->rlock);
> [ 41.723931] lock(((&rwb->window_timer)));
> [ 41.724033] <Interrupt>
> [ 41.724124] lock(&(&q->__queue_lock)->rlock);
> [ 41.724221]
> *** DEADLOCK ***
>
> [ 41.730049] 3 locks held by Starter.procman/1467:
> [ 41.730144] #0: (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff816b2c53>] prepare_bprm_creds+0x53/0x100
> [ 41.730280] #1: (&type->i_mutex_dir_key#2){++++++}, at: [<ffffffff816cdb67>] path_openat+0x1317/0x2730
> [ 41.730415] #2: (&(&q->__queue_lock)->rlock){-.-...}, at: [<ffffffff81c192ff>] cfq_set_request+0x18f/0xd50
> [ 41.730550]
> the dependencies between HARDIRQ-irq-safe lock and the holding lock:
> [ 41.730701] -> (&(&q->__queue_lock)->rlock){-.-...} ops: 68816 {
> [ 41.730804] IN-HARDIRQ-W at:
> [ 41.730897]
> [ 41.730900] [<ffffffff812a457a>] __lock_acquire+0x14ba/0x3a20
> [ 41.731091]
> [ 41.731093] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.731283]
> [ 41.731285] [<ffffffff82b0a700>] _raw_spin_lock_irqsave+0x50/0x70
> [ 41.731476]
> [ 41.731478] [<ffffffff81c17ce4>] cfq_idle_slice_timer+0xa4/0x3b0
> [ 41.731669]
> [ 41.731672] [<ffffffff81307704>] __hrtimer_run_queues+0x294/0xb50
> [ 41.731862]
> [ 41.731865] [<ffffffff81309827>] hrtimer_interrupt+0x187/0x5e0
> [ 41.732056]
> [ 41.732058] [<ffffffff810f50d4>] local_apic_timer_interrupt+0x74/0xe0
> [ 41.732250]
> [ 41.732253] [<ffffffff82b0d576>] smp_apic_timer_interrupt+0x76/0xa0
> [ 41.732444]
> [ 41.732447] [<ffffffff82b0c633>] apic_timer_interrupt+0x93/0xa0
> [ 41.732637]
> [ 41.732639] [<ffffffff825b781b>] cpuidle_enter_state+0x11b/0x820
> [ 41.732830]
> [ 41.732832] [<ffffffff825b7fb7>] cpuidle_enter+0x17/0x20
> [ 41.733021]
> [ 41.733023] [<ffffffff81288147>] call_cpuidle+0x47/0xc0
> [ 41.733212]
> [ 41.733214] [<ffffffff8128854e>] do_idle+0x21e/0x2e0
> [ 41.733401]
> [ 41.733404] [<ffffffff81288b1d>] cpu_startup_entry+0x1d/0x20
> [ 41.733593]
> [ 41.733596] [<ffffffff810f110d>] start_secondary+0x29d/0x370
> [ 41.733785]
> [ 41.733787] [<ffffffff810001c4>] verify_cpu+0x0/0xfc
> [ 41.733975] IN-SOFTIRQ-W at:
> [ 41.734068]
> [ 41.734071] [<ffffffff812a4258>] __lock_acquire+0x1198/0x3a20
> [ 41.734261]
> [ 41.734264] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.734453]
> [ 41.734455] [<ffffffff82b0a700>] _raw_spin_lock_irqsave+0x50/0x70
> [ 41.734646]
> [ 41.734650] [<ffffffff8221f4a2>] scsi_end_request+0x272/0x5a0
> [ 41.734841]
> [ 41.734844] [<ffffffff8222acb5>] scsi_io_completion+0x465/0x1800
> [ 41.735034]
> [ 41.735037] [<ffffffff8220b353>] scsi_finish_command+0x393/0x560
> [ 41.735227]
> [ 41.735230] [<ffffffff82228f35>] scsi_softirq_done+0x2a5/0x350
> [ 41.735420]
> [ 41.735423] [<ffffffff81ba3ff8>] blk_done_softirq+0x208/0x360
> [ 41.735614]
> [ 41.735616] [<ffffffff82b0dc2e>] __do_softirq+0x21e/0x8a9
> [ 41.735805]
> [ 41.735807] [<ffffffff811b4d36>] run_ksoftirqd+0x26/0xf0
> [ 41.735996]
> [ 41.735999] [<ffffffff81210c14>] smpboot_thread_fn+0x3d4/0x7f0
> [ 41.736190]
> [ 41.736193] [<ffffffff81205d4e>] kthread+0x2ce/0x3d0
> [ 41.736381]
> [ 41.736383] [<ffffffff82b0aae1>] ret_from_fork+0x31/0x40
> [ 41.736572] INITIAL USE at:
> [ 41.736665]
> [ 41.736668] [<ffffffff812a3b74>] __lock_acquire+0xab4/0x3a20
> [ 41.736857]
> [ 41.736860] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.737049]
> [ 41.737051] [<ffffffff82b09e94>] _raw_spin_lock_irq+0x44/0x60
> [ 41.737240]
> [ 41.737245] [<ffffffff81bfb294>] blkcg_init_queue+0xb4/0x2e0
> [ 41.737526]
> [ 41.737529] [<ffffffff81b7630a>] blk_alloc_queue_node+0x5ea/0x7e0
> [ 41.737720]
> [ 41.737722] [<ffffffff81b76513>] blk_alloc_queue+0x13/0x20
> [ 41.737911]
> [ 41.737914] [<ffffffff82161919>] brd_alloc+0xf9/0x590
> [ 41.738103]
> [ 41.738107] [<ffffffff83a40642>] brd_init+0x62/0x2ad
> [ 41.738294]
> [ 41.738297] [<ffffffff81002261>] do_one_initcall+0x91/0x230
> [ 41.738486]
> [ 41.738491] [<ffffffff8399eb3d>] kernel_init_freeable+0x44f/0x4e1
> [ 41.738682]
> [ 41.738685] [<ffffffff82af0ba3>] kernel_init+0x13/0x120
> [ 41.738873]
> [ 41.738875] [<ffffffff82b0aae1>] ret_from_fork+0x31/0x40
> [ 41.739065] }
> [ 41.739157] ... key at: [<ffffffff849414c0>] __key.51360+0x0/0x40
> [ 41.739258] ... acquired at:
> [ 41.739349]
> [ 41.739352] [<ffffffff812a0a3a>] check_irq_usage+0x7a/0x180
> [ 41.739538]
> [ 41.739540] [<ffffffff812a559b>] __lock_acquire+0x24db/0x3a20
> [ 41.739727]
> [ 41.739730] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.739915]
> [ 41.739918] [<ffffffff812fe80e>] del_timer_sync+0x4e/0x100
> [ 41.740103]
> [ 41.740107] [<ffffffff81c33ee6>] wbt_disable_default+0x76/0x210
> [ 41.740294]
> [ 41.740296] [<ffffffff81c183c7>] check_blkcg_changed+0x3d7/0xee0
> [ 41.740483]
> [ 41.740485] [<ffffffff81c193f8>] cfq_set_request+0x288/0xd50
> [ 41.740670]
> [ 41.740674] [<ffffffff81b6b583>] elv_set_request+0x83/0xf0
> [ 41.740860]
> [ 41.740862] [<ffffffff81b8032d>] get_request+0x16ad/0x1dc0
> [ 41.741048]
> [ 41.741050] [<ffffffff81b890c5>] blk_queue_bio+0x335/0xe20
> [ 41.741235]
> [ 41.741238] [<ffffffff81b81ca0>] generic_make_request+0x2e0/0x730
> [ 41.741425]
> [ 41.741427] [<ffffffff81b82228>] submit_bio+0x138/0x4c0
> [ 41.741612]
> [ 41.741615] [<ffffffff817528c0>] submit_bh_wbc+0x400/0x5c0
> [ 41.741801]
> [ 41.741803] [<ffffffff81754566>] ll_rw_block+0xc6/0x160
> [ 41.741989]
> [ 41.741992] [<ffffffff81890650>] ext4_find_entry+0xc60/0x1230
> [ 41.742179]
> [ 41.742181] [<ffffffff81891ed2>] ext4_lookup+0x112/0x770
> [ 41.742366]
> [ 41.742368] [<ffffffff816c3ee1>] lookup_open+0xd41/0x1520
> [ 41.742553]
> [ 41.742555] [<ffffffff816cdb8a>] path_openat+0x133a/0x2730
> [ 41.742741]
> [ 41.742743] [<ffffffff816d25b5>] do_filp_open+0x175/0x230
> [ 41.742928]
> [ 41.742931] [<ffffffff816ad999>] do_open_execat+0xb9/0x480
> [ 41.743116]
> [ 41.743119] [<ffffffff816b3351>] do_execveat_common.isra.39+0x651/0x1ec0
> [ 41.743308]
> [ 41.743310] [<ffffffff816b546a>] SyS_execve+0x3a/0x50
> [ 41.743495]
> [ 41.743498] [<ffffffff8100768d>] do_syscall_64+0x18d/0x450
> [ 41.743683]
> [ 41.743686] [<ffffffff82b0a90d>] return_from_SYSCALL_64+0x0/0x7a
>
> [ 41.743961]
> the dependencies between the lock to be acquired
> [ 41.743962] and HARDIRQ-irq-unsafe lock:
> [ 41.744181] -> (((&rwb->window_timer))){+.-...} ops: 362 {
> [ 41.744282] HARDIRQ-ON-W at:
> [ 41.744375]
> [ 41.744378] [<ffffffff812a4289>] __lock_acquire+0x11c9/0x3a20
> [ 41.744568]
> [ 41.744570] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.744760]
> [ 41.744762] [<ffffffff812fe9e8>] call_timer_fn+0x118/0x520
> [ 41.744951]
> [ 41.744954] [<ffffffff812ff6d7>] run_timer_softirq+0x537/0x1450
> [ 41.745144]
> [ 41.745147] [<ffffffff82b0dc2e>] __do_softirq+0x21e/0x8a9
> [ 41.745336]
> [ 41.745338] [<ffffffff811b5093>] irq_exit+0x1a3/0x1d0
> [ 41.745527]
> [ 41.745529] [<ffffffff82b0d57b>] smp_apic_timer_interrupt+0x7b/0xa0
> [ 41.745720]
> [ 41.745723] [<ffffffff82b0c633>] apic_timer_interrupt+0x93/0xa0
> [ 41.745913]
> [ 41.745915] [<ffffffff825b781b>] cpuidle_enter_state+0x11b/0x820
> [ 41.746106]
> [ 41.746108] [<ffffffff825b7fb7>] cpuidle_enter+0x17/0x20
> [ 41.746297]
> [ 41.746299] [<ffffffff81288147>] call_cpuidle+0x47/0xc0
> [ 41.746488]
> [ 41.746490] [<ffffffff8128854e>] do_idle+0x21e/0x2e0
> [ 41.746678]
> [ 41.746680] [<ffffffff81288b1d>] cpu_startup_entry+0x1d/0x20
> [ 41.746870]
> [ 41.746872] [<ffffffff810f110d>] start_secondary+0x29d/0x370
> [ 41.752819]
> [ 41.752821] [<ffffffff810001c4>] verify_cpu+0x0/0xfc
> [ 41.753009] IN-SOFTIRQ-W at:
> [ 41.753102]
> [ 41.753105] [<ffffffff812a4258>] __lock_acquire+0x1198/0x3a20
> [ 41.753295]
> [ 41.753297] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.753487]
> [ 41.753489] [<ffffffff812fe9e8>] call_timer_fn+0x118/0x520
> [ 41.753679]
> [ 41.753681] [<ffffffff812ff6d7>] run_timer_softirq+0x537/0x1450
> [ 41.753872]
> [ 41.753874] [<ffffffff82b0dc2e>] __do_softirq+0x21e/0x8a9
> [ 41.754063]
> [ 41.754065] [<ffffffff811b5093>] irq_exit+0x1a3/0x1d0
> [ 41.754254]
> [ 41.754256] [<ffffffff82b0d57b>] smp_apic_timer_interrupt+0x7b/0xa0
> [ 41.754447]
> [ 41.754450] [<ffffffff82b0c633>] apic_timer_interrupt+0x93/0xa0
> [ 41.754640]
> [ 41.754642] [<ffffffff825b781b>] cpuidle_enter_state+0x11b/0x820
> [ 41.754833]
> [ 41.754835] [<ffffffff825b7fb7>] cpuidle_enter+0x17/0x20
> [ 41.755024]
> [ 41.755026] [<ffffffff81288147>] call_cpuidle+0x47/0xc0
> [ 41.755214]
> [ 41.755216] [<ffffffff8128854e>] do_idle+0x21e/0x2e0
> [ 41.755404]
> [ 41.755406] [<ffffffff81288b1d>] cpu_startup_entry+0x1d/0x20
> [ 41.755596]
> [ 41.755598] [<ffffffff810f110d>] start_secondary+0x29d/0x370
> [ 41.755788]
> [ 41.755790] [<ffffffff810001c4>] verify_cpu+0x0/0xfc
> [ 41.755978] INITIAL USE at:
> [ 41.756070]
> [ 41.756073] [<ffffffff812a3b74>] __lock_acquire+0xab4/0x3a20
> [ 41.756262]
> [ 41.756265] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.756454]
> [ 41.756456] [<ffffffff812fe9e8>] call_timer_fn+0x118/0x520
> [ 41.756645]
> [ 41.756648] [<ffffffff812ff6d7>] run_timer_softirq+0x537/0x1450
> [ 41.756838]
> [ 41.756841] [<ffffffff82b0dc2e>] __do_softirq+0x21e/0x8a9
> [ 41.757029]
> [ 41.757031] [<ffffffff811b5093>] irq_exit+0x1a3/0x1d0
> [ 41.757220]
> [ 41.757222] [<ffffffff82b0d57b>] smp_apic_timer_interrupt+0x7b/0xa0
> [ 41.757413]
> [ 41.757416] [<ffffffff82b0c633>] apic_timer_interrupt+0x93/0xa0
> [ 41.757606]
> [ 41.757608] [<ffffffff825b781b>] cpuidle_enter_state+0x11b/0x820
> [ 41.757798]
> [ 41.757800] [<ffffffff825b7fb7>] cpuidle_enter+0x17/0x20
> [ 41.757989]
> [ 41.757991] [<ffffffff81288147>] call_cpuidle+0x47/0xc0
> [ 41.758180]
> [ 41.758182] [<ffffffff8128854e>] do_idle+0x21e/0x2e0
> [ 41.758371]
> [ 41.758373] [<ffffffff81288b1d>] cpu_startup_entry+0x1d/0x20
> [ 41.758562]
> [ 41.758565] [<ffffffff810f110d>] start_secondary+0x29d/0x370
> [ 41.758754]
> [ 41.758756] [<ffffffff810001c4>] verify_cpu+0x0/0xfc
> [ 41.758943] }
> [ 41.759034] ... key at: [<ffffffff849430c0>] __key.42825+0x0/0x40
> [ 41.759135] ... acquired at:
> [ 41.759227]
> [ 41.759229] [<ffffffff812a0a3a>] check_irq_usage+0x7a/0x180
> [ 41.759415]
> [ 41.759418] [<ffffffff812a559b>] __lock_acquire+0x24db/0x3a20
> [ 41.759604]
> [ 41.759607] [<ffffffff812a7663>] lock_acquire+0x173/0x330
> [ 41.759792]
> [ 41.759795] [<ffffffff812fe80e>] del_timer_sync+0x4e/0x100
> [ 41.759981]
> [ 41.759983] [<ffffffff81c33ee6>] wbt_disable_default+0x76/0x210
> [ 41.760170]
> [ 41.760172] [<ffffffff81c183c7>] check_blkcg_changed+0x3d7/0xee0
> [ 41.760359]
> [ 41.760361] [<ffffffff81c193f8>] cfq_set_request+0x288/0xd50
> [ 41.760547]
> [ 41.760550] [<ffffffff81b6b583>] elv_set_request+0x83/0xf0
> [ 41.760735]
> [ 41.760737] [<ffffffff81b8032d>] get_request+0x16ad/0x1dc0
> [ 41.760923]
> [ 41.760925] [<ffffffff81b890c5>] blk_queue_bio+0x335/0xe20
> [ 41.761111]
> [ 41.761113] [<ffffffff81b81ca0>] generic_make_request+0x2e0/0x730
> [ 41.761300]
> [ 41.761302] [<ffffffff81b82228>] submit_bio+0x138/0x4c0
> [ 41.761487]
> [ 41.761490] [<ffffffff817528c0>] submit_bh_wbc+0x400/0x5c0
> [ 41.761675]
> [ 41.761678] [<ffffffff81754566>] ll_rw_block+0xc6/0x160
> [ 41.761863]
> [ 41.761865] [<ffffffff81890650>] ext4_find_entry+0xc60/0x1230
> [ 41.762052]
> [ 41.762054] [<ffffffff81891ed2>] ext4_lookup+0x112/0x770
> [ 41.762239]
> [ 41.762241] [<ffffffff816c3ee1>] lookup_open+0xd41/0x1520
> [ 41.762427]
> [ 41.762429] [<ffffffff816cdb8a>] path_openat+0x133a/0x2730
> [ 41.762614]
> [ 41.762616] [<ffffffff816d25b5>] do_filp_open+0x175/0x230
> [ 41.762802]
> [ 41.762805] [<ffffffff816ad999>] do_open_execat+0xb9/0x480
> [ 41.762990]
> [ 41.762993] [<ffffffff816b3351>] do_execveat_common.isra.39+0x651/0x1ec0
> [ 41.763182]
> [ 41.763184] [<ffffffff816b546a>] SyS_execve+0x3a/0x50
> [ 41.763369]
> [ 41.763371] [<ffffffff8100768d>] do_syscall_64+0x18d/0x450
> [ 41.763557]
> [ 41.763559] [<ffffffff82b0a90d>] return_from_SYSCALL_64+0x0/0x7a
>
> [ 41.763834]
> stack backtrace:
> [ 41.763952] CPU: 20 PID: 1467 Comm: Starter.procman Not tainted 4.10.0-rc7+ #2
> [ 41.764076] Hardware name: Supermicro X9DRW/X9DRW, BIOS 3.0c 10/30/2014
> [ 41.764178] Call Trace:
> [ 41.764272] dump_stack+0x85/0xc4
> [ 41.764367] check_usage+0xb80/0xdd0
> [ 41.764463] ? check_usage_backwards+0x2f0/0x2f0
> [ 41.764562] ? unwind_next_frame+0x1f3/0x9d0
> [ 41.764659] ? entry_SYSCALL64_slow_path+0x25/0x25
> [ 41.764756] ? unwind_get_return_address+0x17e/0x260
> [ 41.764854] ? __unwind_start+0x390/0x390
> [ 41.764950] ? __unwind_start+0x1d0/0x390
> [ 41.765046] check_irq_usage+0x7a/0x180
> [ 41.765142] __lock_acquire+0x24db/0x3a20
> [ 41.765239] ? debug_check_no_locks_freed+0x290/0x290
> [ 41.765338] ? debug_check_no_locks_freed+0x290/0x290
> [ 41.765439] ? alloc_debug_processing+0x73/0x1b0
> [ 41.765537] lock_acquire+0x173/0x330
> [ 41.765632] ? del_timer_sync+0x5/0x100
> [ 41.765728] del_timer_sync+0x4e/0x100
> [ 41.765824] ? del_timer_sync+0x5/0x100
> [ 41.766008] wbt_disable_default+0x76/0x210
> [ 41.766104] check_blkcg_changed+0x3d7/0xee0
> [ 41.766200] ? check_blkcg_changed+0xbe/0xee0
> [ 41.766296] ? cfq_idle_slice_timer+0x3b0/0x3b0
> [ 41.766394] ? cfq_set_request+0x18f/0xd50
> [ 41.766490] cfq_set_request+0x288/0xd50
> [ 41.766587] ? debug_lockdep_rcu_enabled+0x77/0x90
> [ 41.766684] ? cfq_exit_icq+0x170/0x170
> [ 41.766780] ? mark_held_locks+0xc8/0x120
> [ 41.766875] ? _raw_spin_unlock_irq+0x2c/0x40
> [ 41.766972] ? trace_hardirqs_on_caller+0x3fe/0x580
> [ 41.767070] ? trace_hardirqs_on+0xd/0x10
> [ 41.767165] ? _raw_spin_unlock_irq+0x2c/0x40
> [ 41.767263] ? ioc_create_icq+0x403/0x6a0
> [ 41.767359] elv_set_request+0x83/0xf0
> [ 41.767454] get_request+0x16ad/0x1dc0
> [ 41.767549] ? get_request+0xb0/0x1dc0
> [ 41.767645] ? trace_event_raw_event_block_rq_with_error+0x680/0x680
> [ 41.767747] ? wake_atomic_t_function+0x2a0/0x2a0
> [ 41.767844] ? elv_merge+0x25e/0x300
> [ 41.767939] blk_queue_bio+0x335/0xe20
> [ 41.768034] ? blk_flush_plug_list+0x920/0x920
> [ 41.768131] ? blk_exit_rl+0x40/0x40
> [ 41.768226] ? generic_make_request+0x365/0x730
> [ 41.768323] generic_make_request+0x2e0/0x730
> [ 41.768419] ? generic_make_request+0x2d3/0x730
> [ 41.768516] ? blk_queue_enter+0x510/0x510
> [ 41.768612] ? __find_get_block+0x5b8/0x800
> [ 41.768709] submit_bio+0x138/0x4c0
> [ 41.768803] ? __getblk_gfp+0x14a/0x6f0
> [ 41.768899] ? do_execveat_common.isra.39+0x651/0x1ec0
> [ 41.768997] ? generic_make_request+0x730/0x730
> [ 41.769095] submit_bh_wbc+0x400/0x5c0
> [ 41.769190] ll_rw_block+0xc6/0x160
> [ 41.769285] ext4_find_entry+0xc60/0x1230
> [ 41.769381] ? ext4_search_dir+0x4b0/0x4b0
> [ 41.769478] ? d_alloc_parallel+0x595/0x1550
> [ 41.769574] ? d_alloc_parallel+0x5b4/0x1550
> [ 41.769670] ? lookup_open+0x430/0x1520
> [ 41.769766] ? __d_lookup+0x1aa/0x5e0
> [ 41.775578] ext4_lookup+0x112/0x770
> [ 41.775673] ? __d_lookup+0x1c9/0x5e0
> [ 41.775767] ? __d_lookup+0x3e/0x5e0
> [ 41.775862] ? ext4_cross_rename+0x11a0/0x11a0
> [ 41.775958] ? ext4_cross_rename+0x11a0/0x11a0
> [ 41.776055] lookup_open+0xd41/0x1520
> [ 41.776150] ? vfs_link+0xa90/0xa90
> [ 41.776245] path_openat+0x133a/0x2730
> [ 41.776340] ? path_lookupat+0x3d0/0x3d0
> [ 41.776435] ? save_stack+0xb5/0xd0
> [ 41.776530] ? save_stack_trace+0x1b/0x20
> [ 41.776625] ? save_stack+0x46/0xd0
> [ 41.776718] ? kasan_kmalloc+0xad/0xe0
> [ 41.776813] ? kasan_slab_alloc+0x12/0x20
> [ 41.776908] ? kmem_cache_alloc+0xcc/0x2b0
> [ 41.777005] ? prepare_creds+0x26/0x2b0
> [ 41.777099] ? prepare_exec_creds+0x11/0xe0
> [ 41.777195] ? prepare_bprm_creds+0x5e/0x100
> [ 41.777291] ? do_execveat_common.isra.39+0x2a1/0x1ec0
> [ 41.777389] ? SyS_execve+0x3a/0x50
> [ 41.777483] ? do_syscall_64+0x18d/0x450
> [ 41.777578] ? entry_SYSCALL64_slow_path+0x25/0x25
> [ 41.777675] do_filp_open+0x175/0x230
> [ 41.777770] ? may_open_dev+0xc0/0xc0
> [ 41.777865] ? __slab_alloc+0x5e/0x90
> [ 41.777961] ? debug_lockdep_rcu_enabled+0x77/0x90
> [ 41.778058] do_open_execat+0xb9/0x480
> [ 41.778153] ? setup_arg_pages+0x760/0x760
> [ 41.778250] do_execveat_common.isra.39+0x651/0x1ec0
> [ 41.778347] ? do_execveat_common.isra.39+0x38a/0x1ec0
> [ 41.778446] ? kasan_check_write+0x14/0x20
> [ 41.778541] ? rcu_read_lock_sched_held+0x80/0x130
> [ 41.778639] ? prepare_bprm_creds+0x100/0x100
> [ 41.778735] ? getname_flags+0x102/0x510
> [ 41.778831] SyS_execve+0x3a/0x50
> [ 41.778925] ? ptregs_sys_vfork+0x10/0x10
> [ 41.779020] do_syscall_64+0x18d/0x450
> [ 41.779115] ? setup_new_exec+0x7e0/0x7e0
> [ 41.779222] entry_SYSCALL64_slow_path+0x25/0x25
> [ 41.779327] RIP: 0033:0x7f50f1d88bc7
> [ 41.779420] RSP: 002b:00007ffcfc1d7b98 EFLAGS: 00000202 ORIG_RAX: 000000000000003b
> [ 41.779546] RAX: ffffffffffffffda RBX: 00007f50f336ef80 RCX: 00007f50f1d88bc7
> [ 41.779649] RDX: 0000000001a1fdd0 RSI: 0000000001a7af70 RDI: 0000000001a7aef0
> [ 41.779751] RBP: 0000000000000009 R08: 0000000000000000 R09: 000000000000000f
> [ 41.779854] R10: 0000000000000598 R11: 0000000000000202 R12: 0000000001a7af70
> [ 41.779956] R13: 0000000000000001 R14: 00007f50eac83cb0 R15: 0000000001a7af70
^ permalink raw reply
* Re: [PATCH 3/4] block: optionally merge discontiguous discard bios into a single request
From: Christoph Hellwig @ 2017-02-08 11:57 UTC (permalink / raw)
To: Ming Lei; +Cc: Christoph Hellwig, Jens Axboe, linux-block, linux-nvme
In-Reply-To: <CACVXFVOZfrLGb59q=V_O2xijM6t9uUq8Foj5qWZ5sAQpRSWngA@mail.gmail.com>
On Wed, Feb 08, 2017 at 06:54:24PM +0800, Ming Lei wrote:
> > + struct bio *bio)
> > +{
> > + unsigned short segments = blk_rq_nr_discard_segments(req);
> > +
> > + if (segments >= queue_max_discard_segments(q))
> > + goto no_merge;
> > + if (blk_rq_sectors(req) + bio_sectors(bio) >
> > + blk_rq_get_max_sectors(req, blk_rq_pos(req)))
> > + goto no_merge;
> > +
> > + req->biotail->bi_next = bio;
> > + req->biotail = bio;
> > + req->__data_len += bio->bi_iter.bi_size;
>
> typeof(__data_len) is unsigned, and should be easy to overflow
> for discard rq's merge.
We respect the max_discard_sectors setting which is unsigned as well
above in blk_rq_get_max_sectors(), so we can't actually overflow here.
And please remove the fullquote after your two line comment, thanks!
^ permalink raw reply
* Re: [PATCH 22/24] ubifs: Convert to separately allocated bdi
From: Richard Weinberger @ 2017-02-08 11:24 UTC (permalink / raw)
To: Jan Kara, linux-fsdevel
Cc: Christoph Hellwig, linux-block, Artem Bityutskiy, Adrian Hunter,
linux-mtd
In-Reply-To: <20170202173422.3240-23-jack@suse.cz>
Am 02.02.2017 um 18:34 schrieb Jan Kara:
> Allocate struct backing_dev_info separately instead of embedding it
> inside the superblock. This unifies handling of bdi among users.
>
> CC: Richard Weinberger <richard@nod.at>
> CC: Artem Bityutskiy <dedekind1@gmail.com>
> CC: Adrian Hunter <adrian.hunter@intel.com>
> CC: linux-mtd@lists.infradead.org
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/ubifs/super.c | 23 +++++++----------------
> fs/ubifs/ubifs.h | 3 ---
> 2 files changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index e08aa04fc835..34810eb52b22 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1827,7 +1827,6 @@ static void ubifs_put_super(struct super_block *sb)
> }
>
> ubifs_umount(c);
> - bdi_destroy(&c->bdi);
> ubi_close_volume(c->ubi);
> mutex_unlock(&c->umount_mutex);
> }
> @@ -2019,29 +2018,23 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
> goto out;
> }
>
> + err = ubifs_parse_options(c, data, 0);
> + if (err)
> + goto out_close;
> +
> /*
> * UBIFS provides 'backing_dev_info' in order to disable read-ahead. For
> * UBIFS, I/O is not deferred, it is done immediately in readpage,
> * which means the user would have to wait not just for their own I/O
> * but the read-ahead I/O as well i.e. completely pointless.
> *
> - * Read-ahead will be disabled because @c->bdi.ra_pages is 0.
> + * Read-ahead will be disabled because @sb->s_bdi->ra_pages is 0.
> */
> - c->bdi.name = "ubifs",
> - c->bdi.capabilities = 0;
So ->capabilities is now zero by default since you use __GFP_ZERO in
bdi_alloc().
At least for UBIFS I'll add a comment on this, otherwise it is not so
clear that UBIFS wants a BDI with no capabilities and how it achieves that.
Acked-by: Richard Weinberger <richard@nod.at>
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH 3/4] block: optionally merge discontiguous discard bios into a single request
From: Ming Lei @ 2017-02-08 10:54 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, linux-nvme
In-Reply-To: <20170207164658.32449-4-hch@lst.de>
On Wed, Feb 8, 2017 at 12:46 AM, Christoph Hellwig <hch@lst.de> wrote:
> Add a new merge strategy that merges discard bios into a request until the
> maximum number of discard ranges (or the maximum discard size) is reached
> from the plug merging code. I/O scheduler merging is not wired up yet
> but might also be useful, although not for fast devices like NVMe which
> are the only user for now.
>
> Note that for now we don't support limiting the size of each discard range,
> but if needed that can be added later.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> block/blk-core.c | 27 +++++++++++++++++++++++++++
> block/blk-merge.c | 5 ++++-
> block/blk-mq.c | 3 +++
> block/blk-settings.c | 20 ++++++++++++++++++++
> block/blk-sysfs.c | 12 ++++++++++++
> block/blk.h | 2 ++
> include/linux/blkdev.h | 26 ++++++++++++++++++++++++++
> include/linux/elevator.h | 1 +
> 8 files changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 75fe534861df..b9e857f4afe8 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1485,6 +1485,30 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
> return true;
> }
>
> +bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
> + struct bio *bio)
> +{
> + unsigned short segments = blk_rq_nr_discard_segments(req);
> +
> + if (segments >= queue_max_discard_segments(q))
> + goto no_merge;
> + if (blk_rq_sectors(req) + bio_sectors(bio) >
> + blk_rq_get_max_sectors(req, blk_rq_pos(req)))
> + goto no_merge;
> +
> + req->biotail->bi_next = bio;
> + req->biotail = bio;
> + req->__data_len += bio->bi_iter.bi_size;
typeof(__data_len) is unsigned, and should be easy to overflow
for discard rq's merge.
> + req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
> + req->nr_phys_segments = segments + 1;
> +
> + blk_account_io_start(req, false);
> + return true;
> +no_merge:
> + req_set_nomerge(q, req);
> + return false;
> +}
> +
> /**
> * blk_attempt_plug_merge - try to merge with %current's plugged list
> * @q: request_queue new bio is being queued at
> @@ -1549,6 +1573,9 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
> case ELEVATOR_FRONT_MERGE:
> merged = bio_attempt_front_merge(q, rq, bio);
> break;
> + case ELEVATOR_DISCARD_MERGE:
> + merged = bio_attempt_discard_merge(q, rq, bio);
> + break;
> default:
> break;
> }
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 6cbd90ad5f90..2afa262425d1 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -803,7 +803,10 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
>
> enum elv_merge blk_try_merge(struct request *rq, struct bio *bio)
> {
> - if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
> + if (req_op(rq) == REQ_OP_DISCARD &&
> + queue_max_discard_segments(rq->q) > 1)
> + return ELEVATOR_DISCARD_MERGE;
> + else if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
> return ELEVATOR_BACK_MERGE;
> else if (blk_rq_pos(rq) - bio_sectors(bio) == bio->bi_iter.bi_sector)
> return ELEVATOR_FRONT_MERGE;
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 9294633759d9..89cb2d224488 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -780,6 +780,9 @@ static bool blk_mq_attempt_merge(struct request_queue *q,
> if (blk_mq_sched_allow_merge(q, rq, bio))
> merged = bio_attempt_front_merge(q, rq, bio);
> break;
> + case ELEVATOR_DISCARD_MERGE:
> + merged = bio_attempt_discard_merge(q, rq, bio);
> + break;
> default:
> continue;
> }
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 6eb19bcbf3cb..9a68c8c19aaa 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(blk_queue_lld_busy);
> void blk_set_default_limits(struct queue_limits *lim)
> {
> lim->max_segments = BLK_MAX_SEGMENTS;
> + lim->max_discard_segments = 1;
> lim->max_integrity_segments = 0;
> lim->seg_boundary_mask = BLK_SEG_BOUNDARY_MASK;
> lim->virt_boundary_mask = 0;
> @@ -128,6 +129,7 @@ void blk_set_stacking_limits(struct queue_limits *lim)
> /* Inherit limits from component devices */
> lim->discard_zeroes_data = 1;
> lim->max_segments = USHRT_MAX;
> + lim->max_discard_segments = 1;
> lim->max_hw_sectors = UINT_MAX;
> lim->max_segment_size = UINT_MAX;
> lim->max_sectors = UINT_MAX;
> @@ -337,6 +339,22 @@ void blk_queue_max_segments(struct request_queue *q, unsigned short max_segments
> EXPORT_SYMBOL(blk_queue_max_segments);
>
> /**
> + * blk_queue_max_discard_segments - set max segments for discard requests
> + * @q: the request queue for the device
> + * @max_segments: max number of segments
> + *
> + * Description:
> + * Enables a low level driver to set an upper limit on the number of
> + * segments in a discard request.
> + **/
> +void blk_queue_max_discard_segments(struct request_queue *q,
> + unsigned short max_segments)
> +{
> + q->limits.max_discard_segments = max_segments;
> +}
> +EXPORT_SYMBOL(blk_queue_max_discard_segments);
> +
> +/**
> * blk_queue_max_segment_size - set max segment size for blk_rq_map_sg
> * @q: the request queue for the device
> * @max_size: max size of segment in bytes
> @@ -553,6 +571,8 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
> b->virt_boundary_mask);
>
> t->max_segments = min_not_zero(t->max_segments, b->max_segments);
> + t->max_discard_segments = min_not_zero(t->max_discard_segments,
> + b->max_discard_segments);
> t->max_integrity_segments = min_not_zero(t->max_integrity_segments,
> b->max_integrity_segments);
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 48032c4759a7..070d81bae1d5 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -121,6 +121,12 @@ static ssize_t queue_max_segments_show(struct request_queue *q, char *page)
> return queue_var_show(queue_max_segments(q), (page));
> }
>
> +static ssize_t queue_max_discard_segments_show(struct request_queue *q,
> + char *page)
> +{
> + return queue_var_show(queue_max_discard_segments(q), (page));
> +}
> +
> static ssize_t queue_max_integrity_segments_show(struct request_queue *q, char *page)
> {
> return queue_var_show(q->limits.max_integrity_segments, (page));
> @@ -545,6 +551,11 @@ static struct queue_sysfs_entry queue_max_segments_entry = {
> .show = queue_max_segments_show,
> };
>
> +static struct queue_sysfs_entry queue_max_discard_segments_entry = {
> + .attr = {.name = "max_discard_segments", .mode = S_IRUGO },
> + .show = queue_max_discard_segments_show,
> +};
> +
> static struct queue_sysfs_entry queue_max_integrity_segments_entry = {
> .attr = {.name = "max_integrity_segments", .mode = S_IRUGO },
> .show = queue_max_integrity_segments_show,
> @@ -697,6 +708,7 @@ static struct attribute *default_attrs[] = {
> &queue_max_hw_sectors_entry.attr,
> &queue_max_sectors_entry.attr,
> &queue_max_segments_entry.attr,
> + &queue_max_discard_segments_entry.attr,
> &queue_max_integrity_segments_entry.attr,
> &queue_max_segment_size_entry.attr,
> &queue_iosched_entry.attr,
> diff --git a/block/blk.h b/block/blk.h
> index ae82f2ac4019..d1ea4bd9b9a3 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -100,6 +100,8 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
> struct bio *bio);
> bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
> struct bio *bio);
> +bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
> + struct bio *bio);
> bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
> unsigned int *request_count,
> struct request **same_queue_rq);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index e0bac14347e6..aecca0e7d9ca 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -331,6 +331,7 @@ struct queue_limits {
> unsigned short logical_block_size;
> unsigned short max_segments;
> unsigned short max_integrity_segments;
> + unsigned short max_discard_segments;
>
> unsigned char misaligned;
> unsigned char discard_misaligned;
> @@ -1146,6 +1147,8 @@ extern void blk_queue_bounce_limit(struct request_queue *, u64);
> extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
> extern void blk_queue_chunk_sectors(struct request_queue *, unsigned int);
> extern void blk_queue_max_segments(struct request_queue *, unsigned short);
> +extern void blk_queue_max_discard_segments(struct request_queue *,
> + unsigned short);
> extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
> extern void blk_queue_max_discard_sectors(struct request_queue *q,
> unsigned int max_discard_sectors);
> @@ -1189,6 +1192,15 @@ extern void blk_queue_rq_timeout(struct request_queue *, unsigned int);
> extern void blk_queue_flush_queueable(struct request_queue *q, bool queueable);
> extern void blk_queue_write_cache(struct request_queue *q, bool enabled, bool fua);
>
> +/*
> + * Number of physical segments as sent to the device.
> + *
> + * Normally this is the number of discontiguous data segments sent by the
> + * submitter. But for data-less command like discard we might have no
> + * actual data segments submitted, but the driver might have to add it's
> + * own special payload. In that case we still return 1 here so that this
> + * special payload will be mapped.
> + */
> static inline unsigned short blk_rq_nr_phys_segments(struct request *rq)
> {
> if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
> @@ -1196,6 +1208,15 @@ static inline unsigned short blk_rq_nr_phys_segments(struct request *rq)
> return rq->nr_phys_segments;
> }
>
> +/*
> + * Number of discard segments (or ranges) the driver needs to fill in.
> + * Each discard bio merged into a request is counted as one segment.
> + */
> +static inline unsigned short blk_rq_nr_discard_segments(struct request *rq)
> +{
> + return max_t(unsigned short, rq->nr_phys_segments, 1);
> +}
> +
> extern int blk_rq_map_sg(struct request_queue *, struct request *, struct scatterlist *);
> extern void blk_dump_rq_flags(struct request *, char *);
> extern long nr_blockdev_pages(void);
> @@ -1384,6 +1405,11 @@ static inline unsigned short queue_max_segments(struct request_queue *q)
> return q->limits.max_segments;
> }
>
> +static inline unsigned short queue_max_discard_segments(struct request_queue *q)
> +{
> + return q->limits.max_discard_segments;
> +}
> +
> static inline unsigned int queue_max_segment_size(struct request_queue *q)
> {
> return q->limits.max_segment_size;
> diff --git a/include/linux/elevator.h b/include/linux/elevator.h
> index b38b4e651ea6..8265b6330cc2 100644
> --- a/include/linux/elevator.h
> +++ b/include/linux/elevator.h
> @@ -16,6 +16,7 @@ enum elv_merge {
> ELEVATOR_NO_MERGE = 0,
> ELEVATOR_FRONT_MERGE = 1,
> ELEVATOR_BACK_MERGE = 2,
> + ELEVATOR_DISCARD_MERGE = 3,
> };
>
> typedef enum elv_merge (elevator_merge_fn) (struct request_queue *, struct request **,
> --
> 2.11.0
>
--
Ming Lei
^ permalink raw reply
* RE: [PATCH] genhd: Do not hold event lock when scheduling workqueue elements
From: Dexuan Cui @ 2017-02-08 10:48 UTC (permalink / raw)
To: Jens Axboe, Bart Van Assche, hare@suse.com, hare@suse.de,
Martin K. Petersen
Cc: hch@lst.de, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, jth@kernel.org
In-Reply-To: <9199d528-f220-5b77-d657-c510ca210067@kernel.dk>
PiBGcm9tOiBKZW5zIEF4Ym9lIFttYWlsdG86YXhib2VAa2VybmVsLmRrXQ0KPiBTZW50OiBXZWRu
ZXNkYXksIEZlYnJ1YXJ5IDgsIDIwMTcgMDA6MDkNCj4gVG86IERleHVhbiBDdWkgPGRlY3VpQG1p
Y3Jvc29mdC5jb20+OyBCYXJ0IFZhbiBBc3NjaGUNCj4gPEJhcnQuVmFuQXNzY2hlQHNhbmRpc2su
Y29tPjsgaGFyZUBzdXNlLmNvbTsgaGFyZUBzdXNlLmRlDQo+IENjOiBoY2hAbHN0LmRlOyBsaW51
eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBsaW51eC1ibG9ja0B2Z2VyLmtlcm5lbC5vcmc7DQo+
IGp0aEBrZXJuZWwub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0hdIGdlbmhkOiBEbyBub3QgaG9s
ZCBldmVudCBsb2NrIHdoZW4gc2NoZWR1bGluZyB3b3JrcXVldWUNCj4gZWxlbWVudHMNCj4gDQo+
IE9uIDAyLzA2LzIwMTcgMTE6MjkgUE0sIERleHVhbiBDdWkgd3JvdGU6DQo+ID4+IEZyb206IGxp
bnV4LWJsb2NrLW93bmVyQHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LWJsb2NrLQ0KPiA+
PiBvd25lckB2Z2VyLmtlcm5lbC5vcmddIE9uIEJlaGFsZiBPZiBEZXh1YW4gQ3VpDQo+ID4+IHdp
dGggdGhlIGxpbnV4LW5leHQga2VybmVsLg0KPiA+Pg0KPiA+PiBJIGNhbiBib290IHRoZSBndWVz
dCB3aXRoIGxpbnV4LW5leHQncyBuZXh0LTIwMTcwMTMwIHdpdGhvdXQgYW55IGlzc3VlLA0KPiA+
PiBidXQgc2luY2UgbmV4dC0yMDE3MDEzMSBJIGhhdmVuJ3Qgc3VjY2VlZGVkIGluIGJvb3Rpbmcg
dGhlIGd1ZXN0Lg0KPiA+Pg0KPiA+PiBXaXRoIG5leHQtMjAxNzAyMDMgKG1lbnRpb25lZCBpbiBt
eSBtYWlsIGxhc3QgRnJpZGF5KSwgSSBnb3QgdGhlIHNhbWUNCj4gPj4gY2FsbHRyYWNlIGFzIEhh
bm5lcy4NCj4gPj4NCj4gPj4gV2l0aCB0b2RheSdzIGxpbnV4LW5leHQgKG5leHQtMjAxNzAyMDYp
LCBhY3R1YWxseSB0aGUgY2FsbHRyYWNlIGNoYW5nZWQgdG8NCj4gPj4gdGhlIGJlbG93Lg0KPiA+
PiBbICAxMjIuMDIzMDM2XSAgPyByZW1vdmVfd2FpdF9xdWV1ZSsweDcwLzB4NzANCj4gPj4gWyAg
MTIyLjA1MTM4M10gIGFzeW5jX3N5bmNocm9uaXplX2Z1bGwrMHgxNy8weDIwDQo+ID4+IFsgIDEy
Mi4wNzY5MjVdICBkb19pbml0X21vZHVsZSsweGMxLzB4MWY5DQo+ID4+IFsgIDEyMi4wOTc1MzBd
ICBsb2FkX21vZHVsZSsweDI0YmMvMHgyOTgwDQo+ID4NCj4gPiBJIGRvbid0IGtub3cgd2h5IGl0
IGhhbmdzIGhlcmUsIGJ1dCB0aGlzIGlzIHRoZSBzYW1lIGNhbGx0cmFjZSBpbiBteQ0KPiA+IGxh
c3QtRnJpZGF5IG1haWwsIHdoaWNoIGNvbnRhaW5zIDIgY2FsbHRyYWNlcy4gSXQgbG9va3MgdGhl
IG90aGVyIGNhbGx0cmFjZSBoYXMNCj4gPiBiZWVuIHJlc29sdmVkIGJ5IHNvbWUgY2hhbmdlcyBi
ZXR3ZWVuIG5leHQtMjAxNzAyMDMgYW5kIHRvZGF5Lg0KPiA+DQo+ID4gSGVyZSB0aGUga2VybmVs
IGlzIHRyeWluZyB0byBsb2FkIHRoZSBIeXBlci1WIHN0b3JhZ2UgZHJpdmVyIChodl9zdG9ydnNj
KSwgYW5kDQo+ID4gdGhlIGRyaXZlcidzIF9faW5pdCBhbmQgLnByb2JlIGhhdmUgZmluaXNoZWQg
c3VjY2Vzc2Z1bGx5IGFuZCB0aGVuIHRoZSBrZXJuZWwNCj4gPiBoYW5ncyBoZXJlLg0KPiA+DQo+
ID4gSSBiZWxpZXZlIHNvbWV0aGluZyBpcyBicm9rZW4gcmVjZW50bHksIGJlY2F1c2UgSSBkb24n
dCBoYXZlIGFueSBpc3N1ZSBiZWZvcmUNCj4gPiBKYW4gMzEuDQo+IA0KPiBDYW4geW91IHRyeSBh
bmQgYmlzZWN0IGl0Pw0KPiANCj4gSmVucyBBeGJvZQ0KDQpJIGJpc2VjdGVkIGl0IG9uIHRoZSBi
cmFuY2ggZm9yLTQuMTEvbmV4dCBvZiB0aGUgbGludXgtYmxvY2sgcmVwbyBhbmQgdGhlIGxvZyBz
aG93cw0KdGhlIGZpcnN0IGJhZCBjb21taXQgaXMgDQpbZTljNzg3ZTZdIHNjc2k6IGFsbG9jYXRl
IHNjc2lfY21uZCBzdHJ1Y3R1cmVzIGFzIHBhcnQgb2Ygc3RydWN0IHJlcXVlc3QNCg0KIyBnaXQg
YmlzZWN0IGxvZw0KZ2l0IGJpc2VjdCBzdGFydA0KIyBiYWQ6IFs4MGM2YjE1NzMyZjBkODgzMDAz
MjE0OWNiY2JjOGQ2N2UwNzRiNWU4XSBibGstbXEtc2NoZWQ6ICh1bilyZWdpc3RlciBlbGV2YXRv
ciB3aGVuICh1bilyZWdpc3RlcmluZyBxdWV1ZQ0KZ2l0IGJpc2VjdCBiYWQgODBjNmIxNTczMmYw
ZDg4MzAwMzIxNDljYmNiYzhkNjdlMDc0YjVlOA0KIyBnb29kOiBbMzA5YmQ5NmFmOWUyNmRhMzAz
ODY2MWJmNWNkYWQ3ODBlZWY0OWRkOV0gbWQ6IGNsZWFudXAgYmlvIG9wIC8gZmxhZ3MgaGFuZGxp
bmcgaW4gcmFpZDFfd3JpdGVfcmVxdWVzdA0KZ2l0IGJpc2VjdCBnb29kIDMwOWJkOTZhZjllMjZk
YTMwMzg2NjFiZjVjZGFkNzgwZWVmNDlkZDkNCiMgYmFkOiBbMjc0MTBhODkyN2ZiODliZDE1MGRl
MDhkNzQ5YThlZDdmNjdiNzczOV0gbmJkOiByZW1vdmUgUkVRX1RZUEVfRFJWX1BSSVYgbGVmdG92
ZXJzDQpnaXQgYmlzZWN0IGJhZCAyNzQxMGE4OTI3ZmI4OWJkMTUwZGUwOGQ3NDlhOGVkN2Y2N2I3
NzM5DQojIGJhZDogW2U5Yzc4N2U2NWMwYzM2NTI5NzQ1YmU0N2Q0OTBkOTk4YjRiNmU1ODldIHNj
c2k6IGFsbG9jYXRlIHNjc2lfY21uZCBzdHJ1Y3R1cmVzIGFzIHBhcnQgb2Ygc3RydWN0IHJlcXVl
c3QNCmdpdCBiaXNlY3QgYmFkIGU5Yzc4N2U2NWMwYzM2NTI5NzQ1YmU0N2Q0OTBkOTk4YjRiNmU1
ODkNCiMgZ29vZDogWzMyNzgyNTU3NDEzMjZiNmQ2NmQ4Y2E3ZDFjYjJjNTc2MzNlZTQzZDldIHNj
c2lfZGhfcmRhYzogc3dpdGNoIHRvIHNjc2lfZXhlY3V0ZV9yZXFfZmxhZ3MoKQ0KZ2l0IGJpc2Vj
dCBnb29kIDMyNzgyNTU3NDEzMjZiNmQ2NmQ4Y2E3ZDFjYjJjNTc2MzNlZTQzZDkNCiMgZ29vZDog
WzBmYmMzZTBmZjYyM2YxMDEyZTdjMmFmOTZlNzgxZWViMjZiY2MwZDddIHNjc2k6IHJlbW92ZSBn
ZnBfZmxhZ3MgbWVtYmVyIGluIHNjc2lfaG9zdF9jbWRfcG9vbA0KZ2l0IGJpc2VjdCBnb29kIDBm
YmMzZTBmZjYyM2YxMDEyZTdjMmFmOTZlNzgxZWViMjZiY2MwZDcNCiMgZ29vZDogW2VlZmY2OGM1
NjE4YzhkMDkyMGIxNDUzM2M3MGIyZGYwMDdiZDk0YjRdIHNjc2k6IHJlbW92ZSBzY3NpX2NtZF9k
bWFfcG9vbA0KZ2l0IGJpc2VjdCBnb29kIGVlZmY2OGM1NjE4YzhkMDkyMGIxNDUzM2M3MGIyZGYw
MDdiZDk0YjQNCiMgZ29vZDogW2Q0ODc3N2E2MzNkNmZhN2NjZGUwZjBlNjUwOWYwYzAxZmJmYzUy
OTldIHNjc2k6IHJlbW92ZSBfX3Njc2lfYWxsb2NfcXVldWUNCmdpdCBiaXNlY3QgZ29vZCBkNDg3
NzdhNjMzZDZmYTdjY2RlMGYwZTY1MDlmMGMwMWZiZmM1Mjk5DQojIGZpcnN0IGJhZCBjb21taXQ6
IFtlOWM3ODdlNjVjMGMzNjUyOTc0NWJlNDdkNDkwZDk5OGI0YjZlNTg5XSBzY3NpOiBhbGxvY2F0
ZSBzY3NpX2NtbmQgc3RydWN0dXJlcyBhcyBwYXJ0IG9mIHN0cnVjdCByZXF1ZXN0DQoNClRoYW5r
cywNCi0tIERleHVhbg0K
^ permalink raw reply
* Re: [PATCH] bfq-mq: cause deadlock by executing exit_icq body immediately
From: Paolo Valente @ 2017-02-08 10:39 UTC (permalink / raw)
To: Omar Sandoval
Cc: Jens Axboe, Tejun Heo, linux-block, Linux-Kernal, Ulf Hansson,
Linus Walleij, broonie
In-Reply-To: <20170208103342.GA4647@vader>
> Il giorno 08 feb 2017, alle ore 11:33, Omar Sandoval =
<osandov@osandov.com> ha scritto:
>=20
> On Wed, Feb 08, 2017 at 11:03:01AM +0100, Paolo Valente wrote:
>>=20
>>> Il giorno 07 feb 2017, alle ore 22:45, Omar Sandoval =
<osandov@osandov.com> ha scritto:
>>>=20
>>> On Tue, Feb 07, 2017 at 06:33:46PM +0100, Paolo Valente wrote:
>>>> Hi,
>>>> this patch is meant to show that, if the body of the hook exit_icq =
is executed
>>>> from inside that hook, and not as deferred work, then a circular =
deadlock
>>>> occurs.
>>>>=20
>>>> It happens if, on a CPU
>>>> - the body of icq_exit takes the scheduler lock,
>>>> - it does so from inside the exit_icq hook, which is invoked with =
the queue
>>>> lock held
>>>>=20
>>>> while, on another CPU
>>>> - bfq_bio_merge, after taking the scheduler lock, invokes =
bfq_bic_lookup,
>>>> which, in its turn, takes the queue lock. bfq_bic_lookup needs to =
take such a
>>>> lock, because it invokes ioc_lookup_icq.
>>>>=20
>>>> For more details, here is a lockdep report, right before the =
deadlock did occur.
>>>>=20
>>>> [ 44.059877] =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
>>>> [ 44.124922] [ INFO: possible circular locking dependency =
detected ]
>>>> [ 44.125795] 4.10.0-rc5-bfq-mq+ #38 Not tainted
>>>> [ 44.126414] =
-------------------------------------------------------
>>>> [ 44.127291] sync/2043 is trying to acquire lock:
>>>> [ 44.128918] (&(&bfqd->lock)->rlock){-.-...}, at: =
[<ffffffff90484195>] bfq_exit_icq_bfqq+0x55/0x140
>>>> [ 44.134052]
>>>> [ 44.134052] but task is already holding lock:
>>>> [ 44.134868] (&(&q->__queue_lock)->rlock){-.....}, at: =
[<ffffffff9044738e>] put_io_context_active+0x6e/0xc0
>>>=20
>>> Hey, Paolo,
>>>=20
>>> I only briefly skimmed the code, but what are you using the =
queue_lock
>>> for? You should just use your scheduler lock everywhere. blk-mq =
doesn't
>>> use the queue lock, so the scheduler is the only thing you need =
mutual
>>> exclusion against.
>>=20
>> Hi Omar,
>> the cause of the problem is that the hook functions bfq_request_merge
>> and bfq_allow_bio_merge invoke, directly or through other functions,
>> the function bfq_bic_lookup, which, in its turn, invokes
>> ioc_lookup_icq. The latter must be invoked with the queue lock held.
>> In particular the offending lines in bfq_bic_lookup are:
>>=20
>> spin_lock_irqsave(q->queue_lock, flags);
>> icq =3D icq_to_bic(ioc_lookup_icq(ioc, q));
>> spin_unlock_irqrestore(q->queue_lock, flags);
>>=20
>> Maybe I'm missing something and we can avoid taking this lock?
>=20
> Ah, I didn't realize we still used the q->queue_lock for the icq =
stuff.
> You're right, you still need that lock for ioc_lookup_icq(). Unless
> there's something else I'm forgetting, that should be the only thing =
you
> need it for in the core code, and you should use your scheduler lock =
for
> everything else. What else are you using q->queue_lock for?=20
Nothing. The deadlock follows from that bfq_request_merge gets called
with the scheduler lock already held. Problematic paths start from:
bfq_bio_merge and bfq_insert_request.
I'm trying to understand whether I/we can reorder operations in some
way that avoids the nested locking, but at no avail so far.
Thanks,
Paolo=
^ permalink raw reply
* Re: [PATCH] bfq-mq: cause deadlock by executing exit_icq body immediately
From: Omar Sandoval @ 2017-02-08 10:33 UTC (permalink / raw)
To: Paolo Valente
Cc: Jens Axboe, Tejun Heo, linux-block, Linux-Kernal, Ulf Hansson,
Linus Walleij, broonie
In-Reply-To: <A064B1DA-C96B-4EA9-A412-F2A6D8FB295A@linaro.org>
On Wed, Feb 08, 2017 at 11:03:01AM +0100, Paolo Valente wrote:
>
> > Il giorno 07 feb 2017, alle ore 22:45, Omar Sandoval <osandov@osandov.com> ha scritto:
> >
> > On Tue, Feb 07, 2017 at 06:33:46PM +0100, Paolo Valente wrote:
> >> Hi,
> >> this patch is meant to show that, if the body of the hook exit_icq is executed
> >> from inside that hook, and not as deferred work, then a circular deadlock
> >> occurs.
> >>
> >> It happens if, on a CPU
> >> - the body of icq_exit takes the scheduler lock,
> >> - it does so from inside the exit_icq hook, which is invoked with the queue
> >> lock held
> >>
> >> while, on another CPU
> >> - bfq_bio_merge, after taking the scheduler lock, invokes bfq_bic_lookup,
> >> which, in its turn, takes the queue lock. bfq_bic_lookup needs to take such a
> >> lock, because it invokes ioc_lookup_icq.
> >>
> >> For more details, here is a lockdep report, right before the deadlock did occur.
> >>
> >> [ 44.059877] ======================================================
> >> [ 44.124922] [ INFO: possible circular locking dependency detected ]
> >> [ 44.125795] 4.10.0-rc5-bfq-mq+ #38 Not tainted
> >> [ 44.126414] -------------------------------------------------------
> >> [ 44.127291] sync/2043 is trying to acquire lock:
> >> [ 44.128918] (&(&bfqd->lock)->rlock){-.-...}, at: [<ffffffff90484195>] bfq_exit_icq_bfqq+0x55/0x140
> >> [ 44.134052]
> >> [ 44.134052] but task is already holding lock:
> >> [ 44.134868] (&(&q->__queue_lock)->rlock){-.....}, at: [<ffffffff9044738e>] put_io_context_active+0x6e/0xc0
> >
> > Hey, Paolo,
> >
> > I only briefly skimmed the code, but what are you using the queue_lock
> > for? You should just use your scheduler lock everywhere. blk-mq doesn't
> > use the queue lock, so the scheduler is the only thing you need mutual
> > exclusion against.
>
> Hi Omar,
> the cause of the problem is that the hook functions bfq_request_merge
> and bfq_allow_bio_merge invoke, directly or through other functions,
> the function bfq_bic_lookup, which, in its turn, invokes
> ioc_lookup_icq. The latter must be invoked with the queue lock held.
> In particular the offending lines in bfq_bic_lookup are:
>
> spin_lock_irqsave(q->queue_lock, flags);
> icq = icq_to_bic(ioc_lookup_icq(ioc, q));
> spin_unlock_irqrestore(q->queue_lock, flags);
>
> Maybe I'm missing something and we can avoid taking this lock?
Ah, I didn't realize we still used the q->queue_lock for the icq stuff.
You're right, you still need that lock for ioc_lookup_icq(). Unless
there's something else I'm forgetting, that should be the only thing you
need it for in the core code, and you should use your scheduler lock for
everything else. What else are you using q->queue_lock for?
^ permalink raw reply
* Re: [PATCH 0/4 v2] BDI lifetime fix
From: Jan Kara @ 2017-02-08 10:23 UTC (permalink / raw)
To: Tejun Heo
Cc: Jan Kara, Thiago Jung Bauermann, Jens Axboe, linux-block,
Christoph Hellwig, Dan Williams, Laurent Dufour
In-Reply-To: <20170208075142.GA26317@quack2.suse.cz>
On Wed 08-02-17 08:51:42, Jan Kara wrote:
> On Tue 07-02-17 12:21:01, Tejun Heo wrote:
> > Hello,
> >
> > On Tue, Feb 07, 2017 at 01:33:31PM +0100, Jan Kara wrote:
> > > > We can see above that inode->i_wb is in r31, and the machine crashed at
> > > > 0xc0000000003799a0 so it was trying to dereference wb and crashed.
> > > > r31 is NULL in the crash information.
> > >
> > > Thanks for report and the analysis. After some looking into the code I see
> > > where the problem is. Writeback code assumes inode->i_wb can never become
> > > invalid once it is set however we still call inode_detach_wb() from
> > > __blkdev_put(). So in a way this is a different problem but closely
> > > related.
> >
> > Heh, it feels like we're chasing our own tails.
>
> Pretty much. I went through the history of bdi registration and
> unregistration to understand various constraints and various different
> reasons keep pushing that around and always something breaks...
>
> > > It seems to me that instead of calling inode_detach_wb() in __blkdev_put()
> > > we may just switch blkdev inode to bdi->wb (it is now guaranteed to stay
> > > around). That way bdi_unregister() can complete (destroying all writeback
> > > structures except for bdi->wb) while block device inode can still live with
> > > a valid i_wb structure.
> >
> > So, the problem there would be synchronizing get_wb against the
> > transition. We can do that and inode_switch_wbs_work_fn() already
> > does it but it is a bit nasty.
>
> Yeah, I have prototyped that and it is relatively simple although we have
> to use synchronize_rcu() to be sure unlocked users of i_wb are done before
> switching and that is somewhat ugly. So I'm looking for ways to avoid the
> switching as well. Especially since from high-level POV the switching
> should not be necessary. Everything is going away and there is no real work
> to be done. Just we may be unlucky enough that e.g. flusher is looking
> whether there's some work to do and we remove stuff under its hands. So
> switching seems like a bit of an overkill.
>
> > I'm getting a bit confused here, so the reason we added
> > inode_detach_wb() in __blkdev_put() was because the root wb might go
> > away because it's embedded in the bdi which is embedded in the
> > request_queue which is put and may be released by put_disk().
> >
> > Are you saying that we changed the behavior so that bdi->wb stays
> > around? If so, we can just remove the inode_detach_wb() call, no?
>
> Yes, my patches (currently in linux-block) make bdi->wb stay around as long
> as the block device inode. However things are complicated by the fact that
> these days bdev_inode->i_wb may be pointing even to non-root wb_writeback
> structure. If that happens and we don't call inode_detach_wb(),
> bdi_unregister() will block waiting for reference count on wb_writeback to
> drop to zero which happens only once bdev inode is evicted from inode cache
> which may be far far in the future.
>
> Now I think we can move bdi_unregister() into del_gendisk() (where it IMHO
> belongs anyway as a counterpart to device_add_disk() in which we call
> bdi_register()) and shutdown the block device inode there before calling
> bdi_unregister(). But I'm still figuring out whether it will not break
> something else because the code has lots of interactions...
More news from device shutdown world ;): I was looking more into how device
shutdown works. I was wondering what happens when device gets hot-removed
and how do we shutdown stuff. If I tracked the callback maze correctly, when
we remove scsi disk, we do __scsi_remove_device() -> device_del() ->
bus_remove_device() -> device_release_driver() -> sd_remove() ->
del_gendisk(). We also have __scsi_remove_device() -> blk_cleanup_queue()
-> bdi_unregister() shortly after the previous happening
<DETOUR BEGIN>
This ordering seems to be the real culprit of the bdi name reuse problems
Omar has reported? Same as described in commit 6cd18e711dd8 for MD BTW and
Dan's patch could be IMHO replaced by a move of bdi_unregister() from
blk_cleanup_queue() to del_gendisk() where it logically belongs as a
counterpart of device_add_disk(). I'll test that.
<DETOUR END>
__scsi_remove_device() is also called when the device was hot-removed. At
that point the bdev can still be open and in active use and its i_wb can
point to some non-root wb_writeback struct. Thus bdi_unregister() will
block waiting for that wb_writeback to get released and thus SCSI device
removal will block basically intefinitely (at least until fs on top of bdev
gets unmounted). I believe this is a bug and __scsi_remove_device() is
expected to finish regardless of upper layers still using the bdev. So to
fix this I don't think we can really avoid the switching of bdev inode from
non-root wb_writeback structure to bdi->wb.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH] bfq-mq: cause deadlock by executing exit_icq body immediately
From: Paolo Valente @ 2017-02-08 10:03 UTC (permalink / raw)
To: Omar Sandoval
Cc: Jens Axboe, Tejun Heo, linux-block, Linux-Kernal, Ulf Hansson,
Linus Walleij, broonie
In-Reply-To: <20170207214516.GA14269@vader.DHCP.thefacebook.com>
> Il giorno 07 feb 2017, alle ore 22:45, Omar Sandoval =
<osandov@osandov.com> ha scritto:
>=20
> On Tue, Feb 07, 2017 at 06:33:46PM +0100, Paolo Valente wrote:
>> Hi,
>> this patch is meant to show that, if the body of the hook exit_icq =
is executed
>> from inside that hook, and not as deferred work, then a circular =
deadlock
>> occurs.
>>=20
>> It happens if, on a CPU
>> - the body of icq_exit takes the scheduler lock,
>> - it does so from inside the exit_icq hook, which is invoked with the =
queue
>> lock held
>>=20
>> while, on another CPU
>> - bfq_bio_merge, after taking the scheduler lock, invokes =
bfq_bic_lookup,
>> which, in its turn, takes the queue lock. bfq_bic_lookup needs to =
take such a
>> lock, because it invokes ioc_lookup_icq.
>>=20
>> For more details, here is a lockdep report, right before the deadlock =
did occur.
>>=20
>> [ 44.059877] =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> [ 44.124922] [ INFO: possible circular locking dependency detected =
]
>> [ 44.125795] 4.10.0-rc5-bfq-mq+ #38 Not tainted
>> [ 44.126414] =
-------------------------------------------------------
>> [ 44.127291] sync/2043 is trying to acquire lock:
>> [ 44.128918] (&(&bfqd->lock)->rlock){-.-...}, at: =
[<ffffffff90484195>] bfq_exit_icq_bfqq+0x55/0x140
>> [ 44.134052]
>> [ 44.134052] but task is already holding lock:
>> [ 44.134868] (&(&q->__queue_lock)->rlock){-.....}, at: =
[<ffffffff9044738e>] put_io_context_active+0x6e/0xc0
>=20
> Hey, Paolo,
>=20
> I only briefly skimmed the code, but what are you using the queue_lock
> for? You should just use your scheduler lock everywhere. blk-mq =
doesn't
> use the queue lock, so the scheduler is the only thing you need mutual
> exclusion against.
Hi Omar,
the cause of the problem is that the hook functions bfq_request_merge
and bfq_allow_bio_merge invoke, directly or through other functions,
the function bfq_bic_lookup, which, in its turn, invokes
ioc_lookup_icq. The latter must be invoked with the queue lock held.
In particular the offending lines in bfq_bic_lookup are:
spin_lock_irqsave(q->queue_lock, flags);
icq =3D icq_to_bic(ioc_lookup_icq(ioc, q));
spin_unlock_irqrestore(q->queue_lock, flags);
Maybe I'm missing something and we can avoid taking this lock?
Thanks,
Paolo
> I'm guessing if you stopped using that, your locking
> issues would go away.
^ permalink raw reply
* Re: [PATCH 0/4 v2] BDI lifetime fix
From: Jan Kara @ 2017-02-08 7:51 UTC (permalink / raw)
To: Tejun Heo
Cc: Jan Kara, Thiago Jung Bauermann, Jens Axboe, linux-block,
Christoph Hellwig, Dan Williams, Laurent Dufour
In-Reply-To: <20170207172101.GC6164@htj.duckdns.org>
On Tue 07-02-17 12:21:01, Tejun Heo wrote:
> Hello,
>
> On Tue, Feb 07, 2017 at 01:33:31PM +0100, Jan Kara wrote:
> > > We can see above that inode->i_wb is in r31, and the machine crashed at
> > > 0xc0000000003799a0 so it was trying to dereference wb and crashed.
> > > r31 is NULL in the crash information.
> >
> > Thanks for report and the analysis. After some looking into the code I see
> > where the problem is. Writeback code assumes inode->i_wb can never become
> > invalid once it is set however we still call inode_detach_wb() from
> > __blkdev_put(). So in a way this is a different problem but closely
> > related.
>
> Heh, it feels like we're chasing our own tails.
Pretty much. I went through the history of bdi registration and
unregistration to understand various constraints and various different
reasons keep pushing that around and always something breaks...
> > It seems to me that instead of calling inode_detach_wb() in __blkdev_put()
> > we may just switch blkdev inode to bdi->wb (it is now guaranteed to stay
> > around). That way bdi_unregister() can complete (destroying all writeback
> > structures except for bdi->wb) while block device inode can still live with
> > a valid i_wb structure.
>
> So, the problem there would be synchronizing get_wb against the
> transition. We can do that and inode_switch_wbs_work_fn() already
> does it but it is a bit nasty.
Yeah, I have prototyped that and it is relatively simple although we have
to use synchronize_rcu() to be sure unlocked users of i_wb are done before
switching and that is somewhat ugly. So I'm looking for ways to avoid the
switching as well. Especially since from high-level POV the switching
should not be necessary. Everything is going away and there is no real work
to be done. Just we may be unlucky enough that e.g. flusher is looking
whether there's some work to do and we remove stuff under its hands. So
switching seems like a bit of an overkill.
> I'm getting a bit confused here, so the reason we added
> inode_detach_wb() in __blkdev_put() was because the root wb might go
> away because it's embedded in the bdi which is embedded in the
> request_queue which is put and may be released by put_disk().
>
> Are you saying that we changed the behavior so that bdi->wb stays
> around? If so, we can just remove the inode_detach_wb() call, no?
Yes, my patches (currently in linux-block) make bdi->wb stay around as long
as the block device inode. However things are complicated by the fact that
these days bdev_inode->i_wb may be pointing even to non-root wb_writeback
structure. If that happens and we don't call inode_detach_wb(),
bdi_unregister() will block waiting for reference count on wb_writeback to
drop to zero which happens only once bdev inode is evicted from inode cache
which may be far far in the future.
Now I think we can move bdi_unregister() into del_gendisk() (where it IMHO
belongs anyway as a counterpart to device_add_disk() in which we call
bdi_register()) and shutdown the block device inode there before calling
bdi_unregister(). But I'm still figuring out whether it will not break
something else because the code has lots of interactions...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 1/9] virtio_pci: remove struct virtio_pci_vq_info
From: Jason Wang @ 2017-02-08 2:52 UTC (permalink / raw)
To: Christoph Hellwig
Cc: mst, axboe, pbonzini, virtualization, linux-block, linux-kernel
In-Reply-To: <20170207093851.GA15267@lst.de>
On 2017年02月07日 17:38, Christoph Hellwig wrote:
> On Tue, Feb 07, 2017 at 03:17:02PM +0800, Jason Wang wrote:
>> The check is still there.
> Meh, I could swear I fixed it up. Here is an updated version:
>
> ---
> From bf5e3b7fd272aea32388570503f00d0ab592fc2a Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Wed, 25 Jan 2017 13:40:21 +0100
> Subject: virtio_pci: remove struct virtio_pci_vq_info
>
> We don't really need struct virtio_pci_vq_info, as most field in there
> are redundant:
>
> - the vq backpointer is not strictly neede to start with
> - the entry in the vqs list is not needed - the generic virtqueue already
> has list, we only need to check if it has a callback to get the same
> semantics
> - we can use a simple array to look up the MSI-X vec if needed.
> - That simple array now also duoble serves to replace the per_vq_vectors
> flag
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/virtio/virtio_pci_common.c | 117 +++++++++++--------------------------
> drivers/virtio/virtio_pci_common.h | 25 +-------
> drivers/virtio/virtio_pci_legacy.c | 6 +-
> drivers/virtio/virtio_pci_modern.c | 6 +-
> 4 files changed, 39 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 186cbab327b8..1f9fac7dad61 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -62,16 +62,13 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
> static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
> {
> struct virtio_pci_device *vp_dev = opaque;
> - struct virtio_pci_vq_info *info;
> irqreturn_t ret = IRQ_NONE;
> - unsigned long flags;
> + struct virtqueue *vq;
>
> - spin_lock_irqsave(&vp_dev->lock, flags);
> - list_for_each_entry(info, &vp_dev->virtqueues, node) {
> - if (vring_interrupt(irq, info->vq) == IRQ_HANDLED)
> + list_for_each_entry(vq, &vp_dev->vdev.vqs, list) {
> + if (vring_interrupt(irq, vq) == IRQ_HANDLED)
> ret = IRQ_HANDLED;
> }
> - spin_unlock_irqrestore(&vp_dev->lock, flags);
>
> return ret;
> }
> @@ -167,55 +164,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> return err;
> }
>
> -static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
> - void (*callback)(struct virtqueue *vq),
> - const char *name,
> - u16 msix_vec)
> -{
> - struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> - struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
> - struct virtqueue *vq;
> - unsigned long flags;
> -
> - /* fill out our structure that represents an active queue */
> - if (!info)
> - return ERR_PTR(-ENOMEM);
> -
> - vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, msix_vec);
> - if (IS_ERR(vq))
> - goto out_info;
> -
> - info->vq = vq;
> - if (callback) {
> - spin_lock_irqsave(&vp_dev->lock, flags);
> - list_add(&info->node, &vp_dev->virtqueues);
> - spin_unlock_irqrestore(&vp_dev->lock, flags);
> - } else {
> - INIT_LIST_HEAD(&info->node);
> - }
> -
> - vp_dev->vqs[index] = info;
> - return vq;
> -
> -out_info:
> - kfree(info);
> - return vq;
> -}
> -
> -static void vp_del_vq(struct virtqueue *vq)
> -{
> - struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> - struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> - unsigned long flags;
> -
> - spin_lock_irqsave(&vp_dev->lock, flags);
> - list_del(&info->node);
> - spin_unlock_irqrestore(&vp_dev->lock, flags);
> -
> - vp_dev->del_vq(info);
> - kfree(info);
> -}
> -
> /* the config->del_vqs() implementation */
> void vp_del_vqs(struct virtio_device *vdev)
> {
> @@ -224,16 +172,15 @@ void vp_del_vqs(struct virtio_device *vdev)
> int i;
>
> list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
> - if (vp_dev->per_vq_vectors) {
> - int v = vp_dev->vqs[vq->index]->msix_vector;
> + if (vp_dev->msix_vector_map) {
> + int v = vp_dev->msix_vector_map[vq->index];
>
> if (v != VIRTIO_MSI_NO_VECTOR)
> free_irq(pci_irq_vector(vp_dev->pci_dev, v),
> vq);
> }
> - vp_del_vq(vq);
> + vp_dev->del_vq(vq);
> }
> - vp_dev->per_vq_vectors = false;
>
> if (vp_dev->intx_enabled) {
> free_irq(vp_dev->pci_dev->irq, vp_dev);
> @@ -261,8 +208,8 @@ void vp_del_vqs(struct virtio_device *vdev)
> vp_dev->msix_names = NULL;
> kfree(vp_dev->msix_affinity_masks);
> vp_dev->msix_affinity_masks = NULL;
> - kfree(vp_dev->vqs);
> - vp_dev->vqs = NULL;
> + kfree(vp_dev->msix_vector_map);
> + vp_dev->msix_vector_map = NULL;
> }
>
> static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
> @@ -275,10 +222,6 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
> u16 msix_vec;
> int i, err, nvectors, allocated_vectors;
>
> - vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
> - if (!vp_dev->vqs)
> - return -ENOMEM;
> -
> if (per_vq_vectors) {
> /* Best option: one for change interrupt, one per vq. */
> nvectors = 1;
> @@ -294,7 +237,13 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
> if (err)
> goto error_find;
>
> - vp_dev->per_vq_vectors = per_vq_vectors;
> + if (per_vq_vectors) {
> + vp_dev->msix_vector_map = kmalloc_array(nvqs,
> + sizeof(*vp_dev->msix_vector_map), GFP_KERNEL);
> + if (!vp_dev->msix_vector_map)
> + goto error_find;
> + }
> +
> allocated_vectors = vp_dev->msix_used_vectors;
> for (i = 0; i < nvqs; ++i) {
> if (!names[i]) {
> @@ -304,19 +253,25 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
>
> if (!callbacks[i])
> msix_vec = VIRTIO_MSI_NO_VECTOR;
> - else if (vp_dev->per_vq_vectors)
> + else if (per_vq_vectors)
> msix_vec = allocated_vectors++;
> else
> msix_vec = VP_MSIX_VQ_VECTOR;
> - vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i], msix_vec);
> + vqs[i] = vp_dev->setup_vq(vp_dev, i, callbacks[i], names[i],
> + msix_vec);
> if (IS_ERR(vqs[i])) {
> err = PTR_ERR(vqs[i]);
> goto error_find;
> }
>
> - if (!vp_dev->per_vq_vectors || msix_vec == VIRTIO_MSI_NO_VECTOR)
> + if (!per_vq_vectors)
> continue;
>
> + if (msix_vec == VIRTIO_MSI_NO_VECTOR) {
> + vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR;
> + continue;
> + }
> +
> /* allocate per-vq irq if available and necessary */
> snprintf(vp_dev->msix_names[msix_vec],
> sizeof *vp_dev->msix_names,
> @@ -326,8 +281,12 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
> vring_interrupt, 0,
> vp_dev->msix_names[msix_vec],
> vqs[i]);
> - if (err)
> + if (err) {
> + /* don't free this irq on error */
> + vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR;
> goto error_find;
> + }
> + vp_dev->msix_vector_map[i] = msix_vec;
> }
> return 0;
>
> @@ -343,23 +302,18 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> int i, err;
>
> - vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
> - if (!vp_dev->vqs)
> - return -ENOMEM;
> -
> err = request_irq(vp_dev->pci_dev->irq, vp_interrupt, IRQF_SHARED,
> dev_name(&vdev->dev), vp_dev);
> if (err)
> goto out_del_vqs;
>
> vp_dev->intx_enabled = 1;
> - vp_dev->per_vq_vectors = false;
> for (i = 0; i < nvqs; ++i) {
> if (!names[i]) {
> vqs[i] = NULL;
> continue;
> }
> - vqs[i] = vp_setup_vq(vdev, i, callbacks[i], names[i],
> + vqs[i] = vp_dev->setup_vq(vp_dev, i, callbacks[i], names[i],
> VIRTIO_MSI_NO_VECTOR);
> if (IS_ERR(vqs[i])) {
> err = PTR_ERR(vqs[i]);
> @@ -409,16 +363,15 @@ int vp_set_vq_affinity(struct virtqueue *vq, int cpu)
> {
> struct virtio_device *vdev = vq->vdev;
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> - struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> - struct cpumask *mask;
> - unsigned int irq;
>
> if (!vq->callback)
> return -EINVAL;
>
> if (vp_dev->msix_enabled) {
> - mask = vp_dev->msix_affinity_masks[info->msix_vector];
> - irq = pci_irq_vector(vp_dev->pci_dev, info->msix_vector);
> + int vec = vp_dev->msix_vector_map[vq->index];
> + struct cpumask *mask = vp_dev->msix_affinity_masks[vec];
> + unsigned int irq = pci_irq_vector(vp_dev->pci_dev, vec);
> +
> if (cpu == -1)
> irq_set_affinity_hint(irq, NULL);
> else {
> @@ -498,8 +451,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> vp_dev->vdev.dev.parent = &pci_dev->dev;
> vp_dev->vdev.dev.release = virtio_pci_release_dev;
> vp_dev->pci_dev = pci_dev;
> - INIT_LIST_HEAD(&vp_dev->virtqueues);
> - spin_lock_init(&vp_dev->lock);
>
> /* enable the device */
> rc = pci_enable_device(pci_dev);
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index b2f666250ae0..2038887bdf23 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -31,17 +31,6 @@
> #include <linux/highmem.h>
> #include <linux/spinlock.h>
>
> -struct virtio_pci_vq_info {
> - /* the actual virtqueue */
> - struct virtqueue *vq;
> -
> - /* the list node for the virtqueues list */
> - struct list_head node;
> -
> - /* MSI-X vector (or none) */
> - unsigned msix_vector;
> -};
> -
> /* Our device structure */
> struct virtio_pci_device {
> struct virtio_device vdev;
> @@ -75,13 +64,6 @@ struct virtio_pci_device {
> /* the IO mapping for the PCI config space */
> void __iomem *ioaddr;
>
> - /* a list of queues so we can dispatch IRQs */
> - spinlock_t lock;
> - struct list_head virtqueues;
> -
> - /* array of all queues for house-keeping */
> - struct virtio_pci_vq_info **vqs;
> -
> /* MSI-X support */
> int msix_enabled;
> int intx_enabled;
> @@ -94,16 +76,15 @@ struct virtio_pci_device {
> /* Vectors allocated, excluding per-vq vectors if any */
> unsigned msix_used_vectors;
>
> - /* Whether we have vector per vq */
> - bool per_vq_vectors;
> + /* Map of per-VQ MSI-X vectors, may be NULL */
> + unsigned *msix_vector_map;
>
> struct virtqueue *(*setup_vq)(struct virtio_pci_device *vp_dev,
> - struct virtio_pci_vq_info *info,
> unsigned idx,
> void (*callback)(struct virtqueue *vq),
> const char *name,
> u16 msix_vec);
> - void (*del_vq)(struct virtio_pci_vq_info *info);
> + void (*del_vq)(struct virtqueue *vq);
>
> u16 (*config_vector)(struct virtio_pci_device *vp_dev, u16 vector);
> };
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 6d9e5173d5fa..47292dad0ff9 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -112,7 +112,6 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> }
>
> static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> - struct virtio_pci_vq_info *info,
> unsigned index,
> void (*callback)(struct virtqueue *vq),
> const char *name,
> @@ -130,8 +129,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> if (!num || ioread32(vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN))
> return ERR_PTR(-ENOENT);
>
> - info->msix_vector = msix_vec;
> -
> /* create the vring */
> vq = vring_create_virtqueue(index, num,
> VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,
> @@ -162,9 +159,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> return ERR_PTR(err);
> }
>
> -static void del_vq(struct virtio_pci_vq_info *info)
> +static void del_vq(struct virtqueue *vq)
> {
> - struct virtqueue *vq = info->vq;
> struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
>
> iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 4bf7ab375894..00e6fc1df407 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -293,7 +293,6 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> }
>
> static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> - struct virtio_pci_vq_info *info,
> unsigned index,
> void (*callback)(struct virtqueue *vq),
> const char *name,
> @@ -323,8 +322,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> /* get offset of notification word for this vq */
> off = vp_ioread16(&cfg->queue_notify_off);
>
> - info->msix_vector = msix_vec;
> -
> /* create the vring */
> vq = vring_create_virtqueue(index, num,
> SMP_CACHE_BYTES, &vp_dev->vdev,
> @@ -409,9 +406,8 @@ static int vp_modern_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> return 0;
> }
>
> -static void del_vq(struct virtio_pci_vq_info *info)
> +static void del_vq(struct virtqueue *vq)
> {
> - struct virtqueue *vq = info->vq;
> struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
>
> vp_iowrite16(vq->index, &vp_dev->common->queue_select);
^ permalink raw reply
* Re: [lustre-devel] [PATCH 06/24] lustre: Convert to separately allocated bdi
From: Dilger, Andreas @ 2017-02-08 0:38 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
Drokin, Oleg, Christoph Hellwig, lustre-devel@lists.lustre.org
In-Reply-To: <20170202173422.3240-7-jack@suse.cz>
On Feb 2, 2017, at 10:34, Jan Kara <jack@suse.cz> wrote:
>
> Allocate struct backing_dev_info separately instead of embedding it
> inside superblock. This unifies handling of bdi among users.
>
> CC: Oleg Drokin <oleg.drokin@intel.com>
> CC: Andreas Dilger <andreas.dilger@intel.com>
> CC: James Simmons <jsimmons@infradead.org>
> CC: lustre-devel@lists.lustre.org
> Signed-off-by: Jan Kara <jack@suse.cz>
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
> ---
> .../staging/lustre/lustre/include/lustre_disk.h | 4 ----
> drivers/staging/lustre/lustre/llite/llite_lib.c | 24 +++-------------------
> 2 files changed, 3 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/lustre_disk.h b/drivers/staging/lustre/lustre/include/lustre_disk.h
> index 8886458748c1..a676bccabd43 100644
> --- a/drivers/staging/lustre/lustre/include/lustre_disk.h
> +++ b/drivers/staging/lustre/lustre/include/lustre_disk.h
> @@ -133,13 +133,9 @@ struct lustre_sb_info {
> struct obd_export *lsi_osd_exp;
> char lsi_osd_type[16];
> char lsi_fstype[16];
> - struct backing_dev_info lsi_bdi; /* each client mountpoint needs
> - * own backing_dev_info
> - */
> };
>
> #define LSI_UMOUNT_FAILOVER 0x00200000
> -#define LSI_BDI_INITIALIZED 0x00400000
>
> #define s2lsi(sb) ((struct lustre_sb_info *)((sb)->s_fs_info))
> #define s2lsi_nocast(sb) ((sb)->s_fs_info)
> diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
> index 25f5aed97f63..4f07d2e60d40 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> @@ -861,15 +861,6 @@ void ll_lli_init(struct ll_inode_info *lli)
> mutex_init(&lli->lli_layout_mutex);
> }
>
> -static inline int ll_bdi_register(struct backing_dev_info *bdi)
> -{
> - static atomic_t ll_bdi_num = ATOMIC_INIT(0);
> -
> - bdi->name = "lustre";
> - return bdi_register(bdi, NULL, "lustre-%d",
> - atomic_inc_return(&ll_bdi_num));
> -}
> -
> int ll_fill_super(struct super_block *sb, struct vfsmount *mnt)
> {
> struct lustre_profile *lprof = NULL;
> @@ -879,6 +870,7 @@ int ll_fill_super(struct super_block *sb, struct vfsmount *mnt)
> char *profilenm = get_profile_name(sb);
> struct config_llog_instance *cfg;
> int err;
> + static atomic_t ll_bdi_num = ATOMIC_INIT(0);
>
> CDEBUG(D_VFSTRACE, "VFS Op: sb %p\n", sb);
>
> @@ -901,16 +893,11 @@ int ll_fill_super(struct super_block *sb, struct vfsmount *mnt)
> if (err)
> goto out_free;
>
> - err = bdi_init(&lsi->lsi_bdi);
> - if (err)
> - goto out_free;
> - lsi->lsi_flags |= LSI_BDI_INITIALIZED;
> - lsi->lsi_bdi.capabilities = 0;
> - err = ll_bdi_register(&lsi->lsi_bdi);
> + err = super_setup_bdi_name(sb, "lustre-%d",
> + atomic_inc_return(&ll_bdi_num));
> if (err)
> goto out_free;
>
> - sb->s_bdi = &lsi->lsi_bdi;
> /* kernel >= 2.6.38 store dentry operations in sb->s_d_op. */
> sb->s_d_op = &ll_d_ops;
>
> @@ -1031,11 +1018,6 @@ void ll_put_super(struct super_block *sb)
> if (profilenm)
> class_del_profile(profilenm);
>
> - if (lsi->lsi_flags & LSI_BDI_INITIALIZED) {
> - bdi_destroy(&lsi->lsi_bdi);
> - lsi->lsi_flags &= ~LSI_BDI_INITIALIZED;
> - }
> -
> ll_free_sbi(sb);
> lsi->lsi_llsbi = NULL;
>
> --
> 2.10.2
>
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
^ permalink raw reply
* Re: [lustre-devel] [PATCH 04/24] fs: Provide infrastructure for dynamic BDIs in filesystems
From: Dilger, Andreas @ 2017-02-08 0:38 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
linux-nfs@vger.kernel.org, linux-nilfs@vger.kernel.org,
linux-cifs@vger.kernel.org, ecryptfs@vger.kernel.org,
codalist@coda.cs.cmu.edu, Christoph Hellwig,
cluster-devel@redhat.com, linux-mtd@lists.infradead.org,
osd-dev@open-osd.org, v9fs-developer@lists.sourceforge.net,
ceph-devel@vger.kernel.org, Petr Vandrovec,
linux-afs@lists.infradead.org, linux-btrfs@vger.kernel.org,
lustre-devel@lists.lustre.org
In-Reply-To: <20170202173422.3240-5-jack@suse.cz>
On Feb 2, 2017, at 10:34, Jan Kara <jack@suse.cz> wrote:
>
> Provide helper functions for setting up dynamically allocated
> backing_dev_info structures for filesystems and cleaning them up on
> superblock destruction.
>
> CC: linux-mtd@lists.infradead.org
> CC: linux-nfs@vger.kernel.org
> CC: Petr Vandrovec <petr@vandrovec.name>
> CC: linux-nilfs@vger.kernel.org
> CC: cluster-devel@redhat.com
> CC: osd-dev@open-osd.org
> CC: codalist@coda.cs.cmu.edu
> CC: linux-afs@lists.infradead.org
> CC: ecryptfs@vger.kernel.org
> CC: linux-cifs@vger.kernel.org
> CC: ceph-devel@vger.kernel.org
> CC: linux-btrfs@vger.kernel.org
> CC: v9fs-developer@lists.sourceforge.net
> CC: lustre-devel@lists.lustre.org
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/super.c | 49 ++++++++++++++++++++++++++++++++++++++++
> include/linux/backing-dev-defs.h | 2 +-
> include/linux/fs.h | 6 +++++
> 3 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index ea662b0e5e78..31dc4c6450ef 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -446,6 +446,11 @@ void generic_shutdown_super(struct super_block *sb)
> hlist_del_init(&sb->s_instances);
> spin_unlock(&sb_lock);
> up_write(&sb->s_umount);
> + if (sb->s_iflags & SB_I_DYNBDI) {
> + bdi_put(sb->s_bdi);
> + sb->s_bdi = &noop_backing_dev_info;
> + sb->s_iflags &= ~SB_I_DYNBDI;
> + }
> }
>
> EXPORT_SYMBOL(generic_shutdown_super);
> @@ -1249,6 +1254,50 @@ mount_fs(struct file_system_type *type, int flags, const char *name, void *data)
> }
>
> /*
> + * Setup private BDI for given superblock. I gets automatically cleaned up
(typo) s/I/It/
Looks fine otherwise.
> + * in generic_shutdown_super().
> + */
> +int super_setup_bdi_name(struct super_block *sb, char *fmt, ...)
> +{
> + struct backing_dev_info *bdi;
> + int err;
> + va_list args;
> +
> + bdi = bdi_alloc(GFP_KERNEL);
> + if (!bdi)
> + return -ENOMEM;
> +
> + bdi->name = sb->s_type->name;
> +
> + va_start(args, fmt);
> + err = bdi_register_va(bdi, NULL, fmt, args);
> + va_end(args);
> + if (err) {
> + bdi_put(bdi);
> + return err;
> + }
> + WARN_ON(sb->s_bdi != &noop_backing_dev_info);
> + sb->s_bdi = bdi;
> + sb->s_iflags |= SB_I_DYNBDI;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(super_setup_bdi_name);
> +
> +/*
> + * Setup private BDI for given superblock. I gets automatically cleaned up
> + * in generic_shutdown_super().
> + */
> +int super_setup_bdi(struct super_block *sb)
> +{
> + static atomic_long_t bdi_seq = ATOMIC_LONG_INIT(0);
> +
> + return super_setup_bdi_name(sb, "%.28s-%ld", sb->s_type->name,
> + atomic_long_inc_return(&bdi_seq));
> +}
> +EXPORT_SYMBOL(super_setup_bdi);
> +
> +/*
> * This is an internal function, please use sb_end_{write,pagefault,intwrite}
> * instead.
> */
> diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> index 2ecafc8a2d06..70080b4217f4 100644
> --- a/include/linux/backing-dev-defs.h
> +++ b/include/linux/backing-dev-defs.h
> @@ -143,7 +143,7 @@ struct backing_dev_info {
> congested_fn *congested_fn; /* Function pointer if device is md/dm */
> void *congested_data; /* Pointer to aux data for congested func */
>
> - char *name;
> + const char *name;
>
> struct kref refcnt; /* Reference counter for the structure */
> unsigned int registered:1; /* Is bdi registered? */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c930cbc19342..8ed8b6d1bc54 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1267,6 +1267,9 @@ struct mm_struct;
> /* sb->s_iflags to limit user namespace mounts */
> #define SB_I_USERNS_VISIBLE 0x00000010 /* fstype already mounted */
>
> +/* Temporary flag until all filesystems are converted to dynamic bdis */
> +#define SB_I_DYNBDI 0x00000100
> +
> /* Possible states of 'frozen' field */
> enum {
> SB_UNFROZEN = 0, /* FS is unfrozen */
> @@ -2103,6 +2106,9 @@ extern int vfs_ustat(dev_t, struct kstatfs *);
> extern int freeze_super(struct super_block *super);
> extern int thaw_super(struct super_block *super);
> extern bool our_mnt(struct vfsmount *mnt);
> +extern __printf(2, 3)
> +int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
> +extern int super_setup_bdi(struct super_block *sb);
>
> extern int current_umask(void);
>
> --
> 2.10.2
>
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
^ permalink raw reply
* [GIT PULL] WRITE_SAME regression fix for 4.10
From: Jens Axboe @ 2017-02-07 23:07 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-block@vger.kernel.org
Hi Linus,
Please pull the below branch for a single fix that should go into 4.10,
fixing a regression on some devices with the WRITE_SAME command.
git://git.kernel.dk/linux-block.git for-linus
----------------------------------------------------------------
Christoph Hellwig (1):
block: don't try Write Same from __blkdev_issue_zeroout
block/blk-lib.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
--
Jens Axboe
^ permalink raw reply
* [PATCH] nbd: ref count the socks array
From: Josef Bacik @ 2017-02-07 22:10 UTC (permalink / raw)
To: linux-block, kernel-team, nbd-general, axboe
In preparation for allowing seamless reconnects we need a way to make
sure that we don't free the socks array out from underneath ourselves.
So a socks_ref counter in order to keep track of who is using the socks
array, and only free it and change num_connections once our reference
reduces to zero.
We also need to make sure that somebody calling SET_SOCK isn't coming in
before we're done with our socks array, so add a waitqueue to wait on
previous users of the socks array before initiating a new socks array.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 126 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 91 insertions(+), 35 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 1914ba2..3dc2f1d 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -54,19 +54,24 @@ struct nbd_sock {
#define NBD_TIMEDOUT 0
#define NBD_DISCONNECT_REQUESTED 1
#define NBD_DISCONNECTED 2
-#define NBD_RUNNING 3
+#define NBD_HAS_SOCKS_REF 3
struct nbd_device {
u32 flags;
unsigned long runtime_flags;
+
+ struct mutex socks_lock;
struct nbd_sock **socks;
+ atomic_t socks_ref;
+ wait_queue_head_t socks_wq;
+ int num_connections;
+
int magic;
struct blk_mq_tag_set tag_set;
struct mutex config_lock;
struct gendisk *disk;
- int num_connections;
atomic_t recv_threads;
wait_queue_head_t recv_wq;
loff_t blksize;
@@ -102,7 +107,6 @@ static int part_shift;
static int nbd_dev_dbg_init(struct nbd_device *nbd);
static void nbd_dev_dbg_close(struct nbd_device *nbd);
-
static inline struct device *nbd_to_dev(struct nbd_device *nbd)
{
return disk_to_dev(nbd->disk);
@@ -125,6 +129,27 @@ static const char *nbdcmd_to_ascii(int cmd)
return "invalid";
}
+static int nbd_socks_get_unless_zero(struct nbd_device *nbd)
+{
+ return atomic_add_unless(&nbd->socks_ref, 1, 0);
+}
+
+static void nbd_socks_put(struct nbd_device *nbd)
+{
+ if (atomic_dec_and_test(&nbd->socks_ref)) {
+ mutex_lock(&nbd->socks_lock);
+ if (nbd->num_connections) {
+ int i;
+ for (i = 0; i < nbd->num_connections; i++)
+ kfree(nbd->socks[i]);
+ kfree(nbd->socks);
+ nbd->num_connections = 0;
+ nbd->socks = NULL;
+ }
+ mutex_unlock(&nbd->socks_lock);
+ }
+}
+
static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
{
bdev->bd_inode->i_size = 0;
@@ -190,6 +215,7 @@ static void sock_shutdown(struct nbd_device *nbd)
mutex_lock(&nsock->tx_lock);
kernel_sock_shutdown(nsock->sock, SHUT_RDWR);
mutex_unlock(&nsock->tx_lock);
+ nsock->dead = true;
}
dev_warn(disk_to_dev(nbd->disk), "shutting down sockets\n");
}
@@ -200,6 +226,9 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
struct nbd_device *nbd = cmd->nbd;
+ if (!nbd_socks_get_unless_zero(nbd))
+ return BLK_EH_HANDLED;
+
if (nbd->num_connections > 1) {
dev_err_ratelimited(nbd_to_dev(nbd),
"Connection timed out, retrying\n");
@@ -219,6 +248,7 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
}
mutex_unlock(&nbd->config_lock);
blk_mq_requeue_request(req, true);
+ nbd_socks_put(nbd);
return BLK_EH_RESET_TIMER;
}
mutex_unlock(&nbd->config_lock);
@@ -228,10 +258,9 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
}
set_bit(NBD_TIMEDOUT, &nbd->runtime_flags);
req->errors++;
-
- mutex_lock(&nbd->config_lock);
sock_shutdown(nbd);
- mutex_unlock(&nbd->config_lock);
+ nbd_socks_put(nbd);
+
return BLK_EH_HANDLED;
}
@@ -523,6 +552,7 @@ static void recv_work(struct work_struct *work)
nbd_end_request(cmd);
}
+ nbd_socks_put(nbd);
atomic_dec(&nbd->recv_threads);
wake_up(&nbd->recv_wq);
}
@@ -598,9 +628,16 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
struct nbd_sock *nsock;
int ret;
+ if (!nbd_socks_get_unless_zero(nbd)) {
+ dev_err_ratelimited(disk_to_dev(nbd->disk),
+ "Socks array is empty\n");
+ return -EINVAL;
+ }
+
if (index >= nbd->num_connections) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
"Attempted send on invalid socket\n");
+ nbd_socks_put(nbd);
return -EINVAL;
}
req->errors = 0;
@@ -608,8 +645,10 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
nsock = nbd->socks[index];
if (nsock->dead) {
index = find_fallback(nbd, index);
- if (index < 0)
+ if (index < 0) {
+ nbd_socks_put(nbd);
return -EIO;
+ }
nsock = nbd->socks[index];
}
@@ -627,7 +666,7 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
goto again;
}
mutex_unlock(&nsock->tx_lock);
-
+ nbd_socks_put(nbd);
return ret;
}
@@ -656,6 +695,25 @@ static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
return BLK_MQ_RQ_QUEUE_OK;
}
+static int nbd_wait_for_socks(struct nbd_device *nbd)
+{
+ int ret;
+
+ if (!atomic_read(&nbd->socks_ref))
+ return 0;
+
+ do {
+ mutex_unlock(&nbd->socks_lock);
+ mutex_unlock(&nbd->config_lock);
+ ret = wait_event_interruptible(nbd->socks_wq,
+ atomic_read(&nbd->socks_ref) == 0);
+ mutex_lock(&nbd->config_lock);
+ mutex_lock(&nbd->socks_lock);
+ } while (!ret && atomic_read(&nbd->socks_ref));
+
+ return ret;
+}
+
static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
unsigned long arg)
{
@@ -668,21 +726,30 @@ static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
if (!sock)
return err;
- if (!nbd->task_setup)
+ err = -EINVAL;
+ mutex_lock(&nbd->socks_lock);
+ if (!nbd->task_setup) {
nbd->task_setup = current;
+ if (nbd_wait_for_socks(nbd))
+ goto out;
+ atomic_inc(&nbd->socks_ref);
+ set_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags);
+ }
+
if (nbd->task_setup != current) {
dev_err(disk_to_dev(nbd->disk),
"Device being setup by another task");
- return -EINVAL;
+ goto out;
}
+ err = -ENOMEM;
socks = krealloc(nbd->socks, (nbd->num_connections + 1) *
sizeof(struct nbd_sock *), GFP_KERNEL);
if (!socks)
- return -ENOMEM;
+ goto out;
nsock = kzalloc(sizeof(struct nbd_sock), GFP_KERNEL);
if (!nsock)
- return -ENOMEM;
+ goto out;
nbd->socks = socks;
@@ -694,7 +761,10 @@ static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
if (max_part)
bdev->bd_invalidated = 1;
- return 0;
+ err = 0;
+out:
+ mutex_unlock(&nbd->socks_lock);
+ return err;
}
/* Reset all properties of an NBD device */
@@ -750,20 +820,17 @@ static void send_disconnects(struct nbd_device *nbd)
static int nbd_disconnect(struct nbd_device *nbd, struct block_device *bdev)
{
dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
- if (!nbd->socks)
+ if (!nbd_socks_get_unless_zero(nbd))
return -EINVAL;
mutex_unlock(&nbd->config_lock);
fsync_bdev(bdev);
mutex_lock(&nbd->config_lock);
- /* Check again after getting mutex back. */
- if (!nbd->socks)
- return -EINVAL;
-
if (!test_and_set_bit(NBD_DISCONNECT_REQUESTED,
&nbd->runtime_flags))
send_disconnects(nbd);
+ nbd_socks_put(nbd);
return 0;
}
@@ -773,22 +840,9 @@ static int nbd_clear_sock(struct nbd_device *nbd, struct block_device *bdev)
nbd_clear_que(nbd);
kill_bdev(bdev);
nbd_bdev_reset(bdev);
- /*
- * We want to give the run thread a chance to wait for everybody
- * to clean up and then do it's own cleanup.
- */
- if (!test_bit(NBD_RUNNING, &nbd->runtime_flags) &&
- nbd->num_connections) {
- int i;
-
- for (i = 0; i < nbd->num_connections; i++)
- kfree(nbd->socks[i]);
- kfree(nbd->socks);
- nbd->socks = NULL;
- nbd->num_connections = 0;
- }
nbd->task_setup = NULL;
-
+ if (test_and_clear_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags))
+ nbd_socks_put(nbd);
return 0;
}
@@ -809,7 +863,6 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
goto out_err;
}
- set_bit(NBD_RUNNING, &nbd->runtime_flags);
blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
if (!args) {
@@ -833,6 +886,7 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
for (i = 0; i < num_connections; i++) {
sk_set_memalloc(nbd->socks[i]->sock->sk);
atomic_inc(&nbd->recv_threads);
+ atomic_inc(&nbd->socks_ref);
INIT_WORK(&args[i].work, recv_work);
args[i].nbd = nbd;
args[i].index = i;
@@ -849,7 +903,6 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
mutex_lock(&nbd->config_lock);
nbd->task_recv = NULL;
out_err:
- clear_bit(NBD_RUNNING, &nbd->runtime_flags);
nbd_clear_sock(nbd, bdev);
/* user requested, ignore socket errors */
@@ -1149,12 +1202,15 @@ static int nbd_dev_add(int index)
nbd->magic = NBD_MAGIC;
mutex_init(&nbd->config_lock);
+ mutex_init(&nbd->socks_lock);
+ atomic_set(&nbd->socks_ref, 0);
disk->major = NBD_MAJOR;
disk->first_minor = index << part_shift;
disk->fops = &nbd_fops;
disk->private_data = nbd;
sprintf(disk->disk_name, "nbd%d", index);
init_waitqueue_head(&nbd->recv_wq);
+ init_waitqueue_head(&nbd->socks_wq);
nbd_reset(nbd);
add_disk(disk);
return index;
--
2.7.4
^ permalink raw reply related
* [PATCH] nbd: cleanup ioctl handling
From: Josef Bacik @ 2017-02-07 22:10 UTC (permalink / raw)
To: linux-block, kernel-team, nbd-general, axboe
Break the ioctl handling out into helper functions, some of these things
are getting pretty big and unwieldy.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 265 ++++++++++++++++++++++++++--------------------------
1 file changed, 130 insertions(+), 135 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 164a548..1914ba2 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -99,6 +99,10 @@ static int max_part;
static struct workqueue_struct *recv_workqueue;
static int part_shift;
+static int nbd_dev_dbg_init(struct nbd_device *nbd);
+static void nbd_dev_dbg_close(struct nbd_device *nbd);
+
+
static inline struct device *nbd_to_dev(struct nbd_device *nbd)
{
return disk_to_dev(nbd->disk);
@@ -652,10 +656,17 @@ static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
return BLK_MQ_RQ_QUEUE_OK;
}
-static int nbd_add_socket(struct nbd_device *nbd, struct socket *sock)
+static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
+ unsigned long arg)
{
+ struct socket *sock;
struct nbd_sock **socks;
struct nbd_sock *nsock;
+ int err;
+
+ sock = sockfd_lookup(arg, &err);
+ if (!sock)
+ return err;
if (!nbd->task_setup)
nbd->task_setup = current;
@@ -681,26 +692,20 @@ static int nbd_add_socket(struct nbd_device *nbd, struct socket *sock)
nsock->sock = sock;
socks[nbd->num_connections++] = nsock;
+ if (max_part)
+ bdev->bd_invalidated = 1;
return 0;
}
/* Reset all properties of an NBD device */
static void nbd_reset(struct nbd_device *nbd)
{
- int i;
-
- for (i = 0; i < nbd->num_connections; i++)
- kfree(nbd->socks[i]);
- kfree(nbd->socks);
- nbd->socks = NULL;
nbd->runtime_flags = 0;
nbd->blksize = 1024;
nbd->bytesize = 0;
set_capacity(nbd->disk, 0);
nbd->flags = 0;
nbd->tag_set.timeout = 0;
- nbd->num_connections = 0;
- nbd->task_setup = NULL;
queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
}
@@ -742,74 +747,135 @@ static void send_disconnects(struct nbd_device *nbd)
}
}
-static int nbd_dev_dbg_init(struct nbd_device *nbd);
-static void nbd_dev_dbg_close(struct nbd_device *nbd);
-
-/* Must be called with config_lock held */
-static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
- unsigned int cmd, unsigned long arg)
+static int nbd_disconnect(struct nbd_device *nbd, struct block_device *bdev)
{
- switch (cmd) {
- case NBD_DISCONNECT: {
- dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
- if (!nbd->socks)
- return -EINVAL;
+ dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
+ if (!nbd->socks)
+ return -EINVAL;
- mutex_unlock(&nbd->config_lock);
- fsync_bdev(bdev);
- mutex_lock(&nbd->config_lock);
+ mutex_unlock(&nbd->config_lock);
+ fsync_bdev(bdev);
+ mutex_lock(&nbd->config_lock);
- /* Check again after getting mutex back. */
- if (!nbd->socks)
- return -EINVAL;
+ /* Check again after getting mutex back. */
+ if (!nbd->socks)
+ return -EINVAL;
- if (!test_and_set_bit(NBD_DISCONNECT_REQUESTED,
- &nbd->runtime_flags))
- send_disconnects(nbd);
- return 0;
+ if (!test_and_set_bit(NBD_DISCONNECT_REQUESTED,
+ &nbd->runtime_flags))
+ send_disconnects(nbd);
+ return 0;
+}
+
+static int nbd_clear_sock(struct nbd_device *nbd, struct block_device *bdev)
+{
+ sock_shutdown(nbd);
+ nbd_clear_que(nbd);
+ kill_bdev(bdev);
+ nbd_bdev_reset(bdev);
+ /*
+ * We want to give the run thread a chance to wait for everybody
+ * to clean up and then do it's own cleanup.
+ */
+ if (!test_bit(NBD_RUNNING, &nbd->runtime_flags) &&
+ nbd->num_connections) {
+ int i;
+
+ for (i = 0; i < nbd->num_connections; i++)
+ kfree(nbd->socks[i]);
+ kfree(nbd->socks);
+ nbd->socks = NULL;
+ nbd->num_connections = 0;
}
+ nbd->task_setup = NULL;
- case NBD_CLEAR_SOCK:
- sock_shutdown(nbd);
- nbd_clear_que(nbd);
- kill_bdev(bdev);
- nbd_bdev_reset(bdev);
- /*
- * We want to give the run thread a chance to wait for everybody
- * to clean up and then do it's own cleanup.
- */
- if (!test_bit(NBD_RUNNING, &nbd->runtime_flags)) {
- int i;
-
- for (i = 0; i < nbd->num_connections; i++)
- kfree(nbd->socks[i]);
- kfree(nbd->socks);
- nbd->socks = NULL;
- nbd->num_connections = 0;
- nbd->task_setup = NULL;
- }
- return 0;
+ return 0;
+}
- case NBD_SET_SOCK: {
- int err;
- struct socket *sock = sockfd_lookup(arg, &err);
+static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
+{
+ struct recv_thread_args *args;
+ int num_connections = nbd->num_connections;
+ int error = 0, i;
- if (!sock)
- return err;
+ if (nbd->task_recv)
+ return -EBUSY;
+ if (!nbd->socks)
+ return -EINVAL;
+ if (num_connections > 1 &&
+ !(nbd->flags & NBD_FLAG_CAN_MULTI_CONN)) {
+ dev_err(disk_to_dev(nbd->disk), "server does not support multiple connections per device.\n");
+ error = -EINVAL;
+ goto out_err;
+ }
- err = nbd_add_socket(nbd, sock);
- if (!err && max_part)
- bdev->bd_invalidated = 1;
+ set_bit(NBD_RUNNING, &nbd->runtime_flags);
+ blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
+ args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
+ if (!args) {
+ error = -ENOMEM;
+ goto out_err;
+ }
+ nbd->task_recv = current;
+ mutex_unlock(&nbd->config_lock);
- return err;
+ nbd_parse_flags(nbd, bdev);
+
+ error = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
+ if (error) {
+ dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
+ goto out_recv;
}
- case NBD_SET_BLKSIZE: {
- loff_t bsize = div_s64(nbd->bytesize, arg);
+ nbd_size_update(nbd, bdev);
- return nbd_size_set(nbd, bdev, arg, bsize);
+ nbd_dev_dbg_init(nbd);
+ for (i = 0; i < num_connections; i++) {
+ sk_set_memalloc(nbd->socks[i]->sock->sk);
+ atomic_inc(&nbd->recv_threads);
+ INIT_WORK(&args[i].work, recv_work);
+ args[i].nbd = nbd;
+ args[i].index = i;
+ queue_work(recv_workqueue, &args[i].work);
}
+ wait_event_interruptible(nbd->recv_wq,
+ atomic_read(&nbd->recv_threads) == 0);
+ for (i = 0; i < num_connections; i++)
+ flush_work(&args[i].work);
+ nbd_dev_dbg_close(nbd);
+ nbd_size_clear(nbd, bdev);
+ device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
+out_recv:
+ mutex_lock(&nbd->config_lock);
+ nbd->task_recv = NULL;
+out_err:
+ clear_bit(NBD_RUNNING, &nbd->runtime_flags);
+ nbd_clear_sock(nbd, bdev);
+
+ /* user requested, ignore socket errors */
+ if (test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags))
+ error = 0;
+ if (test_bit(NBD_TIMEDOUT, &nbd->runtime_flags))
+ error = -ETIMEDOUT;
+
+ nbd_reset(nbd);
+ return error;
+}
+/* Must be called with config_lock held */
+static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
+ unsigned int cmd, unsigned long arg)
+{
+ switch (cmd) {
+ case NBD_DISCONNECT:
+ return nbd_disconnect(nbd, bdev);
+ case NBD_CLEAR_SOCK:
+ return nbd_clear_sock(nbd, bdev);
+ case NBD_SET_SOCK:
+ return nbd_add_socket(nbd, bdev, arg);
+ case NBD_SET_BLKSIZE:
+ return nbd_size_set(nbd, bdev, arg,
+ div_s64(nbd->bytesize, arg));
case NBD_SET_SIZE:
return nbd_size_set(nbd, bdev, nbd->blksize,
div_s64(arg, nbd->blksize));
@@ -824,85 +890,14 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
case NBD_SET_FLAGS:
nbd->flags = arg;
return 0;
-
- case NBD_DO_IT: {
- struct recv_thread_args *args;
- int num_connections = nbd->num_connections;
- int error = 0, i;
-
- if (nbd->task_recv)
- return -EBUSY;
- if (!nbd->socks)
- return -EINVAL;
- if (num_connections > 1 &&
- !(nbd->flags & NBD_FLAG_CAN_MULTI_CONN)) {
- dev_err(disk_to_dev(nbd->disk), "server does not support multiple connections per device.\n");
- error = -EINVAL;
- goto out_err;
- }
-
- set_bit(NBD_RUNNING, &nbd->runtime_flags);
- blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
- args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
- if (!args) {
- error = -ENOMEM;
- goto out_err;
- }
- nbd->task_recv = current;
- mutex_unlock(&nbd->config_lock);
-
- nbd_parse_flags(nbd, bdev);
-
- error = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
- if (error) {
- dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
- goto out_recv;
- }
-
- nbd_size_update(nbd, bdev);
-
- nbd_dev_dbg_init(nbd);
- for (i = 0; i < num_connections; i++) {
- sk_set_memalloc(nbd->socks[i]->sock->sk);
- atomic_inc(&nbd->recv_threads);
- INIT_WORK(&args[i].work, recv_work);
- args[i].nbd = nbd;
- args[i].index = i;
- queue_work(recv_workqueue, &args[i].work);
- }
- wait_event_interruptible(nbd->recv_wq,
- atomic_read(&nbd->recv_threads) == 0);
- for (i = 0; i < num_connections; i++)
- flush_work(&args[i].work);
- nbd_dev_dbg_close(nbd);
- nbd_size_clear(nbd, bdev);
- device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
-out_recv:
- mutex_lock(&nbd->config_lock);
- nbd->task_recv = NULL;
-out_err:
- sock_shutdown(nbd);
- nbd_clear_que(nbd);
- kill_bdev(bdev);
- nbd_bdev_reset(bdev);
-
- /* user requested, ignore socket errors */
- if (test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags))
- error = 0;
- if (test_bit(NBD_TIMEDOUT, &nbd->runtime_flags))
- error = -ETIMEDOUT;
-
- nbd_reset(nbd);
- return error;
- }
-
+ case NBD_DO_IT:
+ return nbd_start_device(nbd, bdev);
case NBD_CLEAR_QUE:
/*
* This is for compatibility only. The queue is always cleared
* by NBD_DO_IT or NBD_CLEAR_SOCK.
*/
return 0;
-
case NBD_PRINT_DEBUG:
/*
* For compatibility only, we no longer keep a list of
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] bfq-mq: cause deadlock by executing exit_icq body immediately
From: Omar Sandoval @ 2017-02-07 21:45 UTC (permalink / raw)
To: Paolo Valente
Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson,
linus.walleij, broonie
In-Reply-To: <20170207173346.4789-1-paolo.valente@linaro.org>
On Tue, Feb 07, 2017 at 06:33:46PM +0100, Paolo Valente wrote:
> Hi,
> this patch is meant to show that, if the body of the hook exit_icq is executed
> from inside that hook, and not as deferred work, then a circular deadlock
> occurs.
>
> It happens if, on a CPU
> - the body of icq_exit takes the scheduler lock,
> - it does so from inside the exit_icq hook, which is invoked with the queue
> lock held
>
> while, on another CPU
> - bfq_bio_merge, after taking the scheduler lock, invokes bfq_bic_lookup,
> which, in its turn, takes the queue lock. bfq_bic_lookup needs to take such a
> lock, because it invokes ioc_lookup_icq.
>
> For more details, here is a lockdep report, right before the deadlock did occur.
>
> [ 44.059877] ======================================================
> [ 44.124922] [ INFO: possible circular locking dependency detected ]
> [ 44.125795] 4.10.0-rc5-bfq-mq+ #38 Not tainted
> [ 44.126414] -------------------------------------------------------
> [ 44.127291] sync/2043 is trying to acquire lock:
> [ 44.128918] (&(&bfqd->lock)->rlock){-.-...}, at: [<ffffffff90484195>] bfq_exit_icq_bfqq+0x55/0x140
> [ 44.134052]
> [ 44.134052] but task is already holding lock:
> [ 44.134868] (&(&q->__queue_lock)->rlock){-.....}, at: [<ffffffff9044738e>] put_io_context_active+0x6e/0xc0
Hey, Paolo,
I only briefly skimmed the code, but what are you using the queue_lock
for? You should just use your scheduler lock everywhere. blk-mq doesn't
use the queue lock, so the scheduler is the only thing you need mutual
exclusion against. I'm guessing if you stopped using that, your locking
issues would go away.
^ permalink raw reply
* Re: [PATCH 4/4] nvme: support ranged discard requests
From: Christoph Hellwig @ 2017-02-07 19:34 UTC (permalink / raw)
To: Keith Busch; +Cc: Christoph Hellwig, Jens Axboe, linux-block, linux-nvme
In-Reply-To: <20170207183453.GC10133@localhost.localdomain>
On Tue, Feb 07, 2017 at 01:34:54PM -0500, Keith Busch wrote:
> On Tue, Feb 07, 2017 at 05:46:58PM +0100, Christoph Hellwig wrote:
> > @@ -1233,6 +1243,8 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
> > if (ctrl->vwc & NVME_CTRL_VWC_PRESENT)
> > vwc = true;
> > blk_queue_write_cache(q, vwc, vwc);
> > + blk_queue_max_discard_segments(q,
> > + PAGE_SIZE / sizeof(struct nvme_dsm_range));
>
> We need to use ctrl->page_size rather than PAGE_SIZE.
We need the PAGE_SIZE upper bound because the "special" bvec only
allows a single page payload. I can't find any indication that
the DSM command is limited to a single PRP, so not sure where a
ctrl->page_size would come from. But now that I looked again there
is a hard limit of 256 entries due to NR being a 8-bit field, so
I need to cap on that at least.
^ permalink raw reply
* Re: [PATCH 4/4] nvme: support ranged discard requests
From: Keith Busch @ 2017-02-07 18:34 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, linux-nvme
In-Reply-To: <20170207164658.32449-5-hch@lst.de>
On Tue, Feb 07, 2017 at 05:46:58PM +0100, Christoph Hellwig wrote:
> @@ -1233,6 +1243,8 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
> if (ctrl->vwc & NVME_CTRL_VWC_PRESENT)
> vwc = true;
> blk_queue_write_cache(q, vwc, vwc);
> + blk_queue_max_discard_segments(q,
> + PAGE_SIZE / sizeof(struct nvme_dsm_range));
We need to use ctrl->page_size rather than PAGE_SIZE.
Otherwise, this looks really good!
^ permalink raw reply
* Re: [PATCH 2/2] block: free merged request in the caller
From: Paolo Valente @ 2017-02-07 17:35 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, osandov
In-Reply-To: <1486051573-13445-3-git-send-email-axboe@fb.com>
> Il giorno 02 feb 2017, alle ore 17:06, Jens Axboe <axboe@fb.com> ha =
scritto:
>=20
> If we end up doing a request-to-request merge when we have completed
> a bio-to-request merge, we free the request from deep down in that
> path. For blk-mq-sched, the merge path has to hold the appropriate
> lock, but we don't need it for freeing the request. And in fact
> holding the lock is problematic, since we are now calling the
> mq sched put_rq_private() hook with the lock held. Other call paths
> do not hold this lock.
>=20
> Fix this inconsistency by ensuring that the caller frees a merged
> request. Then we can do it outside of the lock, making it both more
> efficient and fixing the blk-mq-sched problem of invoking parts of
> the scheduler with an unknown lock state.
>=20
> Reported-by: Paolo Valente <paolo.valente@linaro.org>
> Signed-off-by: Jens Axboe <axboe@fb.com>
> ---
> block/blk-core.c | 12 +++++++++---
> block/blk-merge.c | 15 ++++++++++++---
> block/blk-mq-sched.c | 9 ++++++---
> block/blk-mq-sched.h | 3 ++-
> block/mq-deadline.c | 8 ++++++--
> 5 files changed, 35 insertions(+), 12 deletions(-)
>=20
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a5726e01f839..00c90f8cd682 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1591,7 +1591,7 @@ static blk_qc_t blk_queue_bio(struct =
request_queue *q, struct bio *bio)
> {
> struct blk_plug *plug;
> int el_ret, where =3D ELEVATOR_INSERT_SORT;
> - struct request *req;
> + struct request *req, *free;
> unsigned int request_count =3D 0;
> unsigned int wb_acct;
>=20
> @@ -1632,15 +1632,21 @@ static blk_qc_t blk_queue_bio(struct =
request_queue *q, struct bio *bio)
> if (el_ret =3D=3D ELEVATOR_BACK_MERGE) {
> if (bio_attempt_back_merge(q, req, bio)) {
> elv_bio_merged(q, req, bio);
> - if (!attempt_back_merge(q, req))
> + free =3D attempt_back_merge(q, req);
> + if (!free)
> elv_merged_request(q, req, el_ret);
> + else
> + __blk_put_request(q, free);
> goto out_unlock;
> }
> } else if (el_ret =3D=3D ELEVATOR_FRONT_MERGE) {
> if (bio_attempt_front_merge(q, req, bio)) {
> elv_bio_merged(q, req, bio);
> - if (!attempt_front_merge(q, req))
> + free =3D attempt_front_merge(q, req);
> + if (!free)
> elv_merged_request(q, req, el_ret);
> + else
> + __blk_put_request(q, free);
> goto out_unlock;
> }
> }
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 3826fc32b72c..a373416dbc9a 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -733,9 +733,11 @@ static struct request *attempt_merge(struct =
request_queue *q,
> if (blk_rq_cpu_valid(next))
> req->cpu =3D next->cpu;
>=20
> - /* owner-ship of bio passed from next to req */
> + /*
> + * ownership of bio passed from next to req, return 'next' for
> + * the caller to free
> + */
> next->bio =3D NULL;
> - __blk_put_request(q, next);
> return next;
> }
>=20
> @@ -763,12 +765,19 @@ int blk_attempt_req_merge(struct request_queue =
*q, struct request *rq,
> struct request *next)
> {
> struct elevator_queue *e =3D q->elevator;
> + struct request *free;
>=20
> if (!e->uses_mq && e->type->ops.sq.elevator_allow_rq_merge_fn)
> if (!e->type->ops.sq.elevator_allow_rq_merge_fn(q, rq, =
next))
> return 0;
>=20
> - return attempt_merge(q, rq, next) !=3D NULL;
> + free =3D attempt_merge(q, rq, next);
> + if (free) {
> + __blk_put_request(q, free);
> + return 1;
> + }
> +
> + return 0;
> }
>=20
Hi Jens,
blk_attempt_req_merge is reached also from
blk_mq_sched_try_insert_merge, which is invoked in mq-deadline.
According to the current usage of the scheduler lock in mq-deadline,
that lock should be taken before invoking
blk_mq_sched_try_insert_merge too, as I pointed out in a previous
reply of mine. But, if it is taken, then we have again the
double-lock-state problem in put_rq_private.
Unfortunately, from your general explanation on how to best protect
shared data structures, I did not understand (sorry) whether the
scheduler lock must or must not be taken around
blk_mq_sched_try_insert_merge in the current implementation of
mq-deadline. Could you patiently provide a thickheaded-proof, yes/no
answer, if possible?
Thanks,
Paolo
> bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index 114814ec3d49..d93b56d53c4e 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -234,7 +234,8 @@ void blk_mq_sched_move_to_dispatch(struct =
blk_mq_hw_ctx *hctx,
> }
> EXPORT_SYMBOL_GPL(blk_mq_sched_move_to_dispatch);
>=20
> -bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio)
> +bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
> + struct request **merged_request)
> {
> struct request *rq;
> int ret;
> @@ -244,7 +245,8 @@ bool blk_mq_sched_try_merge(struct request_queue =
*q, struct bio *bio)
> if (!blk_mq_sched_allow_merge(q, rq, bio))
> return false;
> if (bio_attempt_back_merge(q, rq, bio)) {
> - if (!attempt_back_merge(q, rq))
> + *merged_request =3D attempt_back_merge(q, rq);
> + if (!*merged_request)
> elv_merged_request(q, rq, ret);
> return true;
> }
> @@ -252,7 +254,8 @@ bool blk_mq_sched_try_merge(struct request_queue =
*q, struct bio *bio)
> if (!blk_mq_sched_allow_merge(q, rq, bio))
> return false;
> if (bio_attempt_front_merge(q, rq, bio)) {
> - if (!attempt_front_merge(q, rq))
> + *merged_request =3D attempt_front_merge(q, rq);
> + if (!*merged_request)
> elv_merged_request(q, rq, ret);
> return true;
> }
> diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
> index 9478aaeb48c5..3643686a54b8 100644
> --- a/block/blk-mq-sched.h
> +++ b/block/blk-mq-sched.h
> @@ -16,7 +16,8 @@ void blk_mq_sched_put_request(struct request *rq);
>=20
> void blk_mq_sched_request_inserted(struct request *rq);
> bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx, struct =
request *rq);
> -bool blk_mq_sched_try_merge(struct request_queue *q, struct bio =
*bio);
> +bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
> + struct request **merged_request);
> bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio =
*bio);
> bool blk_mq_sched_try_insert_merge(struct request_queue *q, struct =
request *rq);
> void blk_mq_sched_restart_queues(struct blk_mq_hw_ctx *hctx);
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index 49583536698c..682fa64f55ff 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -371,12 +371,16 @@ static bool dd_bio_merge(struct blk_mq_hw_ctx =
*hctx, struct bio *bio)
> {
> struct request_queue *q =3D hctx->queue;
> struct deadline_data *dd =3D q->elevator->elevator_data;
> - int ret;
> + struct request *free =3D NULL;
> + bool ret;
>=20
> spin_lock(&dd->lock);
> - ret =3D blk_mq_sched_try_merge(q, bio);
> + ret =3D blk_mq_sched_try_merge(q, bio, &free);
> spin_unlock(&dd->lock);
>=20
> + if (free)
> + blk_mq_free_request(free);
> +
> return ret;
> }
>=20
> --=20
> 2.7.4
>=20
^ permalink raw reply
* [PATCH] bfq-mq: cause deadlock by executing exit_icq body immediately
From: Paolo Valente @ 2017-02-07 17:33 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo
Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie,
Paolo Valente
Hi,
this patch is meant to show that, if the body of the hook exit_icq is executed
from inside that hook, and not as deferred work, then a circular deadlock
occurs.
It happens if, on a CPU
- the body of icq_exit takes the scheduler lock,
- it does so from inside the exit_icq hook, which is invoked with the queue
lock held
while, on another CPU
- bfq_bio_merge, after taking the scheduler lock, invokes bfq_bic_lookup,
which, in its turn, takes the queue lock. bfq_bic_lookup needs to take such a
lock, because it invokes ioc_lookup_icq.
For more details, here is a lockdep report, right before the deadlock did occur.
[ 44.059877] ======================================================
[ 44.124922] [ INFO: possible circular locking dependency detected ]
[ 44.125795] 4.10.0-rc5-bfq-mq+ #38 Not tainted
[ 44.126414] -------------------------------------------------------
[ 44.127291] sync/2043 is trying to acquire lock:
[ 44.128918] (&(&bfqd->lock)->rlock){-.-...}, at: [<ffffffff90484195>] bfq_exit_icq_bfqq+0x55/0x140
[ 44.134052]
[ 44.134052] but task is already holding lock:
[ 44.134868] (&(&q->__queue_lock)->rlock){-.....}, at: [<ffffffff9044738e>] put_io_context_active+0x6e/0xc0
[ 44.136292]
[ 44.136292] which lock already depends on the new lock.
[ 44.136292]
[ 44.137411]
[ 44.137411] the existing dependency chain (in reverse order) is:
[ 44.139936]
[ 44.139936] -> #1 (&(&q->__queue_lock)->rlock){-.....}:
[ 44.141512]
[ 44.141517] [<ffffffff900ee6bb>] lock_acquire+0x11b/0x220
[ 44.142569]
[ 44.142578] [<ffffffff90943e66>] _raw_spin_lock_irqsave+0x56/0x90
[ 44.146365]
[ 44.146371] [<ffffffff904822f5>] bfq_bic_lookup.isra.112+0x25/0x60
[ 44.147523]
[ 44.147525] [<ffffffff9048245d>] bfq_request_merge+0x3d/0xe0
[ 44.149738]
[ 44.149742] [<ffffffff90439aef>] elv_merge+0xcf/0xe0
[ 44.150726]
[ 44.150728] [<ffffffff90453c46>] blk_mq_sched_try_merge+0x36/0x150
[ 44.151878]
[ 44.151881] [<ffffffff9047fb6a>] bfq_bio_merge+0x5a/0xa0
[ 44.153913]
[ 44.153916] [<ffffffff90454500>] __blk_mq_sched_bio_merge+0x60/0x70
[ 44.155089]
[ 44.155091] [<ffffffff9044e6c7>] blk_sq_make_request+0x277/0xa90
[ 44.157455]
[ 44.157458] [<ffffffff90440846>] generic_make_request+0xf6/0x2b0
[ 44.158597]
[ 44.158599] [<ffffffff90440a73>] submit_bio+0x73/0x150
[ 44.160537]
[ 44.160541] [<ffffffff90366e0b>] ext4_mpage_readpages+0x48b/0x950
[ 44.162961]
[ 44.162971] [<ffffffff90313236>] ext4_readpages+0x36/0x40
[ 44.164037]
[ 44.164040] [<ffffffff901eca4e>] __do_page_cache_readahead+0x2ae/0x3a0
[ 44.165224]
[ 44.165227] [<ffffffff901ecc4e>] ondemand_readahead+0x10e/0x4b0
[ 44.166334]
[ 44.166336] [<ffffffff901ed1a1>] page_cache_sync_readahead+0x31/0x50
[ 44.167502]
[ 44.167503] [<ffffffff901dcaed>] generic_file_read_iter+0x68d/0x8d0
[ 44.168661]
[ 44.168663] [<ffffffff9030e6c7>] ext4_file_read_iter+0x37/0xc0
[ 44.169760]
[ 44.169764] [<ffffffff9026e7c3>] __vfs_read+0xe3/0x150
[ 44.171987]
[ 44.171990] [<ffffffff9026ee58>] vfs_read+0xa8/0x170
[ 44.178999]
[ 44.179005] [<ffffffff902761e8>] prepare_binprm+0x118/0x200
[ 44.180080]
[ 44.180083] [<ffffffff90277bcb>] do_execveat_common.isra.39+0x56b/0xa00
[ 44.184409]
[ 44.184414] [<ffffffff902782fa>] SyS_execve+0x3a/0x50
[ 44.185398]
[ 44.185401] [<ffffffff90003e49>] do_syscall_64+0x69/0x160
[ 44.187349]
[ 44.187353] [<ffffffff9094408d>] return_from_SYSCALL_64+0x0/0x7a
[ 44.188475]
[ 44.188475] -> #0 (&(&bfqd->lock)->rlock){-.-...}:
[ 44.199960]
[ 44.199966] [<ffffffff900edd24>] __lock_acquire+0x15e4/0x1890
[ 44.201056]
[ 44.201058] [<ffffffff900ee6bb>] lock_acquire+0x11b/0x220
[ 44.202099]
[ 44.202101] [<ffffffff909434da>] _raw_spin_lock_irq+0x4a/0x80
[ 44.203197]
[ 44.203200] [<ffffffff90484195>] bfq_exit_icq_bfqq+0x55/0x140
[ 44.204848]
[ 44.204851] [<ffffffff9048429c>] bfq_exit_icq+0x1c/0x30
[ 44.205863]
[ 44.205866] [<ffffffff90446e68>] ioc_exit_icq+0x38/0x50
[ 44.206881]
[ 44.206883] [<ffffffff9044739a>] put_io_context_active+0x7a/0xc0
[ 44.215156] sh (2042): drop_caches: 3
[ 44.216738]
[ 44.216742] [<ffffffff90447428>] exit_io_context+0x48/0x50
[ 44.217497]
[ 44.217500] [<ffffffff90095727>] do_exit+0x787/0xc50
[ 44.218207]
[ 44.218209] [<ffffffff90095c80>] do_group_exit+0x50/0xd0
[ 44.218969]
[ 44.218970] [<ffffffff90095d14>] SyS_exit_group+0x14/0x20
[ 44.219716]
[ 44.219718] [<ffffffff90943fc5>] entry_SYSCALL_64_fastpath+0x23/0xc6
[ 44.220550]
[ 44.220550] other info that might help us debug this:
[ 44.220550]
[ 44.223477] Possible unsafe locking scenario:
[ 44.223477]
[ 44.224281] CPU0 CPU1
[ 44.224903] ---- ----
[ 44.225523] lock(&(&q->__queue_lock)->rlock);
[ 44.226144] lock(&(&bfqd->lock)->rlock);
[ 44.227051] lock(&(&q->__queue_lock)->rlock);
[ 44.228019] lock(&(&bfqd->lock)->rlock);
[ 44.228643]
[ 44.228643] *** DEADLOCK ***
[ 44.228643]
[ 44.230136] 2 locks held by sync/2043:
[ 44.230591] #0: (&(&ioc->lock)->rlock/1){......}, at: [<ffffffff90447358>] put_io_context_active+0x38/0xc0
[ 44.231553] #1: (&(&q->__queue_lock)->rlock){-.....}, at: [<ffffffff9044738e>] put_io_context_active+0x6e/0xc0
[ 44.232542]
[ 44.232542] stack backtrace:
[ 44.232974] CPU: 1 PID: 2043 Comm: sync Not tainted 4.10.0-rc5-bfq-mq+ #38
[ 44.238224] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[ 44.239364] Call Trace:
[ 44.239717] dump_stack+0x85/0xc2
[ 44.240186] print_circular_bug+0x1e3/0x250
[ 44.240773] __lock_acquire+0x15e4/0x1890
[ 44.241350] lock_acquire+0x11b/0x220
[ 44.241867] ? bfq_exit_icq_bfqq+0x55/0x140
[ 44.242455] _raw_spin_lock_irq+0x4a/0x80
[ 44.243018] ? bfq_exit_icq_bfqq+0x55/0x140
[ 44.243629] bfq_exit_icq_bfqq+0x55/0x140
[ 44.244192] bfq_exit_icq+0x1c/0x30
[ 44.244713] ioc_exit_icq+0x38/0x50
[ 44.246518] put_io_context_active+0x7a/0xc0
[ 44.247116] exit_io_context+0x48/0x50
[ 44.247647] do_exit+0x787/0xc50
[ 44.248103] do_group_exit+0x50/0xd0
[ 44.249055] SyS_exit_group+0x14/0x20
[ 44.249568] entry_SYSCALL_64_fastpath+0x23/0xc6
[ 44.250208] RIP: 0033:0x7fd70b22ab98
[ 44.250704] RSP: 002b:00007ffc9cc43878 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
[ 44.251745] RAX: ffffffffffffffda RBX: 00007fd70b523620 RCX: 00007fd70b22ab98
[ 44.252730] RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
[ 44.253713] RBP: 00007fd70b523620 R08: 00000000000000e7 R09: ffffffffffffff98
[ 44.254690] R10: 00007ffc9cc437c8 R11: 0000000000000246 R12: 0000000000000000
[ 44.255674] R13: 00007fd70b523c40 R14: 0000000000000000 R15: 0000000000000000
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
block/bfq-mq-iosched.c | 34 +++-------------------------------
block/bfq-mq.h | 3 ---
2 files changed, 3 insertions(+), 34 deletions(-)
diff --git a/block/bfq-mq-iosched.c b/block/bfq-mq-iosched.c
index 05a12b6..6e79bb8 100644
--- a/block/bfq-mq-iosched.c
+++ b/block/bfq-mq-iosched.c
@@ -4001,28 +4001,13 @@ static void bfq_exit_icq_bfqq(struct bfq_io_cq *bic, bool is_sync)
}
}
-static void bfq_exit_icq_body(struct work_struct *work)
-{
- struct bfq_io_cq *bic =
- container_of(work, struct bfq_io_cq, exit_icq_work);
-
- bfq_exit_icq_bfqq(bic, true);
- bfq_exit_icq_bfqq(bic, false);
-}
-
-static void bfq_init_icq(struct io_cq *icq)
-{
- struct bfq_io_cq *bic = icq_to_bic(icq);
-
- INIT_WORK(&bic->exit_icq_work, bfq_exit_icq_body);
-}
-
static void bfq_exit_icq(struct io_cq *icq)
{
struct bfq_io_cq *bic = icq_to_bic(icq);
BUG_ON(!bic);
- kblockd_schedule_work(&bic->exit_icq_work);
+ bfq_exit_icq_bfqq(bic, true);
+ bfq_exit_icq_bfqq(bic, false);
}
/*
@@ -4934,21 +4919,9 @@ static void bfq_exit_queue(struct elevator_queue *e)
BUG_ON(bfqd->in_service_queue);
BUG_ON(!list_empty(&bfqd->active_list));
- list_for_each_entry_safe(bfqq, n, &bfqd->idle_list, bfqq_list) {
- if (bfqq->bic) /* bfqqs without bic are handled below */
- cancel_work_sync(&bfqq->bic->exit_icq_work);
- }
-
spin_lock_irq(&bfqd->lock);
- list_for_each_entry_safe(bfqq, n, &bfqd->idle_list, bfqq_list) {
+ list_for_each_entry_safe(bfqq, n, &bfqd->idle_list, bfqq_list)
bfq_deactivate_bfqq(bfqd, bfqq, false, false);
- /*
- * Make sure that deferred exit_icq_work completes
- * without errors for bfq_queues without bic
- */
- if (!bfqq->bic)
- bfqq->bfqd = NULL;
- }
spin_unlock_irq(&bfqd->lock);
hrtimer_cancel(&bfqd->idle_slice_timer);
@@ -5384,7 +5357,6 @@ static struct elevator_type iosched_bfq_mq = {
.ops.mq = {
.get_rq_priv = bfq_get_rq_private,
.put_rq_priv = bfq_put_rq_private,
- .init_icq = bfq_init_icq,
.exit_icq = bfq_exit_icq,
.insert_requests = bfq_insert_requests,
.dispatch_request = bfq_dispatch_request,
diff --git a/block/bfq-mq.h b/block/bfq-mq.h
index 6e1c0d8..41b9d33 100644
--- a/block/bfq-mq.h
+++ b/block/bfq-mq.h
@@ -345,9 +345,6 @@ struct bfq_io_cq {
uint64_t blkcg_serial_nr; /* the current blkcg serial */
#endif
- /* delayed work to exec the body of the the exit_icq handler */
- struct work_struct exit_icq_work;
-
/*
* Snapshot of the idle window before merging; taken to
* remember this value while the queue is merged, so as to be
--
2.10.0
^ permalink raw reply related
* Re: [PATCH 7/8] mq-deadline: add blk-mq adaptation of the deadline IO scheduler
From: Paolo Valente @ 2017-02-07 17:27 UTC (permalink / raw)
To: Jens Axboe; +Cc: Jens Axboe, linux-block, Linux-Kernal, osandov
In-Reply-To: <6effcdf0-ca5e-0ab9-6866-f144f1c1a635@fb.com>
> Il giorno 02 feb 2017, alle ore 22:32, Jens Axboe <axboe@fb.com> ha =
scritto:
>=20
> On 02/02/2017 02:15 PM, Paolo Valente wrote:
>>=20
>>> Il giorno 02 feb 2017, alle ore 16:30, Jens Axboe <axboe@fb.com> ha =
scritto:
>>>=20
>>> On 02/02/2017 02:19 AM, Paolo Valente wrote:
>>>> The scheme is clear. One comment, in case it could make sense and
>>>> avoid more complexity: since put_rq_priv is invoked in two =
different
>>>> contexts, process or interrupt, I didn't feel so confusing that, =
when
>>>> put_rq_priv is invoked in the context where the lock cannot be held
>>>> (unless one is willing to pay with irq disabling all the times), =
the
>>>> lock is not held, while, when invoked in the context where the lock
>>>> can be held, the lock is actually held, or must be taken.
>>>=20
>>> If you grab the same lock from put_rq_priv, yes, you must make it =
IRQ
>>> disabling in all contexts, and use _irqsave() from put_rq_priv. If =
it's
>>> just freeing resources, you could potentially wait and do that when
>>> someone else needs them, since that part will come from proces =
context.
>>> That would need two locks, though.
>>>=20
>>> As I said above, I would not worry about the IRQ disabling lock.
>>>=20
>>=20
>> I'm sorry, I focused only on the IRQ-disabling consequence of =
grabbing
>> a scheduler lock also in IRQ context. I thought it was a serious
>> enough issue to avoid this option. Yet there is also a deadlock
>> problem related to this option. In fact, the IRQ handler may preempt
>> some process-context code that already holds some other locks, and, =
if
>> some of these locks are already held by another process, which is
>> executing on another CPU and which then tries to take the scheduler
>> lock, or which happens to be preempted by an IRQ handler trying to
>> grab the scheduler lock, then a deadlock occurs. This is not just a
>> speculation, but a problem that did occur before I moved to a
>> deferred-work solution, and that can be readily reproduced. Before
>> moving to a deferred work solution, I tried various code =
manipulations
>> to avoid these deadlocks without resorting to deferred work, but at =
no
>> avail.
>=20
> There are two important rules here:
>=20
> 1) If a lock is ever used in interrupt context, anyone acquiring it =
must
> ensure that interrupts gets disabled.
>=20
> 2) If multiple locks are needed, they need to be acquired in the right
> order.
>=20
> Instead of talking in hypotheticals, be more specific. With the latest
> code, the scheduler lock should now be fine, there should be no cases
> where you are being invoked with it held. I'm assuming you are running
> with lockdep enabled on your kernel? Post the stack traces from your
> problem (and your code...), then we can take a look.
>=20
Hi Jens,
your last change (freeing requests outside merges) did remove two of
out three deadlock scenarios for which I turned some handlers into
deferred work items in bfq-mq. For the remaining one, I'm about to
send a separate email, with the description of the deadlock, together
with the patch that, applied on top of the bfq-mq branch, causes the
deadlock by turning moving back the body of exit_icq from a deferred
work to the exit_icq hook itself. And, yes, as I'll write below, I'm
finally about to share a branch containing bfq-mq.
> Don't punt to deferring work from your put_rq_private() function, =
that's
> a suboptimal work around. It needs to be fixed for real.
>=20
Yeah, sub-optimal also in terms of poor developer time: I spent a lot
of time letting that deferred work, and hopefully be a little
efficient! The actual problem has been that I preferred to try to get
to the bottom of those deadlocks on my own, and not to bother you also
on that issue. Maybe next time I will ask you one more question
instead of one less :)
>> At any rate, bfq seems now to work, so I can finally move from just
>> asking questions endlessly, to proposing actual code to discuss on.
>> I'm about to: port this version of bfq to your improved/fixed
>> blk-mq-sched version in for-4.11 (port postponed, to avoid =
introducing
>> further changes in code that did not yet wok), run more extensive
>> tests, polish commits a little bit, and finally share a branch.
>=20
> Post the code sooner rather than later. There are bound to be things
> that need to be improved or fixed up, let's start this process now. =
The
> framework is pretty much buttoned up at this point, so there's time to
> shift the attention a bit to a consumer of it.
>=20
Ok, to follow this suggestion of yours at 100%, I have postponed
several steps (removal of any invariant check or extra log message,
merging of the various files bfq is made of in just one file, code
polishing), and I'm about to share my current WIP branch in a
follow-up message.
Thanks,
Paolo
> --=20
> Jens Axboe
^ permalink raw reply
* [WIP PATCHSET 0/4] WIP branch for bfq-mq
From: Paolo Valente @ 2017-02-07 17:24 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo
Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie,
Paolo Valente
Hi,
I have finally pushed here [1] the current WIP branch of bfq for
blk-mq, which I have tentatively named bfq-mq.
This branch *IS NOT* meant for merging into mainline and contain code
that mau easily violate code style, and not only, in many
places. Commits implement the following main steps:
1) Add the last version of bfq for blk
2) Clone bfq source files into identical bfq-mq source files
3) Modify bfq-mq files to get a working version of bfq for blk-mq
(cgroups support not yet functional)
In my intentions, the main goals of this branch are:
1) Show, as soon as I could, the changes I made to let bfq-mq comply
with blk-mq-sched framework. I though this could be particularly
useful for Jens, being BFQ identical to CFQ in terms of hook
interfaces and io-context handling, and almost identical in terms
request-merging.
2) Enable people to test this first version bfq-mq. Code is purposely
overfull of log messages and invariant checks that halt the system on
failure (lock assertions, BUG_ONs, ...).
To make it easier to revise commits, I'm sending the patches that
transform bfq into bfq-mq (last four patches in the branch [1]). They
work on two files, bfq-mq-iosched.c and bfq-mq.h, which, at the
beginning, are just copies of bfq-iosched.c and bfq.h.
Thanks,
Paolo
[1] https://github.com/Algodev-github/bfq-mq
Paolo Valente (4):
blk-mq: pass bio to blk_mq_sched_get_rq_priv
Move thinktime from bic to bfqq
Embed bfq-ioc.c and add locking on request queue
Modify interface and operation to comply with blk-mq-sched
block/bfq-cgroup.c | 4 -
block/bfq-mq-iosched.c | 852 +++++++++++++++++++++++++++++------------------
block/bfq-mq.h | 65 ++--
block/blk-mq-sched.c | 8 +-
block/blk-mq-sched.h | 5 +-
include/linux/elevator.h | 2 +-
6 files changed, 567 insertions(+), 369 deletions(-)
--
2.10.0
^ 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