Re-dirty pages on ENOSPC when converting delayed allocations

If we get an error in xfs_page_state_convert() - and it's not EAGAIN - then we throw away the dirty page without converting the delayed allocation. This leaves delayed allocations that can never be removed and confuses code that expects a flush of the file to clear them. We need to re-dirty the page on error so we can try again later or report that the flush failed.

This change is needed to handle the condition where we are at ENOSPC and we exhaust the reserved block pool (because many transactions are executing concurrently) and calls to xfs_trans_reserve() start failing with ENOSPC errors.

Version 2 wont return EAGAIN from xfs_vm_writepage() and also converts an ENOSPC error to an EAGAIN for asynchronous writeback to avoid setting an error in the inode mapping when we don't need to.

fs/xfs/linux-2.6/xfs_aops.c +0 -2

@@ -1147,16 +1147,6 @@ error:
 	if (iohead)
 		xfs_cancel_ioend(iohead);
 
-	/*
-	 * If it's delalloc and we have nowhere to put it,

fs/xfs/linux-2.6/xfs_aops.c +1 -3

@@ -1231,19 +1221,16 @@ xfs_vm_writepage(
 	 * to real space and flush out to disk.
 	 */
 	error = xfs_page_state_convert(inode, page, wbc, 1, unmapped);
-	if (error == -EAGAIN)
-		goto out_fail;
 	if (unlikely(error < 0))
-		goto out_unlock;
+		goto out_fail;
 
 	return 0;
 

out_fail:

	redirty_page_for_writepage(wbc, page);
	unlock_page(page);

fs/xfs/linux-2.6/xfs_aops.c +2 -3

-	return 0;
-out_unlock:
-	unlock_page(page);
+	if (error == -EAGAIN)
+		error = 0;

	return error;
}

Quoted 5 lines · Lachlan McIlroy

If we get an error in xfs_page_state_convert() - and it's not EAGAIN - then we throw away the dirty page without converting the delayed allocation. This leaves delayed allocations that can never be removed and confuses code that expects a flush of the file to clear them. We need to re-dirty the page on error so we can try again later or report that the flush failed.

Actually, those delalloc pages can be removed - they just need to be handled in ->releasepage. The problem there is that the delalloc state is checked by looking at the bufferhead, and by the time we get to ->releasepage the buffer heads have already gone through discard_buffer() and lost the buffer_delay() flag.

IIRC I had a patch that did the delalloc conversion correctly in ->releasepage by utilising a custom ->invalidatepage callouut, but the performance overhead was very bad because it is done a page at a time. ISTR even posting it to oss....

Quoted 5 lines · Lachlan McIlroy

If we get an error in xfs_page_state_convert() - and it's not EAGAIN - then we throw away the dirty page without converting the delayed allocation. This leaves delayed allocations that can never be removed and confuses code that expects a flush of the file to clear them. We need to re-dirty the page on error so we can try again later or report that the flush failed.

Actually, those delalloc pages can be removed - they just need to be handled in ->releasepage. The problem there is that the delalloc state is checked by looking at the bufferhead, and by the time we get to ->releasepage the buffer heads have already gone through discard_buffer() and lost the buffer_delay() flag.

IIRC I had a patch that did the delalloc conversion correctly in ->releasepage by utilising a custom ->invalidatepage callouut, but the performance overhead was very bad because it is done a page at a time. ISTR even posting it to oss....

Quoted 17 lines

This change is needed to handle the condition where we are at ENOSPC and we exhaust the reserved block pool (because many transactions are executing concurrently) and calls to xfs_trans_reserve() start failing with ENOSPC errors.

Version 2 wont return EAGAIN from xfs_vm_writepage() and also converts an ENOSPC error to an EAGAIN for asynchronous writeback to avoid setting an error in the inode mapping when we don't need to.

fs/xfs/linux-2.6/xfs_aops.c +0 -2

@@ -1147,16 +1147,6 @@ error:
 	if (iohead)
 		xfs_cancel_ioend(iohead);
 
-	/*
-	 * If it's delalloc and we have nowhere to put it,

fs/xfs/linux-2.6/xfs_aops.c error: · line 1151

Quoted 10 lines

fs/xfs/linux-2.6/xfs_aops.c +0 -8

-	 * throw it away, unless the lower layers told
-	 * us to try again.
-	 */
-	if (err != -EAGAIN) {
-		if (!unmapped)
-			block_invalidatepage(page, 0);
-		ClearPageUptodate(page);
-	}
 	return err;
 }

So we don't throw away pages here....

Quoted 20 lines

fs/xfs/linux-2.6/xfs_aops.c +1 -3

