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