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
