| Message ID | 20200426214925.10970-4-guoqing.jiang@cloud.ionos.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce set/clear_fs_page_private to cleanup code | expand |
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.
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.
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
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.
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); } /*
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(-)