<div dir="ltr"><div>[PATCH] Initialize trace before journal init<br></div><div><br></div><div>when journal thread init before the initialization of trace,</div><div>it enter into loop (using uninitailized `tid_max`)with</div><div>--debug-enable configured. we need to initialize trace before</div><div> journal thread init.</div><div>this patch extrace `wq_trace_init` from `init_work_queue` and</div><div>call it in the main() function just before the initialization</div><div>of journal file.</div><div>---</div><div> lib/work.c | 48 ++++++++++++++++++++++--------------------------</div><div> sheep/sheep.c | 9 +++++++++</div><div> 2 files changed, 31 insertions(+), 26 deletions(-)</div><div><br></div><div>diff --git a/lib/work.c b/lib/work.c</div><div>index 7e2cd79..e36516a 100644</div><div>--- a/lib/work.c</div><div>+++ b/lib/work.c</div><div>@@ -70,6 +70,7 @@ static size_t nr_nodes = 1;</div><div> static size_t (*wq_get_nr_nodes)(void);</div><div> </div><div> static void *worker_routine(void *arg);</div><div>+int wq_trace_init (void);</div><div> </div><div> #ifdef HAVE_TRACE</div><div> </div><div>@@ -134,27 +135,6 @@ static void suspend(int num)</div><div> <span class="" style="white-space:pre"> </span>eventfd_xwrite(ack_efd, 1); /* ack of resume */</div><div> }</div><div> </div><div>-static int wq_trace_init(void)</div><div>-{</div><div>-<span class="" style="white-space:pre"> </span>tid_max = TID_MAX_DEFAULT;</div><div>-<span class="" style="white-space:pre"> </span>tid_map = alloc_bitmap(NULL, 0, tid_max);</div><div>-</div><div>-<span class="" style="white-space:pre"> </span>resume_efd = eventfd(0, EFD_SEMAPHORE);</div><div>-<span class="" style="white-space:pre"> </span>ack_efd = eventfd(0, EFD_SEMAPHORE);</div><div>-</div><div>-<span class="" style="white-space:pre"> </span>if (resume_efd < 0 || ack_efd < 0) {</div><div>-<span class="" style="white-space:pre"> </span>sd_err("failed to create event fds: %m");</div><div>-<span class="" style="white-space:pre"> </span>return -1;</div><div>-<span class="" style="white-space:pre"> </span>}</div><div>-</div><div>-<span class="" style="white-space:pre"> </span>/* trace uses this signal to suspend the worker threads */</div><div>-<span class="" style="white-space:pre"> </span>if (install_sighandler(SIGUSR2, suspend, false) < 0) {</div><div>-<span class="" style="white-space:pre"> </span>sd_debug("%m");</div><div>-<span class="" style="white-space:pre"> </span>return -1;</div><div>-<span class="" style="white-space:pre"> </span>}</div><div>-<span class="" style="white-space:pre"> </span>return 0;</div><div>-}</div><div>-</div><div> static void trace_set_tid_map(int tid)</div><div> {</div><div> <span class="" style="white-space:pre"> </span>sd_mutex_lock(&tid_map_lock);</div><div>@@ -180,12 +160,32 @@ static void trace_clear_tid_map(int tid)</div><div> </div><div> #else</div><div> </div><div>-static inline int wq_trace_init(void) { return 0; }</div><div> static inline void trace_set_tid_map(int tid) {}</div><div> static inline void trace_clear_tid_map(int tid) {}</div><div> </div><div> #endif<span class="" style="white-space:pre"> </span>/* HAVE_TRACE */</div><div> </div><div>+int wq_trace_init(void)</div><div>+{</div><div>+<span class="" style="white-space:pre"> </span>tid_max = TID_MAX_DEFAULT;</div><div>+<span class="" style="white-space:pre"> </span>tid_map = alloc_bitmap(NULL, 0, tid_max);</div><div>+</div><div>+<span class="" style="white-space:pre"> </span>resume_efd = eventfd(0, EFD_SEMAPHORE);</div><div>+<span class="" style="white-space:pre"> </span>ack_efd = eventfd(0, EFD_SEMAPHORE);</div><div>+</div><div>+<span class="" style="white-space:pre"> </span>if (resume_efd < 0 || ack_efd < 0) {</div><div>+<span class="" style="white-space:pre"> </span>sd_err("failed to create event fds: %m");</div><div>+<span class="" style="white-space:pre"> </span>return -1;</div><div>+<span class="" style="white-space:pre"> </span>}</div><div>+</div><div>+<span class="" style="white-space:pre"> </span>/* trace uses this signal to suspend the worker threads */</div><div>+<span class="" style="white-space:pre"> </span>if (install_sighandler(SIGUSR2, suspend, false) < 0) {</div><div>+<span class="" style="white-space:pre"> </span>sd_debug("%m");</div><div>+<span class="" style="white-space:pre"> </span>return -1;</div><div>+<span class="" style="white-space:pre"> </span>}</div><div>+<span class="" style="white-space:pre"> </span>return 0;</div><div>+}</div><div>+</div><div> static uint64_t get_msec_time(void)</div><div> {</div><div> <span class="" style="white-space:pre"> </span>struct timeval tv;</div><div>@@ -364,10 +364,6 @@ int init_work_queue(size_t (*get_nr_nodes)(void))</div><div> <span class="" style="white-space:pre"> </span>return -1;</div><div> <span class="" style="white-space:pre"> </span>}</div><div> </div><div>-<span class="" style="white-space:pre"> </span>ret = wq_trace_init();</div><div>-<span class="" style="white-space:pre"> </span>if (ret < 0)</div><div>-<span class="" style="white-space:pre"> </span>return ret;</div><div>-</div><div> <span class="" style="white-space:pre"> </span>ret = register_event(efd, worker_thread_request_done, NULL);</div><div> <span class="" style="white-space:pre"> </span>if (ret) {</div><div> <span class="" style="white-space:pre"> </span>sd_err("failed to register event fd %m");</div><div>diff --git a/sheep/sheep.c b/sheep/sheep.c</div><div>index 2e91d0f..071e467 100644</div><div>--- a/sheep/sheep.c</div><div>+++ b/sheep/sheep.c</div><div>@@ -148,6 +148,8 @@ static struct sd_option sheep_options[] = {</div><div> <span class="" style="white-space:pre"> </span>{ 0, NULL, false, NULL },</div><div> };</div><div> </div><div>+extern int wq_trace_init (void);</div><div>+</div><div> static void usage(int status)</div><div> {</div><div> <span class="" style="white-space:pre"> </span>if (status) {</div><div>@@ -885,6 +887,13 @@ int main(int argc, char **argv)</div><div> <span class="" style="white-space:pre"> </span>goto cleanup_log;</div><div> <span class="" style="white-space:pre"> </span>}</div><div> </div><div>+<span class="" style="white-space:pre"> </span>/* We should init trace before journal init */</div><div>+<span class="" style="white-space:pre"> </span>ret = wq_trace_init ();</div><div>+<span class="" style="white-space:pre"> </span>if (ret) {</div><div>+<span class="" style="white-space:pre"> </span>sd_err("failed to init trace");</div><div>+<span class="" style="white-space:pre"> </span>goto cleanup_log;</div><div>+<span class="" style="white-space:pre"> </span>}</div><div>+</div><div> <span class="" style="white-space:pre"> </span>/* We should init journal file before backend init */</div><div> <span class="" style="white-space:pre"> </span>if (uatomic_is_true(&sys->use_journal)) {</div><div> <span class="" style="white-space:pre"> </span>if (!strlen(jpath))</div><div>-- </div><div>1.7.9.5</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 30, 2014 at 4:14 PM, Hitoshi Mitake <span dir="ltr"><<a href="mailto:mitake.hitoshi@gmail.com" target="_blank">mitake.hitoshi@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Oct 30, 2014 at 2:46 PM, Jinzhi Chen <<a href="mailto:nxtjinzhi@gmail.com">nxtjinzhi@gmail.com</a>> wrote:<br>
> thank you for your reply.<br>
> it sounds good to me.<br>
><br>
> how about extract `wq_trace_init` from `init_work_queue` (lib/work.c) and<br>
> call it in the main() function just before the initialization of journal<br>
> file.<br>
> we can initialize work queue trace related variable before the<br>
> initialization of<br>
> work queue. In this manner, we don't need a new function.<br>
<br>
</span>Seems good. Could you prepare v2 with the above scheme?<br>
<br>
Thanks,<br>
Hitoshi<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> Thanks<br>
> Jinzhi Chen<br>
><br>
> On Thu, Oct 30, 2014 at 9:42 AM, Hitoshi Mitake<br>
> <<a href="mailto:mitake.hitoshi@lab.ntt.co.jp">mitake.hitoshi@lab.ntt.co.jp</a>> wrote:<br>
>><br>
>><br>
>> Hi Jinzhi, thanks for your patch.<br>
>><br>
>> At Wed, 29 Oct 2014 13:53:14 +0800,<br>
>> Jinzhi Chen wrote:<br>
>> ><br>
>> > [1 <multipart/alternative (7bit)>]<br>
>> > [1.1 <text/plain; UTF-8 (7bit)>]<br>
>> > journal thread uses `tid_max` which is not initialized until<br>
>> > later initial progress. The default value 0 cause journal<br>
>> > thread loop in the function `trace_set_tid_map` , so set<br>
>> > default value to 1.<br>
>> ><br>
>> > Signed-off-by: jinzhichen <<a href="mailto:nxtjinzhi@gmail.com">nxtjinzhi@gmail.com</a>><br>
>><br>
>> Could you write your name "Jinzhi Chen" instead of jinzhichen in<br>
>> Signed-off-by: line?<br>
>><br>
>> > ---<br>
>> > lib/work.c | 2 +-<br>
>> > 1 file changed, 1 insertion(+), 1 deletion(-)<br>
>> ><br>
>> > diff --git a/lib/work.c b/lib/work.c<br>
>> > index 7e2cd79..7b20685 100644<br>
>> > --- a/lib/work.c<br>
>> > +++ b/lib/work.c<br>
>> > @@ -75,7 +75,7 @@ static void *worker_routine(void *arg);<br>
>> ><br>
>> > #define TID_MAX_DEFAULT 0x8000 /* default maximum tid for most systems<br>
>> > */<br>
>> ><br>
>> > -static size_t tid_max;<br>
>> > +static size_t tid_max = 1;<br>
>><br>
>> How about the below way? Extracting the code<br>
>><br>
>> tid_max = TID_MAX_DEFAULT;<br>
>> tid_map = alloc_bitmap(NULL, 0, tid_max);<br>
>><br>
>> from wq_trace_init(), prepare a new function,<br>
>> e.g. early_trace_init(), which does the above initialization, and let<br>
>> main() call the function in its early part. Practically your patch can<br>
>> solve the problem but the above way will not produce a combination of<br>
>> initialized tid_max and uninitialized tid_map. How do you think?<br>
>><br>
>> Thanks,<br>
>> Hitoshi<br>
>><br>
>> > static unsigned long *tid_map;<br>
>> > static struct sd_mutex tid_map_lock = SD_MUTEX_INITIALIZER;<br>
>> ><br>
>> > --<br>
>> > 1.7.9.5<br>
>> > [1.2 <text/html; UTF-8 (quoted-printable)>]<br>
>> ><br>
>> > [2 <text/plain; us-ascii (7bit)>]<br>
>> > --<br>
>> > sheepdog mailing list<br>
>> > <a href="mailto:sheepdog@lists.wpkg.org">sheepdog@lists.wpkg.org</a><br>
>> > <a href="http://lists.wpkg.org/mailman/listinfo/sheepdog" target="_blank">http://lists.wpkg.org/mailman/listinfo/sheepdog</a><br>
><br>
><br>
><br>
> --<br>
> sheepdog mailing list<br>
> <a href="mailto:sheepdog@lists.wpkg.org">sheepdog@lists.wpkg.org</a><br>
> <a href="http://lists.wpkg.org/mailman/listinfo/sheepdog" target="_blank">http://lists.wpkg.org/mailman/listinfo/sheepdog</a><br>
><br>
</div></div></blockquote></div><br></div>