Re: [PATCH] osd fs: __r4w_get_page rely on PageUptodate for uptodate

[Hugh Dickins <hughd@google.com>]

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>