On Sat 21-06-25 12:42:11, Zhang Yi wrote:
> On 2025/6/20 23:21, Jan Kara wrote:
> > I suspect you thought that the failure to extend transaction in the middle
> > of order 0 folio should not happen because you reserve credits for full
> > page worth of writeback? But those credits could be exhaused by the time we
> > get to mapping third folio because mpage_map_one_extent() only ensures
> > there are credits for mapping one extent.
>
> Ooops, you are right. Sorry, it was my mistake.
>
> >
> > And I think reserving credits for just one extent is fine even from the
> > beginning (as I wrote in my comment to patch 4). We just need to handle
> > this partial case
>
> Yeah.
>
> > which should be possible by just leaving
> > mpd->first_page untouched and leave unlocking to
> > mpage_release_unused_pages().
>
> I was going to use this solution, but it breaks the semantics of the
> mpage_release_unused_pages() and trigger BUG_ON(folio_test_writeback(folio))
> in this function. I don't want to drop this BUG_ON since I think it's
> somewhat useful.
Oh, right. Well, we could modify the BUG_ON to:
/*
* first_page folio can be under writeback if we need to restart
* transaction to map more
*/
BUG_ON((invalidate || folio->inode > mpd->first_page) && folio_test_writeback(folio));
> > But I can be missing some effects, the writeback code is really complex...
>
> Indeed, I was confused by this code for a long time. Thank you a lot for
> patiently correcting my mistakes in my patch.
Thank you for taking time to properly fix these issues :)
> > BTW long-term the code may be easier to follow if we replace
> > mpd->first_page and mpd->next_page with logical block based or byte based
> > indexing. Now when we have large order folios, page is not that important
> > concept for writeback anymore.
>
> I suppose we should do this conversion now.
Yes or this... I guess it would be more obvious what's going on this way.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
