On 23/01/30 09:16AM, Christoph Hellwig wrote:
> 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.
Sure. Will do that.
>
> > 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
Ok, sure. I will try and simplify it.
> need some symbolic constants for dealing with dirty vs uptodate
> state and not just do a single fill for them.
Yes, I agree. Sorry my bad. I will add that change in the next rev.
>
> > + 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.
That is true always yes. In the next rev, I can make it explicitly 0 then
(maybe with just a small comment if required).
>
> 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.
Agreed. Thanks.
>
> > +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.
Sure, will do the same.
>
> > +
> > + iomap_set_range_dirty(folio, iop, offset_in_folio(folio, folio_pos(folio)),
>
> Another overly long line here.
sure. Will do the same.
-ritesh
