Re: [PATCH v2 15/18] btrfs: disk-io: introduce subpage metadata validation check

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

On 2020/12/14 下午6:21, Nikolay Borisov wrote:
>
>
> On 10.12.20 г. 8:39 ч., Qu Wenruo wrote:
>> For subpage metadata validation check, there are some difference:
>> - Read must finish in one bvec
>>    Since we're just reading one subpage range in one page, it should
>>    never be split into two bios nor two bvecs.
>>
>> - How to grab the existing eb
>>    Instead of grabbing eb using page->private, we have to go search radix
>>    tree as we don't have any direct pointer at hand.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/disk-io.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 82 insertions(+)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index b6c03a8b0c72..adda76895058 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -591,6 +591,84 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>>   	return ret;
>>   }
>>
>> +static int validate_subpage_buffer(struct page *page, u64 start, u64 end,
>> +				   int mirror)
>> +{
>> +	struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
>> +	struct extent_buffer *eb;
>> +	int reads_done;
>> +	int ret = 0;
>> +
>> +	if (!IS_ALIGNED(start, fs_info->sectorsize) ||
>
> That's guaranteed by the allocator.
>
>> +	    !IS_ALIGNED(end - start + 1, fs_info->sectorsize) ||
> That's guaranteed by the fact that  nodesize is a multiple of sectorsize.
>
>> +	    !IS_ALIGNED(end - start + 1, fs_info->nodesize)) {
>
> And that's also guaranteed that the size of an eb is always a nodesize.
> Also aren't those checks already performed by the tree-checker during
> write? Just remove this as it adds noise.
>
>> +		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));> +		btrfs_err(fs_info, "invalid tree read bytenr");
>> +		return -EUCLEAN;
>> +	}
>> +
>> +	/*
>> +	 * We don't allow bio merge for subpage metadata read, so we should
>> +	 * only get one eb for each endio hook.
>> +	 */
>> +	ASSERT(end == start + fs_info->nodesize - 1);
>> +	ASSERT(PagePrivate(page));
>> +
>> +	rcu_read_lock();
>> +	eb = radix_tree_lookup(&fs_info->buffer_radix,
>> +			       start / fs_info->sectorsize);
>
> This division op likely produces the kernel robot's warning. It could be
> written to use >> fs_info->sectorsize_bits. Furthermore this usage of
> radix tree + rcu without acquiring the refs is unsafe as per my
> explanation of, essentially, identical issue in patch 12 and our offline
> chat about it.

Another relic I forgot in the long update history, nice find.

>
>> +	rcu_read_unlock();
>> +
>> +	/*
>> +	 * When we are reading one tree block, eb must have been
>> +	 * inserted into the radix tree. If not something is wrong.
>> +	 */
>> +	if (!eb) {
>> +		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
>> +		btrfs_err(fs_info,
>> +			"can't find extent buffer for bytenr %llu",
>> +			start);
>> +		return -EUCLEAN;
>> +	}
>
> That's impossible to execute and such a failure will result in a crash
> so just remove this code.
>
>> +	/*
>> +	 * The pending IO might have been the only thing that kept
>> +	 * this buffer in memory.  Make sure we have a ref for all
>> +	 * this other checks
>> +	 */
>> +	atomic_inc(&eb->refs);
>> +
>> +	reads_done = atomic_dec_and_test(&eb->io_pages);
>> +	/* Subpage read must finish in page read */
>> +	ASSERT(reads_done);
>
> Just ASSERT(atomic_dec_and_test(&eb->io_pages)). Again, for subpage I
> think that's a bit much since it only has 1 page so it's guaranteed that
> it will always be true.

IIRC ASSERT() won't execute whatever in it for non debug build.
Thus ASSERT(atomic_*) would cause non-debug kernel not to decrease the
io_pages and hangs the system.

Exactly the pitfall I'm thinking of.

Thanks,
Qu

>> +
>> +	eb->read_mirror = mirror;
>> +	if (test_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags)) {
>> +		ret = -EIO;
>> +		goto err;
>> +	}
>> +	ret = validate_extent_buffer(eb);
>> +	if (ret < 0)
>> +		goto err;
>> +
>> +	if (test_and_clear_bit(EXTENT_BUFFER_READAHEAD, &eb->bflags))
>> +		btree_readahead_hook(eb, ret);
>> +
>> +	set_extent_buffer_uptodate(eb);
>> +
>> +	free_extent_buffer(eb);
>> +	return ret;
>> +err:
>> +	/*
>> +	 * our io error hook is going to dec the io pages
>> +	 * again, we have to make sure it has something to
>> +	 * decrement
>> +	 */
>
> That comment is slightly ambiguous - it's not the io error hook that
> does the decrement but end_bio_extent_readpage. Just rewrite the comment
> to :
>
> "end_bio_extent_readpage decrements io_pages in case of error, make sure
> it has ...."
>
>> +	atomic_inc(&eb->io_pages);
>> +	clear_extent_buffer_uptodate(eb);
>> +	free_extent_buffer(eb);
>> +	return ret;
>> +}
>> +
>>   int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio,
>>   				   struct page *page, u64 start, u64 end,
>>   				   int mirror)
>> @@ -600,6 +678,10 @@ int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio,
>>   	int reads_done;
>>
>>   	ASSERT(page->private);
>> +
>> +	if (btrfs_sb(page->mapping->host->i_sb)->sectorsize < PAGE_SIZE)
>> +		return validate_subpage_buffer(page, start, end, mirror);
>
> nit: validate_metadata_buffer is called in only once place so I'm
> wondering won't it make it more readable if this check is lifted to its
> sole caller so that when reading end_bio_extent_readpage it's apparent
> what's going on. Though it's apparent that the nesting in the caller
> will get somewhat unwieldy so won't be pressing hard for this.
>> +
>>   	eb = (struct extent_buffer *)page->private;
>>
>>
>>