swap: redirty page if page write fails on swap file

Since commit 62c230b, swap_writepage() calls direct_IO on swap files. However, in that case page isn't redirtied if I/O fails, and is therefore handled afterwards as if it has been successfully written to the swap file, leading to memory corruption when the page is eventually swapped back in.
This patch sets the page dirty when direct_IO() fails. It fixes a memory corruption that happened while using swap-over-NFS.

Signed-off-by Jerome Marchand <[email protected]>

Since commit 62c230b, swap_writepage() calls direct_IO on swap files. However, in that case page isn't redirtied if I/O fails, and is therefore handled afterwards as if it has been successfully written to the swap file, leading to memory corruption when the page is eventually swapped back in.
This patch sets the page dirty when direct_IO() fails. It fixes a memory corruption that happened while using swap-over-NFS.

Signed-off-by Jerome Marchand <[email protected]>

Since commit 62c230b, swap_writepage() calls direct_IO on swap files. However, in that case page isn't redirtied if I/O fails, and is therefore handled afterwards as if it has been successfully written to the swap file, leading to memory corruption when the page is eventually swapped back in.
This patch sets the page dirty when direct_IO() fails. It fixes a memory

corruption that happened while using swap-over-NFS.

Signed-off-by Jerome Marchand <[email protected]>

---
  mm/page_io.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

mm/page_io.c +2 -1

index 78eee32..04ca00d 100644
@@ -222,6 +222,8 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
  		if (ret == PAGE_SIZE) {
  			count_vm_event(PSWPOUT);
  			ret = 0;
+		} else {
+			set_page_dirty(page);
  		}
  		return ret;
  	}
 
--

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

Hi Jerome,

Quoted 6 lines · Jerome Marchand

Since commit 62c230b, swap_writepage() calls direct_IO on swap files. However, in that case page isn't redirtied if I/O fails, and is therefore handled afterwards as if it has been successfully written to the swap file, leading to memory corruption when the page is eventually swapped back in.
This patch sets the page dirty when direct_IO() fails. It fixes a memory

If swapfile has related page cache which cached swapfile in memory? It is not necessary, correct?

Quoted 22 lines

corruption that happened while using swap-over-NFS.

Signed-off-by Jerome Marchand <[email protected]>

---
  mm/page_io.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

mm/page_io.c +2 -0

index 78eee32..04ca00d 100644
@@ -222,6 +222,8 @@ int swap_writepage(struct page *page, struct 
 writeback_control *wbc)
          if (ret == PAGE_SIZE) {
              count_vm_event(PSWPOUT);
              ret = 0;
+        } else {
+            set_page_dirty(page);
          }
          return ret;

      }

Quoted 5 lines

Signature · 4 lines

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

Ping, ;-)

Quoted 12 lines · Simon Jeons

Hi Jerome,

Quoted 7 lines · Jerome Marchand

Since commit 62c230b, swap_writepage() calls direct_IO on swap files. However, in that case page isn't redirtied if I/O fails, and is therefore
handled afterwards as if it has been successfully written to the swap file, leading to memory corruption when the page is eventually swapped back in.
This patch sets the page dirty when direct_IO() fails. It fixes a memory

If swapfile has related page cache which cached swapfile in memory? It is not necessary, correct?

Quoted 23 lines

Quoted 23 lines

corruption that happened while using swap-over-NFS.

Signed-off-by Jerome Marchand <[email protected]>

---
  mm/page_io.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

mm/page_io.c +2 -0

index 78eee32..04ca00d 100644
@@ -222,6 +222,8 @@ int swap_writepage(struct page *page, struct 
 writeback_control *wbc)
          if (ret == PAGE_SIZE) {
              count_vm_event(PSWPOUT);
              ret = 0;
+        } else {
+            set_page_dirty(page);
          }
          return ret;

      }

Quoted 6 lines

Quoted 5 lines

Signature · 4 lines

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

Since commit 62c230b, swap_writepage() calls direct_IO on swap files. However, in that case page isn't redirtied if I/O fails, and is therefore handled afterwards as if it has been successfully written to the swap file, leading to memory corruption when the page is eventually swapped back in.
This patch sets the page dirty when direct_IO() fails. It fixes a memory corruption that happened while using swap-over-NFS.

...

Quoted 11 lines · On Wed, 17 Apr 2013 14:11:55 +0200 Jerome Marchand <[email protected]> wrote:

Since commit 62c230b, swap_writepage() calls direct_IO on swap files. However, in that case page isn't redirtied if I/O fails, and is therefore handled afterwards as if it has been successfully written to the swap file, leading to memory corruption when the page is eventually swapped back in.
This patch sets the page dirty when direct_IO() fails. It fixes a memory corruption that happened while using swap-over-NFS.

...

mm/page_io.c int swap_writepage(struct page *page, struct writeback_control *wbc) · line 229

Quoted 11 lines

mm/page_io.c +2 -0

