linux-toradex.git/block/blk-ioc.c, branch v3.4.49

block: fix ioc leak in put_io_context

2012-03-14T14:34:48+00:00

When put_io_context is called, if ioc->icq_list is empty and refcount
is 1, kernel will not free the ioc.

This is caught by following kmemleak:

unreferenced object 0xffff880036349fe0 (size 216):
  comm "sh", pid 2137, jiffies 4294931140 (age 290579.412s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    01 00 01 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
  backtrace:
    [] kmemleak_alloc+0x26/0x50
    [] kmem_cache_alloc_node+0x1cc/0x2a0
    [] create_io_context_slowpath+0x27/0x130
    [] get_task_io_context+0xbb/0xf0
    [] copy_process+0x188e/0x18b0
    [] do_fork+0x11b/0x420
    [] sys_clone+0x28/0x30
    [] stub_clone+0x13/0x20
    [] 0xffffffffffffffff

ioc should be freed if ioc->icq_list is empty.
Signed-off-by: Xiaotian Feng 
Acked-by: Vivek Goyal 
Acked-by: Tejun Heo 
Signed-off-by: Jens Axboe

block: exit_io_context() should call elevator_exit_icq_fn()

2012-02-15T08:45:53+00:00

While updating locking, b2efa05265 "block, cfq: unlink
cfq_io_context's immediately" moved elevator_exit_icq_fn() invocation
from exit_io_context() to the final ioc put.  While this doesn't cause
catastrophic failure, it effectively removes task exit notification to
elevator and cause noticeable IO performance degradation with CFQ.

On task exit, CFQ used to immediately expire the slice if it was being
used by the exiting task as no more IO would be issued by the task;
however, after b2efa05265, the notification is lost and disk could sit
idle needlessly, leading to noticeable IO performance degradation for
certain workloads.

This patch renames ioc_exit_icq() to ioc_destroy_icq(), separates
elevator_exit_icq_fn() invocation into ioc_exit_icq() and invokes it
from exit_io_context().  ICQ_EXITED flag is added to avoid invoking
the callback more than once for the same icq.

Walking icq_list from ioc side and invoking elevator callback requires
reverse double locking.  This may be better implemented using RCU;
unfortunately, using RCU isn't trivial.  e.g. RCU protection would
need to cover request_queue and queue_lock switch on cleanup makes
grabbing queue_lock from RCU unsafe.  Reverse double locking should
do, at least for now.

Signed-off-by: Tejun Heo 
Reported-and-bisected-by: Shaohua Li 
LKML-Reference: 
Tested-by: Shaohua Li 
Signed-off-by: Jens Axboe

block: simplify ioc_release_fn()

2012-02-15T08:45:52+00:00

Reverse double lock dancing in ioc_release_fn() can be simplified by
just using trylock on the queue_lock and back out from ioc lock on
trylock failure.  Simplify it.

Signed-off-by: Tejun Heo 
Tested-by: Shaohua Li 
Signed-off-by: Jens Axboe

block: replace icq->changed with icq->flags

2012-02-15T08:45:49+00:00

icq->changed was used for ICQ_*_CHANGED bits.  Rename it to flags and
access it under ioc->lock instead of using atomic bitops.
ioc_get_changed() is added so that the changed part can be fetched and
cleared as before.

icq->flags will be used to carry other flags.

Signed-off-by: Tejun Heo 
Tested-by: Shaohua Li 
Signed-off-by: Jens Axboe

block: fix lockdep warning on io_context release put_io_context()

2012-02-11T11:37:25+00:00

11a3122f6c "block: strip out locking optimization in put_io_context()"
removed ioc_lock depth lockdep annoation along with locking
optimization; however, while recursing from put_io_context() is no
longer possible, ioc_release_fn() may still end up putting the last
reference of another ioc through elevator, which wlil grab ioc->lock
triggering spurious (as the ioc is always different one) A-A deadlock
warning.

As this can only happen one time from ioc_release_fn(), using non-zero
subclass from ioc_release_fn() is enough.  Use subclass 1.

Signed-off-by: Tejun Heo 
Signed-off-by: Jens Axboe

block: strip out locking optimization in put_io_context()

2012-02-07T06:51:30+00:00

put_io_context() performed a complex trylock dancing to avoid
deferring ioc release to workqueue.  It was also broken on UP because
trylock was always assumed to succeed which resulted in unbalanced
preemption count.

While there are ways to fix the UP breakage, even the most
pathological microbench (forced ioc allocation and tight fork/exit
loop) fails to show any appreciable performance benefit of the
optimization.  Strip it out.  If there turns out to be workloads which
are affected by this change, simpler optimization from the discussion
thread can be applied later.

Signed-off-by: Tejun Heo 
LKML-Reference: <1328514611.21268.66.camel@sli10-conroe>
Signed-off-by: Jens Axboe

block: fix ioc locking warning

2012-02-06T07:57:29+00:00

Meelis reported a warning:

WARNING: at kernel/timer.c:1122 run_timer_softirq+0x199/0x1ec()
Hardware name: 939Dual-SATA2
timer: cfq_idle_slice_timer+0x0/0xaa preempt leak: 00000102 -> 00000103
Modules linked in: sr_mod cdrom videodev media drm_kms_helper ohci_hcd ehci_hcd v4l2_compat_ioctl32 usbcore i2c_ali15x3 snd_seq drm snd_timer snd_seq
Pid: 0, comm: swapper Not tainted 3.3.0-rc2-00110-gd125666 #176
Call Trace:
   [] warn_slowpath_common+0x7e/0x96
 [] ? cfq_slice_expired+0x1d/0x1d
 [] warn_slowpath_fmt+0x41/0x43
 [] ? cfq_idle_slice_timer+0xa1/0xaa
 [] ? cfq_slice_expired+0x1d/0x1d
 [] run_timer_softirq+0x199/0x1ec
 [] ? timekeeping_get_ns+0x12/0x31
 [] ? apic_write+0x11/0x13
 [] __do_softirq+0x74/0xfa
 [] call_softirq+0x1a/0x30
 [] do_softirq+0x31/0x68
 [] irq_exit+0x3d/0xa3
 [] smp_apic_timer_interrupt+0x6b/0x77
 [] apic_timer_interrupt+0x69/0x70
   [] ? sched_clock_cpu+0x73/0x7d
 [] ? sched_clock_cpu+0x73/0x7d
 [] ? default_idle+0x1e/0x32
 [] ? default_idle+0x18/0x32
 [] cpu_idle+0x87/0xd1
 [] rest_init+0x85/0x89
 [] start_kernel+0x2eb/0x2f8
 [] x86_64_start_reservations+0x7e/0x82
 [] x86_64_start_kernel+0xf0/0xf7

this_q == locked_q is possible. There are two problems here:
1. In UP case, there is preemption counter issue as spin_trylock always
successes.
2. In SMP case, the loop breaks too earlier.

Signed-off-by: Shaohua Li 
Reported-by: Meelis Roos 
Reported-by: Knut Petersen 
Tested-by: Knut Petersen 
Signed-off-by: Jens Axboe

block: remove WARN_ON_ONCE() in exit_io_context()

2011-12-27T17:52:16+00:00

6e736be7 "block: make ioc get/put interface more conventional and fix
race on alloction" added WARN_ON_ONCE() in exit_io_context() which
triggers if !PF_EXITING.  All tasks hitting exit_io_context() from
task exit should have PF_EXITING set but task struct tearing down
after fork failure calls into the function without PF_EXITING,
triggering the condition.

  WARNING: at block/blk-ioc.c:234 exit_io_context+0x40/0x92()
  Pid: 17090, comm: trinity Not tainted 3.2.0-rc6-next-20111222-sasha-dirty #77
  Call Trace:
   [] warn_slowpath_common+0x8f/0xb2
   [] warn_slowpath_null+0x18/0x1a
   [] exit_io_context+0x40/0x92
   [] copy_process+0x126f/0x1453
   [] do_fork+0x120/0x3e9
   [] sys_clone+0x26/0x28
   [] stub_clone+0x13/0x20
  ---[ end trace a2e4eb670b375238 ]---

Reported-by: Sasha Levin 
Signed-off-by: Tejun Heo 
Signed-off-by: Jens Axboe

block: an exiting task should be allowed to create io_context

2011-12-25T13:29:14+00:00

While fixing io_context creation / task exit race condition,
6e736be7f2 "block: make ioc get/put interface more conventional and
fix race on alloction" also prevented an exiting (%PF_EXITING) task
from creating its own io_context.  This is incorrect as exit path may
issue IOs, e.g. from exit_files(), and if those IOs are the first ones
issued by the task, io_context needs to be created to process the IOs.

Combined with the existing problem of io_context / io_cq creation
failure having the possibility of stalling IO, this problem results in
deterministic full IO lockup with certain workloads.

Fix it by allowing io_context creation regardless of %PF_EXITING for
%current.

Signed-off-by: Tejun Heo 
Reported-by: Andrew Morton 
Reported-by: Hugh Dickins 
Signed-off-by: Jens Axboe

block: ioc_cgroup_changed() needs to be exported

2011-12-19T09:36:44+00:00

With the ioc changed, ioc_cgroup_changed() can be used by modular
code. So ensure that it is exported.

Reported-by: Stephen Rothwell 
Signed-off-by: Jens Axboe