Re: [PATCH 5/9] ext4: Add support for writepages calls that cannot map blocks

[Jan Kara <jack@suse.cz>]

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@gmail.com>

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@suse.com>
SUSE Labs, CR