Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.
Tejun, I do not have another iteration of the patch yet. we have two customers who reported this problem. One of them has not responded to requests to test a/any patch and other one is not able to recreate this with the latest release. And I am not able to recreate this problem on my own but working on a setup. So if you have any patch, I would be happy to test/deploy it on a first setup I can get hold of. I was also trying to figure out whether this is some refcounting issue or not since I was thinking that put by anybody other than blk_cleanup_queue() should not result on freeing/blk_release_queue() i.e. put by blk_cleanup_queue() should always be the last put. Regards, Shirish On Fri, Jul 4, 2014 at 12:18 PM, Tejun Heo wrote: > On Tue, Jun 24, 2014 at 05:43:40PM -0400, Tejun Heo wrote: >> Hello, >> >> On Tue, Jun 24, 2014 at 04:37:58PM -0500, Shirish Pargaonkar wrote: >> > When we start from blk_cleanup_queue(), we put request queue in bypass >> > mode, >> > drain it (and service queues), and then destroy blkcgs (explicitly) >> > >> > When we start from blk_release_queue(), we do not drain first and then >> > destroy blkcgs. So if we destroy blkcg and then call (implicitly) and >> > bail out of >> > blk_drain_queue, we would not have drained the service queues which >> > is not what we want. >> >> I'm not really following you. What do you mean "when we start from >> blk_release_queue()"? blk_release_queue() is called after the last >> put which can only follow blk_cleanup_queue() if the queue is fully >> initialized. The queue is already in bypass mode and fully drained by >> the time control reaches blk_release_queue(). Module [un]load >> re-invoking the path doesn't change anything. >> >> > I do not see any harm in waiting till end to release blkcgs (as I >> > understand). >> >> Well, the harm there is not freeing those blkgs unless all the blkcg >> policies are unloaded which is usually never on most systems. > > Ping. We have a patch which makes this problem more visible. Are you > still planning to re-spin the patch? > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.
On Tue, Jun 24, 2014 at 05:43:40PM -0400, Tejun Heo wrote: > Hello, > > On Tue, Jun 24, 2014 at 04:37:58PM -0500, Shirish Pargaonkar wrote: > > When we start from blk_cleanup_queue(), we put request queue in bypass mode, > > drain it (and service queues), and then destroy blkcgs (explicitly) > > > > When we start from blk_release_queue(), we do not drain first and then > > destroy blkcgs. So if we destroy blkcg and then call (implicitly) and > > bail out of > > blk_drain_queue, we would not have drained the service queues which > > is not what we want. > > I'm not really following you. What do you mean "when we start from > blk_release_queue()"? blk_release_queue() is called after the last > put which can only follow blk_cleanup_queue() if the queue is fully > initialized. The queue is already in bypass mode and fully drained by > the time control reaches blk_release_queue(). Module [un]load > re-invoking the path doesn't change anything. > > > I do not see any harm in waiting till end to release blkcgs (as I > > understand). > > Well, the harm there is not freeing those blkgs unless all the blkcg > policies are unloaded which is usually never on most systems. Ping. We have a patch which makes this problem more visible. Are you still planning to re-spin the patch? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.
Hello, On Tue, Jun 24, 2014 at 04:37:58PM -0500, Shirish Pargaonkar wrote: > When we start from blk_cleanup_queue(), we put request queue in bypass mode, > drain it (and service queues), and then destroy blkcgs (explicitly) > > When we start from blk_release_queue(), we do not drain first and then > destroy blkcgs. So if we destroy blkcg and then call (implicitly) and > bail out of > blk_drain_queue, we would not have drained the service queues which > is not what we want. I'm not really following you. What do you mean "when we start from blk_release_queue()"? blk_release_queue() is called after the last put which can only follow blk_cleanup_queue() if the queue is fully initialized. The queue is already in bypass mode and fully drained by the time control reaches blk_release_queue(). Module [un]load re-invoking the path doesn't change anything. > I do not see any harm in waiting till end to release blkcgs (as I understand). Well, the harm there is not freeing those blkgs unless all the blkcg policies are unloaded which is usually never on most systems. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.
When we start from blk_cleanup_queue(), we put request queue in bypass mode, drain it (and service queues), and then destroy blkcgs (explicitly) When we start from blk_release_queue(), we do not drain first and then destroy blkcgs. So if we destroy blkcg and then call (implicitly) and bail out of blk_drain_queue, we would not have drained the service queues which is not what we want. I do not see any harm in waiting till end to release blkcgs (as I understand). Regards, Shirish On Tue, Jun 24, 2014 at 4:09 PM, Tejun Heo wrote: > Hello, > > On Mon, Jun 23, 2014 at 01:52:25PM -0500, shirishpargaon...@gmail.com wrote: >> From: Shirish Pargaonkar >> >> Release blkg infrastructure only after last policy is deactivated >> (i.e. let blkg_destroy_all() be called only from blkcg_deactivate_policy()) >> >> Otherwise, module can oops because root_blkg gets assigned NULL during >> cleanup and we attempt draining the service queues via root_blkg afterwords. > > I'm not sure this fix makes sense. Cleanup path oopses on an already > freed resource. How can the solution be not freeing? Why not simply > make blkcg_drain_queue() bail if the blkgs are all destroyed? The > whole thing is fully synchronized with the queuelock, right? > > Can you please also cc Jens when you post the next iteration? > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.
Hello, On Mon, Jun 23, 2014 at 01:52:25PM -0500, shirishpargaon...@gmail.com wrote: > From: Shirish Pargaonkar > > Release blkg infrastructure only after last policy is deactivated > (i.e. let blkg_destroy_all() be called only from blkcg_deactivate_policy()) > > Otherwise, module can oops because root_blkg gets assigned NULL during > cleanup and we attempt draining the service queues via root_blkg afterwords. I'm not sure this fix makes sense. Cleanup path oopses on an already freed resource. How can the solution be not freeing? Why not simply make blkcg_drain_queue() bail if the blkgs are all destroyed? The whole thing is fully synchronized with the queuelock, right? Can you please also cc Jens when you post the next iteration? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.
These are the type of oopses observed... 1) Oops: Kernel access of bad area, sig: 11 [#1] SMP NR_CPUS=2048 NUMA pSeries Modules linked in: bfa ch scsi_dh_alua fcoe megaraid_sas qla2xxx libosd scsi_debug(-) lpfc qla4xxx stex iscsi_boot_sysfs bnx2fc cnic uio libfcoe libfc scsi_dh_emc crc_t10dif scsi_transport_fc ps3stor_lib scsi_dh libiscsi scsi_transport_iscsi af_packet xfs libcrc32c autofs4 btrfs xor raid6_pq sr_mod cdrom ohci_pci ohci_hcd ehci_hcd ibmvscsi(X) scsi_transport_srp scsi_tgt virtio_blk virtio_net usbcore usb_common virtio_pci virtio_ring virtio dm_mod sg scsi_mod [last unloaded: sd_mod] Supported: Yes CPU: 0 PID: 5757 Comm: modprobe Tainted: GWX 3.12.14-1-default #1 task: c0007a029510 ti: c3124000 task.ti: c3124000 NIP: c03d503c LR: c03d1c60 CTR: c03db920 REGS: c3126c70 TRAP: 0300 Tainted: GWX (3.12.14-1-default) MSR: 80019033 CR: 24082822 XER: SOFTE: 0 CFAR: 3fffb3f5855c DAR: 0028, DSISR: 4000 GPR00: c03d1c60 c3126ef0 c10779d8 GPR04: 0001 c0007d1c7628 0001 GPR08: c03db920 c03ad3d0 GPR12: 24082828 cfe8 10020bc0 GPR16: 0001 1003a068 100204a0 GPR20: 3fffeeb812d0 3fffeeb812c8 GPR24: 01001ade02b0 c00077121100 c00078b39c40 GPR28: c00078b3a290 c00078b39c40 c0007e087480 c00078b39c40 NIP [c03d503c] .blk_throtl_drain+0x2c/0x180 LR [c03d1c60] .blkcg_drain_queue+0x10/0x30 PACATMSCRATCH [8001d033] Call Trace: [c3126ef0] [c3126f90] 0xc3126f90 (unreliable) [c3126f80] [c03d1c60] .blkcg_drain_queue+0x10/0x30 [c3126ff0] [c03ad74c] .__blk_drain_queue+0xac/0x240 [c3127090] [c03afd18] .blk_queue_bypass_start+0xa8/0x110 [c3127110] [c03d0af4] .blkcg_deactivate_policy+0x64/0x210 [c31271c0] [c03d5310] .blk_throtl_exit+0x40/0x70 [c3127240] [c03d1cd8] .blkcg_exit_queue+0x58/0x80 [c31272c0] [c03b38f0] .blk_release_queue+0x30/0x150 [c3127340] [c03e4750] .kobject_cleanup+0xd0/0x230 [c31273d0] [c03ad3e4] .blk_put_queue+0x14/0x30 [c3127440] [d05a368c] .scsi_device_dev_release_usercontext+0x11c/0x180 [scsi_mod] [c31274f0] [c00d7b08] .execute_in_process_context+0x88/0xa0 [c3127560] [d05a355c] .scsi_device_dev_release+0x1c/0x30 [scsi_mod] [c31275d0] [c04c5ee4] .device_release+0x54/0xe0 [c3127660] [c03e4750] .kobject_cleanup+0xd0/0x230 [c31276f0] [c04c64dc] .put_device+0x1c/0x30 [c3127760] [d05a44a8] .__scsi_remove_device+0xe8/0x150 [scsi_mod] [c31277e0] [d05a2034] .scsi_forget_host+0x94/0xa0 [scsi_mod] [c3127860] [d0593574] .scsi_remove_host+0x94/0x1a0 [scsi_mod] [c31278f0] [d3f43344] .sdebug_driver_remove+0x44/0xf0 [scsi_debug] [c3127990] [c04cc03c] .__device_release_driver+0x9c/0x120 [c3127a10] [c04cc0f0] .device_release_driver+0x30/0x60 [c3127a90] [c04cb58c] .bus_remove_device+0x12c/0x1d0 [c3127b20] [c04c67cc] .device_del+0x15c/0x250 [c3127bb0] [c04c68ec] .device_unregister+0x2c/0x90 [c3127c30] [d3f42d94] .sdebug_remove_adapter+0x94/0xe0 [scsi_debug] [c3127cb0] [d3f488e0] .scsi_debug_exit+0x34/0xdfc [scsi_debug] [c3127d30] [c0142ad4] .SyS_delete_module+0x1f4/0x340 [c3127e30] [c0009efc] syscall_exit+0x0/0x7c Instruction dump: 4bfffd18 7c0802a6 fba1ffe8 fbc1fff0 fbe1fff8 7c7d1b78 f8010010 f821ff71 ebc30700 e93e00a0 3860 e92905c8 4bd7b541 6000 7c7f1b79 ---[ end trace e7bf9df70e311596 ]--- 2) modprobe -r scsi_debug cpu 0x3: Vector: 300 (Data Access) at [c002db203300] pc: c03d10fc: blk_throtl_drain+0x3c/0x190 lr: c03cd7b8: blkcg_drain_queue+0x28/0x40 sp: c002db203580 msr: 80019033 dar: 28 dsisr: 4000 current = 0xc002d4fe9510 paca= 0xc7e40a80 softe: 0 irq_happened: 0x01 pid = 6783, comm = modprobe enter ? for help [c002db2035c0] c03cd7b8 blkcg_drain_queue+0x28/0x40 [c002db2035f0] c03a640c __blk_drain_queue+0xbc/0x250 [c002db203640] c03a8bb8 blk_queue_bypass_start+0xb8/0x120 [c002db203680] c03cc548 blkcg_deactivate_policy+0x78/0x220 [c002db2036d0] c03d7670 cfq_exit_queue+0x120/0x170 [c002db203720] c03a0bec elevator_exit+0x5c/0x90 [c002db203750] c03ad008 blk_release_queue+0x98/0x160 [c002db203780] c03e2124 kobject_cleanup+0xd4/0x240 [c002db203800] c
[PATCH] blkio: Release blkg infrastructure only after last policy is deactivated.
From: Shirish Pargaonkar
Release blkg infrastructure only after last policy is deactivated
(i.e. let blkg_destroy_all() be called only from blkcg_deactivate_policy())
Otherwise, module can oops because root_blkg gets assigned NULL during
cleanup and we attempt draining the service queues via root_blkg afterwords.
Signed-off-by: Shirish Pargaonkar
Reported-and-tested-by: Sachin Sant
---
block/blk-cgroup.c | 15 ---
block/blk-sysfs.c | 2 +-
2 files changed, 1 insertion(+), 16 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 069bc20..3920de6 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -878,21 +878,6 @@ void blkcg_drain_queue(struct request_queue *q)
blk_throtl_drain(q);
}
-/**
- * blkcg_exit_queue - exit and release blkcg part of request_queue
- * @q: request_queue being released
- *
- * Called from blk_release_queue(). Responsible for exiting blkcg part.
- */
-void blkcg_exit_queue(struct request_queue *q)
-{
- spin_lock_irq(q->queue_lock);
- blkg_destroy_all(q);
- spin_unlock_irq(q->queue_lock);
-
- blk_throtl_exit(q);
-}
-
/*
* We cannot support shared io contexts, as we have no mean to support
* two tasks with the same ioc in two different groups without major rework
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 23321fb..ee24cd2 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -503,7 +503,7 @@ static void blk_release_queue(struct kobject *kobj)
blk_sync_queue(q);
- blkcg_exit_queue(q);
+ blk_throtl_exit(q);
if (q->elevator) {
spin_lock_irq(q->queue_lock);
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
