Re: [PATCH] btrfs: remove btrfs_set_range_writeback()

[David Sterba <dsterba@suse.cz>]

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.