Re: [PATCH v2 0/9] btrfs: Replace the btrfs_workers with kernel workqueue

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

于 2013年09月13日 01:37, David Sterba 写道:
> On Thu, Sep 12, 2013 at 04:08:15PM +0800, Qu Wenruo wrote:
>> Use kernel workqueue and kernel workqueue based new btrfs_workqueue_struct to replace
>> the old btrfs_workers.
>> The main goal is to reduce the redundant codes(800 lines vs 200 lines) and
>> try to get benefits from the latest workqueue changes.
>>
>> About the performance, the test suite I used is bonnie++,
>> and there seems no significant regression.
> You're replacing a core infrastructure building block, more testing is
> absolutely required, but using the available infrastructure is a good
> move.
Definitely needs more test since the I lack enough different disks to 
test with.
>
> I found a few things that do not replace the current implementation
> one-to-one:
>
> * 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.
>
> * od high priority tasks were handled in threads with unchanged priority
>    and just prioritized within the queue
>    newly addded WQ_HIGHPRI elevates the nice level of the thread, ie.
>    it's not the same thing as before -- I need to look closer
Also true, since I didn't find a way to ensure the high priority work
to be executed before any normal priority work,
I choose this workaround.
(Seems the original btrfs_workers also have some mechanism to avoid
starving, so I think this way maybe OK)
>
> * the idle_thresh attribute is not reflected in the new code, I don't
>    know if the kernel workqueues have something equivalent
It seems that kernel will not create kthread without any control,
but still needs more investigation to make sure.
>
>
> Other random comments:
>
> * you can use the same files for the new helpers, instead of bwq.[ch]
The way I used is to avoid naming confliction and easy to clean.
If needed I'll also use the async-thread.[ch]
>
> * btrfs_workqueue_struct can drop the _struct suffix
The naming rule is mostly copied from kernel wq and just add "btrfs_" prefix
if no naming confliction.
Will modify if needed.
> * WQ_MEM_RECLAIM for the scrub thread does not seem right
>
> * WQ_FREEZABLE should be probably set
Will modify soon.

>
>
> david
>

Thanks for the comment.

Qu

-- 
-----------------------------------------------------
Qu Wenruo
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
No. 6 Wenzhu Road, Nanjing, 210012, China
TEL: +86+25-86630566-8526
COINS: 7998-8526
FAX: +86+25-83317685
MAIL: quwenruo@cn.fujitsu.com
-----------------------------------------------------