linux-kernel - Re: [PATCH v3] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache

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: <1619343.1669233783@warthog.procyon.org.uk>
Date:   Wed, 23 Nov 2022 20:03:03 +0000
From:   David Howells <dhowells@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     dhowells@...hat.com, willy@...radead.org, dwysocha@...hat.com,
        Rohith Surabattula <rohiths.msft@...il.com>,
        Steve French <sfrench@...ba.org>,
        Shyam Prasad N <nspmangalore@...il.com>,
        Dominique Martinet <asmadeus@...ewreck.org>,
        Ilya Dryomov <idryomov@...il.com>, linux-cachefs@...hat.com,
        linux-cifs@...r.kernel.org, linux-afs@...ts.infradead.org,
        v9fs-developer@...ts.sourceforge.net, ceph-devel@...r.kernel.org,
        linux-nfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] mm, netfs, fscache: Stop read optimisation when folio removed from pagecache

Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> But I also think it's strange in another way, with that odd placement of
> 
>         mapping_clear_release_always(inode->i_mapping);
> 
> at inode eviction time. That just feels very random.

I was under the impression that a warning got splashed if unexpected
address_space flags were set when ->evict_inode() returned.  I may be thinking
of page flags.  If it doesn't, fine, this isn't required.

> Similarly, that change to shrink_folio_list() looks strange, with the
> nasty folio_needs_release() helper. It seems entirely pointless, with
> the use then being
> 
>                 if (folio_needs_release(folio)) {
>                         if (!filemap_release_folio(folio, sc->gfp_mask))
>                                 goto activate_locked;

Unfortunately, that can't be simply folded down.  It actually does something
extra if folio_has_private() was set, filemap_release_folio() succeeds but
there was no mapping:

		 * Rarely, folios can have buffers and no ->mapping.
		 * These are the folios which were not successfully
		 * invalidated in truncate_cleanup_folio().  We try to
		 * drop those buffers here and if that worked, and the
		 * folio is no longer mapped into process address space
		 * (refcount == 1) it can be freed.  Otherwise, leave
		 * the folio on the LRU so it is swappable.

Possibly I could split the if-statement and make it two separate cases:

		/*
		 * If the folio has buffers, try to free the buffer
		 * mappings associated with this folio. If we succeed
		 * we try to free the folio as well.
		 *
		 * We do this even if the folio is dirty.
		 * filemap_release_folio() does not perform I/O, but it
		 * is possible for a folio to have the dirty flag set,
		 * but it is actually clean (all its buffers are clean).
		 * This happens if the buffers were written out directly,
		 * with submit_bh(). ext3 will do this, as well as
		 * the blockdev mapping.  filemap_release_folio() will
		 * discover that cleanness and will drop the buffers
		 * and mark the folio clean - it can be freed.
		 */
		if (!filemap_release_folio(folio, sc->gfp_mask))
			goto activate_locked;

filemap_release_folio() will return true if folio_has_private() is false,
which would allow us to reach the next part, which we would then skip.

		/*
		 * Rarely, folios can have buffers and no ->mapping.
		 * These are the folios which were not successfully
		 * invalidated in truncate_cleanup_folio().  We try to
		 * drop those buffers here and if that worked, and the
		 * folio is no longer mapped into process address space
		 * (refcount == 1) it can be freed.  Otherwise, leave
		 * the folio on the LRU so it is swappable.
		 */
		if (!mapping && folio_has_private(folio) &&
		    folio_ref_count(folio) == 1) {
			folio_unlock(folio);
			if (folio_put_testzero(folio))
				goto free_it;
			 /*
			  * rare race with speculative reference.
			  * the speculative reference will free
			  * this folio shortly, so we may
			  * increment nr_reclaimed here (and
			  * leave it off the LRU).
			  */
			nr_reclaimed += nr_pages;
			continue;
		}

But that will malfunction if try_to_free_buffers(), as called from
folio_has_private(), manages to clear the private bits.  I wonder if it might
be possible to fold this bit into filemap_release_folio() somehow.

I really need a three-state return from filemap_release_folio() - maybe:

	0	couldn't release
	1	released
	2	there was no private

The ordinary "if (filemap_release_folio()) { ... }" would work as expected.
shrink_folio_list() could do something different between case 1 and case 2.

> And the change to mm/filemap.c is completely unacceptable in all
> forms, and this added test
> 
> +       if ((!mapping || !mapping_release_always(mapping)) &&
> +           !folio_test_private(folio) &&
> +           !folio_test_private_2(folio))
> +               return true;
> 
> will not be accepted even during the merge window. That code makes no
> sense what-so-ever, and is in no way acceptable.
>
> That code makes no sense what-so-ever. Why isn't it using
> "folio_has_private()"?

It should be, yes.

> Why is this done as an open-coded - and *badly* so - version of
> !folio_needs_release() that you for some reason made private to mm/vmscan.c?

Yeah, in retrospect, I should have put that in mm/internal.h.

David