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

Groups > linux.kernel > #1455994 > 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 Peter Zijlstra <peterz@infradead.org> - 2016-08-03 21:50 +0200
    Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-03 23:40 +0200
      Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Peter Zijlstra <peterz@infradead.org> - 2016-08-04 16:20 +0200
        Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Peter Zijlstra <peterz@infradead.org> - 2016-08-08 12:30 +0200
          Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Bart Van Assche <bvanassche@acm.org> - 2016-08-08 16:40 +0200
            Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-08 18:30 +0200
              Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-09 19:20 +0200
                Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-10 21:00 +0200
                Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-10 21:10 +0200
                  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
              Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Peter Zijlstra <peterz@infradead.org> - 2016-08-10 21:20 +0200
              Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs Oleg Nesterov <oleg@redhat.com> - 2016-08-10 23:30 +0200

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

On Wed, Aug 03, 2016 at 09:35:03AM -0700, Bart Van Assche wrote:
> If try_to_wakeup() reads the task state before abort_exclusive_wait()
> sets the task state and if autoremove_wake_function() is called after
> abort_exclusive_wait() has removed a task from a wait list then the
> cascading mechanism for exclusive wakeups in abort_exclusive_wait()
> won't be triggered. Avoid this by serializing the task state change
> in abort_exclusive_wait() and try_to_wakeup().

I'm dense.. what!?

	CPU0			CPU1			CPU2

	

				__lock_page_killable()
				  __wait_on_bit_lock()
				    bit_wait_io()
				      schedule()
	__wake_up_bit()
	  __wake_up(.nr_exclusive=1)
	    spin_lock(&q->lock)
	    __wake_up_common()
	      autoremove_wake_func()
	        try_to_wake_up(p, TASK_NORMAL)
		list_del_init(&wait->task_list)
	    spin_unlock(&q->lock)

							complete_signal(p)
							  signal_wake_up(p, 1)
							    sigaddset(&p->pending.signal, SIGKILL)
							    try_to_wake_up(p, TASK_WAKEKILL)

				      if (signal_pending_state(TASK_KILLABLE))
				        return -EINTR;
				    abort_exclusive_wait()
				      __set_current_state(RUNNING)
				      spin_lock(q->lock)
				      if (!list_empty()) /* empty */
				      else if (waitqueue_active()) /* pending ? */
				        __wake_up_locked_key(q, mode, key)
				      spin_unlock(q->lock)


That seems to do the right thing, so clearly I misunderstand. Please
clarify.


