[RFC PATCH 0/9] Introduce set/clear_fs_page_private to cleanup code

[RFC PATCH 0/9] Introduce set/clear_fs_page_private to cleanup code

[Guoqing Jiang]

Hi,

Based on the previous thread [1], this patchset introduces set_fs_page_private
and clear_fs_page_private to replace attach_page_buffers and __clear_page_buffers.
Thanks a lot for the constructive suggestion from Matthew and Dave.

And sorry for cross post to different lists since it modifies different subsystems.

[1]. https://lore.kernel.org/linux-fsdevel/20200420221424.GH5820@bombadil.infradead.org/

Thanks,
Guoqing

Guoqing Jiang (9):
  include/linux/pagemap.h: introduce set/clear_fs_page_private
  md: remove __clear_page_buffers and use set/clear_fs_page_private
  btrfs: use set/clear_fs_page_private
  fs/buffer.c: use set/clear_fs_page_private
  f2fs: use set/clear_fs_page_private
  iomap: use set/clear_fs_page_private
  ntfs: replace attach_page_buffers with set_fs_page_private
  orangefs: use set/clear_fs_page_private
  buffer_head.h: remove attach_page_buffers

 drivers/md/md-bitmap.c      | 12 ++----------
 fs/btrfs/disk-io.c          |  4 +---
 fs/btrfs/extent_io.c        | 21 ++++++---------------
 fs/btrfs/inode.c            | 17 ++++-------------
 fs/buffer.c                 | 16 ++++------------
 fs/f2fs/f2fs.h              | 11 ++---------
 fs/iomap/buffered-io.c      | 14 +++-----------
 fs/ntfs/aops.c              |  2 +-
 fs/ntfs/mft.c               |  2 +-
 fs/orangefs/inode.c         | 24 ++++++------------------
 include/linux/buffer_head.h |  8 --------
 include/linux/pagemap.h     | 22 ++++++++++++++++++++++
 12 files changed, 52 insertions(+), 101 deletions(-)

-- 
2.17.1

[RFC PATCH 1/9] include/linux/pagemap.h: introduce set/clear_fs_page_private

[Guoqing Jiang]

The logic in attach_page_buffers and  __clear_page_buffers are quite
paired, but

1. they are located in different files.

2. attach_page_buffers is implemented in buffer_head.h, so it could be
   used by other files. But __clear_page_buffers is static function in
   buffer.c and other potential users can't call the function, md-bitmap
   even copied the function.

So, introduce the new set/clear_fs_page_private to replace them since
they are mostly used in fs layer. With the new pair of function, we will
remove the usage of attach_page_buffers and  __clear_page_buffers in
next patches.

Suggested-by: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: William Kucharski <william.kucharski@oracle.com> 
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Yafang Shao <laoar.shao@gmail.com>
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 include/linux/pagemap.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index a8f7bd8ea1c6..2a165ea647fe 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -205,6 +205,28 @@ static inline int page_cache_add_speculative(struct page *page, int count)
 	return __page_cache_add_speculative(page, count);
 }
 
+static inline void *set_fs_page_private(struct page *page, void *data)
+{
+	get_page(page);
+	set_page_private(page, (unsigned long)data);
+	SetPagePrivate(page);
+
+	return data;
+}
+
+static inline 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;
+}
+
 #ifdef CONFIG_NUMA
 extern struct page *__page_cache_alloc(gfp_t gfp);
 #else
-- 
2.17.1

[RFC PATCH 2/9] md: remove __clear_page_buffers and use set/clear_fs_page_private

[Guoqing Jiang]

After introduce set/clear_fs_page_private in pagemap.h, we can remove
the duplicat code and call the new functions.

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 | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index b952bd45bd6a..b935473c82b7 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;
@@ -345,7 +337,7 @@ static void free_buffers(struct page *page)
 		free_buffer_head(bh);
 		bh = next;
 	}
-	__clear_page_buffers(page);
+	clear_fs_page_private(page);
 	put_page(page);
 }
 
@@ -374,7 +366,7 @@ static int read_page(struct file *file, unsigned long index,
 		ret = -ENOMEM;
 		goto out;
 	}
