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

[Christoph Hellwig <hch@infradead.org>]

>  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

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

>  			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.  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);

>  	}
>  }
>  
> @@ -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.

> +DECLARE_EVENT_CLASS(wb_thread_class,
> +	TP_PROTO(struct backing_dev_info *bdi),
> +	TP_ARGS(bdi),
> +	TP_STRUCT__entry(
> +		__array(char, name, 32)
> +	),
> +	TP_fast_assign(
> +		strncpy(__entry->name, dev_name(bdi->dev), 32);
> +	),
> +	TP_printk("bdi %s",
> +		  __entry->name
> +	)

In fact you already have one.