Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460710 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-11 19:40 +0200
    Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-12 18:20 +0200
      Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-13 18:40 +0200
        Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-16 15:10 +0200
          Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-17 19:40 +0200
      Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-13 19:10 +0200

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

Hi Bart,

On 08/10, Bart Van Assche wrote:
>
> That's an excellent catch. With your previous patch and this patch applied I
> can't reproduce the hang in truncate_inode_pages_range() anymore.

Great, thanks.

I'll send another debugging patch tomorrow, I was a bit busy today. The next
step is obvious, we need to know the caller.

But just in case, this doesn't necessarily mean that the usage of
__ClearPageLocked() is actually buggy, we don't really know this so far...

And I can't understand another oddity. Your test-case hangs in kill_bdev()
path which sleeps with bdev->bd_openers == 0 under bdev->bd_mutex so it can't
be re-opened. However, since your change in abort_exclusive_wait() helped,
there should be the readers sleeping in lock_killable() and thus bd_openers
can't be zero.

Nevermind, I don't understand this code even remotely, we will see later
who should be asked.

> I still
> see some other wait_on_page_bit() hangs after an I/O error has occurred.
> However, the hangs that I still see are related to waiting on buffer head
> state changes and not on the PG_locked page flag.

I don't know if this is right or not... lets discuss this later.

Thanks!

Oleg.

[toc] | [next] | [standalone]

#1461299

On 08/11, Oleg Nesterov wrote:
>
> I'll send another debugging patch tomorrow, I was a bit busy today. The next
> step is obvious, we need to know the caller.

Please drop two patches I sent before anf try the new one below.

Which kernel version do you use?

Oleg.
---

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index e5a3244..533da3ab 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -711,6 +711,15 @@ static inline int page_has_private(struct page *page)
 	return !!(page->flags & PAGE_FLAGS_PRIVATE);
 }
 
+void unlock_page_x(struct page *page);
+static inline void __ClearPageLocked_x(struct page *page)
+{
+	if (PageLocked(compound_head(page)))
+		unlock_page_x(page);
+}
+
+#define __ClearPageLocked(page)	__ClearPageLocked_x(page)
+
 #undef PF_ANY
 #undef PF_HEAD
 #undef PF_NO_TAIL
diff --git a/mm/filemap.c b/mm/filemap.c
index 20f3b1f..fb320fb 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -837,6 +837,43 @@ void unlock_page(struct page *page)
 }
 EXPORT_SYMBOL(unlock_page);
 