-	attach_page_buffers(page, bh);
+	set_fs_page_private(page, bh);
 	blk_cur = index << (PAGE_SHIFT - inode->i_blkbits);
 	while (bh) {
 		block = blk_cur;
-- 
2.17.1

[RFC PATCH 3/9] btrfs: use set/clear_fs_page_private

[Guoqing Jiang]

Since the new pair function is introduced, we can call them to clean the
code in btrfs.

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   |  4 +---
 fs/btrfs/extent_io.c | 21 ++++++---------------
 fs/btrfs/inode.c     | 17 ++++-------------
 3 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index a6cb5cbbdb9f..1230863e80f9 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -980,9 +980,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_fs_page_private(page);
 	}
 }
 
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 39e45b8a5031..de8c2d5a99db 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3076,22 +3076,16 @@ static int submit_extent_page(unsigned int opf,
 static void attach_extent_buffer_page(struct extent_buffer *eb,
 				      struct page *page)
 {
-	if (!PagePrivate(page)) {
-		SetPagePrivate(page);
-		get_page(page);
-		set_page_private(page, (unsigned long)eb);
-	} else {
+	if (!PagePrivate(page))
+		set_fs_page_private(page, eb);
+	else
 		WARN_ON(page->private != (unsigned long)eb);
-	}
 }
 
 void set_page_extent_mapped(struct page *page)
 {
-	if (!PagePrivate(page)) {
-		SetPagePrivate(page);
-		get_page(page);
-		set_page_private(page, EXTENT_PAGE_PRIVATE);
-	}
+	if (!PagePrivate(page))
+		set_fs_page_private(page, (void *)EXTENT_PAGE_PRIVATE);
 }
 
 static struct extent_map *
@@ -4929,10 +4923,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_fs_page_private(page);
 		}
 
 		if (mapped)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 320d1062068d..07871c57ba96 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_fs_page_private(page);
 	return ret;
 }
 
@@ -8331,11 +8328,9 @@ static int btrfs_migratepage(struct address_space *mapping,
 
 	if (page_has_private(page)) {
 		ClearPagePrivate(page);
-		get_page(newpage);
-		set_page_private(newpage, page_private(page));
+		set_fs_page_private(newpage, (void *)page_private(page));
 		set_page_private(page, 0);
 		put_page(page);
-		SetPagePrivate(newpage);
 	}
 
 	if (PagePrivate2(page)) {
@@ -8458,11 +8453,7 @@ 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);
-	}
+	clear_fs_page_private(page);
 }
 
 /*
-- 
2.17.1

[RFC PATCH 4/9] fs/buffer.c: use set/clear_fs_page_private

[Guoqing Jiang]

Since the new pair function is introduced, we can call them to clean the
code in buffer.c.

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

diff --git a/fs/buffer.c b/fs/buffer.c
index a60f60396cfa..1d9b32e77c2b 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -123,14 +123,6 @@ void __wait_on_buffer(struct buffer_head * bh)
 }
 EXPORT_SYMBOL(__wait_on_buffer);
 
-static void
-__clear_page_buffers(struct page *page)
-{
-	ClearPagePrivate(page);
-	set_page_private(page, 0);
-	put_page(page);
-}
-
 static void buffer_io_error(struct buffer_head *bh, char *msg)
 {
 	if (!test_bit(BH_Quiet, &bh->b_state))
@@ -906,7 +898,7 @@ link_dev_buffers(struct page *page, struct buffer_head *head)
 		bh = bh->b_this_page;
 	} while (bh);
 	tail->b_this_page = head;
-	attach_page_buffers(page, head);
+	set_fs_page_private(page, head);
 }
 
 static sector_t blkdev_max_block(struct block_device *bdev, unsigned int size)
@@ -1580,7 +1572,7 @@ void create_empty_buffers(struct page *page,
 			bh = bh->b_this_page;
 		} while (bh != head);
 	}
-	attach_page_buffers(page, head);
+	set_fs_page_private(page, head);
 	spin_unlock(&page->mapping->private_lock);
 }
 EXPORT_SYMBOL(create_empty_buffers);
@@ -2567,7 +2559,7 @@ static void attach_nobh_buffers(struct page *page, struct buffer_head *head)
 			bh->b_this_page = head;
 		bh = bh->b_this_page;
 	} while (bh != head);
-	attach_page_buffers(page, head);
+	set_fs_page_private(page, head);
 	spin_unlock(&page->mapping->private_lock);
 }
 
@@ -3227,7 +3219,7 @@ drop_buffers(struct page *page, struct buffer_head **buffers_to_free)
 		bh = next;
 	} while (bh != head);
 	*buffers_to_free = head;
-	__clear_page_buffers(page);
+	clear_fs_page_private(page);
 	return 1;
 failed:
 	return 0;
-- 
2.17.1

[RFC PATCH 5/9] f2fs: use set/clear_fs_page_private

[Guoqing Jiang]

Since the new pair function is introduced, we can call them to clean the
code in f2fs.h.

Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Chao Yu <chao@kernel.org>
Cc: linux-f2fs-devel@lists.sourceforge.net
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 fs/f2fs/f2fs.h | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index ba470d5687fe..70ad8c51e0fc 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3051,19 +3051,12 @@ static inline void f2fs_set_page_private(struct page *page,
 	if (PagePrivate(page))
 		return;
 
-	get_page(page);
-	SetPagePrivate(page);
-	set_page_private(page, data);
+	set_fs_page_private(page, (void *)data);
 }
 
 static inline void f2fs_clear_page_private(struct page *page)
 {
-	if (!PagePrivate(page))
-		return;
-
-	set_page_private(page, 0);
-	ClearPagePrivate(page);
-	f2fs_put_page(page, 0);
+	clear_fs_page_private(page);
 }
 
 /*
-- 
2.17.1

[RFC PATCH 6/9] iomap: use set/clear_fs_page_private

[Guoqing Jiang]

Since the new pair function is introduced, we can call them to clean the
code in iomap.

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 | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 89e21961d1ad..cc48bf4f1193 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -59,24 +59,18 @@ iomap_page_create(struct inode *inode, struct page *page)
 	 * migrate_page_move_mapping() assumes that pages with private data have
 	 * their count elevated by 1.
 	 */
