Re: [PATCH] btrfs: remove btrfs_set_range_writeback()

[David Sterba <dsterba@suse.cz>]

I have more comments on the grammar than on the code, sorry.

On Sun, Oct 06, 2024 at 11:10:22AM +1030, Qu Wenruo wrote:
> The function btrfs_set_range_writeback() is originally a callback for
                                           was

> metadata and data, to mark a range with writeback flag.
> 
> Then it was converted into a common function call for both metadata and
> data.
> 
> >From the very beginning, the function is only called on a full page,
                                         'had been' because of the other past tense, idk

> later converted to handle range inside a page.
> 
> But it never needs to handle multiple pages, and since commit
               needed

> 8189197425e7 ("btrfs: refactor __extent_writepage_io() to do
> sector-by-sector submission") the function is only called on a
                                             has been

> sector-by-sector basis.
> 
> This makes the function unnecessary, and can be converted to a simple
> btrfs_folio_set_writeback() call instead.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/btrfs_inode.h |  1 -
>  fs/btrfs/extent_io.c   |  2 +-
>  fs/btrfs/inode.c       | 22 ----------------------
>  3 files changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index e152fde888fc..c514bab532fa 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -577,7 +577,6 @@ void btrfs_merge_delalloc_extent(struct btrfs_inode *inode, struct extent_state
>  				 struct extent_state *other);
>  void btrfs_split_delalloc_extent(struct btrfs_inode *inode,
>  				 struct extent_state *orig, u64 split);
> -void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end);
>  void btrfs_evict_inode(struct inode *inode);
>  struct inode *btrfs_alloc_inode(struct super_block *sb);
>  void btrfs_destroy_inode(struct inode *inode);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 9fbc83c76b94..d87dcafab537 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1359,7 +1359,7 @@ static int submit_one_sector(struct btrfs_inode *inode,
>  	 * a folio for a range already written to disk.
>  	 */
>  	btrfs_folio_clear_dirty(fs_info, folio, filepos, sectorsize);
> -	btrfs_set_range_writeback(inode, filepos, filepos + sectorsize - 1);
> +	btrfs_folio_set_writeback(fs_info, folio, filepos, sectorsize);
>  	/*
>  	 * Above call should set the whole folio with writeback flag, even
>  	 * just for a single subpage sector.
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 103ec917ca9d..21e51924742a 100644
> --- 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, what's the status regarding large folios?

I assume that this now implicitly supports them, we don't necessarily
need the assertion as I think we have more places where this would be
detected.