[0/5] export __clear_page_buffers to cleanup code
mbox series

Message ID 20200418225123.31850-1-guoqing.jiang@cloud.ionos.com
Headers show
Series
  • export __clear_page_buffers to cleanup code
Related show

Message

Guoqing Jiang April 18, 2020, 10:51 p.m. UTC
Hi,

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.

Thanks,
Guoqing

Guoqing Jiang (5):
  fs/buffer: export __clear_page_buffers
  btrfs: call __clear_page_buffers to simplify code
  iomap: call __clear_page_buffers in iomap_page_release
  orangefs: call __clear_page_buffers to simplify code
  md-bitmap: don't duplicate code for __clear_page_buffers

 drivers/md/md-bitmap.c      |  8 --------
 fs/btrfs/disk-io.c          |  5 ++---
 fs/btrfs/extent_io.c        |  6 ++----
 fs/btrfs/inode.c            | 14 ++++----------
 fs/buffer.c                 |  4 ++--
 fs/iomap/buffered-io.c      |  4 +---
 fs/orangefs/inode.c         | 17 +++++------------
 include/linux/buffer_head.h |  1 +
 8 files changed, 17 insertions(+), 42 deletions(-)

Comments

Matthew Wilcox April 19, 2020, 3:14 a.m. UTC | #1
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.
Gao Xiang April 19, 2020, 5:14 a.m. UTC | #2
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.

Agreed with the new naming (__clear_page_buffers is confusing), that is not
only for initial use buffer head, but a generic convention for all unlocked
PagePrivate pages (such migration & reclaim paths indicate that).

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mm.h?h=v5.7-rc1#n990

Thanks,
Gao Xiang


>
Guoqing Jiang April 19, 2020, 1:15 p.m. UTC | #3
On 19.04.20 05:14, 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.

Thank for reminder, I missed that.

> 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.
>

Thanks for your suggestion!

Thanks,
Guoqing
Guoqing Jiang April 19, 2020, 1:22 p.m. UTC | #4
On 19.04.20 07:14, Gao Xiang 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.
> Agreed with the new naming (__clear_page_buffers is confusing), that is not
> only for initial use buffer head, but a generic convention for all unlocked
> PagePrivate pages (such migration & reclaim paths indicate that).
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mm.h?h=v5.7-rc1#n990

Thanks for the link, and will rename the function to clear_page_private.

Thanks,
Guoqing
Guoqing Jiang April 19, 2020, 8:31 p.m. UTC | #5
Hi Mattew,

On 19.04.20 05:14, 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.

Seems the build option BLK_DEV_MD is depended on BLOCK, and buffer.c
is relied on the same option.

ifeq ($(CONFIG_BLOCK),y)/x
obj-y +=        buffer.o block_dev.o direct-io.o mpage.o
else
obj-y +=        no-block.o
endif

So I am not sure it is necessary to move the function to include/linux/mm.h
if there is no sequencing problem, thanks for your any further suggestion.

Thanks,
Guoqing
Matthew Wilcox April 19, 2020, 9:17 p.m. UTC | #6
On Sun, Apr 19, 2020 at 10:31:26PM +0200, Guoqing Jiang wrote:
> On 19.04.20 05:14, 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.
> 
> Seems the build option BLK_DEV_MD is depended on BLOCK, and buffer.c
> is relied on the same option.
> 
> ifeq ($(CONFIG_BLOCK),y)/x
> obj-y +=        buffer.o block_dev.o direct-io.o mpage.o
> else
> obj-y +=        no-block.o
> endif
> 
> So I am not sure it is necessary to move the function to include/linux/mm.h
> if there is no sequencing problem, thanks for your any further suggestion.

The sequencing problem is that there will be _two_ definitions of
__clear_page_buffers().

The reason it should go in mm.h is that it's a very simple function
and it will be better to inline it than call an external function.
The two things are not related to each other.
Dave Chinner April 19, 2020, 11:20 p.m. UTC | #7
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.
Matthew Wilcox April 20, 2020, 12:30 a.m. UTC | #8
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);
 }
Guoqing Jiang April 20, 2020, 9:14 p.m. UTC | #9
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
Matthew Wilcox April 20, 2020, 10:14 p.m. UTC | #10
On Mon, Apr 20, 2020 at 11:14:35PM +0200, Guoqing Jiang wrote:
> On 20.04.20 02:30, Matthew Wilcox wrote:
> > On Mon, Apr 20, 2020 at 09:20:46AM +1000, Dave Chinner wrote:
> > > 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!
> 
> > 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).

No.  Changing the semantics of set_page_private() without changing the
function signature is bad because it makes patches silently break when
applied to trees on either side of the change.  So we need a new name.

> 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*.

perhaps it should be in pagemap.h since it's for pagecache pages.

> > 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.

Yes.

> 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.

That's kind of the way things go ... you start pulling on one thread
and all of a sudden, you're weaving a new coat ;-)
Dave Chinner April 21, 2020, 1:53 a.m. UTC | #11
On Sun, Apr 19, 2020 at 05:30:25PM -0700, 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...
> > 
> > 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()

Except set_fs() is already used for something else entirely, so now
we have the same prefix having completely different meanings.

Naming is hard.

Realistically, we want these interfaces for use cases where we have
an external object we store in page->private, right? That's an
unsigned long, not a pointer, so we have to cast page->private for
this sort of use. So, yes, I'd definitely like to see:

> 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 *:

This sort of API cleanup.

> 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;
> }

I think we need a page_private() variant that returns a void *
rather than having to cast it everywhere we directly access the
private pointer. i.e. The API becomes:

page_get_private_ptr()		- replaces page_private()
page_set_private_ptr()		- replaces set_page_private()
page_clear_private_ptr()	- replaces clear_page_private()

> 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);
>  }

*nod*

Much nicers :)

Cheers,

Dave.