@@ -222,6 +222,8 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
 		if (ret == PAGE_SIZE) {
 			count_vm_event(PSWPOUT);
 			ret = 0;
+		} else {
+			set_page_dirty(page);
 		}
 		return ret;
 	}

So what happens to the page now? It remains dirty and the kernel later tries to write it again?

And if that write also fails, the page is
effectively leaked until process exit?

Aside: Mel, __swap_writepage() is fairly hair-raising. It unlocks the page before doing the IO and doesn't set PageWriteback(). Why such an exception from normal handling?

Also, what is protecting the page from concurrent reclaim or exit() during the above swap_writepage()?

Seems that the code needs a bunch of fixes or a bunch of comments explaining why it is safe and why it has to be this way.

Quoted 12 lines · Andrew Morton

Quoted 10 lines · On Wed, 17 Apr 2013 14:11:55 +0200 Jerome Marchand <[email protected]> wrote:

Since commit 62c230b, swap_writepage() calls direct_IO on swap files. However, in that case page isn't redirtied if I/O fails, and is therefore handled afterwards as if it has been successfully written to the swap file, leading to memory corruption when the page is eventually swapped back in.
This patch sets the page dirty when direct_IO() fails. It fixes a memory corruption that happened while using swap-over-NFS.

...

Quoted 13 lines

mm/page_io.c int swap_writepage(struct page *page, struct writeback_control *wbc) · line 229

Quoted 11 lines

mm/page_io.c +2 -0

@@ -222,6 +222,8 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
  		if (ret == PAGE_SIZE) {
  			count_vm_event(PSWPOUT);
  			ret = 0;
+		} else {
+			set_page_dirty(page);
  		}
  		return ret;
  	}

So what happens to the page now? It remains dirty and the kernel later tries to write it again?

Yes. Also, AS_EIO or AS_ENOSPC is set to the address space flags (in this case, swapper_space).

Quoted 3 lines

And if that write also fails, the page is
effectively leaked until process exit?

AFAICT, there is no special handling for that page afterwards, so if all subsequent attempts fail, it's indeed going to stay in memory until freed.

Jerome

Quoted 8 lines

Aside: Mel, __swap_writepage() is fairly hair-raising. It unlocks the page before doing the IO and doesn't set PageWriteback(). Why such an exception from normal handling?

Also, what is protecting the page from concurrent reclaim or exit() during the above swap_writepage()?

Quoted 3 lines

Seems that the code needs a bunch of fixes or a bunch of comments explaining why it is safe and why it has to be this way.

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

As pointed out by Andrew Morton, the swap-over-NFS writeback is not setting PageWriteback before it is queued for direct IO. While swap pages do not participate in BDI or process dirty accounting and the IO is synchronous, the writeback bit is still required and not setting it in this case was an oversight. swapoff depends on the page writeback to synchronoise all pending writes on a swap page before it is reused. Swapcache freeing and reuse depend on checking the PageWriteback under lock to ensure the page is safe to reuse.

Direct IO handlers and the direct IO handler for NFS do not deal with PageWriteback as they are synchronous writes. In the case of NFS, it schedules pages (or a page in the case of swap) for IO and then waits synchronously for IO to complete in nfs_direct_write(). It is recognised that this is a slowdown from normal swap handling which is asynchronous and uses a completion handler. Shoving PageWriteback handling down into direct IO handlers looks like a bad fit to handle the swap case although it may have to be dealt with some day if swap is converted to use direct IO in general and bmap is finally done away with. At that point it will be necessary to refit asynchronous direct IO with completion handlers onto the swap subsystem.

As swapcache currently depends on PageWriteback to protect against races, this patch sets PageWriteback under the page lock before queueing it for direct IO. It is cleared when the direct IO handler returns. IO errors are treated similarly to the direct-to-bio case except PageError is not set as in the case of swap-over-NFS, it is likely to be a transient error.

It was asked what prevents such a page being reclaimed in parallel. With this patch applied, such a page will now be skipped (most of the time) or blocked until the writeback completes. Reclaim checks PageWriteback under the page lock before calling try_to_free_swap and the page lock should prevent the page being requeued for IO before it is freed.

This and Jerome's related patch should considered for -stable as far back as 3.6 when swap-over-NFS was introduced.

mm/page_io.c +8 -0

 
...

@@ -223,8 +224,24 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
 			count_vm_event(PSWPOUT);
 			ret = 0;
 		} else {
+			/*
+			 * In the case of swap-over-nfs, this can be a
+			 * temporary failure if the system has limited
+			 * memory for allocating transmit buffers.
+			 * Mark the page dirty and avoid
+			 * rotate_reclaimable_page but rate-limit the
+			 * messages but do not flag PageError like
+			 * the normal direct-to-bio case as it could

Quoted 14 lines

(quoted diff) +12 -0

 		} else {
+			/*
+			 * In the case of swap-over-nfs, this can be a
+			 * temporary failure if the system has limited
+			 * memory for allocating transmit buffers.
+			 * Mark the page dirty and avoid
+			 * rotate_reclaimable_page but rate-limit the
+			 * messages but do not flag PageError like
+			 * the normal direct-to-bio case as it could
+			 * be temporary.
+			 */
 			set_page_dirty(page);
+			ClearPageReclaim(page);
+			if (printk_ratelimit()) {

Review

Quoted 5 lines

(quoted diff) +4 -0

+				pr_err("Write-error on dio swapfile (%Lu)\n",
+					(unsigned long long)page_file_offset(page));
+			}
 		}
