* [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
@ 2013-05-30 18:05 ` Rafael Aquini
0 siblings, 0 replies; 14+ messages in thread
From: Rafael Aquini @ 2013-05-30 18:05 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, akpm, hughd, shli, riel, lwoodman, kosaki.motohiro,
kamezawa.hiroyu, stable
read_swap_cache_async() can race against get_swap_page(), and stumble across
a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
swapcache yet. This transient swap_map state is expected to be transitory,
but the actual placement of discard at scan_swap_map() inserts a wait for
I/O completion thus making the thread at read_swap_cache_async() to loop
around its -EEXIST case, while the other end at get_swap_page()
is scheduled away at scan_swap_map(). This can leave the system deadlocked
if the I/O completion happens to be waiting on the CPU workqueue where
read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
This patch introduces a cond_resched() call to make the aforementioned
read_swap_cache_async() busy loop condition to bail out when necessary,
thus avoiding the subtle race window.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
mm/swap_state.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/mm/swap_state.c b/mm/swap_state.c
index b3d40dc..9ad9e3b 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -336,8 +336,20 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
* Swap entry may have been freed since our caller observed it.
*/
err = swapcache_prepare(entry);
- if (err == -EEXIST) { /* seems racy */
+ if (err == -EEXIST) {
radix_tree_preload_end();
+ /*
+ * We might race against get_swap_page() and stumble
+ * across a SWAP_HAS_CACHE swap_map entry whose page
+ * has not been brought into the swapcache yet, while
+ * the other end is scheduled away waiting on discard
+ * I/O completion.
+ * In order to avoid turning this transitory state
+ * into a permanent loop around this -EEXIST case,
+ * lets just conditionally invoke the scheduler,
+ * if there are some more important tasks to run.
+ */
+ cond_resched();
continue;
}
if (err) { /* swp entry is obsolete ? */
--
1.8.1.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
@ 2013-05-30 18:05 ` Rafael Aquini
0 siblings, 0 replies; 14+ messages in thread
From: Rafael Aquini @ 2013-05-30 18:05 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, akpm, hughd, shli, riel, lwoodman, kosaki.motohiro,
kamezawa.hiroyu, stable
read_swap_cache_async() can race against get_swap_page(), and stumble across
a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
swapcache yet. This transient swap_map state is expected to be transitory,
but the actual placement of discard at scan_swap_map() inserts a wait for
I/O completion thus making the thread at read_swap_cache_async() to loop
around its -EEXIST case, while the other end at get_swap_page()
is scheduled away at scan_swap_map(). This can leave the system deadlocked
if the I/O completion happens to be waiting on the CPU workqueue where
read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
This patch introduces a cond_resched() call to make the aforementioned
read_swap_cache_async() busy loop condition to bail out when necessary,
thus avoiding the subtle race window.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
mm/swap_state.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/mm/swap_state.c b/mm/swap_state.c
index b3d40dc..9ad9e3b 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -336,8 +336,20 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
* Swap entry may have been freed since our caller observed it.
*/
err = swapcache_prepare(entry);
- if (err == -EEXIST) { /* seems racy */
+ if (err == -EEXIST) {
radix_tree_preload_end();
+ /*
+ * We might race against get_swap_page() and stumble
+ * across a SWAP_HAS_CACHE swap_map entry whose page
+ * has not been brought into the swapcache yet, while
+ * the other end is scheduled away waiting on discard
+ * I/O completion.
+ * In order to avoid turning this transitory state
+ * into a permanent loop around this -EEXIST case,
+ * lets just conditionally invoke the scheduler,
+ * if there are some more important tasks to run.
+ */
+ cond_resched();
continue;
}
if (err) { /* swp entry is obsolete ? */
--
1.8.1.4
--
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>
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
2013-05-30 18:05 ` Rafael Aquini
@ 2013-05-30 18:32 ` Greg KH
-1 siblings, 0 replies; 14+ messages in thread
From: Greg KH @ 2013-05-30 18:32 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, linux-mm, akpm, hughd, shli, riel, lwoodman,
kosaki.motohiro, kamezawa.hiroyu, stable
On Thu, May 30, 2013 at 03:05:00PM -0300, Rafael Aquini wrote:
> read_swap_cache_async() can race against get_swap_page(), and stumble across
> a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> swapcache yet. This transient swap_map state is expected to be transitory,
> but the actual placement of discard at scan_swap_map() inserts a wait for
> I/O completion thus making the thread at read_swap_cache_async() to loop
> around its -EEXIST case, while the other end at get_swap_page()
> is scheduled away at scan_swap_map(). This can leave the system deadlocked
> if the I/O completion happens to be waiting on the CPU workqueue where
> read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
>
> This patch introduces a cond_resched() call to make the aforementioned
> read_swap_cache_async() busy loop condition to bail out when necessary,
> thus avoiding the subtle race window.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> mm/swap_state.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
@ 2013-05-30 18:32 ` Greg KH
0 siblings, 0 replies; 14+ messages in thread
From: Greg KH @ 2013-05-30 18:32 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, linux-mm, akpm, hughd, shli, riel, lwoodman,
kosaki.motohiro, kamezawa.hiroyu, stable
On Thu, May 30, 2013 at 03:05:00PM -0300, Rafael Aquini wrote:
> read_swap_cache_async() can race against get_swap_page(), and stumble across
> a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> swapcache yet. This transient swap_map state is expected to be transitory,
> but the actual placement of discard at scan_swap_map() inserts a wait for
> I/O completion thus making the thread at read_swap_cache_async() to loop
> around its -EEXIST case, while the other end at get_swap_page()
> is scheduled away at scan_swap_map(). This can leave the system deadlocked
> if the I/O completion happens to be waiting on the CPU workqueue where
> read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
>
> This patch introduces a cond_resched() call to make the aforementioned
> read_swap_cache_async() busy loop condition to bail out when necessary,
> thus avoiding the subtle race window.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> mm/swap_state.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
--
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
2013-05-30 18:05 ` Rafael Aquini
@ 2013-05-30 19:55 ` Johannes Weiner
-1 siblings, 0 replies; 14+ messages in thread
From: Johannes Weiner @ 2013-05-30 19:55 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, linux-mm, akpm, hughd, shli, riel, lwoodman,
kosaki.motohiro, kamezawa.hiroyu, stable
On Thu, May 30, 2013 at 03:05:00PM -0300, Rafael Aquini wrote:
> read_swap_cache_async() can race against get_swap_page(), and stumble across
> a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> swapcache yet. This transient swap_map state is expected to be transitory,
> but the actual placement of discard at scan_swap_map() inserts a wait for
> I/O completion thus making the thread at read_swap_cache_async() to loop
> around its -EEXIST case, while the other end at get_swap_page()
> is scheduled away at scan_swap_map(). This can leave the system deadlocked
> if the I/O completion happens to be waiting on the CPU workqueue where
waitqueue?
> read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
>
> This patch introduces a cond_resched() call to make the aforementioned
> read_swap_cache_async() busy loop condition to bail out when necessary,
> thus avoiding the subtle race window.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> mm/swap_state.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index b3d40dc..9ad9e3b 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -336,8 +336,20 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> * Swap entry may have been freed since our caller observed it.
> */
> err = swapcache_prepare(entry);
> - if (err == -EEXIST) { /* seems racy */
> + if (err == -EEXIST) {
> radix_tree_preload_end();
> + /*
> + * We might race against get_swap_page() and stumble
> + * across a SWAP_HAS_CACHE swap_map entry whose page
> + * has not been brought into the swapcache yet, while
> + * the other end is scheduled away waiting on discard
> + * I/O completion.
> + * In order to avoid turning this transitory state
> + * into a permanent loop around this -EEXIST case,
> + * lets just conditionally invoke the scheduler,
> + * if there are some more important tasks to run.
> + */
> + cond_resched();
Might be worth mentioning the !CONFIG_PREEMPT deadlock scenario here,
especially since under CONFIG_PREEMPT the radix_tree_preload_end() is
already a scheduling point through the preempt_enable().
Other than that, the patch looks good to me!
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
@ 2013-05-30 19:55 ` Johannes Weiner
0 siblings, 0 replies; 14+ messages in thread
From: Johannes Weiner @ 2013-05-30 19:55 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, linux-mm, akpm, hughd, shli, riel, lwoodman,
kosaki.motohiro, kamezawa.hiroyu, stable
On Thu, May 30, 2013 at 03:05:00PM -0300, Rafael Aquini wrote:
> read_swap_cache_async() can race against get_swap_page(), and stumble across
> a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> swapcache yet. This transient swap_map state is expected to be transitory,
> but the actual placement of discard at scan_swap_map() inserts a wait for
> I/O completion thus making the thread at read_swap_cache_async() to loop
> around its -EEXIST case, while the other end at get_swap_page()
> is scheduled away at scan_swap_map(). This can leave the system deadlocked
> if the I/O completion happens to be waiting on the CPU workqueue where
waitqueue?
> read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
>
> This patch introduces a cond_resched() call to make the aforementioned
> read_swap_cache_async() busy loop condition to bail out when necessary,
> thus avoiding the subtle race window.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> ---
> mm/swap_state.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index b3d40dc..9ad9e3b 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -336,8 +336,20 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> * Swap entry may have been freed since our caller observed it.
> */
> err = swapcache_prepare(entry);
> - if (err == -EEXIST) { /* seems racy */
> + if (err == -EEXIST) {
> radix_tree_preload_end();
> + /*
> + * We might race against get_swap_page() and stumble
> + * across a SWAP_HAS_CACHE swap_map entry whose page
> + * has not been brought into the swapcache yet, while
> + * the other end is scheduled away waiting on discard
> + * I/O completion.
> + * In order to avoid turning this transitory state
> + * into a permanent loop around this -EEXIST case,
> + * lets just conditionally invoke the scheduler,
> + * if there are some more important tasks to run.
> + */
> + cond_resched();
Might be worth mentioning the !CONFIG_PREEMPT deadlock scenario here,
especially since under CONFIG_PREEMPT the radix_tree_preload_end() is
already a scheduling point through the preempt_enable().
Other than that, the patch looks good to me!
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
--
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
2013-05-30 18:05 ` Rafael Aquini
@ 2013-05-30 19:59 ` KOSAKI Motohiro
-1 siblings, 0 replies; 14+ messages in thread
From: KOSAKI Motohiro @ 2013-05-30 19:59 UTC (permalink / raw)
To: Rafael Aquini
Cc: LKML, linux-mm, Andrew Morton, Hugh Dickins, shli, riel,
Larry Woodman, KAMEZAWA Hiroyuki, stable
On Thu, May 30, 2013 at 2:05 PM, Rafael Aquini <aquini@redhat.com> wrote:
> read_swap_cache_async() can race against get_swap_page(), and stumble across
> a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> swapcache yet. This transient swap_map state is expected to be transitory,
> but the actual placement of discard at scan_swap_map() inserts a wait for
> I/O completion thus making the thread at read_swap_cache_async() to loop
> around its -EEXIST case, while the other end at get_swap_page()
> is scheduled away at scan_swap_map(). This can leave the system deadlocked
> if the I/O completion happens to be waiting on the CPU workqueue where
> read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
>
> This patch introduces a cond_resched() call to make the aforementioned
> read_swap_cache_async() busy loop condition to bail out when necessary,
> thus avoiding the subtle race window.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
@ 2013-05-30 19:59 ` KOSAKI Motohiro
0 siblings, 0 replies; 14+ messages in thread
From: KOSAKI Motohiro @ 2013-05-30 19:59 UTC (permalink / raw)
To: Rafael Aquini
Cc: LKML, linux-mm, Andrew Morton, Hugh Dickins, shli, riel,
Larry Woodman, KAMEZAWA Hiroyuki, stable
On Thu, May 30, 2013 at 2:05 PM, Rafael Aquini <aquini@redhat.com> wrote:
> read_swap_cache_async() can race against get_swap_page(), and stumble across
> a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> swapcache yet. This transient swap_map state is expected to be transitory,
> but the actual placement of discard at scan_swap_map() inserts a wait for
> I/O completion thus making the thread at read_swap_cache_async() to loop
> around its -EEXIST case, while the other end at get_swap_page()
> is scheduled away at scan_swap_map(). This can leave the system deadlocked
> if the I/O completion happens to be waiting on the CPU workqueue where
> read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
>
> This patch introduces a cond_resched() call to make the aforementioned
> read_swap_cache_async() busy loop condition to bail out when necessary,
> thus avoiding the subtle race window.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
2013-05-30 19:55 ` Johannes Weiner
@ 2013-05-30 21:56 ` Rafael Aquini
-1 siblings, 0 replies; 14+ messages in thread
From: Rafael Aquini @ 2013-05-30 21:56 UTC (permalink / raw)
To: Johannes Weiner
Cc: linux-kernel, linux-mm, akpm, hughd, shli, riel, lwoodman,
kosaki.motohiro, kamezawa.hiroyu, stable
On Thu, May 30, 2013 at 03:55:39PM -0400, Johannes Weiner wrote:
> On Thu, May 30, 2013 at 03:05:00PM -0300, Rafael Aquini wrote:
> > read_swap_cache_async() can race against get_swap_page(), and stumble across
> > a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> > swapcache yet. This transient swap_map state is expected to be transitory,
> > but the actual placement of discard at scan_swap_map() inserts a wait for
> > I/O completion thus making the thread at read_swap_cache_async() to loop
> > around its -EEXIST case, while the other end at get_swap_page()
> > is scheduled away at scan_swap_map(). This can leave the system deadlocked
> > if the I/O completion happens to be waiting on the CPU workqueue where
>
> waitqueue?
>
Ugh! I will repost this to fix it and the "compeletion" typo at subject...
> > read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
> >
> > This patch introduces a cond_resched() call to make the aforementioned
> > read_swap_cache_async() busy loop condition to bail out when necessary,
> > thus avoiding the subtle race window.
> >
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > ---
> > mm/swap_state.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index b3d40dc..9ad9e3b 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -336,8 +336,20 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> > * Swap entry may have been freed since our caller observed it.
> > */
> > err = swapcache_prepare(entry);
> > - if (err == -EEXIST) { /* seems racy */
> > + if (err == -EEXIST) {
> > radix_tree_preload_end();
> > + /*
> > + * We might race against get_swap_page() and stumble
> > + * across a SWAP_HAS_CACHE swap_map entry whose page
> > + * has not been brought into the swapcache yet, while
> > + * the other end is scheduled away waiting on discard
> > + * I/O completion.
> > + * In order to avoid turning this transitory state
> > + * into a permanent loop around this -EEXIST case,
> > + * lets just conditionally invoke the scheduler,
> > + * if there are some more important tasks to run.
> > + */
> > + cond_resched();
>
> Might be worth mentioning the !CONFIG_PREEMPT deadlock scenario here,
> especially since under CONFIG_PREEMPT the radix_tree_preload_end() is
> already a scheduling point through the preempt_enable().
>
Nice suggestion, will do it. Thanks for reviewing this patch!
> Other than that, the patch looks good to me!
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
@ 2013-05-30 21:56 ` Rafael Aquini
0 siblings, 0 replies; 14+ messages in thread
From: Rafael Aquini @ 2013-05-30 21:56 UTC (permalink / raw)
To: Johannes Weiner
Cc: linux-kernel, linux-mm, akpm, hughd, shli, riel, lwoodman,
kosaki.motohiro, kamezawa.hiroyu, stable
On Thu, May 30, 2013 at 03:55:39PM -0400, Johannes Weiner wrote:
> On Thu, May 30, 2013 at 03:05:00PM -0300, Rafael Aquini wrote:
> > read_swap_cache_async() can race against get_swap_page(), and stumble across
> > a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> > swapcache yet. This transient swap_map state is expected to be transitory,
> > but the actual placement of discard at scan_swap_map() inserts a wait for
> > I/O completion thus making the thread at read_swap_cache_async() to loop
> > around its -EEXIST case, while the other end at get_swap_page()
> > is scheduled away at scan_swap_map(). This can leave the system deadlocked
> > if the I/O completion happens to be waiting on the CPU workqueue where
>
> waitqueue?
>
Ugh! I will repost this to fix it and the "compeletion" typo at subject...
> > read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
> >
> > This patch introduces a cond_resched() call to make the aforementioned
> > read_swap_cache_async() busy loop condition to bail out when necessary,
> > thus avoiding the subtle race window.
> >
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > ---
> > mm/swap_state.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index b3d40dc..9ad9e3b 100644
> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -336,8 +336,20 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> > * Swap entry may have been freed since our caller observed it.
> > */
> > err = swapcache_prepare(entry);
> > - if (err == -EEXIST) { /* seems racy */
> > + if (err == -EEXIST) {
> > radix_tree_preload_end();
> > + /*
> > + * We might race against get_swap_page() and stumble
> > + * across a SWAP_HAS_CACHE swap_map entry whose page
> > + * has not been brought into the swapcache yet, while
> > + * the other end is scheduled away waiting on discard
> > + * I/O completion.
> > + * In order to avoid turning this transitory state
> > + * into a permanent loop around this -EEXIST case,
> > + * lets just conditionally invoke the scheduler,
> > + * if there are some more important tasks to run.
> > + */
> > + cond_resched();
>
> Might be worth mentioning the !CONFIG_PREEMPT deadlock scenario here,
> especially since under CONFIG_PREEMPT the radix_tree_preload_end() is
> already a scheduling point through the preempt_enable().
>
Nice suggestion, will do it. Thanks for reviewing this patch!
> Other than that, the patch looks good to me!
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
--
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
2013-05-30 18:05 ` Rafael Aquini
@ 2013-05-30 22:02 ` Hugh Dickins
-1 siblings, 0 replies; 14+ messages in thread
From: Hugh Dickins @ 2013-05-30 22:02 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, linux-mm, akpm, shli, riel, lwoodman,
kosaki.motohiro, kamezawa.hiroyu, stable
On Thu, 30 May 2013, Rafael Aquini wrote:
> read_swap_cache_async() can race against get_swap_page(), and stumble across
> a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> swapcache yet. This transient swap_map state is expected to be transitory,
> but the actual placement of discard at scan_swap_map() inserts a wait for
> I/O completion thus making the thread at read_swap_cache_async() to loop
> around its -EEXIST case, while the other end at get_swap_page()
> is scheduled away at scan_swap_map(). This can leave the system deadlocked
> if the I/O completion happens to be waiting on the CPU workqueue where
> read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
>
> This patch introduces a cond_resched() call to make the aforementioned
> read_swap_cache_async() busy loop condition to bail out when necessary,
> thus avoiding the subtle race window.
Yes, I never realized this at the time I inserted discard there.
As you know, Shaohua has a better swap discard implementation, which
avoids the problem by using SWAP_MAP_BAD, but this cond_resched() is
a good simple workaround for now - thanks.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org
> ---
> mm/swap_state.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index b3d40dc..9ad9e3b 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -336,8 +336,20 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> * Swap entry may have been freed since our caller observed it.
> */
> err = swapcache_prepare(entry);
> - if (err == -EEXIST) { /* seems racy */
> + if (err == -EEXIST) {
> radix_tree_preload_end();
> + /*
> + * We might race against get_swap_page() and stumble
> + * across a SWAP_HAS_CACHE swap_map entry whose page
> + * has not been brought into the swapcache yet, while
> + * the other end is scheduled away waiting on discard
> + * I/O completion.
> + * In order to avoid turning this transitory state
> + * into a permanent loop around this -EEXIST case,
> + * lets just conditionally invoke the scheduler,
> + * if there are some more important tasks to run.
> + */
> + cond_resched();
> continue;
> }
> if (err) { /* swp entry is obsolete ? */
> --
> 1.8.1.4
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion
@ 2013-05-30 22:02 ` Hugh Dickins
0 siblings, 0 replies; 14+ messages in thread
From: Hugh Dickins @ 2013-05-30 22:02 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-kernel, linux-mm, akpm, shli, riel, lwoodman,
kosaki.motohiro, kamezawa.hiroyu, stable
On Thu, 30 May 2013, Rafael Aquini wrote:
> read_swap_cache_async() can race against get_swap_page(), and stumble across
> a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
> swapcache yet. This transient swap_map state is expected to be transitory,
> but the actual placement of discard at scan_swap_map() inserts a wait for
> I/O completion thus making the thread at read_swap_cache_async() to loop
> around its -EEXIST case, while the other end at get_swap_page()
> is scheduled away at scan_swap_map(). This can leave the system deadlocked
> if the I/O completion happens to be waiting on the CPU workqueue where
> read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
>
> This patch introduces a cond_resched() call to make the aforementioned
> read_swap_cache_async() busy loop condition to bail out when necessary,
> thus avoiding the subtle race window.
Yes, I never realized this at the time I inserted discard there.
As you know, Shaohua has a better swap discard implementation, which
avoids the problem by using SWAP_MAP_BAD, but this cond_resched() is
a good simple workaround for now - thanks.
>
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org
> ---
> mm/swap_state.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index b3d40dc..9ad9e3b 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -336,8 +336,20 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> * Swap entry may have been freed since our caller observed it.
> */
> err = swapcache_prepare(entry);
> - if (err == -EEXIST) { /* seems racy */
> + if (err == -EEXIST) {
> radix_tree_preload_end();
> + /*
> + * We might race against get_swap_page() and stumble
> + * across a SWAP_HAS_CACHE swap_map entry whose page
> + * has not been brought into the swapcache yet, while
> + * the other end is scheduled away waiting on discard
> + * I/O completion.
> + * In order to avoid turning this transitory state
> + * into a permanent loop around this -EEXIST case,
> + * lets just conditionally invoke the scheduler,
> + * if there are some more important tasks to run.
> + */
> + cond_resched();
> continue;
> }
> if (err) { /* swp entry is obsolete ? */
> --
> 1.8.1.4
--
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O completion
2013-05-30 18:05 ` Rafael Aquini
@ 2013-05-30 22:49 ` Rafael Aquini
-1 siblings, 0 replies; 14+ messages in thread
From: Rafael Aquini @ 2013-05-30 22:49 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, akpm, hughd, shli, riel, lwoodman, kosaki.motohiro,
kamezawa.hiroyu
read_swap_cache_async() can race against get_swap_page(), and stumble across
a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
swapcache yet. This transient swap_map state is expected to be transitory,
but the actual placement of discard at scan_swap_map() inserts a wait for
I/O completion thus making the thread at read_swap_cache_async() to loop
around its -EEXIST case, while the other end at get_swap_page()
is scheduled away at scan_swap_map(). This can leave the system deadlocked
if the I/O completion happens to be waiting on the CPU waitqueue where
read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
This patch introduces a cond_resched() call to make the aforementioned
read_swap_cache_async() busy loop condition to bail out when necessary,
thus avoiding the subtle race window.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Acked-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org
---
Changelog:
* fixed typos in commit message; (hannes)
* enhanced the commentary on the deadlock scenario; (hannes)
* include the received ACKs and Cc: stable
mm/swap_state.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/mm/swap_state.c b/mm/swap_state.c
index b3d40dc..f24ab0d 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -336,8 +336,24 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
* Swap entry may have been freed since our caller observed it.
*/
err = swapcache_prepare(entry);
- if (err == -EEXIST) { /* seems racy */
+ if (err == -EEXIST) {
radix_tree_preload_end();
+ /*
+ * We might race against get_swap_page() and stumble
+ * across a SWAP_HAS_CACHE swap_map entry whose page
+ * has not been brought into the swapcache yet, while
+ * the other end is scheduled away waiting on discard
+ * I/O completion at scan_swap_map().
+ *
+ * In order to avoid turning this transitory state
+ * into a permanent loop around this -EEXIST case
+ * if !CONFIG_PREEMPT and the I/O completion happens
+ * to be waiting on the CPU waitqueue where we are now
+ * busy looping, we just conditionally invoke the
+ * scheduler here, if there are some more important
+ * tasks to run.
+ */
+ cond_resched();
continue;
}
if (err) { /* swp entry is obsolete ? */
--
1.8.1.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O completion
@ 2013-05-30 22:49 ` Rafael Aquini
0 siblings, 0 replies; 14+ messages in thread
From: Rafael Aquini @ 2013-05-30 22:49 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, akpm, hughd, shli, riel, lwoodman, kosaki.motohiro,
kamezawa.hiroyu
read_swap_cache_async() can race against get_swap_page(), and stumble across
a SWAP_HAS_CACHE entry in the swap map whose page wasn't brought into the
swapcache yet. This transient swap_map state is expected to be transitory,
but the actual placement of discard at scan_swap_map() inserts a wait for
I/O completion thus making the thread at read_swap_cache_async() to loop
around its -EEXIST case, while the other end at get_swap_page()
is scheduled away at scan_swap_map(). This can leave the system deadlocked
if the I/O completion happens to be waiting on the CPU waitqueue where
read_swap_cache_async() is busy looping and !CONFIG_PREEMPT.
This patch introduces a cond_resched() call to make the aforementioned
read_swap_cache_async() busy loop condition to bail out when necessary,
thus avoiding the subtle race window.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Acked-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org
---
Changelog:
* fixed typos in commit message; (hannes)
* enhanced the commentary on the deadlock scenario; (hannes)
* include the received ACKs and Cc: stable
mm/swap_state.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/mm/swap_state.c b/mm/swap_state.c
index b3d40dc..f24ab0d 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -336,8 +336,24 @@ struct page *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
* Swap entry may have been freed since our caller observed it.
*/
err = swapcache_prepare(entry);
- if (err == -EEXIST) { /* seems racy */
+ if (err == -EEXIST) {
radix_tree_preload_end();
+ /*
+ * We might race against get_swap_page() and stumble
+ * across a SWAP_HAS_CACHE swap_map entry whose page
+ * has not been brought into the swapcache yet, while
+ * the other end is scheduled away waiting on discard
+ * I/O completion at scan_swap_map().
+ *
+ * In order to avoid turning this transitory state
+ * into a permanent loop around this -EEXIST case
+ * if !CONFIG_PREEMPT and the I/O completion happens
+ * to be waiting on the CPU waitqueue where we are now
+ * busy looping, we just conditionally invoke the
+ * scheduler here, if there are some more important
+ * tasks to run.
+ */
+ cond_resched();
continue;
}
if (err) { /* swp entry is obsolete ? */
--
1.8.1.4
--
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>
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-05-30 22:50 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-30 18:05 [PATCH] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O compeletion Rafael Aquini
2013-05-30 18:05 ` Rafael Aquini
2013-05-30 18:32 ` Greg KH
2013-05-30 18:32 ` Greg KH
2013-05-30 19:55 ` Johannes Weiner
2013-05-30 19:55 ` Johannes Weiner
2013-05-30 21:56 ` Rafael Aquini
2013-05-30 21:56 ` Rafael Aquini
2013-05-30 19:59 ` KOSAKI Motohiro
2013-05-30 19:59 ` KOSAKI Motohiro
2013-05-30 22:02 ` Hugh Dickins
2013-05-30 22:02 ` Hugh Dickins
2013-05-30 22:49 ` [PATCH v2] swap: avoid read_swap_cache_async() race to deadlock while waiting on discard I/O completion Rafael Aquini
2013-05-30 22:49 ` Rafael Aquini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.