2023-05-26 13:33:05

by Vincent Whitchurch

[permalink] [raw]
Subject: [PATCH mm-nonmm-unstable 0/2] squashfs: fixups for caching

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

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

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

Best regards,
--
Vincent Whitchurch <[email protected]>



2023-05-26 13:50:26

by Vincent Whitchurch

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

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.

Signed-off-by: Vincent Whitchurch <[email protected]>
---
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


2023-05-26 13:53:55

by Vincent Whitchurch

[permalink] [raw]
Subject: [PATCH mm-nonmm-unstable 2/2] squashfs: fix page indices

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 <[email protected]>
---
fs/squashfs/block.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
index f2412e5fc84b..447fb04f2b61 100644
--- a/fs/squashfs/block.c
+++ b/fs/squashfs/block.c
@@ -142,7 +142,7 @@ 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 +153,7 @@ 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 +192,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


2023-05-26 14:09:02

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH mm-nonmm-unstable 2/2] squashfs: fix page indices

On Fri, May 26, 2023 at 03:25:46PM +0200, Vincent Whitchurch wrote:
> The page cache functions want the page index as an argument but we're
> currently passing in the byte address.

Can you avoid the overly long lines by movning the GFP_NOFS parameters
to the following lines?

The rest look good.

2023-05-26 14:33:01

by Christoph Hellwig

[permalink] [raw]