![]() | ||||||||||
Messages in this thread |
Jens Axboe wrote:
> On Thu, Oct 20 2005, Tejun Heo wrote:
>
>> Hi, Jens.
>>
>>On Thu, Oct 20, 2005 at 12:00:03PM +0200, Jens Axboe wrote:
>>
>>>On Wed, Oct 19 2005, Tejun Heo wrote:
>>>
>>>>@@ -40,6 +40,11 @@
>>>> static DEFINE_SPINLOCK(elv_list_lock);
>>>> static LIST_HEAD(elv_list);
>>>>
>>>>+static inline sector_t rq_last_sector(struct request *rq)
>>>>+{
>>>>+ return rq->sector + rq->nr_sectors;
>>>>+}
>>>
>>>Slightly misnamed, since it's really the sector after the last sector
>>>:-)
>>>
>>>I've renamed that to rq_end_sector() instead.
>>
>> Maybe rename request_queue->last_sector too?
>
>
> Yeah agree.
>
>
>>>>+/*
>>>>+ * Insert rq into dispatch queue of q. Queue lock must be held on
>>>>+ * entry. If sort != 0, rq is sort-inserted; otherwise, rq will be
>>>>+ * appended to the dispatch queue. To be used by specific elevators.
>>>>+ */
>>>>+void elv_dispatch_insert(request_queue_t *q, struct request *rq, int sort)
>>>>+{
>>>>+ sector_t boundary;
>>>>+ unsigned max_back;
>>>>+ struct list_head *entry;
>>>>+
>>>>+ if (!sort) {
>>>>+ /* Specific elevator is performing sort. Step away. */
>>>>+ q->last_sector = rq_last_sector(rq);
>>>>+ q->boundary_rq = rq;
>>>>+ list_add_tail(&rq->queuelist, &q->queue_head);
>>>>+ return;
>>>>+ }
>>>>+
>>>>+ boundary = q->last_sector;
>>>>+ max_back = q->max_back_kb * 2;
>>>>+ boundary = boundary > max_back ? boundary - max_back : 0;
>>>
>>>This looks really strange, what are you doing with boundary here?
>>>
>>
>> Taking backward seeking into account. I reasonsed that if specific
>>elevator chooses the next request with backward seeking,
>>elv_dispatch_insert() shouldn't change the order because that may
>>result in less efficient seek pattern. At the second thought,
>>specific elevators always perform sorting by itself in such cases, so
>>this seems unnecessary. I think we can strip this thing out.
>
>
> It wasn't so much the actual action as the logic. You overwrite boundary
> right away and it seems really strange to complare the absolute rq
> location with the max_back_in_sectors offset?
>
> But lets just kill it, care to send a patch when I have pushed this
> stuff out?
>
Sure.
>
>>>> if (rq == q->last_merge)
>>>> q->last_merge = NULL;
>>>>
>>>>+ if (!q->boundary_rq || q->boundary_rq == rq) {
>>>>+ q->last_sector = rq_last_sector(rq);
>>>>+ q->boundary_rq = NULL;
>>>>+ }
>>>
>>>This seems to be the only place where you clear ->boundary_rq, that
>>>can't be right. What about rq-to-rq merging, ->boundary_rq could be
>>>freed and you wont notice. Generally I don't really like keeping
>>>pointers to rqs around, it's given us problems in the past with the
>>>last_merge bits even. For now I've added a clear of this in
>>>__blk_put_request() as well.
>>
>> Oh, please don't do that. Now, it's guaranteed that there are only
>>three paths a request can travel.
>>
>> set_req_fn ->
>>
>> i. add_req_fn -> (merged_fn ->)* -> dispatch_fn -> activate_req_fn ->
>> (deactivate_req_fn -> activate_req_fn ->)* -> completed_req_fn
>> ii. add_req_fn -> (merged_fn ->)* -> merge_req_fn
>> iii. [none]
>>
>> -> put_req_fn
>>
>> These three are the only paths a request can travel. Also note that
>>dispatched requests don't get merged. So, after dispatched, the only
>>way out is via elevator_complete_req_fn and that's why that's the only
>>place ->boundary_rq is cleared. I've also documented above in biodoc
>>so that we can simplify codes knowing above information.
>
>
> Ah, it's my mistake, you only set it on dispatch. I was thinking it had
> an earlier life time, so there's no bug there at all. Thanks for
> clearing that up.
>
Thanks.
--
tejun
-
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/
| |||||||||
| Last update: 2005-10-20 14:21 [from the cache] ©2003-2008 | ||||||||||