Re: [PATCH] mm: Make truncate_inode_pages_range() killable

Re: [PATCH] mm: Make truncate_inode_pages_range() killable

[Hugh Dickins]

On Fri, 14 Apr 2017, Bart Van Assche wrote:

> The default behavior of multipathd is to run kpartx against newly
> discovered paths. Avoid that these kpartx processes become unkillable
> if there are no paths left and when using queue_if_no_path. This patch
> avoids that kpartx sporadically hangs as follows:
> 
> Call Trace:
>  __schedule+0x3df/0xc10
>  schedule+0x3d/0x90
>  io_schedule+0x16/0x40
>  __lock_page+0x111/0x140
>  truncate_inode_pages_range+0x462/0x790
>  truncate_inode_pages+0x15/0x20
>  kill_bdev+0x35/0x40
>  __blkdev_put+0x76/0x220
>  blkdev_put+0x4e/0x170
>  blkdev_close+0x25/0x30
>  __fput+0xed/0x1f0
>  ____fput+0xe/0x10
>  task_work_run+0x85/0xc0
>  do_exit+0x311/0xc70
>  do_group_exit+0x50/0xd0
>  get_signal+0x2c7/0x930
>  do_signal+0x28/0x6b0
>  exit_to_usermode_loop+0x62/0xa0
>  do_syscall_64+0xda/0x140
>  entry_SYSCALL64_slow_path+0x25/0x25
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: linux-mm@kvack.org

Changing a fundamental function, silently not to do its essential job,
when something in the kernel has forgotten (or is slow to) unlock_page():
that seems very wrong to me in many ways.  But linux-fsdevel, Cc'ed, will
be a better forum to advise on how to solve the problem you're seeing.

Hugh

> ---
>  mm/truncate.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 6263affdef88..91abd16d74f8 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -20,6 +20,7 @@
>  #include <linux/task_io_accounting_ops.h>
>  #include <linux/buffer_head.h>	/* grr. try_to_release_page,
>  				   do_invalidatepage */
> +#include <linux/sched/signal.h>
>  #include <linux/shmem_fs.h>
>  #include <linux/cleancache.h>
>  #include <linux/rmap.h>
> @@ -366,7 +367,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  		return;
>  
>  	index = start;
> -	for ( ; ; ) {
> +	for ( ; !signal_pending_state(TASK_WAKEKILL, current); ) {
>  		cond_resched();
>  		if (!pagevec_lookup_entries(&pvec, mapping, index,
>  			min(end - index, (pgoff_t)PAGEVEC_SIZE), indices)) {
> @@ -400,7 +401,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  				continue;
>  			}
>  
> -			lock_page(page);
> +			if (lock_page_killable(page))
> +				break;
>  			WARN_ON(page_to_index(page) != index);
>  			wait_on_page_writeback(page);
>  			truncate_inode_page(mapping, page);
> -- 
> 2.12.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Make truncate_inode_pages_range() killable

[Bart Van Assche]

On Fri, 2017-04-14 at 17:40 -0700, Hugh Dickins wrote:
> Changing a fundamental function, silently not to do its essential job,
> when something in the kernel has forgotten (or is slow to) unlock_page():
> that seems very wrong to me in many ways.  But linux-fsdevel, Cc'ed, will
> be a better forum to advise on how to solve the problem you're seeing.

Hello Hugh,

It seems like you have misunderstood the purpose of the patch I posted. It's
neither a missing unlock_page() nor slow I/O that I want to address but a
genuine deadlock. In case you would not be familiar with the queue_if_no_path
multipath configuration option, the multipath.conf man page is available at
e.g. https://linux.die.net/man/5/multipath.conf.

Bart.

Re: [PATCH] mm: Make truncate_inode_pages_range() killable

[Michal Hocko]

On Sat 15-04-17 00:59:46, Bart Van Assche wrote:
> On Fri, 2017-04-14 at 17:40 -0700, Hugh Dickins wrote:
> > Changing a fundamental function, silently not to do its essential job,
> > when something in the kernel has forgotten (or is slow to) unlock_page():
> > that seems very wrong to me in many ways.  But linux-fsdevel, Cc'ed, will
> > be a better forum to advise on how to solve the problem you're seeing.
> 
> Hello Hugh,
> 
> It seems like you have misunderstood the purpose of the patch I posted. It's
> neither a missing unlock_page() nor slow I/O that I want to address but a
> genuine deadlock. In case you would not be familiar with the queue_if_no_path
> multipath configuration option, the multipath.conf man page is available at
> e.g. https://linux.die.net/man/5/multipath.conf.

So, whole is holding the page lock and why it cannot make forward
progress? Is the storage gone so that the ongoing IO will never
terminate? Btw. we have many other places which wait for the page lock
!killable way. Why they are any different from this case?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: Make truncate_inode_pages_range() killable

[Bart Van Assche]

On Tue, 2017-04-18 at 10:15 +0200, Michal Hocko wrote:
> On Sat 15-04-17 00:59:46, Bart Van Assche wrote:
> > On Fri, 2017-04-14 at 17:40 -0700, Hugh Dickins wrote:
> > > Changing a fundamental function, silently not to do its essential job,
> > > when something in the kernel has forgotten (or is slow to) unlock_page():
> > > that seems very wrong to me in many ways.  But linux-fsdevel, Cc'ed, will
> > > be a better forum to advise on how to solve the problem you're seeing.
> > 
> > It seems like you have misunderstood the purpose of the patch I posted. It's
> > neither a missing unlock_page() nor slow I/O that I want to address but a
> > genuine deadlock. In case you would not be familiar with the queue_if_no_path
> > multipath configuration option, the multipath.conf man page is available at
> > e.g. https://linux.die.net/man/5/multipath.conf.
> 
> So, who is holding the page lock and why it cannot make forward
> progress? Is the storage gone so that the ongoing IO will never
> terminate? Btw. we have many other places which wait for the page lock
> !killable way. Why they are any different from this case?

Hello Michal,

queue_if_no_path means that if no paths are available that the dm-mpath driver
does not complete an I/O request until a path becomes available. A standard
test for multipathed storage is to alternatingly remove and restore all paths.

If the reported lockup happens at the end of a test I can break the cycle by
running "dmsetup message ${mpath} 0 fail_if_no_path". That command causes
pending I/O requests to fail if no paths are available.

I think it is rather unintuitive that kill -9 does not work for a process that
uses a dm-mpath device for I/O as long as no paths are available.

The call stack I reported in the first e-mail in this thread is what I ran
into while running multipath tests. I'm not sure why I have not yet hit any
other code paths that perform an unkillable wait on a page lock.

Bart.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>