Re: [PATCH v3] blkcg: allocate struct blkcg_gq outside request queue spinlock

From	Tejun Heo <tj@kernel.org>
Newsgroups	linux.kernel
Subject	Re: [PATCH v3] blkcg: allocate struct blkcg_gq outside request queue spinlock
Date	2017-03-04 20:30 +0100
Message-ID	<thnxn-5re-15@gated-at.bofh.it> (permalink)
References	<th13Q-5HD-15@gated-at.bofh.it> <th6Zz-1i8-5@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

Hello, Tahsin.

Generally looks good.  Please see below.

> @@ -184,24 +185,48 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
> +	if (unlikely(!wb_congested)) {
>  		ret = -ENOMEM;
>  		goto err_put_css;
> +	} else if (unlikely(!blkg)) {
> +		ret = -ENOMEM;
> +		goto err_put_congested;
>  	}

I'm not sure this error handling logic is correct.  We can have any
combo of alloc failure here, right?

> @@ -694,9 +695,20 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
>  	blkg = blkg_lookup(blkcg, q);
>  	if (unlikely(!blkg)) {
>  		spin_lock_irq(q->queue_lock);
> -		blkg = blkg_lookup_create(blkcg, q);
> -		if (IS_ERR(blkg))
> -			blkg = NULL;
> +
> +		/*
> +		 * This could be the first entry point of blkcg implementation
> +		 * and we shouldn't allow anything to go through for a bypassing
> +		 * queue.
> +		 */
> +		if (likely(!blk_queue_bypass(q))) {
> +			blkg = blkg_lookup_create(blkcg, q,
> +						  GFP_NOWAIT | __GFP_NOWARN,
> +						  NULL);
> +			if (IS_ERR(blkg))
> +				blkg = NULL;
> +		}

Heh, this seems a bit too fragile.  I get that this covers both call
paths into lookup_create which needs the checking, but it seems a bit
too nasty to do it this way.  Would it be ugly if we factor out both
policy enabled and bypass tests into a helper and have inner
__blkg_lookup_create() which skips it?  We can call the same helper
from blkg_create() when @pol.  Sorry that this is involving so much
bikeshedding.

Thanks!

-- 
tejun

Thread

Re: [PATCH] blkcg: allocate struct blkcg_gq outside request queue  spinlock Tejun Heo <tj@kernel.org> - 2017-03-01 18:00 +0100
  Re: [PATCH v2] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-02 01:20 +0100
  [PATCH v2] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-02 04:00 +0100
    Re: [PATCH v2] blkcg: allocate struct blkcg_gq outside request queue  spinlock Tejun Heo <tj@kernel.org> - 2017-03-02 20:50 +0100
      Re: [PATCH v2] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-02 23:50 +0100
        Re: [PATCH v2] blkcg: allocate struct blkcg_gq outside request queue  spinlock Tejun Heo <tj@kernel.org> - 2017-03-03 20:30 +0100
          [PATCH v3] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-04 02:50 +0100
            Re: [PATCH v3] blkcg: allocate struct blkcg_gq outside request queue  spinlock Tejun Heo <tj@kernel.org> - 2017-03-04 20:30 +0100
              Re: [PATCH v4] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-05 15:30 +0100
              [PATCH v4] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-05 15:30 +0100
                Re: [PATCH v4] blkcg: allocate struct blkcg_gq outside request queue  spinlock Tejun Heo <tj@kernel.org> - 2017-03-06 21:10 +0100
                [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-09 09:10 +0100
                Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue  spinlock Tejun Heo <tj@kernel.org> - 2017-03-09 19:30 +0100
                Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue  spinlock Jens Axboe <axboe@kernel.dk> - 2017-03-11 23:50 +0100
                Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue  spinlock Jens Axboe <axboe@kernel.dk> - 2017-03-12 00:00 +0100
                Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-12 05:40 +0100
                Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue  spinlock Jens Axboe <axboe@kernel.dk> - 2017-03-13 15:40 +0100
                Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue spinlock Tahsin Erdogan <tahsin@google.com> - 2017-03-13 17:20 +0100
            Re: [lkp-robot] [blkcg] ad63af3cb7: BUG:sleeping_function_called_from_invalid_context_at_mm/slab.h Tahsin Erdogan <tahsin@google.com> - 2017-03-09 09:10 +0100

csiph-web