[PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Gao Xiang via Linux-erofs]

From: Gao Xiang <hsiangkao@redhat.com>

pcluster should be only set up for all managed pages instead of
temporary pages. Since it currently uses page->mapping to identify,
the impact is minor for now.

Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
Cc: <stable@vger.kernel.org> # 5.5+
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/erofs/zdata.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 50912a5420b4..86fd3bf62af6 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -1078,8 +1078,11 @@ static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
 		cond_resched();
 		goto repeat;
 	}
-	set_page_private(page, (unsigned long)pcl);
-	SetPagePrivate(page);
+
+	if (tocache) {
+		set_page_private(page, (unsigned long)pcl);
+		SetPagePrivate(page);
+	}
 out:	/* the only exit (for tracing and debugging) */
 	return page;
 }
-- 
2.24.0

[PATCH 2/4] erofs: get rid of magical Z_EROFS_MAPPING_STAGING

[Gao Xiang via Linux-erofs]

From: Gao Xiang <hsiangkao@redhat.com>

Previously, we played around with magical page->mapping for
short-lived temporary pages since we need to identify different
types of pages in the same pcluster but both invalidated and
short-lived temporary pages can have page->mapping == NULL.
It was considered as safe because temporary pages are all
non-LRU / non-movable pages.

This patch tends to use specific page->private to identify
short-lived pages instead so it won't rely on page->mapping
anymore. Details are described in "compress.h" as well.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
prelimitary test is done. will do stress test then.

 fs/erofs/compress.h     | 50 ++++++++++++++++++++++++++++++-----------
 fs/erofs/decompressor.c |  2 +-
 fs/erofs/zdata.c        | 42 +++++++++++++++++++++-------------
 fs/erofs/zdata.h        |  1 +
 4 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/fs/erofs/compress.h b/fs/erofs/compress.h
index 3d452443c545..2bbf47f353ef 100644
--- a/fs/erofs/compress.h
+++ b/fs/erofs/compress.h
@@ -26,30 +26,54 @@ struct z_erofs_decompress_req {
 	bool inplace_io, partial_decoding;
 };
 
+#define Z_EROFS_SHORTLIVED_PAGE		(-1UL << 2)
+
 /*
- * - 0x5A110C8D ('sallocated', Z_EROFS_MAPPING_STAGING) -
- * used to mark temporary allocated pages from other
- * file/cached pages and NULL mapping pages.
+ * For all pages in a pcluster, page->private should be one of
+ * Type                         Last 2bits      page->private
+ * short-lived page             00              Z_EROFS_SHORTLIVED_PAGE
+ * cached/managed page          00              pointer to z_erofs_pcluster
+ * online page (file-backed,    01/10/11        sub-index << 2 | count
+ *              some pages can be used for inplace I/O)
+ *
+ * page->mapping should be one of
+ * Type                 page->mapping
+ * short-lived page     NULL
+ * cached/managed page  non-NULL or NULL (invalidated/truncated page)
+ * online page          non-NULL
+ *
+ * For all managed pages, PG_private should be set with 1 extra refcount,
+ * which is used for page reclaim / migration.
  */
-#define Z_EROFS_MAPPING_STAGING         ((void *)0x5A110C8D)
 
-/* check if a page is marked as staging */
-static inline bool z_erofs_page_is_staging(struct page *page)
+/*
+ * short-lived pages are pages directly from buddy system with specific
+ * page->private (no need to set PagePrivate since these are non-LRU /
+ * non-movable pages and bypass reclaim / migration code).
+ */
+static inline bool z_erofs_is_shortlived_page(struct page *page)
 {
-	return page->mapping == Z_EROFS_MAPPING_STAGING;
+	if (page->private != Z_EROFS_SHORTLIVED_PAGE)
+		return false;
+
+	DBG_BUGON(page->mapping);
+	return true;
 }
 
