Date:	Sun, 1 Nov 2015 15:39:07 -0800 (PST)
From:	Hugh Dickins <hughd@...gle.com>
To:	Boaz Harrosh <ooo@...ctrozaur.com>
cc:	Hugh Dickins <hughd@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Benny Halevy <bhalevy@...marydata.com>,
	Trond Myklebust <trond.myklebust@...marydata.com>,
	Christoph Lameter <cl@...ux.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-nfs@...r.kernel.org, linux-mm@...ck.org, osd-dev@...n-osd.org
Subject: Re: [PATCH] osd fs: __r4w_get_page rely on PageUptodate for
 uptodate

On Sun, 1 Nov 2015, Boaz Harrosh wrote:
> On 10/29/2015 08:43 PM, Hugh Dickins wrote:
> > Patch "mm: migrate dirty page without clear_page_dirty_for_io etc",
> > presently staged in mmotm and linux-next, simplifies the migration of
> > a PageDirty pagecache page: one stat needs moving from zone to zone
> > and that's about all.
> > 
> > It's convenient and safest for it to shift the PageDirty bit from old
> > page to new, just before updating the zone stats: before copying data
> > and marking the new PageUptodate.  This is all done while both pages
> > are isolated and locked, just as before; and just as before, there's
> > a moment when the new page is visible in the radix_tree, but not yet
> > PageUptodate.  What's new is that it may now be briefly visible as
> > PageDirty before it is PageUptodate.
> > 
> > When I scoured the tree to see if this could cause a problem anywhere,
> > the only places I found were in two similar functions __r4w_get_page():
> > which look up a page with find_get_page() (not using page lock), then
> > claim it's uptodate if it's PageDirty or PageWriteback or PageUptodate.
> > 
> > I'm not sure whether that was right before, but now it might be wrong
> > (on rare occasions): only claim the page is uptodate if PageUptodate.
> > Or perhaps the page in question could never be migratable anyway?
> > 
> 
> Hi Sir Hugh

Hi Boaz - please pardon my informality :)

> 
> I'm sorry, I admit the code is clear as mud, but your patch below is wrong.
> 
> The *uptodate return from __r4w_get_page is not really "up-to-date" at all
> actually it means: "do we need to read the page from storage" writable/dirty pages
> we do not read from storage but use the newest data in memory.
> 
> r4w means read-for-write which is when we need to bring in the full stripe to
> re-calculate raid5/6 . (when only the partial stripe is written)

Yes, that's what I understood from the code too, and how PageUptodate
is usually used: it allows the caller to bypass the overhead of locking
the page, rechecking PageUptodate, and reading it in if still necessary.

> 
> The scenario below of: "briefly visible as PageDirty before it is PageUptodate"
> is fine in this case because in both cases we do not need to read the page.

But when do you think you have a PageDirty (or PageWriteback) page which
is not PageUptodate?  We do not ClearPageUptodate when a page is modified.

PageUptodate normally remains set for as long as that page remains caching
that offset of the file.  I think it's true to say that PageUptodate is
only cleared when an error, or sometimes an invalidation, occurs (or of
course when the page is freed for reuse).

I was going to suggest that you check through the places which
ClearPageUptodate, but that is rather a confusing exercise, since I think
the majority of them are actually redundant - pages don't come from the
allocator with PageUptodate set, and a filesystem would already be in
trouble if it set PageUptodate before the page was initialized (usually
by reading its data in from disk).  So I think those ClearPageUptodates
on read error are redundant; though I'm not daring to remove them
(and they have no bearing on this patch at hand).

> 
> Thanks for looking
> Boaz
> 
> > Signed-off-by: Hugh Dickins <hughd@...gle.com>
> 
> 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.

Thanks,
Hugh

> 
> > ---
> > 
> >  fs/exofs/inode.c             |    5 +----
> >  fs/nfs/objlayout/objio_osd.c |    5 +----
> >  2 files changed, 2 insertions(+), 8 deletions(-)
> > 
> > --- 4.3-next/fs/exofs/inode.c	2015-08-30 11:34:09.000000000 -0700
> > +++ linux/fs/exofs/inode.c	2015-10-28 16:55:18.795554294 -0700
> > @@ -592,10 +592,7 @@ static struct page *__r4w_get_page(void
> >  			}
> >  			unlock_page(page);
> >  		}
> > -		if (PageDirty(page) || PageWriteback(page))
> > -			*uptodate = true;
> > -		else
> > -			*uptodate = PageUptodate(page);
> > +		*uptodate = PageUptodate(page);
> >  		EXOFS_DBGMSG2("index=0x%lx uptodate=%d\n", index, *uptodate);
> >  		return page;
> >  	} else {
> > --- 4.3-next/fs/nfs/objlayout/objio_osd.c	2015-10-21 18:35:07.620645439 -0700
> > +++ linux/fs/nfs/objlayout/objio_osd.c	2015-10-28 16:53:55.083686639 -0700
> > @@ -476,10 +476,7 @@ static struct page *__r4w_get_page(void
> >  		}
> >  		unlock_page(page);
> >  	}
> > -	if (PageDirty(page) || PageWriteback(page))
> > -		*uptodate = true;
> > -	else
> > -		*uptodate = PageUptodate(page);
> > +	*uptodate = PageUptodate(page);
> >  	dprintk("%s: index=0x%lx uptodate=%d\n", __func__, index, *uptodate);
> >  	return page;
> >  }
> > 
> 
> 
--
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/

Powered by blists - more mailing lists