Re: [PATCH 1/3] writeback: Initial tracing support

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com