[PATCH 1/2] btrfs: Cleanup the btrfs_workqueue related function type

[PATCH 1/2] btrfs: Cleanup the btrfs_workqueue related function type

[quwenruo]

The new btrfs_workqueue still use open-coded function defition,
this patch will change them into btrfs_func_t type which is much the
same as kernel workqueue.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/async-thread.c |  6 +++---
 fs/btrfs/async-thread.h | 20 +++++++++++---------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
index a709585..d8c07e5 100644
--- a/fs/btrfs/async-thread.c
+++ b/fs/btrfs/async-thread.c
@@ -255,9 +255,9 @@ static void normal_work_helper(struct work_struct *arg)
 }
 
 void btrfs_init_work(struct btrfs_work *work,
-		     void (*func)(struct btrfs_work *),
-		     void (*ordered_func)(struct btrfs_work *),
-		     void (*ordered_free)(struct btrfs_work *))
+		     btrfs_func_t func,
+		     btrfs_func_t ordered_func,
+		     btrfs_func_t ordered_free)
 {
 	work->func = func;
 	work->ordered_func = ordered_func;
diff --git a/fs/btrfs/async-thread.h b/fs/btrfs/async-thread.h
index 08d7174..0a891cd 100644
--- a/fs/btrfs/async-thread.h
+++ b/fs/btrfs/async-thread.h
@@ -23,11 +23,13 @@
 struct btrfs_workqueue;
 /* Internal use only */
 struct __btrfs_workqueue;
+struct btrfs_work;
+typedef void (*btrfs_func_t)(struct btrfs_work *arg);
 
 struct btrfs_work {
-	void (*func)(struct btrfs_work *arg);
-	void (*ordered_func)(struct btrfs_work *arg);
-	void (*ordered_free)(struct btrfs_work *arg);
+	btrfs_func_t func;
+	btrfs_func_t ordered_func;
+	btrfs_func_t ordered_free;
 
 	/* Don't touch things below */
 	struct work_struct normal_work;
@@ -37,13 +39,13 @@ struct btrfs_work {
 };
 
 struct btrfs_workqueue *btrfs_alloc_workqueue(char *name,
-						     int flags,
-						     int max_active,
-						     int thresh);
+					      int flags,
+					      int max_active,
+					      int thresh);
 void btrfs_init_work(struct btrfs_work *work,
-		     void (*func)(struct btrfs_work *),
-		     void (*ordered_func)(struct btrfs_work *),
-		     void (*ordered_free)(struct btrfs_work *));
+		     btrfs_func_t func,
+		     btrfs_func_t ordered_func,
+		     btrfs_func_t ordered_free);
 void btrfs_queue_work(struct btrfs_workqueue *wq,
 		      struct btrfs_work *work);
 void btrfs_destroy_workqueue(struct btrfs_workqueue *wq);
-- 
1.9.0

[PATCH 1/2] btrfs: Cleanup the btrfs_workqueue related function type

[quwenruo]

The new btrfs_workqueue still use open-coded function defition,
this patch will change them into btrfs_func_t type which is much the
same as kernel workqueue.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/async-thread.c |  6 +++---
 fs/btrfs/async-thread.h | 20 +++++++++++---------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
index a709585..d8c07e5 100644
--- a/fs/btrfs/async-thread.c
+++ b/fs/btrfs/async-thread.c
@@ -255,9 +255,9 @@ static void normal_work_helper(struct work_struct *arg)
 }
 
 void btrfs_init_work(struct btrfs_work *work,
-		     void (*func)(struct btrfs_work *),
-		     void (*ordered_func)(struct btrfs_work *),
-		     void (*ordered_free)(struct btrfs_work *))
+		     btrfs_func_t func,
+		     btrfs_func_t ordered_func,
+		     btrfs_func_t ordered_free)
 {
 	work->func = func;
 	work->ordered_func = ordered_func;
diff --git a/fs/btrfs/async-thread.h b/fs/btrfs/async-thread.h
index 08d7174..0a891cd 100644
--- a/fs/btrfs/async-thread.h
+++ b/fs/btrfs/async-thread.h
@@ -23,11 +23,13 @@
 struct btrfs_workqueue;
 /* Internal use only */
 struct __btrfs_workqueue;
+struct btrfs_work;
+typedef void (*btrfs_func_t)(struct btrfs_work *arg);
 
 struct btrfs_work {
-	void (*func)(struct btrfs_work *arg);
-	void (*ordered_func)(struct btrfs_work *arg);
-	void (*ordered_free)(struct btrfs_work *arg);
+	btrfs_func_t func;
+	btrfs_func_t ordered_func;
+	btrfs_func_t ordered_free;
 
 	/* Don't touch things below */
 	struct work_struct normal_work;
@@ -37,13 +39,13 @@ struct btrfs_work {
 };
 
 struct btrfs_workqueue *btrfs_alloc_workqueue(char *name,
-						     int flags,
-						     int max_active,
-						     int thresh);
+					      int flags,
+					      int max_active,
+					      int thresh);
 void btrfs_init_work(struct btrfs_work *work,
-		     void (*func)(struct btrfs_work *),
-		     void (*ordered_func)(struct btrfs_work *),
-		     void (*ordered_free)(struct btrfs_work *));
+		     btrfs_func_t func,
+		     btrfs_func_t ordered_func,
+		     btrfs_func_t ordered_free);
 void btrfs_queue_work(struct btrfs_workqueue *wq,
 		      struct btrfs_work *work);
 void btrfs_destroy_workqueue(struct btrfs_workqueue *wq);
