Re: [PATCH v2 17/18] btrfs: integrate page status update for read path into begin/end_page_read()

[Nikolay Borisov <nborisov@suse.com>]

On 10.12.20 г. 8:39 ч., Qu Wenruo wrote:
> In btrfs data page read path, the page status update are handled in two
> different locations:
> 
>   btrfs_do_read_page()
>   {
> 	while (cur <= end) {
> 		/* No need to read from disk */
> 		if (HOLE/PREALLOC/INLINE){
> 			memset();
> 			set_extent_uptodate();
> 			continue;
> 		}
> 		/* Read from disk */
> 		ret = submit_extent_page(end_bio_extent_readpage);
>   }
> 
>   end_bio_extent_readpage()
>   {
> 	endio_readpage_uptodate_page_status();
>   }
> 
> This is fine for sectorsize == PAGE_SIZE case, as for above loop we
> should only hit one branch and then exit.
> 
> But for subpage, there are more works to be done in page status update:
> - Page Unlock condition
>   Unlike regular page size == sectorsize case, we can no longer just
>   unlock a page.
>   Only the last reader of the page can unlock the page.
>   This means, we can unlock the page either in the while() loop, or in
>   the endio function.
> 
> - Page uptodate condition
>   Since we have multiple sectors to read for a page, we can only mark
>   the full page uptodate if all sectors are uptodate.
> 
> To handle both subpage and regular cases, introduce a pair of functions
> to help handling page status update:
> 
> - being_page_read()
>   For regular case, it does nothing.
>   For subpage case, it update the reader counters so that later
>   end_page_read() can know who is the last one to unlock the page.
> 
> - end_page_read()
>   This is just endio_readpage_uptodate_page_status() renamed.
>   The original name is a little too long and too specific for endio.
> 
>   The only new trick added is the condition for page unlock.
>   Now for subage data, we unlock the page if we're the last reader.
> 
> This does not only provide the basis for subpage data read, but also
> hide the special handling of page read from the main read loop.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 39 +++++++++++++++++++++++++-----------
>  fs/btrfs/subpage.h   | 47 ++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 68 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 4e4ed9c453ae..56174e7f0ae8 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2841,8 +2841,18 @@ static void endio_readpage_release_extent(struct processed_extent *processed,
>  	processed->uptodate = uptodate;
>  }
>  
> -static void endio_readpage_update_page_status(struct page *page, bool uptodate,
> -					      u64 start, u64 end)
> +static void begin_data_page_read(struct btrfs_fs_info *fs_info, struct page *page)
> +{
> +	ASSERT(PageLocked(page));
> +	if (fs_info->sectorsize == PAGE_SIZE)
> +		return;
> +
> +	ASSERT(PagePrivate(page) && page->private);
2nd part of the assert condition is redundant, page->private should only
be set via the respective generic helper which is never called with NULL
as the 2nd argument.

> +	ASSERT(page->mapping->host != fs_info->btree_inode);
That function is only called by btrfs_do_readpage which is used only for
data read out, so do we really need this? I understand you want to be
extra careful but I think this is going over the top.

> +	btrfs_subpage_start_reader(fs_info, page, page_offset(page), PAGE_SIZE);
> +}
> +
> +static void end_page_read(struct page *page, bool uptodate, u64 start, u64 end)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
>  	u32 len;
> @@ -2860,7 +2870,12 @@ static void endio_readpage_update_page_status(struct page *page, bool uptodate,
>  
>  	if (fs_info->sectorsize == PAGE_SIZE)
>  		unlock_page(page);
> -	/* Subpage locking will be handled in later patches */
> +	else if (page->mapping->host != fs_info->btree_inode)

Use is_data_inode() helper

> +		/*
> +		 * For subpage data, unlock the page if we're the last reader.
> +		 * For subpage metadata, page lock is not utilized for read.
> +		 */
> +		btrfs_subpage_end_reader(fs_info, page, start, len);
>  }
>  
>  /*

<snip>
> diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
> index 8592234d773e..6c801ef00d2d 100644
> --- a/fs/btrfs/subpage.h
> +++ b/fs/btrfs/subpage.h
> @@ -31,6 +31,9 @@ struct btrfs_subpage {
>  			u16 tree_block_bitmap;
>  		};
>  		/* structures only used by data */
> +		struct {
> +			atomic_t readers;
> +		};
>  	};
>  };
>  
> @@ -48,6 +51,17 @@ static inline void btrfs_subpage_clamp_range(struct page *page,
>  		     orig_start + orig_len) - *start;
>  }
>  
> +static inline void btrfs_subpage_assert(struct btrfs_fs_info *fs_info,
> +					struct page *page, u64 start, u32 len)
> +{
> +	/* Basic checks */
> +	ASSERT(PagePrivate(page) && page->private);
> +	ASSERT(IS_ALIGNED(start, fs_info->sectorsize) &&
> +	       IS_ALIGNED(len, fs_info->sectorsize));
> +	ASSERT(page_offset(page) <= start &&
> +	       start + len <= page_offset(page) + PAGE_SIZE);
> +}
> +
>  /*
>   * Convert the [start, start + len) range into a u16 bitmap
>   *
> @@ -59,12 +73,8 @@ static inline u16 btrfs_subpage_calc_bitmap(struct btrfs_fs_info *fs_info,
>  	int bit_start = (start - page_offset(page)) >> fs_info->sectorsize_bits;
>  	int nbits = len >> fs_info->sectorsize_bits;
>  
> -	/* Basic checks */
> -	ASSERT(PagePrivate(page) && page->private);
> -	ASSERT(IS_ALIGNED(start, fs_info->sectorsize) &&
> -	       IS_ALIGNED(len, fs_info->sectorsize));
> -	ASSERT(page_offset(page) <= start &&
> -	       start + len <= page_offset(page) + PAGE_SIZE);
> +	btrfs_subpage_assert(fs_info, page, start, len);
> +
>  	/*
>  	 * Here nbits can be 16, thus can go beyond u16 range. Here we make the
>  	 * first left shift to be calculated in unsigned long (u32), then
> @@ -73,6 +83,31 @@ static inline u16 btrfs_subpage_calc_bitmap(struct btrfs_fs_info *fs_info,
>  	return (u16)(((1UL << nbits) - 1) << bit_start);
>  }
>  
> +static inline void btrfs_subpage_start_reader(struct btrfs_fs_info *fs_info,
> +					      struct page *page, u64 start,
> +					      u32 len)
> +{
> +	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
> +	int nbits = len >> fs_info->sectorsize_bits;
> +
> +	btrfs_subpage_assert(fs_info, page, start, len);
> +
> +	ASSERT(atomic_read(&subpage->readers) == 0);
> +	atomic_set(&subpage->readers, nbits);

To make this more explicit implement it via atomic_add_unless and assert
on the return value.

> +}
> +
> +static inline void btrfs_subpage_end_reader(struct btrfs_fs_info *fs_info,
> +			struct page *page, u64 start, u32 len)
> +{
> +	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
> +	int nbits = len >> fs_info->sectorsize_bits;
> +
> +	btrfs_subpage_assert(fs_info, page, start, len);
> +	ASSERT(atomic_read(&subpage->readers) >= nbits);
> +	if (atomic_sub_and_test(nbits, &subpage->readers))
> +		unlock_page(page);
> +}
> +
>  static inline void btrfs_subpage_set_tree_block(struct btrfs_fs_info *fs_info,
>  			struct page *page, u64 start, u32 len)
>  {
>