[RFC,1/9] include/linux/pagemap.h: introduce set/clear_fs_page_private
diff mbox series

Message ID 20200426214925.10970-2-guoqing.jiang@cloud.ionos.com
State New
Headers show
Series
  • Introduce set/clear_fs_page_private to cleanup code
Related show

Commit Message

Guoqing Jiang April 26, 2020, 9:49 p.m. UTC
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(+)

Comments

Christoph Hellwig April 27, 2020, 5:52 a.m. UTC | #1
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?
Guoqing Jiang April 27, 2020, 8:10 a.m. UTC | #2
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

Patch
diff mbox series

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