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]

Message-ID: <x49my0tc3xz.fsf@segfault.boston.devel.redhat.com>
Date:	Mon, 04 Jan 2010 09:58:16 -0500
From:	Jeff Moyer <jmoyer@...hat.com>
To:	Shaohua Li <shaohua.li@...el.com>
Cc:	Corrado Zoccolo <czoccolo@...il.com>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	"jens.axboe\@oracle.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

Shaohua Li <shaohua.li@...el.com> writes:
> seek_mean could be very big sometimes, using it as close criteria is meaningless
> as this doen't improve any performance. So if it's big, let's fallback to
> default value.

I think the real problem is that we lost a check for a seeky queue.  The
close cooperator code initially was written to help interleaved
sequential I/Os.  Given that, I'm not a big fan of the proposed patch.
I'd rather see an extra check for CFQQ_SEEKY added to the close
cooperator code paths and leave cfq_rq_close alone.

Cheers,
Jeff

>
> Signed-off-by: Shaohua Li<shaohua.li@...el.com>
>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index e2f8046..e80bd47 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -1675,13 +1675,17 @@ static inline sector_t cfq_dist_from_last(struct cfq_data *cfqd,
>  #define CFQQ_SEEKY(cfqq)	((cfqq)->seek_mean > CFQQ_SEEK_THR)
>  
>  static inline int cfq_rq_close(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> -			       struct request *rq)
> +			       struct request *rq, bool for_preempt)
>  {
>  	sector_t sdist = cfqq->seek_mean;
>  
>  	if (!sample_valid(cfqq->seek_samples))
>  		sdist = CFQQ_SEEK_THR;
>  
> +	/* if seek_mean is big, using it as close criteria is meaningless */
> +	if (sdist > CFQQ_SEEK_THR && !for_preempt)
> +		sdist = CFQQ_SEEK_THR;
> +
>  	return cfq_dist_from_last(cfqd, rq) <= sdist;
>  }
>  
> @@ -1709,7 +1713,7 @@ 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 (cfq_rq_close(cfqd, cur_cfqq, __cfqq->next_rq, false))
>  		return __cfqq;
>  
>  	if (blk_rq_pos(__cfqq->next_rq) < sector)
> @@ -1720,7 +1724,7 @@ 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, false))
>  		return __cfqq;
>  
>  	return NULL;
> @@ -3143,7 +3147,7 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
>  	 * if this request is as-good as one we would expect from the
>  	 * current cfqq, let it preempt
>  	 */
> -	if (cfq_rq_close(cfqd, cfqq, rq))
> +	if (cfq_rq_close(cfqd, cfqq, rq, true))
>  		return true;
>  
>  	return false;
--
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/