| Message ID | 63068e8e-8bee-b208-8441-a3c39a9d9eb6@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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;
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(-)