+		end_page_writeback(page);

A pox upon printk_ratelimit()! Both its code comment and the checkpatch warning explain why.

mm/page_io.c renamed from mm/page_io.c~mm-swap-mark-swap-pages-writeback-before-queueing-for-direct-io-fix +2 -4

@@ -244,10 +244,8 @@ int __swap_writepage(struct page *page,
 			 */
 			set_page_dirty(page);
 			ClearPageReclaim(page);
-			if (printk_ratelimit()) {
-				pr_err("Write-error on dio swapfile (%Lu)\n",
-					(unsigned long long)page_file_offset(page));
-			}
+			pr_err_ratelimited("Write error on dio swapfile (%Lu)\n",
+				(unsigned long long)page_file_offset(page));
 		}
 		end_page_writeback(page);
 		return ret;

Do we need to cast the loff_t? afaict all architectures use long long. I didn't get a warning from sparc64 with the cast removed, and sparc64 is the one which likes to use different underlying types.

I think I'll remove it and wait for Fengguang's nastygram.

As pointed out by Andrew Morton, the swap-over-NFS writeback is not setting PageWriteback before it is queued for direct IO. While swap pages do not

participate in BDI or process dirty accounting and the IO is synchronous, the writeback bit is still required and not setting it in this case was an oversight. swapoff depends on the page writeback to synchronoise all pending writes on a swap page before it is reused. Swapcache freeing and reuse depend on checking the PageWriteback under lock to ensure the page is safe to reuse.

Direct IO handlers and the direct IO handler for NFS do not deal with PageWriteback as they are synchronous writes. In the case of NFS, it schedules pages (or a page in the case of swap) for IO and then waits synchronously for IO to complete in nfs_direct_write(). It is recognised that this is a slowdown from normal swap handling which is asynchronous and uses a completion handler. Shoving PageWriteback handling down into direct IO handlers looks like a bad fit to handle the swap case although it may have to be dealt with some day if swap is converted to use direct IO in general and bmap is finally done away with. At that point it will be necessary to refit asynchronous direct IO with completion handlers onto the swap subsystem.

As swapcache currently depends on PageWriteback to protect against races, this patch sets PageWriteback under the page lock before queueing it for direct IO. It is cleared when the direct IO handler returns. IO errors are treated similarly to the direct-to-bio case except PageError is not set as in the case of swap-over-NFS, it is likely to be a transient error.

It was asked what prevents such a page being reclaimed in parallel. With this patch applied, such a page will now be skipped (most of the time) or blocked until the writeback completes. Reclaim checks PageWriteback under the page lock before calling try_to_free_swap and the page lock should prevent the page being requeued for IO before it is freed.

This and Jerome's related patch should considered for -stable as far back as 3.6 when swap-over-NFS was introduced.

Signed-off-by Mel Gorman <[email protected]>

---
  mm/page_io.c | 17 +++++++++++++++++
  1 file changed, 17 insertions(+)

mm/page_io.c +17 -1

index 04ca00d..ec04247 100644
@@ -214,6 +214,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
  		kiocb.ki_left = PAGE_SIZE;
  		kiocb.ki_nbytes = PAGE_SIZE;
 
+		set_page_writeback(page);
  		unlock_page(page);
  		ret = mapping->a_ops->direct_IO(KERNEL_WRITE,
  						&kiocb, &iov,

@@ -223,8 +224,24 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
  			count_vm_event(PSWPOUT);
  			ret = 0;
  		} else {
+			/*
+			 * In the case of swap-over-nfs, this can be a
+			 * temporary failure if the system has limited
+			 * memory for allocating transmit buffers.
+			 * Mark the page dirty and avoid
+			 * rotate_reclaimable_page but rate-limit the
+			 * messages but do not flag PageError like
+			 * the normal direct-to-bio case as it could
+			 * be temporary.
+			 */
  			set_page_dirty(page);
+			ClearPageReclaim(page);
+			if (printk_ratelimit()) {
+				pr_err("Write-error on dio swapfile (%Lu)\n",
+					(unsigned long long)page_file_offset(page));
+			}
  		}
+		end_page_writeback(page);
  		return ret;
  	}
 
--

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

Hi Mel,

Quoted 2 lines · Mel Gorman

As pointed out by Andrew Morton, the swap-over-NFS writeback is not setting PageWriteback before it is queued for direct IO. While swap pages do not

Before commit commit 62c230bc1 (mm: add support for a filesystem to activate swap files and use direct_IO for writing swap pages), swap pages will write to page cache firstly and then writeback?