[PATCH 0/5] export __clear_page_buffers to cleanup code

[PATCH 0/5] export __clear_page_buffers to cleanup code

[Guoqing Jiang]

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(-)

-- 
2.17.1

[PATCH 1/5] fs/buffer: export __clear_page_buffers

[Guoqing Jiang]

Export the function so others (such as md, btrfs and iomap) can reuse it.

Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 fs/buffer.c                 | 4 ++--
 include/linux/buffer_head.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index f73276d746bb..05b7489d9aa3 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -123,13 +123,13 @@ void __wait_on_buffer(struct buffer_head * bh)
 }
 EXPORT_SYMBOL(__wait_on_buffer);
 
-static void
-__clear_page_buffers(struct page *page)
+void __clear_page_buffers(struct page *page)
 {
 	ClearPagePrivate(page);
 	set_page_private(page, 0);
 	put_page(page);
 }
+EXPORT_SYMBOL(__clear_page_buffers);
 
 static void buffer_io_error(struct buffer_head *bh, char *msg)
 {
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index e0b020eaf32e..735b094d89e1 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -181,6 +181,7 @@ static inline void clean_bdev_bh_alias(struct buffer_head *bh)
 
 void mark_buffer_async_write(struct buffer_head *bh);
 void __wait_on_buffer(struct buffer_head *);
+void __clear_page_buffers(struct page *page);
 wait_queue_head_t *bh_waitq_head(struct buffer_head *bh);
 struct buffer_head *__find_get_block(struct block_device *bdev, sector_t block,
 			unsigned size);
-- 
2.17.1

Re: [PATCH 1/5] fs/buffer: export __clear_page_buffers

[Nikolay Borisov]

On 19.04.20 г. 1:51 ч., Guoqing Jiang wrote:
> Export the function so others (such as md, btrfs and iomap) can reuse it.
> 
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>  fs/buffer.c                 | 4 ++--
>  include/linux/buffer_head.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index f73276d746bb..05b7489d9aa3 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -123,13 +123,13 @@ void __wait_on_buffer(struct buffer_head * bh)
>  }
>  EXPORT_SYMBOL(__wait_on_buffer);
>  
> -static void
> -__clear_page_buffers(struct page *page)
> +void __clear_page_buffers(struct page *page)
>  {
>  	ClearPagePrivate(page);
>  	set_page_private(page, 0);
>  	put_page(page);
>  }
> +EXPORT_SYMBOL(__clear_page_buffers);

Since this is being exported there is no reason to have __ prefix.
Rename the function as well, also why are you exporting it EXPORT_SYMBOL
and not EXPORT_SYMBOL_GPL just for the sake of consistency with other
functions in this file or some other reason?

>  
>  static void buffer_io_error(struct buffer_head *bh, char *msg)
>  {
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index e0b020eaf32e..735b094d89e1 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -181,6 +181,7 @@ static inline void clean_bdev_bh_alias(struct buffer_head *bh)
>  
>  void mark_buffer_async_write(struct buffer_head *bh);
>  void __wait_on_buffer(struct buffer_head *);
> +void __clear_page_buffers(struct page *page);
>  wait_queue_head_t *bh_waitq_head(struct buffer_head *bh);
>  struct buffer_head *__find_get_block(struct block_device *bdev, sector_t block,
>  			unsigned size);
> 

Re: [PATCH 1/5] fs/buffer: export __clear_page_buffers

[Guoqing Jiang]

On 19.04.20 09:56, Nikolay Borisov wrote:
>
> On 19.04.20 г. 1:51 ч., Guoqing Jiang wrote:
>> Export the function so others (such as md, btrfs and iomap) can reuse it.
>>
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>> ---
>>   fs/buffer.c                 | 4 ++--
>>   include/linux/buffer_head.h | 1 +
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/buffer.c b/fs/buffer.c
>> index f73276d746bb..05b7489d9aa3 100644
>> --- a/fs/buffer.c
>> +++ b/fs/buffer.c
>> @@ -123,13 +123,13 @@ void __wait_on_buffer(struct buffer_head * bh)
>>   }
>>   EXPORT_SYMBOL(__wait_on_buffer);
>>   
>> -static void
>> -__clear_page_buffers(struct page *page)
>> +void __clear_page_buffers(struct page *page)
>>   {
>>   	ClearPagePrivate(page);
>>   	set_page_private(page, 0);
>>   	put_page(page);
>>   }
>> +EXPORT_SYMBOL(__clear_page_buffers);
> Since this is being exported there is no reason to have __ prefix.
> Rename the function as well,

