linux-kernel - Re: [PATCH v17 03/14] shmem: Implement splice-read

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZBDUUAGO9HbZG8EW@casper.infradead.org>
Date:   Tue, 14 Mar 2023 20:08:48 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     David Howells <dhowells@...hat.com>, Jens Axboe <axboe@...nel.dk>,
        Al Viro <viro@...iv.linux.org.uk>,
        Christoph Hellwig <hch@...radead.org>, Jan Kara <jack@...e.cz>,
        Jeff Layton <jlayton@...nel.org>,
        David Hildenbrand <david@...hat.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Logan Gunthorpe <logang@...tatee.com>,
        Hillf Danton <hdanton@...a.com>, linux-fsdevel@...r.kernel.org,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Daniel Golle <daniel@...rotopia.org>,
        Guenter Roeck <groeck7@...il.com>,
        Christoph Hellwig <hch@....de>,
        John Hubbard <jhubbard@...dia.com>,
        Hugh Dickins <hughd@...gle.com>
Subject: Re: [PATCH v17 03/14] shmem: Implement splice-read

On Tue, Mar 14, 2023 at 11:02:40AM -0700, Linus Torvalds wrote:
> On Tue, Mar 14, 2023 at 9:43 AM Matthew Wilcox <willy@...radead.org> wrote:
> >
> > The problem is that we might have swapped out the shmem folio.  So we
> > don't want to clear the page, but ask swap to fill the page.
> 
> Doesn't shmem_swapin_folio() already basically do all that work?
> 
> The real oddity with shmem - compared to other filesystems - is that
> the xarray has a value entry instead of being a real folio. And yes,
> the current filemap code will then just ignore such entries as
> "doesn't exist", and so the regular read iterators will all fail on
> it.
> 
> But while filemap_get_read_batch() will stop at a value-folio, I feel
> like filemap_create_folio() should be able to turn a value page into a
> "real" page. Right now it already allocates said page, but then I
> think filemap_add_folio() will return -EEXIST when said entry exists
> as a value.
> 
> But *if* instead of -EEXIST we could just replace the value with the
> (already locked) page, and have some sane way to pass that value
> (which is the swap entry data) to readpage(), I think that should just
> do it all.

This was basically what I had in mind:

I don't think this will handle a case like:

Alloc order-0 folio at index 4
Alloc order-0 folio at index 7
Swap out both folios
Alloc order-9 folio at indices 0-511

But I don't see where shmem currently handles that either.  Maybe it
falls back to order-0 folios instead of the crude BUG_ON I put in.
Hugh?

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index 82c1262f396f..30f2502314de 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -114,12 +114,6 @@ int shmem_get_folio(struct inode *inode, pgoff_t index, struct folio **foliop,
 struct folio *shmem_read_folio_gfp(struct address_space *mapping,
 		pgoff_t index, gfp_t gfp);
 
-static inline struct folio *shmem_read_folio(struct address_space *mapping,
-		pgoff_t index)
-{
-	return shmem_read_folio_gfp(mapping, index, mapping_gfp_mask(mapping));
-}
-
 static inline struct page *shmem_read_mapping_page(
 				struct address_space *mapping, pgoff_t index)
 {
diff --git a/mm/filemap.c b/mm/filemap.c
index 57c1b154fb5a..8e4f95c5b65a 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -877,6 +877,8 @@ noinline int __filemap_add_folio(struct address_space *mapping,
 					order, gfp);
 		xas_lock_irq(&xas);
 		xas_for_each_conflict(&xas, entry) {
+			if (old)
+				BUG_ON(shmem_mapping(mapping));
 			old = entry;
 			if (!xa_is_value(entry)) {
 				xas_set_err(&xas, -EEXIST);
@@ -885,7 +887,12 @@ noinline int __filemap_add_folio(struct address_space *mapping,
 		}
 
 		if (old) {
-			if (shadowp)
+			if (shmem_mapping(mapping)) {
+				folio_set_swap_entry(folio,
+						radix_to_swp_entry(old));
+				folio_set_swapcache(folio);
+				folio_set_swapbacked(folio);
+			} else if (shadowp)
 				*shadowp = old;
 			/* entry may have been split before we acquired lock */
 			order = xa_get_order(xas.xa, xas.xa_index);
diff --git a/mm/shmem.c b/mm/shmem.c
index 8e60826e4246..ea75c7dcf5ec 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2059,6 +2059,18 @@ int shmem_get_folio(struct inode *inode, pgoff_t index, struct folio **foliop,
 			mapping_gfp_mask(inode->i_mapping), NULL, NULL, NULL);
 }
 
+static int shmem_read_folio(struct file *file, struct folio *folio)
+{
+	if (folio_test_swapcache(folio)) {
+		swap_readpage(&folio->page, true, NULL);
+	} else {
+		folio_zero_segment(folio, 0, folio_size(folio));
+		folio_mark_uptodate(folio);
+		folio_unlock(folio);
+	}
+	return 0;
+}
+
 /*
  * This is like autoremove_wake_function, but it removes the wait queue
  * entry unconditionally - even if something else had already woken the
@@ -2396,7 +2408,8 @@ static int shmem_fadvise_willneed(struct address_space *mapping,
 	xa_for_each_range(&mapping->i_pages, index, folio, start, end) {
 		if (!xa_is_value(folio))
 			continue;
-		folio = shmem_read_folio(mapping, index);
+		folio = shmem_read_folio_gfp(mapping, index,
+						mapping_gfp_mask(mapping));
 		if (!IS_ERR(folio))
 			folio_put(folio);
 	}
@@ -4037,6 +4050,7 @@ static int shmem_error_remove_page(struct address_space *mapping,
 }
 
 const struct address_space_operations shmem_aops = {
+	.read_folio	= shmem_read_folio,
 	.writepage	= shmem_writepage,
 	.dirty_folio	= noop_dirty_folio,
 #ifdef CONFIG_TMPFS