Re: [PATCH 08/19] ceph: address space operations

[Sage Weil]

On Fri, 24 Jul 2009, Andi Kleen wrote:
> > The part I don't understand is what actually happens to pages after the 
> > error flag set.  They're still uptodate, but no longer dirty?  And can be 
> > overwritten/redirtied?  There's also an error flag on the address_space.  
> > Are there any guidelines as far as which should be used?
> 
> Ideally both. The Page error flag prevents the data from being
> consumed and the address space error flag makes sure errors are 
> getting reported on fsync()/close() etc.  Also AS error is useful when
> you don't have a clear page to assign the error to, e.g. if you
> get an error indication that's not tied to a read operation.
> 
> BTW the upcoming hwpoison code can set such errors asynchronously
> under you.

I looked through the various callers, and these look correct:

readpages -- return error code.  Doesn't matter, we'll get a call to 
->readpage later.

writepages (async) -- return error code if we fail during request setup, 
or call mapping_set_error() on completion.  If 
wait_on_page_writeback_range() is any indication, that's correct.


These I'm unsure about:

readpage -- currently calls SetPageError(page) _and_ returns error code.  
I don't see any page error checks outside the writeout paths, so a simple 
return value looks sufficient for a synchronous read.  OTOH 
mpage_end_io_read() and end_buffer_async_read() both do SetPageError() for 
async readpage (although I don't see where that is checked).

writepage -- SetPageError(page) _and_ return error code.  I see that 
wait_on_page_writeback_range() and write_one_page() check the page error 
after wait_on_page_writeback(), so that looks correct for an async 
writepage.  My writepage() is currently synchronous (i.e. does 
wait_on_page_writeback itself), so worry the page error bit is 
superfluous?  I should make it async, regardless.  Also, 
mpage_end_io_write() sets both PageError and AS_EIO on the mapping, so an 
async writepage should also set the mapping bit in the async case.


I'm guessing on the (??) items, but this at least looks consistent (as far 
as the sync vs async implementations go):

readpage (sync) -- return error and SetPageError (??)
readpage (async) -- SetPageError
readpages -- doesn't matter
writepage (sync) -- return error and SetPageError (__writepage() sets 
	mapping error)   (??)
writepage (async) -- SetPageError AND mapping_set_error
writepages (sync) -- return error AND mapping_set_error (??)
writepages (async) -- mapping_set_error

Does that sound right?  If so, I can update 
Documentation/filesystems/vfs.txt appropriately.

Thanks-
sage

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/