Re: [PATCHv11 8/8] iomap: Add per-block dirty state tracking to improve performance

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

"Ritesh Harjani (IBM)" <ritesh.list@gmail.com> writes:

> @@ -1637,7 +1758,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
>  		struct writeback_control *wbc, struct inode *inode,
>  		struct folio *folio, u64 end_pos)
>  {
> -	struct iomap_folio_state *ifs = ifs_alloc(inode, folio, 0);
> +	struct iomap_folio_state *ifs = folio->private;
>  	struct iomap_ioend *ioend, *next;
>  	unsigned len = i_blocksize(inode);
>  	unsigned nblocks = i_blocks_per_folio(inode, folio);
> @@ -1645,6 +1766,11 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
>  	int error = 0, count = 0, i;
>  	LIST_HEAD(submit_list);
>  
> +	if (!ifs && nblocks > 1) {
> +		ifs = ifs_alloc(inode, folio, 0);
> +		iomap_set_range_dirty(folio, 0, folio_size(folio));
> +	}
> +
>  	WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) != 0);
>  
>  	/*
> @@ -1653,7 +1779,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
>  	 * invalid, grab a new one.
>  	 */
>  	for (i = 0; i < nblocks && pos < end_pos; i++, pos += len) {
> -		if (ifs && !ifs_block_is_uptodate(ifs, i))
> +		if (ifs && !ifs_block_is_dirty(folio, ifs, i))
>  			continue;
>  
>  		error = wpc->ops->map_blocks(wpc, inode, pos);
> @@ -1697,6 +1823,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
>  		}
>  	}
>  
> +	iomap_clear_range_dirty(folio, 0, end_pos - folio_pos(folio));
>  	folio_start_writeback(folio);
>  	folio_unlock(folio);
>  

I think we should fold below change with this patch. 
end_pos is calculated in iomap_do_writepage() such that it is either
folio_pos(folio) + folio_size(folio), or if this value becomes more then
isize, than end_pos is made isize.

The current patch does not have a functional problem I guess. But in
some cases where truncate races with writeback, it will end up marking
more bits & later doesn't clear those. Hence I think we should correct
it using below diff.

I have added a WARN_ON_ONCE, but if you think it is obvious and not
required, feel free to drop it.

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 2fd9413838de..6c03e5842d44 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1766,9 +1766,11 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
        int error = 0, count = 0, i;
        LIST_HEAD(submit_list);

+       WARN_ON_ONCE(end_pos <= pos);
+
        if (!ifs && nblocks > 1) {
                ifs = ifs_alloc(inode, folio, 0);
-               iomap_set_range_dirty(folio, 0, folio_size(folio));
+               iomap_set_range_dirty(folio, 0, end_pos - pos);
        }

        WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) != 0);

-ritesh