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

[Guoqing Jiang <guoqing.jiang@cloud.ionos.com>]

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