From: Yue Hu <[email protected]>
Note that z_erofs_lz4_decompress() will return a positive value if
decompression succeeds. However, we do not copy_from_pcpubuf() due
to !ret. Let's fix it.
Signed-off-by: Yue Hu <[email protected]>
---
fs/erofs/decompressor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index a5bc4b1..e4cab4e 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -326,7 +326,7 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq,
rq->inplace_io = false;
ret = alg->decompress(rq, dst);
- if (!ret)
+ if (ret > 0)
copy_from_pcpubuf(rq->out, dst, rq->pageofs_out,
rq->outputsize);
--
1.9.1
Hi Yue,
On Wed, Oct 13, 2021 at 05:29:05PM +0800, Yue Hu wrote:
> From: Yue Hu <[email protected]>
>
> Note that z_erofs_lz4_decompress() will return a positive value if
> decompression succeeds. However, we do not copy_from_pcpubuf() due
> to !ret. Let's fix it.
>
> Signed-off-by: Yue Hu <[email protected]>
Thanks for catching this. This is a valid issue, but it has no real
impact to the current kernels since such pcluster in practice will be
!inplace_io and trigger "if (nrpages_out == 1 && !rq->inplace_io) {"
above for upstream strategies.
Our customized lz4 implementation will return 0 if success instead, so
it has no issue to our previous products as well.
For such cases, how about updating z_erofs_lz4_decompress() to return
0 if success instead rather than outputsize. Since I'll return 0 if
success for LZMA as well.
Thanks,
Gao Xiang
On Wed, Oct 13, 2021 at 07:51:55PM +0800, Gao Xiang wrote:
> Hi Yue,
>
> On Wed, Oct 13, 2021 at 05:29:05PM +0800, Yue Hu wrote:
> > From: Yue Hu <[email protected]>
> >
> > Note that z_erofs_lz4_decompress() will return a positive value if
> > decompression succeeds. However, we do not copy_from_pcpubuf() due
> > to !ret. Let's fix it.
> >
> > Signed-off-by: Yue Hu <[email protected]>
>
> Thanks for catching this. This is a valid issue, but it has no real
> impact to the current kernels since such pcluster in practice will be
> !inplace_io and trigger "if (nrpages_out == 1 && !rq->inplace_io) {"
> above for upstream strategies.
>
> Our customized lz4 implementation will return 0 if success instead, so
> it has no issue to our previous products as well.
>
> For such cases, how about updating z_erofs_lz4_decompress() to return
> 0 if success instead rather than outputsize. Since I'll return 0 if
> success for LZMA as well.
In addition, I'm fine to getting rid of such path as well, since it has
no real impact to our current upstream decompression strategy.
If it has some use cases due to decompression strategy change, we could
re-add it with some evaluation (some real numbers) later.
Thanks,
Gao Xiang
>
> Thanks,
> Gao Xiang
Hi Xiang,
On Wed, 13 Oct 2021 19:51:55 +0800
Gao Xiang <[email protected]> wrote:
> Hi Yue,
>
> On Wed, Oct 13, 2021 at 05:29:05PM +0800, Yue Hu wrote:
> > From: Yue Hu <[email protected]>
> >
> > Note that z_erofs_lz4_decompress() will return a positive value if
> > decompression succeeds. However, we do not copy_from_pcpubuf() due
> > to !ret. Let's fix it.
> >
> > Signed-off-by: Yue Hu <[email protected]>
>
> Thanks for catching this. This is a valid issue, but it has no real
> impact to the current kernels since such pcluster in practice will be
> !inplace_io and trigger "if (nrpages_out == 1 && !rq->inplace_io) {"
> above for upstream strategies.
>
> Our customized lz4 implementation will return 0 if success instead, so
> it has no issue to our previous products as well.
Yes, i just find the issue when i try to implement a new feature of
tail-packing inline compressed data. No problem in my current version.
Thanks.
>
> For such cases, how about updating z_erofs_lz4_decompress() to return
> 0 if success instead rather than outputsize. Since I'll return 0 if
> success for LZMA as well.
>
> Thanks,
> Gao Xiang
On Wed, Oct 13, 2021 at 09:10:05PM +0800, Yue Hu wrote:
> Hi Xiang,
>
> On Wed, 13 Oct 2021 19:51:55 +0800
> Gao Xiang <[email protected]> wrote:
>
> > Hi Yue,
> >
> > On Wed, Oct 13, 2021 at 05:29:05PM +0800, Yue Hu wrote:
> > > From: Yue Hu <[email protected]>
> > >
> > > Note that z_erofs_lz4_decompress() will return a positive value if
> > > decompression succeeds. However, we do not copy_from_pcpubuf() due
> > > to !ret. Let's fix it.
> > >
> > > Signed-off-by: Yue Hu <[email protected]>
> >
> > Thanks for catching this. This is a valid issue, but it has no real
> > impact to the current kernels since such pcluster in practice will be
> > !inplace_io and trigger "if (nrpages_out == 1 && !rq->inplace_io) {"
> > above for upstream strategies.
> >
> > Our customized lz4 implementation will return 0 if success instead, so
> > it has no issue to our previous products as well.
>
> Yes, i just find the issue when i try to implement a new feature of
> tail-packing inline compressed data. No problem in my current version.
Yeah, please help update the return value of z_erofs_lz4_decompress()
and get rid of such unneeded fast path.
Thanks,
Gao Xiang
>
> Thanks.
>
> >
> > For such cases, how about updating z_erofs_lz4_decompress() to return
> > 0 if success instead rather than outputsize. Since I'll return 0 if
> > success for LZMA as well.
> >
> > Thanks,
> > Gao Xiang
>
On Thu, 14 Oct 2021 00:03:02 +0800
Gao Xiang <[email protected]> wrote:
> On Wed, Oct 13, 2021 at 09:10:05PM +0800, Yue Hu wrote:
> > Hi Xiang,
> >
> > On Wed, 13 Oct 2021 19:51:55 +0800
> > Gao Xiang <[email protected]> wrote:
> >
> > > Hi Yue,
> > >
> > > On Wed, Oct 13, 2021 at 05:29:05PM +0800, Yue Hu wrote:
> > > > From: Yue Hu <[email protected]>
> > > >
> > > > Note that z_erofs_lz4_decompress() will return a positive value if
> > > > decompression succeeds. However, we do not copy_from_pcpubuf() due
> > > > to !ret. Let's fix it.
> > > >
> > > > Signed-off-by: Yue Hu <[email protected]>
> > >
> > > Thanks for catching this. This is a valid issue, but it has no real
> > > impact to the current kernels since such pcluster in practice will be
> > > !inplace_io and trigger "if (nrpages_out == 1 && !rq->inplace_io) {"
> > > above for upstream strategies.
> > >
> > > Our customized lz4 implementation will return 0 if success instead, so
> > > it has no issue to our previous products as well.
> >
> > Yes, i just find the issue when i try to implement a new feature of
> > tail-packing inline compressed data. No problem in my current version.
>
> Yeah, please help update the return value of z_erofs_lz4_decompress()
> and get rid of such unneeded fast path.
OK, will update in next version.
Thanks.
>
> Thanks,
> Gao Xiang
>
> >
> > Thanks.
> >
> > >
> > > For such cases, how about updating z_erofs_lz4_decompress() to return
> > > 0 if success instead rather than outputsize. Since I'll return 0 if
> > > success for LZMA as well.
> > >
> > > Thanks,
> > > Gao Xiang
> >