Re: [RFCv2 3/3] iomap: Support subpage size dirty tracking to improve write performance

["Ritesh Harjani (IBM)" <ritesh.list@gmail.com>]

On 23/01/30 06:01PM, Matthew Wilcox wrote:
> On Mon, Jan 30, 2023 at 09:16:33AM -0800, Christoph Hellwig wrote:
> > > +		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.
>
> And I don't think this 'from_writeback' argument is well-named.
> Presumably it's needed because folio_test_dirty() will be false
> at this point in the writeback path because it got cleared by the VFS?

Yes, folio_test_dirty() is false. We clear it in write_cache_pages() by calling
clear_page_dirty_for_io() before calling iomap_do_writepage().

> But in any case, it should be called 'dirty' or something, not tell me
> where the function was called from.  I think what this should really
> do is:
>
> 		if (dirty)
> 			iop_set_dirty(iop, 0, nr_blocks);
> 		if (folio_test_uptodate(folio))
> 			iop_set_uptodate(iop, 0, nr_blocks);

Sure I got the idea. I will use "bool is_dirty".

>
> > > +			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.
>
> You are not missing anything.  I don't understand the mental process
> that gets someone to writing that.  It should logically be 0.

Sorry about the confusion. Yes, that is correct. I ended up using above at some
place and 0 at others. Then for final cleanup I ended up using the above call.

I will correct it in the next rev.

>
> > 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.
>
> hahaha, you think the same.  ok, i'm leaving my above comment though ;-)
>
No problem ;)

Thanks for your review!!
-ritesh