Re: [PATCH] Re-dirty pages on I/O error

Lachlan McIlroy
2008-09-12T06:35:37Z

Eric Sandeen wrote:
> Christoph Hellwig wrote:
>> On Thu, Sep 11, 2008 at 06:37:33PM +1000, Lachlan McIlroy wrote:
>>> 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.
>>>
>>> --- a/fs/xfs/linux-2.6/xfs_aops.c 2008-09-11 16:32:11.000000000 +1000
>>> +++ b/fs/xfs/linux-2.6/xfs_aops.c 2008-09-11 15:44:09.000000000 +1000
>>> @@ -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);
>>> - }
>> 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.
>
> http://oss.sgi.com/cgi-bin/cvsweb.cgi/xfs-linux/pagebuf/Attic/page_buf_io.c.diff?r1=1.2;r2=1.3;hideattic=0
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.

>
> I don't remember the details fo why.... ah here's a clue
>
> http://oss.sgi.com/archives/xfs/2002-01/msg00475.html
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.

>
>> As _xfs_force_shutdown was written, it tried to schedule in an interrupt context
>> and caused a BUG() 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.

>
> 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().

>
> -Eric
>
>>> @@ -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.
>>
>>
>
>
>