Hello, On Tue, Feb 28, 2017 at 03:51:27PM -0800, Tahsin Erdogan wrote: > On Tue, Feb 28, 2017 at 2:47 PM, Tejun Heo <tj@xxxxxxxxxx> wrote: > >> + if (!blkcg_policy_enabled(q, pol)) { > >> + ret = -EOPNOTSUPP; > >> + goto fail; > > > > Pulling this out of the queue_lock doesn't seem safe to me. This > > function may end up calling into callbacks of disabled policies this > > way. > > I will move this to within the lock. To make things safe, I am also > thinking of rechecking both blkcg_policy_enabled() and > blk_queue_bypass() after reacquiring the locks in each iteration. > > >> + parent = blkcg_parent(blkcg); > >> + while (parent && !__blkg_lookup(parent, q, false)) { > >> + pos = parent; > >> + parent = blkcg_parent(parent); > >> + } > > > > Hmm... how about adding @new_blkg to blkg_lookup_create() and calling > > it with non-NULL @new_blkg until it succeeds? Wouldn't that be > > simpler? > > > >> + > >> + new_blkg = blkg_alloc(pos, q, GFP_KERNEL); > > The challenge with that approach is creating a new_blkg with the right > blkcg before passing to blkg_lookup_create(). blkg_lookup_create() > walks down the hierarchy and will try to fill the first missing entry > and the preallocated new_blkg must have been created with the right > blkcg (feel free to send a code fragment if you think I am > misunderstanding the suggestion). Ah, indeed, but we can break out allocation of blkg and its initialization, right? It's a bit more work but then we'd be able to do something like. retry: new_blkg = alloc; lock; sanity checks; blkg = blkg_lookup_and_create(..., new_blkg); if (!blkg) { unlock; goto retry; } Thanks. -- tejun