Date: Sun, 4 Oct 2009 22:59:55 +0200
From: Jens Axboe <jens.axboe@oracle.com>
To: Corrado Zoccolo <czoccolo@gmail.com>
Cc: Linux-Kernel <linux-kernel@vger.kernel.org>
Subject: Re: bug in elevator/cfq regarding start_time?
Message-ID: <20091004205955.GM26573@kernel.dk>
References: <4e5e476b0910041354o54c95626g974264976adaf53c@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4e5e476b0910041354o54c95626g974264976adaf53c@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1388
Lines: 32

On Sun, Oct 04 2009, Corrado Zoccolo wrote:
> Hi Jens,
> I found that the start_time is not monotonic in the cfqq fifo_list.
> The problem seems to be that:
> * attempt_merge overrides one of the start_time:
>         if (time_after(req->start_time, next->start_time))
>                 req->start_time = next->start_time;
> * then it calls elv_merge_requests, that in turn invokes cfq's
> elevator_merge_req_fn, i.e. cfq_merged_requests
> * cfq_merged_requests checks start_times to decide whether requests
> need to be repositioned
>         if (!list_empty(&rq->queuelist) && !list_empty(&next->queuelist) &&
>             time_before(next->start_time, rq->start_time))
>                 list_move(&rq->queuelist, &next->queuelist);
> 
> Other I/O schedulers are not affected, since none of them uses
> start_time explicitly, but they have a copy in one of the private
> fields.

Good spotting! So we can either add the same copy in cfq, or we can just
move the ->start_time check and assignment in attempt_merge() to post
the elv_merge_requests() call. I think the latter would work fine, but
I'll check and fix tomorrow.

-- 
Jens Axboe

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