[PATCH 0/4] Squashfs: Update code to not use page->index

[PATCH 0/4] Squashfs: Update code to not use page->index

[Phillip Lougher]

In the near future page->index will be removed [1].  Any code which
still uses page->index needs to be updated.

This patch-set contains 4 patches which updates most of the code in
Squashfs.  The exceptions are functions which have been fixed in
recent patches [2] & [3].

Thanks

Phillip

[1]: https://lore.kernel.org/all/Zp8fgUSIBGQ1TN0D@casper.infradead.org/
[2]: https://lore.kernel.org/all/20240817101146.2347378-1-lizetao1@huawei.com/
[3]: https://lore.kernel.org/all/20240817101146.2347378-2-lizetao1@huawei.com/

[PATCH 1/4] Squashfs: Update page_actor to not use page->index

[Phillip Lougher]

This commit removes an unnecessary use of page->index,
and moves the other use over to folio->index.

Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
---
 fs/squashfs/file.c        |  2 +-
 fs/squashfs/file_direct.c |  3 ++-
 fs/squashfs/page_actor.c  | 11 ++++++++---
 fs/squashfs/page_actor.h  |  3 ++-
 4 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index a8c1e7f9a609..2b6b63f4ccd1 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -589,7 +589,7 @@ static void squashfs_readahead(struct readahead_control *ractl)
 			goto skip_pages;
 
 		actor = squashfs_page_actor_init_special(msblk, pages, nr_pages,
-							 expected);
+							expected, start);
 		if (!actor)
 			goto skip_pages;
 
