On Fri, Nov 08, 2013 at 08:53:00AM +0800, Qu Wenruo wrote:
> On Thu, 7 Nov 2013 17:41:34 +0100, David Sterba wrote:
> >On Thu, Nov 07, 2013 at 01:51:53PM +0800, Qu Wenruo wrote:
> >>@@ -753,6 +754,19 @@ struct btrfs_workqueue_struct *btrfs_alloc_workqueue(char *name,
> >> }
> >> }
> >>+ if (high_name) {
> >>+ ret->high_wq = alloc_workqueue(high_name,
> >>+ flags | WQ_HIGHPRI,
> >>+ max_active);
> >I'd really like to add the btrfs- prefix of the workqueue. Quoting our
> >previous discussion:
> Sorry, I forgot to add the "btrfs-" prefix.
> But on the other hand, I prefer to add the prefix outside the
> alloc_btrfs_workqueue,
> which means change the strings passed to alloc_btrfs_workqueue.
> (Maybe add a small macro?)
>
> Which way do you prefer?
It's actually quite simple, alloc_workque takes a printk format string,
so:
alloc_workqueue("%s-%s", flags, max_active, "btrfs", name);
but I consider the following to be harder to get around.
> >>>* the thread names lost the btrfs- prefix, this makes it hard to
> >>> identify the processes and we want this, either debugging or
> >>> performance monitoring
> >>Yes, that's right.
> >>But the problem is, even I added "btrfs-" prefix to the wq,
> >>the real work executor is kernel workers without any prefix.
> >>Still hard to debugging due to the workqueue mechanism.
> >AFAICS the string passed down to alloc_workqueue ends up in the process
> >name, this is what I'm expecing and don't understand what do you mean.
> >
> Sorry for the misunderstanding words.
> What I mean is that, since we use the kernel workqueue,
> there will be no kthread named the "btrfs-worker" or something like it.
> (Only when WQ_MEM_RECLAIM is set, the rescuer kthread will have the name,
> See kernel/workqueue.c if(flags & WQ_MEM_RECLAIM) para).
You're right, I was not aware of that, and actually checked the
workqueus that have the MEM_RECLAIM bit set. Most of the btrfs IO
workers will have to have this bit set, there are some of them that do
not need it (scrub or readahead).
> The real executor will be something like kworker/u8:6(Unbound workqueue),
> so even the prefix is added, it's still harder than the original way to
> debug workqueue.
Right, the question is if we really need it that way. We can set the
MEM_RECLAIM bit but it looks like an abuse of the interface.
> What makes it worse is that, to simulate the thresholding and ordering,
> I added serveral helper functions, which even makes kernel tracing not so
> meaningful(all function queued to workqueue will all be normal_helper or
> ordered_helper).
That's a valid concern. Can we extend the tracepoints to keep the
btrfs-worker name?
So, as a first step, we can add MEM_RECLAIM to all threads, this should
do the conversion close to the current state. Next we can start tuning the
workqueue flags. This is an internal change, should be safe to do later.
david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