-- 
1.9.0

[PATCH 2/2] btrfs: Add ftrace for btrfs_workqueue

[quwenruo]

Add ftrace for btrfs_workqueue for further workqueue tunning.
This patch needs to applied after the workqueue replace patchset.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/async-thread.c      |  7 ++++
 include/trace/events/btrfs.h | 82 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
index d8c07e5..00623dd 100644
--- a/fs/btrfs/async-thread.c
+++ b/fs/btrfs/async-thread.c
@@ -24,6 +24,7 @@
 #include <linux/freezer.h>
 #include <linux/workqueue.h>
 #include "async-thread.h"
+#include "ctree.h"
 
 #define WORK_DONE_BIT 0
 #define WORK_ORDER_DONE_BIT 1
@@ -210,6 +211,7 @@ static void run_ordered_work(struct __btrfs_workqueue *wq)
 		 */
 		if (test_and_set_bit(WORK_ORDER_DONE_BIT, &work->flags))
 			break;
+		trace_btrfs_ordered_sched(work);
 		spin_unlock_irqrestore(lock, flags);
 		work->ordered_func(work);
 
@@ -223,6 +225,7 @@ static void run_ordered_work(struct __btrfs_workqueue *wq)
 		 * with the lock held though
 		 */
 		work->ordered_free(work);
+		trace_btrfs_all_work_done(work);
 	}
 	spin_unlock_irqrestore(lock, flags);
 }
@@ -246,12 +249,15 @@ static void normal_work_helper(struct work_struct *arg)
 		need_order = 1;
 	wq = work->wq;
 
+	trace_btrfs_work_sched(work);
 	thresh_exec_hook(wq);
 	work->func(work);
 	if (need_order) {
 		set_bit(WORK_DONE_BIT, &work->flags);
 		run_ordered_work(wq);
 	}
