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/[email protected]/
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
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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "Darrick J. Wong" <[email protected]>
Cc: William Kucharski <[email protected]>
Cc: "Kirill A. Shutemov" <[email protected]>
Cc: Andreas Gruenbacher <[email protected]>
Cc: Yang Shi <[email protected]>
Cc: Yafang Shao <[email protected]>
Signed-off-by: Guoqing Jiang <[email protected]>
---
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
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?
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