Re: [PATCH] btrfs: remove btrfs_set_range_writeback()

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

在 2024/10/9 08:09, David Sterba 写道:
> On Wed, Oct 09, 2024 at 07:26:36AM +1030, Qu Wenruo wrote:
>>>> --- a/fs/btrfs/inode.c
>>>> +++ b/fs/btrfs/inode.c
>>>> @@ -8939,28 +8939,6 @@ static int btrfs_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
>>>>    	return finish_open_simple(file, ret);
>>>>    }
>>>>
>>>> -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end)
>>>> -{
>>>> -	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>>>> -	unsigned long index = start >> PAGE_SHIFT;
>>>> -	unsigned long end_index = end >> PAGE_SHIFT;
>>>> -	struct folio *folio;
>>>> -	u32 len;
>>>> -
>>>> -	ASSERT(end + 1 - start <= U32_MAX);
>>>> -	len = end + 1 - start;
>>>> -	while (index <= end_index) {
>>>> -		folio = __filemap_get_folio(inode->vfs_inode.i_mapping, index, 0, 0);
>>>> -		ASSERT(!IS_ERR(folio)); /* folios should be in the extent_io_tree */
>>>> -
>>>> -		/* This is for data, which doesn't yet support larger folio. */
>>>> -		ASSERT(folio_order(folio) == 0);
>>>> -		btrfs_folio_set_writeback(fs_info, folio, start, len);
>>>
>>> So the new code is just btrfs_folio_set_writeback(), with the removed
>>> comment and assertion,
>>
>> Firstly, the length check is already inside btrfs_folio_set_writeback()
>> for the subpage cases.
>> If it's not subpage, we do not even need to check the range (it's always
>> page aligned).
>>
>> Secondly for the folio, we do not need the ASSERT(), because this time
>> we have the folio pointer already.
>>
>> So for the assert part, there is no change.
>
> Ok.
>
>>> what's the status regarding large folios?
>>
>> That stays the same, no larger folio support.
>>
>> The larger folio support requires us to get rid of the per-fs
>> sectors_per_page check, but using folio_size() to do the calculation.
>>
>> That will still be a lot of work to do before we can support larger
>> folios for data.
>
> My question was about this specific place in the code, if we e.g. remove
> various assertions making sure we don't accidentally get there with
> large folios after they get enabled in the future. It's ok when other
> code makes different checks that would prevent it, but that's what I did
> not immediately see. Thanks.

Oh, in that case we have quite some such checks already in place.

For this particular case (buffered write path), we have ASSERT()
checking for the folio order at end_bbio_data_read(), the same applies
to the buffered read path too:

	ASSERT(folio_order(folio) == 0);

So it's still fine, although a little late (at endio time vs at
submission time).

Furthermore if we're going subpage, btrfs_subpage_assert() also does the
folio order check.

So we should get the ASSERT()s triggered pretty easily if there is a
larger folio passed in.

Thanks,
Qu