-static inline bool z_erofs_put_stagingpage(struct list_head *pagepool,
-					   struct page *page)
+static inline bool z_erofs_put_shortlivedpage(struct list_head *pagepool,
+					      struct page *page)
 {
-	if (!z_erofs_page_is_staging(page))
+	if (!z_erofs_is_shortlived_page(page))
 		return false;
 
-	/* staging pages should not be used by others at the same time */
-	if (page_ref_count(page) > 1)
+	/* short-lived pages should not be used by others at the same time */
+	if (page_ref_count(page) > 1) {
 		put_page(page);
-	else
+	} else {
+		/* follow the pcluster rule above. */
+		set_page_private(page, 0);
 		list_add(&page->lru, pagepool);
+	}
 	return true;
 }
 
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index cbadbf55c6c2..1cb1ffd10569 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -76,7 +76,7 @@ static int z_erofs_lz4_prepare_destpages(struct z_erofs_decompress_req *rq,
 			victim = erofs_allocpage(pagepool, GFP_KERNEL);
 			if (!victim)
 				return -ENOMEM;
-			victim->mapping = Z_EROFS_MAPPING_STAGING;
+			set_page_private(victim, Z_EROFS_SHORTLIVED_PAGE);
 		}
 		rq->out[i] = victim;
 	}
diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 86fd3bf62af6..afeadf413c2c 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -255,6 +255,7 @@ int erofs_try_to_free_cached_page(struct address_space *mapping,
 		erofs_workgroup_unfreeze(&pcl->obj, 1);
 
 		if (ret) {
+			set_page_private(page, 0);
 			ClearPagePrivate(page);
 			put_page(page);
 		}
@@ -648,12 +649,12 @@ static int z_erofs_do_read_page(struct z_erofs_decompress_frontend *fe,
 
 retry:
 	err = z_erofs_attach_page(clt, page, page_type);
-	/* should allocate an additional staging page for pagevec */
+	/* should allocate an additional short-lived page for pagevec */
 	if (err == -EAGAIN) {
 		struct page *const newpage =
 				alloc_page(GFP_NOFS | __GFP_NOFAIL);
 
-		newpage->mapping = Z_EROFS_MAPPING_STAGING;
+		set_page_private(newpage, Z_EROFS_SHORTLIVED_PAGE);
 		err = z_erofs_attach_page(clt, newpage,
 					  Z_EROFS_PAGE_TYPE_EXCLUSIVE);
 		if (!err)
@@ -710,6 +711,11 @@ static void z_erofs_decompress_kickoff(struct z_erofs_decompressqueue *io,
 		queue_work(z_erofs_workqueue, &io->u.work);
 }
 
+static bool z_erofs_page_is_invalidated(struct page *page)
+{
+	return !page->mapping && !z_erofs_is_shortlived_page(page);
+}
+
 static void z_erofs_decompressqueue_endio(struct bio *bio)
 {
 	tagptr1_t t = tagptr_init(tagptr1_t, bio->bi_private);
@@ -722,7 +728,7 @@ static void z_erofs_decompressqueue_endio(struct bio *bio)
 		struct page *page = bvec->bv_page;
 
 		DBG_BUGON(PageUptodate(page));
-		DBG_BUGON(!page->mapping);
+		DBG_BUGON(z_erofs_page_is_invalidated(page));
 
 		if (err)
 			SetPageError(page);
@@ -795,9 +801,9 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 
 		/* all pages in pagevec ought to be valid */
 		DBG_BUGON(!page);
-		DBG_BUGON(!page->mapping);
+		DBG_BUGON(z_erofs_page_is_invalidated(page));
 
-		if (z_erofs_put_stagingpage(pagepool, page))
+		if (z_erofs_put_shortlivedpage(pagepool, page))
 			continue;
 
 		if (page_type == Z_EROFS_VLE_PAGE_TYPE_HEAD)
@@ -831,9 +837,9 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 
 		/* all compressed pages ought to be valid */
 		DBG_BUGON(!page);
-		DBG_BUGON(!page->mapping);
+		DBG_BUGON(z_erofs_page_is_invalidated(page));
 
-		if (!z_erofs_page_is_staging(page)) {
+		if (!z_erofs_is_shortlived_page(page)) {
 			if (erofs_page_is_managed(sbi, page)) {
 				if (!PageUptodate(page))
 					err = -EIO;
@@ -858,7 +864,7 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 			overlapped = true;
 		}
 
-		/* PG_error needs checking for inplaced and staging pages */
+		/* PG_error needs checking for all non-managed pages */
 		if (PageError(page)) {
 			DBG_BUGON(PageUptodate(page));
 			err = -EIO;
@@ -897,8 +903,8 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 		if (erofs_page_is_managed(sbi, page))
 			continue;
 
-		/* recycle all individual staging pages */
-		(void)z_erofs_put_stagingpage(pagepool, page);
+		/* recycle all individual short-lived pages */
+		(void)z_erofs_put_shortlivedpage(pagepool, page);
 
 		WRITE_ONCE(compressed_pages[i], NULL);
 	}
@@ -908,10 +914,10 @@ static int z_erofs_decompress_pcluster(struct super_block *sb,
 		if (!page)
 			continue;
 
-		DBG_BUGON(!page->mapping);
+		DBG_BUGON(z_erofs_page_is_invalidated(page));
 
-		/* recycle all individual staging pages */
-		if (z_erofs_put_stagingpage(pagepool, page))
+		/* recycle all individual short-lived pages */
+		if (z_erofs_put_shortlivedpage(pagepool, page))
 			continue;
 
 		if (err < 0)
@@ -1011,13 +1017,17 @@ static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
 	mapping = READ_ONCE(page->mapping);
 
 	/*
-	 * unmanaged (file) pages are all locked solidly,
+	 * file-backed online pages in plcuster are all locked steady,
 	 * therefore it is impossible for `mapping' to be NULL.
 	 */
 	if (mapping && mapping != mc)
 		/* ought to be unmanaged pages */
 		goto out;
 
+	/* directly return for shortlived page as well */
+	if (z_erofs_is_shortlived_page(page))
+		goto out;
+
 	lock_page(page);
 
 	/* only true if page reclaim goes wrong, should never happen */
@@ -1062,8 +1072,8 @@ static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
 out_allocpage:
 	page = erofs_allocpage(pagepool, gfp | __GFP_NOFAIL);
 	if (!tocache || add_to_page_cache_lru(page, mc, index + nr, gfp)) {
-		/* non-LRU / non-movable temporary page is needed */
-		page->mapping = Z_EROFS_MAPPING_STAGING;
+		/* turn into temporary page if fails */
+		set_page_private(page, Z_EROFS_SHORTLIVED_PAGE);
 		tocache = false;
 	}
 
diff --git a/fs/erofs/zdata.h b/fs/erofs/zdata.h
index 68c9b29fc0ca..b503b353d4ab 100644
--- a/fs/erofs/zdata.h
+++ b/fs/erofs/zdata.h
@@ -173,6 +173,7 @@ static inline void z_erofs_onlinepage_endio(struct page *page)
 
 	v = atomic_dec_return(u.o);
 	if (!(v & Z_EROFS_ONLINEPAGE_COUNT_MASK)) {
+		set_page_private(page, 0);
 		ClearPagePrivate(page);
 		if (!PageError(page))
 			SetPageUptodate(page);
-- 
2.24.0

[PATCH 3/4] erofs: insert to managed cache after adding to pcl

[Gao Xiang via Linux-erofs]

From: Gao Xiang <hsiangkao@redhat.com>

Previously, it could be some concern to call add_to_page_cache_lru()
with page->mapping == Z_EROFS_MAPPING_STAGING (!= NULL).

In contrast, page->private is used instead now, so partially revert
commit 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
with some adaption for simplicity.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/erofs/zdata.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index afeadf413c2c..edd7325570e1 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -1071,28 +1071,19 @@ static struct page *pickup_page_for_submission(struct z_erofs_pcluster *pcl,
 	put_page(page);
 out_allocpage:
 	page = erofs_allocpage(pagepool, gfp | __GFP_NOFAIL);
-	if (!tocache || add_to_page_cache_lru(page, mc, index + nr, gfp)) {
-		/* turn into temporary page if fails */
-		set_page_private(page, Z_EROFS_SHORTLIVED_PAGE);
-		tocache = false;
-	}
-
 	if (oldpage != cmpxchg(&pcl->compressed_pages[nr], oldpage, page)) {
-		if (tocache) {
-			/* since it added to managed cache successfully */
-			unlock_page(page);
-			put_page(page);
-		} else {
-			list_add(&page->lru, pagepool);
-		}
+		list_add(&page->lru, pagepool);
 		cond_resched();
 		goto repeat;
 	}
 
-	if (tocache) {
-		set_page_private(page, (unsigned long)pcl);
-		SetPagePrivate(page);
+	if (!tocache || add_to_page_cache_lru(page, mc, index + nr, gfp)) {
+		/* turn into temporary page if fails */
+		set_page_private(page, Z_EROFS_SHORTLIVED_PAGE);
+		goto out;
 	}
+	set_page_private(page, (unsigned long)pcl);
+	SetPagePrivate(page);
 out:	/* the only exit (for tracing and debugging) */
 	return page;
 }
-- 
2.24.0

[PATCH 4/4] erofs: complete a missing case for inplace I/O

[Gao Xiang via Linux-erofs]

From: Gao Xiang <hsiangkao@redhat.com>

Add a missing case which could cause unnecessary page allocation
but not directly use inplace I/O instead, which increases runtime
extra memory footprint.

The detail is, considering a file-backed page, the right half of
the page is chosen to be cached (e.g. the end page) and some of
its data doesn't exist in managed cache, so the pcluster will be
kept in the submission chain. (In other words, it cannot be
decompressed in advance, for example, due to the bypass chain).

Currently, the pcluster for this case is downgraded as NOINPLACE,
and stop the left half of the page from doing inplace I/O (so an
extra page allocation is needed then.)

Let's avoid such unnecessary downgrade instead.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/erofs/zdata.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index edd7325570e1..ef12275f7fcc 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -160,7 +160,8 @@ static void preload_compressed_pages(struct z_erofs_collector *clt,
 	const unsigned int clusterpages = BIT(pcl->clusterbits);
 	struct page **pages = clt->compressedpages;
 	pgoff_t index = pcl->obj.index + (pages - pcl->compressed_pages);
-	bool standalone = true;
+	bool io_cant_bypass = false;
+	bool updated = false;
 
 	if (clt->mode < COLLECT_PRIMARY_FOLLOWED)
 		return;
@@ -180,20 +181,31 @@ static void preload_compressed_pages(struct z_erofs_collector *clt,
 		} else if (type == DELAYEDALLOC) {
 			t = tagptr_init(compressed_page_t, PAGE_UNALLOCATED);
 		} else {	/* DONTALLOC */
-			if (standalone)
+			/* update to first inplace I/O page */
+			if (!updated) {
 				clt->compressedpages = pages;
-			standalone = false;
+				updated = true;
+			}
+			io_cant_bypass = true;
 			continue;
 		}
 
-		if (!cmpxchg_relaxed(pages, NULL, tagptr_cast_ptr(t)))
+		if (!cmpxchg_relaxed(pages, NULL, tagptr_cast_ptr(t))) {
+			if (type == DELAYEDALLOC)
+				io_cant_bypass = true;
 			continue;
+		}
 
 		if (page)
 			put_page(page);
 	}
 
-	if (standalone)		/* downgrade to PRIMARY_FOLLOWED_NOINPLACE */
+	/*
+	 * for COLLECT_PRIMARY_FOLLOWED pcluster, if all managed pages have
+	 * been found (which means it can be handled without submittng I/O)
+	 * it's unsafe to do inplace I/O. let's downgrade to NOINPLACE instead.
+	 */
+	if (!io_cant_bypass)
 		clt->mode = COLLECT_PRIMARY_FOLLOWED_NOINPLACE;
 }
 
-- 
2.24.0

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Vladimir Zapolskiy]

Hello Gao Xiang,

On 10/22/20 5:57 PM, Gao Xiang via Linux-erofs wrote:
> From: Gao Xiang <hsiangkao@redhat.com>
> 
> pcluster should be only set up for all managed pages instead of
> temporary pages. Since it currently uses page->mapping to identify,
> the impact is minor for now.
> 
> Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
> Cc: <stable@vger.kernel.org> # 5.5+
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

I was looking exactly at this problem recently, my change is one-to-one
to your fix, thus I can provide a tag:

Tested-by: Vladimir Zapolskiy <vladimir@tuxera.com>


The fixed problem is minor, but the kernel log becomes polluted, if
a page allocation debug option is enabled:

     % md5sum ~/erofs/testfile
     BUG: Bad page state in process kworker/u9:0  pfn:687de
     page:0000000057b8bcb4 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x687de
     flags: 0x4000000000002000(private)
     raw: 4000000000002000 dead000000000100 dead000000000122 0000000000000000
     raw: 0000000000000000 ffff888066758690 00000000ffffffff 0000000000000000
     page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
     Modules linked in:
     CPU: 1 PID: 602 Comm: kworker/u9:0 Not tainted 5.9.1 #2
     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
     Workqueue: erofs_unzipd z_erofs_decompressqueue_work
     Call Trace:
      dump_stack+0x84/0xba
      bad_page.cold+0xac/0xb1
      check_free_page_bad+0xb0/0xc0
      free_pcp_prepare+0x2c8/0x2d0
      free_unref_page+0x18/0xf0
      put_pages_list+0x11a/0x120
      z_erofs_decompressqueue_work+0xc9/0x110
      ? z_erofs_decompress_pcluster.isra.0+0xf10/0xf10
      ? read_word_at_a_time+0x12/0x20
      ? strscpy+0xc7/0x1a0
      process_one_work+0x30c/0x730
      worker_thread+0x91/0x640
      ? __kasan_check_read+0x11/0x20
      ? rescuer_thread+0x8a0/0x8a0
      kthread+0x1dd/0x200
      ? kthread_unpark+0xa0/0xa0
      ret_from_fork+0x1f/0x30
     Disabling lock debugging due to kernel taint

--
Best wishes,
Vladimir

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Gao Xiang]

Hi Vladimir,

On Fri, Oct 30, 2020 at 02:20:31PM +0200, Vladimir Zapolskiy wrote:
> Hello Gao Xiang,
> 
> On 10/22/20 5:57 PM, Gao Xiang via Linux-erofs wrote:
> > From: Gao Xiang <hsiangkao@redhat.com>
> > 
> > pcluster should be only set up for all managed pages instead of
> > temporary pages. Since it currently uses page->mapping to identify,
> > the impact is minor for now.
> > 
> > Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
> > Cc: <stable@vger.kernel.org> # 5.5+
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> 
> I was looking exactly at this problem recently, my change is one-to-one
> to your fix, thus I can provide a tag:
> 
> Tested-by: Vladimir Zapolskiy <vladimir@tuxera.com>

Many thanks for confirming this!
I found this when I was killing magical stagingpage page->mapping,
it's somewhat late :-)

> 
> 
> The fixed problem is minor, but the kernel log becomes polluted, if
> a page allocation debug option is enabled:
> 
>     % md5sum ~/erofs/testfile
>     BUG: Bad page state in process kworker/u9:0  pfn:687de
>     page:0000000057b8bcb4 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x687de
>     flags: 0x4000000000002000(private)
>     raw: 4000000000002000 dead000000000100 dead000000000122 0000000000000000
>     raw: 0000000000000000 ffff888066758690 00000000ffffffff 0000000000000000
>     page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
>     Modules linked in:
>     CPU: 1 PID: 602 Comm: kworker/u9:0 Not tainted 5.9.1 #2
>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
>     Workqueue: erofs_unzipd z_erofs_decompressqueue_work
>     Call Trace:
>      dump_stack+0x84/0xba
>      bad_page.cold+0xac/0xb1
>      check_free_page_bad+0xb0/0xc0
>      free_pcp_prepare+0x2c8/0x2d0
>      free_unref_page+0x18/0xf0
>      put_pages_list+0x11a/0x120
>      z_erofs_decompressqueue_work+0xc9/0x110
>      ? z_erofs_decompress_pcluster.isra.0+0xf10/0xf10
>      ? read_word_at_a_time+0x12/0x20
>      ? strscpy+0xc7/0x1a0
>      process_one_work+0x30c/0x730
>      worker_thread+0x91/0x640
>      ? __kasan_check_read+0x11/0x20
>      ? rescuer_thread+0x8a0/0x8a0
>      kthread+0x1dd/0x200
>      ? kthread_unpark+0xa0/0xa0
>      ret_from_fork+0x1f/0x30
>     Disabling lock debugging due to kernel taint

Yeah, I can make a pull-request to Linus if you need this to be in master
now, or I can post it for v5.11-rc1 since 5.4 LTS isn't effected (and it
would be only a print problem with debugging option.)

Thanks,
Gao Xiang

> 
> --
> Best wishes,
> Vladimir
> 

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Vladimir Zapolskiy]

Hi Gao Xiang,

On 10/30/20 2:47 PM, Gao Xiang wrote:
> Hi Vladimir,
> 
> On Fri, Oct 30, 2020 at 02:20:31PM +0200, Vladimir Zapolskiy wrote:
>> Hello Gao Xiang,
>>
>> On 10/22/20 5:57 PM, Gao Xiang via Linux-erofs wrote:
>>> From: Gao Xiang <hsiangkao@redhat.com>
>>>
>>> pcluster should be only set up for all managed pages instead of
>>> temporary pages. Since it currently uses page->mapping to identify,
>>> the impact is minor for now.
>>>
>>> Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
>>> Cc: <stable@vger.kernel.org> # 5.5+
>>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
>>
>> I was looking exactly at this problem recently, my change is one-to-one
>> to your fix, thus I can provide a tag:
>>
>> Tested-by: Vladimir Zapolskiy <vladimir@tuxera.com>
> 
> Many thanks for confirming this!
> I found this when I was killing magical stagingpage page->mapping,
> it's somewhat late :-)
> 

sure, for me it was an exciting immersion into the filesystem code :)

>>
>>
>> The fixed problem is minor, but the kernel log becomes polluted, if
>> a page allocation debug option is enabled:
>>
>>      % md5sum ~/erofs/testfile
>>      BUG: Bad page state in process kworker/u9:0  pfn:687de
>>      page:0000000057b8bcb4 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x687de
>>      flags: 0x4000000000002000(private)
>>      raw: 4000000000002000 dead000000000100 dead000000000122 0000000000000000
>>      raw: 0000000000000000 ffff888066758690 00000000ffffffff 0000000000000000
>>      page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
>>      Modules linked in:
>>      CPU: 1 PID: 602 Comm: kworker/u9:0 Not tainted 5.9.1 #2
>>      Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
>>      Workqueue: erofs_unzipd z_erofs_decompressqueue_work
>>      Call Trace:
>>       dump_stack+0x84/0xba
>>       bad_page.cold+0xac/0xb1
>>       check_free_page_bad+0xb0/0xc0
>>       free_pcp_prepare+0x2c8/0x2d0
>>       free_unref_page+0x18/0xf0
>>       put_pages_list+0x11a/0x120
>>       z_erofs_decompressqueue_work+0xc9/0x110
>>       ? z_erofs_decompress_pcluster.isra.0+0xf10/0xf10
>>       ? read_word_at_a_time+0x12/0x20
>>       ? strscpy+0xc7/0x1a0
>>       process_one_work+0x30c/0x730
>>       worker_thread+0x91/0x640
>>       ? __kasan_check_read+0x11/0x20
>>       ? rescuer_thread+0x8a0/0x8a0
>>       kthread+0x1dd/0x200
>>       ? kthread_unpark+0xa0/0xa0
>>       ret_from_fork+0x1f/0x30
>>      Disabling lock debugging due to kernel taint
> 
> Yeah, I can make a pull-request to Linus if you need this to be in master
> now, or I can post it for v5.11-rc1 since 5.4 LTS isn't effected (and it
> would be only a print problem with debugging option.)
> 

As for myself I don't utterly need this fix on the master branch ASAP, however
it might be reasonable to get it included right into the next v5.10 release,
because I believe it'll be an LTS. Eventually it's up to you to make a decision,
from my side I won't urge you, the fixed issue is obviously a non-critical one.

Thank you for the original fix and taking my opinion into consideration :)

