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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.0907240857420.14156@cobra.newdream.net>
Date:	Fri, 24 Jul 2009 09:52:37 -0700 (PDT)
From:	Sage Weil <sage@...dream.net>
To:	Andi Kleen <andi@...stfloor.org>
cc:	Trond Myklebust <trond.myklebust@....uio.no>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/19] ceph: address space operations

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/