Re: [PATCH 33/36] block: add io_context->active_ref

[Tejun Heo]

Hello, Vivek.

On Wed, Feb 22, 2012 at 01:47:36PM -0500, Vivek Goyal wrote:
> On Tue, Feb 21, 2012 at 05:47:00PM -0800, Tejun Heo wrote:
> > Currently ioc->nr_tasks is used to decide two things - whether an ioc
> > is done issuing IOs and whether it's shared by multiple tasks.  This
> > patch separate out the first into ioc->active_ref, which is acquired
> > and released using {get|put}_io_context_active() respectively.
> > 
> > This will be used to associate bio's with a given task.  This patch
> > doesn't introduce any visible behavior change.
> 
> Do we really need to spilit nr_tasks and active_ref stuff. IIUC, you
> are creatinv active_ref so that if somebody has taken active_ref in
> the system, then CFQ will idle and wait for more IO. But what if that
> bio gets throttled. Or gets delayed somewhere higher in the stack. Then
> we are unnecessarily idling in CFQ.

I think it's better to keep the distinction clear.  CFQ idling while
bios being throttled is unrelated to task exiting or not.  It can
easily happen while the task is live and well and if that's a
situation which needs to be addressed it better be solved for the
generic case rather than modifying something which is mostly
unrelated.  It *may* be helpful now but if you stack up unrelated
hacks like that, it quickly becomes a difficult-to-maintain mess where
modifying something completely unrelated breaks something else on the
other side.

It indeed is ugly to have ref, active_ref and nr_tasks tho.  If we can
remove CLONE_IO, nr_tasks will go away with it.  Maybe, I don't know.
Let's see.

Thanks.

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