Re: [PATCH v2 16/18] btrfs: introduce btrfs_subpage for data inodes

[Nikolay Borisov <nborisov@suse.com>]

On 10.12.20 г. 8:39 ч., Qu Wenruo wrote:
> To support subpage sector size, data also need extra info to make sure
> which sectors in a page are uptodate/dirty/...
> 
> This patch will make pages for data inodes to get btrfs_subpage
> structure attached, and detached when the page is freed.
> 
> This patch also slightly changes the timing when
> set_page_extent_mapped() to make sure:
> - We have page->mapping set
>   page->mapping->host is used to grab btrfs_fs_info, thus we can only
>   call this function after page is mapped to an inode.
> 
>   One call site attaches pages to inode manually, thus we have to modify
>   the timing of set_page_extent_mapped() a little.
> 
> - As soon as possible, before other operations
>   Since memory allocation can fail, we have to do extra error handling.
>   Calling set_page_extent_mapped() as soon as possible can simply the
>   error handling for several call sites.
> 
> The idea is pretty much the same as iomap_page, but with more bitmaps
> for btrfs specific cases.
> 
> Currently the plan is to switch iomap if iomap can provide sector
> aligned write back (only write back dirty sectors, but not the full
> page, data balance require this feature).
> 
> So we will stick to btrfs specific bitmap for now.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/compression.c      | 10 ++++++--
>  fs/btrfs/extent_io.c        | 47 +++++++++++++++++++++++++++++++++----
>  fs/btrfs/extent_io.h        |  3 ++-
>  fs/btrfs/file.c             | 10 +++++---
>  fs/btrfs/free-space-cache.c | 15 +++++++++---
>  fs/btrfs/inode.c            | 12 ++++++----
>  fs/btrfs/ioctl.c            |  5 +++-
>  fs/btrfs/reflink.c          |  5 +++-
>  fs/btrfs/relocation.c       | 12 ++++++++--
>  9 files changed, 98 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 5ae3fa0386b7..6d203acfdeb3 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -542,13 +542,19 @@ static noinline int add_ra_bio_pages(struct inode *inode,
>  			goto next;
>  		}
>  
> -		end = last_offset + PAGE_SIZE - 1;
>  		/*
>  		 * at this point, we have a locked page in the page cache
>  		 * for these bytes in the file.  But, we have to make
>  		 * sure they map to this compressed extent on disk.
>  		 */
> -		set_page_extent_mapped(page);
> +		ret = set_page_extent_mapped(page);
> +		if (ret < 0) {
> +			unlock_page(page);
> +			put_page(page);
> +			break;
> +		}
> +
> +		end = last_offset + PAGE_SIZE - 1;
>  		lock_extent(tree, last_offset, end);
>  		read_lock(&em_tree->lock);
>  		em = lookup_extent_mapping(em_tree, last_offset,
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 64a19c1884fc..4e4ed9c453ae 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3191,10 +3191,40 @@ static int attach_extent_buffer_page(struct extent_buffer *eb,
>  	return 0;
>  }
>  
> -void set_page_extent_mapped(struct page *page)
> +int __must_check set_page_extent_mapped(struct page *page)
>  {
> -	if (!PagePrivate(page))
> +	struct btrfs_fs_info *fs_info;
> +
> +	ASSERT(page->mapping);
> +
> +	if (PagePrivate(page))
> +		return 0;
> +
> +	fs_info = btrfs_sb(page->mapping->host->i_sb);
> +	if (fs_info->sectorsize == PAGE_SIZE) {
>  		attach_page_private(page, (void *)EXTENT_PAGE_PRIVATE);
> +		return 0;
> +	}
> +
> +	return btrfs_attach_subpage(fs_info, page);

In all previous patches < PAGE_SIZE is the special case, in this
function it's reversed. For the sake of consistency change that so
btrfs_attch_subpage is executed inside the conditional.

> +}
> +
> +void clear_page_extent_mapped(struct page *page)
> +{
> +	struct btrfs_fs_info *fs_info;
> +
> +	ASSERT(page->mapping);
> +
> +	if (!PagePrivate(page))
> +		return;
> +
> +	fs_info = btrfs_sb(page->mapping->host->i_sb);
> +	if (fs_info->sectorsize == PAGE_SIZE) {
> +		detach_page_private(page);
> +		return;
> +	}
> +
> +	btrfs_detach_subpage(fs_info, page);

DITTO

>  }
>  
>  static struct extent_map *

<snip>

> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index a29b50208eee..9b878616b489 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1373,6 +1373,12 @@ static noinline int prepare_pages(struct inode *inode, struct page **pages,
>  			goto fail;
>  		}
>  
> +		err = set_page_extent_mapped(pages[i]);
> +		if (err < 0) {
> +			faili = i;
> +			goto fail;
> +		}
> +
>  		if (i == 0)
>  			err = prepare_uptodate_page(inode, pages[i], pos,
>  						    force_uptodate);
> @@ -1470,10 +1476,8 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages,
>  	 * We'll call btrfs_dirty_pages() later on, and that will flip around
>  	 * delalloc bits and dirty the pages as required.
>  	 */
> -	for (i = 0; i < num_pages; i++) {
> -		set_page_extent_mapped(pages[i]);
> +	for (i = 0; i < num_pages; i++)
>  		WARN_ON(!PageLocked(pages[i]));
> -	}
The comment above this needs to be removed/rewritten I guess?
Essentially that set_page_extent_mapped is moved to prepare_pages.

>  
>  	return ret;
>  }

<snip>

> @@ -8355,7 +8357,9 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
>  	wait_on_page_writeback(page);
>  
>  	lock_extent_bits(io_tree, page_start, page_end, &cached_state);
> -	set_page_extent_mapped(page);
> +	ret = set_page_extent_mapped(page);
> +	if (ret < 0)
> +		goto out_unlock;

You should use ret2, ret in this function is used for the retval of
vmf_error().

>  
>  	/*
>  	 * we can't set the delalloc bits if there are pending ordered

<snip>