linux-ext4 - Re: [PATCH 10/31] ext4: Convert ext4_convert_inline_data_to

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: <ZByJD2ufs9hM5usF@casper.infradead.org>
Date:   Thu, 23 Mar 2023 17:14:55 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Andreas Dilger <adilger.kernel@...ger.ca>,
        linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 10/31] ext4: Convert ext4_convert_inline_data_to_extent()
 to use a folio

On Tue, Mar 14, 2023 at 06:36:21PM -0400, Theodore Ts'o wrote:
> On Thu, Jan 26, 2023 at 08:23:54PM +0000, Matthew Wilcox (Oracle) wrote:
> > Saves a number of calls to compound_head().
> 
> Is this left over from an earlier version of this patch series?  There
> are no changes to calls to compound_head() that I can find in this
> patch.

They're hidden.  Here are the ones from this patch:

-       if (!PageUptodate(page)) {
-               unlock_page(page);
-               put_page(page);
-               unlock_page(page);
-               put_page(page);

That's five.  I may have missed some.

> > @@ -565,10 +564,9 @@ static int ext4_convert_inline_data_to_extent(struct address_space *mapping,
> >  
> >  	/* We cannot recurse into the filesystem as the transaction is already
> >  	 * started */
> > -	flags = memalloc_nofs_save();
> > -	page = grab_cache_page_write_begin(mapping, 0);
> > -	memalloc_nofs_restore(flags);
> > -	if (!page) {
> > +	folio = __filemap_get_folio(mapping, 0, FGP_WRITEBEGIN | FGP_NOFS,
> > +			mapping_gfp_mask(mapping));
> > +	if (!folio) {
> >  		ret = -ENOMEM;
> >  		goto out;
> >  	}
> 
> Is there a reason why to use FGP_NOFS as opposed to using
> memalloc_nofs_{save,restore}()?
> 
> I thought using memalloc_nofs_save() is considered the perferred
> approach by mm-folks.

Ideally, yes, we'd use memalloc_nofs_save(), but not like this!  The way
it's supposed to be used is at the point where you do something which
makes the fs non-reentrant.  ie when you start the transaction, you should
be calling memalloc_nofs_save() and when you finish the transaction,
you should be calling memalloc_nofs_restore().  That way, you don't
need to adorn the entire filesystem with GFP_NOFS/FGP_NOFS/whatever,
you have one place where you mark yourself non-reentrant and you're done.

Once ext4 does this every time it starts a transaction, we can drop
the FGP_NOFS flag usage in ext4, and once every filesystem does it,
we can drop the entire flag, and that will make me happy.  It's a long
road, though.