@@ -1231,19 +1221,16 @@ xfs_vm_writepage(
 	 * to real space and flush out to disk.
 	 */
 	error = xfs_page_state_convert(inode, page, wbc, 1, unmapped);
-	if (error == -EAGAIN)
-		goto out_fail;
 	if (unlikely(error < 0))
-		goto out_unlock;
+		goto out_fail;
 
 	return 0;
 

out_fail:

	redirty_page_for_writepage(wbc, page);
	unlock_page(page);

fs/xfs/linux-2.6/xfs_aops.c +2 -3

-	return 0;
-out_unlock:
-	unlock_page(page);
+	if (error == -EAGAIN)
+		error = 0;

Quoted 2 lines

	return error;
}

And we redirty every page that comes through here with an error.

IOWs on permanent IO errors we can't get rid of the pages without a forced shutdown. That was my main objection to the first version of the patch.

fs/xfs/xfs_iomap.c xfs_iomap( · line 276

Quoted 12 lines

fs/xfs/xfs_iomap.c +2 -0

@@ -269,6 +269,8 @@ xfs_iomap(
 
 		error = xfs_iomap_write_allocate(ip, offset, count,
 						 &imap, &nimaps);
+		if ((flags & BMAPI_TRYLOCK) && error == ENOSPC)
+			error = EAGAIN;
 		break;
 	}
 

But you've added the special ENOSPC case to avoid having an error reported on non-blocking flushes that I suggested. That's not exactly what I meant or thought I was suggesting.

What I thought I suggested was to do the above ENOSPC swizzling for the non-blocking case, but still throw away pages in the blocking flush case. That is, remove the first two hunks of the patch, and just use the third hunk. That way we don't introduce entertaining new ENOSPC problems by retaining the current behaviour, but we still fix the prolonged depletion of the reserve pool by delalloc reservations which seemed to be the cause of all the ENOSPC problems.

If we get an error in xfs_page_state_convert() - and it's not EAGAIN - then we throw away the dirty page without converting the delayed allocation. This leaves delayed allocations that can never be removed and confuses code that expects a flush of the file to clear them. We need to re-dirty the page on error so we can try again later or report that the flush failed.

This change is needed to handle the condition where we are at ENOSPC and we exhaust the reserved block pool (because many transactions are executing concurrently) and calls to xfs_trans_reserve() start failing with ENOSPC errors.

Version 2 wont return EAGAIN from xfs_vm_writepage() and also converts an ENOSPC error to an EAGAIN for asynchronous writeback to avoid setting an error in the inode mapping when we don't need to.

fs/xfs/linux-2.6/xfs_aops.c 2008-10-07 17:58:04.000000000 +1000 renamed from fs/xfs/linux-2.6/xfs_aops.c 2008-10-07 17:02:04.000000000 +1000 +2 -14

@@ -1147,16 +1147,6 @@ error:
    if (iohead)
        xfs_cancel_ioend(iohead);
 
-    /*
-     * If it's delalloc and we have nowhere to put it,
-     * throw it away, unless the lower layers told
-     * us to try again.
-     */
-    if (err != -EAGAIN) {
-        if (!unmapped)
-            block_invalidatepage(page, 0);
-        ClearPageUptodate(page);
-    }
    return err;
 }
 

@@ -1185,7 +1175,7 @@ xfs_vm_writepage(
    struct page        *page,
    struct writeback_control *wbc)
 {
-    int            error;
+    int            error = 0;
    int            need_trans;
    int            delalloc, unmapped, unwritten;
    struct inode        *inode = page->mapping->host;

@@ -1231,19 +1221,16 @@ xfs_vm_writepage(
     * to real space and flush out to disk.
     */
    error = xfs_page_state_convert(inode, page, wbc, 1, unmapped);
-    if (error == -EAGAIN)
-        goto out_fail;
    if (unlikely(error < 0))
-        goto out_unlock;
+        goto out_fail;
 
    return 0;
 

out_fail:

    redirty_page_for_writepage(wbc, page);
    unlock_page(page);

fs/xfs/linux-2.6/xfs_aops.c 2008-10-07 17:58:04.000000000 +1000 renamed from fs/xfs/linux-2.6/xfs_aops.c 2008-10-07 17:02:04.000000000 +1000 +2 -3

-    return 0;
-out_unlock:
-    unlock_page(page);
+    if (error == -EAGAIN)
+        error = 0;
    return error;

}

fs/xfs/xfs_iomap.c 2008-10-07 17:58:04.000000000 +1000 renamed from fs/xfs/xfs_iomap.c 2008-10-07 17:02:04.000000000 +1000 +2 -0

@@ -269,6 +269,8 @@ xfs_iomap(
 
        error = xfs_iomap_write_allocate(ip, offset, count,
                         &imap, &nimaps);
+        if ((flags & BMAPI_TRYLOCK) && error == ENOSPC)
+            error = EAGAIN;
        break;
    }