Re: [PATCH 1/2]block: optimize non-queueable flush request drive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello,

On Tue, May 03, 2011 at 02:44:31PM +0800, Shaohua Li wrote:
> > As I've said several times already, I really don't like this magic
> > being done in the completion path.  Can't you detect the condition on
> > issue of the second/following flush and append it to the running list?
>
> hmm, don't understand it. blk_flush_complete_seq is called when the
> second flush is issued. or do you mean do this when the second flush is
> issued to disk? but when the second flush is issued the first flush is
> already finished.

Ah, okay, my bad.  That's the next sequence logic, so the right place.
Still, please do the followings.

* Put it in a separate patch.

* Preferably, detect the actual condition (back to back flush) rather
  than the queueability test unless it's too complicated.

* Please make pending/running paths look more symmetrical.

> > If you already have tried that but this way still seems better, can
> > you please explain why?
> > 
> > Also, this is a separate logic.  Please put it in a separate patch.
> > The first patch should implement queue holding while flushing, which
> > should remove the regression, right?
>
> ok. holding queue has no performance gain in my test, but it reduced a
> lot of request requeue.

No, holding the queue should remove the regression completely.  Please
read on.

> > Hmmm... why do you need separate ->flush_exclusive_running?  Doesn't
> > pending_idx != running_idx already have the same information?
>
> when pending_idx != running_idx, flush request is added into queue tail,
> but this doesn't mean flush request is dispatched to disk. there might
> be other requests in the queue head, which we should dispatch. And flush
> request might be reqeueud. Just checking pending_idx != running_idx will
> cause queue hang because we thought flush is dispatched and then hold
> the queue, but actually flush isn't dispatched yet, the queue should
> dispatch other normal requests.

Don't hold elv_next_request().  Hold ->elevator_dispatch_fn().

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux