Re: [RFCv2 3/3] iomap: Support subpage size dirty tracking to improve write performance

[Christoph Hellwig <hch@infradead.org>]

On Mon, Jan 30, 2023 at 09:44:13PM +0530, Ritesh Harjani (IBM) wrote:
> +iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags,
> +		  bool from_writeback)
>  {
>  	struct iomap_page *iop = to_iomap_page(folio);
>  	unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
> @@ -58,12 +59,32 @@ iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags)
>  	else
>  		gfp = GFP_NOFS | __GFP_NOFAIL;
>  
> -	iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(nr_blocks)),
> +	iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(2 * nr_blocks)),
>  		      gfp);
>  	if (iop) {

Please just return early here for the allocation failure case instead of 
adding a lot of code with extra indentation.

>  		spin_lock_init(&iop->state_lock);
> -		if (folio_test_uptodate(folio))
> -			bitmap_fill(iop->state, nr_blocks);
> +		/*
> +		 * iomap_page_create can get called from writeback after
> +		 * a truncate_inode_partial_folio operation on a large folio.
> +		 * For large folio the iop structure is freed in
> +		 * iomap_invalidate_folio() to ensure we can split the folio.
> +		 * That means we will have to let go of the optimization of
> +		 * tracking dirty bits here and set all bits as dirty if
> +		 * the folio is marked uptodate.
> +		 */
> +		if (from_writeback && folio_test_uptodate(folio))
> +			bitmap_fill(iop->state, 2 * nr_blocks);
> +		else if (folio_test_uptodate(folio)) {

This code is very confusing.  First please only check
folio_test_uptodate one, and then check the from_writeback flag
inside the branch.  And as mentioned last time I think you really
need some symbolic constants for dealing with dirty vs uptodate
state and not just do a single fill for them.

> +			unsigned start = offset_in_folio(folio,
> +					folio_pos(folio)) >> inode->i_blkbits;
> +			bitmap_set(iop->state, start, nr_blocks);

Also this code leaves my head scratching.  Unless I'm missing something
important

	 offset_in_folio(folio, folio_pos(folio))

must always return 0.

Also the from_writeback logic is weird.  I'd rather have a
"bool is_dirty" argument and then pass true for writeback beause
we know the folio is dirty, false where we know it can't be
dirty and do the folio_test_dirty in the caller where we don't
know the state.

> +bool iomap_dirty_folio(struct address_space *mapping, struct folio *folio)
> +{
> +	unsigned int nr_blocks = i_blocks_per_folio(mapping->host, folio);
> +	struct iomap_page *iop = iomap_page_create(mapping->host, folio, 0, false);

Please avoid the overly long line.  In fact with such long function
calls I'd generally prefer if the initialization was moved out of the
declaration.

> +
> +	iomap_set_range_dirty(folio, iop, offset_in_folio(folio, folio_pos(folio)),

Another overly long line here.