| Message ID | 20201210063905.75727-5-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add read-only support for subpage sector size | expand |
On 10.12.20 г. 8:38 ч., Qu Wenruo wrote: > This patch will extract the code to grab an extent buffer from a page > into a helper, grab_extent_buffer_from_page(). > > This reduces one indent level, and provides the work place for later > expansion for subapge support. > > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 52 +++++++++++++++++++++++++++----------------- > 1 file changed, 32 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 612fe60b367e..6350c2687c7e 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -5251,6 +5251,32 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, > } > #endif > > +static struct extent_buffer *grab_extent_buffer_from_page(struct page *page) nit: Make the name just grab_extent_buffer/get_extent_buffer, given you pass in a page as an input parameter "from_page" is obvious. > +{ > + struct extent_buffer *exists; > + > + /* Page not yet attached to an extent buffer */ > + if (!PagePrivate(page)) > + return NULL; > + > + /* > + * We could have already allocated an eb for this page > + * and attached one so lets see if we can get a ref on > + * the existing eb, and if we can we know it's good and > + * we can just return that one, else we know we can just > + * overwrite page->private. > + */ > + exists = (struct extent_buffer *)page->private; > + if (atomic_inc_not_zero(&exists->refs)) { > + mark_extent_buffer_accessed(exists, page); > + return exists; > + } nit: This patch slightly changes the timing of mark_extent_buffer_accessed, as it's now called under mapping->private_lock and respective page locked. Looking at map_extent_buffer_accessed it does iterate pages and call mark_page_accessed on them as well as calling check_buffer_tre_ref which does some atomic ops. While it might not be a big hit I'd expect there will be some minimal performance regression. > + > + WARN_ON(PageDirty(page)); > + detach_page_private(page); > + return NULL; > +} > + > struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > u64 start, u64 owner_root, int level) > { > @@ -5296,26 +5322,12 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > } > > spin_lock(&mapping->private_lock); > - if (PagePrivate(p)) { > - /* > - * We could have already allocated an eb for this page > - * and attached one so lets see if we can get a ref on > - * the existing eb, and if we can we know it's good and > - * we can just return that one, else we know we can just > - * overwrite page->private. > - */ > - exists = (struct extent_buffer *)p->private; > - if (atomic_inc_not_zero(&exists->refs)) { > - spin_unlock(&mapping->private_lock); > - unlock_page(p); > - put_page(p); > - mark_extent_buffer_accessed(exists, p); > - goto free_eb; > - } > - exists = NULL; > - > - WARN_ON(PageDirty(p)); > - detach_page_private(p); > + exists = grab_extent_buffer_from_page(p); > + if (exists) { > + spin_unlock(&mapping->private_lock); > + unlock_page(p); > + put_page(p); > + goto free_eb; > } > attach_extent_buffer_page(eb, p); > spin_unlock(&mapping->private_lock); >
On 12/10/20 1:38 AM, Qu Wenruo wrote: > This patch will extract the code to grab an extent buffer from a page > into a helper, grab_extent_buffer_from_page(). > > This reduces one indent level, and provides the work place for later > expansion for subapge support. > > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 52 +++++++++++++++++++++++++++----------------- > 1 file changed, 32 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 612fe60b367e..6350c2687c7e 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -5251,6 +5251,32 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, > } > #endif > > +static struct extent_buffer *grab_extent_buffer_from_page(struct page *page) > +{ > + struct extent_buffer *exists; > + > + /* Page not yet attached to an extent buffer */ > + if (!PagePrivate(page)) > + return NULL; > + > + /* > + * We could have already allocated an eb for this page > + * and attached one so lets see if we can get a ref on > + * the existing eb, and if we can we know it's good and > + * we can just return that one, else we know we can just > + * overwrite page->private. > + */ > + exists = (struct extent_buffer *)page->private; > + if (atomic_inc_not_zero(&exists->refs)) { > + mark_extent_buffer_accessed(exists, page); > + return exists; > + } > + > + WARN_ON(PageDirty(page)); > + detach_page_private(page); > + return NULL; > +} > + > struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > u64 start, u64 owner_root, int level) > { > @@ -5296,26 +5322,12 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > } > > spin_lock(&mapping->private_lock); > - if (PagePrivate(p)) { > - /* > - * We could have already allocated an eb for this page > - * and attached one so lets see if we can get a ref on > - * the existing eb, and if we can we know it's good and > - * we can just return that one, else we know we can just > - * overwrite page->private. > - */ > - exists = (struct extent_buffer *)p->private; > - if (atomic_inc_not_zero(&exists->refs)) { > - spin_unlock(&mapping->private_lock); > - unlock_page(p); > - put_page(p); > - mark_extent_buffer_accessed(exists, p); > - goto free_eb; > - } > - exists = NULL; > - > - WARN_ON(PageDirty(p)); > - detach_page_private(p); > + exists = grab_extent_buffer_from_page(p); > + if (exists) { > + spin_unlock(&mapping->private_lock); > + unlock_page(p); > + put_page(p); Put the mark_extent_buffer_accessed() here. Thanks, Josef
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 612fe60b367e..6350c2687c7e 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -5251,6 +5251,32 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, } #endif +static struct extent_buffer *grab_extent_buffer_from_page(struct page *page) +{ + struct extent_buffer *exists; + + /* Page not yet attached to an extent buffer */ + if (!PagePrivate(page)) + return NULL; + + /* + * We could have already allocated an eb for this page + * and attached one so lets see if we can get a ref on + * the existing eb, and if we can we know it's good and + * we can just return that one, else we know we can just + * overwrite page->private. + */ + exists = (struct extent_buffer *)page->private; + if (atomic_inc_not_zero(&exists->refs)) { + mark_extent_buffer_accessed(exists, page); + return exists; + } + + WARN_ON(PageDirty(page)); + detach_page_private(page); + return NULL; +} + struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start, u64 owner_root, int level) { @@ -5296,26 +5322,12 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, } spin_lock(&mapping->private_lock); - if (PagePrivate(p)) { - /* - * We could have already allocated an eb for this page - * and attached one so lets see if we can get a ref on - * the existing eb, and if we can we know it's good and - * we can just return that one, else we know we can just - * overwrite page->private. - */ - exists = (struct extent_buffer *)p->private; - if (atomic_inc_not_zero(&exists->refs)) { - spin_unlock(&mapping->private_lock); - unlock_page(p); - put_page(p); - mark_extent_buffer_accessed(exists, p); - goto free_eb; - } - exists = NULL; - - WARN_ON(PageDirty(p)); - detach_page_private(p); + exists = grab_extent_buffer_from_page(p); + if (exists) { + spin_unlock(&mapping->private_lock); + unlock_page(p); + put_page(p); + goto free_eb; } attach_extent_buffer_page(eb, p); spin_unlock(&mapping->private_lock);