Thread (13 messages) 13 messages, 3 authors, 2017-05-25

Re: [PATCH BUGFIX] block, bfq: access and cache blkg data only when safe

From: Tejun Heo <tj@kernel.org>
Date: 2017-05-19 14:54:49
Also in: lkml

Hello, Paolo.

On Fri, May 19, 2017 at 10:39:08AM +0200, Paolo Valente wrote:
Operations on blkg objects in blk-cgroup are protected with the
request_queue lock, which is no more the lock that protects
I/O-scheduler operations in blk-mq. The latter are now protected with
finer-grained per-scheduler-instance locks. As a consequence, if blkg
and blkg-related objects are accessed in a blk-mq I/O scheduler, it is
possible to have races, unless proper care is taken for these
accesses. BFQ does access these objects, and does incur these races.

This commit addresses this issue without introducing further locks, by
exploiting the following facts.  Destroy operations on a blkg invoke,
as a first step, hooks of the scheduler associated with the blkg. And
these hooks are executed with bfqd->lock held for BFQ. As a
consequence, for any blkg associated with the request queue an
instance of BFQ is attached to, we are guaranteed that such a blkg is
not destroyed and that all the pointers it contains are consistent,
(only) while that instance is holding its bfqd->lock. A blkg_lookup
performed with bfqd->lock held then returns a fully consistent blkg,
which remains consistent until this lock is held.

In view of these facts, this commit caches any needed blkg data (only)
when it (safely) detects a parent-blkg change for an internal entity,
and, to cache these data safely, it gets the new blkg, through a
blkg_lookup, and copies data while keeping the bfqd->lock held. As of
now, BFQ needs to cache only the path of the blkg, which is used in
the bfq_log_* functions.

This commit also removes or updates some stale comments on locking
issues related to blk-cgroup operations.
For a quick fix, this is fine but I think it'd be much better to
update blkcg core so that we protect lookups with rcu and refcnt the
blkg with percpu refs so that we can use blkcg correctly for all
purposes with blk-mq.  There's no reason to hold up the immediate fix
for that but it'd be nice to at least note what we should be doing in
the longer term in a comment.

Thanks.

-- 
tejun
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help