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