-	get_page(page);
-	set_page_private(page, (unsigned long)iop);
-	SetPagePrivate(page);
-	return iop;
+	return (struct iomap_page *)set_fs_page_private(page, iop);
 }
 
 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);
 }
 
@@ -556,11 +550,9 @@ iomap_migrate_page(struct address_space *mapping, struct page *newpage,
 
 	if (page_has_private(page)) {
 		ClearPagePrivate(page);
-		get_page(newpage);
-		set_page_private(newpage, page_private(page));
+		set_fs_page_private(newpage, (void *)page_private(page));
 		set_page_private(page, 0);
 		put_page(page);
-		SetPagePrivate(newpage);
 	}
 
 	if (mode != MIGRATE_SYNC_NO_COPY)
-- 
2.17.1

[RFC PATCH 7/9] ntfs: replace attach_page_buffers with set_fs_page_private

[Guoqing Jiang]

Call the new function since attach_page_buffers will be removed.

Cc: Anton Altaparmakov <anton@tuxera.com>
Cc: linux-ntfs-dev@lists.sourceforge.net
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 fs/ntfs/aops.c | 2 +-
 fs/ntfs/mft.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
index 554b744f41bf..d8c60e5793c0 100644
--- a/fs/ntfs/aops.c
+++ b/fs/ntfs/aops.c
@@ -1732,7 +1732,7 @@ void mark_ntfs_record_dirty(struct page *page, const unsigned int ofs) {
 				bh = bh->b_this_page;
 			} while (bh);
 			tail->b_this_page = head;
-			attach_page_buffers(page, head);
+			set_fs_page_private(page, head);
 		} else
 			buffers_to_free = bh;
 	}
diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index 3aac5c917afe..e61cb280aee0 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -504,7 +504,7 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned long mft_no,
 			bh = bh->b_this_page;
 		} while (bh);
 		tail->b_this_page = head;
-		attach_page_buffers(page, head);
+		set_fs_page_private(page, head);
 	}
 	bh = head = page_buffers(page);
 	BUG_ON(!bh);
-- 
2.17.1

[RFC PATCH 8/9] orangefs: use set/clear_fs_page_private

[Guoqing Jiang]

Since the new pair function is introduced, we can call them to clean the
code in orangefs.

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>
---
 fs/orangefs/inode.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index 12ae630fbed7..893099d36e20 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -64,9 +64,7 @@ static int orangefs_writepage_locked(struct page *page,
 	}
 	if (wr) {
 		kfree(wr);
-		set_page_private(page, 0);
-		ClearPagePrivate(page);
-		put_page(page);
+		clear_fs_page_private(page);
 	}
 	return ret;
 }
@@ -409,9 +407,7 @@ static int orangefs_write_begin(struct file *file,
 	wr->len = len;
 	wr->uid = current_fsuid();
 	wr->gid = current_fsgid();
-	SetPagePrivate(page);
-	set_page_private(page, (unsigned long)wr);
-	get_page(page);
+	set_fs_page_private(page, wr);
 okay:
 	return 0;
 }
@@ -460,17 +456,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_fs_page_private(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_fs_page_private(page);
 		/* XXX is this right? only caller in fs */
 		cancel_dirty_page(page);
 		return;
@@ -537,9 +529,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_fs_page_private(page);
 	}
 }
 
