On 20.04.20 02:30, Matthew Wilcox wrote:
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...
Dave, thanks for the valuable suggestion!
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()
Hmm, maybe it is better to keep the original name (set/clear_page_private).
Because,
1. it would be weird for other subsystems (not belong to fs scope) to
call the
function which is supposed to be used in fs, though we can add a wrapper
for other users out of fs.
2. no function in mm.h is named like *fs*.
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;
}
Seems some functions could probably use the above helper, such as
iomap_page_create, iomap_migrate_page, get_page_bootmem and
f2fs_set_page_private etc.
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);
}
Really appreciate for your input though the thing is a little beyond my
original intention ;-), will try to send a new version after reading more
fs code.
Best Regards,
Guoqing
