Re: [PATCH 5/9] writeback: support > 1 flusher thread per bdi

[Jens Axboe]

On Thu, Aug 06 2009, Jan Kara wrote:
>   Actually, looking again that the work struct "state" field has lots of
> free bits. I think the code looks nicer with the attached patch, what do
> you think?
> 
> > >   2) I'd introduce a flag with the meaning: free the work when you are
> > > done. Obviusly this flag makes sence only with dynamically allocated work
> > > structure. There would be no "on stack" flag.
> > >   3) I'd create a function:
> > > bdi_wait_work_submitted()
> > >   which you'd have to call whenever you didn't set the flag and want to
> > > free the work (either explicitely, or via returning from a function which
> > > has the structure on stack).
> > >   It would do:
> > > bdi_wait_on_work_clear(work);
> > > call_rcu(&work->rcu_head, bdi_work_free);
> > > 
> > >   wb_work_complete() would just depending on the flag setting either
> > > completely do away with the work struct or just do bdi_work_clear().
> > > 
> > >   IMO that would make the code easier to check and also less prone to
> > > errors (currently you have to think twice when you have to wait for the rcu
> > > period, call bdi_work_free, etc.).
> > 
> > Didn't we go over all that last time, too?
>   Well, probably about something similar. But this time I have a patch ;-)
> Compile tested only... IMO it looks nicer this way as it wraps up all the
> details of work freeing into one function.

The first patch looks nice and obvious, I'll fold that in with the
original patch if you don't mind. It's definitely cleaner, instead of
overloading the pointer.

The second one I'd rather hold off on, I've run over the existing code
many times and tested it heavily threaded and know it's safe. So I'd
rather not introduce any drastic changes there so close to 2.6.32, but
I'd be happy to revisit this soon after merge. OK?

-- 
Jens Axboe

--
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/