Re: [PATCH v4 00/18] Replace btrfs_workers with kernel workqueue based btrfs_workqueue

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

On fri, 20 Dec 2013 05:30:48 -0800, Josef Bacik wrote:
>
> On 12/19/2013 07:08 PM, Qu Wenruo wrote:
>> I'm sorry but I failed to reproduce the problem.
>> Btrfs/012 in xfstests has been run for serveral hours but nothing 
>> happened.
>>
>> Would you please give me some more details about the environment or 
>> the panic backtrace?
>>
>
> Ok so it wasn't that test, it was just ./check -g auto. It would 
> sometimes die at btrfs/012, other times it would make it as far as 
> generic/083 before it keeled over. I bisected it down to
>
> btrfs: Replace fs_info->workers with btrfs_workqueue
>
> which of course is just the first patch that you start using the new 
> code which isn't helpful. My dmesg from one of my runs is here
>
> http://ur1.ca/g867b
>
> All of the initial panics looked exactly the same. I'm just going to 
> kick the series out for now while you track down the problem. Thanks,
>
> Josef
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Thanks for your dmesg a lot.

It's quite sad that I still failed to reproduce the same bug on all my 
test environment during the weekend.

But according to the dmesg, which is expired now though, I made the 
following assumption:

There is a possibility that out of order execution made the "work->wq = 
wq" sentence executed behind the "queue_work()" call,
and the "normal_work_helper" is executed in another CPU instantly, which 
caused the problem.

------
static inline void __btrfs_queue_work(struct __btrfs_workqueue *wq,
				      struct btrfs_work *work)
{
	unsigned long flags;

	work->wq = wq;
/* There is no memory barrier ensure the work->wq = wq is executed
before queue_work */
	thresh_queue_hook(wq);
	if (work->ordered_func) {
		spin_lock_irqsave(&wq->list_lock, flags);
		list_add_tail(&work->ordered_list, &wq->ordered_list);
		spin_unlock_irqrestore(&wq->list_lock, flags);
	}
	queue_work(wq->normal_wq, &work->normal_work);
}
------

The memory order problem involves compiler or even CPU hardware (AMD 
CPUS seems to have a weaker memory order than Intel),
so to simulate the bug, I used the following codes tried to reproduce 
the bug, and the result is pretty much the same as your dmesg.
------
static inline void __btrfs_queue_work(struct __btrfs_workqueue *wq,
				      struct btrfs_work *work)
{
	unsigned long flags;
	int in_order = get_random_int() % 1000

	if (in_order)
		work->wq = wq;
	thresh_queue_hook(wq);
	if (work->ordered_func) {
		spin_lock_irqsave(&wq->list_lock, flags);
		list_add_tail(&work->ordered_list, &wq->ordered_list);
		spin_unlock_irqrestore(&wq->list_lock, flags);
	}
	queue_work(wq->normal_wq, &work->normal_work);

/* Try make the codes out of order since the Intel CPU
seems obey the memory order quite well */

	if (!in_order)
		work->wq = wq;
}
------

The fix is as easy as just adding a smp_wmb() behind the "work->wq = wq" 
sentence, but since I failed to reproduce the bug,
I can't confirm whether the fix is good or not.

I know it's impolite but would you please first try the smp_wmb() first 
to confirm the fix?

If the fix is good I'll send the formal V5 patchset.

Thanks

Qu