Yes, it need to be renamed.

>   also why are you exporting it EXPORT_SYMBOL
> and not EXPORT_SYMBOL_GPL just for the sake of consistency with other
> functions in this file or some other reason?

Because I followed EXPORT_SYMBOL(__wait_on_buffer).

Thanks,
Guoqing

[PATCH 2/5] btrfs: call __clear_page_buffers to simplify code

[Guoqing Jiang]

Some places can be replaced with __clear_page_buffers after the function
is exported.

Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 fs/btrfs/disk-io.c   |  5 ++---
 fs/btrfs/extent_io.c |  6 ++----
 fs/btrfs/inode.c     | 14 ++++----------
 3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index a6cb5cbbdb9f..0f1e5690e8a4 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -17,6 +17,7 @@
 #include <linux/error-injection.h>
 #include <linux/crc32c.h>
 #include <linux/sched/mm.h>
+#include <linux/buffer_head.h>
 #include <asm/unaligned.h>
 #include <crypto/hash.h>
 #include "ctree.h"
@@ -980,9 +981,7 @@ static void btree_invalidatepage(struct page *page, unsigned int offset,
 		btrfs_warn(BTRFS_I(page->mapping->host)->root->fs_info,
 			   "page private not zero on page %llu",
 			   (unsigned long long)page_offset(page));
-		ClearPagePrivate(page);
-		set_page_private(page, 0);
-		put_page(page);
+		__clear_page_buffers(page);
 	}
 }
 
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 39e45b8a5031..317a1cdc7d3e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -13,6 +13,7 @@
 #include <linux/pagevec.h>
 #include <linux/prefetch.h>
 #include <linux/cleancache.h>
+#include <linux/buffer_head.h>
 #include "extent_io.h"
 #include "extent-io-tree.h"
 #include "extent_map.h"
@@ -4929,10 +4930,7 @@ static void btrfs_release_extent_buffer_pages(struct extent_buffer *eb)
 			 * We need to make sure we haven't be attached
 			 * to a new eb.
 			 */
