Hello, Joseph.
On Mon, Mar 05, 2018 at 11:03:16PM +0800, Joseph Qi wrote:
> +static void blkg_pd_offline(struct blkcg_gq *blkg)
> +{
> + int i;
> +
> + lockdep_assert_held(blkg->q->queue_lock);
> + lockdep_assert_held(&blkg->blkcg->lock);
> +
> + for (i = 0; i < BLKCG_MAX_POLS; i++) {
> + struct blkcg_policy *pol = blkcg_policy[i];
> +
> + if (blkg->pd[i] && !blkg->pd_offline[i] && pol->pd_offline_fn) {
> + pol->pd_offline_fn(blkg->pd[i]);
> + blkg->pd_offline[i] = true;
Can we move this flag into blkg_policy_data?
> + while (!hlist_empty(&blkcg->blkg_list)) {
> + struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
> + struct blkcg_gq,
> + blkcg_node);
> + struct request_queue *q = blkg->q;
> +
> + if (spin_trylock(q->queue_lock)) {
> + blkg_destroy(blkg);
> + spin_unlock(q->queue_lock);
> + } else {
> + spin_unlock_irq(&blkcg->lock);
> + cpu_relax();
> + spin_lock_irq(&blkcg->lock);
> + }
Can we factor out the above loop? It's something subtle and painful
and I think it'd be better to have it separated out and documented.
Other than that, looks great.
Thanks.
--
tejun
