[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

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 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
>>>
>> .
>>