--
Best wishes,
Vladimir

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Gao Xiang]

On Fri, Oct 30, 2020 at 03:32:55PM +0200, Vladimir Zapolskiy wrote:
> Hi Gao Xiang,
> 
> On 10/30/20 2:47 PM, Gao Xiang wrote:
> > Hi Vladimir,
> > 
> > On Fri, Oct 30, 2020 at 02:20:31PM +0200, Vladimir Zapolskiy wrote:
> > > Hello Gao Xiang,
> > > 
> > > On 10/22/20 5:57 PM, Gao Xiang via Linux-erofs wrote:
> > > > From: Gao Xiang <hsiangkao@redhat.com>
> > > > 
> > > > pcluster should be only set up for all managed pages instead of
> > > > temporary pages. Since it currently uses page->mapping to identify,
> > > > the impact is minor for now.
> > > > 
> > > > Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
> > > > Cc: <stable@vger.kernel.org> # 5.5+
> > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > 
> > > I was looking exactly at this problem recently, my change is one-to-one
> > > to your fix, thus I can provide a tag:
> > > 
> > > Tested-by: Vladimir Zapolskiy <vladimir@tuxera.com>
> > 
> > Many thanks for confirming this!
> > I found this when I was killing magical stagingpage page->mapping,
> > it's somewhat late :-)
> > 
> 
> sure, for me it was an exciting immersion into the filesystem code :)