+	if (!need_order)
+		trace_btrfs_all_work_done(work);
 }
 
 void btrfs_init_work(struct btrfs_work *work,
@@ -280,6 +286,7 @@ static inline void __btrfs_queue_work(struct __btrfs_workqueue *wq,
 		spin_unlock_irqrestore(&wq->list_lock, flags);
 	}
 	queue_work(wq->normal_wq, &work->normal_work);
+	trace_btrfs_work_queued(work);
 }
 
 void btrfs_queue_work(struct btrfs_workqueue *wq,
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index 3176cdc..c346919 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -21,6 +21,7 @@ struct btrfs_block_group_cache;
 struct btrfs_free_cluster;
 struct map_lookup;
 struct extent_buffer;
+struct btrfs_work;
 
 #define show_ref_type(type)						\
 	__print_symbolic(type,						\
@@ -982,6 +983,87 @@ TRACE_EVENT(free_extent_state,
 		  (void *)__entry->ip)
 );
 
+DECLARE_EVENT_CLASS(btrfs__work,
+
+	TP_PROTO(struct btrfs_work *work),
+
+	TP_ARGS(work),
+
+	TP_STRUCT__entry(
+		__field(	void *,	work			)
+		__field(	void *, wq			)
+		__field(	void *,	func			)
+		__field(	void *,	ordered_func		)
+		__field(	void *,	ordered_free		)
+	),
+
+	TP_fast_assign(
+		__entry->work		= work;
+		__entry->wq		= work->wq;
+		__entry->func		= work->func;
+		__entry->ordered_func	= work->ordered_func;
+		__entry->ordered_free	= work->ordered_free;
+	),
+
+	TP_printk("work=%p, wq=%p, func=%p, ordered_func=%p, ordered_free=%p",
+		  __entry->work, __entry->wq, __entry->func,
+		  __entry->ordered_func, __entry->ordered_free)
+);
+
+/* For situiations that the work is freed */
+DECLARE_EVENT_CLASS(btrfs__work__done,
+
+	TP_PROTO(struct btrfs_work *work),
+
+	TP_ARGS(work),
+
+	TP_STRUCT__entry(
+		__field(	void *,	work			)
+	),
+
+	TP_fast_assign(
+		__entry->work		= work;
+	),
+
+	TP_printk("work->%p", __entry->work)
+);
+
+DEFINE_EVENT(btrfs__work, btrfs_work_queued,
+
+	TP_PROTO(struct btrfs_work *work),
+
+	TP_ARGS(work)
+);
+
+DEFINE_EVENT(btrfs__work, btrfs_work_sched,
+
+	TP_PROTO(struct btrfs_work *work),
+
+	TP_ARGS(work)
+);
+
+DEFINE_EVENT(btrfs__work, btrfs_normal_work_done,
+
+	TP_PROTO(struct btrfs_work *work),
+
+	TP_ARGS(work)
+);
+
+DEFINE_EVENT(btrfs__work__done, btrfs_all_work_done,
+
+	TP_PROTO(struct btrfs_work *work),
+
+	TP_ARGS(work)
+);
+
+DEFINE_EVENT(btrfs__work, btrfs_ordered_sched,
+
+	TP_PROTO(struct btrfs_work *work),
+
+	TP_ARGS(work)
+);
+
+
 #endif /* _TRACE_BTRFS_H */
 
 /* This part must be outside protection */
-- 
1.9.0

Re: [PATCH 1/2] btrfs: Cleanup the btrfs_workqueue related function type

[David Sterba]

On Thu, Mar 06, 2014 at 04:19:50AM +0000, quwenruo@cn.fujitsu.com wrote:
> @@ -23,11 +23,13 @@
>  struct btrfs_workqueue;
>  /* Internal use only */
>  struct __btrfs_workqueue;
> +struct btrfs_work;
> +typedef void (*btrfs_func_t)(struct btrfs_work *arg);

I don't see what's wrong with the non-typedef type, CodingStyle
discourages from using typedefs in general (Chapter 5).

The name btrfs_func_t is a generic, if you really need to use a typedef
here, please change it to something closer to the workqueues, eg.
btrfs_work_func_t.

Re: [PATCH 1/2] btrfs: Cleanup the btrfs_workqueue related function type

[quwenruo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 954 bytes --]

On Fri, 14 Mar 2014 15:39:03 +0100, David Sterba wrote:
> On Thu, Mar 06, 2014 at 04:19:50AM +0000, quwenruo@cn.fujitsu.com wrote:
>> @@ -23,11 +23,13 @@
>>   struct btrfs_workqueue;
>>   /* Internal use only */
>>   struct __btrfs_workqueue;
>> +struct btrfs_work;
>> +typedef void (*btrfs_func_t)(struct btrfs_work *arg);
> I don't see what's wrong with the non-typedef type, CodingStyle
> discourages from using typedefs in general (Chapter 5).
>
> The name btrfs_func_t is a generic, if you really need to use a typedef
> here, please change it to something closer to the workqueues, eg.
> btrfs_work_func_t.
>
btrfs_func_t is just following the work_func_t naming style,
for btrfs it's only used to reduce the length of prototype definition.

Since btrfs_func_t is only used in btrfs, this patch can be ignored.

Thanks
Quÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [PATCH 1/2] btrfs: Cleanup the btrfs_workqueue related function type

[David Sterba]

On Sun, Mar 16, 2014 at 12:41:17AM +0000, quwenruo@cn.fujitsu.com wrote:
> On Fri, 14 Mar 2014 15:39:03 +0100, David Sterba wrote:
> > On Thu, Mar 06, 2014 at 04:19:50AM +0000, quwenruo@cn.fujitsu.com wrote:
> >> @@ -23,11 +23,13 @@
> >>   struct btrfs_workqueue;
> >>   /* Internal use only */
> >>   struct __btrfs_workqueue;
> >> +struct btrfs_work;
> >> +typedef void (*btrfs_func_t)(struct btrfs_work *arg);
> > I don't see what's wrong with the non-typedef type, CodingStyle
> > discourages from using typedefs in general (Chapter 5).
> >
> > The name btrfs_func_t is a generic, if you really need to use a typedef
> > here, please change it to something closer to the workqueues, eg.
> > btrfs_work_func_t.
> >
> btrfs_func_t is just following the work_func_t naming style,
> for btrfs it's only used to reduce the length of prototype definition.
> 
> Since btrfs_func_t is only used in btrfs, this patch can be ignored.

Thanks for considerations.  In case of the generic workqueues interface
it makes sense to provide a shortcut to the callback definition because
it appears in many structures.