Message ID | 4E09B457.9050800@parallels.com |
---|---|
State | New, archived |
Headers | show |
On Tue, Jun 28, 2011 at 03:00:39PM +0400, Konstantin Khlebnikov wrote: > Vivek Goyal wrote: > >This patch series seems to be working for me. I did testing for ext4 only. > >This series is based on for-3.1/core branch of Jen's block tree. > >Konstantin, can you please give it a try and see if it fixes your > >issue. > > It works for me too, for ext3 and ext4, on top 3.0-rc5, after these trivial fixes: > > --- a/block/cfq-iosched.c > +++ b/block/cfq-iosched.c > @@ -1511,7 +1519,7 @@ static void cfq_add_rq_rb(struct request *rq) > * if that happens, put the alias on the dispatch list > */ > while ((__alias = elv_rb_add(&cfqq->sort_list, rq)) != NULL) > - cfq_dispatch_insert(cfqd->queue, __alias); > + cfq_dispatch_insert(cfqd->queue, __alias, false); > > if (!cfq_cfqq_on_rr(cfqq)) > cfq_add_cfqq_rr(cfqd, cfqq); > @@ -3797,12 +3797,11 @@ cfq_set_depends_on_task(struct request_queue *q, struct task_struct *tsk) > */ > rcu_read_lock(); > if (task_blkio_cgroup(current) == task_blkio_cgroup(tsk)) > - return; > - rcu_read_unlock(); > + goto out_unlock_rcu; > > cic = cfq_cic_lookup(cfqd, current->io_context); > if (!cic) > - return; > + goto out_unlock_rcu; You have done this change because you want to keep cfq_cic_lookup() also in rcu read side critical section? I am assuming that it works even without this. Though keeping it under rcu is probably more correct as cic objects are freed in rcu manner. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Vivek Goyal wrote: > On Tue, Jun 28, 2011 at 03:00:39PM +0400, Konstantin Khlebnikov wrote: >> Vivek Goyal wrote: >>> This patch series seems to be working for me. I did testing for ext4 only. >>> This series is based on for-3.1/core branch of Jen's block tree. >>> Konstantin, can you please give it a try and see if it fixes your >>> issue. >> >> It works for me too, for ext3 and ext4, on top 3.0-rc5, after these trivial fixes: >> >> --- a/block/cfq-iosched.c >> +++ b/block/cfq-iosched.c >> @@ -1511,7 +1519,7 @@ static void cfq_add_rq_rb(struct request *rq) >> * if that happens, put the alias on the dispatch list >> */ >> while ((__alias = elv_rb_add(&cfqq->sort_list, rq)) != NULL) >> - cfq_dispatch_insert(cfqd->queue, __alias); >> + cfq_dispatch_insert(cfqd->queue, __alias, false); >> >> if (!cfq_cfqq_on_rr(cfqq)) >> cfq_add_cfqq_rr(cfqd, cfqq); >> @@ -3797,12 +3797,11 @@ cfq_set_depends_on_task(struct request_queue *q, struct task_struct *tsk) >> */ >> rcu_read_lock(); >> if (task_blkio_cgroup(current) == task_blkio_cgroup(tsk)) >> - return; >> - rcu_read_unlock(); >> + goto out_unlock_rcu; >> >> cic = cfq_cic_lookup(cfqd, current->io_context); >> if (!cic) >> - return; >> + goto out_unlock_rcu; > > You have done this change because you want to keep cfq_cic_lookup() also > in rcu read side critical section? I am assuming that it works even > without this. Though keeping it under rcu is probably more correct as > cic objects are freed in rcu manner. No, your just forgot to relese rcu in case task_blkio_cgroup(current) == task_blkio_cgroup(tsk) > > Thanks > Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 28, 2011 at 06:42:57PM +0400, Konstantin Khlebnikov wrote: > Vivek Goyal wrote: > >On Tue, Jun 28, 2011 at 03:00:39PM +0400, Konstantin Khlebnikov wrote: > >>Vivek Goyal wrote: > >>>This patch series seems to be working for me. I did testing for ext4 only. > >>>This series is based on for-3.1/core branch of Jen's block tree. > >>>Konstantin, can you please give it a try and see if it fixes your > >>>issue. > >> > >>It works for me too, for ext3 and ext4, on top 3.0-rc5, after these trivial fixes: > >> > >>--- a/block/cfq-iosched.c > >>+++ b/block/cfq-iosched.c > >>@@ -1511,7 +1519,7 @@ static void cfq_add_rq_rb(struct request *rq) > >> * if that happens, put the alias on the dispatch list > >> */ > >> while ((__alias = elv_rb_add(&cfqq->sort_list, rq)) != NULL) > >>- cfq_dispatch_insert(cfqd->queue, __alias); > >>+ cfq_dispatch_insert(cfqd->queue, __alias, false); > >> > >> if (!cfq_cfqq_on_rr(cfqq)) > >> cfq_add_cfqq_rr(cfqd, cfqq); > >>@@ -3797,12 +3797,11 @@ cfq_set_depends_on_task(struct request_queue *q, struct task_struct *tsk) > >> */ > >> rcu_read_lock(); > >> if (task_blkio_cgroup(current) == task_blkio_cgroup(tsk)) > >>- return; > >>- rcu_read_unlock(); > >>+ goto out_unlock_rcu; > >> > >> cic = cfq_cic_lookup(cfqd, current->io_context); > >> if (!cic) > >>- return; > >>+ goto out_unlock_rcu; > > > >You have done this change because you want to keep cfq_cic_lookup() also > >in rcu read side critical section? I am assuming that it works even > >without this. Though keeping it under rcu is probably more correct as > >cic objects are freed in rcu manner. > > No, your just forgot to relese rcu in case task_blkio_cgroup(current) == task_blkio_cgroup(tsk) Oh, that's right. Thanks for catching this. I will fix it. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/block/cfq-iosched.c +++ b/block/cfq-iosched.c @@ -1511,7 +1519,7 @@ static void cfq_add_rq_rb(struct request *rq) * if that happens, put the alias on the dispatch list */ while ((__alias = elv_rb_add(&cfqq->sort_list, rq)) != NULL) - cfq_dispatch_insert(cfqd->queue, __alias); + cfq_dispatch_insert(cfqd->queue, __alias, false); if (!cfq_cfqq_on_rr(cfqq)) cfq_add_cfqq_rr(cfqd, cfqq); @@ -3797,12 +3797,11 @@ cfq_set_depends_on_task(struct request_queue *q, struct task_struct *tsk) */ rcu_read_lock(); if (task_blkio_cgroup(current) == task_blkio_cgroup(tsk)) - return; - rcu_read_unlock(); + goto out_unlock_rcu; cic = cfq_cic_lookup(cfqd, current->io_context); if (!cic) - return; + goto out_unlock_rcu; task_lock(tsk); spin_lock_irq(q->queue_lock); @@ -3847,6 +3846,8 @@ cfq_set_depends_on_task(struct request_queue *q, struct task_struct *tsk) out_unlock: spin_unlock_irq(q->queue_lock); task_unlock(tsk); +out_unlock_rcu: + rcu_read_unlock(); } -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org