> +++ b/kernel/sched/wait.c
> @@ -277,10 +277,17 @@ void abort_exclusive_wait(wait_queue_head_t *q, wait_queue_t *wait,
>  			unsigned int mode, void *key)
>  {
>  	unsigned long flags;
> +	long wake_up;
> +
> +	/* Serialize against try_to_wake_up() */
> +	raw_spin_lock_irqsave(&current->pi_lock, flags);
> +	wake_up = current->state & (TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE);
> +	if (wake_up)
> +		__set_current_state(TASK_RUNNING);
> +	raw_spin_unlock_irqrestore(&current->pi_lock, flags);
>  
> -	__set_current_state(TASK_RUNNING);
>  	spin_lock_irqsave(&q->lock, flags);
> -	if (!list_empty(&wait->task_list))
> +	if (wake_up)
>  		list_del_init(&wait->task_list);
>  	else if (waitqueue_active(q))
>  		__wake_up_locked_key(q, mode, key);

That just feels wrong,.. very wrong.

[toc] | [next] | [standalone]

#1456036

Hi Bart,

I too can't understand the problem. Perhaps you missed the fact that
abort_exclusive_wait() does everything under wait_queue_head_t->lock ?

On 08/03, Bart Van Assche wrote:
>
> try_to_wake_up() locks task_struct.pi_lock but abort_exclusive_wait() not.
> My assumption is that the following sequence of events leads to the lockup
> that I had mentioned in the description of my patch:
> * try_to_wake_up() is called for the task that will execute
>   abort_exclusive_wait().
> * After try_to_wake_up() has checked task_struct.state and before
>   autoremove_wake_function() has tried to remove the task from the wait
>   queue, abort_exclusive_wait() is executed for the same task.

But we do not care if we race with another try_to_wake_up(), or even with
another exclusive wake_up_nr(wq)/whatever unless wq is the same.

And if this wq is the same, then wake_up_nr() will do try_to_wake_up/autoremove
either before or after abort_exclusive_wait(), wake_up_nr() takes the same
wq->lock.

And this means that abort_exclusive_wait() can't be called "After try_to_wake_up()"
and "before autoremove_wake_function()".

Oleg.

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

#1456431

On Wed, Aug 03, 2016 at 02:51:23PM -0700, Bart Van Assche wrote:
> So I started testing the patch below that should fix the same hang but
> without triggering any wait list corruption.
> 
> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
> index f15d6b6..4e3f651 100644
> --- a/kernel/sched/wait.c
> +++ b/kernel/sched/wait.c
> @@ -282,7 +282,7 @@ void abort_exclusive_wait(wait_queue_head_t *q,
> wait_queue_t *wait,
>  	spin_lock_irqsave(&q->lock, flags);
>  	if (!list_empty(&wait->task_list))
>  		list_del_init(&wait->task_list);
> -	else if (waitqueue_active(q))
> +	if (waitqueue_active(q))
>  		__wake_up_locked_key(q, mode, key);
>  	spin_unlock_irqrestore(&q->lock, flags);
>  }

So the problem with this patch is that it will violate the nr_exclusive
semantics in that it can result in too many wakeups -- which is a much
less severe (typically harmless) issue.

We now always wake up the next waiter, even if there wasn't an actual
wakeup we raced against. And if we then also get a wakeup, we can end up
with 2 woken tasks (instead of the nr_exclusive=1).

Now, since wait loops must all deal with spurious wakeups, this ends up
as harmless overhead.

But I'd still like to understand where we loose the wakeup. What are you
doing to reproduce this issue?

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

#1457689

On Fri, Aug 05, 2016 at 10:41:33AM -0700, Bart Van Assche wrote:
> On 08/04/2016 07:09 AM, Peter Zijlstra wrote:

> >But I'd still like to understand where we loose the wakeup.
> 
> My assumption is that __wake_up_common() and signal delivery happen
> concurrently, that __wake_up_common() wakes up bit_wait_io() and that signal
> delivery happens after bit_wait_io() has been woken up but before it tests
> the signal pending state.

That would be the exact scenario I drew a picture of, no? I'm still
failing to see the hole there.

Please draw a picture like that and illustrate the hole.

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

#1457839

On 08/08/16 03:22, Peter Zijlstra wrote:
> That would be the exact scenario I drew a picture of, no? I'm still
> failing to see the hole there.
> 
> Please draw a picture like that and illustrate the hole.
 
Hi Peter,

This is the sequence of which I think that it leads to the missed wakeup:

Task 1                    Task 2                    Task 3                    Task 4

lock_page()
 ...
                          lock_page_killable()
                           __lock_page_killable()
                            __wait_on_bit_lock()
                             bit_wait_io()
                              io_schedule()
                               ...
                                                                              lock_page()
                                                                               __lock_page()
                                                                                __wait_on_bit_lock()
                                                                                 bit_wait_io()
                                                                                  io_schedule()
                                                                                   ...


                                                    (signal delivery to task 2)
                                                    try_to_wake_up(task2, ..., ...)
                                                    (try_to_wake_up() returns 1)

unlock_page()
 wake_up_page()
  __wake_up_bit()
   __wake_up(wq, TASK_NORMAL, 1, &key)
    __wake_up_common(wq, mode=TASK_NORMAL, nr_exclusive=1, 0, key)
     wake_bit_function()
      autoremove_wake_function()
       default_wake_function()
        try_to_wake_up() <- skips task 2 because task 3 already changed
                            the task state of task 2
       (autoremove_wake_function() does not do
        list_del_init(&wait->task_list))


                              bit_wait_io() returns -EINTR
                             abort_exclusive_wait() is called by __wait_on_bit_lock()


In the above sequence task 1 does not remove task 2 from the waitqueue
because task 3 had already woken up task 2. The result is that when task 2
calls abort_exclusive_wait() that task 2 is still on the waitqueue. With the
current implementation of abort_exclusive_wait() in the above scenario task
4 is not woken up although it should be woken up. Hence the patch that removes
the "else" keyword from abort_exclusive_wait().

Bart.

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

#1457896

On 08/08, Bart Van Assche wrote:
>
> This is the sequence of which I think that it leads to the missed wakeup:
>
> Task 1                    Task 2                    Task 3                    Task 4
>
> lock_page()
>  ...
>                           lock_page_killable()
>                            __lock_page_killable()
>                             __wait_on_bit_lock()
>                              bit_wait_io()
>                               io_schedule()
>                                ...
>                                                                               lock_page()
>                                                                                __lock_page()
>                                                                                 __wait_on_bit_lock()
>                                                                                  bit_wait_io()
>                                                                                   io_schedule()
>                                                                                    ...
>
>
>                                                     (signal delivery to task 2)
>                                                     try_to_wake_up(task2, ..., ...)
>                                                     (try_to_wake_up() returns 1)
>
> unlock_page()
>  wake_up_page()
>   __wake_up_bit()
>    __wake_up(wq, TASK_NORMAL, 1, &key)
>     __wake_up_common(wq, mode=TASK_NORMAL, nr_exclusive=1, 0, key)
>      wake_bit_function()
>       autoremove_wake_function()
>        default_wake_function()
>         try_to_wake_up() <- skips task 2 because task 3 already changed
>                             the task state of task 2
>        (autoremove_wake_function() does not do
>         list_del_init(&wait->task_list))

Yes.

But since it skips task2, __wake_up_common() doesn't decrement nr_exclusive,
doesn't stop. It continues the list_for_each_entry_safe() loop, and finds the
sleeping task4, and wakes it up,

>                               bit_wait_io() returns -EINTR
>                              abort_exclusive_wait() is called by __wait_on_bit_lock()
>
>
> In the above sequence task 1 does not remove task 2 from the waitqueue
> because task 3 had already woken up task 2. The result is that when task 2
> calls abort_exclusive_wait() that task 2 is still on the waitqueue.

Yes, but this is fine,

> With the
> current implementation of abort_exclusive_wait() in the above scenario task
> 4 is not woken up although it should be woken up.

See above, it must be already woken by __wake_up_common().



So far _I think_ that the bug is somewhere else... Say, someone clears
PG_locked without wake_up(). Then SIGKILL sent to the task sleeping in
sys_read() "adds" the necessary wakeup...

Do you use external modules during the testing?

Oleg.

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

#1459008

On 08/08, Bart Van Assche wrote:
>
> No external modules were loaded when I triggered the lockup

Heh. Could you test the patch below?

Oleg.

--- x/kernel/sched/wait.c
+++ x/kernel/sched/wait.c
@@ -283,7 +283,7 @@ void abort_exclusive_wait(wait_queue_hea
 	if (!list_empty(&wait->task_list))
 		list_del_init(&wait->task_list);
 	else if (waitqueue_active(q))
-		__wake_up_locked_key(q, mode, key);
+		__wake_up_locked_key(q, TASK_NORMAL, key);
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 EXPORT_SYMBOL(abort_exclusive_wait);

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

#1459536

On 08/10, Bart Van Assche wrote:
>
> On 08/10/2016 03:46 AM, Oleg Nesterov wrote:
> > OK. Could you  try another debugging patch below?
> >
> > Oleg.
> > ---
> >
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index e5a3244..9d5f892 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(struct page *page);
> > +static inline void __ClearPageLocked_x(struct page *page)
> > +{
> > +	if (PageLocked(compound_head(page)))
> > +		unlock_page(page);
> > +}
> > +
> > +#define __ClearPageLocked(page)	__ClearPageLocked_x(page)
> > +
> >  #undef PF_ANY
> >  #undef PF_HEAD
> >  #undef PF_NO_TAIL
>
> Hi Oleg,
>
> Are you sure that all __ClearPageLocked() users pass the compound head
> to that macro?

Hmm. it obviously should... which kernel version do you use for testing?

From include/linux/page-flags.h

	__PAGEFLAG(Locked, locked, PF_NO_TAIL)

and

	#define PF_NO_TAIL(page, enforce) ({                                    \
		VM_BUG_ON_PGFLAGS(enforce && PageTail(page), page);     \
	compound_head(page);})
	
