Christoph Hellwig <hch@xxxxxxxxxxxxx> writes:
> 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.
Branch won't be needed I guess, will just move the initialization after
the return.
> 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.
Sure.
-ritesh
