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?
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);
> > + 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.
> 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 ;-)
