* [PATCH] erofs-utils: fix battach on full buffer block @ 2021-01-18 12:39 Hu Weiwen 2021-01-18 13:59 ` Gao Xiang 0 siblings, 1 reply; 17+ messages in thread From: Hu Weiwen @ 2021-01-18 12:39 UTC (permalink / raw) To: Gao Xiang, linux-erofs; +Cc: Hu Weiwen When __erofs_battach() is called on an buffer block of which (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' will not be updated correctly. This bug can be reproduced by: mkdir bug-repo head -c 4032 /dev/urandom > bug-repo/1 head -c 4095 /dev/urandom > bug-repo/2 head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo Then mount this image and see that file `3' in the image is different from `bug-repo/3'. This patch fix this by: * Don't inline tail-end data in this case, since the tail-end data will be in a different block from inode. * Correctly handle `battach' in this case. Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> --- lib/cache.c | 4 ++-- lib/inode.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cache.c b/lib/cache.c index 9e65429..464a43c 100644 --- a/lib/cache.c +++ b/lib/cache.c @@ -102,7 +102,7 @@ static int __erofs_battach(struct erofs_buffer_block *bb, bool dryrun) { const erofs_off_t alignedoffset = roundup(bb->buffers.off, alignsize); - const int oob = cmpsgn(roundup(bb->buffers.off % EROFS_BLKSIZ, + const int oob = cmpsgn(roundup((bb->buffers.off - 1) % EROFS_BLKSIZ + 1, alignsize) + incr + extrasize, EROFS_BLKSIZ); bool tailupdate = false; @@ -134,7 +134,7 @@ static int __erofs_battach(struct erofs_buffer_block *bb, tail_blkaddr = blkaddr + BLK_ROUND_UP(bb->buffers.off); erofs_bupdate_mapped(bb); } - return (alignedoffset + incr) % EROFS_BLKSIZ; + return (alignedoffset + incr - 1) % EROFS_BLKSIZ + 1; } int erofs_bh_balloon(struct erofs_buffer_head *bh, erofs_off_t incr) diff --git a/lib/inode.c b/lib/inode.c index 4ed6aed..d6a64cc 100644 --- a/lib/inode.c +++ b/lib/inode.c @@ -531,7 +531,7 @@ int erofs_prepare_tail_block(struct erofs_inode *inode) } /* expend a block as the tail block (should be successful) */ ret = erofs_bh_balloon(bh, EROFS_BLKSIZ); - DBG_BUGON(ret); + DBG_BUGON(ret < 0); return 0; } -- 2.30.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-18 12:39 [PATCH] erofs-utils: fix battach on full buffer block Hu Weiwen @ 2021-01-18 13:59 ` Gao Xiang 2021-01-18 15:52 ` 胡玮文 2021-01-19 6:02 ` 胡玮文 0 siblings, 2 replies; 17+ messages in thread From: Gao Xiang @ 2021-01-18 13:59 UTC (permalink / raw) To: Hu Weiwen; +Cc: linux-erofs Hi Weiwen, On Mon, Jan 18, 2021 at 08:39:45PM +0800, Hu Weiwen wrote: > When __erofs_battach() is called on an buffer block of which > (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' will not be > updated correctly. This bug can be reproduced by: > > mkdir bug-repo > head -c 4032 /dev/urandom > bug-repo/1 > head -c 4095 /dev/urandom > bug-repo/2 > head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size > mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo > > Then mount this image and see that file `3' in the image is different > from `bug-repo/3'. > > This patch fix this by: > > * Don't inline tail-end data in this case, since the tail-end data will > be in a different block from inode. > * Correctly handle `battach' in this case. > I will evaluate this condition later, yet if you have some interest and extra time, could you also help on writing a regression testcase for this, so we can look after such regression in case of the future code changes? This is also an ongoing work for the next erofs-utils release, see: https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/ Thanks, Gao Xiang ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-18 13:59 ` Gao Xiang @ 2021-01-18 15:52 ` 胡玮文 2021-01-18 23:28 ` Gao Xiang 2021-01-19 6:02 ` 胡玮文 1 sibling, 1 reply; 17+ messages in thread From: 胡玮文 @ 2021-01-18 15:52 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-erofs Hi Xiang, I would like to. If I understand it correctly, tests should based on experimental-tests branch, and be submitted to this mail-list, too? I wonder if we already have some CI service set up to run these tests automatically? I saw a mail from Travis CI in the mail-list archive, but it seems I don't have access to that organization. On Mon, Jan 18, 2021 at 09:59:16PM +0800, Gao Xiang wrote: > Hi Weiwen, > > On Mon, Jan 18, 2021 at 08:39:45PM +0800, Hu Weiwen wrote: > > When __erofs_battach() is called on an buffer block of which > > (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' will not be > > updated correctly. This bug can be reproduced by: > > > > mkdir bug-repo > > head -c 4032 /dev/urandom > bug-repo/1 > > head -c 4095 /dev/urandom > bug-repo/2 > > head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size > > mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo > > > > Then mount this image and see that file `3' in the image is different > > from `bug-repo/3'. > > > > This patch fix this by: > > > > * Don't inline tail-end data in this case, since the tail-end data will > > be in a different block from inode. > > * Correctly handle `battach' in this case. > > > > I will evaluate this condition later, yet if you have some interest > and extra time, could you also help on writing a regression testcase > for this, so we can look after such regression in case of the future > code changes? > > This is also an ongoing work for the next erofs-utils release, see: > https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/ > > Thanks, > Gao Xiang ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-18 15:52 ` 胡玮文 @ 2021-01-18 23:28 ` Gao Xiang 0 siblings, 0 replies; 17+ messages in thread From: Gao Xiang @ 2021-01-18 23:28 UTC (permalink / raw) To: 胡玮文; +Cc: linux-erofs On Mon, Jan 18, 2021 at 11:52:05PM +0800, 胡玮文 wrote: > Hi Xiang, > > I would like to. If I understand it correctly, tests should based on > experimental-tests branch, and be submitted to this mail-list, too? > > I wonder if we already have some CI service set up to run these tests > automatically? I saw a mail from Travis CI in the mail-list archive, but it > seems I don't have access to that organization. A travis CI was once used for avoiding erofs-utils build regression... I haven't set up a CI to run such regression tests (since it's still in experimental branch...). Also, it seems that travis CI.org will be obsoleted in the near future, and I'm not sure if I like travisCI.com... Therefore, github action would be better (recently I tried to autobuild erofs-utils deb by using github action and it seems fine... will investigate to automaticly run regression tests when I have extra free slots...) Thanks, Gao Xiang ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-18 13:59 ` Gao Xiang 2021-01-18 15:52 ` 胡玮文 @ 2021-01-19 6:02 ` 胡玮文 2021-01-19 15:43 ` Gao Xiang 1 sibling, 1 reply; 17+ messages in thread From: 胡玮文 @ 2021-01-19 6:02 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-erofs Hi Xiang, After further investgate, this bug will not reveal in any released version of mkfs.erofs. Previous patch v5 [1] will map all allocated bb when erofs_mapbh() is called on an already mapped bb, which triggers this bug. before that patch, under the same condition, __erofs_battach() will only be called on bb which is not mapped, thus no need to update `tail_blkaddr'. [1]: https://lore.kernel.org/linux-erofs/20210118123431.22533-1-sehuww@mail.scut.edu.cn/ Hu Weiwen On Mon, Jan 18, 2021 at 09:59:16PM +0800, Gao Xiang wrote: > Hi Weiwen, > > On Mon, Jan 18, 2021 at 08:39:45PM +0800, Hu Weiwen wrote: > > When __erofs_battach() is called on an buffer block of which > > (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' will not be > > updated correctly. This bug can be reproduced by: > > > > mkdir bug-repo > > head -c 4032 /dev/urandom > bug-repo/1 > > head -c 4095 /dev/urandom > bug-repo/2 > > head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size > > mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo > > > > Then mount this image and see that file `3' in the image is different > > from `bug-repo/3'. > > > > This patch fix this by: > > > > * Don't inline tail-end data in this case, since the tail-end data will > > be in a different block from inode. > > * Correctly handle `battach' in this case. > > > > I will evaluate this condition later, yet if you have some interest > and extra time, could you also help on writing a regression testcase > for this, so we can look after such regression in case of the future > code changes? > > This is also an ongoing work for the next erofs-utils release, see: > https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/ > > Thanks, > Gao Xiang ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-19 6:02 ` 胡玮文 @ 2021-01-19 15:43 ` Gao Xiang 2021-01-20 4:57 ` 胡玮文 0 siblings, 1 reply; 17+ messages in thread From: Gao Xiang @ 2021-01-19 15:43 UTC (permalink / raw) To: 胡玮文; +Cc: linux-erofs Hi Weiwen, On Tue, Jan 19, 2021 at 02:02:56PM +0800, 胡玮文 wrote: > Hi Xiang, > > After further investgate, this bug will not reveal in any released version of > mkfs.erofs. Previous patch v5 [1] will map all allocated bb when erofs_mapbh() > is called on an already mapped bb, which triggers this bug. before that patch, > under the same condition, __erofs_battach() will only be called on bb which is > not mapped, thus no need to update `tail_blkaddr'. Good to know this, thanks! I haven't looked into that (I will test it this weekend.) IMO, although this is not a regression, yet it seems it's potential harmful if we didn't notice this... So I think a proper testcase is still useful to look after this... If you have extra time, could you help on it? Also, without the detail of this, I think the fix might be folded into the original patchset (could you resend it?). In addition, I think after last_mapped_block is introduced, we might not need tail_blkaddr anymore, not sure. But I'm very cautious about this in case of introducing any new regression... Thanks, Gao Xiang > > [1]: https://lore.kernel.org/linux-erofs/20210118123431.22533-1-sehuww@mail.scut.edu.cn/ > > Hu Weiwen > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-19 15:43 ` Gao Xiang @ 2021-01-20 4:57 ` 胡玮文 2021-01-20 5:12 ` Gao Xiang 0 siblings, 1 reply; 17+ messages in thread From: 胡玮文 @ 2021-01-20 4:57 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-erofs [-- Attachment #1: Type: text/plain, Size: 2101 bytes --] > 在 2021年1月19日，23:43，Gao Xiang <hsiangkao@redhat.com> 写道： > > Hi Weiwen, > >> On Tue, Jan 19, 2021 at 02:02:56PM +0800, 胡玮文 wrote: >> Hi Xiang, >> >> After further investgate, this bug will not reveal in any released version of >> mkfs.erofs. Previous patch v5 [1] will map all allocated bb when erofs_mapbh() >> is called on an already mapped bb, which triggers this bug. before that patch, >> under the same condition, __erofs_battach() will only be called on bb which is >> not mapped, thus no need to update `tail_blkaddr'. > > Good to know this, thanks! I haven't looked into that (I will test it this > weekend.) IMO, although this is not a regression, yet it seems it's potential > harmful if we didn't notice this... So I think a proper testcase is still > useful to look after this... If you have extra time, could you help on it? Hi Xiang, I’m working on this. I have written a test case for this. And I’m also working on setting up GitHub actions to run tests automatically. So far, I’ve got uncompressed tests works, but when lz4 is enable, all test (except 001) fail. I have not found out why. You may see my progress at https://github.com/huww98/erofs-utils/tree/experimental-tests. I will send patches once everything is sorted out. > Also, without the detail of this, I think the fix might be folded into > the original patchset (could you resend it?). In addition, I think after You mean add a new commit [PATCH v6 3/3], or merge it into [PATCH v7 2/2]? I send it as a separate patch set because it may be merged independent of the cache.c optimization. > last_mapped_block is introduced, we might not need tail_blkaddr anymore, > not sure. But I'm very cautious about this in case of introducing any > new regression... I think we still need it, because already mapped bb may be dropped, last_map_block does not always reflect tail_blkaddr. Hu Weiwen > Thanks, > Gao Xiang > >> >> [1]: https://lore.kernel.org/linux-erofs/20210118123431.22533-1-sehuww@mail.scut.edu.cn/ >> >> Hu Weiwen >> [-- Attachment #2: Type: text/html, Size: 3501 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-20 4:57 ` 胡玮文 @ 2021-01-20 5:12 ` Gao Xiang 2021-01-21 6:07 ` 胡玮文 2021-01-21 16:26 ` [PATCH v2] " Hu Weiwen 0 siblings, 2 replies; 17+ messages in thread From: Gao Xiang @ 2021-01-20 5:12 UTC (permalink / raw) To: 胡玮文; +Cc: linux-erofs Hi Weiwen, On Wed, Jan 20, 2021 at 12:57:39PM +0800, 胡玮文 wrote: > > > 在 2021年1月19日，23:43，Gao Xiang <hsiangkao@redhat.com> 写道： > > > > Hi Weiwen, > > > >> On Tue, Jan 19, 2021 at 02:02:56PM +0800, 胡玮文 wrote: > >> Hi Xiang, > >> > >> After further investgate, this bug will not reveal in any released version of > >> mkfs.erofs. Previous patch v5 [1] will map all allocated bb when erofs_mapbh() > >> is called on an already mapped bb, which triggers this bug. before that patch, > >> under the same condition, __erofs_battach() will only be called on bb which is > >> not mapped, thus no need to update `tail_blkaddr'. > > > > Good to know this, thanks! I haven't looked into that (I will test it this > > weekend.) IMO, although this is not a regression, yet it seems it's potential > > harmful if we didn't notice this... So I think a proper testcase is still > > useful to look after this... If you have extra time, could you help on it? > > Hi Xiang, > > I’m working on this. I have written a test case for this. And I’m also working on setting up GitHub actions to run tests automatically. So far, I’ve got uncompressed tests works, but when lz4 is enable, all test (except 001) fail. I have not found out why. You may see my progress at https://github.com/huww98/erofs-utils/tree/experimental-tests. I will send patches once everything is sorted out. It would be better to know which kernel version github action is used (at least it seems no good if version is < 5.4)? also could you confirm the lz4 version as well (lz4-1.9.3)? if erofsfuse is used, specify "FSTYP=erofsfuse make check" to test it. The temporary results are in "tests/results/", could you also check and debug it? (please kindly confirm the testcases work well on your local computer, since such testcase is still WIP, I'm not sure if it has some running issues as well) > > > Also, without the detail of this, I think the fix might be folded into > > the original patchset (could you resend it?). In addition, I think after > > You mean add a new commit [PATCH v6 3/3], or merge it into [PATCH v7 2/2]? I send it as a separate patch set because it may be merged independent of the cache.c optimization. Resend v7 and fold it into [v7 2/2] would be better... > > > last_mapped_block is introduced, we might not need tail_blkaddr anymore, > > not sure. But I'm very cautious about this in case of introducing any > > new regression... > > I think we still need it, because already mapped bb may be dropped, last_map_block does not always reflect tail_blkaddr. Okay, that makes sense... Thanks, Gao Xiang > > Hu Weiwen > > > Thanks, > > Gao Xiang > > > >> > >> [1]: https://lore.kernel.org/linux-erofs/20210118123431.22533-1-sehuww@mail.scut.edu.cn/ > >> > >> Hu Weiwen > >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-20 5:12 ` Gao Xiang @ 2021-01-21 6:07 ` 胡玮文 2021-01-21 7:22 ` 胡玮文 2021-01-21 9:22 ` Gao Xiang 2021-01-21 16:26 ` [PATCH v2] " Hu Weiwen 1 sibling, 2 replies; 17+ messages in thread From: 胡玮文 @ 2021-01-21 6:07 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-erofs Hi Xiang, On Wed, Jan 20, 2021 at 01:12:16PM +0800, Gao Xiang wrote: > Hi Weiwen, > > On Wed, Jan 20, 2021 at 12:57:39PM +0800, 胡玮文 wrote: > > > > > 在 2021年1月19日，23:43，Gao Xiang <hsiangkao@redhat.com> 写道： > > > > > > Hi Weiwen, > > > > > >> On Tue, Jan 19, 2021 at 02:02:56PM +0800, 胡玮文 wrote: > > >> Hi Xiang, > > >> > > >> After further investgate, this bug will not reveal in any released version of > > >> mkfs.erofs. Previous patch v5 [1] will map all allocated bb when erofs_mapbh() > > >> is called on an already mapped bb, which triggers this bug. before that patch, > > >> under the same condition, __erofs_battach() will only be called on bb which is > > >> not mapped, thus no need to update `tail_blkaddr'. > > > > > > Good to know this, thanks! I haven't looked into that (I will test it this > > > weekend.) IMO, although this is not a regression, yet it seems it's potential > > > harmful if we didn't notice this... So I think a proper testcase is still > > > useful to look after this... If you have extra time, could you help on it? > > > > Hi Xiang, > > > > I’m working on this. I have written a test case for this. And I’m also working on setting up GitHub actions to run tests automatically. So far, I’ve got uncompressed tests works, but when lz4 is enable, all test (except 001) fail. I have not found out why. You may see my progress at https://github.com/huww98/erofs-utils/tree/experimental-tests. I will send patches once everything is sorted out. > > It would be better to know which kernel version github action is used (at least > it seems no good if version is < 5.4)? also could you confirm the lz4 version > as well (lz4-1.9.3)? if erofsfuse is used, specify "FSTYP=erofsfuse make check" > to test it. I've verified kernel version is 5.4.0-1032-azure for ubuntu-20.04, and erofs mount succeeded. for lz4, I'm installing v1.9.3 manually from source. I haven't tried fuse, will give it a try later. > The temporary results are in "tests/results/", could you also check and debug > it? (please kindly confirm the testcases work well on your local computer, > since such testcase is still WIP, I'm not sure if it has some running issues > as well) I've downloaded "tests/results/" and it's test 007 (check for bad lz4 versions) that fails with output "test LZ4_compress_HC_destSize(1048576) error (4098 < 4116)". And it's the same error on my PC. Investigating. BTW, why not use a more meaningful name for each test rather than a sequence number? Hu Weiwen > > > > > Also, without the detail of this, I think the fix might be folded into > > > the original patchset (could you resend it?). In addition, I think after > > > > You mean add a new commit [PATCH v6 3/3], or merge it into [PATCH v7 2/2]? I send it as a separate patch set because it may be merged independent of the cache.c optimization. > > Resend v7 and fold it into [v7 2/2] would be better... > > > > > > last_mapped_block is introduced, we might not need tail_blkaddr anymore, > > > not sure. But I'm very cautious about this in case of introducing any > > > new regression... > > > > I think we still need it, because already mapped bb may be dropped, last_map_block does not always reflect tail_blkaddr. > > Okay, that makes sense... > > Thanks, > Gao Xiang > > > > > Hu Weiwen > > > > > Thanks, > > > Gao Xiang > > > > > >> > > >> [1]: https://lore.kernel.org/linux-erofs/20210118123431.22533-1-sehuww@mail.scut.edu.cn/ > > >> > > >> Hu Weiwen > > >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-21 6:07 ` 胡玮文 @ 2021-01-21 7:22 ` 胡玮文 2021-01-21 9:22 ` Gao Xiang 1 sibling, 0 replies; 17+ messages in thread From: 胡玮文 @ 2021-01-21 7:22 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-erofs On Thu, Jan 21, 2021 at 02:07:38PM +0800, 胡玮文 wrote: > Hi Xiang, > > On Wed, Jan 20, 2021 at 01:12:16PM +0800, Gao Xiang wrote: > > Hi Weiwen, > > > > On Wed, Jan 20, 2021 at 12:57:39PM +0800, 胡玮文 wrote: > > > > > > > 在 2021年1月19日，23:43，Gao Xiang <hsiangkao@redhat.com> 写道： > > > > > > > > Hi Weiwen, > > > > > > > >> On Tue, Jan 19, 2021 at 02:02:56PM +0800, 胡玮文 wrote: > > > >> Hi Xiang, > > > >> > > > >> After further investgate, this bug will not reveal in any released version of > > > >> mkfs.erofs. Previous patch v5 [1] will map all allocated bb when erofs_mapbh() > > > >> is called on an already mapped bb, which triggers this bug. before that patch, > > > >> under the same condition, __erofs_battach() will only be called on bb which is > > > >> not mapped, thus no need to update `tail_blkaddr'. > > > > > > > > Good to know this, thanks! I haven't looked into that (I will test it this > > > > weekend.) IMO, although this is not a regression, yet it seems it's potential > > > > harmful if we didn't notice this... So I think a proper testcase is still > > > > useful to look after this... If you have extra time, could you help on it? > > > > > > Hi Xiang, > > > > > > I’m working on this. I have written a test case for this. And I’m also working on setting up GitHub actions to run tests automatically. So far, I’ve got uncompressed tests works, but when lz4 is enable, all test (except 001) fail. I have not found out why. You may see my progress at https://github.com/huww98/erofs-utils/tree/experimental-tests. I will send patches once everything is sorted out. > > > > It would be better to know which kernel version github action is used (at least > > it seems no good if version is < 5.4)? also could you confirm the lz4 version > > as well (lz4-1.9.3)? if erofsfuse is used, specify "FSTYP=erofsfuse make check" > > to test it. > > I've verified kernel version is 5.4.0-1032-azure for ubuntu-20.04, and erofs > mount succeeded. for lz4, I'm installing v1.9.3 manually from source. I haven't > tried fuse, will give it a try later. > > > The temporary results are in "tests/results/", could you also check and debug > > it? (please kindly confirm the testcases work well on your local computer, > > since such testcase is still WIP, I'm not sure if it has some running issues > > as well) > > I've downloaded "tests/results/" and it's test 007 (check for bad lz4 versions) > that fails with output "test LZ4_compress_HC_destSize(1048576) error (4098 < > 4116)". And it's the same error on my PC. Investigating. OK, I realized I just need to run ldconfig after installing new version of liblz4.so > BTW, why not use a more meaningful name for each test rather than a sequence > number? > > Hu Weiwen > > > > > > > > Also, without the detail of this, I think the fix might be folded into > > > > the original patchset (could you resend it?). In addition, I think after > > > > > > You mean add a new commit [PATCH v6 3/3], or merge it into [PATCH v7 2/2]? I send it as a separate patch set because it may be merged independent of the cache.c optimization. > > > > Resend v7 and fold it into [v7 2/2] would be better... > > > > > > > > > last_mapped_block is introduced, we might not need tail_blkaddr anymore, > > > > not sure. But I'm very cautious about this in case of introducing any > > > > new regression... > > > > > > I think we still need it, because already mapped bb may be dropped, last_map_block does not always reflect tail_blkaddr. > > > > Okay, that makes sense... > > > > Thanks, > > Gao Xiang > > > > > > > > Hu Weiwen > > > > > > > Thanks, > > > > Gao Xiang > > > > > > > >> > > > >> [1]: https://lore.kernel.org/linux-erofs/20210118123431.22533-1-sehuww@mail.scut.edu.cn/ > > > >> > > > >> Hu Weiwen > > > >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] erofs-utils: fix battach on full buffer block 2021-01-21 6:07 ` 胡玮文 2021-01-21 7:22 ` 胡玮文 @ 2021-01-21 9:22 ` Gao Xiang 1 sibling, 0 replies; 17+ messages in thread From: Gao Xiang @ 2021-01-21 9:22 UTC (permalink / raw) To: 胡玮文; +Cc: linux-erofs On Thu, Jan 21, 2021 at 02:07:38PM +0800, 胡玮文 wrote: ... > > I've downloaded "tests/results/" and it's test 007 (check for bad lz4 versions) > that fails with output "test LZ4_compress_HC_destSize(1048576) error (4098 < > 4116)". And it's the same error on my PC. Investigating. > > BTW, why not use a more meaningful name for each test rather than a sequence > number? I'm not good at English naming (but such cases need stable names), also see xfstests: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/tests/xfs the meaningful description can be written down at tests/Makefile.am... Yet they have no interest in integrating such testcases (also generic/ cases are not useful for EROFS either), and EROFS has many specific optimized paths so I decided to make a light-weight regression testcases in erofs-utils to look after it and kernel EROFS... Thanks, Gao Xiang > > Hu Weiwen ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] erofs-utils: fix battach on full buffer block 2021-01-20 5:12 ` Gao Xiang 2021-01-21 6:07 ` 胡玮文 @ 2021-01-21 16:26 ` Hu Weiwen 2021-01-22 2:31 ` Gao Xiang ` (3 more replies) 1 sibling, 4 replies; 17+ messages in thread From: Hu Weiwen @ 2021-01-21 16:26 UTC (permalink / raw) To: hsiangkao; +Cc: linux-erofs When __erofs_battach() is called on an buffer block of which (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' will not be updated correctly. This bug can be reproduced by: mkdir bug-repo head -c 4032 /dev/urandom > bug-repo/1 head -c 4095 /dev/urandom > bug-repo/2 head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo Then mount this image and see that file `3' in the image is different from `bug-repo/3'. This patch fix this by: * Don't inline tail-end data in this case, since the tail-end data will be in a different block from inode. * Correctly handle `battach' in this case. Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> --- Hi Xiang, I still think send this as a seperate patch would be better. In previous v6 patch, I have fixed the erofs_mapbh() behaviour so that there should be no user-visible bug introduced in that patch. And this patch is almost unrelated to that optimization. Compared with v1, this version fixes an error when compression is enabled. Thanks, Hu Weiwen lib/cache.c | 4 ++-- lib/compress.c | 2 +- lib/inode.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/cache.c b/lib/cache.c index c9a8c50..a7de72d 100644 --- a/lib/cache.c +++ b/lib/cache.c @@ -102,7 +102,7 @@ static int __erofs_battach(struct erofs_buffer_block *bb, bool dryrun) { const erofs_off_t alignedoffset = roundup(bb->buffers.off, alignsize); - const int oob = cmpsgn(roundup(bb->buffers.off % EROFS_BLKSIZ, + const int oob = cmpsgn(roundup((bb->buffers.off - 1) % EROFS_BLKSIZ + 1, alignsize) + incr + extrasize, EROFS_BLKSIZ); bool tailupdate = false; @@ -134,7 +134,7 @@ static int __erofs_battach(struct erofs_buffer_block *bb, tail_blkaddr = blkaddr + BLK_ROUND_UP(bb->buffers.off); erofs_bupdate_mapped(bb); } - return (alignedoffset + incr) % EROFS_BLKSIZ; + return (alignedoffset + incr - 1) % EROFS_BLKSIZ + 1; } int erofs_bh_balloon(struct erofs_buffer_head *bh, erofs_off_t incr) diff --git a/lib/compress.c b/lib/compress.c index 2b1f93c..670ac72 100644 --- a/lib/compress.c +++ b/lib/compress.c @@ -457,7 +457,7 @@ int erofs_write_compressed_file(struct erofs_inode *inode) close(fd); ret = erofs_bh_balloon(bh, blknr_to_addr(compressed_blocks)); - DBG_BUGON(ret); + DBG_BUGON(ret < 0); erofs_info("compressed %s (%llu bytes) into %u blocks", inode->i_srcpath, (unsigned long long)inode->i_size, diff --git a/lib/inode.c b/lib/inode.c index 4ed6aed..d6a64cc 100644 --- a/lib/inode.c +++ b/lib/inode.c @@ -531,7 +531,7 @@ int erofs_prepare_tail_block(struct erofs_inode *inode) } /* expend a block as the tail block (should be successful) */ ret = erofs_bh_balloon(bh, EROFS_BLKSIZ); - DBG_BUGON(ret); + DBG_BUGON(ret < 0); return 0; } -- 2.30.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] erofs-utils: fix battach on full buffer block 2021-01-21 16:26 ` [PATCH v2] " Hu Weiwen @ 2021-01-22 2:31 ` Gao Xiang 2021-01-22 12:43 ` Gao Xiang ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Gao Xiang @ 2021-01-22 2:31 UTC (permalink / raw) To: Hu Weiwen; +Cc: linux-erofs Hi Weiwen, On Fri, Jan 22, 2021 at 12:26:06AM +0800, Hu Weiwen wrote: > When __erofs_battach() is called on an buffer block of which > (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' will not be > updated correctly. This bug can be reproduced by: > > mkdir bug-repo > head -c 4032 /dev/urandom > bug-repo/1 > head -c 4095 /dev/urandom > bug-repo/2 > head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size > mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo > > Then mount this image and see that file `3' in the image is different > from `bug-repo/3'. > > This patch fix this by: > > * Don't inline tail-end data in this case, since the tail-end data will > be in a different block from inode. > * Correctly handle `battach' in this case. > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > --- > Hi Xiang, > > I still think send this as a seperate patch would be better. In previous v6 > patch, I have fixed the erofs_mapbh() behaviour so that there should be no > user-visible bug introduced in that patch. And this patch is almost unrelated > to that optimization. > > Compared with v1, this version fixes an error when compression is enabled. I cannot tell too much of it for now (since it's not an obvious update, I need to investagate). I will leave it this weekend. Thanks, Gao Xiang > > Thanks, > Hu Weiwen ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] erofs-utils: fix battach on full buffer block 2021-01-21 16:26 ` [PATCH v2] " Hu Weiwen 2021-01-22 2:31 ` Gao Xiang @ 2021-01-22 12:43 ` Gao Xiang 2021-02-14 15:22 ` Gao Xiang via Linux-erofs 2021-02-14 16:00 ` [PATCH v3] erofs-utils: fix battach on full buffer blocks Gao Xiang via Linux-erofs 3 siblings, 0 replies; 17+ messages in thread From: Gao Xiang @ 2021-01-22 12:43 UTC (permalink / raw) To: Hu Weiwen; +Cc: linux-erofs Hi Weiwen, On Fri, Jan 22, 2021 at 12:26:06AM +0800, Hu Weiwen wrote: > When __erofs_battach() is called on an buffer block of which > (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' will not be > updated correctly. This bug can be reproduced by: > > mkdir bug-repo > head -c 4032 /dev/urandom > bug-repo/1 > head -c 4095 /dev/urandom > bug-repo/2 > head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size > mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo > > Then mount this image and see that file `3' in the image is different > from `bug-repo/3'. > > This patch fix this by: > > * Don't inline tail-end data in this case, since the tail-end data will > be in a different block from inode. > * Correctly handle `battach' in this case. > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > --- > Hi Xiang, > > I still think send this as a seperate patch would be better. In previous v6 > patch, I have fixed the erofs_mapbh() behaviour so that there should be no > user-visible bug introduced in that patch. And this patch is almost unrelated > to that optimization. > > Compared with v1, this version fixes an error when compression is enabled. > I have to say I still don't get the point of the whole description above and the patch itself honestly. even if (bb->buffers.off % EROFS_BLKSIZ == 0), the whole block can be used for tail-packing inline + inode. Assume that you testcase above is the case you addressed, could you elaborate them in detail? If the original behavior is no user-visiable, I'm not sure what issue you'd like to resolve... Thanks, Gao Xiang > Thanks, > Hu Weiwen ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] erofs-utils: fix battach on full buffer block 2021-01-21 16:26 ` [PATCH v2] " Hu Weiwen 2021-01-22 2:31 ` Gao Xiang 2021-01-22 12:43 ` Gao Xiang @ 2021-02-14 15:22 ` Gao Xiang via Linux-erofs 2021-02-14 16:00 ` [PATCH v3] erofs-utils: fix battach on full buffer blocks Gao Xiang via Linux-erofs 3 siblings, 0 replies; 17+ messages in thread From: Gao Xiang via Linux-erofs @ 2021-02-14 15:22 UTC (permalink / raw) To: Hu Weiwen; +Cc: linux-erofs Hi Weiwen, On Fri, Jan 22, 2021 at 12:26:06AM +0800, Hu Weiwen wrote: > When __erofs_battach() is called on an buffer block of which > (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' will not be > updated correctly. This bug can be reproduced by: > > mkdir bug-repo > head -c 4032 /dev/urandom > bug-repo/1 > head -c 4095 /dev/urandom > bug-repo/2 > head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size > mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo > > Then mount this image and see that file `3' in the image is different > from `bug-repo/3'. > > This patch fix this by: > > * Don't inline tail-end data in this case, since the tail-end data will > be in a different block from inode. > * Correctly handle `battach' in this case. > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > --- Now I get what you mentioned to, and I think this is a valid case and might influence old versions (even I have no idea how to reproduce it effectively.) So I tend to apply this patch right now, and thanks for your patch! Reviewed-by: Gao Xiang <hsiangkao@aol.com> With updated commit message below: When the subsequent erofs_battach() is called on an buffer block of which (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' won't be updated correctly. This bug can be reproduced by: mkdir bug-repo head -c 4032 /dev/urandom > bug-repo/1 head -c 4095 /dev/urandom > bug-repo/2 head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo Then mount this image and see that file `3' in the image is different from `bug-repo/3'. This patch fix this by: * Handle `oob' and `tail_blkaddr' for the case above properly; * Don't inline tail-packing data for such case, since the tail-packing data is actually in a different block from inode itself even kernel can handle such cases properly. Thanks, Gao Xiang ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3] erofs-utils: fix battach on full buffer blocks 2021-01-21 16:26 ` [PATCH v2] " Hu Weiwen ` (2 preceding siblings ...) 2021-02-14 15:22 ` Gao Xiang via Linux-erofs @ 2021-02-14 16:00 ` Gao Xiang via Linux-erofs 2021-02-22 15:34 ` Li GuiFu via Linux-erofs 3 siblings, 1 reply; 17+ messages in thread From: Gao Xiang via Linux-erofs @ 2021-02-14 16:00 UTC (permalink / raw) To: linux-erofs From: Hu Weiwen <sehuww@mail.scut.edu.cn> When the subsequent erofs_battach() is called on an buffer block of which (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' won't be updated correctly. This bug can be reproduced by: mkdir bug-repo head -c 4032 /dev/urandom > bug-repo/1 head -c 4095 /dev/urandom > bug-repo/2 head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo Then mount this image and see that file `3' in the image is different from `bug-repo/3'. This patch fix this by: * Handle `oob' and `tail_blkaddr' for the case above properly; * Don't inline tail-packing data for such case, since the tail-packing data is actually in a different block from inode itself even kernel can handle such cases properly. Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> Reviewed-by: Gao Xiang <hsiangkao@aol.com> Signed-off-by: Gao Xiang <hsiangkao@aol.com> --- changes since v2: - update commit message; - refine 2 asserts from < 0 to != EROFS_BLKSIZ. lib/cache.c | 4 ++-- lib/compress.c | 3 ++- lib/inode.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/cache.c b/lib/cache.c index 40d3b1f3f4d5..e3327c3f1586 100644 --- a/lib/cache.c +++ b/lib/cache.c @@ -102,7 +102,7 @@ static int __erofs_battach(struct erofs_buffer_block *bb, bool dryrun) { const erofs_off_t alignedoffset = roundup(bb->buffers.off, alignsize); - const int oob = cmpsgn(roundup(bb->buffers.off % EROFS_BLKSIZ, + const int oob = cmpsgn(roundup((bb->buffers.off - 1) % EROFS_BLKSIZ + 1, alignsize) + incr + extrasize, EROFS_BLKSIZ); bool tailupdate = false; @@ -134,7 +134,7 @@ static int __erofs_battach(struct erofs_buffer_block *bb, tail_blkaddr = blkaddr + BLK_ROUND_UP(bb->buffers.off); erofs_bupdate_mapped(bb); } - return (alignedoffset + incr) % EROFS_BLKSIZ; + return (alignedoffset + incr - 1) % EROFS_BLKSIZ + 1; } int erofs_bh_balloon(struct erofs_buffer_head *bh, erofs_off_t incr) diff --git a/lib/compress.c b/lib/compress.c index 2b1f93c389ff..4b685cd27080 100644 --- a/lib/compress.c +++ b/lib/compress.c @@ -456,8 +456,9 @@ int erofs_write_compressed_file(struct erofs_inode *inode) vle_write_indexes_final(&ctx); close(fd); + DBG_BUGON(!compressed_blocks); ret = erofs_bh_balloon(bh, blknr_to_addr(compressed_blocks)); - DBG_BUGON(ret); + DBG_BUGON(ret != EROFS_BLKSIZ); erofs_info("compressed %s (%llu bytes) into %u blocks", inode->i_srcpath, (unsigned long long)inode->i_size, diff --git a/lib/inode.c b/lib/inode.c index 6371aa563673..40189fed37dd 100644 --- a/lib/inode.c +++ b/lib/inode.c @@ -531,7 +531,7 @@ int erofs_prepare_tail_block(struct erofs_inode *inode) } /* expend a block as the tail block (should be successful) */ ret = erofs_bh_balloon(bh, EROFS_BLKSIZ); - DBG_BUGON(ret); + DBG_BUGON(ret != EROFS_BLKSIZ); return 0; } -- 2.24.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3] erofs-utils: fix battach on full buffer blocks 2021-02-14 16:00 ` [PATCH v3] erofs-utils: fix battach on full buffer blocks Gao Xiang via Linux-erofs @ 2021-02-22 15:34 ` Li GuiFu via Linux-erofs 0 siblings, 0 replies; 17+ messages in thread From: Li GuiFu via Linux-erofs @ 2021-02-22 15:34 UTC (permalink / raw) To: Gao Xiang, linux-erofs On 2021/2/15 0:00, Gao Xiang via Linux-erofs wrote: > From: Hu Weiwen <sehuww@mail.scut.edu.cn> > > When the subsequent erofs_battach() is called on an buffer block of > which (bb->buffers.off % EROFS_BLKSIZ == 0), `tail_blkaddr' won't be > updated correctly. This bug can be reproduced by: > > mkdir bug-repo > head -c 4032 /dev/urandom > bug-repo/1 > head -c 4095 /dev/urandom > bug-repo/2 > head -c 12345 /dev/urandom > bug-repo/3 # arbitrary size > mkfs.erofs -Eforce-inode-compact bug-repo.erofs.img bug-repo > Then mount this image and see that file `3' in the image is different > from `bug-repo/3'. > > This patch fix this by: > * Handle `oob' and `tail_blkaddr' for the case above properly; > * Don't inline tail-packing data for such case, since the tail-packing > data is actually in a different block from inode itself even kernel > can handle such cases properly. > > Signed-off-by: Hu Weiwen <sehuww@mail.scut.edu.cn> > Reviewed-by: Gao Xiang <hsiangkao@aol.com> > Signed-off-by: Gao Xiang <hsiangkao@aol.com> > --- It looks good Reviewed-by: Li Guifu <bluce.lee@aliyun.com> Thanks, ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-02-22 15:39 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-18 12:39 [PATCH] erofs-utils: fix battach on full buffer block Hu Weiwen 2021-01-18 13:59 ` Gao Xiang 2021-01-18 15:52 ` 胡玮文 2021-01-18 23:28 ` Gao Xiang 2021-01-19 6:02 ` 胡玮文 2021-01-19 15:43 ` Gao Xiang 2021-01-20 4:57 ` 胡玮文 2021-01-20 5:12 ` Gao Xiang 2021-01-21 6:07 ` 胡玮文 2021-01-21 7:22 ` 胡玮文 2021-01-21 9:22 ` Gao Xiang 2021-01-21 16:26 ` [PATCH v2] " Hu Weiwen 2021-01-22 2:31 ` Gao Xiang 2021-01-22 12:43 ` Gao Xiang 2021-02-14 15:22 ` Gao Xiang via Linux-erofs 2021-02-14 16:00 ` [PATCH v3] erofs-utils: fix battach on full buffer blocks Gao Xiang via Linux-erofs 2021-02-22 15:34 ` Li GuiFu via Linux-erofs
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).