On Thu 01-12-22 16:43:59, Ritesh Harjani (IBM) wrote:
> On 22/11/30 05:35PM, Jan Kara wrote:
> > Add support for calls to ext4_writepages() than cannot map blocks. These
> > will be issued from jbd2 transaction commit code.
>
> I guess we should expand the description of mpage_prepare_extent_to_map()
> function now. Other than that the patch looks good to me.
>
> Please feel free to add:
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
Thanks for review!
> > /*
> > * mpage_prepare_extent_to_map - find & lock contiguous range of dirty pages
> > * and underlying extent to map
>
> Since we are overloading this function. this can be also called with can_map
> as 0. Maybe good to add some description around that?
Well, it was somewhat overloaded already before but you're right some
documentation update is in order :) I'll do that.
> > @@ -2651,14 +2665,30 @@ static int mpage_prepare_extent_to_map(struct mpage_da_data *mpd)
>
>
> adding context of code so that it doesn't get missed in the discussion.
>
> <...>
> /* If we can't merge this page, we are done. */
> if (mpd->map.m_len > 0 && mpd->next_page != page->index)
> goto out;
>
> I guess this also will not hold for us given we will always have m_len to be 0.
> <...>
Correct.
> > + /*
> > + * Writeout for transaction commit where we cannot
> > + * modify metadata is simple. Just submit the page.
> > + */
> > + if (!mpd->can_map) {
> > + if (ext4_page_nomap_can_writeout(page)) {
> > + err = mpage_submit_page(mpd, page);
> > + if (err < 0)
> > + goto out;
> > + } else {
> > + unlock_page(page);
> > + mpd->first_page++;
>
> We anyway should always have mpd->map.m_len = 0.
> That means, we always set mpd->first_page = page->index above.
> So this might not be useful. But I guess for consistency of the code,
> or to avoid any future bugs, this isn't harmful to keep.
Yes, it is mostly for consistency but it is also needed so that once we
exit the loop, mpage_release_unused_pages() starts working from a correct
page.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
