Christoph Hellwig <hch@xxxxxxxxxxxxx> 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
