From: Colin Ian King <[email protected]>
The while-loop iterates until src is non-null or i is 3, however, the
loop counter i is not intinitialied to zero, causing incorrect iteration
counts. Fix this by initializing it to zero.
Addresses-Coverity: ("Uninitialized scalar variable")
Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend")
Signed-off-by: Colin Ian King <[email protected]>
---
fs/erofs/decompressor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index fe46a9c34923..8687ff81406b 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -154,6 +154,7 @@ static void *z_erofs_handle_inplace_io(struct z_erofs_decompress_req *rq,
}
kunmap_atomic(inpage);
might_sleep();
+ i = 0;
while (1) {
src = vm_map_ram(rq->in, nrpages_in, -1);
/* retry two more times (totally 3 times) */
--
2.30.2
Hi Colin,
On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> The while-loop iterates until src is non-null or i is 3, however, the
> loop counter i is not intinitialied to zero, causing incorrect iteration
> counts. Fix this by initializing it to zero.
>
> Addresses-Coverity: ("Uninitialized scalar variable")
> Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend")
> Signed-off-by: Colin Ian King <[email protected]>
Thank you very much for catching this! It looks good to me,
Reviewed-by: Gao Xiang <[email protected]>
(btw, may I fold this into the original patchset? since such big pcluster
patchset is just applied to for-next for further integration testing, and
the commit id is not stable yet..)
Thanks,
Gao Xiang
> ---
> fs/erofs/decompressor.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index fe46a9c34923..8687ff81406b 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -154,6 +154,7 @@ static void *z_erofs_handle_inplace_io(struct z_erofs_decompress_req *rq,
> }
> kunmap_atomic(inpage);
> might_sleep();
> + i = 0;
> while (1) {
> src = vm_map_ram(rq->in, nrpages_in, -1);
> /* retry two more times (totally 3 times) */
> --
> 2.30.2
>
On Wed, 2021-04-07 at 07:54 +0800, Gao Xiang wrote:
> Hi Colin,
>
> On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote:
> > From: Colin Ian King <[email protected]>
> >
> > The while-loop iterates until src is non-null or i is 3, however, the
> > loop counter i is not intinitialied to zero, causing incorrect iteration
> > counts. Fix this by initializing it to zero.
> >
> > Addresses-Coverity: ("Uninitialized scalar variable")
> > Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend")
> > Signed-off-by: Colin Ian King <[email protected]>
>
> Thank you very much for catching this! It looks good to me,
> Reviewed-by: Gao Xiang <[email protected]>
>
> (btw, may I fold this into the original patchset? since such big pcluster
> ?patchset is just applied to for-next for further integration testing, and
> ?the commit id is not stable yet..)
>
> Thanks,
> Gao Xiang
I think this code is odd and would be more intelligible using
a for loop like:
---
fs/erofs/decompressor.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 27aa6a99b371..5a64f4649414 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -286,28 +286,24 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq,
}
ret = alg->prepare_destpages(rq, pagepool);
- if (ret < 0) {
+ if (ret < 0)
return ret;
- } else if (ret) {
+ if (ret) {
dst = page_address(*rq->out);
dst_maptype = 1;
goto dstmap_out;
}
- i = 0;
- while (1) {
+ for (i = 0; i < 3; i++) {
dst = vm_map_ram(rq->out, nrpages_out, -1);
-
+ if (dst) {
+ dst_maptype = 2;
+ goto dstmap_out;
+ }
/* retry two more times (totally 3 times) */
- if (dst || ++i >= 3)
- break;
vm_unmap_aliases();
}
-
- if (!dst)
- return -ENOMEM;
-
- dst_maptype = 2;
+ return -ENOMEM;
dstmap_out:
ret = alg->decompress(rq, dst + rq->pageofs_out);
Hi Joe,
On Tue, Apr 06, 2021 at 08:38:44PM -0700, Joe Perches wrote:
> On Wed, 2021-04-07 at 07:54 +0800, Gao Xiang wrote:
> > Hi Colin,
> >
> > On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote:
> > > From: Colin Ian King <[email protected]>
> > >
> > > The while-loop iterates until src is non-null or i is 3, however, the
> > > loop counter i is not intinitialied to zero, causing incorrect iteration
> > > counts. Fix this by initializing it to zero.
> > >
> > > Addresses-Coverity: ("Uninitialized scalar variable")
> > > Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend")
> > > Signed-off-by: Colin Ian King <[email protected]>
> >
> > Thank you very much for catching this! It looks good to me,
> > Reviewed-by: Gao Xiang <[email protected]>
> >
> > (btw, may I fold this into the original patchset? since such big pcluster
> > patchset is just applied to for-next for further integration testing, and
> > the commit id is not stable yet..)
> >
> > Thanks,
> > Gao Xiang
>
> I think this code is odd and would be more intelligible using
> a for loop like:
Thanks for your reply/suggestion.
> ---
> fs/erofs/decompressor.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
> index 27aa6a99b371..5a64f4649414 100644
> --- a/fs/erofs/decompressor.c
> +++ b/fs/erofs/decompressor.c
> @@ -286,28 +286,24 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq,
> }
>
> ret = alg->prepare_destpages(rq, pagepool);
> - if (ret < 0) {
> + if (ret < 0)
> return ret;
> - } else if (ret) {
> + if (ret) {
> dst = page_address(*rq->out);
> dst_maptype = 1;
> goto dstmap_out;
> }
I agree with the modification here, thanks!
>
> - i = 0;
> - while (1) {
> + for (i = 0; i < 3; i++) {
> dst = vm_map_ram(rq->out, nrpages_out, -1);
> -
> + if (dst) {
> + dst_maptype = 2;
> + goto dstmap_out;
> + }
> /* retry two more times (totally 3 times) */
> - if (dst || ++i >= 3)
> - break;
> vm_unmap_aliases();
That is not quite equivalent, since after trying more than 3 times,
(I think) no need to do the final vm_unmap_aliases(), since it's
only used for the next vm_map_ram(). Similar logic also see:
fs/xfs/xfs_buf.c: _xfs_buf_map_pages():
/*
* vm_map_ram() will allocate auxiliary structures (e.g.
* pagetables) with GFP_KERNEL, yet we are likely to be under
* GFP_NOFS context here. Hence we need to tell memory reclaim
* that we are in such a context via PF_MEMALLOC_NOFS to prevent
* memory reclaim re-entering the filesystem here and
* potentially deadlocking.
*/
nofs_flag = memalloc_nofs_save();
do {
bp->b_addr = vm_map_ram(bp->b_pages, bp->b_page_count,
-1);
if (bp->b_addr)
break;
vm_unmap_aliases();
} while (retried++ <= 1);
memalloc_nofs_restore(nofs_flag);
if (!bp->b_addr)
return -ENOMEM;
but yeah with some modification (and extra vm_unmap_aliases() here
as well...)
Thanks,
Gao Xiang