+void unlock_page_x(struct page *__page)
+{
+	struct page *page = compound_head(__page);
+	wait_queue_head_t *wq = page_waitqueue(page);
+	struct wait_bit_key key = __WAIT_BIT_KEY_INITIALIZER(&page->flags, PG_locked);
+	wait_queue_t *curr, *next;
+	unsigned long flags;
+	bool w = false;
+
+	#define W() do {								\
+		if (!w) { w = true; pr_crit("XXXXXXXXXXXX\n"); dump_stack(); }	\
+	} while (0)
+
+	clear_bit_unlock(PG_locked, &page->flags);
+	smp_mb__after_atomic();
+
+	if (!waitqueue_active(wq))
+		return;
+
+	spin_lock_irqsave(&wq->lock, flags);
+	list_for_each_entry_safe(curr, next, &wq->task_list, task_list) {
+		if (curr->func == wake_bit_function) {
+			struct wait_bit_queue *wb = container_of(curr, struct wait_bit_queue, wait);
+			if (wb->key.flags == key.flags && wb->key.bit_nr == PG_locked) {
+				W();
+				pr_crit("XXX flags = %x, waiter:\n", curr->flags);
+				sched_show_task(curr->private);
+			}
+		} else {
+			W();
+			pr_crit("XXX flags = %x, func = %pF\n", curr->flags, curr->func);
+		}
+		curr->func(curr, TASK_NORMAL, 0, &key);
+	}
+	spin_unlock_irqrestore(&wq->lock, flags);
+}
+
 /**
  * end_page_writeback - end writeback against a page
  * @page: the page

[toc] | [prev] | [next] | [standalone]

#1461643

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.

[toc] | [prev] | [next] | [standalone]

#1463769

On 08/15, Bart Van Assche wrote:
>
> On 08/13/2016 09:32 AM, Oleg Nesterov wrote:
>> On 08/12, Bart Van Assche wrote:
>>> 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 previous testing results with
>> my previous patch or with your change in abort_exclusive_wait().
>
> Hello Oleg,
>
> My opinion is that all this means is that we do not yet have a full
> understanding of what is going on.

Sure.

> BTW, I have improved my page lock owner instrumentation patch such that
> it prints a call stack of the lock owner if lock_page() takes too long.
> The following call stack was reported:
>
> __lock_page / pid 8549 / m 0x2: timeout - continuing to wait for 8549
>   [<ffffffff8102b316>] save_stack_trace+0x26/0x50
>   [<ffffffff81152bee>] add_to_page_cache_lru+0x7e/0x170
>   [<ffffffff8121bfc5>] mpage_readpages+0xc5/0x170
>   [<ffffffff81215548>] blkdev_readpages+0x18/0x20
>   [<ffffffff81163a68>] __do_page_cache_readahead+0x268/0x310
>   [<ffffffff811640a8>] force_page_cache_readahead+0xa8/0x100
>   [<ffffffff81164139>] page_cache_sync_readahead+0x39/0x40
>   [<ffffffff81153967>] generic_file_read_iter+0x707/0x920
>   [<ffffffff81215920>] blkdev_read_iter+0x30/0x40
>   [<ffffffff811d4b4b>] __vfs_read+0xbb/0x130
>   [<ffffffff811d4f31>] vfs_read+0x91/0x130
>   [<ffffffff811d62b4>] SyS_read+0x44/0xa0
>   [<ffffffff816281e5>] entry_SYSCALL_64_fastpath+0x18/0xa8
>
> My understanding of mpage_readpages() is that the page unlock happens
> after readahead I/O completed (see also page_endio()). So this probably
> means that an I/O request submitted because of readahead code did not
> get completed. I will see whether I can find anything that's wrong in
> the block layer.

Perhaps. But this means another problem! Or you didn't wait enough. Or
your previous testing was wrong.

Because, once again, your changes in abort_exclusive_wait(), and my
debugging patch which adds wakeup into ClearPageLocked() suggest that
the problem is NOT that the page is still locked.


I'd still like to know what happens with the last patch I sent (without
any other changes)... but now I am totally confused.

If only I could reproduce. Or at least understand what are you doing to
hit thi bug ;)

Oleg.

[toc] | [prev] | [next] | [standalone]

#1464688

On 08/16, Bart Van Assche wrote:
>
> On 08/16/2016 06:06 AM, Oleg Nesterov wrote:
>> If only I could reproduce. Or at least understand what are you doing to
>> hit this bug ;)
>
> Hello Oleg,
>
> What I'm doing to hit this bug is to run the test script that is
> available at https://github.com/bvanassche/srp-test on a setup that is
> equipped with at least one InfiniBand adapter. I see the following
> possibilities for you to reproduce this:
> * Ask a colleague for access to an IB setup.
> * Add RoCE support to the srp-test script and run that script against a
>   v4.8 kernel + ib_srp-backport + SCST ib_srpt drivers. These last two
>   (out-of-tree) drivers namely support SRP over RoCE. The upstream
>   drivers not yet. The SRP-over-RoCE functionality will be sent
>   upstream as soon as standardization of this protocol by the T10
>   committee has finished (this work has already been started and will
>   probably be finished later this year).
>
> Please let me know if you need more information.

Heh ;) I can't understand any single word above.

So I'll give up. Previously you reported that this patch

	http://marc.info/?l=linux-kernel&m=147085570503588

the problem goes away. In this case the next one

	http://marc.info/?l=linux-kernel&m=147101858416463

could give us more info but you didn't try it so far (without other
changes).

It seems you find the root of this problem somewhere else, hopefully
you will resolve it soon.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1461657

Forgot to mention...

On 08/12, Bart Van Assche wrote:
>
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1643,7 +1643,12 @@ find_page:
>  			 * wait_on_page_locked is used to avoid unnecessarily
>  			 * serialisations and why it's safe.
>  			 */
> -			wait_on_page_locked_killable(page);
> +			error = wait_on_page_locked_killable(page);
> +			if (error == -EINTR) {
> +				put_page(page);
> +				goto out;
> +			}
> +			error = 0;

This change probably makes sense regardless although I'd suggest to
simplify it:

	-		wait_on_page_locked_killable(page);
	+		error = wait_on_page_locked_killable(page);
	+		if (unlikely(error))
	+			goto readpage_error;


but it looks off-topic. And the changelog looks misleading/wrong.

I do not think this change makes sense in this debugging session,

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web