lkml.org 
[lkml]   [2017]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] blkcg: allocate struct blkcg_gq outside request queue spinlock
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

\
 
 \ /
  Last update: 2017-03-04 20:23    [W:0.400 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site