Matthew Wilcox <willy@xxxxxxxxxxxxx> writes:
> On Mon, Jun 19, 2023 at 07:58:51AM +0530, Ritesh Harjani (IBM) wrote:
>> +static void ifs_calc_range(struct folio *folio, size_t off, size_t len,
>> + enum iomap_block_state state, unsigned int *first_blkp,
>> + unsigned int *nr_blksp)
>> +{
>> + struct inode *inode = folio->mapping->host;
>> + unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
>> + unsigned int first = off >> inode->i_blkbits;
>> + unsigned int last = (off + len - 1) >> inode->i_blkbits;
>> +
>> + *first_blkp = first + (state * blks_per_folio);
>> + *nr_blksp = last - first + 1;
>> +}
>
> As I said, this is not 'first_blkp'. It's first_bitp. I think this
> misunderstanding is related to Andreas' complaint, but it's not quite
> the same.
>
We represent each FS block as a bit in the bitmap. So first_blkp or
first_bitp or first_blkbitp essentially means the same.
I went with first_blk, first_blkp in the first place based on your
suggestion itself [1].
[1]: https://lore.kernel.org/linux-xfs/Y%2FvxlVUJ31PZYaRa@xxxxxxxxxxxxxxxxxxxx/
>> static inline bool ifs_is_fully_uptodate(struct folio *folio,
>> struct iomap_folio_state *ifs)
>> {
>> struct inode *inode = folio->mapping->host;
>> + unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
>> + unsigned int nr_blks = (IOMAP_ST_UPTODATE + 1) * blks_per_folio;
>>
>> - return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
>> + return bitmap_full(ifs->state, nr_blks);
>
> I think we have a gap in our bitmap APIs. We don't have a
> 'bitmap_range_full(src, pos, nbits)'. We could use find_next_zero_bit(),
> but that's going to do more work than necessary.
>
> Given this lack, perhaps it's time to say that you're making all of
> this too hard by using an enum, and pretending that we can switch the
> positions of 'uptodate' and 'dirty' in the bitmap just by changing
> the enum.
Actually I never wanted to use the the enum this way. That's why I was
not fond of the idea behind using enum in all the bitmap state
manipulation APIs (test/set/).
It was only intended to be passed as a state argument to ifs_calc_range()
function to keep all the first_blkp and nr_blksp calculation at one
place. And just use it's IOMAP_ST_MAX value while allocating state bitmap.
It was never intended to be used like this.
We can even now go back to this original idea and keep the use of the
enum limited to what I just mentioned above i.e. for ifs_calc_range().
And maybe just use this in ifs_alloc()?
BUILD_BUG_ON(IOMAP_ST_UPTODATE == 0);
BUILD_BUG_ON(IOMAP_ST_DIRTY == 1);
> Define the uptodate bits to be the first ones in the bitmap,
> document it (and why), and leave it at that.
Do you think we can go with above suggestion, or do you still think we
need to drop it?
In case if we drop it, then should we open code the calculations for
first_blk, last_blk? These calculations are done in exact same fashion
at 3 places ifs_set_range_uptodate(), ifs_clear_range_dirty() and
ifs_set_range_dirty().
Thoughts?
-ritesh
