Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1591525
| From | Tahsin Erdogan <tahsin@google.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v2] blkcg: allocate struct blkcg_gq outside request queue spinlock |
| Date | 2017-03-02 23:50 +0100 |
| Message-ID | <tgHHP-ok-11@gated-at.bofh.it> (permalink) |
| References | <tgfLA-5N5-15@gated-at.bofh.it> <tgmDn-2sk-9@gated-at.bofh.it> <tgETE-6XA-9@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Thu, Mar 2, 2017 at 11:32 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Tahsin.
>
> On Wed, Mar 01, 2017 at 03:43:19PM -0800, Tahsin Erdogan wrote:
>> @@ -258,18 +258,22 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
>> struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
>> - struct request_queue *q)
>> + struct request_queue *q, bool wait_ok)
>
> I'm okay with this direction but it probably would be better if the
> parameter is gfp_mask and we branch on __GFP_WAIT in the function.
Agreed, gfp mask is better as a parameter.
>
>> {
>> struct blkcg_gq *blkg;
>>
>> @@ -300,7 +304,30 @@ struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
>> parent = blkcg_parent(parent);
>> }
>>
>> - blkg = blkg_create(pos, q, NULL);
>> + if (wait_ok) {
>> + struct blkcg_gq *new_blkg;
>> +
>> + spin_unlock_irq(q->queue_lock);
>> + rcu_read_unlock();
>> +
>> + new_blkg = blkg_alloc(pos, q, GFP_KERNEL);
>> +
>> + rcu_read_lock();
>> + spin_lock_irq(q->queue_lock);
>> +
>> + if (unlikely(!new_blkg))
>> + return ERR_PTR(-ENOMEM);
>> +
>> + if (unlikely(blk_queue_bypass(q))) {
>> + blkg_free(new_blkg);
>> + return ERR_PTR(blk_queue_dying(q) ?
>> + -ENODEV : -EBUSY);
>> + }
>> +
>> + blkg = blkg_create(pos, q, new_blkg);
>> + } else
>> + blkg = blkg_create(pos, q, NULL);
>
> So, while I'm okay with the approach, now we're creating a hybrid
> approach where we have both pre-allocation and allocation mode
> altering mechanisms. If we're going to take this route, I think the
> right thing to do is passing down @gfp_mask all the way down to
> blkg_create().
>
>> @@ -789,6 +816,7 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>> {
>> struct gendisk *disk;
>> struct blkcg_gq *blkg;
>> + struct request_queue *q;
>> struct module *owner;
>> unsigned int major, minor;
>> int key_len, part, ret;
>> @@ -812,18 +840,27 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol,
>> return -ENODEV;
>> }
>>
>> + q = disk->queue;
>> +
>> rcu_read_lock();
>> - spin_lock_irq(disk->queue->queue_lock);
>> + spin_lock_irq(q->queue_lock);
>>
>> - if (blkcg_policy_enabled(disk->queue, pol))
>> - blkg = blkg_lookup_create(blkcg, disk->queue);
>> - else
>> + if (blkcg_policy_enabled(q, pol)) {
>> + blkg = blkg_lookup_create(blkcg, q, true /* wait_ok */);
>> +
>> + /*
>> + * blkg_lookup_create() may have dropped and reacquired the
>> + * queue lock. Check policy enabled state again.
>> + */
>> + if (!IS_ERR(blkg) && unlikely(!blkcg_policy_enabled(q, pol)))
>> + blkg = ERR_PTR(-EOPNOTSUPP);
>
> And let blkg_create() verify these conditions after releasing and
> regrabbing the lock.
>
> This also means that the init path can simply pass in GFP_KERNEL.
I tried that approach, but I encountered two issues that complicate things:
1) Pushing down blk_queue_bypass(q) check in blkg_create() doesn't
quite work because when blkcg_init_queue() calls blkg_create(), the
queue is still in bypassing mode.
2) Pushing down blkcg_policy_enabled() doesn't work well either,
because blkcg_init_queue() doesn't have a policy to pass down. We
could let it pass a NULL parameter but that would make blkg_create
more ugly.
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll 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