Thanks for your effort on this!

You could also post related kernel message in advance and
I will definitly look into that as well. :)

> 
> > > 
> > > 
> > > The fixed problem is minor, but the kernel log becomes polluted, if
> > > a page allocation debug option is enabled:
> > > 
> > >      % md5sum ~/erofs/testfile
> > >      BUG: Bad page state in process kworker/u9:0  pfn:687de
> > >      page:0000000057b8bcb4 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x687de
> > >      flags: 0x4000000000002000(private)
> > >      raw: 4000000000002000 dead000000000100 dead000000000122 0000000000000000
> > >      raw: 0000000000000000 ffff888066758690 00000000ffffffff 0000000000000000
> > >      page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
> > >      Modules linked in:
> > >      CPU: 1 PID: 602 Comm: kworker/u9:0 Not tainted 5.9.1 #2
> > >      Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1 04/01/2014
> > >      Workqueue: erofs_unzipd z_erofs_decompressqueue_work
> > >      Call Trace:
> > >       dump_stack+0x84/0xba
> > >       bad_page.cold+0xac/0xb1
> > >       check_free_page_bad+0xb0/0xc0
> > >       free_pcp_prepare+0x2c8/0x2d0
> > >       free_unref_page+0x18/0xf0
> > >       put_pages_list+0x11a/0x120
> > >       z_erofs_decompressqueue_work+0xc9/0x110
> > >       ? z_erofs_decompress_pcluster.isra.0+0xf10/0xf10
> > >       ? read_word_at_a_time+0x12/0x20
> > >       ? strscpy+0xc7/0x1a0
> > >       process_one_work+0x30c/0x730
> > >       worker_thread+0x91/0x640
> > >       ? __kasan_check_read+0x11/0x20
> > >       ? rescuer_thread+0x8a0/0x8a0
> > >       kthread+0x1dd/0x200
> > >       ? kthread_unpark+0xa0/0xa0
> > >       ret_from_fork+0x1f/0x30
> > >      Disabling lock debugging due to kernel taint
> > 
> > Yeah, I can make a pull-request to Linus if you need this to be in master
> > now, or I can post it for v5.11-rc1 since 5.4 LTS isn't effected (and it
> > would be only a print problem with debugging option.)
> > 
> 
> As for myself I don't utterly need this fix on the master branch ASAP, however
> it might be reasonable to get it included right into the next v5.10 release,
> because I believe it'll be an LTS. Eventually it's up to you to make a decision,
> from my side I won't urge you, the fixed issue is obviously a non-critical one.
> 
> Thank you for the original fix and taking my opinion into consideration :)

