Re: [PATCH 0/5] export __clear_page_buffers to cleanup code

[Matthew Wilcox <willy@xxxxxxxxxxxxx>]

On Mon, Apr 20, 2020 at 09:20:46AM +1000, Dave Chinner wrote:
> On Sat, Apr 18, 2020 at 08:14:43PM -0700, Matthew Wilcox wrote:
> > On Sun, Apr 19, 2020 at 12:51:18AM +0200, Guoqing Jiang wrote:
> > > When reading md code, I find md-bitmap.c copies __clear_page_buffers from
> > > buffer.c, and after more search, seems there are some places in fs could
> > > use this function directly. So this patchset tries to export the function
> > > and use it to cleanup code.
> > 
> > OK, I see why you did this, but there are a couple of problems with it.
> > 
> > One is just a sequencing problem; between exporting __clear_page_buffers()
> > and removing it from the md code, the md code won't build.
> > 
> > More seriously, most of this code has nothing to do with buffers.  It
> > uses page->private for its own purposes.
> > 
> > What I would do instead is add:
> > 
> > clear_page_private(struct page *page)
> > {
> > 	ClearPagePrivate(page);
> > 	set_page_private(page, 0);
> > 	put_page(page);
> > }
> > 
> > to include/linux/mm.h, then convert all callers of __clear_page_buffers()
> > to call that instead.
> 
> While I think this is the right direction, I don't like the lack of
> symmetry between set_page_private() and clear_page_private() this
> creates.  i.e. set_page_private() just assigned page->private, while
> clear_page_private clears both a page flag and page->private, and it
> also drops a page reference, too.
> 
> Anyone expecting to use set/clear_page_private as a matched pair (as
> the names suggest they are) is in for a horrible surprise...
> 
> This is a public service message brought to you by the Department
> of We Really Suck At API Design.

Oh, blast.  I hadn't noticed that.  And we're horribly inconsistent
with how we use set_page_private() too -- rb_alloc_aux_page() doesn't
increment the page's refcount, for example.

So, new (pair of) names:

set_fs_page_private()
clear_fs_page_private()

since it really seems like it's only page cache pages which need to
follow the rules about setting PagePrivate and incrementing the refcount.
Also, I think I'd like to see them take/return a void *:

void *set_fs_page_private(struct page *page, void *data)
{
	get_page(page);
	set_page_private(page, (unsigned long)data);
	SetPagePrivate(page);
	return data;
}

void *clear_fs_page_private(struct page *page)
{
	void *data = (void *)page_private(page);

	if (!PagePrivate(page))
		return NULL;
	ClearPagePrivate(page);
	set_page_private(page, 0);
	put_page(page);
	return data;
}

That makes iomap simpler:

 static void
 iomap_page_release(struct page *page)
 {
-	struct iomap_page *iop = to_iomap_page(page);
+	struct iomap_page *iop = clear_fs_page_private(page);

 	if (!iop)
 		return;
 	WARN_ON_ONCE(atomic_read(&iop->read_count));
 	WARN_ON_ONCE(atomic_read(&iop->write_count));
-	ClearPagePrivate(page);
-	set_page_private(page, 0);
-	put_page(page);
 	kfree(iop);
 }