Re: [PATCH v2 03/24] btrfs: extent_io: replace extent_start/extent_len with better structure for end_bio_extent_readpage()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3170 bytes --]



On 2020/11/19 上午12:05, David Sterba wrote:
> On Fri, Nov 13, 2020 at 08:51:28PM +0800, Qu Wenruo wrote:
>>  }
>>  
>> +/*
>> + * Records previously processed extent range.
>> + *
>> + * For endio_readpage_release_extent() to handle a full extent range, reducing
>> + * the extent io operations.
>> + */
>> +struct processed_extent {
>> +	struct btrfs_inode *inode;
>> +	u64 start;	/* file offset in @inode */
>> +	u64 end;	/* file offset in @inode */
> 
> Please don't use the in-line comments for struct members.

Even for such short description?

That's a little overkilled to me now.

Thanks,
Qu
> 
>> +	bool uptodate;
>> +};
>> +
>> +/*
>> + * Try to release processed extent range.
>> + *
>> + * May not release the extent range right now if the current range is contig
> 
> 'contig' means what? If it's for 'contiguous' then please spell it out
> in text and use the abbreviated form only for variables.
> 
>> + * with processed extent.
>> + *
>> + * Will release processed extent when any of @inode, @uptodate, the range is
>> + * no longer contig with processed range.
>> + * Pass @inode == NULL will force processed extent to be released.
>> + */
>>  static void
>> -endio_readpage_release_extent(struct extent_io_tree *tree, u64 start, u64 len,
>> -			      int uptodate)
>> +endio_readpage_release_extent(struct processed_extent *processed,
>> +			      struct btrfs_inode *inode, u64 start, u64 end,
>> +			      bool uptodate)
>>  {
>>  	struct extent_state *cached = NULL;
>> -	u64 end = start + len - 1;
>> +	struct extent_io_tree *tree;
>>  
>> -	if (uptodate && tree->track_uptodate)
>> -		set_extent_uptodate(tree, start, end, &cached, GFP_ATOMIC);
>> -	unlock_extent_cached_atomic(tree, start, end, &cached);
>> +	/* We're the first extent, initialize @processed */
>> +	if (!processed->inode)
>> +		goto update;
>> +
>> +	/*
>> +	 * Contig with processed extent. Just uptodate the end
>> +	 *
>> +	 * Several things to notice:
>> +	 * - Bio can be merged as long as on-disk bytenr is contig
>> +	 *   This means we can have page belonging to other inodes, thus need to
>> +	 *   check if the inode matches.
>> +	 * - Bvec can contain range beyond current page for multi-page bvec
>> +	 *   Thus we need to do processed->end + 1 >= start check
>> +	 */
>> +	if (processed->inode == inode && processed->uptodate == uptodate &&
>> +	    processed->end + 1 >= start && end >= processed->end) {
>> +		processed->end = end;
>> +		return;
>> +	}
>> +
>> +	tree = &processed->inode->io_tree;
>> +	/*
>> +	 * Now we have a range not contig with processed range, release the
>> +	 * processed range now.
>> +	 */
>> +	if (processed->uptodate && tree->track_uptodate)
>> +		set_extent_uptodate(tree, processed->start, processed->end,
>> +				    &cached, GFP_ATOMIC);
>> +	unlock_extent_cached_atomic(tree, processed->start, processed->end,
>> +				    &cached);
>> +
>> +update:
>> +	/* Update @processed to current range */
>> +	processed->inode = inode;
>> +	processed->start = start;
>> +	processed->end = end;
>> +	processed->uptodate = uptodate;
>>  }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]