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

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

Christoph Hellwig <hch@infradead.org> writes:

>> +static inline bool iomap_iof_is_block_dirty(struct folio *folio,
>> +			struct iomap_folio *iof, int block)
>
> Two tabs indents here please and for various other functions.
>

Sure. 

>> +{
>> +	struct inode *inode = folio->mapping->host;
>> +	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
>> +	unsigned int first_blk = off >> inode->i_blkbits;
>> +	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
>> +	unsigned int nr_blks = last_blk - first_blk + 1;
>
> Given how many places we we opencode this logic I wonder if a helper
> would be usefuļ, even if the calling conventions are a bit odd.

I agree that moving it to a common helper would come useful as it is
open coded at 3 places.

>
> To make this nicer it would also be good an take a neat trick from
> the btrfs subpage support and use an enum for the bits, e.g.:
>
> enum iomap_block_state {
> 	IOMAP_ST_UPTODATE,
> 	IOMAP_ST_DIRTY,
>
> 	IOMAP_ST_MAX,
> };
>

I think the only remaining piece is the naming of this enum and struct
iomap_page.

Ok, so here is what I think my preference would be -

This enum perfectly qualifies for "iomap_block_state" as it holds the
state of per-block.
Then the struct iomap_page (iop) should be renamed to struct
"iomap_folio_state" (ifs), because that holds the state information of all the
blocks within a folio.

Is this something which sounds ok to others too? 

>
> static void iomap_ibs_calc_range(struct folio *folio, size_t off, size_t len,
> 		enum iomap_block_state state, unsigned int *first_blk,
> 		unsigned int *nr_blks)
> {
> 	struct inode *inode = folio->mapping->host;
> 	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> 	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
>
> 	*first_blk = state * blks_per_folio + (off >> inode->i_blkbits);
> 	*nr_blks = last_blk - first_blk + 1;
> }
>

Sure, like the idea of the state enum. Will make the changes.

>> +	/*
>> +	 * iof->state tracks two sets of state flags when the
>> +	 * filesystem block size is smaller than the folio size.
>> +	 * The first state tracks per-block uptodate and the
>> +	 * second tracks per-block dirty state.
>> +	 */
>> +	iof = kzalloc(struct_size(iof, state, BITS_TO_LONGS(2 * nr_blocks)),
>>  		      gfp);
>
> with the above this can use IOMAP_ST_MAX and make the whole thing a
> little more robus.
>

yes.

>>
>>  	if (iof) {
>
> No that this adds a lot more initialization I'd do a
>
> 	if (!iof)
> 		return NULL;
>
> here and unindent the rest.

Sure. Is it ok to fold this change in the same patch, right?
Or does it qualify for a seperate patch?

-ritesh