Re-dirty pages on I/O error
PATCH 1/1 Re-dirty pages on I/O error
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.
fs/xfs/linux-2.6/xfs_aops.c +0 -0
@@ -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; }
@@ -1216,8 +1206,11 @@ xfs_vm_writepage(
* then mark the page dirty again and leave the page
* as is.
*/
- if (current_test_flags(PF_FSTRANS) && need_trans)
- goto out_fail;
+ if (current_test_flags(PF_FSTRANS) && need_trans) {
+ redirty_page_for_writepage(wbc, page);
+ unlock_page(page);
+ return -EAGAIN;
+ }
/* * Delay hooking up buffer heads until we have @@ -1231,20 +1224,14 @@ 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;
- return 0;
+ if (error < 0) {
+ redirty_page_for_writepage(wbc, page);
+ unlock_page(page);
+ return error;
+ }
-out_fail:
- redirty_page_for_writepage(wbc, page);
- unlock_page(page); return 0;
-out_unlock:
- unlock_page(page);
- return error;
}
STATIC int
Quoted 12 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.fs/xfs/linux-2.6/xfs_aops.c +0
-0@@ -1147,16 +1147,6 @@ error:if (iohead)xfs_cancel_ioend (iohead);
fs/xfs/linux-2.6/xfs_aops.c error: · line 1159
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +0
-10-/* -* 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); - }
While this always looked fishy to me we it needs a good explanation to kill this. I try to remember why Steve did it this way long time ago.
fs/xfs/linux-2.6/xfs_aops.c xfs_vm_writepage( · line 1212
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +4
-2@@ -1216,8 +1206,11 @@ xfs_vm_writepage(* then mark the page dirty again and leave the page * as is. */ -if (current_test_flags (PF_FSTRANS ) && need_trans) - goto out_fail; +if (current_test_flags (PF_FSTRANS ) && need_trans) { +redirty_page_for_writepage (wbc, page); +unlock_page (page); +return -EAGAIN ;
The redirty, unlock, return sequence is duplicated after your patch, I think we should still keep the out_fail goto. Also returning -EGAIN from ->writepage is wrong. The return values goes through handle_write_error and mapping_set_error into the return value of e.g. msync. If you look at all similar writepage implementation they only return a negative error for a real error condition and simply return 0 when just redirtying it due to transaction constraints or when trylocks fail.
Quoted 13 lines · Christoph Hellwig
Quoted 12 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.fs/xfs/linux-2.6/xfs_aops.c +0
-0@@ -1147,16 +1147,6 @@ error:if (iohead)xfs_cancel_ioend (iohead);
Quoted 13 lines
fs/xfs/linux-2.6/xfs_aops.c error: · line 1159
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +0
-10-/* -* 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); - }While this always looked fishy to me we it needs a good explanation to kill this. I try to remember why Steve did it this way long time ago.
Hrm some of that was my fault, ages ago.
I don't remember the details fo why.... ah here's a clue
http://oss.sgi.com/archives/xfs/2002-01/msg00475.html
Quoted 6 lines
As
_xfs_force_shutdownwas written, it tried to schedule in an interrupt context and caused aBUG()to be thrown.Also, even if we didn't try to deal with leftover buffers in the interrupt, they subsequently had their delalloc flags removed, and thus queued up to clobber block 0 (1,2,3) on the disk, thus corrupting the filesystem.
so back then, delalloc buffers w/o a home would eventually slam into the superblock, I guess.
Anyway, if this is redirtied, will it ever go away for an IO error that persists?
-Eric
Quoted 21 lines
fs/xfs/linux-2.6/xfs_aops.c xfs_vm_writepage( · line 1212
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +4
-2@@ -1216,8 +1206,11 @@ xfs_vm_writepage(* then mark the page dirty again and leave the page * as is. */ -if (current_test_flags (PF_FSTRANS ) && need_trans) - goto out_fail; +if (current_test_flags (PF_FSTRANS ) && need_trans) { +redirty_page_for_writepage (wbc, page); +unlock_page (page); +return -EAGAIN ;The redirty, unlock, return sequence is duplicated after your patch, I think we should still keep the
out_failgoto. Also returning-EGAINfrom ->writepage is wrong. The return values goes throughhandle_write_errorandmapping_set_errorinto the return value of e.g. msync. If you look at all similar writepage implementation they only return a negative error for a real error condition and simply return 0 when just redirtying it due to transaction constraints or when trylocks fail.
Quoted 13 lines · Christoph Hellwig
Quoted 12 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.fs/xfs/linux-2.6/xfs_aops.c +0
-0@@ -1147,16 +1147,6 @@ error:if (iohead)xfs_cancel_ioend (iohead);
Quoted 13 lines
fs/xfs/linux-2.6/xfs_aops.c error: · line 1159
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +0
-10-/* -* 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); - }While this always looked fishy to me we it needs a good explanation to kill this. I try to remember why Steve did it this way long time ago.
We'd need an even better explanation to keep it. It's just plain wrong and has caused serious customer issues for us.
At ENOSPC calls to xfs_trans_reserve() can fail if the reserved space is also exhausted (see m_resblks). This causes conversion of delayed allocs to fail with ENOSPC. This dodgy code throws the page away and leaves the delayed alloc. A direct I/O then panics because it finds a delayed allocation when it does not expect one.
This code has been there since the initial version of the file.
Quoted 12 lines
fs/xfs/linux-2.6/xfs_aops.c xfs_vm_writepage( · line 1212
Quoted 11 lines
fs/xfs/linux-2.6/xfs_aops.c +4
-2@@ -1216,8 +1206,11 @@ xfs_vm_writepage(* then mark the page dirty again and leave the page * as is. */ -if (current_test_flags (PF_FSTRANS ) && need_trans) - goto out_fail; +if (current_test_flags (PF_FSTRANS ) && need_trans) { +redirty_page_for_writepage (wbc, page); +unlock_page (page); +return -EAGAIN ;
Quoted 8 lines
The redirty, unlock, return sequence is duplicated after your patch, I think we should still keep the
out_failgoto. Also returning-EGAINfrom ->writepage is wrong. The return values goes throughhandle_write_errorandmapping_set_errorinto the return value of e.g. msync. If you look at all similar writepage implementation they only return a negative error for a real error condition and simply return 0 when just redirtying it due to transaction constraints or when trylocks fail.
I got rid of the goto because it looked messy but if you want it back I'll change it.
As for the EAGAIN, I'll change that to 0. I don't think it makes any difference to the problem I'm trying to fix.
Quoted 27 lines · Eric Sandeen
Quoted 13 lines · Christoph Hellwig
Quoted 12 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.fs/xfs/linux-2.6/xfs_aops.c +0
-0@@ -1147,16 +1147,6 @@ error:if (iohead)xfs_cancel_ioend (iohead);Quoted 12 lines
fs/xfs/linux-2.6/xfs_aops.c error: · line 1159
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +0
-10-/* -* 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); - }While this always looked fishy to me we it needs a good explanation to kill this. I try to remember why Steve did it this way long time ago.
Quoted 3 lines
Hrm some of that was my fault, ages ago.
That revision history doesn't match the ptools history - probably cause the file no longer exists. Anyway it looks like the hack was mostly there before your change.
Quoted 4 lines
I don't remember the details fo why.... ah here's a clue
Geez, that's so long ago that I doubt it's still an issue. So it sounds like this hack was added to prevent further issues after a forced shutdown has already occurred. If we leave this hack in there's a good chance it will cause a forced shutdown.
Quoted 10 lines
Quoted 7 lines
As
_xfs_force_shutdownwas written, it tried to schedule in an interrupt context and caused aBUG()to be thrown.Also, even if we didn't try to deal with leftover buffers in the interrupt, they subsequently had their delalloc flags removed, and thus queued up to clobber block 0 (1,2,3) on the disk, thus corrupting the filesystem.
so back then, delalloc buffers w/o a home would eventually slam into the superblock, I guess.
Really? How do you figure? Delayed allocations have a special block number of -1. When that is converted to the physical disk address it should end up beyond the end of the volume.
Quoted 3 lines
Anyway, if this is redirtied, will it ever go away for an IO error that persists?
For the case that I am trying to fix the error should not be persistent. If there is a persistent error (and it doesn't result in a forced shutdown) then I think we'll just keep trying to flush the data.
For errors that are ultimately persistant we wont know on the first I/O that these are going to be persistant errors so I don't think it makes sense to toss the data straight away - we may have temporarily lost our link to the storage.
If there is a serious error (ie that results in a forced shutdown) then we should end up tossing the data in xfs_sync_inodes().
Quoted 26 lines
-Eric
Quoted 20 lines
fs/xfs/linux-2.6/xfs_aops.c xfs_vm_writepage( · line 1212
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +4
-2@@ -1216,8 +1206,11 @@ xfs_vm_writepage(* then mark the page dirty again and leave the page * as is. */ -if (current_test_flags (PF_FSTRANS ) && need_trans) - goto out_fail; +if (current_test_flags (PF_FSTRANS ) && need_trans) { +redirty_page_for_writepage (wbc, page); +unlock_page (page); +return -EAGAIN ;The redirty, unlock, return sequence is duplicated after your patch, I think we should still keep the
out_failgoto. Also returning-EGAINfrom ->writepage is wrong. The return values goes throughhandle_write_errorandmapping_set_errorinto the return value of e.g. msync. If you look at all similar writepage implementation they only return a negative error for a real error condition and simply return 0 when just redirtying it due to transaction constraints or when trylocks fail.
Quoted 27 lines · Lachlan McIlroy
Quoted 26 lines · Eric Sandeen
Quoted 25 lines · Christoph Hellwig
Quoted 12 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.fs/xfs/linux-2.6/xfs_aops.c +0
-0@@ -1147,16 +1147,6 @@ error:if (iohead)xfs_cancel_ioend (iohead);fs/xfs/linux-2.6/xfs_aops.c error: · line 1159
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +0
-10-/* -* 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); - }While this always looked fishy to me we it needs a good explanation to kill this. I try to remember why Steve did it this way long time ago.
Quoted 6 lines
Quoted 3 lines
Hrm some of that was my fault, ages ago.
That revision history doesn't match the ptools history - probably cause the file no longer exists. Anyway it looks like the hack was mostly there before your change.
Before my change it just cleared the delalloc flag....
Quoted 7 lines
Quoted 3 lines
I don't remember the details fo why.... ah here's a clue
Geez, that's so long ago that I doubt it's still an issue. So it sounds like this hack was added to prevent further issues after a forced shutdown has already occurred. If we leave this hack in there's a good chance it will cause a forced shutdown.
I'm not arguing to keep it, just trying to figure out where it's from :)
Quoted 10 lines
Quoted 7 lines
Quoted 5 lines
As
_xfs_force_shutdownwas written, it tried to schedule in an interrupt context and caused aBUG()to be thrown.
Also, even if we didn't try to deal with leftover buffers in the interrupt, they subsequently had their delalloc flags removed, and thus queued up to clobber block 0 (1,2,3) on the disk, thus corrupting the filesystem.so back then, delalloc buffers w/o a home would eventually slam into the superblock, I guess.
Really? How do you figure? Delayed allocations have a special block number of -1. When that is converted to the physical disk address it should end up beyond the end of the volume.
Well, I don't remember how things worked back in 2002 for sure, but pretty sure the net result was that we simply cleared the delalloc page on the flag, and it eventually came out at block 0. Back then. A lot has changed since!
-Eric
Quoted 14 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.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 keep dirty pages around that we can't write back? If we are in a low memory situation and the block device has gone bad, that will prevent memory reclaim from making progress.
i.e. if we have a bad disk, a user can now take down the system by running it out of clean memory....
The EAGAIN case is for when we can't get the locks we need during non-blocking writeback, which is a common case if there is concurrent writes to this inode....
fs/xfs/linux-2.6/xfs_aops.c xfs_vm_writepage( · line 1213
Quoted 11 lines
fs/xfs/linux-2.6/xfs_aops.c +5
-2@@ -1216,8 +1206,11 @@ xfs_vm_writepage(* then mark the page dirty again and leave the page * as is. */ -if (current_test_flags (PF_FSTRANS ) && need_trans) - goto out_fail; +if (current_test_flags (PF_FSTRANS ) && need_trans) { +redirty_page_for_writepage (wbc, page); +unlock_page (page); +return -EAGAIN ; + }
Should not return an error here - the redirty_page_for_writepage() call effective says "can't do this right away, but no error needs to be reported because it can be written again later". Happens all the time with non-blocking writes.
Quoted 5 lines
fs/xfs/linux-2.6/xfs_aops.c +0
-1@@ -1231,20 +1224,14 @@ xfs_vm_writepage(* to real space and flush out to disk. */ error =xfs_page_state_convert (inode, page, wbc,1 , unmapped); -if (error == -EAGAIN )
fs/xfs/linux-2.6/xfs_aops.c xfs_vm_writepage( · line 1232
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +5
-4- goto out_fail; -if (unlikely (error <0 )) - goto out_unlock; -return 0 ; +if (error <0 ) { +redirty_page_for_writepage (wbc, page); +unlock_page (page); +return error; + }
That needs the EAGAIN exception - that's not an error. For EIO, though, we should really be invalidating the page like we currently do. However, it should be silent as per the current behaviour - a rate-limited log warning is really needed here...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 13 lines · Dave Chinner
Quoted 12 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.fs/xfs/linux-2.6/xfs_aops.c +0
-0@@ -1147,16 +1147,6 @@ error:if (iohead)xfs_cancel_ioend (iohead);
Quoted 14 lines
fs/xfs/linux-2.6/xfs_aops.c error: · line 1151
Quoted 12 lines
fs/xfs/linux-2.6/xfs_aops.c +0
-10-/* -* 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; }So we keep dirty pages around that we can't write back?
Yes.
Quoted 3 lines
If we are in a low memory situation and the block device has gone bad, that will prevent memory reclaim from making progress.
How do you differentiate "gone bad" from temporarily unavailable?
Quoted 3 lines
i.e. if we have a bad disk, a user can now take down the system by running it out of clean memory....
I'm sure there's many ways a malicious user could already do that. Would you rather have data corruption?
We've allowed the write() to succeed. We've accepted the data. We have an obligation to write it do disk. Either we keep trying in the face of errors or we take down the filesystem.
Quoted 5 lines
The EAGAIN case is for when we can't get the locks we need during non-blocking writeback, which is a common case if there is concurrent writes to this inode....
Quoted 16 lines
fs/xfs/linux-2.6/xfs_aops.c xfs_vm_writepage( · line 1213
Quoted 11 lines
fs/xfs/linux-2.6/xfs_aops.c +5
-2@@ -1216,8 +1206,11 @@ xfs_vm_writepage(* then mark the page dirty again and leave the page * as is. */ -if (current_test_flags (PF_FSTRANS ) && need_trans) - goto out_fail; +if (current_test_flags (PF_FSTRANS ) && need_trans) { +redirty_page_for_writepage (wbc, page); +unlock_page (page); +return -EAGAIN ; + }Should not return an error here - the
redirty_page_for_writepage()call effective says "can't do this right away, but no error needs to be reported because it can be written again later". Happens all the time with non-blocking writes.
Christoph already pointed that one out.
Quoted 10 lines
Quoted 10 lines
fs/xfs/linux-2.6/xfs_aops.c +0
-4@@ -1231,20 +1224,14 @@ 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;
Quoted 12 lines
fs/xfs/linux-2.6/xfs_aops.c xfs_vm_writepage( · line 1232
Quoted 6 lines
fs/xfs/linux-2.6/xfs_aops.c +5
-1-return 0 ; +if (error <0 ) { +redirty_page_for_writepage (wbc, page); +unlock_page (page); +return error; + }That needs the EAGAIN exception - that's not an error. For EIO, though, we should really be invalidating the page like we currently do. However, it should be silent as per the current behaviour - a rate-limited log warning is really needed here...
The EAGAIN case can be exceptioned. The error we are getting here is ENOSPC because xfs_trans_reserve() is failing. It looks to me like __writepage() and mapping_set_error() want to know about that error. We also need that error to be propogated back to any callers of xfs_flushinval_pages() and the only way to do that is to return the actual error. Just redirtying the page wont return an error all the way back up the call stack.
Silently discarding data just doesn't make sense. Issuing a log message isn't much better - noone will look for it until after they realise there's a problem and all their files are corrupt.
Quoted 8 lines · Lachlan McIlroy
Quoted 1 line · Dave Chinner
So we keep dirty pages around that we can't write back?
Yes.
Quoted 3 lines
If we are in a low memory situation and the block device has gone bad, that will prevent memory reclaim from making progress.
How do you differentiate "gone bad" from temporarily unavailable?
The only "temporary" error you can get in writeback is a path failure. IIRC, XVM will give an ENODEV on a path failure, but I don't think that dm-multipath does. Other than that, a write failure is unrecoverable. Any other error is permanent....
Quoted 3 lines
Quoted 2 lines
i.e. if we have a bad disk, a user can now take down the system by running it out of clean memory....
I'm sure there's many ways a malicious user could already do that.
That's no excuse for introducing a new way of taking down the system when a disk fails. Error handling in linux is bad enough without intentionally preventing the system from recovering from I/O errors...
Quoted 1 line
Would you rather have data corruption?
Data corruption as a result of an I/O error? What else can we be expected to do? Log the error and continue onwards....
Face it - if the drive is dead then we can't write the data anywhere, so keeping it around and potentially killing the system completely makes even less sense. At some point we *have to give up* on data we can't write back....
Quoted 3 lines
We've allowed the
write()to succeed. We've accepted the data. We have an obligation to write it do disk. Either we keep trying in the face of errors or we take down the filesystem.
It's write-behind buffering. We give best effort, not guaranteed writeback. If the system crashes, that data is lost. If we get an I/O error, that data is lost. If the application cares, it uses fsync and it gets the error and can handle it.
.....
Quoted 2 lines
The EAGAIN case can be exceptioned. The error we are getting here is ENOSPC because
xfs_trans_reserve()is failing.
<sigh>
Please - put that detail in the patch description. I'm getting a little tired of having to draw out the reasons for your patches one little bit at a time.
So: why is xfs_trans_reserve() failing? Aren't all the transactions in the writeback path marked as XFS_TRANS_RESERVE? That allows the transaction reserve to succeed when at ENOSPC by dipping into the reserved blocks. Did we run out of reserved blocks (i.e. the reserve pool is not big enough)? Or is there some other case that leads to ENOSPC in the writeback path that we've never considered before?
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 45 lines · Dave Chinner
Quoted 8 lines · Lachlan McIlroy
Quoted 1 line · Dave Chinner
So we keep dirty pages around that we can't write back?
Yes.
Quoted 3 lines
If we are in a low memory situation and the block device has gone bad, that will prevent memory reclaim from making progress.
How do you differentiate "gone bad" from temporarily unavailable?
The only "temporary" error you can get in writeback is a path failure. IIRC, XVM will give an ENODEV on a path failure, but I don't think that dm-multipath does. Other than that, a write failure is unrecoverable. Any other error is permanent....
Quoted 3 lines
Quoted 2 lines
i.e. if we have a bad disk, a user can now take down the system by running it out of clean memory....
I'm sure there's many ways a malicious user could already do that.
That's no excuse for introducing a new way of taking down the system when a disk fails. Error handling in linux is bad enough without intentionally preventing the system from recovering from I/O errors...
Quoted 1 line
Would you rather have data corruption?
Data corruption as a result of an I/O error? What else can we be expected to do? Log the error and continue onwards....
Face it - if the drive is dead then we can't write the data anywhere, so keeping it around and potentially killing the system completely makes even less sense. At some point we *have to give up* on data we can't write back....
Quoted 3 lines
We've allowed the
write()to succeed. We've accepted the data. We have an obligation to write it do disk. Either we keep trying in the face of errors or we take down the filesystem.It's write-behind buffering. We give best effort, not guaranteed writeback. If the system crashes, that data is lost. If we get an I/O error, that data is lost. If the application cares, it uses fsync and it gets the error and can handle it.
.....
Quoted 8 lines
Quoted 2 lines
The EAGAIN case can be exceptioned. The error we are getting here is ENOSPC because
xfs_trans_reserve()is failing.<sigh>
Please - put that detail in the patch description. I'm getting a little tired of having to draw out the reasons for your patches one little bit at a time.
I intentionally left out that detail because that's not what I'm trying to fix here. Discarding data arbitrarily is wrong and needs to be fixed regardless of the error. I've already mentioned what the cause of the ENOSPC is earlier in this thread.
Quoted 7 lines
So: why is
xfs_trans_reserve()failing? Aren't all the transactions in the writeback path marked asXFS_TRANS_RESERVE? That allows the transaction reserve to succeed when at ENOSPC by dipping into the reserved blocks. Did we run out of reserved blocks (i.e. the reserve pool is not big enough)? Or is there some other case that leads to ENOSPC in the writeback path that we've never considered before?
Yes, xfs_trans_reserve() is failing, it is marked XFS_TRANS_RESERVE and we ran out of the reserved pool. We've tried bumping the pool from 1024 blocks to 16384 blocks and we can still cause it to fail so we'll need to make the default even higher. This ENOSPC error is not necessarily a permanent error and in this case we shouldn't discard the page.