[PATCH] do_generic_file_read(): Fail immediately if killed

[PATCH] do_generic_file_read(): Fail immediately if killed

[Bart Van Assche]

If a fatal signal has been received, fail immediately instead of
trying to read more data.

See also commit ebded02788b5 ("mm: filemap: avoid unnecessary
calls to lock_page when waiting for IO to complete during a read")

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Jan Kara <jack@suse.cz>
Cc: Hugh Dickins <hughd@google.com>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 mm/filemap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 2a9e84f6..bd8ab63 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1721,7 +1721,9 @@ 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 (unlikely(error))
+				goto readpage_error;
 			if (PageUptodate(page))
 				goto page_ok;
 
-- 
2.9.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] do_generic_file_read(): Fail immediately if killed

[Jan Kara]

On Tue 16-08-16 17:00:43, Bart Van Assche wrote:
> If a fatal signal has been received, fail immediately instead of
> trying to read more data.
> 
> See also commit ebded02788b5 ("mm: filemap: avoid unnecessary
> calls to lock_page when waiting for IO to complete during a read")
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Oleg Nesterov <oleg@redhat.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

BTW: Did you see some real world impact of the change? If yes, it would be
good to describe in the changelog.

								Honza
> ---
>  mm/filemap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 2a9e84f6..bd8ab63 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1721,7 +1721,9 @@ 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 (unlikely(error))
> +				goto readpage_error;
>  			if (PageUptodate(page))
>  				goto page_ok;
>  
> -- 
> 2.9.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
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] do_generic_file_read(): Fail immediately if killed

[Oleg Nesterov]

On 08/16, Bart Van Assche wrote:
>
> If a fatal signal has been received, fail immediately instead of
> trying to read more data.

This looks a bit misleading to me.

If wait_on_page_locked_killable() was interrupted then this page is most
likely is not PageUptodate() and in this case do_generic_file_read() will
fail after lock_page_killable().

But as I already said, I belive the change itself is fine,

> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1721,7 +1721,9 @@ 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 (unlikely(error))
> +				goto readpage_error;
>  			if (PageUptodate(page))
>  				goto page_ok;

Acked-by: Oleg Nesterov <oleg@redhat.com>

--
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] do_generic_file_read(): Fail immediately if killed

[Bart Van Assche]

On 08/17/2016 03:02 AM, Jan Kara wrote:
> On Tue 16-08-16 17:00:43, Bart Van Assche wrote:
>> If a fatal signal has been received, fail immediately instead of
>> trying to read more data.
>>
>> See also commit ebded02788b5 ("mm: filemap: avoid unnecessary
>> calls to lock_page when waiting for IO to complete during a read")
>
> The patch looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> BTW: Did you see some real world impact of the change? If yes, it would be
> good to describe in the changelog.

Hello Jan,

Thanks for the review.

This patch has an impact on my tests. However, I do not yet have a full
root-cause analysis for what I observed in my tests. That is why I
hadn't mentioned any further details in the patch description.

While running fio on top of a filesystem (ext4 or xfs), dm-mpath and
the ib_srp driver I noticed that removing and restoring paths triggered
several types of hangs. The call trace of one of these hangs, the one
that made me look at do_generic_file_read(), can be found below. I'm
currently testing a block layer patch to see whether it resolves this
hang.

Bart.


kpartx          D ffff8800409d3be8     0 16392  16355 0x00000000
Call Trace:
 [<ffffffff8161f577>] schedule+0x37/0x90
 [<ffffffff81623bcf>] schedule_timeout+0x27f/0x470
 [<ffffffff8161e94f>] io_schedule_timeout+0x9f/0x110
 [<ffffffff8161fd16>] bit_wait_io+0x16/0x60
 [<ffffffff8161f9a6>] __wait_on_bit+0x56/0x80
 [<ffffffff81152e2d>] wait_on_page_bit_killable+0xbd/0xc0
 [<ffffffff81152f60>] generic_file_read_iter+0x130/0x770
 [<ffffffff812134b0>] blkdev_read_iter+0x30/0x40
 [<ffffffff811d267b>] __vfs_read+0xbb/0x130
 [<ffffffff811d2a61>] vfs_read+0x91/0x130
 [<ffffffff811d3de4>] SyS_read+0x44/0xa0
 [<ffffffff81624fa5>] entry_SYSCALL_64_fastpath+0x18/0xa8

--
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] do_generic_file_read(): Fail immediately if killed

[Michal Hocko]

On Tue 16-08-16 17:00:43, Bart Van Assche wrote:
> If a fatal signal has been received, fail immediately instead of
> trying to read more data.
> 
> See also commit ebded02788b5 ("mm: filemap: avoid unnecessary
> calls to lock_page when waiting for IO to complete during a read")
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Oleg Nesterov <oleg@redhat.com>

If not anything else it makes code more readable because the real error
is hidden in page_not_up_to_date: currently...

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/filemap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 2a9e84f6..bd8ab63 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1721,7 +1721,9 @@ 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 (unlikely(error))
> +				goto readpage_error;
>  			if (PageUptodate(page))
>  				goto page_ok;
>  
> -- 
> 2.9.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>

-- 
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>