Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Dec 2022 12:50:19 +0100
From:   Jan Kara <jack@suse.cz>
To:     "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Cc:     Jan Kara <jack@suse.cz>, Ted Tso <tytso@mit.edu>,
        linux-ext4@vger.kernel.org, Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 5/9] ext4: Add support for writepages calls that cannot
 map blocks
Message-ID: <20221201115019.jot525ry25gk4ggh@quack3>
References: <20221130162435.2324-1-jack@suse.cz>
 <20221130163608.29034-5-jack@suse.cz>
 <20221201111359.onr5edsaaxcr2ndh@riteshh-domain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221201111359.onr5edsaaxcr2ndh@riteshh-domain>
Precedence: bulk

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