On Mon, May 08, 2023 at 12:58:00AM +0530, Ritesh Harjani (IBM) wrote:
> +static inline void iop_clear_range(struct iomap_page *iop,
> + unsigned int start_blk, unsigned int nr_blks)
> +{
> + bitmap_clear(iop->state, start_blk, nr_blks);
> +}
Similar to the other trivial bitmap wrappers added earlier in the
series I don't think this one is very useful.
> + struct iomap_page *iop = to_iomap_page(folio);
> + 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;
> + unsigned long flags;
> +
> + if (!iop)
> + return;
> + spin_lock_irqsave(&iop->state_lock, flags);
> + iop_set_range(iop, first_blk + blks_per_folio, nr_blks);
> + spin_unlock_irqrestore(&iop->state_lock, flags);
All the variable initializations except for ios should really move
into a branch here. Or we just use separate helpers for the case
where we actually have an iops and isolate all that, which would
be my preference (but goes counter to the direction of changes
earlier in the series to the existing functions).
> +static void iop_clear_range_dirty(struct folio *folio, size_t off, size_t len)
> +{
> + struct iomap_page *iop = to_iomap_page(folio);
> + 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;
> + unsigned long flags;
> +
> + if (!iop)
> + return;
> + spin_lock_irqsave(&iop->state_lock, flags);
> + iop_clear_range(iop, first_blk + blks_per_folio, nr_blks);
> + spin_unlock_irqrestore(&iop->state_lock, flags);
> +}
Same here.