@@ -740,9 +730,7 @@ vm_fault_t orangefs_page_mkwrite(struct vm_fault *vmf)
 	wr->len = PAGE_SIZE;
 	wr->uid = current_fsuid();
 	wr->gid = current_fsgid();
-	SetPagePrivate(page);
-	set_page_private(page, (unsigned long)wr);
-	get_page(page);
+	set_fs_page_private(page, wr);
 okay:
 
 	file_update_time(vmf->vma->vm_file);
-- 
2.17.1

[RFC PATCH 9/9] buffer_head.h: remove attach_page_buffers

[Guoqing Jiang]

All the callers have replaced attach_page_buffers with the new function
set_fs_page_private, so remove it.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Roman Gushchin <guro@fb.com>
Cc: Andreas Dilger <adilger@dilger.ca>
Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
---
 include/linux/buffer_head.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 15b765a181b8..22fb11e2d2e0 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -272,14 +272,6 @@ void buffer_init(void);
  * inline definitions
  */
 
-static inline void attach_page_buffers(struct page *page,
-		struct buffer_head *head)
-{
-	get_page(page);
-	SetPagePrivate(page);
-	set_page_private(page, (unsigned long)head);
-}
-
 static inline void get_bh(struct buffer_head *bh)
 {
         atomic_inc(&bh->b_count);
-- 
2.17.1

Re: [RFC PATCH 3/9] btrfs: use set/clear_fs_page_private

[Dave Chinner]

On Sun, Apr 26, 2020 at 11:49:19PM +0200, Guoqing Jiang wrote:
> Since the new pair function is introduced, we can call them to clean the
> code in btrfs.
> 
> 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>

....

>  void set_page_extent_mapped(struct page *page)
>  {
> -	if (!PagePrivate(page)) {
> -		SetPagePrivate(page);
> -		get_page(page);
> -		set_page_private(page, EXTENT_PAGE_PRIVATE);
> -	}
> +	if (!PagePrivate(page))
> +		set_fs_page_private(page, (void *)EXTENT_PAGE_PRIVATE);

Change the definition of EXTENT_PAGE_PRIVATE so the cast is not
needed? Nothing ever reads EXTENT_PAGE_PRIVATE; it's only there to
set the private flag for other code to check and release the extent
mapping reference to the page...

> @@ -8331,11 +8328,9 @@ static int btrfs_migratepage(struct address_space *mapping,
>  
>  	if (page_has_private(page)) {
>  		ClearPagePrivate(page);
> -		get_page(newpage);
> -		set_page_private(newpage, page_private(page));
> +		set_fs_page_private(newpage, (void *)page_private(page));
>  		set_page_private(page, 0);
>  		put_page(page);
> -		SetPagePrivate(newpage);
>  	}

This is just:
		set_fs_page_private(newpage, clear_fs_page_private(page));

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 8/9] orangefs: use set/clear_fs_page_private

[Dave Chinner]

On Sun, Apr 26, 2020 at 11:49:24PM +0200, Guoqing Jiang wrote:
> Since the new pair function is introduced, we can call them to clean the
> code in orangefs.
> 
> 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>
> ---
>  fs/orangefs/inode.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
> index 12ae630fbed7..893099d36e20 100644
> --- a/fs/orangefs/inode.c
> +++ b/fs/orangefs/inode.c
> @@ -64,9 +64,7 @@ static int orangefs_writepage_locked(struct page *page,
>  	}
>  	if (wr) {
>  		kfree(wr);
> -		set_page_private(page, 0);
> -		ClearPagePrivate(page);
> -		put_page(page);
> +		clear_fs_page_private(page);

THis is a pre-existing potential use-after-free vector. The wr
pointer held in the page->private needs to be cleared from the page
before it is freed.

>  	}
>  	return ret;
>  }
> @@ -409,9 +407,7 @@ static int orangefs_write_begin(struct file *file,
>  	wr->len = len;
>  	wr->uid = current_fsuid();
>  	wr->gid = current_fsgid();
> -	SetPagePrivate(page);
> -	set_page_private(page, (unsigned long)wr);
> -	get_page(page);
> +	set_fs_page_private(page, wr);
>  okay:
>  	return 0;
>  }
> @@ -460,17 +456,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_fs_page_private(page);

Ditto:
		wr = clear_fs_page_private(page);
		kfree(wr);

>  		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_fs_page_private(page);

And again.

>  		/* XXX is this right? only caller in fs */
>  		cancel_dirty_page(page);
>  		return;
> @@ -537,9 +529,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_fs_page_private(page);

And again.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 8/9] orangefs: use set/clear_fs_page_private

