Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs

[Oleg Nesterov <oleg@redhat.com>]

On 08/12, Bart Van Assche wrote:
>
> On 08/12/2016 09:16 AM, Oleg Nesterov wrote:
> > Please drop two patches I sent before and try the new one below.
>
> Hello Oleg,
>
> Thanks for the patch. In addition to your patch I also applied the
> attached two patches

And I guess you did this because you think we do not have enough
confusion so you decided to add a bit more ;)

Could you please test my patch alone without additional changes?

> before I started testing. It took some time
> before I could reproduce the hang in truncate_inode_pages_range().

all I can say this contradicts with the prvious testing results with
my previous patch or with your change in abort_exclusive_wait().

> +int __lock_page_impl(struct page *page, int mode)
> +{
> +	struct page *page_head = compound_head(page);
> +	DEFINE_WAIT_BIT(wait, &page_head->flags, PG_locked);
> +	struct task_struct *owner;
> +	int res;
> +
> +	for (;;) {
> +		wait.key.timeout = jiffies + 30 * HZ;
> +		res = __wait_on_bit_lock(page_waitqueue(page_head),
> +					 &wait, bit_wait_io_timeout, mode);
> +		if (res == 0) {
> +			set_page_lock_owner(page, current);

this is not right, you should use page_head. Although I doubt this can
make a difference in this case. The same for get_page_lock_owner() below.

> +			break;
> +		}
> +		if (res == -EINTR)
> +			break;
> +		owner = get_page_lock_owner(page);
> +		pr_info("%s / pid %d / m %#x: %s - continuing to wait for %d\n",
> +			__func__, task_pid_nr(current), mode, res == -EAGAIN ?
> +			"timeout" : "interrupted",
> +			owner ? task_pid_nr(owner) : 0);

I thought about the similar debugging patch too. But this is not what
we need. Note that if res == -EAGAIN then another exlcusive waiter was
already woken and it can lock this page and set get_page_lock_owner().
So this can't actually help if the problem is the missed/lost wakeup.

Not that it explains the strange dmesg you reported. Perhaps your patch
has other bugs, or my patch is buggy, or both. Please do not mix them.

As for "add the timeout" idea it makes sense too and perhaps we will test
this later, but we can start with the much more simple patch.

Oleg.