Re: [PATCH 09/17] btrfs: refactor btrfs_release_extent_buffer_pages()

Nikolay Borisov
2020-09-11T11:18:16Z

On 8.09.20 г. 10:52 ч., Qu Wenruo wrote:
> We have attach_extent_buffer_page() and it get utilized in
> btrfs_clone_extent_buffer() and alloc_extent_buffer().
>
> But in btrfs_release_extent_buffer_pages() we manually call
> detach_page_private().
>
> This is fine for current code, but if we're going to support subpage
> size, we will do a lot of more work other than just calling
> detach_page_private().
>
> This patch will extract the main work of btrfs_clone_extent_buffer()

Did you mean to type btrfs_release_extent_buffer_pages instead of
clone_extent_buffer ?

> into detach_extent_buffer_page() so that later subpage size support can
> put their own code into them.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> fs/btrfs/extent_io.c | 58 +++++++++++++++++++-------------------------
> 1 file changed, 25 insertions(+), 33 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 3c8fe40f67fa..1cb41dab7a1d 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4920,6 +4920,29 @@ int extent_buffer_under_io(const struct extent_buffer *eb)
> test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
> }
>
> +static void detach_extent_buffer_page(struct extent_buffer *eb,
> + struct page *page)
> +{
> + bool mapped = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);

nit: Now you are performing the atomic op once per page rather than once
per-eb.

> +
> + if (!page)
> + return;
> +
> + if (mapped)
> + spin_lock(&page->mapping->private_lock);
> + if (PagePrivate(page) && page->private == (unsigned long)eb) {
> + BUG_ON(test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
> + BUG_ON(PageDirty(page));
> + BUG_ON(PageWriteback(page));
> + /* We need to make sure we haven't be attached to a new eb. */
> + detach_page_private(page);
> + }
> + if (mapped)
> + spin_unlock(&page->mapping->private_lock);
> + /* One for when we allocated the page */
> + put_page(page);
> +}
> +
> /*
> * Release all pages attached to the extent buffer.
> */
> @@ -4927,43 +4950,12 @@ static void btrfs_release_extent_buffer_pages(struct extent_buffer *eb)
> {
> int i;
> int num_pages;
> - int mapped = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
>
> BUG_ON(extent_buffer_under_io(eb));
>
> num_pages = num_extent_pages(eb);
> - for (i = 0; i < num_pages; i++) {
> - struct page *page = eb->pages[i];
> -
> - if (!page)
> - continue;
> - if (mapped)
> - spin_lock(&page->mapping->private_lock);
> - /*
> - * We do this since we'll remove the pages after we've
> - * removed the eb from the radix tree, so we could race
> - * and have this page now attached to the new eb. So
> - * only clear page_private if it's still connected to
> - * this eb.
> - */
> - if (PagePrivate(page) &&
> - page->private == (unsigned long)eb) {
> - BUG_ON(test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
> - BUG_ON(PageDirty(page));
> - BUG_ON(PageWriteback(page));
> - /*
> - * We need to make sure we haven't be attached
> - * to a new eb.
> - */
> - detach_page_private(page);
> - }
> -
> - if (mapped)
> - spin_unlock(&page->mapping->private_lock);
> -
> - /* One for when we allocated the page */
> - put_page(page);
> - }
> + for (i = 0; i < num_pages; i++)
> + detach_extent_buffer_page(eb, eb->pages[i]);
> }
>
> /*
>