[Matthew Wilcox]

On Mon, Apr 27, 2020 at 08:24:55AM +1000, Dave Chinner wrote:
> > @@ -460,17 +456,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_fs_page_private(page);
> 
> Ditto:
> 		wr = clear_fs_page_private(page);
> 		kfree(wr);

You don't want to be as succinct as the btrfs change you suggested?

		kfree(clear_fs_page_private(page));

Re: [RFC PATCH 6/9] iomap: use set/clear_fs_page_private

[Matthew Wilcox]

On Sun, Apr 26, 2020 at 11:49:22PM +0200, Guoqing Jiang wrote:
> @@ -59,24 +59,18 @@ iomap_page_create(struct inode *inode, struct page *page)
>  	 * migrate_page_move_mapping() assumes that pages with private data have
>  	 * their count elevated by 1.
>  	 */
> -	get_page(page);
> -	set_page_private(page, (unsigned long)iop);
> -	SetPagePrivate(page);
> -	return iop;
> +	return (struct iomap_page *)set_fs_page_private(page, iop);
>  }

This cast is unnecessary.  void * will be automatically cast to the
appropriate pointer type.

> @@ -556,11 +550,9 @@ iomap_migrate_page(struct address_space *mapping, struct page *newpage,
>  
>  	if (page_has_private(page)) {
>  		ClearPagePrivate(page);
> -		get_page(newpage);
> -		set_page_private(newpage, page_private(page));
> +		set_fs_page_private(newpage, (void *)page_private(page));
>  		set_page_private(page, 0);
>  		put_page(page);
> -		SetPagePrivate(newpage);
>  	}

Same comment here as for the btrfs migrate page that Dave reviewed.

Re: [RFC PATCH 5/9] f2fs: use set/clear_fs_page_private

[Chao Yu]

On 2020/4/27 5:49, Guoqing Jiang wrote:
> Since the new pair function is introduced, we can call them to clean the
> code in f2fs.h.
> 
> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> Cc: Chao Yu <chao@kernel.org>
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

Acked-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [RFC PATCH 8/9] orangefs: use set/clear_fs_page_private

[Dave Chinner]

On Sun, Apr 26, 2020 at 05:12:34PM -0700, Matthew Wilcox wrote:
> On Mon, Apr 27, 2020 at 08:24:55AM +1000, Dave Chinner wrote:
> > > @@ -460,17 +456,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_fs_page_private(page);
> > 
> > Ditto:
> > 		wr = clear_fs_page_private(page);
> > 		kfree(wr);
> 
> You don't want to be as succinct as the btrfs change you suggested?
> 
> 		kfree(clear_fs_page_private(page));

That could be done, yes. I was really just trying to point out the
use after free that was occurring here rather than write compact
code...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 8/9] orangefs: use set/clear_fs_page_private

[Gao Xiang]

