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.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
