linux-erofs.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [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	[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	[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	[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
on how to clone and mirror all data and code used for this inbox