On Mon, Apr 27, 2020 at 08:24:55AM +1000, Dave Chinner wrote:
> On Sun, Apr 26, 2020 at 11:49:24PM +0200, Guoqing Jiang wrote:
> > Since the new pair function is introduced, we can call them to clean the
> > code in orangefs.
> > 
> > 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>
> > ---
> >  fs/orangefs/inode.c | 24 ++++++------------------
> >  1 file changed, 6 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
> > index 12ae630fbed7..893099d36e20 100644
> > --- a/fs/orangefs/inode.c
> > +++ b/fs/orangefs/inode.c
> > @@ -64,9 +64,7 @@ static int orangefs_writepage_locked(struct page *page,
> >  	}
> >  	if (wr) {
> >  		kfree(wr);
> > -		set_page_private(page, 0);
> > -		ClearPagePrivate(page);
> > -		put_page(page);
> > +		clear_fs_page_private(page);
> 
> THis is a pre-existing potential use-after-free vector. The wr
> pointer held in the page->private needs to be cleared from the page
> before it is freed.

I'm not familar with orangefs. In my opinion, generally all temporary
page->private access (r/w) should be properly protected by some locks,
most of time I think it could be at least page lock since .migratepage,
.invalidatepage, .releasepage, .. (such paths) are already called with
page locked (honestly I'm interested in this topic, please correct me
if I'm wrong).

I agree that the suggested modification is more clear and easy to read.

Thanks,
Gao Xiang

Re: [RFC PATCH 8/9] orangefs: use set/clear_fs_page_private

[Gao Xiang]

On Mon, Apr 27, 2020 at 10:58:02AM +0800, Gao Xiang wrote:
> On Mon, Apr 27, 2020 at 08:24:55AM +1000, Dave Chinner wrote:
> > On Sun, Apr 26, 2020 at 11:49:24PM +0200, Guoqing Jiang wrote:
> > > Since the new pair function is introduced, we can call them to clean the
> > > code in orangefs.
> > > 
> > > 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>
> > > ---
> > >  fs/orangefs/inode.c | 24 ++++++------------------
> > >  1 file changed, 6 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
> > > index 12ae630fbed7..893099d36e20 100644
> > > --- a/fs/orangefs/inode.c
> > > +++ b/fs/orangefs/inode.c
> > > @@ -64,9 +64,7 @@ static int orangefs_writepage_locked(struct page *page,
> > >  	}
> > >  	if (wr) {
> > >  		kfree(wr);
> > > -		set_page_private(page, 0);
> > > -		ClearPagePrivate(page);
> > > -		put_page(page);
> > > +		clear_fs_page_private(page);
> > 
> > THis is a pre-existing potential use-after-free vector. The wr
> > pointer held in the page->private needs to be cleared from the page
> > before it is freed.
> 
> I'm not familar with orangefs. In my opinion, generally all temporary
> page->private access (r/w) should be properly protected by some locks,

... page->private pointers (there may be some other uses rather than
as references). sorry about that...

> most of time I think it could be at least page lock since .migratepage,
> .invalidatepage, .releasepage, .. (such paths) are already called with
> page locked (honestly I'm interested in this topic, please correct me
> if I'm wrong).
> 
> I agree that the suggested modification is more clear and easy to read.
> 
> Thanks,
> Gao Xiang
> 
> 

Re: [RFC PATCH 1/9] include/linux/pagemap.h: introduce set/clear_fs_page_private

[Christoph Hellwig]

Why not attach_page_private and clear_page_private as that conveys
the use case a little better?

> +static inline void *set_fs_page_private(struct page *page, void *data)
> +{
> +	get_page(page);
> +	set_page_private(page, (unsigned long)data);
> +	SetPagePrivate(page);
> +
> +	return data;
> +}
> +
> +static inline 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;
> +}

Can you add kerneldoc comments describing them, including why we
take the refernces?  Also what is the point of the return value
of set_fs_page_private?

Re: [RFC PATCH 3/9] btrfs: use set/clear_fs_page_private

[Christoph Hellwig]

On Mon, Apr 27, 2020 at 08:20:54AM +1000, Dave Chinner wrote:
> >  void set_page_extent_mapped(struct page *page)
> >  {
> > -	if (!PagePrivate(page)) {
> > -		SetPagePrivate(page);
> > -		get_page(page);
> > -		set_page_private(page, EXTENT_PAGE_PRIVATE);
> > -	}
> > +	if (!PagePrivate(page))
> > +		set_fs_page_private(page, (void *)EXTENT_PAGE_PRIVATE);
> 
> Change the definition of EXTENT_PAGE_PRIVATE so the cast is not
> needed? Nothing ever reads EXTENT_PAGE_PRIVATE; it's only there to
> set the private flag for other code to check and release the extent
> mapping reference to the page...

IIRC there as a patch on the btrfs list to remove EXTENT_PAGE_PRIVATE,
it might be better to not bother changing it.  Maybe the btrfs
maintainers remember this better.

Re: [RFC PATCH 6/9] iomap: use set/clear_fs_page_private

[Christoph Hellwig]

On Sun, Apr 26, 2020 at 05:26:31PM -0700, Matthew Wilcox wrote:
> On Sun, Apr 26, 2020 at 11:49:22PM +0200, Guoqing Jiang wrote:
> > @@ -59,24 +59,18 @@ iomap_page_create(struct inode *inode, struct page *page)
> >  	 * migrate_page_move_mapping() assumes that pages with private data have
> >  	 * their count elevated by 1.
> >  	 */
> > -	get_page(page);
> > -	set_page_private(page, (unsigned long)iop);
> > -	SetPagePrivate(page);
> > -	return iop;
> > +	return (struct iomap_page *)set_fs_page_private(page, iop);
> >  }
> 
> This cast is unnecessary.  void * will be automatically cast to the
> appropriate pointer type.

I also find the pattern eather strange.  A:

	attach_page_private(page, iop);
	return iop;

explains the intent much better.

Re: [RFC PATCH 6/9] iomap: use set/clear_fs_page_private

[Christoph Hellwig]

FYI, you've only Cced the xfs list on this one patch.  Please Cc the
whole list to everyone, otherwise a person just on the xfs list has
no idea what your helpers added in patch 1 actually do.

Re: [RFC PATCH 1/9] include/linux/pagemap.h: introduce set/clear_fs_page_private

[Guoqing Jiang]

Hi Christoph,

On 4/27/20 7:52 AM, Christoph Hellwig wrote:
> Why not attach_page_private and clear_page_private as that conveys
> the use case a little better?

Yes, thanks for the good suggestion. Will rename them if no one else
has new idea ...

>> +static inline void *set_fs_page_private(struct page *page, void *data)
>> +{
>> +	get_page(page);
>> +	set_page_private(page, (unsigned long)data);
>> +	SetPagePrivate(page);
>> +
>> +	return data;
>> +}
>> +
>> +static inline 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;
>> +}
> Can you add kerneldoc comments describing them, including why we
> take the refernces?

Ok, will do.

> Also what is the point of the return value of set_fs_page_private?

In this way, iomap_page_create can just return the function, but you
don't like this way as you replied, will change the return value to "void".

Thanks,
Guoqing

Re: [RFC PATCH 5/9] f2fs: use set/clear_fs_page_private

[Guoqing Jiang]

On 4/27/20 4:22 AM, Chao Yu wrote:
> On 2020/4/27 5:49, Guoqing Jiang wrote:
>> Since the new pair function is introduced, we can call them to clean the
>> code in f2fs.h.
>>
>> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
>> Cc: Chao Yu <chao@kernel.org>
>> Cc: linux-f2fs-devel@lists.sourceforge.net
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
> Acked-by: Chao Yu <yuchao0@huawei.com>
>

Thanks for your review.

Guoqing

Re: [RFC PATCH 6/9] iomap: use set/clear_fs_page_private

[Guoqing Jiang]

On 4/27/20 7:57 AM, Christoph Hellwig wrote:
> FYI, you've only Cced the xfs list on this one patch.  Please Cc the
> whole list to everyone, otherwise a person just on the xfs list has
> no idea what your helpers added in patch 1 actually do.

Sorry, I should cc more lists for patch 1, thanks for reminder!

Thanks,
Guoqing

Re: [RFC PATCH 3/9] btrfs: use set/clear_fs_page_private

[Guoqing Jiang]

On 4/27/20 12:20 AM, Dave Chinner wrote:
> On Sun, Apr 26, 2020 at 11:49:19PM +0200, Guoqing Jiang wrote:
>> Since the new pair function is introduced, we can call them to clean the
>> code in btrfs.
>>
>> 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>
> ....
>
>>   void set_page_extent_mapped(struct page *page)
>>   {
>> -	if (!PagePrivate(page)) {
>> -		SetPagePrivate(page);
>> -		get_page(page);
>> -		set_page_private(page, EXTENT_PAGE_PRIVATE);
>> -	}
>> +	if (!PagePrivate(page))
>> +		set_fs_page_private(page, (void *)EXTENT_PAGE_PRIVATE);
> Change the definition of EXTENT_PAGE_PRIVATE so the cast is not
> needed? Nothing ever reads EXTENT_PAGE_PRIVATE; it's only there to
> set the private flag for other code to check and release the extent
> mapping reference to the page...

Not know the code well, so I just make the cast ...

>> @@ -8331,11 +8328,9 @@ static int btrfs_migratepage(struct address_space *mapping,
>>   
>>   	if (page_has_private(page)) {
>>   		ClearPagePrivate(page);
>> -		get_page(newpage);
>> -		set_page_private(newpage, page_private(page));
>> +		set_fs_page_private(newpage, (void *)page_private(page));
>>   		set_page_private(page, 0);
>>   		put_page(page);
>> -		SetPagePrivate(newpage);
>>   	}
> This is just:
> 		set_fs_page_private(newpage, clear_fs_page_private(page));
>

Thanks a lot! It is more better.

Thanks,
Guoqing

Re: [RFC PATCH 6/9] iomap: use set/clear_fs_page_private

[Guoqing Jiang]

On 4/27/20 7:55 AM, Christoph Hellwig wrote:
> On Sun, Apr 26, 2020 at 05:26:31PM -0700, Matthew Wilcox wrote:
>> On Sun, Apr 26, 2020 at 11:49:22PM +0200, Guoqing Jiang wrote:
>>> @@ -59,24 +59,18 @@ iomap_page_create(struct inode *inode, struct page *page)
>>>   	 * migrate_page_move_mapping() assumes that pages with private data have
>>>   	 * their count elevated by 1.
>>>   	 */
>>> -	get_page(page);
>>> -	set_page_private(page, (unsigned long)iop);
>>> -	SetPagePrivate(page);
>>> -	return iop;
>>> +	return (struct iomap_page *)set_fs_page_private(page, iop);
>>>   }
>> This cast is unnecessary.  void * will be automatically cast to the
>> appropriate pointer type.
> I also find the pattern eather strange.  A:
>
> 	attach_page_private(page, iop);
> 	return iop;
>
> explains the intent much better.

Thanks for the review, will do it.

Thanks,
Guoqing

Re: [RFC PATCH 6/9] iomap: use set/clear_fs_page_private

[Guoqing Jiang]

On 4/27/20 2:26 AM, Matthew Wilcox wrote:
> On Sun, Apr 26, 2020 at 11:49:22PM +0200, Guoqing Jiang wrote:
>> @@ -59,24 +59,18 @@ iomap_page_create(struct inode *inode, struct page *page)
>>   	 * migrate_page_move_mapping() assumes that pages with private data have
>>   	 * their count elevated by 1.
>>   	 */
>> -	get_page(page);
>> -	set_page_private(page, (unsigned long)iop);
>> -	SetPagePrivate(page);
>> -	return iop;
>> +	return (struct iomap_page *)set_fs_page_private(page, iop);
>>   }
> This cast is unnecessary.  void * will be automatically cast to the
> appropriate pointer type.
>
>> @@ -556,11 +550,9 @@ iomap_migrate_page(struct address_space *mapping, struct page *newpage,
>>   
>>   	if (page_has_private(page)) {
>>   		ClearPagePrivate(page);
>> -		get_page(newpage);
>> -		set_page_private(newpage, page_private(page));
>> +		set_fs_page_private(newpage, (void *)page_private(page));
>>   		set_page_private(page, 0);
>>   		put_page(page);
>> -		SetPagePrivate(newpage);
>>   	}
> Same comment here as for the btrfs migrate page that Dave reviewed.

Yes, thanks for the review.

Thanks,
Guoqing

Re: [RFC PATCH 8/9] orangefs: use set/clear_fs_page_private

[Guoqing Jiang]

Hi Mattew and Dave,

On 4/27/20 4:27 AM, Dave Chinner wrote:
> On Sun, Apr 26, 2020 at 05:12:34PM -0700, Matthew Wilcox wrote:
>> On Mon, Apr 27, 2020 at 08:24:55AM +1000, Dave Chinner wrote:
>>>> @@ -460,17 +456,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_fs_page_private(page);
>>> Ditto:
>>> 		wr = clear_fs_page_private(page);
>>> 		kfree(wr);
>> You don't want to be as succinct as the btrfs change you suggested?
>>
>> 		kfree(clear_fs_page_private(page));
> That could be done, yes. I was really just trying to point out the
> use after free that was occurring here rather than write compact
> code...

Really appreciate for your review, thanks.

Best Regards,
Guoqing

Re: [RFC PATCH 3/9] btrfs: use set/clear_fs_page_private

[David Sterba]

On Sun, Apr 26, 2020 at 10:54:28PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 27, 2020 at 08:20:54AM +1000, Dave Chinner wrote:
> > >  void set_page_extent_mapped(struct page *page)
> > >  {
> > > -	if (!PagePrivate(page)) {
> > > -		SetPagePrivate(page);
> > > -		get_page(page);
> > > -		set_page_private(page, EXTENT_PAGE_PRIVATE);
> > > -	}
> > > +	if (!PagePrivate(page))
> > > +		set_fs_page_private(page, (void *)EXTENT_PAGE_PRIVATE);
> > 
> > Change the definition of EXTENT_PAGE_PRIVATE so the cast is not
> > needed? Nothing ever reads EXTENT_PAGE_PRIVATE; it's only there to
> > set the private flag for other code to check and release the extent
> > mapping reference to the page...
> 
> IIRC there as a patch on the btrfs list to remove EXTENT_PAGE_PRIVATE,
> it might be better to not bother changing it.  Maybe the btrfs
> maintainers remember this better.

The patch removing it is part of patchset adding full iomap support to
btrfs,
(https://lore.kernel.org/linux-btrfs/20190905150650.21089-4-rgoldwyn@suse.de/)
but it'll still take some time so I'm OK with using the
set_fs_page_private helper and adding the cast to EXTENT_PAGE_PRIVATE
definition.