[PATCH 1/3] staging: erofs: fix error handling when failed to read compresssed data

[PATCH 1/3] staging: erofs: fix error handling when failed to read compresssed data

[Gao Xiang]

Complete read error handling paths for all three kinds of
compressed pages:

 1) For cache-managed pages, PG_uptodate will be checked since
    read_endio will unlock and SetPageUptodate for these pages;

 2) For inplaced pages, read_endio cannot SetPageUptodate directly
    since it should be used to mark the final decompressed data,
    PG_error will be set with page locked for IO error instead;

 3) For staging pages, PG_error is used, which is similar to
    what we do for inplaced pages.

Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
Cc: <stable@vger.kernel.org> # 4.19+
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---

This series focus on fixing error handling when failed to read
compresssed data due to previous incomplete paths.

In addition, the last 2 patches add IO error fault injection
for reading paths, which I have used to test the first patch as well.

Thanks,
Gao Xiang

 drivers/staging/erofs/unzip_vle.c | 41 ++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 8715bc50e09c..3416d3f10324 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -972,6 +972,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 	overlapped = false;
 	compressed_pages = grp->compressed_pages;
 
+	err = 0;
 	for (i = 0; i < clusterpages; ++i) {
 		unsigned int pagenr;
 
@@ -981,26 +982,39 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 		DBG_BUGON(!page);
 		DBG_BUGON(!page->mapping);
 
-		if (z_erofs_is_stagingpage(page))
-			continue;
+		if (!z_erofs_is_stagingpage(page)) {
 #ifdef EROFS_FS_HAS_MANAGED_CACHE
-		if (page->mapping == MNGD_MAPPING(sbi)) {
-			DBG_BUGON(!PageUptodate(page));
-			continue;
-		}
+			if (page->mapping == MNGD_MAPPING(sbi)) {
+				if (unlikely(!PageUptodate(page)))
+					err = -EIO;
+				continue;
+			}
 #endif
 
-		/* only non-head page could be reused as a compressed page */
-		pagenr = z_erofs_onlinepage_index(page);
+			/*
+			 * only if non-head page can be selected
+			 * for inplace decompression
+			 */
+			pagenr = z_erofs_onlinepage_index(page);
 
-		DBG_BUGON(pagenr >= nr_pages);
-		DBG_BUGON(pages[pagenr]);
-		++sparsemem_pages;
-		pages[pagenr] = page;
+			DBG_BUGON(pagenr >= nr_pages);
+			DBG_BUGON(pages[pagenr]);
+			++sparsemem_pages;
+			pages[pagenr] = page;
+
+			overlapped = true;
+		}
 
-		overlapped = true;
+		/* PG_error needs checking for inplaced and staging pages */
+		if (unlikely(PageError(page))) {
+			DBG_BUGON(PageUptodate(page));
+			err = -EIO;
+		}
 	}
 