Yeah, v5.10 is a LTS version, and you are right, I will try to make a
pull-request after I get Chao's RVB.

Thanks,
Gao Xiang

> 
> --
> Best wishes,
> Vladimir
> 

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Chao Yu]

On 2020/10/22 22:57, Gao Xiang wrote:
> From: Gao Xiang <hsiangkao@redhat.com>
> 
> pcluster should be only set up for all managed pages instead of
> temporary pages. Since it currently uses page->mapping to identify,
> the impact is minor for now.
> 
> Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
> Cc: <stable@vger.kernel.org> # 5.5+
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Gao Xiang]

On Wed, Nov 04, 2020 at 09:05:56AM +0800, Chao Yu wrote:
> On 2020/10/22 22:57, Gao Xiang wrote:
> > From: Gao Xiang <hsiangkao@redhat.com>
> > 
> > pcluster should be only set up for all managed pages instead of
> > temporary pages. Since it currently uses page->mapping to identify,
> > the impact is minor for now.
> > 
> > Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
> > Cc: <stable@vger.kernel.org> # 5.5+
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks, I've also added a note to the commit message like this,
"
[ Update: Vladimir reported the kernel log becomes polluted
  because PAGE_FLAGS_CHECK_AT_FREE flag(s) set if the page
  allocation debug option is enabled. ]
"
Will apply all of this to -fixes branch.

Thanks,
Gao Xiang

> 
> Thanks,
> 

Re: [PATCH 1/4] erofs: fix setting up pcluster for temporary pages

[Chao Yu]

On 2020/11/4 9:11, Gao Xiang wrote:
> On Wed, Nov 04, 2020 at 09:05:56AM +0800, Chao Yu wrote:
>> On 2020/10/22 22:57, Gao Xiang wrote:
>>> From: Gao Xiang <hsiangkao@redhat.com>
>>>
>>> pcluster should be only set up for all managed pages instead of
>>> temporary pages. Since it currently uses page->mapping to identify,
>>> the impact is minor for now.
>>>
>>> Fixes: 5ddcee1f3a1c ("erofs: get rid of __stagingpage_alloc helper")
>>> Cc: <stable@vger.kernel.org> # 5.5+
>>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
>>
>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks, I've also added a note to the commit message like this,
> "
> [ Update: Vladimir reported the kernel log becomes polluted
>    because PAGE_FLAGS_CHECK_AT_FREE flag(s) set if the page
>    allocation debug option is enabled. ]
> "
> Will apply all of this to -fixes branch.

Thanks for noticing that, looks fine to me.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>> Thanks,
>>
> 
> .
>