-			ClearPagePrivate(page);
-			set_page_private(page, 0);
-			/* One for the page private */
-			put_page(page);
+			__clear_page_buffers(page);
 		}
 
 		if (mapped)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 320d1062068d..95886202c74f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8303,11 +8303,8 @@ btrfs_readpages(struct file *file, struct address_space *mapping,
 static int __btrfs_releasepage(struct page *page, gfp_t gfp_flags)
 {
 	int ret = try_release_extent_mapping(page, gfp_flags);
-	if (ret == 1) {
-		ClearPagePrivate(page);
-		set_page_private(page, 0);
-		put_page(page);
-	}
+	if (ret == 1)
+		__clear_page_buffers(page);
 	return ret;
 }
 
@@ -8458,11 +8455,8 @@ static void btrfs_invalidatepage(struct page *page, unsigned int offset,
 	}
 
 	ClearPageChecked(page);
-	if (PagePrivate(page)) {
-		ClearPagePrivate(page);
-		set_page_private(page, 0);
-		put_page(page);
-	}
+	if (PagePrivate(page))
+		__clear_page_buffers(page);
 }
 
 /*
-- 
2.17.1

Re: [PATCH 2/5] btrfs: call __clear_page_buffers to simplify code

[David Sterba]

On Sun, Apr 19, 2020 at 12:51:20AM +0200, Guoqing Jiang wrote:
> Some places can be replaced with __clear_page_buffers after the function
> is exported.
> 
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Cc: David Sterba <dsterba@suse.com>
> Cc: linux-btrfs@vger.kernel.org
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>  fs/btrfs/disk-io.c   |  5 ++---
>  fs/btrfs/extent_io.c |  6 ++----
>  fs/btrfs/inode.c     | 14 ++++----------
>  3 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index a6cb5cbbdb9f..0f1e5690e8a4 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -17,6 +17,7 @@
>  #include <linux/error-injection.h>
>  #include <linux/crc32c.h>
>  #include <linux/sched/mm.h>
> +#include <linux/buffer_head.h>

I'm not really thrilled to see buffer_head.h being added back, we're on
the track to remove buffer_head usage completely and adding it just for
one helper does not seem great to me.

Re: [PATCH 2/5] btrfs: call __clear_page_buffers to simplify code

[Guoqing Jiang]

On 19.04.20 21:46, David Sterba wrote:
> On Sun, Apr 19, 2020 at 12:51:20AM +0200, Guoqing Jiang wrote:
>> Some places can be replaced with __clear_page_buffers after the function
>> is exported.
>>
>> Cc: Chris Mason <clm@fb.com>
>> Cc: Josef Bacik <josef@toxicpanda.com>
>> Cc: David Sterba <dsterba@suse.com>
>> Cc: linux-btrfs@vger.kernel.org
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>> ---
>>   fs/btrfs/disk-io.c   |  5 ++---
>>   fs/btrfs/extent_io.c |  6 ++----
>>   fs/btrfs/inode.c     | 14 ++++----------
>>   3 files changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index a6cb5cbbdb9f..0f1e5690e8a4 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/error-injection.h>
>>   #include <linux/crc32c.h>
>>   #include <linux/sched/mm.h>
>> +#include <linux/buffer_head.h>
> I'm not really thrilled to see buffer_head.h being added back, we're on
> the track to remove buffer_head usage completely and adding it just for
> one helper does not seem great to me.

Thanks for your reply, will drop this one.

Thanks,
Guoqing

[PATCH 3/5] iomap: call __clear_page_buffers in iomap_page_release

[Guoqing Jiang]

After the helper is exported, we can call it to simplify code a little.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 fs/iomap/buffered-io.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 89e21961d1ad..b06568ad9a7a 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -74,9 +74,7 @@ iomap_page_release(struct page *page)
 		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);
+	__clear_page_buffers(page);
 	kfree(iop);
 }
 
-- 
2.17.1

Re: [PATCH 3/5] iomap: call __clear_page_buffers in iomap_page_release

[Christoph Hellwig]

On Sun, Apr 19, 2020 at 12:51:21AM +0200, Guoqing Jiang wrote:
> After the helper is exported, we can call it to simplify code a little.
> 
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: linux-xfs@vger.kernel.org
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> ---
>  fs/iomap/buffered-io.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 89e21961d1ad..b06568ad9a7a 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -74,9 +74,7 @@ iomap_page_release(struct page *page)
>  		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);
> +	__clear_page_buffers(page);

We should not call a helper mentioning buffers in code that has
nothing to do with buffers.  If you want to us __clear_page_buffers
more widely please give it a better name first.

You'll also forgot to Cc me on the other patches in the series, which is
a completel no-go as it doesn't allow for a proper review.

Re: [PATCH 3/5] iomap: call __clear_page_buffers in iomap_page_release

[Guoqing Jiang]

On 19.04.20 09:47, Christoph Hellwig wrote:
> On Sun, Apr 19, 2020 at 12:51:21AM +0200, Guoqing Jiang wrote:
>> After the helper is exported, we can call it to simplify code a little.
>>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Cc: Darrick J. Wong <darrick.wong@oracle.com>
>> Cc: linux-xfs@vger.kernel.org
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
>> ---
>>   fs/iomap/buffered-io.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
>> index 89e21961d1ad..b06568ad9a7a 100644
>> --- a/fs/iomap/buffered-io.c
>> +++ b/fs/iomap/buffered-io.c
>> @@ -74,9 +74,7 @@ iomap_page_release(struct page *page)
>>   		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);
>> +	__clear_page_buffers(page);
> We should not call a helper mentioning buffers in code that has
> nothing to do with buffers.  If you want to us __clear_page_buffers
> more widely please give it a better name first.

Yes, I will rename it to clear_page_private as Matthew suggested.

> You'll also forgot to Cc me on the other patches in the series, which is
> a completel no-go as it doesn't allow for a proper review.

Sorry, will cc you with the series next time.

Thanks,
Guoqing

[RFC PATCH 4/5] orangefs: call __clear_page_buffers to simplify code

[Guoqing Jiang]

Since __clear_page_buffers is exported, we can call __clear_page_buffers
to simplify code in the four places.

Cc: Mike Marshall <hubcap@omnibond.com>
Cc: Martin Brandenburg <martin@omnibond.com>
Cc: devel@lists.orangefs.org
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
The order for set_page_private and ClearPagePrivate is swapped in
__clear_page_buffers, not sure it is identical or not, so this is
RFC.

Thanks,
Guoqing

 fs/orangefs/inode.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index 12ae630fbed7..8e1591d8bf24 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/bvec.h>
+#include <linux/buffer_head.h>
 #include "protocol.h"
 #include "orangefs-kernel.h"
 #include "orangefs-bufmap.h"
@@ -64,9 +65,7 @@ static int orangefs_writepage_locked(struct page *page,
 	}
 	if (wr) {
 		kfree(wr);
-		set_page_private(page, 0);
-		ClearPagePrivate(page);
-		put_page(page);
+		__clear_page_buffers(page);
 	}
 	return ret;
 }
@@ -460,17 +459,13 @@ static void orangefs_invalidatepage(struct page *page,
 
 	if (offset == 0 && length == PAGE_SIZE) {
 		kfree((struct orangefs_write_range *)page_private(page));
-		set_page_private(page, 0);
-		ClearPagePrivate(page);
-		put_page(page);
+		__clear_page_buffers(page);
 		return;
 	/* write range entirely within invalidate range (or equal) */
 	} else if (page_offset(page) + offset <= wr->pos &&
 	    wr->pos + wr->len <= page_offset(page) + offset + length) {
 		kfree((struct orangefs_write_range *)page_private(page));
-		set_page_private(page, 0);
-		ClearPagePrivate(page);
-		put_page(page);
+		__clear_page_buffers(page);
 		/* XXX is this right? only caller in fs */
 		cancel_dirty_page(page);
 		return;
@@ -537,9 +532,7 @@ static void orangefs_freepage(struct page *page)
 {
 	if (PagePrivate(page)) {
 		kfree((struct orangefs_write_range *)page_private(page));
-		set_page_private(page, 0);
-		ClearPagePrivate(page);
-		put_page(page);
+		__clear_page_buffers(page);
 	}
 }
 
-- 
2.17.1

[PATCH 5/5] md-bitmap: don't duplicate code for __clear_page_buffers

[Guoqing Jiang]

After __clear_page_buffers is exported, we can use it directly instead of
copy the implementation.

Cc: Song Liu <song@kernel.org>
Cc: linux-raid@vger.kernel.org
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 drivers/md/md-bitmap.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index b952bd45bd6a..a1464417ada6 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -324,14 +324,6 @@ static void end_bitmap_write(struct buffer_head *bh, int uptodate)
 		wake_up(&bitmap->write_wait);
 }
 
-/* copied from buffer.c */
-static void
-__clear_page_buffers(struct page *page)
-{
-	ClearPagePrivate(page);
-	set_page_private(page, 0);
-	put_page(page);
-}
 static void free_buffers(struct page *page)
 {
 	struct buffer_head *bh;
-- 
2.17.1

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

[Matthew Wilcox]

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.

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

[Gao Xiang]

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


> 

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

[Guoqing Jiang]

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

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

[Guoqing Jiang]

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

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

[Guoqing Jiang]

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

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

[Matthew Wilcox]

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.

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

[Dave Chinner]

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@fromorbit.com

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

[Matthew Wilcox]

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

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

[Guoqing Jiang]

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

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

[Matthew Wilcox]

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

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

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com