and this matches compound_head() in lock/unlock_page().

> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -711,6 +711,17 @@ static inline int page_has_private(struct page *page)
>  	return !!(page->flags & PAGE_FLAGS_PRIVATE);
>  }
>
> +void unlock_page(struct page *page);
> +static inline void __ClearPageLocked_x(struct page *page)
> +{
> +	if (PageLocked(compound_head(page)))
> +		unlock_page(page);
> +	else
> +		__ClearPageLocked(page);
> +}

No, no. If you use an old kernel (which doesn't call compound_head() in
lock_page()), then just remove compound_head() from __ClearPageLocked_x()
above:

	static inline void __ClearPageLocked_x(struct page *page)
	{
		if (PageLocked(page))
			unlock_page(page);
	}

even if this shouldn't make any difference afaics, note the
VM_BUG_ON_PGFLAGS() above.

Oleg.

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

#1459606

On 08/09, Bart Van Assche wrote:
>
> On 08/09/2016 10:15 AM, Oleg Nesterov wrote:
> >
> > --- x/kernel/sched/wait.c
> > +++ x/kernel/sched/wait.c
> > @@ -283,7 +283,7 @@ void abort_exclusive_wait(wait_queue_hea
> >  	if (!list_empty(&wait->task_list))
> >  		list_del_init(&wait->task_list);
> >  	else if (waitqueue_active(q))
> > -		__wake_up_locked_key(q, mode, key);
> > +		__wake_up_locked_key(q, TASK_NORMAL, key);
> >  	spin_unlock_irqrestore(&q->lock, flags);
> >  }
> >  EXPORT_SYMBOL(abort_exclusive_wait);
>
> Hello Oleg,
>
> That patch looks interesting to me.

