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

[Qu Wenruo <wqu@suse.com>]

On 2020/9/11 下午7:17, Nikolay Borisov wrote:
> 
> 
> 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 ?

Oh, that's what I mean exactly...

> 
>> 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.

Makes sense, I should just extract the for () loop into a function
rather than the loop body.

Thanks,
Qu

> 
>> +
>> +	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]);
>>  }
>>  
>>  /*
>>
>