linux-kernel - Re: [PATCH]cfq-iosched: don't take requests with long distence as close

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 15 Jan 2010 20:32:10 +0100
From:	Corrado Zoccolo <czoccolo@...il.com>
To:	"Zhang, Yanmin" <yanmin_zhang@...ux.intel.com>
Cc:	Jeff Moyer <jmoyer@...hat.com>, Shaohua Li <shaohua.li@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"jens.axboe@...cle.com" <jens.axboe@...cle.com>,
	"Zhang, Yanmin" <yanmin.zhang@...el.com>
Subject: Re: [PATCH]cfq-iosched: don't take requests with long distence as 
	close

On Tue, Jan 12, 2010 at 3:43 AM, Zhang, Yanmin
<yanmin_zhang@...ux.intel.com> wrote:
> On Mon, 2010-01-11 at 16:05 +0100, Corrado Zoccolo wrote:
> > On Mon, Jan 11, 2010 at 6:20 AM, Zhang, Yanmin
> > <yanmin_zhang@...ux.intel.com> wrote:
> > > On Thu, 2010-01-07 at 09:30 -0500, Jeff Moyer wrote:
> > Hi Yanmin,
> > are you testing Jeff's patch with your full fio script, instead of the
> > simplified one?
> Thanks for your reminder. I tested the patch with simplified one.
>
> > Since they are fixing the merging part, that happens only with the
> > full fio script.
> Ok. I tested the full fio script a moment ago and didn't find improvement.
> > >>
> > >> Shaohua Li noticed that cfq currently can merge with seeky queues, which
> > >> causes unwanted merge/unmerge activity.  We already know that the
> > >> cur_cfqq is not seeky, so this patch just makes sure that the non-seeky
> > >> queue is not merged with a seeky one.
> > >>
> > >> Signed-off-by: Jeff Moyer <jmoyer@...hat.com>
> > >>
> > >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> > >> index 8df4fe5..3db9050 100644
> > >> --- a/block/cfq-iosched.c
> > >> +++ b/block/cfq-iosched.c
> > >> @@ -1677,6 +1677,10 @@ static inline int cfq_rq_close(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> > >>       return cfq_dist_from_last(cfqd, rq) <= sdist;
> > >>  }
> > >>
> > >> +/*
> > >> + * Search for a cfqq that is issuing non-seeky I/Os within the seek
> > >> + * mean of the current cfqq.
> > >> + */
> > >>  static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
> > >>                                   struct cfq_queue *cur_cfqq)
> > >>  {
> > >> @@ -1701,7 +1705,14 @@ static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
> > >>        * will contain the closest sector.
> > >>        */
> > >>       __cfqq = rb_entry(parent, struct cfq_queue, p_node);
> > >> -     if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq))
> > >> +     /*
> > >> +      * If the cfqq does not have enough seek samples, assume it is
> > >> +      * sequential until proven otherwise.  If it is assumed that the
> > >> +      * queue is seeky first, then the close cooperator detection logic
> > >> +      * may never trigger as one queue strays further from the other(s).
> > >> +      */
> > >> +     if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq) &&
> > >> +         (!sample_valid(__cfqq->seek_samples) || !CFQQ_SEEKY(__cfqq)))
> > >>               return __cfqq;
> > >>
> > >>       if (blk_rq_pos(__cfqq->next_rq) < sector)
> > >> @@ -1712,7 +1723,8 @@ static struct cfq_queue *cfqq_close(struct cfq_data *cfqd,
> > >>               return NULL;
> > >>
> > >>       __cfqq = rb_entry(node, struct cfq_queue, p_node);
> > >> -     if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq))
> > >> +     if (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq) &&
> > >> +         (!sample_valid(__cfqq->seek_samples) || !CFQQ_SEEKY(__cfqq)))
> > >>               return __cfqq;
> > >>
> > >>       return NULL;
Hi Jeff,
I think this patch has the same flaw as Shaohua's.
The seekiness check that you introduce in cfq_rq_close is already
present in its caller, cfq_close_cooperator, so it is not effective.
Up to now, the only patch that improves this situation is the one that
changes the unmerge policy to unmerge after a single time slice.

Thanks,
Corado
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/