On Mon, Jan 30, 2023 at 09:44:13PM +0530, Ritesh Harjani (IBM) wrote:
> +iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags,
> + bool from_writeback)
> {
> struct iomap_page *iop = to_iomap_page(folio);
> unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
> @@ -58,12 +59,32 @@ iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags)
> else
> gfp = GFP_NOFS | __GFP_NOFAIL;
>
> - iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(nr_blocks)),
> + iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(2 * nr_blocks)),
> gfp);
> if (iop) {
Please just return early here for the allocation failure case instead of
adding a lot of code with extra indentation.
> spin_lock_init(&iop->state_lock);
> - if (folio_test_uptodate(folio))
> - bitmap_fill(iop->state, nr_blocks);
> + /*
> + * iomap_page_create can get called from writeback after
> + * a truncate_inode_partial_folio operation on a large folio.
> + * For large folio the iop structure is freed in
> + * iomap_invalidate_folio() to ensure we can split the folio.
> + * That means we will have to let go of the optimization of
> + * tracking dirty bits here and set all bits as dirty if
> + * the folio is marked uptodate.
> + */
> + if (from_writeback && folio_test_uptodate(folio))
> + bitmap_fill(iop->state, 2 * nr_blocks);
> + else if (folio_test_uptodate(folio)) {
This code is very confusing. First please only check
folio_test_uptodate one, and then check the from_writeback flag
inside the branch. And as mentioned last time I think you really
need some symbolic constants for dealing with dirty vs uptodate
state and not just do a single fill for them.
> + unsigned start = offset_in_folio(folio,
> + folio_pos(folio)) >> inode->i_blkbits;
> + bitmap_set(iop->state, start, nr_blocks);
Also this code leaves my head scratching. Unless I'm missing something
important
offset_in_folio(folio, folio_pos(folio))
must always return 0.
Also the from_writeback logic is weird. I'd rather have a
"bool is_dirty" argument and then pass true for writeback beause
we know the folio is dirty, false where we know it can't be
dirty and do the folio_test_dirty in the caller where we don't
know the state.
> +bool iomap_dirty_folio(struct address_space *mapping, struct folio *folio)
> +{
> + unsigned int nr_blocks = i_blocks_per_folio(mapping->host, folio);
> + struct iomap_page *iop = iomap_page_create(mapping->host, folio, 0, false);
Please avoid the overly long line. In fact with such long function
calls I'd generally prefer if the initialization was moved out of the
declaration.
> +
> + iomap_set_range_dirty(folio, iop, offset_in_folio(folio, folio_pos(folio)),
Another overly long line here.