diff --git a/fs/squashfs/file_direct.c b/fs/squashfs/file_direct.c
index 2a689ce71de9..0586e6ba94bf 100644
--- a/fs/squashfs/file_direct.c
+++ b/fs/squashfs/file_direct.c
@@ -67,7 +67,8 @@ int squashfs_readpage_block(struct page *target_page, u64 block, int bsize,
 	 * Create a "page actor" which will kmap and kunmap the
 	 * page cache pages appropriately within the decompressor
 	 */
-	actor = squashfs_page_actor_init_special(msblk, page, pages, expected);
+	actor = squashfs_page_actor_init_special(msblk, page, pages, expected,
+						start_index << PAGE_SHIFT);
 	if (actor == NULL)
 		goto out;
 
diff --git a/fs/squashfs/page_actor.c b/fs/squashfs/page_actor.c
index 81af6c4ca115..2b3e807d4dea 100644
--- a/fs/squashfs/page_actor.c
+++ b/fs/squashfs/page_actor.c
@@ -60,6 +60,11 @@ struct squashfs_page_actor *squashfs_page_actor_init(void **buffer,
 }
 
 /* Implementation of page_actor for decompressing directly into page cache. */
+static loff_t page_next_index(struct squashfs_page_actor *actor)
+{
+	return page_folio(actor->page[actor->next_page])->index;
+}
+
 static void *handle_next_page(struct squashfs_page_actor *actor)
 {
 	int max_pages = (actor->length + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -68,7 +73,7 @@ static void *handle_next_page(struct squashfs_page_actor *actor)
 		return NULL;
 
 	if ((actor->next_page == actor->pages) ||
-			(actor->next_index != actor->page[actor->next_page]->index)) {
+			(actor->next_index != page_next_index(actor))) {
 		actor->next_index++;
 		actor->returned_pages++;
 		actor->last_page = NULL;
@@ -103,7 +108,7 @@ static void direct_finish_page(struct squashfs_page_actor *actor)
 }
 
 struct squashfs_page_actor *squashfs_page_actor_init_special(struct squashfs_sb_info *msblk,
-	struct page **page, int pages, int length)
+	struct page **page, int pages, int length, loff_t start_index)
 {
 	struct squashfs_page_actor *actor = kmalloc(sizeof(*actor), GFP_KERNEL);
 
@@ -125,7 +130,7 @@ struct squashfs_page_actor *squashfs_page_actor_init_special(struct squashfs_sb_
 	actor->pages = pages;
 	actor->next_page = 0;
 	actor->returned_pages = 0;
-	actor->next_index = page[0]->index & ~((1 << (msblk->block_log - PAGE_SHIFT)) - 1);
+	actor->next_index = start_index >> PAGE_SHIFT;
 	actor->pageaddr = NULL;
 	actor->last_page = NULL;
 	actor->alloc_buffer = msblk->decompressor->alloc_buffer;
diff --git a/fs/squashfs/page_actor.h b/fs/squashfs/page_actor.h
index 97d4983559b1..c6d837f0e9ca 100644
--- a/fs/squashfs/page_actor.h
+++ b/fs/squashfs/page_actor.h
@@ -29,7 +29,8 @@ extern struct squashfs_page_actor *squashfs_page_actor_init(void **buffer,
 				int pages, int length);
 extern struct squashfs_page_actor *squashfs_page_actor_init_special(
 				struct squashfs_sb_info *msblk,
-				struct page **page, int pages, int length);
+				struct page **page, int pages, int length,
+				loff_t start_index);
 static inline struct page *squashfs_page_actor_free(struct squashfs_page_actor *actor)
 {
 	struct page *last_page = actor->last_page;
-- 
2.39.2

[PATCH 2/4] Squashfs: Update squashfs_readahead() to not use page->index

[Phillip Lougher]

This commit removes references to page->index in the pages returned
from __readahead_batch(), and instead uses the 'start' variable.

This does reveal a bug in the previous code in that 'start' was
not updated every time around the loop.  This is fixed in this
commit.

Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
---
 fs/squashfs/file.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index 2b6b63f4ccd1..50fe5a078b83 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -551,7 +551,6 @@ static void squashfs_readahead(struct readahead_control *ractl)
 		return;
 
 	for (;;) {
-		pgoff_t index;
 		int res, bsize;
 		u64 block = 0;
 		unsigned int expected;
@@ -570,13 +569,8 @@ static void squashfs_readahead(struct readahead_control *ractl)
 		if (readahead_pos(ractl) >= i_size_read(inode))
 			goto skip_pages;
 
-		index = pages[0]->index >> shift;
-
-		if ((pages[nr_pages - 1]->index >> shift) != index)
-			goto skip_pages;
-
-		if (index == file_end && squashfs_i(inode)->fragment_block !=
-						SQUASHFS_INVALID_BLK) {
+		if (start >> msblk->block_log == file_end &&
+				squashfs_i(inode)->fragment_block != SQUASHFS_INVALID_BLK) {
 			res = squashfs_readahead_fragment(pages, nr_pages,
 							  expected);
 			if (res)
@@ -584,7 +578,7 @@ static void squashfs_readahead(struct readahead_control *ractl)
 			continue;
 		}
 
-		bsize = read_blocklist(inode, index, &block);
+		bsize = read_blocklist(inode, start >> msblk->block_log, &block);
 		if (bsize == 0)
 			goto skip_pages;
 
@@ -602,7 +596,7 @@ static void squashfs_readahead(struct readahead_control *ractl)
 
 			/* Last page (if present) may have trailing bytes not filled */
 			bytes = res % PAGE_SIZE;
-			if (index == file_end && bytes && last_page)
+			if (start >> msblk->block_log == file_end && bytes && last_page)
 				memzero_page(last_page, bytes,
 					     PAGE_SIZE - bytes);
 
@@ -616,6 +610,8 @@ static void squashfs_readahead(struct readahead_control *ractl)
 			unlock_page(pages[i]);
 			put_page(pages[i]);
 		}
+
+		start += readahead_batch_length(ractl);
 	}
 
 	kfree(pages);
-- 
2.39.2

[PATCH 3/4] Squashfs: Update squashfs_readpage_block() to not use page->index

[Phillip Lougher]

This commit replaces references to page->index to folio->index or
their equivalent.

Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
---
 fs/squashfs/file_direct.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/squashfs/file_direct.c b/fs/squashfs/file_direct.c
index 0586e6ba94bf..646d4d421f99 100644
--- a/fs/squashfs/file_direct.c
+++ b/fs/squashfs/file_direct.c
@@ -23,15 +23,15 @@ int squashfs_readpage_block(struct page *target_page, u64 block, int bsize,
 	int expected)
 
 {
+	struct folio *folio = page_folio(target_page);
 	struct inode *inode = target_page->mapping->host;
 	struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
-
 	loff_t file_end = (i_size_read(inode) - 1) >> PAGE_SHIFT;
 	int mask = (1 << (msblk->block_log - PAGE_SHIFT)) - 1;
-	loff_t start_index = target_page->index & ~mask;
+	loff_t start_index = folio->index & ~mask;
 	loff_t end_index = start_index | mask;
 	int i, n, pages, bytes, res = -ENOMEM;
-	struct page **page;
+	struct page **page, *last_page;
 	struct squashfs_page_actor *actor;
 	void *pageaddr;
 
@@ -46,7 +46,7 @@ int squashfs_readpage_block(struct page *target_page, u64 block, int bsize,
 
 	/* Try to grab all the pages covered by the Squashfs block */
 	for (i = 0, n = start_index; n <= end_index; n++) {
-		page[i] = (n == target_page->index) ? target_page :
+		page[i] = (n == folio->index) ? target_page :
 			grab_cache_page_nowait(target_page->mapping, n);
 
 		if (page[i] == NULL)
@@ -75,7 +75,7 @@ int squashfs_readpage_block(struct page *target_page, u64 block, int bsize,
 	/* Decompress directly into the page cache buffers */
 	res = squashfs_read_data(inode->i_sb, block, bsize, NULL, actor);
 
-	squashfs_page_actor_free(actor);
+	last_page = squashfs_page_actor_free(actor);
 
 	if (res < 0)
 		goto mark_errored;
@@ -87,8 +87,8 @@ int squashfs_readpage_block(struct page *target_page, u64 block, int bsize,
 
 	/* Last page (if present) may have trailing bytes not filled */
 	bytes = res % PAGE_SIZE;
-	if (page[pages - 1]->index == end_index && bytes) {
-		pageaddr = kmap_local_page(page[pages - 1]);
+	if (end_index == file_end && last_page && bytes) {
+		pageaddr = kmap_local_page(last_page);
 		memset(pageaddr + bytes, 0, PAGE_SIZE - bytes);
 		kunmap_local(pageaddr);
 	}
-- 
2.39.2

[PATCH 4/4] Squashfs: Rewrite and update squashfs_readahead_fragment() to not use page->index

[Phillip Lougher]

The previous implementation lacked error checking (e.g. the bytes
returned by squashfs_fill_page() is not checked), and the use of
page->index could not be removed without substantially rewriting
the routine to use the page actor abstraction used elsewhere.

Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
---
 fs/squashfs/file.c | 66 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 16 deletions(-)

diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index 50fe5a078b83..5a3745e52025 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -494,39 +494,73 @@ static int squashfs_read_folio(struct file *file, struct folio *folio)
 }
 
 static int squashfs_readahead_fragment(struct page **page,
-	unsigned int pages, unsigned int expected)
+	unsigned int pages, unsigned int expected, loff_t start)
 {
 	struct inode *inode = page[0]->mapping->host;
 	struct squashfs_cache_entry *buffer = squashfs_get_fragment(inode->i_sb,
 		squashfs_i(inode)->fragment_block,
 		squashfs_i(inode)->fragment_size);
 	struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
-	unsigned int n, mask = (1 << (msblk->block_log - PAGE_SHIFT)) - 1;
-	int error = buffer->error;
+	int i, bytes, copied;
+	struct squashfs_page_actor *actor;
+	unsigned int offset;
+	void *addr;
+	struct page *last_page;
 
-	if (error)
+	if (buffer->error)
 		goto out;
 
-	expected += squashfs_i(inode)->fragment_offset;
+	actor = squashfs_page_actor_init_special(msblk, page, pages,
+							expected, start);
+	if (!actor)
+		goto out;
 
-	for (n = 0; n < pages; n++) {
-		unsigned int base = (page[n]->index & mask) << PAGE_SHIFT;
-		unsigned int offset = base + squashfs_i(inode)->fragment_offset;
+	squashfs_actor_nobuff(actor);
+	addr = squashfs_first_page(actor);
 
-		if (expected > offset) {
-			unsigned int avail = min_t(unsigned int, expected -
-				offset, PAGE_SIZE);
+	for (copied = offset = 0; offset < expected; offset += PAGE_SIZE) {
+		int avail = min_t(int, expected - offset, PAGE_SIZE);
 
-			squashfs_fill_page(page[n], buffer, offset, avail);
+		if (!IS_ERR(addr)) {
+			bytes = squashfs_copy_data(addr, buffer, offset +
+					squashfs_i(inode)->fragment_offset, avail);
+
+			if (bytes != avail)
+				goto failed;
 		}
 
-		unlock_page(page[n]);
-		put_page(page[n]);
+		copied += avail;
+		addr = squashfs_next_page(actor);
 	}
 
+	last_page = squashfs_page_actor_free(actor);
+
+	if (copied == expected) {
+		/* Last page (if present) may have trailing bytes not filled */
+		bytes = copied % PAGE_SIZE;
+		if (bytes && last_page)
+			memzero_page(last_page, bytes, PAGE_SIZE - bytes);
+
+		for (i = 0; i < pages; i++) {
+			flush_dcache_page(page[i]);
+			SetPageUptodate(page[i]);
+		}
+	}
+
+	for (i = 0; i < pages; i++) {
+		unlock_page(page[i]);
+		put_page(page[i]);
+	}
+
+	squashfs_cache_put(buffer);
+	return 0;
+
+failed:
+	squashfs_page_actor_free(actor);
+
 out:
 	squashfs_cache_put(buffer);
-	return error;
+	return 1;
 }
 
 static void squashfs_readahead(struct readahead_control *ractl)
@@ -572,7 +606,7 @@ static void squashfs_readahead(struct readahead_control *ractl)
 		if (start >> msblk->block_log == file_end &&
 				squashfs_i(inode)->fragment_block != SQUASHFS_INVALID_BLK) {
 			res = squashfs_readahead_fragment(pages, nr_pages,
-							  expected);
+							  expected, start);
 			if (res)
 				goto skip_pages;
 			continue;
-- 
2.39.2

Re: [PATCH 0/4] Squashfs: Update code to not use page->index

[Christian Brauner]

On Mon, 19 Aug 2024 00:58:43 +0100, Phillip Lougher wrote:
> In the near future page->index will be removed [1].  Any code which
> still uses page->index needs to be updated.
> 
> This patch-set contains 4 patches which updates most of the code in
> Squashfs.  The exceptions are functions which have been fixed in
> recent patches [2] & [3].
> 
> [...]

Applied to the vfs.folio branch of the vfs/vfs.git tree.
Patches in the vfs.folio branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.folio

[1/4] Squashfs: Update page_actor to not use page->index
      https://git.kernel.org/vfs/vfs/c/2258e22f05af
[2/4] Squashfs: Update squashfs_readahead() to not use page->index
      https://git.kernel.org/vfs/vfs/c/6f09ffb1f4fa
[3/4] Squashfs: Update squashfs_readpage_block() to not use page->index
      https://git.kernel.org/vfs/vfs/c/7f73fcde4d93
[4/4] Squashfs: Rewrite and update squashfs_readahead_fragment() to not use page->index
      https://git.kernel.org/vfs/vfs/c/fd54fa6efe0d