[PATCH for-4.19] staging: erofs: fix mis-acted TAIL merging behavior

[PATCH for-4.19] staging: erofs: fix mis-acted TAIL merging behavior

[Gao Xiang]

commit a112152f6f3a2a88caa6f414d540bd49e406af60 upstream.

EROFS has an optimized path called TAIL merging, which is designed
to merge multiple reads and the corresponding decompressions into
one if these requests read continuous pages almost at the same time.

In general, it behaves as follows:
 ________________________________________________________________
  ... |  TAIL  .  HEAD  |  PAGE  |  PAGE  |  TAIL    . HEAD | ...
 _____|_combined page A_|________|________|_combined page B_|____
        1  ]  ->  [  2                          ]  ->  [ 3
If the above three reads are requested in the order 1-2-3, it will
generate a large work chain rather than 3 individual work chains
to reduce scheduling overhead and boost up sequential read.

However, if Read 2 is processed slightly earlier than Read 1,
currently it still generates 2 individual work chains (chain 1, 2)
but it does in-place decompression for combined page A, moreover,
if chain 2 decompresses ahead of chain 1, it will be a race and
lead to corrupted decompressed page. This patch fixes it.

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>
---
 drivers/staging/erofs/unzip_vle.c | 69 +++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index 1279241449f4..24bd1d6deb66 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -57,15 +57,30 @@ enum z_erofs_vle_work_role {
 	Z_EROFS_VLE_WORK_SECONDARY,
 	Z_EROFS_VLE_WORK_PRIMARY,
 	/*
-	 * The current work has at least been linked with the following
-	 * processed chained works, which means if the processing page
-	 * is the tail partial page of the work, the current work can
-	 * safely use the whole page, as illustrated below:
-	 * +--------------+-------------------------------------------+
-	 * |  tail page   |      head page (of the previous work)     |
-	 * +--------------+-------------------------------------------+
-	 *   /\  which belongs to the current work
-	 * [  (*) this page can be used for the current work itself.  ]
+	 * The current work was the tail of an exist chain, and the previous
+	 * processed chained works are all decided to be hooked up to it.
+	 * A new chain should be created for the remaining unprocessed works,
+	 * therefore different from Z_EROFS_VLE_WORK_PRIMARY_FOLLOWED,
+	 * the next work cannot reuse the whole page in the following scenario:
+	 *  ________________________________________________________________
+	 * |      tail (partial) page     |       head (partial) page       |
+	 * |  (belongs to the next work)  |  (belongs to the current work)  |
+	 * |_______PRIMARY_FOLLOWED_______|________PRIMARY_HOOKED___________|
+	 */
+	Z_EROFS_VLE_WORK_PRIMARY_HOOKED,
+	/*
+	 * The current work has been linked with the processed chained works,
+	 * and could be also linked with the potential remaining works, which
+	 * means if the processing page is the tail partial page of the work,
+	 * the current work can safely use the whole page (since the next work
+	 * is under control) for in-place decompression, as illustrated below:
+	 *  ________________________________________________________________
+	 * |  tail (partial) page  |          head (partial) page           |
+	 * | (of the current work) |         (of the previous work)         |
+	 * |  PRIMARY_FOLLOWED or  |                                        |
+	 * |_____PRIMARY_HOOKED____|____________PRIMARY_FOLLOWED____________|
+	 *
+	 * [  (*) the above page can be used for the current work itself.  ]
 	 */
 	Z_EROFS_VLE_WORK_PRIMARY_FOLLOWED,
 	Z_EROFS_VLE_WORK_MAX
@@ -234,10 +249,10 @@ static int z_erofs_vle_work_add_page(
 	return ret ? 0 : -EAGAIN;
 }
 
-static inline bool try_to_claim_workgroup(
-	struct z_erofs_vle_workgroup *grp,
-	z_erofs_vle_owned_workgrp_t *owned_head,
-	bool *hosted)
+static enum z_erofs_vle_work_role
+try_to_claim_workgroup(struct z_erofs_vle_workgroup *grp,
+		       z_erofs_vle_owned_workgrp_t *owned_head,
+		       bool *hosted)
 {
 	DBG_BUGON(*hosted == true);
 
@@ -251,6 +266,9 @@ static inline bool try_to_claim_workgroup(
 
 		*owned_head = grp;
 		*hosted = true;
+		/* lucky, I am the followee :) */
+		return Z_EROFS_VLE_WORK_PRIMARY_FOLLOWED;
+
 	} else if (grp->next == Z_EROFS_VLE_WORKGRP_TAIL) {
 		/*
 		 * type 2, link to the end of a existing open chain,
@@ -260,12 +278,11 @@ static inline bool try_to_claim_workgroup(
 		if (Z_EROFS_VLE_WORKGRP_TAIL != cmpxchg(&grp->next,
 			Z_EROFS_VLE_WORKGRP_TAIL, *owned_head))
 			goto retry;
-
 		*owned_head = Z_EROFS_VLE_WORKGRP_TAIL;
-	} else
-		return false;	/* :( better luck next time */
+		return Z_EROFS_VLE_WORK_PRIMARY_HOOKED;
+	}
 
-	return true;	/* lucky, I am the followee :) */
+	return Z_EROFS_VLE_WORK_PRIMARY; /* :( better luck next time */
 }
 
 static struct z_erofs_vle_work *
@@ -337,12 +354,8 @@ z_erofs_vle_work_lookup(struct super_block *sb,
 	*hosted = false;
 	if (!primary)
 		*role = Z_EROFS_VLE_WORK_SECONDARY;
-	/* claim the workgroup if possible */
-	else if (try_to_claim_workgroup(grp, owned_head, hosted))
-		*role = Z_EROFS_VLE_WORK_PRIMARY_FOLLOWED;
-	else
-		*role = Z_EROFS_VLE_WORK_PRIMARY;
-
+	else	/* claim the workgroup if possible */
+		*role = try_to_claim_workgroup(grp, owned_head, hosted);
 	return work;
 }
 
@@ -419,6 +432,9 @@ static inline void __update_workgrp_llen(struct z_erofs_vle_workgroup *grp,
 	}
 }
 
+#define builder_is_hooked(builder) \
+	((builder)->role >= Z_EROFS_VLE_WORK_PRIMARY_HOOKED)
+
 #define builder_is_followed(builder) \
 	((builder)->role >= Z_EROFS_VLE_WORK_PRIMARY_FOLLOWED)
 
@@ -583,7 +599,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 	struct z_erofs_vle_work_builder *const builder = &fe->builder;
 	const loff_t offset = page_offset(page);
 
-	bool tight = builder_is_followed(builder);
+	bool tight = builder_is_hooked(builder);
 	struct z_erofs_vle_work *work = builder->work;
 
 #ifdef EROFS_FS_HAS_MANAGED_CACHE
@@ -646,7 +662,7 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 		builder->role = Z_EROFS_VLE_WORK_PRIMARY;
 #endif
 
-	tight &= builder_is_followed(builder);
+	tight &= builder_is_hooked(builder);
 	work = builder->work;
 hitted:
 	cur = end - min_t(unsigned, offset + end - map->m_la, end);
@@ -661,6 +677,9 @@ static int z_erofs_do_read_page(struct z_erofs_vle_frontend *fe,
 			(tight ? Z_EROFS_PAGE_TYPE_EXCLUSIVE :
 				Z_EROFS_VLE_PAGE_TYPE_TAIL_SHARED));
 
+	if (cur)
+		tight &= builder_is_followed(builder);
+
 retry:
 	err = z_erofs_vle_work_add_page(builder, page, page_type);
 	/* should allocate an additional staging page for pagevec */
-- 
2.14.5

Re: [PATCH for-4.19] staging: erofs: fix mis-acted TAIL merging behavior

[Greg KH]

On Thu, Mar 07, 2019 at 01:34:04PM +0800, Gao Xiang wrote:
> commit a112152f6f3a2a88caa6f414d540bd49e406af60 upstream.
> 
> EROFS has an optimized path called TAIL merging, which is designed
> to merge multiple reads and the corresponding decompressions into
> one if these requests read continuous pages almost at the same time.
> 
> In general, it behaves as follows:
>  ________________________________________________________________
>   ... |  TAIL  .  HEAD  |  PAGE  |  PAGE  |  TAIL    . HEAD | ...
>  _____|_combined page A_|________|________|_combined page B_|____
>         1  ]  ->  [  2                          ]  ->  [ 3
> If the above three reads are requested in the order 1-2-3, it will
> generate a large work chain rather than 3 individual work chains
> to reduce scheduling overhead and boost up sequential read.
> 
> However, if Read 2 is processed slightly earlier than Read 1,
> currently it still generates 2 individual work chains (chain 1, 2)
> but it does in-place decompression for combined page A, moreover,
> if chain 2 decompresses ahead of chain 1, it will be a race and
> lead to corrupted decompressed page. This patch fixes it.
> 
> 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>
> ---
>  drivers/staging/erofs/unzip_vle.c | 69 +++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 25 deletions(-)
> 

thanks for the backport, now applied.

greg k-h

Re: [PATCH for-4.19] staging: erofs: fix mis-acted TAIL merging behavior

[Gao Xiang]

Hi Greg,

On 2019/3/7 20:25, Greg KH wrote:
> On Thu, Mar 07, 2019 at 01:34:04PM +0800, Gao Xiang wrote:
>> commit a112152f6f3a2a88caa6f414d540bd49e406af60 upstream.
>>
>> EROFS has an optimized path called TAIL merging, which is designed
>> to merge multiple reads and the corresponding decompressions into
>> one if these requests read continuous pages almost at the same time.
>>
>> In general, it behaves as follows:
>>  ________________________________________________________________
>>   ... |  TAIL  .  HEAD  |  PAGE  |  PAGE  |  TAIL    . HEAD | ...
>>  _____|_combined page A_|________|________|_combined page B_|____
>>         1  ]  ->  [  2                          ]  ->  [ 3
>> If the above three reads are requested in the order 1-2-3, it will
>> generate a large work chain rather than 3 individual work chains
>> to reduce scheduling overhead and boost up sequential read.
>>
>> However, if Read 2 is processed slightly earlier than Read 1,
>> currently it still generates 2 individual work chains (chain 1, 2)
>> but it does in-place decompression for combined page A, moreover,
>> if chain 2 decompresses ahead of chain 1, it will be a race and
>> lead to corrupted decompressed page. This patch fixes it.
>>
>> 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>
>> ---
>>  drivers/staging/erofs/unzip_vle.c | 69 +++++++++++++++++++++++++--------------
>>  1 file changed, 44 insertions(+), 25 deletions(-)
>>
> 
> thanks for the backport, now applied.

Yes, it seems that Sasha's bot cannot cherry-pick it directly,
therefore I made this patch for 4.19 by hand...

BTW, there is another patch called "staging: erofs: compressed_pages should not be accessed again after freed"
in the same condition... And I also send 4.19 and 4.20 version to stable mailing list,
could you also kindly check them out?

Thanks,
Gao Xiang

> 
> greg k-h
> 

Re: [PATCH for-4.19] staging: erofs: fix mis-acted TAIL merging behavior

[Greg KH]

On Thu, Mar 07, 2019 at 09:04:06PM +0800, Gao Xiang wrote:
> Hi Greg,
> 
> On 2019/3/7 20:25, Greg KH wrote:
> > On Thu, Mar 07, 2019 at 01:34:04PM +0800, Gao Xiang wrote:
> >> commit a112152f6f3a2a88caa6f414d540bd49e406af60 upstream.
> >>
> >> EROFS has an optimized path called TAIL merging, which is designed
> >> to merge multiple reads and the corresponding decompressions into
> >> one if these requests read continuous pages almost at the same time.
> >>
> >> In general, it behaves as follows:
> >>  ________________________________________________________________
> >>   ... |  TAIL  .  HEAD  |  PAGE  |  PAGE  |  TAIL    . HEAD | ...
> >>  _____|_combined page A_|________|________|_combined page B_|____
> >>         1  ]  ->  [  2                          ]  ->  [ 3
> >> If the above three reads are requested in the order 1-2-3, it will
> >> generate a large work chain rather than 3 individual work chains
> >> to reduce scheduling overhead and boost up sequential read.
> >>
> >> However, if Read 2 is processed slightly earlier than Read 1,
> >> currently it still generates 2 individual work chains (chain 1, 2)
> >> but it does in-place decompression for combined page A, moreover,
> >> if chain 2 decompresses ahead of chain 1, it will be a race and
> >> lead to corrupted decompressed page. This patch fixes it.
> >>
> >> 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>
> >> ---
> >>  drivers/staging/erofs/unzip_vle.c | 69 +++++++++++++++++++++++++--------------
> >>  1 file changed, 44 insertions(+), 25 deletions(-)
> >>
> > 
> > thanks for the backport, now applied.
> 
> Yes, it seems that Sasha's bot cannot cherry-pick it directly,
> therefore I made this patch for 4.19 by hand...
> 
> BTW, there is another patch called "staging: erofs: compressed_pages should not be accessed again after freed"
> in the same condition... And I also send 4.19 and 4.20 version to stable mailing list,
> could you also kindly check them out?

Yes, please give me a chance to catch up on stable pending patches to
process:

	$ mdfrm -c ~/mail/stable/
	585 messages in /home/gregkh/mail/stable/

You are in good company :)

