On Tue, Jul 06, 2010 at 10:19:12PM -0400, Christoph Hellwig wrote:
> > static void bdi_queue_work(struct backing_dev_info *bdi,
> > struct wb_writeback_work *work)
> > {
> > + trace_wb_sched_queue(bdi, work);
>
> Okay, I'm getting more nitpicky with every repost, but the naming here
> seems odd.
>
> First why the unreadle wb prefix instead of a proper writeback?
> And the sched in here seems rather useless, I'd just call it
>
> trace_writeback_queue
Done.
>
> > @@ -74,12 +85,16 @@ static void bdi_queue_work(struct backing_dev_info *bdi,
> > * it gets created and wakes up, we'll run this work.
> > */
> > if (unlikely(!bdi->wb.task)) {
> > + trace_wb_sched_default(bdi, work);
>
> trace_writeback_no_thread
>
> > wake_up_process(default_backing_dev_info.wb.task);
> > } else {
> > struct bdi_writeback *wb = &bdi->wb;
> >
> > - if (wb->task)
> > + if (wb->task) {
> > + trace_wb_sched_task(bdi, work);
>
> trace_writeback_wake
>
> then again I don't think we actually need this, if we got the
> trace_writeback_queue event, and no trace_writeback_no_thread it's
> implicit that we got here.
OK, fixed.
> > wake_up_process(wb->task);
> > + } else
> > + trace_wb_sched_notask(bdi, work);
>
> This case can't actually happen as we checked for the task a few lines
> above.
Right, my bad.
> Might be worth to throw in preparation patch in your series
> to something like:
>
>
> if (unlikely(!bdi->wb.task)) {
> wake_up_process(default_backing_dev_info.wb.task);
> - } else {
> - struct bdi_writeback *wb = &bdi->wb;
> -
> - if (wb->task)
> - wake_up_process(wb->task);
> - }
> + return;
> + }
> +
> + wake_up_process(bdi->wb.task);
>
> > }
> > }
Perhaps. making the above changes means it's not necessary...
> >
> > @@ -95,8 +110,10 @@ __bdi_start_writeback(struct backing_dev_info *bdi, long nr_pages,
> > */
> > work = kzalloc(sizeof(*work), GFP_ATOMIC);
> > if (!work) {
> > - if (bdi->wb.task)
> > + if (bdi->wb.task) {
> > + trace_wb_sched_nowork(bdi, work);
>
> trace_writeback_nowork
>
> > + TP_fast_assign(
> > + strncpy(__entry->name, dev_name(bdi->dev), 32);
> > + __entry->nr_pages = work ? work->nr_pages : 0;
> > + __entry->sb_dev = work ? work->sb ? work->sb->s_dev : 0 : 0;
> > + __entry->sync_mode = work ? work->sync_mode : -1;
> > + __entry->for_kupdate = work ? work->for_kupdate : -1;
> > + __entry->range_cyclic = work ? work->range_cyclic : -1;
> > + __entry->for_background = work ? work->for_background : -1;
> > + ),
>
> If you had a separate class for those tracepoints that do not have
> a work item the code would be a lot simpler.
Yup, fixed.
I'll post a new version in a short while.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