And I'll redo/resend it, __wake_up_locked_key(mode) is simply wrong I think.

But it can't affect lock_page() because TASK_KILLABLE includes TASK_UNINTERRUPTIBLE
and we do not have lock_page_interruptible().


> Unfortunately even with that patch
> applied I still see lockups.

Thanks. I hoped this change can fix some another exclusive wait...

OK. Could you  try another debugging patch below?

Oleg.
---

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index e5a3244..9d5f892 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(struct page *page);
+static inline void __ClearPageLocked_x(struct page *page)
+{
+	if (PageLocked(compound_head(page)))
+		unlock_page(page);
+}
+
+#define __ClearPageLocked(page)	__ClearPageLocked_x(page)
+
 #undef PF_ANY
 #undef PF_HEAD
 #undef PF_NO_TAIL

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

#1460710

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] | [prev] | [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] | [next] | [standalone]

#1459624

On Wed, Aug 10, 2016 at 12:57:25PM +0200, Oleg Nesterov wrote:
> This condition is fine, and the trace is clear. This means that lock_page_killable()
> was interrupted and wake_bit_function() was not called. We do not need another wakeup
> in this case but somehow it helps. Again, I think because the necessary wakeup was
> already lost/missed.

I suspect the same. Removing that else generates 'spurious' wakeups,
which can unstick the situation, hiding the real source of the problem.

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

#1459997

On 08/09, Bart Van Assche wrote:
>
> Hello Oleg,
>
> Something that puzzles me is that removing the "else" keyword from
> abort_exclusive_wait() is sufficient to avoid the hang.

Yes, we need to understand this.

> If there would
> be code that clears PG_locked without calling wake_up() this hang
> probably would also be triggered by workloads that do not wake up
> lock_page_killable() with a signal.

Yes, and I already have another debugging patch to test this... it simply turns
lock_page_killable() into lock_page(). But lets check __ClearPageLocked() first
(the patch I sent a minute ago).

> BTW, the
> WARN_ONCE(!list_empty(&wait->task_list) && waitqueue_active(q), "mode =
> %#x\n", mode) statement that I added in abort_exclusive_wait() just
> produced the following call stack:

This condition is fine, and the trace is clear. This means that lock_page_killable()
was interrupted and wake_bit_function() was not called. We do not need another wakeup
in this case but somehow it helps. Again, I think because the necessary wakeup was
already lost/missed.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web