+	if (unlikely(err))
+		goto out;
+
 	llen = (nr_pages << PAGE_SHIFT) - work->pageofs;
 
 	if (z_erofs_vle_workgrp_fmt(grp) == Z_EROFS_VLE_WORKGRP_FMT_PLAIN) {
@@ -1194,6 +1208,7 @@ pickup_page_for_submission(struct z_erofs_vle_workgroup *grp,
 	if (page->mapping == mc) {
 		WRITE_ONCE(grp->compressed_pages[nr], page);
 
+		ClearPageError(page);
 		if (!PagePrivate(page)) {
 			/*
 			 * impossible to be !PagePrivate(page) for
-- 
2.12.2

[PATCH 2/3] staging: erofs: introduce erofs_page_is_managed()

[Gao Xiang]

1) In order to clean up unnecessary
   page->mapping == MNGD_MAPPING(sbi) wrapped by #ifdefs;

2) Needed by "staging: erofs: support IO read error injection".

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/internal.h  |  7 +++++++
 drivers/staging/erofs/unzip_vle.c | 31 +++++++++----------------------
 2 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index e3bfde00c7d2..ba1d86f3470e 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -268,8 +268,15 @@ int erofs_try_to_free_cached_page(struct address_space *mapping,
 				  struct page *page);
 
 #define MNGD_MAPPING(sbi)	((sbi)->managed_cache->i_mapping)
+static inline bool erofs_page_is_managed(const struct erofs_sb_info *sbi,
+					 struct page *page)
+{
+	return page->mapping == MNGD_MAPPING(sbi);
+}
 #else
 #define MNGD_MAPPING(sbi)	(NULL)
+static inline bool erofs_page_is_managed(const struct erofs_sb_info *sbi,
+					 struct page *page) { return false; }
 #endif
 
 #define DEFAULT_MAX_SYNC_DECOMPRESS_PAGES	3
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 3416d3f10324..d05fed4f8013 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -844,11 +844,9 @@ static void z_erofs_vle_unzip_kickoff(void *ptr, int bios)
 static inline void z_erofs_vle_read_endio(struct bio *bio)
 {
 	const blk_status_t err = bio->bi_status;
+	struct erofs_sb_info *sbi = NULL;
 	unsigned int i;
 	struct bio_vec *bvec;
-#ifdef EROFS_FS_HAS_MANAGED_CACHE
-	struct address_space *mc = NULL;
-#endif
 	struct bvec_iter_all iter_all;
 
 	bio_for_each_segment_all(bvec, bio, i, iter_all) {
@@ -858,20 +856,12 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
 		DBG_BUGON(PageUptodate(page));
 		DBG_BUGON(!page->mapping);
 
-#ifdef EROFS_FS_HAS_MANAGED_CACHE
-		if (unlikely(!mc && !z_erofs_is_stagingpage(page))) {
-			struct inode *const inode = page->mapping->host;
-			struct super_block *const sb = inode->i_sb;
+		if (unlikely(!sbi && !z_erofs_is_stagingpage(page)))
+			sbi = EROFS_SB(page->mapping->host->i_sb);
 
-			mc = MNGD_MAPPING(EROFS_SB(sb));
-		}
-
-		/*
-		 * If mc has not gotten, it equals NULL,
-		 * however, page->mapping never be NULL if working properly.
-		 */
-		cachemngd = (page->mapping == mc);
-#endif
+		/* sbi should already be gotten if the page is managed */
+		if (sbi)
+			cachemngd = erofs_page_is_managed(sbi, page);
 
 		if (unlikely(err))
 			SetPageError(page);
@@ -983,13 +973,11 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 		DBG_BUGON(!page->mapping);
 
 		if (!z_erofs_is_stagingpage(page)) {
-#ifdef EROFS_FS_HAS_MANAGED_CACHE
-			if (page->mapping == MNGD_MAPPING(sbi)) {
+			if (erofs_page_is_managed(sbi, page)) {
 				if (unlikely(!PageUptodate(page)))
 					err = -EIO;
 				continue;
 			}
-#endif
 
 			/*
 			 * only if non-head page can be selected
@@ -1054,10 +1042,9 @@ static int z_erofs_vle_unzip(struct super_block *sb,
 	for (i = 0; i < clusterpages; ++i) {
 		page = compressed_pages[i];
 
-#ifdef EROFS_FS_HAS_MANAGED_CACHE
-		if (page->mapping == MNGD_MAPPING(sbi))
+		if (erofs_page_is_managed(sbi, page))
 			continue;
-#endif
+
 		/* recycle all individual staging pages */
 		(void)z_erofs_gather_if_stagingpage(page_pool, page);
 
-- 
2.12.2

[PATCH 3/3] staging: erofs: support IO read error injection

[Gao Xiang]

Used to simulate disk IO read error for testing fatal
error tolerance.

Here are the details,
1) use bio->bi_private to indicate super_block
   for non-compressed bios since some (mainly meta)
   pages can be of the corresponding bdev inode;
2) get super_block dynamically for compressed bios,
   therefore it could not inject bios full of staging
   pages, yet it doesn't affect the normal usage.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 drivers/staging/erofs/Documentation/filesystems/erofs.txt |  1 +
 drivers/staging/erofs/data.c                              | 10 +++++++++-
 drivers/staging/erofs/internal.h                          |  3 ++-
 drivers/staging/erofs/super.c                             |  3 ++-
 drivers/staging/erofs/unzip_vle.c                         | 10 ++++++++--
 5 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/erofs/Documentation/filesystems/erofs.txt b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
index 961ec4da7705..74cf84ac48a3 100644
--- a/drivers/staging/erofs/Documentation/filesystems/erofs.txt
+++ b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
@@ -60,6 +60,7 @@ fault_injection=%d     Enable fault injection in all supported types with
                        specified injection rate. Supported injection type:
                        Type_Name                Type_Value
                        FAULT_KMALLOC            0x000000001
+                       FAULT_READ_IO            0x000000002
 (no)user_xattr         Setup Extended User Attributes. Note: xattr is enabled
                        by default if CONFIG_EROFS_FS_XATTR is selected.
 (no)acl                Setup POSIX Access Control List. Note: acl is enabled
diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index 526e0dbea5b5..16302ee54261 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -17,11 +17,17 @@
 
 static inline void read_endio(struct bio *bio)
 {
+	struct super_block *const sb = bio->bi_private;
 	int i;
 	struct bio_vec *bvec;
-	const blk_status_t err = bio->bi_status;
+	blk_status_t err = bio->bi_status;
 	struct bvec_iter_all iter_all;
 
+	if (time_to_inject(EROFS_SB(sb), FAULT_READ_IO)) {
+		erofs_show_injection_info(FAULT_READ_IO);
+		err = BLK_STS_IOERR;
+	}
+
 	bio_for_each_segment_all(bvec, bio, i, iter_all) {
 		struct page *page = bvec->bv_page;
 
@@ -69,6 +75,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
 			err = PTR_ERR(bio);
 			goto err_out;
 		}
+		bio->bi_private = sb;
 
 		err = bio_add_page(bio, page, PAGE_SIZE, 0);
 		if (unlikely(err != PAGE_SIZE)) {
@@ -288,6 +295,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
 			bio = NULL;
 			goto err_out;
 		}
+		bio->bi_private = inode->i_sb;
 	}
 
 	err = bio_add_page(bio, page, PAGE_SIZE, 0);
diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
index ba1d86f3470e..5d7addd9cff0 100644
--- a/drivers/staging/erofs/internal.h
+++ b/drivers/staging/erofs/internal.h
@@ -44,11 +44,12 @@
 
 enum {
 	FAULT_KMALLOC,
+	FAULT_READ_IO,
 	FAULT_MAX,
 };
 
 #ifdef CONFIG_EROFS_FAULT_INJECTION
-extern char *erofs_fault_name[FAULT_MAX];
+extern const char *erofs_fault_name[FAULT_MAX];
 #define IS_FAULT_SET(fi, type) ((fi)->inject_type & (1 << (type)))
 
 struct erofs_fault_info {
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 15c784fba879..ce025e6e4399 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -141,8 +141,9 @@ static int superblock_read(struct super_block *sb)
 }
 
 #ifdef CONFIG_EROFS_FAULT_INJECTION
-char *erofs_fault_name[FAULT_MAX] = {
+const char *erofs_fault_name[FAULT_MAX] = {
 	[FAULT_KMALLOC]		= "kmalloc",
+	[FAULT_READ_IO]		= "read IO error",
 };
 
 static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index d05fed4f8013..9fabdf596438 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -843,8 +843,8 @@ static void z_erofs_vle_unzip_kickoff(void *ptr, int bios)
 
 static inline void z_erofs_vle_read_endio(struct bio *bio)
 {
-	const blk_status_t err = bio->bi_status;
 	struct erofs_sb_info *sbi = NULL;
+	blk_status_t err = bio->bi_status;
 	unsigned int i;
 	struct bio_vec *bvec;
 	struct bvec_iter_all iter_all;
@@ -856,9 +856,15 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
 		DBG_BUGON(PageUptodate(page));
 		DBG_BUGON(!page->mapping);
 
-		if (unlikely(!sbi && !z_erofs_is_stagingpage(page)))
+		if (unlikely(!sbi && !z_erofs_is_stagingpage(page))) {
 			sbi = EROFS_SB(page->mapping->host->i_sb);
 
+			if (time_to_inject(sbi, FAULT_READ_IO)) {
+				erofs_show_injection_info(FAULT_READ_IO);
+				err = BLK_STS_IOERR;
+			}
+		}
+
 		/* sbi should already be gotten if the page is managed */
 		if (sbi)
 			cachemngd = erofs_page_is_managed(sbi, page);
-- 
2.12.2

Re: [PATCH 1/3] staging: erofs: fix error handling when failed to read compresssed data

[Gao Xiang]

ping?

Hi Chao,
could you take some time looking into this series?

Thanks,
Gao Xiang

On 2019/3/19 21:54, Gao Xiang wrote:
> Complete read error handling paths for all three kinds of
> compressed pages:
> 
>  1) For cache-managed pages, PG_uptodate will be checked since
>     read_endio will unlock and SetPageUptodate for these pages;
> 
>  2) For inplaced pages, read_endio cannot SetPageUptodate directly
>     since it should be used to mark the final decompressed data,
>     PG_error will be set with page locked for IO error instead;
> 
>  3) For staging pages, PG_error is used, which is similar to
>     what we do for inplaced pages.
> 
> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
> Cc: <stable@vger.kernel.org> # 4.19+
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
> 
> This series focus on fixing error handling when failed to read
> compresssed data due to previous incomplete paths.
> 
> In addition, the last 2 patches add IO error fault injection
> for reading paths, which I have used to test the first patch as well.
> 
> Thanks,
> Gao Xiang
> 
>  drivers/staging/erofs/unzip_vle.c | 41 ++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index 8715bc50e09c..3416d3f10324 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -972,6 +972,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
>  	overlapped = false;
>  	compressed_pages = grp->compressed_pages;
>  
> +	err = 0;
>  	for (i = 0; i < clusterpages; ++i) {
>  		unsigned int pagenr;
>  
> @@ -981,26 +982,39 @@ static int z_erofs_vle_unzip(struct super_block *sb,
>  		DBG_BUGON(!page);
>  		DBG_BUGON(!page->mapping);
>  
> -		if (z_erofs_is_stagingpage(page))
> -			continue;
> +		if (!z_erofs_is_stagingpage(page)) {
>  #ifdef EROFS_FS_HAS_MANAGED_CACHE
> -		if (page->mapping == MNGD_MAPPING(sbi)) {
> -			DBG_BUGON(!PageUptodate(page));
> -			continue;
> -		}
> +			if (page->mapping == MNGD_MAPPING(sbi)) {
> +				if (unlikely(!PageUptodate(page)))
> +					err = -EIO;
> +				continue;
> +			}
>  #endif
>  
> -		/* only non-head page could be reused as a compressed page */
> -		pagenr = z_erofs_onlinepage_index(page);
> +			/*
> +			 * only if non-head page can be selected
> +			 * for inplace decompression
> +			 */
> +			pagenr = z_erofs_onlinepage_index(page);
>  
> -		DBG_BUGON(pagenr >= nr_pages);
> -		DBG_BUGON(pages[pagenr]);
> -		++sparsemem_pages;
> -		pages[pagenr] = page;
> +			DBG_BUGON(pagenr >= nr_pages);
> +			DBG_BUGON(pages[pagenr]);
> +			++sparsemem_pages;
> +			pages[pagenr] = page;
> +
> +			overlapped = true;
> +		}
>  
> -		overlapped = true;
> +		/* PG_error needs checking for inplaced and staging pages */
> +		if (unlikely(PageError(page))) {
> +			DBG_BUGON(PageUptodate(page));
> +			err = -EIO;
> +		}
>  	}
>  
> +	if (unlikely(err))
> +		goto out;
> +
>  	llen = (nr_pages << PAGE_SHIFT) - work->pageofs;
>  
>  	if (z_erofs_vle_workgrp_fmt(grp) == Z_EROFS_VLE_WORKGRP_FMT_PLAIN) {
> @@ -1194,6 +1208,7 @@ pickup_page_for_submission(struct z_erofs_vle_workgroup *grp,
>  	if (page->mapping == mc) {
>  		WRITE_ONCE(grp->compressed_pages[nr], page);
>  
> +		ClearPageError(page);
>  		if (!PagePrivate(page)) {
>  			/*
>  			 * impossible to be !PagePrivate(page) for
> 

Re: [PATCH 1/3] staging: erofs: fix error handling when failed to read compresssed data

[Gao Xiang]

Hi,

On 2019/3/25 8:38, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 3883a79abd02 staging: erofs: introduce VLE decompression support.
> 
> The bot has tested the following trees: v5.0.3, v4.19.30.
> 
> v5.0.3: Build OK!
> v4.19.30: Failed to apply! Possible dependencies:
>     390c642e1264 ("staging: erofs: fix integer overflow on 32-bit platform")
>     42d40b4ad840 ("staging: erofs: unzip_vle.c: Replace comparison to NULL.")
>     6e78901a9f23 ("staging: erofs: separate erofs_get_meta_page")
>     7dd68b147d60 ("staging: erofs: use explicit unsigned int type")
>     8be31270362b ("staging: erofs: introduce erofs_grab_bio")
>     9248fce714d5 ("staging: erofs: revisit the page submission flow")
>     a07eeddf5b63 ("staging: erofs: clean up z_erofs_map_blocks_iter")
>     ab47dd2b0819 ("staging: erofs: cleanup z_erofs_vle_work_{lookup, register}")
>     b27661cf99c2 ("staging: erofs: fold in `__update_workgrp_llen'")
>     c1448fa88025 ("staging: erofs: introduce MNGD_MAPPING helper")
> 
> 
> How should we proceed with this patch?

I will do the 4.19 backport patch after get a "Reviewed-by: " tag,
thanks for the reminder.

Thanks,
Gao Xiang

> 
> --
> Thanks,
> Sasha
> 

Re: [PATCH 1/3] staging: erofs: fix error handling when failed to read compresssed data

[Chao Yu]

On 2019/3/19 21:54, Gao Xiang wrote:
> Complete read error handling paths for all three kinds of
> compressed pages:
> 
>  1) For cache-managed pages, PG_uptodate will be checked since
>     read_endio will unlock and SetPageUptodate for these pages;
> 
>  2) For inplaced pages, read_endio cannot SetPageUptodate directly
>     since it should be used to mark the final decompressed data,
>     PG_error will be set with page locked for IO error instead;
> 
>  3) For staging pages, PG_error is used, which is similar to
>     what we do for inplaced pages.
> 
> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
> Cc: <stable@vger.kernel.org> # 4.19+
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

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

Thanks,

Re: [PATCH 2/3] staging: erofs: introduce erofs_page_is_managed()

[Chao Yu]

On 2019/3/19 21:55, Gao Xiang wrote:
> 1) In order to clean up unnecessary
>    page->mapping == MNGD_MAPPING(sbi) wrapped by #ifdefs;
> 
> 2) Needed by "staging: erofs: support IO read error injection".
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

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

Thanks,

Re: [PATCH 3/3] staging: erofs: support IO read error injection

[Chao Yu]

On 2019/3/19 21:55, Gao Xiang wrote:
> Used to simulate disk IO read error for testing fatal
> error tolerance.
> 
> Here are the details,
> 1) use bio->bi_private to indicate super_block
>    for non-compressed bios since some (mainly meta)
>    pages can be of the corresponding bdev inode;
> 2) get super_block dynamically for compressed bios,
>    therefore it could not inject bios full of staging
>    pages, yet it doesn't affect the normal usage.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>  drivers/staging/erofs/Documentation/filesystems/erofs.txt |  1 +
>  drivers/staging/erofs/data.c                              | 10 +++++++++-
>  drivers/staging/erofs/internal.h                          |  3 ++-
>  drivers/staging/erofs/super.c                             |  3 ++-
>  drivers/staging/erofs/unzip_vle.c                         | 10 ++++++++--
>  5 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/erofs/Documentation/filesystems/erofs.txt b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> index 961ec4da7705..74cf84ac48a3 100644
> --- a/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> +++ b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> @@ -60,6 +60,7 @@ fault_injection=%d     Enable fault injection in all supported types with
>                         specified injection rate. Supported injection type:
>                         Type_Name                Type_Value
>                         FAULT_KMALLOC            0x000000001
> +                       FAULT_READ_IO            0x000000002
>  (no)user_xattr         Setup Extended User Attributes. Note: xattr is enabled
>                         by default if CONFIG_EROFS_FS_XATTR is selected.
>  (no)acl                Setup POSIX Access Control List. Note: acl is enabled
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index 526e0dbea5b5..16302ee54261 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -17,11 +17,17 @@
>  
>  static inline void read_endio(struct bio *bio)
>  {
> +	struct super_block *const sb = bio->bi_private;
>  	int i;
>  	struct bio_vec *bvec;
> -	const blk_status_t err = bio->bi_status;
> +	blk_status_t err = bio->bi_status;
>  	struct bvec_iter_all iter_all;
>  
> +	if (time_to_inject(EROFS_SB(sb), FAULT_READ_IO)) {
> +		erofs_show_injection_info(FAULT_READ_IO);
> +		err = BLK_STS_IOERR;
> +	}
> +
>  	bio_for_each_segment_all(bvec, bio, i, iter_all) {
>  		struct page *page = bvec->bv_page;
>  
> @@ -69,6 +75,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>  			err = PTR_ERR(bio);
>  			goto err_out;
>  		}
> +		bio->bi_private = sb;

Will it be better to initialize bio's field in erofs_grab_bio()?

>  
>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
>  		if (unlikely(err != PAGE_SIZE)) {
> @@ -288,6 +295,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>  			bio = NULL;
>  			goto err_out;
>  		}
> +		bio->bi_private = inode->i_sb;

Ditto.

Thanks,

>  	}
>  
>  	err = bio_add_page(bio, page, PAGE_SIZE, 0);
> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
> index ba1d86f3470e..5d7addd9cff0 100644
> --- a/drivers/staging/erofs/internal.h
> +++ b/drivers/staging/erofs/internal.h
> @@ -44,11 +44,12 @@
>  
>  enum {
>  	FAULT_KMALLOC,
> +	FAULT_READ_IO,
>  	FAULT_MAX,
>  };
>  
>  #ifdef CONFIG_EROFS_FAULT_INJECTION
> -extern char *erofs_fault_name[FAULT_MAX];
> +extern const char *erofs_fault_name[FAULT_MAX];
>  #define IS_FAULT_SET(fi, type) ((fi)->inject_type & (1 << (type)))
>  
>  struct erofs_fault_info {
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 15c784fba879..ce025e6e4399 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -141,8 +141,9 @@ static int superblock_read(struct super_block *sb)
>  }
>  
>  #ifdef CONFIG_EROFS_FAULT_INJECTION
> -char *erofs_fault_name[FAULT_MAX] = {
> +const char *erofs_fault_name[FAULT_MAX] = {
>  	[FAULT_KMALLOC]		= "kmalloc",
> +	[FAULT_READ_IO]		= "read IO error",
>  };
>  
>  static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index d05fed4f8013..9fabdf596438 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -843,8 +843,8 @@ static void z_erofs_vle_unzip_kickoff(void *ptr, int bios)
>  
>  static inline void z_erofs_vle_read_endio(struct bio *bio)
>  {
> -	const blk_status_t err = bio->bi_status;
>  	struct erofs_sb_info *sbi = NULL;
> +	blk_status_t err = bio->bi_status;
>  	unsigned int i;
>  	struct bio_vec *bvec;
>  	struct bvec_iter_all iter_all;
> @@ -856,9 +856,15 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
>  		DBG_BUGON(PageUptodate(page));
>  		DBG_BUGON(!page->mapping);
>  
> -		if (unlikely(!sbi && !z_erofs_is_stagingpage(page)))
> +		if (unlikely(!sbi && !z_erofs_is_stagingpage(page))) {
>  			sbi = EROFS_SB(page->mapping->host->i_sb);
>  
> +			if (time_to_inject(sbi, FAULT_READ_IO)) {
> +				erofs_show_injection_info(FAULT_READ_IO);
> +				err = BLK_STS_IOERR;
> +			}
> +		}
> +
>  		/* sbi should already be gotten if the page is managed */
>  		if (sbi)
>  			cachemngd = erofs_page_is_managed(sbi, page);
> 

Re: [PATCH 3/3] staging: erofs: support IO read error injection

[Gao Xiang]

Hi Chao,

On 2019/3/25 9:47, Chao Yu wrote:
> On 2019/3/19 21:55, Gao Xiang wrote:
>> Used to simulate disk IO read error for testing fatal
>> error tolerance.
>>
>> Here are the details,
>> 1) use bio->bi_private to indicate super_block
>>    for non-compressed bios since some (mainly meta)
>>    pages can be of the corresponding bdev inode;
>> 2) get super_block dynamically for compressed bios,
>>    therefore it could not inject bios full of staging
>>    pages, yet it doesn't affect the normal usage.
>>
>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>> ---
>>  drivers/staging/erofs/Documentation/filesystems/erofs.txt |  1 +
>>  drivers/staging/erofs/data.c                              | 10 +++++++++-
>>  drivers/staging/erofs/internal.h                          |  3 ++-
>>  drivers/staging/erofs/super.c                             |  3 ++-
>>  drivers/staging/erofs/unzip_vle.c                         | 10 ++++++++--
>>  5 files changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/Documentation/filesystems/erofs.txt b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
>> index 961ec4da7705..74cf84ac48a3 100644
>> --- a/drivers/staging/erofs/Documentation/filesystems/erofs.txt
>> +++ b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
>> @@ -60,6 +60,7 @@ fault_injection=%d     Enable fault injection in all supported types with
>>                         specified injection rate. Supported injection type:
>>                         Type_Name                Type_Value
>>                         FAULT_KMALLOC            0x000000001
>> +                       FAULT_READ_IO            0x000000002
>>  (no)user_xattr         Setup Extended User Attributes. Note: xattr is enabled
>>                         by default if CONFIG_EROFS_FS_XATTR is selected.
>>  (no)acl                Setup POSIX Access Control List. Note: acl is enabled
>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>> index 526e0dbea5b5..16302ee54261 100644
>> --- a/drivers/staging/erofs/data.c
>> +++ b/drivers/staging/erofs/data.c
>> @@ -17,11 +17,17 @@
>>  
>>  static inline void read_endio(struct bio *bio)
>>  {
>> +	struct super_block *const sb = bio->bi_private;
>>  	int i;
>>  	struct bio_vec *bvec;
>> -	const blk_status_t err = bio->bi_status;
>> +	blk_status_t err = bio->bi_status;
>>  	struct bvec_iter_all iter_all;
>>  
>> +	if (time_to_inject(EROFS_SB(sb), FAULT_READ_IO)) {
>> +		erofs_show_injection_info(FAULT_READ_IO);
>> +		err = BLK_STS_IOERR;
>> +	}
>> +
>>  	bio_for_each_segment_all(bvec, bio, i, iter_all) {
>>  		struct page *page = bvec->bv_page;
>>  
>> @@ -69,6 +75,7 @@ struct page *__erofs_get_meta_page(struct super_block *sb,
>>  			err = PTR_ERR(bio);
>>  			goto err_out;
>>  		}
>> +		bio->bi_private = sb;
> 
> Will it be better to initialize bio's field in erofs_grab_bio()?

Agreed and it will be fixed since bi_private field in erofs bios will be
initialized.

Thanks for the review :)

Thanks,
Gao Xiang

> 
>>  
>>  		err = bio_add_page(bio, page, PAGE_SIZE, 0);
>>  		if (unlikely(err != PAGE_SIZE)) {
>> @@ -288,6 +295,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>>  			bio = NULL;
>>  			goto err_out;
>>  		}
>> +		bio->bi_private = inode->i_sb;
> 
> Ditto.
> 
> Thanks,
> 
>>  	}
>>  
>>  	err = bio_add_page(bio, page, PAGE_SIZE, 0);
>> diff --git a/drivers/staging/erofs/internal.h b/drivers/staging/erofs/internal.h
>> index ba1d86f3470e..5d7addd9cff0 100644
>> --- a/drivers/staging/erofs/internal.h
>> +++ b/drivers/staging/erofs/internal.h
>> @@ -44,11 +44,12 @@
>>  
>>  enum {
>>  	FAULT_KMALLOC,
>> +	FAULT_READ_IO,
>>  	FAULT_MAX,
>>  };
>>  
>>  #ifdef CONFIG_EROFS_FAULT_INJECTION
>> -extern char *erofs_fault_name[FAULT_MAX];
>> +extern const char *erofs_fault_name[FAULT_MAX];
>>  #define IS_FAULT_SET(fi, type) ((fi)->inject_type & (1 << (type)))
>>  
>>  struct erofs_fault_info {
>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>> index 15c784fba879..ce025e6e4399 100644
>> --- a/drivers/staging/erofs/super.c
>> +++ b/drivers/staging/erofs/super.c
>> @@ -141,8 +141,9 @@ static int superblock_read(struct super_block *sb)
>>  }
>>  
>>  #ifdef CONFIG_EROFS_FAULT_INJECTION
>> -char *erofs_fault_name[FAULT_MAX] = {
>> +const char *erofs_fault_name[FAULT_MAX] = {
>>  	[FAULT_KMALLOC]		= "kmalloc",
>> +	[FAULT_READ_IO]		= "read IO error",
>>  };
>>  
>>  static void __erofs_build_fault_attr(struct erofs_sb_info *sbi,
>> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
>> index d05fed4f8013..9fabdf596438 100644
>> --- a/drivers/staging/erofs/unzip_vle.c
>> +++ b/drivers/staging/erofs/unzip_vle.c
>> @@ -843,8 +843,8 @@ static void z_erofs_vle_unzip_kickoff(void *ptr, int bios)
>>  
>>  static inline void z_erofs_vle_read_endio(struct bio *bio)
>>  {
>> -	const blk_status_t err = bio->bi_status;
>>  	struct erofs_sb_info *sbi = NULL;
>> +	blk_status_t err = bio->bi_status;
>>  	unsigned int i;
>>  	struct bio_vec *bvec;
>>  	struct bvec_iter_all iter_all;
>> @@ -856,9 +856,15 @@ static inline void z_erofs_vle_read_endio(struct bio *bio)
>>  		DBG_BUGON(PageUptodate(page));
>>  		DBG_BUGON(!page->mapping);
>>  
>> -		if (unlikely(!sbi && !z_erofs_is_stagingpage(page)))
>> +		if (unlikely(!sbi && !z_erofs_is_stagingpage(page))) {
>>  			sbi = EROFS_SB(page->mapping->host->i_sb);
>>  
>> +			if (time_to_inject(sbi, FAULT_READ_IO)) {
>> +				erofs_show_injection_info(FAULT_READ_IO);
>> +				err = BLK_STS_IOERR;
>> +			}
>> +		}
>> +
>>  		/* sbi should already be gotten if the page is managed */
>>  		if (sbi)
>>  			cachemngd = erofs_page_is_managed(sbi, page);
>>

Re: [PATCH 1/3] staging: erofs: fix error handling when failed to read compresssed data

[Chao Yu]

On 2019/3/22 11:25, Gao Xiang wrote:
> ping?
> 
> Hi Chao,
> could you take some time looking into this series?

Done, sorry for the delay.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
> On 2019/3/19 21:54, Gao Xiang wrote:
>> Complete read error handling paths for all three kinds of
>> compressed pages:
>>
>>  1) For cache-managed pages, PG_uptodate will be checked since
>>     read_endio will unlock and SetPageUptodate for these pages;
>>
>>  2) For inplaced pages, read_endio cannot SetPageUptodate directly
>>     since it should be used to mark the final decompressed data,
>>     PG_error will be set with page locked for IO error instead;
>>
>>  3) For staging pages, PG_error is used, which is similar to
>>     what we do for inplaced pages.
>>
>> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
>> Cc: <stable@vger.kernel.org> # 4.19+
>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>> ---
>>
>> This series focus on fixing error handling when failed to read
>> compresssed data due to previous incomplete paths.
>>
>> In addition, the last 2 patches add IO error fault injection
>> for reading paths, which I have used to test the first patch as well.
>>
>> Thanks,
>> Gao Xiang
>>
>>  drivers/staging/erofs/unzip_vle.c | 41 ++++++++++++++++++++++++++-------------
>>  1 file changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
>> index 8715bc50e09c..3416d3f10324 100644
>> --- a/drivers/staging/erofs/unzip_vle.c
>> +++ b/drivers/staging/erofs/unzip_vle.c
>> @@ -972,6 +972,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
>>  	overlapped = false;
>>  	compressed_pages = grp->compressed_pages;
>>  
>> +	err = 0;
>>  	for (i = 0; i < clusterpages; ++i) {
>>  		unsigned int pagenr;
>>  
>> @@ -981,26 +982,39 @@ static int z_erofs_vle_unzip(struct super_block *sb,
>>  		DBG_BUGON(!page);
>>  		DBG_BUGON(!page->mapping);
>>  
>> -		if (z_erofs_is_stagingpage(page))
>> -			continue;
>> +		if (!z_erofs_is_stagingpage(page)) {
>>  #ifdef EROFS_FS_HAS_MANAGED_CACHE
>> -		if (page->mapping == MNGD_MAPPING(sbi)) {
>> -			DBG_BUGON(!PageUptodate(page));
>> -			continue;
>> -		}
>> +			if (page->mapping == MNGD_MAPPING(sbi)) {
>> +				if (unlikely(!PageUptodate(page)))
>> +					err = -EIO;
>> +				continue;
>> +			}
>>  #endif
>>  
>> -		/* only non-head page could be reused as a compressed page */
>> -		pagenr = z_erofs_onlinepage_index(page);
>> +			/*
>> +			 * only if non-head page can be selected
>> +			 * for inplace decompression
>> +			 */
>> +			pagenr = z_erofs_onlinepage_index(page);
>>  
>> -		DBG_BUGON(pagenr >= nr_pages);
>> -		DBG_BUGON(pages[pagenr]);
>> -		++sparsemem_pages;
>> -		pages[pagenr] = page;
>> +			DBG_BUGON(pagenr >= nr_pages);
>> +			DBG_BUGON(pages[pagenr]);
>> +			++sparsemem_pages;
>> +			pages[pagenr] = page;
>> +
>> +			overlapped = true;
>> +		}
>>  
>> -		overlapped = true;
>> +		/* PG_error needs checking for inplaced and staging pages */
>> +		if (unlikely(PageError(page))) {
>> +			DBG_BUGON(PageUptodate(page));
>> +			err = -EIO;
>> +		}
>>  	}
>>  
>> +	if (unlikely(err))
>> +		goto out;
>> +
>>  	llen = (nr_pages << PAGE_SHIFT) - work->pageofs;
>>  
>>  	if (z_erofs_vle_workgrp_fmt(grp) == Z_EROFS_VLE_WORKGRP_FMT_PLAIN) {
>> @@ -1194,6 +1208,7 @@ pickup_page_for_submission(struct z_erofs_vle_workgroup *grp,
>>  	if (page->mapping == mc) {
>>  		WRITE_ONCE(grp->compressed_pages[nr], page);
>>  
>> +		ClearPageError(page);
>>  		if (!PagePrivate(page)) {
>>  			/*
>>  			 * impossible to be !PagePrivate(page) for
>>
> .
> 

Re: [PATCH 1/3] staging: erofs: fix error handling when failed to read compresssed data

[Gao Xiang]

Hi Chao,

On 2019/3/25 10:05, Chao Yu wrote:
> On 2019/3/22 11:25, Gao Xiang wrote:
>> ping?
>>
>> Hi Chao,
>> could you take some time looking into this series?
> 
> Done, sorry for the delay.

It doesn't matter. It is helpful for our kernel upgrade
since it keeps up with the latest community code now ;)

Thanks for keeping on taking time on erofs. *thumb*

Thanks,
Gao Xiang

> 
> Thanks,
> 
>>
>> Thanks,
>> Gao Xiang
>>
>> On 2019/3/19 21:54, Gao Xiang wrote:
>>> Complete read error handling paths for all three kinds of
>>> compressed pages:
>>>
>>>  1) For cache-managed pages, PG_uptodate will be checked since
>>>     read_endio will unlock and SetPageUptodate for these pages;
>>>
>>>  2) For inplaced pages, read_endio cannot SetPageUptodate directly
>>>     since it should be used to mark the final decompressed data,
>>>     PG_error will be set with page locked for IO error instead;
>>>
>>>  3) For staging pages, PG_error is used, which is similar to
>>>     what we do for inplaced pages.
>>>
>>> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
>>> Cc: <stable@vger.kernel.org> # 4.19+
>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>> ---
>>>
>>> This series focus on fixing error handling when failed to read
>>> compresssed data due to previous incomplete paths.
>>>
>>> In addition, the last 2 patches add IO error fault injection
>>> for reading paths, which I have used to test the first patch as well.
>>>
>>> Thanks,
>>> Gao Xiang
>>>
>>>  drivers/staging/erofs/unzip_vle.c | 41 ++++++++++++++++++++++++++-------------
>>>  1 file changed, 28 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
>>> index 8715bc50e09c..3416d3f10324 100644
>>> --- a/drivers/staging/erofs/unzip_vle.c
>>> +++ b/drivers/staging/erofs/unzip_vle.c
>>> @@ -972,6 +972,7 @@ static int z_erofs_vle_unzip(struct super_block *sb,
>>>  	overlapped = false;
>>>  	compressed_pages = grp->compressed_pages;
>>>  
>>> +	err = 0;
>>>  	for (i = 0; i < clusterpages; ++i) {
>>>  		unsigned int pagenr;
>>>  
>>> @@ -981,26 +982,39 @@ static int z_erofs_vle_unzip(struct super_block *sb,
>>>  		DBG_BUGON(!page);
>>>  		DBG_BUGON(!page->mapping);
>>>  
>>> -		if (z_erofs_is_stagingpage(page))
>>> -			continue;
>>> +		if (!z_erofs_is_stagingpage(page)) {
>>>  #ifdef EROFS_FS_HAS_MANAGED_CACHE
>>> -		if (page->mapping == MNGD_MAPPING(sbi)) {
>>> -			DBG_BUGON(!PageUptodate(page));
>>> -			continue;
>>> -		}
>>> +			if (page->mapping == MNGD_MAPPING(sbi)) {
>>> +				if (unlikely(!PageUptodate(page)))
>>> +					err = -EIO;
>>> +				continue;
>>> +			}
>>>  #endif
>>>  
>>> -		/* only non-head page could be reused as a compressed page */
>>> -		pagenr = z_erofs_onlinepage_index(page);
>>> +			/*
>>> +			 * only if non-head page can be selected
>>> +			 * for inplace decompression
>>> +			 */
>>> +			pagenr = z_erofs_onlinepage_index(page);
>>>  
>>> -		DBG_BUGON(pagenr >= nr_pages);
>>> -		DBG_BUGON(pages[pagenr]);
>>> -		++sparsemem_pages;
>>> -		pages[pagenr] = page;
>>> +			DBG_BUGON(pagenr >= nr_pages);
>>> +			DBG_BUGON(pages[pagenr]);
>>> +			++sparsemem_pages;
>>> +			pages[pagenr] = page;
>>> +
>>> +			overlapped = true;
>>> +		}
>>>  
>>> -		overlapped = true;
>>> +		/* PG_error needs checking for inplaced and staging pages */
>>> +		if (unlikely(PageError(page))) {
>>> +			DBG_BUGON(PageUptodate(page));
>>> +			err = -EIO;
>>> +		}
>>>  	}
>>>  
>>> +	if (unlikely(err))
>>> +		goto out;
>>> +
>>>  	llen = (nr_pages << PAGE_SHIFT) - work->pageofs;
>>>  
>>>  	if (z_erofs_vle_workgrp_fmt(grp) == Z_EROFS_VLE_WORKGRP_FMT_PLAIN) {
>>> @@ -1194,6 +1208,7 @@ pickup_page_for_submission(struct z_erofs_vle_workgroup *grp,
>>>  	if (page->mapping == mc) {
>>>  		WRITE_ONCE(grp->compressed_pages[nr], page);
>>>  
>>> +		ClearPageError(page);
>>>  		if (!PagePrivate(page)) {
>>>  			/*
>>>  			 * impossible to be !PagePrivate(page) for
>>>
>> .
>>