[PATCH mm-nonmm-unstable v2 0/2] squashfs: fixups for caching

[PATCH mm-nonmm-unstable v2 0/2] squashfs: fixups for caching

[Vincent Whitchurch]

These are a couple of fixups for
squashfs-cache-partial-compressed-blocks.patch which is currently in
mm-nonmm-unstable.

---
Changes in v2:
- Add Christoph's Reviewed-by in 1/2
- Restrict line lengths to 80 columns in 2/2
- Link to v1: https://lore.kernel.org/r/20230526-squashfs-cache-fixup-v1-0-d54a7fa23e7b@axis.com

---
Vincent Whitchurch (2):
      squashfs: fix page update race
      squashfs: fix page indices

 fs/squashfs/block.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
---
base-commit: 84cc8b966a3d4cde585761d05cc448dc1da0824f
change-id: 20230526-squashfs-cache-fixup-194431de42eb

Best regards,
-- 
Vincent Whitchurch <vincent.whitchurch@axis.com>

[PATCH mm-nonmm-unstable v2 1/2] squashfs: fix page update race

[Vincent Whitchurch]

We only put the page into the cache after we've read it, so the
PageUptodate() check should not be necessary.  In fact, it's actively
harmful since the check could fail (since we used find_get_page() and
not find_lock_page()) and we could end up submitting a page for I/O
after it has been read and while it's actively being used, which could
lead to corruption depending on what the block driver does with it.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 fs/squashfs/block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
index 6285f5afb6c6..f2412e5fc84b 100644
--- a/fs/squashfs/block.c
+++ b/fs/squashfs/block.c
@@ -92,7 +92,7 @@ static int squashfs_bio_read_cached(struct bio *fullbio,
 	bio_for_each_segment_all(bv, fullbio, iter_all) {
 		struct page *page = bv->bv_page;
 
-		if (page->mapping == cache_mapping && PageUptodate(page)) {
+		if (page->mapping == cache_mapping) {
 			idx++;
 			continue;
 		}

-- 
2.34.1

[PATCH mm-nonmm-unstable v2 2/2] squashfs: fix page indices

[Vincent Whitchurch]

The page cache functions want the page index as an argument but we're
currently passing in the byte address.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 fs/squashfs/block.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
index f2412e5fc84b..6aa9c2e1e8eb 100644
--- a/fs/squashfs/block.c
+++ b/fs/squashfs/block.c
@@ -142,7 +142,8 @@ static int squashfs_bio_read_cached(struct bio *fullbio,
 
 	if (head_to_cache) {
 		int ret = add_to_page_cache_lru(head_to_cache, cache_mapping,
-						read_start, GFP_NOIO);
+						read_start >> PAGE_SHIFT,
+						GFP_NOIO);
 
 		if (!ret) {
 			SetPageUptodate(head_to_cache);
@@ -153,7 +154,8 @@ static int squashfs_bio_read_cached(struct bio *fullbio,
 
 	if (tail_to_cache) {
 		int ret = add_to_page_cache_lru(tail_to_cache, cache_mapping,
-						read_end - PAGE_SIZE, GFP_NOIO);
+						(read_end >> PAGE_SHIFT) - 1,
+						GFP_NOIO);
 
 		if (!ret) {
 			SetPageUptodate(tail_to_cache);
@@ -192,7 +194,7 @@ static int squashfs_bio_read(struct super_block *sb, u64 index, int length,
 
 		if (cache_mapping)
 			page = find_get_page(cache_mapping,
-					     read_start + i * PAGE_SIZE);
+					     (read_start >> PAGE_SHIFT) + i);
 		if (!page)
 			page = alloc_page(GFP_NOIO);
 

-- 
2.34.1

Re: [PATCH mm-nonmm-unstable v2 2/2] squashfs: fix page indices

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH mm-nonmm-unstable v2 1/2] squashfs: fix page update race

[Phillip Lougher]

On 26/05/2023 14:57, Vincent Whitchurch wrote:
> We only put the page into the cache after we've read it, so the
> PageUptodate() check should not be necessary.  In fact, it's actively
> harmful since the check could fail (since we used find_get_page() and
> not find_lock_page()) and we could end up submitting a page for I/O
> after it has been read and while it's actively being used, which could
> lead to corruption depending on what the block driver does with it.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>


Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk>

Re: [PATCH mm-nonmm-unstable v2 2/2] squashfs: fix page indices

[Phillip Lougher]

On 26/05/2023 14:57, Vincent Whitchurch wrote:

> The page cache functions want the page index as an argument but we're
> currently passing in the byte address.
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk>

Re: [PATCH mm-nonmm-unstable v2 1/2] squashfs: fix page update race

[Vincent Whitchurch]

On Fri, 2023-05-26 at 15:57 +0200, Vincent Whitchurch wrote:
> We only put the page into the cache after we've read it, so the
> PageUptodate() check should not be necessary.  In fact, it's actively
> harmful since the check could fail (since we used find_get_page() and
> not find_lock_page()) and we could end up submitting a page for I/O
> after it has been read and while it's actively being used, which could
> lead to corruption depending on what the block driver does with it.

It turns out that removing the PageUptodate() check entirely wasn't
correct.

While it's true that the squashfs code only puts the page into the cache
after it's been read as I wrote above, migration on the other hand
replaces the page in the mapping _before_ copying the contents over, so
a PageUptodate() check is still needed.

The original problem can be fixed by moving the PageUptodate() check to
squashfs_bio_read() and ignoring the cached page entirely if it's not up
to date.

I'll post a fix shortly.