greg k-h

Re: [PATCH for-4.19] staging: erofs: fix mis-acted TAIL merging behavior

[Gao Xiang]

On 2019/3/7 23:21, Greg KH wrote:
> On Thu, Mar 07, 2019 at 09:04:06PM +0800, Gao Xiang wrote:
>> Hi Greg,
>>
>> On 2019/3/7 20:25, Greg KH wrote:
>>> On Thu, Mar 07, 2019 at 01:34:04PM +0800, Gao Xiang wrote:
>>>> commit a112152f6f3a2a88caa6f414d540bd49e406af60 upstream.
>>>>
>>>> EROFS has an optimized path called TAIL merging, which is designed
>>>> to merge multiple reads and the corresponding decompressions into
>>>> one if these requests read continuous pages almost at the same time.
>>>>
>>>> In general, it behaves as follows:
>>>>  ________________________________________________________________
>>>>   ... |  TAIL  .  HEAD  |  PAGE  |  PAGE  |  TAIL    . HEAD | ...
>>>>  _____|_combined page A_|________|________|_combined page B_|____
>>>>         1  ]  ->  [  2                          ]  ->  [ 3
>>>> If the above three reads are requested in the order 1-2-3, it will
>>>> generate a large work chain rather than 3 individual work chains
>>>> to reduce scheduling overhead and boost up sequential read.
>>>>
>>>> However, if Read 2 is processed slightly earlier than Read 1,
>>>> currently it still generates 2 individual work chains (chain 1, 2)
>>>> but it does in-place decompression for combined page A, moreover,
>>>> if chain 2 decompresses ahead of chain 1, it will be a race and
>>>> lead to corrupted decompressed page. This patch fixes it.
>>>>
>>>> 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>
>>>> ---
>>>>  drivers/staging/erofs/unzip_vle.c | 69 +++++++++++++++++++++++++--------------
>>>>  1 file changed, 44 insertions(+), 25 deletions(-)
>>>>
>>>
>>> thanks for the backport, now applied.
>>
>> Yes, it seems that Sasha's bot cannot cherry-pick it directly,
>> therefore I made this patch for 4.19 by hand...
>>
>> BTW, there is another patch called "staging: erofs: compressed_pages should not be accessed again after freed"
>> in the same condition... And I also send 4.19 and 4.20 version to stable mailing list,
>> could you also kindly check them out?
> 
> Yes, please give me a chance to catch up on stable pending patches to
> process:
> 
> 	$ mdfrm -c ~/mail/stable/
> 	585 messages in /home/gregkh/mail/stable/
> 
> You are in good company :)

Thanks for taking time.
I received more reports which shows some 5.1-rc patches are failed to be applied,
I will backport them later. :)

Thanks,
Gao Xiang

> 
> greg k-h
>