[sheepdog] [PATCH] init trace for work queue before journal init

[Hitoshi Mitake]

At Mon, 24 Nov 2014 00:18:11 +0800,
Jinzhi Chen wrote:
> 
> [1  <multipart/alternative (7bit)>]
> [1.1  <text/plain; utf-8 (quoted-printable)>]
> hi hitoshi,
> I’m  so sorry that this patch introduce a bug in the dog command.
> recently  I read the  source code of dog , it seems that 
> dog uses `init_work_queue` as well as sheep! 
> so extract `wq_trace_init` from `init_work_queue` causes 
> dog’s new thread enter into  loop, because `tid_max` is not initialized.
> 
> all in all, the main problem is the code below will enter into loop when `tid_max` is 0.
> ---
> 		/* enlarge bitmap size */
>                 while (tid > tid_max)
>                         tid_max *= 2;
> ―
> and we need to init `tid_max` before journal init.
> and we need to make sure both sheep and dog will not enter into loop.
> 
> maybe the solution is:
> 1. revert the patch & assign tid_max to 1 in its declaration 
> or
> 2. call `wq_trace_init` in the dog’s main() function before `init_work_queue`
> or
> 3. make sure `tid_max` is not 0 in `trace_set_tid_map`
> 
> 
> what do you think

Ah, I should find and point the problem at that time. Thanks! And I
like the 2nd idea. Could you prepare a patch?

Thanks,
Hitoshi

> 
> 
> Thanks
> Jinzhi Chen
> 
> > On Nov 13, 2014, at 10:07 AM, Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp> wrote:
> > 
> > At Wed, 12 Nov 2014 14:21:22 +0800,
> > Jinzhi Chen wrote:
> >> 
> >> when journal thread init before the initialization of trace,
> >> it enter into loop (using uninitailized `tid_max`)with
> >> --debug-enable configured. we need to initialize trace before
> >> journal thread init.
> >> this patch extrace `wq_trace_init` from `init_work_queue` and
> >> call it in the main() function just before the initialization
> >> of journal file.
> >> 
> >> Signed-off-by: Jinzhi Chen <nxtjinzhi at gmail.com>
> >> ---
> >> include/work.h | 1 +
> >> lib/work.c     | 8 ++------
> >> sheep/sheep.c  | 7 +++++++
> >> 3 files changed, 10 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/include/work.h b/include/work.h
> >> index b2fa0cf..c2008ba 100644
> >> --- a/include/work.h
> >> +++ b/include/work.h
> >> @@ -63,6 +63,7 @@ struct work_queue *create_work_queue(const char *name, enum wq_thread_control);
> >> struct work_queue *create_ordered_work_queue(const char *name);
> >> void queue_work(struct work_queue *q, struct work *work);
> >> bool work_queue_empty(struct work_queue *q);
> >> +int wq_trace_init(void);
> >> 
> >> #ifdef HAVE_TRACE
> >> void suspend_worker_threads(void);
> >> diff --git a/lib/work.c b/lib/work.c
> >> index 7e2cd79..95953e4 100644
> >> --- a/lib/work.c
> >> +++ b/lib/work.c
> >> @@ -134,7 +134,7 @@ static void suspend(int num)
> >> 	eventfd_xwrite(ack_efd, 1); /* ack of resume */
> >> }
> >> 
> >> -static int wq_trace_init(void)
> >> +int wq_trace_init(void)
> >> {
> >> 	tid_max = TID_MAX_DEFAULT;
> >> 	tid_map = alloc_bitmap(NULL, 0, tid_max);
> >> @@ -180,7 +180,7 @@ static void trace_clear_tid_map(int tid)
> >> 
> >> #else
> >> 
> >> -static inline int wq_trace_init(void) { return 0; }
> >> +inline int wq_trace_init(void) { return 0; }
> > 
> > Applied, thanks. But I removed the above inline because it is not
> > required.
> > 
> > Thanks,
> > Hitoshi
> > 
> > 
> >> static inline void trace_set_tid_map(int tid) {}
> >> static inline void trace_clear_tid_map(int tid) {}
> >> 
> >> @@ -364,10 +364,6 @@ int init_work_queue(size_t (*get_nr_nodes)(void))
> >> 		return -1;
> >> 	}
> >> 
> >> -	ret = wq_trace_init();
> >> -	if (ret < 0)
> >> -		return ret;
> >> -
> >> 	ret = register_event(efd, worker_thread_request_done, NULL);
> >> 	if (ret) {
> >> 		sd_err("failed to register event fd %m");
> >> diff --git a/sheep/sheep.c b/sheep/sheep.c
> >> index 2e91d0f..ef45a33 100644
> >> --- a/sheep/sheep.c
> >> +++ b/sheep/sheep.c
> >> @@ -885,6 +885,13 @@ int main(int argc, char **argv)
> >> 		goto cleanup_log;
> >> 	}
> >> 
> >> +	/* We should init trace for work queue before journal init */
> >> +	ret = wq_trace_init();
> >> +	if (ret) {
> >> +		sd_err("failed to init trace for work queue");
> >> +		goto cleanup_log;
> >> +	}
> >> +
> >> 	/* We should init journal file before backend init */
> >> 	if (uatomic_is_true(&sys->use_journal)) {
> >> 		if (!strlen(jpath))
> >> -- 
> >> 1.9.3 (Apple Git-50)
> >> 
> >> -- 
> >> sheepdog mailing list
> >> sheepdog at lists.wpkg.org <mailto:sheepdog at lists.wpkg.org>
> >> http://lists.wpkg.org/mailman/listinfo/sheepdog <http://lists.wpkg.org/mailman/listinfo/sheepdog>
> [1.2  <text/html; utf-8 (quoted-printable)>]
> 
> [2  <text/plain; us-ascii (7bit)>]
> -- 
> sheepdog mailing list
> sheepdog at lists.wpkg.org
> http://lists.wpkg.org/mailman/listinfo/sheepdog