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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221201115019.jot525ry25gk4ggh@quack3>
Date:   Thu, 1 Dec 2022 12:50:19 +0100
From:   Jan Kara <jack@...e.cz>
To:     "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
Cc:     Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>,
        linux-ext4@...r.kernel.org, Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH 5/9] ext4: Add support for writepages calls that cannot
 map blocks

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