On Mon, 2 Nov 2015, Boaz Harrosh wrote: > On 11/02/2015 01:39 AM, Hugh Dickins wrote: > <> > >> This patch is not correct! > > > > I think you have actually confirmed that the patch is correct: > > why bother to test PageDirty or PageWriteback when PageUptodate > > already tells you what you need? > > > > Or do these filesystems do something unusual with PageUptodate > > when PageDirty is set? I didn't find it. > > > > This is kind of delicate stuff. It took me a while to get it right > when I did it. I don't remember all the details. > > But consider this option: Thanks, yes, it helps to have a concrete example in front of us. > > exofs_write_begin on a full PAGE_CACHE_SIZE, the page is instantiated > new in page-cache is that PageUptodate(page) then? I thought not. Right, PageUptodate must not be set until the page has been filled with the correct data. Nor is PageDirty or PageWriteback set at this point, actually. Once page is filled with the correct data, either exofs_write_end() (which uses simple_write_end()) or (internally) exofs_commit_chunk() is called. > (exofs does not set that) It's simple_write_end() or exofs_commit_chunk() which SetPageUptodate in this case. And after that each calls set_page_dirty(), which does the SetPageDirty, before unlocking the page which was supplied locked by exofs_write_begin(). So I don't see where the page is PageDirty without being PageUptodate. > > Now that page I do not want to read in. The latest data is in memory. > (Same when this page is in writeback, dirty-bit is cleared) Understood, but that's what PageUptodate is for. (Quite what happens if there's a write error is not so clear: I think that typically PageError gets set and PageUptodate cleared, to read back in from disk what's actually there - but lose the data we wanted to write; but I can understand different filesystems making different choices there, and didn't study exofs's choice.) > > So for sure if page is dirty or writeback then we surly do not need a read. > only if not then we need to consider the PageUptodate(page) state. PageUptodate is the proper flag to check, to ask if the page contains the correct data: there is no need to consider Dirty or Writeback. > > Do you think the code is actually wrong as is? Not that I know of: just a little too complicated and confusing. But becomes slightly wrong if my simplification to page migration goes through, since that introduces an instant when PageDirty is set before the new page contains the correct data and is marked Uptodate. Hence my patch. > > BTW: Very similar code is in fs/nfs/objlayout/objio_osd.c::__r4w_get_page Indeed, the patch makes the same adjustment to that code too. > > > Thanks, > > Hugh > > > <> > > Thanks > Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
