* [PATCH] xfs/097: Remove wrong broken assignment operation
@ 2019-10-07 7:15 Yang Xu
2019-10-07 15:12 ` Darrick J. Wong
0 siblings, 1 reply; 10+ messages in thread
From: Yang Xu @ 2019-10-07 7:15 UTC (permalink / raw)
To: fstests; +Cc: Yang Xu
On old kernel, since commit ded188b8609 ("xfs: Fix the situation that mount
operation rejects corrupted XFS") running this case got the mismatched output,
as below:
-----------------------------------
+ check fs
+ corrupt image
+ mount image && modify files
-broken: 1
+broken: 0
+ repair fs
+ mount image (2)
------------------------------------
It fails because the broken is always equal to 0 when _try_scratch_mount
succeed. So remove this wrong assignment operation.
Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
tests/xfs/097 | 2 --
1 file changed, 2 deletions(-)
diff --git a/tests/xfs/097 b/tests/xfs/097
index 1cb7d69c..20791738 100755
--- a/tests/xfs/097
+++ b/tests/xfs/097
@@ -81,8 +81,6 @@ done
echo "+ mount image && modify files"
broken=1
if _try_scratch_mount >> $seqres.full 2>&1; then
-
- broken=0
for x in `seq 65 70`; do
touch "${TESTFILE}.${x}" 2> /dev/null && broken=0
done
--
2.18.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-07 7:15 [PATCH] xfs/097: Remove wrong broken assignment operation Yang Xu @ 2019-10-07 15:12 ` Darrick J. Wong 2019-10-08 2:39 ` Yang Xu 0 siblings, 1 reply; 10+ messages in thread From: Darrick J. Wong @ 2019-10-07 15:12 UTC (permalink / raw) To: Yang Xu; +Cc: fstests On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: > On old kernel, since commit ded188b8609 ("xfs: Fix the situation that mount > operation rejects corrupted XFS") running this case got the mismatched output, > as below: But why did the output mismatch? Did the fs heal itself? Did allocating 5 more files somehow avoid touching the finobt? Is the assignment logic in the loop broken? --D > ----------------------------------- > + check fs > + corrupt image > + mount image && modify files > -broken: 1 > +broken: 0 > + repair fs > + mount image (2) > ------------------------------------ > > It fails because the broken is always equal to 0 when _try_scratch_mount > succeed. So remove this wrong assignment operation. > > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > --- > tests/xfs/097 | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/tests/xfs/097 b/tests/xfs/097 > index 1cb7d69c..20791738 100755 > --- a/tests/xfs/097 > +++ b/tests/xfs/097 > @@ -81,8 +81,6 @@ done > echo "+ mount image && modify files" > broken=1 > if _try_scratch_mount >> $seqres.full 2>&1; then > - > - broken=0 > for x in `seq 65 70`; do > touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 > done > -- > 2.18.1 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-07 15:12 ` Darrick J. Wong @ 2019-10-08 2:39 ` Yang Xu 2019-10-14 16:39 ` Darrick J. Wong 0 siblings, 1 reply; 10+ messages in thread From: Yang Xu @ 2019-10-08 2:39 UTC (permalink / raw) To: Darrick J. Wong; +Cc: fstests on 2019/10/07 23:12, Darrick J. Wong wrote: > On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: >> On old kernel, since commit ded188b8609 ("xfs: Fix the situation that mount >> operation rejects corrupted XFS") running this case got the mismatched output, >> as below: > > But why did the output mismatch? Did the fs heal itself? Did > allocating 5 more files somehow avoid touching the finobt? Is the > assignment logic in the loop broken? The output mismatch because on old kernel, we can mount the corrupted xfs and touch action will be refused. so broken is equal to 0. The fs doesn't heal ifself. allocating 5 more file will touch the finobt. You can see this url https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b eg xfs/087, they all delete "broken=0" before allocationg 5 more file. commit ded188b86 compatibled old kernel(permit mount and refuse touch) and new kernel(refuse mount) behavior on corrupted xfs. Or, I misunderstand this case? > > --D > >> ----------------------------------- >> + check fs >> + corrupt image >> + mount image && modify files >> -broken: 1 >> +broken: 0 >> + repair fs >> + mount image (2) >> ------------------------------------ >> >> It fails because the broken is always equal to 0 when _try_scratch_mount >> succeed. So remove this wrong assignment operation. >> >> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >> --- >> tests/xfs/097 | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/tests/xfs/097 b/tests/xfs/097 >> index 1cb7d69c..20791738 100755 >> --- a/tests/xfs/097 >> +++ b/tests/xfs/097 >> @@ -81,8 +81,6 @@ done >> echo "+ mount image && modify files" >> broken=1 >> if _try_scratch_mount >> $seqres.full 2>&1; then >> - >> - broken=0 >> for x in `seq 65 70`; do >> touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 >> done >> -- >> 2.18.1 >> >> >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-08 2:39 ` Yang Xu @ 2019-10-14 16:39 ` Darrick J. Wong 2019-10-15 6:27 ` Yang Xu 0 siblings, 1 reply; 10+ messages in thread From: Darrick J. Wong @ 2019-10-14 16:39 UTC (permalink / raw) To: Yang Xu; +Cc: fstests On Tue, Oct 08, 2019 at 10:39:59AM +0800, Yang Xu wrote: > > > on 2019/10/07 23:12, Darrick J. Wong wrote: > > On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: > > > On old kernel, since commit ded188b8609 ("xfs: Fix the situation that mount > > > operation rejects corrupted XFS") running this case got the mismatched output, > > > as below: > > > > But why did the output mismatch? Did the fs heal itself? Did > > allocating 5 more files somehow avoid touching the finobt? Is the > > assignment logic in the loop broken? > > The output mismatch because on old kernel, we can mount the corrupted xfs > and touch action will be refused. so broken is equal to 0. > The fs doesn't heal ifself. > allocating 5 more file will touch the finobt. > > You can see this url > https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b > > eg xfs/087, they all delete "broken=0" before allocationg 5 more file. > commit ded188b86 compatibled old kernel(permit mount and refuse touch) and > new kernel(refuse mount) behavior on corrupted xfs. Or, I misunderstand > this case? How old is the kernel? At some point (4.10, I think?) we added a patch to reserve metadata blocks for future free inode btree expansion. That required us to count the blocks in the finobt, at which point xfs/097's behavior changed such that the fs doesn't mount after the test corrupts the finobt. --D > > > > --D > > > > > ----------------------------------- > > > + check fs > > > + corrupt image > > > + mount image && modify files > > > -broken: 1 > > > +broken: 0 > > > + repair fs > > > + mount image (2) > > > ------------------------------------ > > > > > > It fails because the broken is always equal to 0 when _try_scratch_mount > > > succeed. So remove this wrong assignment operation. > > > > > > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > > > --- > > > tests/xfs/097 | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/tests/xfs/097 b/tests/xfs/097 > > > index 1cb7d69c..20791738 100755 > > > --- a/tests/xfs/097 > > > +++ b/tests/xfs/097 > > > @@ -81,8 +81,6 @@ done > > > echo "+ mount image && modify files" > > > broken=1 > > > if _try_scratch_mount >> $seqres.full 2>&1; then > > > - > > > - broken=0 > > > for x in `seq 65 70`; do > > > touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 > > > done > > > -- > > > 2.18.1 > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-14 16:39 ` Darrick J. Wong @ 2019-10-15 6:27 ` Yang Xu 2019-10-21 12:09 ` Yang Xu 0 siblings, 1 reply; 10+ messages in thread From: Yang Xu @ 2019-10-15 6:27 UTC (permalink / raw) To: Darrick J. Wong; +Cc: fstests on 2019/10/15 0:39, Darrick J. Wong wrote: > On Tue, Oct 08, 2019 at 10:39:59AM +0800, Yang Xu wrote: >> >> >> on 2019/10/07 23:12, Darrick J. Wong wrote: >>> On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: >>>> On old kernel, since commit ded188b8609 ("xfs: Fix the situation that mount >>>> operation rejects corrupted XFS") running this case got the mismatched output, >>>> as below: >>> >>> But why did the output mismatch? Did the fs heal itself? Did >>> allocating 5 more files somehow avoid touching the finobt? Is the >>> assignment logic in the loop broken? >> >> The output mismatch because on old kernel, we can mount the corrupted xfs >> and touch action will be refused. so broken is equal to 0. >> The fs doesn't heal ifself. >> allocating 5 more file will touch the finobt. >> >> You can see this url >> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b >> >> eg xfs/087, they all delete "broken=0" before allocationg 5 more file. >> commit ded188b86 compatibled old kernel(permit mount and refuse touch) and >> new kernel(refuse mount) behavior on corrupted xfs. Or, I misunderstand >> this case? > > How old is the kernel? At some point (4.10, I think?) we added a patch > to reserve metadata blocks for future free inode btree expansion. That > required us to count the blocks in the finobt, at which point xfs/097's > behavior changed such that the fs doesn't mount after the test corrupts > the finobt. I test this case on kernel-3.10.0-1062.el7.x86_64. I find the patch you said to reserve metadata blocks for future free inode btree expansion. This kernel doesn't backport this commit 76d771b4 ("xfs: use per-AG reservations for the finobt"), so it permmits to mount. I can understand your meaning. But from xfstests commit ded188b86, it looks like refuse touch or refuse mount is acceptable for xfstests. Also, xfs/087 is a similar case but it sets broken=1 instead of broken =0. Before this kernel commit 76d771b4, xfs/087(xfs/097) permits mount and refuse touch, after this commit, xfs/087(xfs/097) refuses mount. I think we should keep xfs/097 consistent with xfs/087. What do you think about it? ps:my patch is intend to fix the inconsistent of broken assignment operation that xfstests commit ded188b86 introduced. Thanks Yang Xu > > --D > >>> >>> --D >>> >>>> ----------------------------------- >>>> + check fs >>>> + corrupt image >>>> + mount image && modify files >>>> -broken: 1 >>>> +broken: 0 >>>> + repair fs >>>> + mount image (2) >>>> ------------------------------------ >>>> >>>> It fails because the broken is always equal to 0 when _try_scratch_mount >>>> succeed. So remove this wrong assignment operation. >>>> >>>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >>>> --- >>>> tests/xfs/097 | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/tests/xfs/097 b/tests/xfs/097 >>>> index 1cb7d69c..20791738 100755 >>>> --- a/tests/xfs/097 >>>> +++ b/tests/xfs/097 >>>> @@ -81,8 +81,6 @@ done >>>> echo "+ mount image && modify files" >>>> broken=1 >>>> if _try_scratch_mount >> $seqres.full 2>&1; then >>>> - >>>> - broken=0 >>>> for x in `seq 65 70`; do >>>> touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 >>>> done >>>> -- >>>> 2.18.1 >>>> >>>> >>>> >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-15 6:27 ` Yang Xu @ 2019-10-21 12:09 ` Yang Xu 2019-10-21 15:50 ` Darrick J. Wong 0 siblings, 1 reply; 10+ messages in thread From: Yang Xu @ 2019-10-21 12:09 UTC (permalink / raw) To: Darrick J. Wong, guaneryu; +Cc: fstests on 2019/10/15 14:27, Yang Xu wrote: > > > on 2019/10/15 0:39, Darrick J. Wong wrote: >> On Tue, Oct 08, 2019 at 10:39:59AM +0800, Yang Xu wrote: >>> >>> >>> on 2019/10/07 23:12, Darrick J. Wong wrote: >>>> On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: >>>>> On old kernel, since commit ded188b8609 ("xfs: Fix the situation >>>>> that mount >>>>> operation rejects corrupted XFS") running this case got the >>>>> mismatched output, >>>>> as below: >>>> >>>> But why did the output mismatch? Did the fs heal itself? Did >>>> allocating 5 more files somehow avoid touching the finobt? Is the >>>> assignment logic in the loop broken? >>> >>> The output mismatch because on old kernel, we can mount the corrupted >>> xfs >>> and touch action will be refused. so broken is equal to 0. >>> The fs doesn't heal ifself. >>> allocating 5 more file will touch the finobt. >>> >>> You can see this url >>> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b >>> >>> >>> eg xfs/087, they all delete "broken=0" before allocationg 5 more file. >>> commit ded188b86 compatibled old kernel(permit mount and refuse >>> touch) and >>> new kernel(refuse mount) behavior on corrupted xfs. Or, I misunderstand >>> this case? >> >> How old is the kernel? At some point (4.10, I think?) we added a patch >> to reserve metadata blocks for future free inode btree expansion. That >> required us to count the blocks in the finobt, at which point xfs/097's >> behavior changed such that the fs doesn't mount after the test corrupts >> the finobt. > I test this case on kernel-3.10.0-1062.el7.x86_64. > I find the patch you said to reserve metadata blocks for future free > inode btree expansion. This kernel doesn't backport this commit 76d771b4 > ("xfs: use per-AG reservations for the finobt"), so it permmits to mount. > > I can understand your meaning. But from xfstests commit ded188b86, it > looks like refuse touch or refuse mount is acceptable for xfstests. > > Also, xfs/087 is a similar case but it sets broken=1 instead of broken > =0. Before this kernel commit 76d771b4, xfs/087(xfs/097) permits mount > and refuse touch, after this commit, xfs/087(xfs/097) refuses mount. > I think we should keep xfs/097 consistent with xfs/087. What do you > think about it? > > ps:my patch is intend to fix the inconsistent of broken assignment > operation that xfstests commit ded188b86 introduced. Hi Darrick Do you have some questions on this patch? Hi Eryu What do you think about this patch(I only want to keep xfs/097 consistent with xfs/087). > > Thanks > Yang Xu > >> >> --D >> >>>> >>>> --D >>>> >>>>> ----------------------------------- >>>>> + check fs >>>>> + corrupt image >>>>> + mount image && modify files >>>>> -broken: 1 >>>>> +broken: 0 >>>>> + repair fs >>>>> + mount image (2) >>>>> ------------------------------------ >>>>> >>>>> It fails because the broken is always equal to 0 when >>>>> _try_scratch_mount >>>>> succeed. So remove this wrong assignment operation. >>>>> >>>>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >>>>> --- >>>>> tests/xfs/097 | 2 -- >>>>> 1 file changed, 2 deletions(-) >>>>> >>>>> diff --git a/tests/xfs/097 b/tests/xfs/097 >>>>> index 1cb7d69c..20791738 100755 >>>>> --- a/tests/xfs/097 >>>>> +++ b/tests/xfs/097 >>>>> @@ -81,8 +81,6 @@ done >>>>> echo "+ mount image && modify files" >>>>> broken=1 >>>>> if _try_scratch_mount >> $seqres.full 2>&1; then >>>>> - >>>>> - broken=0 >>>>> for x in `seq 65 70`; do >>>>> touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 >>>>> done >>>>> -- >>>>> 2.18.1 >>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-21 12:09 ` Yang Xu @ 2019-10-21 15:50 ` Darrick J. Wong 2019-10-22 1:49 ` Yang Xu 0 siblings, 1 reply; 10+ messages in thread From: Darrick J. Wong @ 2019-10-21 15:50 UTC (permalink / raw) To: Yang Xu; +Cc: guaneryu, fstests On Mon, Oct 21, 2019 at 08:09:39PM +0800, Yang Xu wrote: > > > on 2019/10/15 14:27, Yang Xu wrote: > > > > > > on 2019/10/15 0:39, Darrick J. Wong wrote: > > > On Tue, Oct 08, 2019 at 10:39:59AM +0800, Yang Xu wrote: > > > > > > > > > > > > on 2019/10/07 23:12, Darrick J. Wong wrote: > > > > > On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: > > > > > > On old kernel, since commit ded188b8609 ("xfs: Fix the > > > > > > situation that mount > > > > > > operation rejects corrupted XFS") running this case got > > > > > > the mismatched output, > > > > > > as below: > > > > > > > > > > But why did the output mismatch? Did the fs heal itself? Did > > > > > allocating 5 more files somehow avoid touching the finobt? Is the > > > > > assignment logic in the loop broken? > > > > > > > > The output mismatch because on old kernel, we can mount the > > > > corrupted xfs > > > > and touch action will be refused. so broken is equal to 0. > > > > The fs doesn't heal ifself. > > > > allocating 5 more file will touch the finobt. > > > > > > > > You can see this url > > > > https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b > > > > > > > > > > > > eg xfs/087, they all delete "broken=0" before allocationg 5 more file. > > > > commit ded188b86 compatibled old kernel(permit mount and refuse > > > > touch) and > > > > new kernel(refuse mount) behavior on corrupted xfs. Or, I misunderstand > > > > this case? > > > > > > How old is the kernel? At some point (4.10, I think?) we added a patch > > > to reserve metadata blocks for future free inode btree expansion. That > > > required us to count the blocks in the finobt, at which point xfs/097's > > > behavior changed such that the fs doesn't mount after the test corrupts > > > the finobt. > > I test this case on kernel-3.10.0-1062.el7.x86_64. > > I find the patch you said to reserve metadata blocks for future free > > inode btree expansion. This kernel doesn't backport this commit 76d771b4 > > ("xfs: use per-AG reservations for the finobt"), so it permmits to > > mount. > > > > I can understand your meaning. But from xfstests commit ded188b86, it > > looks like refuse touch or refuse mount is acceptable for xfstests. > > > > Also, xfs/087 is a similar case but it sets broken=1 instead of broken > > =0. Before this kernel commit 76d771b4, xfs/087(xfs/097) permits mount > > and refuse touch, after this commit, xfs/087(xfs/097) refuses mount. > > I think we should keep xfs/097 consistent with xfs/087. What do you > > think about it? > > > > ps:my patch is intend to fix the inconsistent of broken assignment > > operation that xfstests commit ded188b86 introduced. > Hi Darrick > Do you have some questions on this patch? Does it still pass on upstreeam 5.4? --D > Hi Eryu > What do you think about this patch(I only want to keep xfs/097 consistent > with xfs/087). > > > > Thanks > > Yang Xu > > > > > > > > --D > > > > > > > > > > > > > --D > > > > > > > > > > > ----------------------------------- > > > > > > + check fs > > > > > > + corrupt image > > > > > > + mount image && modify files > > > > > > -broken: 1 > > > > > > +broken: 0 > > > > > > + repair fs > > > > > > + mount image (2) > > > > > > ------------------------------------ > > > > > > > > > > > > It fails because the broken is always equal to 0 when > > > > > > _try_scratch_mount > > > > > > succeed. So remove this wrong assignment operation. > > > > > > > > > > > > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > > > > > > --- > > > > > > tests/xfs/097 | 2 -- > > > > > > 1 file changed, 2 deletions(-) > > > > > > > > > > > > diff --git a/tests/xfs/097 b/tests/xfs/097 > > > > > > index 1cb7d69c..20791738 100755 > > > > > > --- a/tests/xfs/097 > > > > > > +++ b/tests/xfs/097 > > > > > > @@ -81,8 +81,6 @@ done > > > > > > echo "+ mount image && modify files" > > > > > > broken=1 > > > > > > if _try_scratch_mount >> $seqres.full 2>&1; then > > > > > > - > > > > > > - broken=0 > > > > > > for x in `seq 65 70`; do > > > > > > touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 > > > > > > done > > > > > > -- > > > > > > 2.18.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-21 15:50 ` Darrick J. Wong @ 2019-10-22 1:49 ` Yang Xu 2019-10-22 1:55 ` Darrick J. Wong 0 siblings, 1 reply; 10+ messages in thread From: Yang Xu @ 2019-10-22 1:49 UTC (permalink / raw) To: Darrick J. Wong; +Cc: guaneryu, fstests on 2019/10/21 23:50, Darrick J. Wong wrote: > On Mon, Oct 21, 2019 at 08:09:39PM +0800, Yang Xu wrote: >> >> >> on 2019/10/15 14:27, Yang Xu wrote: >>> >>> >>> on 2019/10/15 0:39, Darrick J. Wong wrote: >>>> On Tue, Oct 08, 2019 at 10:39:59AM +0800, Yang Xu wrote: >>>>> >>>>> >>>>> on 2019/10/07 23:12, Darrick J. Wong wrote: >>>>>> On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: >>>>>>> On old kernel, since commit ded188b8609 ("xfs: Fix the >>>>>>> situation that mount >>>>>>> operation rejects corrupted XFS") running this case got >>>>>>> the mismatched output, >>>>>>> as below: >>>>>> >>>>>> But why did the output mismatch? Did the fs heal itself? Did >>>>>> allocating 5 more files somehow avoid touching the finobt? Is the >>>>>> assignment logic in the loop broken? >>>>> >>>>> The output mismatch because on old kernel, we can mount the >>>>> corrupted xfs >>>>> and touch action will be refused. so broken is equal to 0. >>>>> The fs doesn't heal ifself. >>>>> allocating 5 more file will touch the finobt. >>>>> >>>>> You can see this url >>>>> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b >>>>> >>>>> >>>>> eg xfs/087, they all delete "broken=0" before allocationg 5 more file. >>>>> commit ded188b86 compatibled old kernel(permit mount and refuse >>>>> touch) and >>>>> new kernel(refuse mount) behavior on corrupted xfs. Or, I misunderstand >>>>> this case? >>>> >>>> How old is the kernel? At some point (4.10, I think?) we added a patch >>>> to reserve metadata blocks for future free inode btree expansion. That >>>> required us to count the blocks in the finobt, at which point xfs/097's >>>> behavior changed such that the fs doesn't mount after the test corrupts >>>> the finobt. >>> I test this case on kernel-3.10.0-1062.el7.x86_64. >>> I find the patch you said to reserve metadata blocks for future free >>> inode btree expansion. This kernel doesn't backport this commit 76d771b4 >>> ("xfs: use per-AG reservations for the finobt"), so it permmits to >>> mount. >>> >>> I can understand your meaning. But from xfstests commit ded188b86, it >>> looks like refuse touch or refuse mount is acceptable for xfstests. >>> >>> Also, xfs/087 is a similar case but it sets broken=1 instead of broken >>> =0. Before this kernel commit 76d771b4, xfs/087(xfs/097) permits mount >>> and refuse touch, after this commit, xfs/087(xfs/097) refuses mount. >>> I think we should keep xfs/097 consistent with xfs/087. What do you >>> think about it? >>> >>> ps:my patch is intend to fix the inconsistent of broken assignment >>> operation that xfstests commit ded188b86 introduced. >> Hi Darrick >> Do you have some questions on this patch? > > Does it still pass on upstreeam 5.4? Of course. It still can pass on upstream 5.4. -------------------------------------- echo "+ mount image && modify files" broken=1 //on kernel with commit d771b4 ("xfs: use per-AG reservations for the //finobt", it will not run into this if judgement, so broken=1. //on kernel without this kernel commit, it will run into this if //judgement and touch will be refused, so broken is still equal to 1. if _try_scratch_mount >> $seqres.full 2>&1; then for x in `seq 65 70`; do touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 done umount "${SCRATCH_MNT}" fi -------------------------------------- > > --D > >> Hi Eryu >> What do you think about this patch(I only want to keep xfs/097 consistent >> with xfs/087). >>> >>> Thanks >>> Yang Xu >>> >>>> >>>> --D >>>> >>>>>> >>>>>> --D >>>>>> >>>>>>> ----------------------------------- >>>>>>> + check fs >>>>>>> + corrupt image >>>>>>> + mount image && modify files >>>>>>> -broken: 1 >>>>>>> +broken: 0 >>>>>>> + repair fs >>>>>>> + mount image (2) >>>>>>> ------------------------------------ >>>>>>> >>>>>>> It fails because the broken is always equal to 0 when >>>>>>> _try_scratch_mount >>>>>>> succeed. So remove this wrong assignment operation. >>>>>>> >>>>>>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >>>>>>> --- >>>>>>> tests/xfs/097 | 2 -- >>>>>>> 1 file changed, 2 deletions(-) >>>>>>> >>>>>>> diff --git a/tests/xfs/097 b/tests/xfs/097 >>>>>>> index 1cb7d69c..20791738 100755 >>>>>>> --- a/tests/xfs/097 >>>>>>> +++ b/tests/xfs/097 >>>>>>> @@ -81,8 +81,6 @@ done >>>>>>> echo "+ mount image && modify files" >>>>>>> broken=1 >>>>>>> if _try_scratch_mount >> $seqres.full 2>&1; then >>>>>>> - >>>>>>> - broken=0 >>>>>>> for x in `seq 65 70`; do >>>>>>> touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 >>>>>>> done >>>>>>> -- >>>>>>> 2.18.1 >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-22 1:49 ` Yang Xu @ 2019-10-22 1:55 ` Darrick J. Wong 2019-10-22 2:06 ` Yang Xu 0 siblings, 1 reply; 10+ messages in thread From: Darrick J. Wong @ 2019-10-22 1:55 UTC (permalink / raw) To: Yang Xu; +Cc: guaneryu, fstests On Tue, Oct 22, 2019 at 09:49:48AM +0800, Yang Xu wrote: > > > on 2019/10/21 23:50, Darrick J. Wong wrote: > > On Mon, Oct 21, 2019 at 08:09:39PM +0800, Yang Xu wrote: > > > > > > > > > on 2019/10/15 14:27, Yang Xu wrote: > > > > > > > > > > > > on 2019/10/15 0:39, Darrick J. Wong wrote: > > > > > On Tue, Oct 08, 2019 at 10:39:59AM +0800, Yang Xu wrote: > > > > > > > > > > > > > > > > > > on 2019/10/07 23:12, Darrick J. Wong wrote: > > > > > > > On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: > > > > > > > > On old kernel, since commit ded188b8609 ("xfs: Fix the > > > > > > > > situation that mount > > > > > > > > operation rejects corrupted XFS") running this case got > > > > > > > > the mismatched output, > > > > > > > > as below: > > > > > > > > > > > > > > But why did the output mismatch? Did the fs heal itself? Did > > > > > > > allocating 5 more files somehow avoid touching the finobt? Is the > > > > > > > assignment logic in the loop broken? > > > > > > > > > > > > The output mismatch because on old kernel, we can mount the > > > > > > corrupted xfs > > > > > > and touch action will be refused. so broken is equal to 0. > > > > > > The fs doesn't heal ifself. > > > > > > allocating 5 more file will touch the finobt. > > > > > > > > > > > > You can see this url > > > > > > https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b > > > > > > > > > > > > > > > > > > eg xfs/087, they all delete "broken=0" before allocationg 5 more file. > > > > > > commit ded188b86 compatibled old kernel(permit mount and refuse > > > > > > touch) and > > > > > > new kernel(refuse mount) behavior on corrupted xfs. Or, I misunderstand > > > > > > this case? > > > > > > > > > > How old is the kernel? At some point (4.10, I think?) we added a patch > > > > > to reserve metadata blocks for future free inode btree expansion. That > > > > > required us to count the blocks in the finobt, at which point xfs/097's > > > > > behavior changed such that the fs doesn't mount after the test corrupts > > > > > the finobt. > > > > I test this case on kernel-3.10.0-1062.el7.x86_64. > > > > I find the patch you said to reserve metadata blocks for future free > > > > inode btree expansion. This kernel doesn't backport this commit 76d771b4 > > > > ("xfs: use per-AG reservations for the finobt"), so it permmits to > > > > mount. > > > > > > > > I can understand your meaning. But from xfstests commit ded188b86, it > > > > looks like refuse touch or refuse mount is acceptable for xfstests. > > > > > > > > Also, xfs/087 is a similar case but it sets broken=1 instead of broken > > > > =0. Before this kernel commit 76d771b4, xfs/087(xfs/097) permits mount > > > > and refuse touch, after this commit, xfs/087(xfs/097) refuses mount. > > > > I think we should keep xfs/097 consistent with xfs/087. What do you > > > > think about it? > > > > > > > > ps:my patch is intend to fix the inconsistent of broken assignment > > > > operation that xfstests commit ded188b86 introduced. > > > Hi Darrick > > > Do you have some questions on this patch? > > > > Does it still pass on upstreeam 5.4? > > Of course. It still can pass on upstream 5.4. > > -------------------------------------- > echo "+ mount image && modify files" > broken=1 > //on kernel with commit d771b4 ("xfs: use per-AG reservations for the > //finobt", it will not run into this if judgement, so broken=1. > //on kernel without this kernel commit, it will run into this if //judgement > and touch will be refused, so broken is still equal to 1. > if _try_scratch_mount >> $seqres.full 2>&1; then > for x in `seq 65 70`; do > touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 > done > umount "${SCRATCH_MNT}" > fi > -------------------------------------- Ah, ok. Looks good to me then. Sorry I was a little slow on the uptake. :/ Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > > > > > --D > > > > > Hi Eryu > > > What do you think about this patch(I only want to keep xfs/097 consistent > > > with xfs/087). > > > > > > > > Thanks > > > > Yang Xu > > > > > > > > > > > > > > --D > > > > > > > > > > > > > > > > > > > --D > > > > > > > > > > > > > > > ----------------------------------- > > > > > > > > + check fs > > > > > > > > + corrupt image > > > > > > > > + mount image && modify files > > > > > > > > -broken: 1 > > > > > > > > +broken: 0 > > > > > > > > + repair fs > > > > > > > > + mount image (2) > > > > > > > > ------------------------------------ > > > > > > > > > > > > > > > > It fails because the broken is always equal to 0 when > > > > > > > > _try_scratch_mount > > > > > > > > succeed. So remove this wrong assignment operation. > > > > > > > > > > > > > > > > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > > > > > > > > --- > > > > > > > > tests/xfs/097 | 2 -- > > > > > > > > 1 file changed, 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/tests/xfs/097 b/tests/xfs/097 > > > > > > > > index 1cb7d69c..20791738 100755 > > > > > > > > --- a/tests/xfs/097 > > > > > > > > +++ b/tests/xfs/097 > > > > > > > > @@ -81,8 +81,6 @@ done > > > > > > > > echo "+ mount image && modify files" > > > > > > > > broken=1 > > > > > > > > if _try_scratch_mount >> $seqres.full 2>&1; then > > > > > > > > - > > > > > > > > - broken=0 > > > > > > > > for x in `seq 65 70`; do > > > > > > > > touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 > > > > > > > > done > > > > > > > > -- > > > > > > > > 2.18.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs/097: Remove wrong broken assignment operation 2019-10-22 1:55 ` Darrick J. Wong @ 2019-10-22 2:06 ` Yang Xu 0 siblings, 0 replies; 10+ messages in thread From: Yang Xu @ 2019-10-22 2:06 UTC (permalink / raw) To: Darrick J. Wong; +Cc: guaneryu, fstests on 2019/10/22 9:55, Darrick J. Wong wrote: > On Tue, Oct 22, 2019 at 09:49:48AM +0800, Yang Xu wrote: >> >> >> on 2019/10/21 23:50, Darrick J. Wong wrote: >>> On Mon, Oct 21, 2019 at 08:09:39PM +0800, Yang Xu wrote: >>>> >>>> >>>> on 2019/10/15 14:27, Yang Xu wrote: >>>>> >>>>> >>>>> on 2019/10/15 0:39, Darrick J. Wong wrote: >>>>>> On Tue, Oct 08, 2019 at 10:39:59AM +0800, Yang Xu wrote: >>>>>>> >>>>>>> >>>>>>> on 2019/10/07 23:12, Darrick J. Wong wrote: >>>>>>>> On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote: >>>>>>>>> On old kernel, since commit ded188b8609 ("xfs: Fix the >>>>>>>>> situation that mount >>>>>>>>> operation rejects corrupted XFS") running this case got >>>>>>>>> the mismatched output, >>>>>>>>> as below: >>>>>>>> >>>>>>>> But why did the output mismatch? Did the fs heal itself? Did >>>>>>>> allocating 5 more files somehow avoid touching the finobt? Is the >>>>>>>> assignment logic in the loop broken? >>>>>>> >>>>>>> The output mismatch because on old kernel, we can mount the >>>>>>> corrupted xfs >>>>>>> and touch action will be refused. so broken is equal to 0. >>>>>>> The fs doesn't heal ifself. >>>>>>> allocating 5 more file will touch the finobt. >>>>>>> >>>>>>> You can see this url >>>>>>> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b >>>>>>> >>>>>>> >>>>>>> eg xfs/087, they all delete "broken=0" before allocationg 5 more file. >>>>>>> commit ded188b86 compatibled old kernel(permit mount and refuse >>>>>>> touch) and >>>>>>> new kernel(refuse mount) behavior on corrupted xfs. Or, I misunderstand >>>>>>> this case? >>>>>> >>>>>> How old is the kernel? At some point (4.10, I think?) we added a patch >>>>>> to reserve metadata blocks for future free inode btree expansion. That >>>>>> required us to count the blocks in the finobt, at which point xfs/097's >>>>>> behavior changed such that the fs doesn't mount after the test corrupts >>>>>> the finobt. >>>>> I test this case on kernel-3.10.0-1062.el7.x86_64. >>>>> I find the patch you said to reserve metadata blocks for future free >>>>> inode btree expansion. This kernel doesn't backport this commit 76d771b4 >>>>> ("xfs: use per-AG reservations for the finobt"), so it permmits to >>>>> mount. >>>>> >>>>> I can understand your meaning. But from xfstests commit ded188b86, it >>>>> looks like refuse touch or refuse mount is acceptable for xfstests. >>>>> >>>>> Also, xfs/087 is a similar case but it sets broken=1 instead of broken >>>>> =0. Before this kernel commit 76d771b4, xfs/087(xfs/097) permits mount >>>>> and refuse touch, after this commit, xfs/087(xfs/097) refuses mount. >>>>> I think we should keep xfs/097 consistent with xfs/087. What do you >>>>> think about it? >>>>> >>>>> ps:my patch is intend to fix the inconsistent of broken assignment >>>>> operation that xfstests commit ded188b86 introduced. >>>> Hi Darrick >>>> Do you have some questions on this patch? >>> >>> Does it still pass on upstreeam 5.4? >> >> Of course. It still can pass on upstream 5.4. >> >> -------------------------------------- >> echo "+ mount image && modify files" >> broken=1 >> //on kernel with commit d771b4 ("xfs: use per-AG reservations for the >> //finobt", it will not run into this if judgement, so broken=1. >> //on kernel without this kernel commit, it will run into this if //judgement >> and touch will be refused, so broken is still equal to 1. >> if _try_scratch_mount >> $seqres.full 2>&1; then >> for x in `seq 65 70`; do >> touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 >> done >> umount "${SCRATCH_MNT}" >> fi >> -------------------------------------- > > Ah, ok. Looks good to me then. Sorry I was a little slow on the > uptake. :/ It doesn't matter. Thanks for your review. > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > --D > >> >>> >>> --D >>> >>>> Hi Eryu >>>> What do you think about this patch(I only want to keep xfs/097 consistent >>>> with xfs/087). >>>>> >>>>> Thanks >>>>> Yang Xu >>>>> >>>>>> >>>>>> --D >>>>>> >>>>>>>> >>>>>>>> --D >>>>>>>> >>>>>>>>> ----------------------------------- >>>>>>>>> + check fs >>>>>>>>> + corrupt image >>>>>>>>> + mount image && modify files >>>>>>>>> -broken: 1 >>>>>>>>> +broken: 0 >>>>>>>>> + repair fs >>>>>>>>> + mount image (2) >>>>>>>>> ------------------------------------ >>>>>>>>> >>>>>>>>> It fails because the broken is always equal to 0 when >>>>>>>>> _try_scratch_mount >>>>>>>>> succeed. So remove this wrong assignment operation. >>>>>>>>> >>>>>>>>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> >>>>>>>>> --- >>>>>>>>> tests/xfs/097 | 2 -- >>>>>>>>> 1 file changed, 2 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/tests/xfs/097 b/tests/xfs/097 >>>>>>>>> index 1cb7d69c..20791738 100755 >>>>>>>>> --- a/tests/xfs/097 >>>>>>>>> +++ b/tests/xfs/097 >>>>>>>>> @@ -81,8 +81,6 @@ done >>>>>>>>> echo "+ mount image && modify files" >>>>>>>>> broken=1 >>>>>>>>> if _try_scratch_mount >> $seqres.full 2>&1; then >>>>>>>>> - >>>>>>>>> - broken=0 >>>>>>>>> for x in `seq 65 70`; do >>>>>>>>> touch "${TESTFILE}.${x}" 2> /dev/null && broken=0 >>>>>>>>> done >>>>>>>>> -- >>>>>>>>> 2.18.1 >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-22 2:06 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-07 7:15 [PATCH] xfs/097: Remove wrong broken assignment operation Yang Xu 2019-10-07 15:12 ` Darrick J. Wong 2019-10-08 2:39 ` Yang Xu 2019-10-14 16:39 ` Darrick J. Wong 2019-10-15 6:27 ` Yang Xu 2019-10-21 12:09 ` Yang Xu 2019-10-21 15:50 ` Darrick J. Wong 2019-10-22 1:49 ` Yang Xu 2019-10-22 1:55 ` Darrick J. Wong 2019-10-22 2:06 ` Yang Xu
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).