[PATCH] xfs/098: fix xfs_repair on newer xfsprogs

[PATCH] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

Make sure xfs_repair can't clear the log by default when it is corrupted.
xfs_repair always and only clear the log when the -L parameter is specified.
This has updated by:
Commit f2053bc ("xfs_repair: don't clear the log by default")

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 tests/xfs/098 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/xfs/098 b/tests/xfs/098
index d91d617..0fe8d93 100755
--- a/tests/xfs/098
+++ b/tests/xfs/098
@@ -93,7 +93,9 @@ echo "+ mount image"
 _scratch_mount 2>/dev/null && _fail "mount should not succeed"
 
 echo "+ repair fs"
-_scratch_xfs_repair >> $seqres.full 2>&1
+_scratch_xfs_repair >> $seqres.full 2>&1 && _fail "xfs_repair should not succeed without -L option"
+
+_scratch_xfs_repair -L >> $seqres.full 2>&1
 
 echo "+ mount image (2)"
 _scratch_mount
-- 
1.8.3.1

Re: [PATCH] xfs/098: fix xfs_repair on newer xfsprogs

[Zorro Lang]

On Thu, Aug 25, 2016 at 05:22:31PM +0800, Xiao Yang wrote:
> Make sure xfs_repair can't clear the log by default when it is corrupted.
> xfs_repair always and only clear the log when the -L parameter is specified.
> This has updated by:
> Commit f2053bc ("xfs_repair: don't clear the log by default")
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  tests/xfs/098 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/xfs/098 b/tests/xfs/098
> index d91d617..0fe8d93 100755
> --- a/tests/xfs/098
> +++ b/tests/xfs/098
> @@ -93,7 +93,9 @@ echo "+ mount image"
>  _scratch_mount 2>/dev/null && _fail "mount should not succeed"
>  
>  echo "+ repair fs"
> -_scratch_xfs_repair >> $seqres.full 2>&1
> +_scratch_xfs_repair >> $seqres.full 2>&1 && _fail "xfs_repair should not succeed without -L option"

Hi,

If you make it fail at here, this case can't run for old xfsprogs(without
commit f2053bc).

> +
> +_scratch_xfs_repair -L >> $seqres.full 2>&1

For compatibility, maybe you can use "-L" directly. Or use -L
after _scratch_xfs_repair return error.

Thanks,
Zorro

>  
>  echo "+ mount image (2)"
>  _scratch_mount
> -- 
> 1.8.3.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs/098: fix xfs_repair on newer xfsprogs

[Darrick J. Wong]

On Thu, Aug 25, 2016 at 08:09:03PM +0800, Zorro Lang wrote:
> On Thu, Aug 25, 2016 at 05:22:31PM +0800, Xiao Yang wrote:
> > Make sure xfs_repair can't clear the log by default when it is corrupted.
> > xfs_repair always and only clear the log when the -L parameter is specified.
> > This has updated by:
> > Commit f2053bc ("xfs_repair: don't clear the log by default")
> > 
> > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> > ---
> >  tests/xfs/098 | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/xfs/098 b/tests/xfs/098
> > index d91d617..0fe8d93 100755
> > --- a/tests/xfs/098
> > +++ b/tests/xfs/098
> > @@ -93,7 +93,9 @@ echo "+ mount image"
> >  _scratch_mount 2>/dev/null && _fail "mount should not succeed"
> >  
> >  echo "+ repair fs"
> > -_scratch_xfs_repair >> $seqres.full 2>&1
> > +_scratch_xfs_repair >> $seqres.full 2>&1 && _fail "xfs_repair should not succeed without -L option"
> 
> Hi,
> 
> If you make it fail at here, this case can't run for old xfsprogs(without
> commit f2053bc).
> 
> > +
> > +_scratch_xfs_repair -L >> $seqres.full 2>&1
> 
> For compatibility, maybe you can use "-L" directly. Or use -L
> after _scratch_xfs_repair return error.

I suggest _repair_scratch_fs.

--D

> 
> Thanks,
> Zorro
> 
> >  
> >  echo "+ mount image (2)"
> >  _scratch_mount
> > -- 
> > 1.8.3.1
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs/098: fix xfs_repair on newer xfsprogs

[Zorro Lang]

On Thu, Aug 25, 2016 at 08:40:52AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 25, 2016 at 08:09:03PM +0800, Zorro Lang wrote:
> > On Thu, Aug 25, 2016 at 05:22:31PM +0800, Xiao Yang wrote:
> > > Make sure xfs_repair can't clear the log by default when it is corrupted.
> > > xfs_repair always and only clear the log when the -L parameter is specified.
> > > This has updated by:
> > > Commit f2053bc ("xfs_repair: don't clear the log by default")
> > > 
> > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> > > ---
> > >  tests/xfs/098 | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/xfs/098 b/tests/xfs/098
> > > index d91d617..0fe8d93 100755
> > > --- a/tests/xfs/098
> > > +++ b/tests/xfs/098
> > > @@ -93,7 +93,9 @@ echo "+ mount image"
> > >  _scratch_mount 2>/dev/null && _fail "mount should not succeed"
> > >  
> > >  echo "+ repair fs"
> > > -_scratch_xfs_repair >> $seqres.full 2>&1
> > > +_scratch_xfs_repair >> $seqres.full 2>&1 && _fail "xfs_repair should not succeed without -L option"
> > 
> > Hi,
> > 
> > If you make it fail at here, this case can't run for old xfsprogs(without
> > commit f2053bc).
> > 
> > > +
> > > +_scratch_xfs_repair -L >> $seqres.full 2>&1
> > 
> > For compatibility, maybe you can use "-L" directly. Or use -L
> > after _scratch_xfs_repair return error.
> 
> I suggest _repair_scratch_fs.

Oh, you're right. You already wrote a common function to do that...
Thanks for remind me:)

Thanks,
Zorro

> 
> --D
> 
> > 
> > Thanks,
> > Zorro
> > 
> > >  
> > >  echo "+ mount image (2)"
> > >  _scratch_mount
> > > -- 
> > > 1.8.3.1
> > > 
> > > 
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

于 2016/08/25 23:40, Darrick J. Wong 写道:
> On Thu, Aug 25, 2016 at 08:09:03PM +0800, Zorro Lang wrote:
>> On Thu, Aug 25, 2016 at 05:22:31PM +0800, Xiao Yang wrote:
>>> Make sure xfs_repair can't clear the log by default when it is corrupted.
>>> xfs_repair always and only clear the log when the -L parameter is specified.
>>> This has updated by:
>>> Commit f2053bc ("xfs_repair: don't clear the log by default")
>>>
>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>>> ---
>>>   tests/xfs/098 | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/xfs/098 b/tests/xfs/098
>>> index d91d617..0fe8d93 100755
>>> --- a/tests/xfs/098
>>> +++ b/tests/xfs/098
>>> @@ -93,7 +93,9 @@ echo "+ mount image"
>>>   _scratch_mount 2>/dev/null&&  _fail "mount should not succeed"
>>>
>>>   echo "+ repair fs"
>>> -_scratch_xfs_repair>>  $seqres.full 2>&1
>>> +_scratch_xfs_repair>>  $seqres.full 2>&1&&  _fail "xfs_repair should not succeed without -L option"
>> Hi,
>>
>> If you make it fail at here, this case can't run for old xfsprogs(without
>> commit f2053bc).
>>
>>> +
>>> +_scratch_xfs_repair -L>>  $seqres.full 2>&1
>> For compatibility, maybe you can use "-L" directly. Or use -L
>> after _scratch_xfs_repair return error.
> I suggest _repair_scratch_fs.
>
> --D
>
Hi Darrick
I changed _repair_scratch_fs because xfs_repair return 1 when log is 
corrupted.

Thanks for your review.

Regards,
Xiao Yang

>> Thanks,
>> Zorro
>>
>>>
>>>   echo "+ mount image (2)"
>>>   _scratch_mount
>>> -- 
>>> 1.8.3.1
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH] xfs/098: fix xfs_repair on newer xfsprogs

[Darrick J. Wong]

On Fri, Aug 26, 2016 at 11:32:56AM +0800, Xiao Yang wrote:
> 于 2016/08/25 23:40, Darrick J. Wong 写道:
> >On Thu, Aug 25, 2016 at 08:09:03PM +0800, Zorro Lang wrote:
> >>On Thu, Aug 25, 2016 at 05:22:31PM +0800, Xiao Yang wrote:
> >>>Make sure xfs_repair can't clear the log by default when it is corrupted.
> >>>xfs_repair always and only clear the log when the -L parameter is specified.
> >>>This has updated by:
> >>>Commit f2053bc ("xfs_repair: don't clear the log by default")
> >>>
> >>>Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> >>>---
> >>>  tests/xfs/098 | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/tests/xfs/098 b/tests/xfs/098
> >>>index d91d617..0fe8d93 100755
> >>>--- a/tests/xfs/098
> >>>+++ b/tests/xfs/098
> >>>@@ -93,7 +93,9 @@ echo "+ mount image"
> >>>  _scratch_mount 2>/dev/null&&  _fail "mount should not succeed"
> >>>
> >>>  echo "+ repair fs"
> >>>-_scratch_xfs_repair>>  $seqres.full 2>&1
> >>>+_scratch_xfs_repair>>  $seqres.full 2>&1&&  _fail "xfs_repair should not succeed without -L option"
> >>Hi,
> >>
> >>If you make it fail at here, this case can't run for old xfsprogs(without
> >>commit f2053bc).
> >>
> >>>+
> >>>+_scratch_xfs_repair -L>>  $seqres.full 2>&1
> >>For compatibility, maybe you can use "-L" directly. Or use -L
> >>after _scratch_xfs_repair return error.
> >I suggest _repair_scratch_fs.
> >
> >--D
> >
> Hi Darrick
> I changed _repair_scratch_fs because xfs_repair return 1 when log is
> corrupted.

xfs_repair returns 2 when the log is corrupted, 1 when there's corruption left
to be fixed *or* some kind of operation error happened, and 0 if either it
found nothing wrong or all the corruptions were fixed.  The manpage lies.

(Proceeding on to the rest of the messages in this thread...)

--D

> Thanks for your review.
> 
> Regards,
> Xiao Yang
> 
> >>Thanks,
> >>Zorro
> >>
> >>>
> >>>  echo "+ mount image (2)"
> >>>  _scratch_mount
> >>>-- 
> >>>1.8.3.1
> >>>
> >>>
> >>>
> >>>--
> >>>To unsubscribe from this list: send the line "unsubscribe fstests" in
> >>>the body of a message to majordomo@vger.kernel.org
> >>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

Make sure xfs_repair can't clear the log by default when it is corrupted.
xfs_repair always and only clear the log when the -L parameter is specified.
This has updated by:
Commit f2053bc ("xfs_repair: don't clear the log by default")

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc     | 4 ++--
 tests/xfs/098 | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/rc b/common/rc
index 3fb0600..c693a31 100644
--- a/common/rc
+++ b/common/rc
@@ -1143,9 +1143,9 @@ _repair_scratch_fs()
     xfs)
         _scratch_xfs_repair "$@" 2>&1
 	res=$?
-	if [ "$res" -eq 2 ]; then
+	if [ "$res" -ne 0 ]; then
 		echo "xfs_repair returns $res; replay log?"
-		_scratch_mount
+		_scratch_mount 2>&1
 		res=$?
 		if [ "$res" -gt 0 ]; then
 			echo "mount returns $res; zap log?"
diff --git a/tests/xfs/098 b/tests/xfs/098
index d91d617..eb33bb1 100755
--- a/tests/xfs/098
+++ b/tests/xfs/098
@@ -93,7 +93,7 @@ echo "+ mount image"
 _scratch_mount 2>/dev/null && _fail "mount should not succeed"
 
 echo "+ repair fs"
-_scratch_xfs_repair >> $seqres.full 2>&1
+_repair_scratch_fs >> $seqres.full
 
 echo "+ mount image (2)"
 _scratch_mount
-- 
1.8.3.1

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Eryu Guan]

On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
> Make sure xfs_repair can't clear the log by default when it is corrupted.
> xfs_repair always and only clear the log when the -L parameter is specified.
> This has updated by:
> Commit f2053bc ("xfs_repair: don't clear the log by default")

Can you please put more details in commit log? e.g. what's the problem
you want to fix, what's the symptom, etc. I had a hard time
understanding the problems without running the test.

> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  common/rc     | 4 ++--
>  tests/xfs/098 | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 3fb0600..c693a31 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1143,9 +1143,9 @@ _repair_scratch_fs()
>      xfs)
>          _scratch_xfs_repair "$@" 2>&1
>  	res=$?
> -	if [ "$res" -eq 2 ]; then
> +	if [ "$res" -ne 0 ]; then
>  		echo "xfs_repair returns $res; replay log?"
> -		_scratch_mount
> +		_scratch_mount 2>&1
>  		res=$?
>  		if [ "$res" -gt 0 ]; then
>  			echo "mount returns $res; zap log?"
> diff --git a/tests/xfs/098 b/tests/xfs/098
> index d91d617..eb33bb1 100755
> --- a/tests/xfs/098
> +++ b/tests/xfs/098
> @@ -93,7 +93,7 @@ echo "+ mount image"
>  _scratch_mount 2>/dev/null && _fail "mount should not succeed"
>  
>  echo "+ repair fs"
> -_scratch_xfs_repair >> $seqres.full 2>&1
> +_repair_scratch_fs >> $seqres.full

The above two redirection updates seem not necessary to me, mount
failure message got redirected to $seqres.full in both cases. Any reason
doing so?

Thanks,
Eryu

>  
>  echo "+ mount image (2)"
>  _scratch_mount
> -- 
> 1.8.3.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

On 2016/08/26 12:42, Eryu Guan wrote:
> On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
>> Make sure xfs_repair can't clear the log by default when it is corrupted.
>> xfs_repair always and only clear the log when the -L parameter is specified.
>> This has updated by:
>> Commit f2053bc ("xfs_repair: don't clear the log by default")
> Can you please put more details in commit log? e.g. what's the problem
> you want to fix, what's the symptom, etc. I had a hard time
> understanding the problems without running the test.
>
Hi Eryu
xfs_repair without -L option succeeded to repair filesystem at xfs/098 
if log is corrupted.  However,
this feature has been changed by following patch since 
xfsprogs-dev(4.3.0),  we have to use -L option to
repair filesystem if log is corrupted.
Commit f2053bc ("xfs_repair: don't clear the log by default")

xfs/098 will fail to repair filesystem if log is corrupted since 
xfsprogs-dev(4.3.0), so fix it.

Thanks
Xiao Yang.
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   common/rc     | 4 ++--
>>   tests/xfs/098 | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 3fb0600..c693a31 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -1143,9 +1143,9 @@ _repair_scratch_fs()
>>       xfs)
>>           _scratch_xfs_repair "$@" 2>&1
>>   	res=$?
>> -	if [ "$res" -eq 2 ]; then
>> +	if [ "$res" -ne 0 ]; then
>>   		echo "xfs_repair returns $res; replay log?"
>> -		_scratch_mount
>> +		_scratch_mount 2>&1
>>   		res=$?
>>   		if [ "$res" -gt 0 ]; then
>>   			echo "mount returns $res; zap log?"
>> diff --git a/tests/xfs/098 b/tests/xfs/098
>> index d91d617..eb33bb1 100755
>> --- a/tests/xfs/098
>> +++ b/tests/xfs/098
>> @@ -93,7 +93,7 @@ echo "+ mount image"
>>   _scratch_mount 2>/dev/null&&  _fail "mount should not succeed"
>>
>>   echo "+ repair fs"
>> -_scratch_xfs_repair>>  $seqres.full 2>&1
>> +_repair_scratch_fs>>  $seqres.full
> The above two redirection updates seem not necessary to me, mount
> failure message got redirected to $seqres.full in both cases. Any reason
> doing so?
>
> Thanks,
> Eryu
>
Hi Eryu
if xfs_repair without -L option can succeed to repair filesystem, the 
second mount will skip.

Thanks
Xiao Yang.
>>
>>   echo "+ mount image (2)"
>>   _scratch_mount
>> -- 
>> 1.8.3.1
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> .
>

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Zorro Lang]

On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
> Make sure xfs_repair can't clear the log by default when it is corrupted.
> xfs_repair always and only clear the log when the -L parameter is specified.
> This has updated by:
> Commit f2053bc ("xfs_repair: don't clear the log by default")
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  common/rc     | 4 ++--
>  tests/xfs/098 | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 3fb0600..c693a31 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1143,9 +1143,9 @@ _repair_scratch_fs()

Hi Xiao

You should explain why you changed this function in commit log. Or
the reviewer can't understand why you change it.

>      xfs)
>          _scratch_xfs_repair "$@" 2>&1
>  	res=$?
> -	if [ "$res" -eq 2 ]; then
> +	if [ "$res" -ne 0 ]; then

Hi Darrick,

The xfs_repair manpage said:
xfs_repair run without the -n option will always return a status code of 0.

I don't understand why you think it return 2 here? (Please check below)


>  		echo "xfs_repair returns $res; replay log?"
> -		_scratch_mount
> +		_scratch_mount 2>&1
>  		res=$?
>  		if [ "$res" -gt 0 ]; then
>  			echo "mount returns $res; zap log?"
> diff --git a/tests/xfs/098 b/tests/xfs/098
> index d91d617..eb33bb1 100755
> --- a/tests/xfs/098
> +++ b/tests/xfs/098
> @@ -93,7 +93,7 @@ echo "+ mount image"
>  _scratch_mount 2>/dev/null && _fail "mount should not succeed"
>  
>  echo "+ repair fs"
> -_scratch_xfs_repair >> $seqres.full 2>&1
> +_repair_scratch_fs >> $seqres.full

If just call xfs_repair without any options, the _repair_scratch_fs won't
help to call xfs_repair -L I think.

So I think this patch won't fix the problem.

Feel free to correct me, if I misunderstand something:)

Thanks,
Zorro

>  
>  echo "+ mount image (2)"
>  _scratch_mount
> -- 
> 1.8.3.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

On 2016/08/26 12:48, Zorro Lang wrote:
> On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
>> Make sure xfs_repair can't clear the log by default when it is corrupted.
>> xfs_repair always and only clear the log when the -L parameter is specified.
>> This has updated by:
>> Commit f2053bc ("xfs_repair: don't clear the log by default")
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   common/rc     | 4 ++--
>>   tests/xfs/098 | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 3fb0600..c693a31 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -1143,9 +1143,9 @@ _repair_scratch_fs()
> Hi Xiao
>
> You should explain why you changed this function in commit log. Or
> the reviewer can't understand why you change it.
>
>>       xfs)
>>           _scratch_xfs_repair "$@" 2>&1
>>   	res=$?
>> -	if [ "$res" -eq 2 ]; then
>> +	if [ "$res" -ne 0 ]; then
> Hi Darrick,
>
> The xfs_repair manpage said:
> xfs_repair run without the -n option will always return a status code of 0.
>
> I don't understand why you think it return 2 here? (Please check below)
>
Hi Zorro

I don't understand why it return 2 here too.  I want to change this 
function because xfs_repair
without -L option return 1 when log is corrupted on newer xfsprogs-dev.
>>   		echo "xfs_repair returns $res; replay log?"
>> -		_scratch_mount
>> +		_scratch_mount 2>&1
>>   		res=$?
>>   		if [ "$res" -gt 0 ]; then
>>   			echo "mount returns $res; zap log?"
>> diff --git a/tests/xfs/098 b/tests/xfs/098
>> index d91d617..eb33bb1 100755
>> --- a/tests/xfs/098
>> +++ b/tests/xfs/098
>> @@ -93,7 +93,7 @@ echo "+ mount image"
>>   _scratch_mount 2>/dev/null&&  _fail "mount should not succeed"
>>
>>   echo "+ repair fs"
>> -_scratch_xfs_repair>>  $seqres.full 2>&1
>> +_repair_scratch_fs>>  $seqres.full
> If just call xfs_repair without any options, the _repair_scratch_fs won't
> help to call xfs_repair -L I think.
>
> So I think this patch won't fix the problem.
>
> Feel free to correct me, if I misunderstand something:)
>
> Thanks,
> Zorro
>
If xfs_repair without any option succeed to repair filesystem when log 
is corrupted,
_repair_scratch_fs don't need  to call  xfs_repair -L.  If it failed to 
repair filesystem,
_repair_scratch_fs needs  to call  xfs_repair -L.

Thanks
Xiao Yang.
>>
>>   echo "+ mount image (2)"
>>   _scratch_mount
>> -- 
>> 1.8.3.1
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> .
>

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Zorro Lang]

On Fri, Aug 26, 2016 at 02:10:00PM +0800, Xiao Yang wrote:
> On 2016/08/26 12:48, Zorro Lang wrote:
> >On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
> >>Make sure xfs_repair can't clear the log by default when it is corrupted.
> >>xfs_repair always and only clear the log when the -L parameter is specified.
> >>This has updated by:
> >>Commit f2053bc ("xfs_repair: don't clear the log by default")
> >>
> >>Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> >>---
> >>  common/rc     | 4 ++--
> >>  tests/xfs/098 | 2 +-
> >>  2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/common/rc b/common/rc
> >>index 3fb0600..c693a31 100644
> >>--- a/common/rc
> >>+++ b/common/rc
> >>@@ -1143,9 +1143,9 @@ _repair_scratch_fs()
> >Hi Xiao
> >
> >You should explain why you changed this function in commit log. Or
> >the reviewer can't understand why you change it.
> >
> >>      xfs)
> >>          _scratch_xfs_repair "$@" 2>&1
> >>  	res=$?
> >>-	if [ "$res" -eq 2 ]; then
> >>+	if [ "$res" -ne 0 ]; then
> >Hi Darrick,
> >
> >The xfs_repair manpage said:
> >xfs_repair run without the -n option will always return a status code of 0.
> >
> >I don't understand why you think it return 2 here? (Please check below)
> >
> Hi Zorro
> 
> I don't understand why it return 2 here too.  I want to change this
> function because xfs_repair
> without -L option return 1 when log is corrupted on newer xfsprogs-dev.
> >>  		echo "xfs_repair returns $res; replay log?"
> >>-		_scratch_mount
> >>+		_scratch_mount 2>&1
> >>  		res=$?
> >>  		if [ "$res" -gt 0 ]; then
> >>  			echo "mount returns $res; zap log?"
> >>diff --git a/tests/xfs/098 b/tests/xfs/098
> >>index d91d617..eb33bb1 100755
> >>--- a/tests/xfs/098
> >>+++ b/tests/xfs/098
> >>@@ -93,7 +93,7 @@ echo "+ mount image"
> >>  _scratch_mount 2>/dev/null&&  _fail "mount should not succeed"
> >>
> >>  echo "+ repair fs"
> >>-_scratch_xfs_repair>>  $seqres.full 2>&1
> >>+_repair_scratch_fs>>  $seqres.full

You should print the stderr to $seqres.full too. Because in
"_repair_scratch_fs", its code likes below:

    xfs)
        _scratch_xfs_repair "$@" 2>&1
>>> This repair won't clear the corrupted log anymore.

        res=$?
        if [ "$res" -eq 2 ]; then
                echo "xfs_repair returns $res; replay log?"
                _scratch_mount
>>> So this mount maybe failed if it can't deal with the corrupted log.
>>> If it print some error messages, it'll break the golden image of xfs/098

                res=$?
                if [ "$res" -gt 0 ]; then
                        echo "mount returns $res; zap log?"
                        _scratch_xfs_repair -L 2>&1


> >If just call xfs_repair without any options, the _repair_scratch_fs won't
> >help to call xfs_repair -L I think.
> >
> >So I think this patch won't fix the problem.
> >
> >Feel free to correct me, if I misunderstand something:)
> >
> >Thanks,
> >Zorro
> >
> If xfs_repair without any option succeed to repair filesystem when
> log is corrupted,
> _repair_scratch_fs don't need  to call  xfs_repair -L.  If it failed
> to repair filesystem,
> _repair_scratch_fs needs  to call  xfs_repair -L.

Oh, sorry, I just tried to run ths case. The "_scratch_xfs_repair" really return
non-zero when it try to repair a corrupted xfs...

But the manpage(man xfs_repair) really said:
xfs_repair run without the -n option will always return a status code of 0.

Maybe we should update the manpage? I'll check it later.

Any way, there's still a problem in your patch, please see above:

Thanks,
Zorro

> 
> Thanks
> Xiao Yang.
> >>
> >>  echo "+ mount image (2)"
> >>  _scratch_mount
> >>-- 
> >>1.8.3.1
> >>
> >>
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe fstests" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >.
> >
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Zorro Lang]

On Fri, Sep 09, 2016 at 05:29:37PM +0800, Xiao Yang wrote:
> 于 2016/08/26 17:05, Zorro Lang 写道:
> >On Fri, Aug 26, 2016 at 02:10:00PM +0800, Xiao Yang wrote:
> >>On 2016/08/26 12:48, Zorro Lang wrote:
> >>>On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
> >>>>Make sure xfs_repair can't clear the log by default when it is corrupted.
> >>>>xfs_repair always and only clear the log when the -L parameter is specified.
> >>>>This has updated by:
> >>>>Commit f2053bc ("xfs_repair: don't clear the log by default")
> >>>>
> >>>>Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> >>>>---
> >>>>  common/rc     | 4 ++--
> >>>>  tests/xfs/098 | 2 +-
> >>>>  2 files changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>>diff --git a/common/rc b/common/rc
> >>>>index 3fb0600..c693a31 100644
> >>>>--- a/common/rc
> >>>>+++ b/common/rc
> >>>>@@ -1143,9 +1143,9 @@ _repair_scratch_fs()
> >>>Hi Xiao
> >>>
> >>>You should explain why you changed this function in commit log. Or
> >>>the reviewer can't understand why you change it.
> >>>
> >>>>      xfs)
> >>>>          _scratch_xfs_repair "$@" 2>&1
> >>>>  	res=$?
> >>>>-	if [ "$res" -eq 2 ]; then
> >>>>+	if [ "$res" -ne 0 ]; then
> >>>Hi Darrick,
> >>>
> >>>The xfs_repair manpage said:
> >>>xfs_repair run without the -n option will always return a status code of 0.
> >>>
> >>>I don't understand why you think it return 2 here? (Please check below)
> >>>
> >>Hi Zorro
> >>
> >>I don't understand why it return 2 here too.  I want to change this
> >>function because xfs_repair
> >>without -L option return 1 when log is corrupted on newer xfsprogs-dev.
> >>>>  		echo "xfs_repair returns $res; replay log?"
> >>>>-		_scratch_mount
> >>>>+		_scratch_mount 2>&1
> >>>>  		res=$?
> >>>>  		if [ "$res" -gt 0 ]; then
> >>>>  			echo "mount returns $res; zap log?"
> >>>>diff --git a/tests/xfs/098 b/tests/xfs/098
> >>>>index d91d617..eb33bb1 100755
> >>>>--- a/tests/xfs/098
> >>>>+++ b/tests/xfs/098
> >>>>@@ -93,7 +93,7 @@ echo "+ mount image"
> >>>>  _scratch_mount 2>/dev/null&&   _fail "mount should not succeed"
> >>>>
> >>>>  echo "+ repair fs"
> >>>>-_scratch_xfs_repair>>   $seqres.full 2>&1
> >>>>+_repair_scratch_fs>>   $seqres.full
> >You should print the stderr to $seqres.full too. Because in
> >"_repair_scratch_fs", its code likes below:
> >
> >     xfs)
> >         _scratch_xfs_repair "$@" 2>&1
> >>>>This repair won't clear the corrupted log anymore.
> >         res=$?
> >         if [ "$res" -eq 2 ]; then
> >                 echo "xfs_repair returns $res; replay log?"
> >                 _scratch_mount
> >>>>So this mount maybe failed if it can't deal with the corrupted log.
> >>>>If it print some error messages, it'll break the golden image of xfs/098
> >                 res=$?
> >                 if [ "$res" -gt 0 ]; then
> >                         echo "mount returns $res; zap log?"
> >                         _scratch_xfs_repair -L 2>&1
> >
> >
> >>>If just call xfs_repair without any options, the _repair_scratch_fs won't
> >>>help to call xfs_repair -L I think.
> >>>
> >>>So I think this patch won't fix the problem.
> >>>
> >>>Feel free to correct me, if I misunderstand something:)
> >>>
> >>>Thanks,
> >>>Zorro
> >>>
> >>If xfs_repair without any option succeed to repair filesystem when
> >>log is corrupted,
> >>_repair_scratch_fs don't need  to call  xfs_repair -L.  If it failed
> >>to repair filesystem,
> >>_repair_scratch_fs needs  to call  xfs_repair -L.
> >Oh, sorry, I just tried to run ths case. The "_scratch_xfs_repair" really return
> >non-zero when it try to repair a corrupted xfs...
> >
> >But the manpage(man xfs_repair) really said:
> >xfs_repair run without the -n option will always return a status code of 0.
> >
> >Maybe we should update the manpage? I'll check it later.
> >
> >Any way, there's still a problem in your patch, please see above:
> >
> >Thanks,
> >Zorro
> Hi Zorro
> Do you know why it returns 2 instead of 1 when we use xfs_repair
> without any options.
> I can't understand it, because it always return 1 on my machine.

Hi Xiao,

Please CC the mail list, there's no secret. And the most important
thing is if I said something wrong, others great developers maybe
glad to correct me:-P

I've asked DJ Wong about the return value of xfs_repair, and he
already replied:

"xfs_repair returns 2 when the log is corrupted, 1 when there's corruption left
to be fixed *or* some kind of operation error happened, and 0 if either it
found nothing wrong or all the corruptions were fixed."

I'm sure that email has been sent to you too.

If you can't understand why it return 1, you can check your xfs/098.full file,
you'll find:

"Phase 1 - find and verify superblock...
Phase 2 - using internal log
        - zero log...
Log inconsistent (didn't find previous header)
failed to find log head
zero_log: cannot find log head/tail (xlog_find_tail=5)

fatal error -- ERROR: The log head and/or tail cannot be discovered. Attempt to mount the
filesystem to replay the log or use the -L option to destroy the log and
attempt a repair.
xfs_repair failed, err=1"

This output from below xfsprogs code:

        error = xlog_find_tail(log, &head_blk, &tail_blk);
        if (error) {
                do_warn(
                _("zero_log: cannot find log head/tail (xlog_find_tail=%d)\n"),
                        error);
                if (!no_modify && !zap_log)
>>> [exit from here] >>>    do_error(_(
"ERROR: The log head and/or tail cannot be discovered. Attempt to mount the\n"
"filesystem to replay the log or use the -L option to destroy the log and\n"
"attempt a repair.\n"));
        } else {
                if (verbose) {
                        do_warn(
        _("zero_log: head block %" PRId64 " tail block %" PRId64 "\n"),
                                head_blk, tail_blk);
                }
                if (!no_modify && head_blk != tail_blk) {
                        if (zap_log) {
                                do_warn(_(
"ALERT: The filesystem has valuable metadata changes in a log which is being\n"
"destroyed because the -L option was used.\n"));
                        } else {
                                do_warn(_(
"ERROR: The filesystem has valuable metadata changes in a log which needs to\n"
"be replayed.  Mount the filesystem to replay the log, and unmount it before\n"
"re-running xfs_repair.  If you are unable to mount the filesystem, then use\n"
"the -L option to destroy the log and attempt a repair.\n"
"Note that destroying the log may cause corruption -- please attempt a mount\n"
"of the filesystem before doing this.\n"));
                                exit(2);
                        }
                }
        }

I've marked [exit from here] for you. do_error will call exit(1). And the output
message already tell you the reason about why it fail.

You can keep reading, there's a "exit(2)" at the end of above code. I can't find
more exit(2) from xfsprogs/repair/ . So maybe this's the only one place which
can return 2. From the information above that exit(2), you can see that
xfs_repair will return 2 when it find there're some valuable metadata changes in
a log. It think a mount operation maybe can replay this log, so it return 2 and
suggest the user try to mount the filesystem. If mount can't replay the log, -L
is the next choice.

So I think the _repair_scratch_fs function in xfstests/common/rc doesn't think
about above situation. xfs_repair doesn't always return 2 if log corrupted.
Only xfs_repair feel log can be replay, it'll return 2, or it'll return 1. So
maybe we should change "if [ $res -eq 2 ]" to "if [ $res -ne 0 ]". Or we need
to change xfs_repair to make it return 2:-P

For xfs/098's problem, you can change the line#96:
from
_scratch_xfs_repair >> $seqres.full 2>&1
to
_repair_scratch_fs >> $seqres.full 2>&1

And _repair_scratch_fs need to be modified as I said above. I think I should write
a patch to describe the return value of xfs_repair(without -n). The current
xfs_repair manpage said:
"xfs_repair run without the -n option will always return a status code of 0."
it's wrong.

OK, I've talked too much. If anyone feel anything wrong, please corrent me:)

Thanks,
Zorro

> 
> Thanks,
> yang
> >>Thanks
> >>Xiao Yang.
> >>>>  echo "+ mount image (2)"
> >>>>  _scratch_mount
> >>>>-- 
> >>>>1.8.3.1
> >>>>
> >>>>
> >>>>
> >>>>--
> >>>>To unsubscribe from this list: send the line "unsubscribe fstests" in
> >>>>the body of a message to majordomo@vger.kernel.org
> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>.
> >>>
> >>
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe fstests" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >.
> >
> 
> 
> 

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Zorro Lang]

On Fri, Sep 09, 2016 at 05:29:37PM +0800, Xiao Yang wrote:
> 于 2016/08/26 17:05, Zorro Lang 写道:
> >On Fri, Aug 26, 2016 at 02:10:00PM +0800, Xiao Yang wrote:
> >>On 2016/08/26 12:48, Zorro Lang wrote:
> >>>On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
> >>>>Make sure xfs_repair can't clear the log by default when it is corrupted.
> >>>>xfs_repair always and only clear the log when the -L parameter is specified.
> >>>>This has updated by:
> >>>>Commit f2053bc ("xfs_repair: don't clear the log by default")
> >>>>
> >>>>Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> >>>>---
> >>>>  common/rc     | 4 ++--
> >>>>  tests/xfs/098 | 2 +-
> >>>>  2 files changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>>diff --git a/common/rc b/common/rc
> >>>>index 3fb0600..c693a31 100644
> >>>>--- a/common/rc
> >>>>+++ b/common/rc
> >>>>@@ -1143,9 +1143,9 @@ _repair_scratch_fs()
> >>>Hi Xiao
> >>>
> >>>You should explain why you changed this function in commit log. Or
> >>>the reviewer can't understand why you change it.
> >>>
> >>>>      xfs)
> >>>>          _scratch_xfs_repair "$@" 2>&1
> >>>>  	res=$?
> >>>>-	if [ "$res" -eq 2 ]; then
> >>>>+	if [ "$res" -ne 0 ]; then
> >>>Hi Darrick,
> >>>
> >>>The xfs_repair manpage said:
> >>>xfs_repair run without the -n option will always return a status code of 0.
> >>>
> >>>I don't understand why you think it return 2 here? (Please check below)
> >>>
> >>Hi Zorro
> >>
> >>I don't understand why it return 2 here too.  I want to change this
> >>function because xfs_repair
> >>without -L option return 1 when log is corrupted on newer xfsprogs-dev.
> >>>>  		echo "xfs_repair returns $res; replay log?"
> >>>>-		_scratch_mount
> >>>>+		_scratch_mount 2>&1
> >>>>  		res=$?
> >>>>  		if [ "$res" -gt 0 ]; then
> >>>>  			echo "mount returns $res; zap log?"
> >>>>diff --git a/tests/xfs/098 b/tests/xfs/098
> >>>>index d91d617..eb33bb1 100755
> >>>>--- a/tests/xfs/098
> >>>>+++ b/tests/xfs/098
> >>>>@@ -93,7 +93,7 @@ echo "+ mount image"
> >>>>  _scratch_mount 2>/dev/null&&   _fail "mount should not succeed"
> >>>>
> >>>>  echo "+ repair fs"
> >>>>-_scratch_xfs_repair>>   $seqres.full 2>&1
> >>>>+_repair_scratch_fs>>   $seqres.full
> >You should print the stderr to $seqres.full too. Because in
> >"_repair_scratch_fs", its code likes below:
> >
> >     xfs)
> >         _scratch_xfs_repair "$@" 2>&1
> >>>>This repair won't clear the corrupted log anymore.
> >         res=$?
> >         if [ "$res" -eq 2 ]; then
> >                 echo "xfs_repair returns $res; replay log?"
> >                 _scratch_mount
> >>>>So this mount maybe failed if it can't deal with the corrupted log.
> >>>>If it print some error messages, it'll break the golden image of xfs/098
> >                 res=$?
> >                 if [ "$res" -gt 0 ]; then
> >                         echo "mount returns $res; zap log?"
> >                         _scratch_xfs_repair -L 2>&1
> >
> >
> >>>If just call xfs_repair without any options, the _repair_scratch_fs won't
> >>>help to call xfs_repair -L I think.
> >>>
> >>>So I think this patch won't fix the problem.
> >>>
> >>>Feel free to correct me, if I misunderstand something:)
> >>>
> >>>Thanks,
> >>>Zorro
> >>>
> >>If xfs_repair without any option succeed to repair filesystem when
> >>log is corrupted,
> >>_repair_scratch_fs don't need  to call  xfs_repair -L.  If it failed
> >>to repair filesystem,
> >>_repair_scratch_fs needs  to call  xfs_repair -L.
> >Oh, sorry, I just tried to run ths case. The "_scratch_xfs_repair" really return
> >non-zero when it try to repair a corrupted xfs...
> >
> >But the manpage(man xfs_repair) really said:
> >xfs_repair run without the -n option will always return a status code of 0.
> >
> >Maybe we should update the manpage? I'll check it later.
> >
> >Any way, there's still a problem in your patch, please see above:
> >
> >Thanks,
> >Zorro
> Hi Zorro
> Do you know why it returns 2 instead of 1 when we use xfs_repair
> without any options.
> I can't understand it, because it always return 1 on my machine.

Hi Xiao,

Please CC the mail list, there's no secret. And the most important
thing is if I said something wrong, others great developers maybe
glad to correct me:-P

I've asked DJ Wong about the return value of xfs_repair, and he
already replied:

"xfs_repair returns 2 when the log is corrupted, 1 when there's corruption left
to be fixed *or* some kind of operation error happened, and 0 if either it
found nothing wrong or all the corruptions were fixed."

I'm sure that email has been sent to you too.

If you can't understand why it return 1, you can check your xfs/098.full file,
you'll find:

"Phase 1 - find and verify superblock...
Phase 2 - using internal log
        - zero log...
Log inconsistent (didn't find previous header)
failed to find log head
zero_log: cannot find log head/tail (xlog_find_tail=5)

fatal error -- ERROR: The log head and/or tail cannot be discovered. Attempt to mount the
filesystem to replay the log or use the -L option to destroy the log and
attempt a repair.
xfs_repair failed, err=1"

This output from below xfsprogs code:

        error = xlog_find_tail(log, &head_blk, &tail_blk);
        if (error) {
                do_warn(
                _("zero_log: cannot find log head/tail (xlog_find_tail=%d)\n"),
                        error);
                if (!no_modify && !zap_log)
>>> [exit from here] >>>    do_error(_(
"ERROR: The log head and/or tail cannot be discovered. Attempt to mount the\n"
"filesystem to replay the log or use the -L option to destroy the log and\n"
"attempt a repair.\n"));
        } else {
                if (verbose) {
                        do_warn(
        _("zero_log: head block %" PRId64 " tail block %" PRId64 "\n"),
                                head_blk, tail_blk);
                }
                if (!no_modify && head_blk != tail_blk) {
                        if (zap_log) {
                                do_warn(_(
"ALERT: The filesystem has valuable metadata changes in a log which is being\n"
"destroyed because the -L option was used.\n"));
                        } else {
                                do_warn(_(
"ERROR: The filesystem has valuable metadata changes in a log which needs to\n"
"be replayed.  Mount the filesystem to replay the log, and unmount it before\n"
"re-running xfs_repair.  If you are unable to mount the filesystem, then use\n"
"the -L option to destroy the log and attempt a repair.\n"
"Note that destroying the log may cause corruption -- please attempt a mount\n"
"of the filesystem before doing this.\n"));
                                exit(2);
                        }
                }
        }

I've marked [exit from here] for you. do_error will call exit(1). And the output
message already tell you the reason about why it fail.

You can keep reading, there's a "exit(2)" at the end of above code. I can't find
more exit(2) from xfsprogs/repair/ . So maybe this's the only one place which
can return 2. From the information above that exit(2), you can see that
xfs_repair will return 2 when it find there're some valuable metadata changes in
a log. It think a mount operation maybe can replay this log, so it return 2 and
suggest the user try to mount the filesystem. If mount can't replay the log, -L
is the next choice.

So I think the _repair_scratch_fs function in xfstests/common/rc doesn't think
about above situation. xfs_repair doesn't always return 2 if log corrupted.
Only xfs_repair feel log can be replay, it'll return 2, or it'll return 1. So
maybe we should change "if [ $res -eq 2 ]" to "if [ $res -ne 0 ]". Or we need
to change xfs_repair to make it return 2:-P

For xfs/098's problem, you can change the line#96:
from
_scratch_xfs_repair >> $seqres.full 2>&1
to
_repair_scratch_fs >> $seqres.full 2>&1

And _repair_scratch_fs need to be modified as I said above. I think I should write
a patch to describe the return value of xfs_repair(without -n). The current
xfs_repair manpage said:
"xfs_repair run without the -n option will always return a status code of 0."
it's wrong.

OK, I've talked too much. If anyone feel anything wrong, please corrent me:)

Thanks,
Zorro

> 
> Thanks,
> yang
> >>Thanks
> >>Xiao Yang.
> >>>>  echo "+ mount image (2)"
> >>>>  _scratch_mount
> >>>>-- 
> >>>>1.8.3.1
> >>>>
> >>>>
> >>>>
> >>>>--
> >>>>To unsubscribe from this list: send the line "unsubscribe fstests" in
> >>>>the body of a message to majordomo@vger.kernel.org
> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>.
> >>>
> >>
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe fstests" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >.
> >
> 
> 
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

于 2016/09/09 20:28, Zorro Lang 写道:
> On Fri, Sep 09, 2016 at 05:29:37PM +0800, Xiao Yang wrote:
>> 于 2016/08/26 17:05, Zorro Lang 写道:
>>> On Fri, Aug 26, 2016 at 02:10:00PM +0800, Xiao Yang wrote:
>>>> On 2016/08/26 12:48, Zorro Lang wrote:
>>>>> On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
>>>>>> Make sure xfs_repair can't clear the log by default when it is corrupted.
>>>>>> xfs_repair always and only clear the log when the -L parameter is specified.
>>>>>> This has updated by:
>>>>>> Commit f2053bc ("xfs_repair: don't clear the log by default")
>>>>>>
>>>>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>>>>>> ---
>>>>>>  common/rc     | 4 ++--
>>>>>>  tests/xfs/098 | 2 +-
>>>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/common/rc b/common/rc
>>>>>> index 3fb0600..c693a31 100644
>>>>>> --- a/common/rc
>>>>>> +++ b/common/rc
>>>>>> @@ -1143,9 +1143,9 @@ _repair_scratch_fs()
>>>>> Hi Xiao
>>>>>
>>>>> You should explain why you changed this function in commit log. Or
>>>>> the reviewer can't understand why you change it.
>>>>>
>>>>>>      xfs)
>>>>>>          _scratch_xfs_repair "$@" 2>&1
>>>>>>  	res=$?
>>>>>> -	if [ "$res" -eq 2 ]; then
>>>>>> +	if [ "$res" -ne 0 ]; then
>>>>> Hi Darrick,
>>>>>
>>>>> The xfs_repair manpage said:
>>>>> xfs_repair run without the -n option will always return a status code of 0.
>>>>>
>>>>> I don't understand why you think it return 2 here? (Please check below)
>>>>>
>>>> Hi Zorro
>>>>
>>>> I don't understand why it return 2 here too.  I want to change this
>>>> function because xfs_repair
>>>> without -L option return 1 when log is corrupted on newer xfsprogs-dev.
>>>>>>  		echo "xfs_repair returns $res; replay log?"
>>>>>> -		_scratch_mount
>>>>>> +		_scratch_mount 2>&1
>>>>>>  		res=$?
>>>>>>  		if [ "$res" -gt 0 ]; then
>>>>>>  			echo "mount returns $res; zap log?"
>>>>>> diff --git a/tests/xfs/098 b/tests/xfs/098
>>>>>> index d91d617..eb33bb1 100755
>>>>>> --- a/tests/xfs/098
>>>>>> +++ b/tests/xfs/098
>>>>>> @@ -93,7 +93,7 @@ echo "+ mount image"
>>>>>>  _scratch_mount 2>/dev/null&&   _fail "mount should not succeed"
>>>>>>
>>>>>>  echo "+ repair fs"
>>>>>> -_scratch_xfs_repair>>   $seqres.full 2>&1
>>>>>> +_repair_scratch_fs>>   $seqres.full
>>> You should print the stderr to $seqres.full too. Because in
>>> "_repair_scratch_fs", its code likes below:
>>>
>>>     xfs)
>>>         _scratch_xfs_repair "$@" 2>&1
>>>>>> This repair won't clear the corrupted log anymore.
>>>         res=$?
>>>         if [ "$res" -eq 2 ]; then
>>>                 echo "xfs_repair returns $res; replay log?"
>>>                 _scratch_mount
>>>>>> So this mount maybe failed if it can't deal with the corrupted log.
>>>>>> If it print some error messages, it'll break the golden image of xfs/098
>>>                 res=$?
>>>                 if [ "$res" -gt 0 ]; then
>>>                         echo "mount returns $res; zap log?"
>>>                         _scratch_xfs_repair -L 2>&1
>>>
>>>
>>>>> If just call xfs_repair without any options, the _repair_scratch_fs won't
>>>>> help to call xfs_repair -L I think.
>>>>>
>>>>> So I think this patch won't fix the problem.
>>>>>
>>>>> Feel free to correct me, if I misunderstand something:)
>>>>>
>>>>> Thanks,
>>>>> Zorro
>>>>>
>>>> If xfs_repair without any option succeed to repair filesystem when
>>>> log is corrupted,
>>>> _repair_scratch_fs don't need  to call  xfs_repair -L.  If it failed
>>>> to repair filesystem,
>>>> _repair_scratch_fs needs  to call  xfs_repair -L.
>>> Oh, sorry, I just tried to run ths case. The "_scratch_xfs_repair" really return
>>> non-zero when it try to repair a corrupted xfs...
>>>
>>> But the manpage(man xfs_repair) really said:
>>> xfs_repair run without the -n option will always return a status code of 0.
>>>
>>> Maybe we should update the manpage? I'll check it later.
>>>
>>> Any way, there's still a problem in your patch, please see above:
>>>
>>> Thanks,
>>> Zorro
>> Hi Zorro
>> Do you know why it returns 2 instead of 1 when we use xfs_repair
>> without any options.
>> I can't understand it, because it always return 1 on my machine.
> Hi Xiao,
>
> Please CC the mail list, there's no secret. And the most important
> thing is if I said something wrong, others great developers maybe
> glad to correct me:-P
>
> I've asked DJ Wong about the return value of xfs_repair, and he
> already replied:
>
> "xfs_repair returns 2 when the log is corrupted, 1 when there's corruption left
> to be fixed *or* some kind of operation error happened, and 0 if either it
> found nothing wrong or all the corruptions were fixed."
>
> I'm sure that email has been sent to you too.
>
> If you can't understand why it return 1, you can check your xfs/098.full file,
> you'll find:
>
> "Phase 1 - find and verify superblock...
> Phase 2 - using internal log
>         - zero log...
> Log inconsistent (didn't find previous header)
> failed to find log head
> zero_log: cannot find log head/tail (xlog_find_tail=5)
>
> fatal error -- ERROR: The log head and/or tail cannot be discovered. Attempt to mount the
> filesystem to replay the log or use the -L option to destroy the log and
> attempt a repair.
> xfs_repair failed, err=1"
>
> This output from below xfsprogs code:
>
>         error = xlog_find_tail(log, &head_blk, &tail_blk);
>         if (error) {
>                 do_warn(
>                 _("zero_log: cannot find log head/tail (xlog_find_tail=%d)\n"),
>                         error);
>                 if (!no_modify && !zap_log)
>>>> [exit from here] >>>    do_error(_(
> "ERROR: The log head and/or tail cannot be discovered. Attempt to mount the\n"
> "filesystem to replay the log or use the -L option to destroy the log and\n"
> "attempt a repair.\n"));
>         } else {
>                 if (verbose) {
>                         do_warn(
>         _("zero_log: head block %" PRId64 " tail block %" PRId64 "\n"),
>                                 head_blk, tail_blk);
>                 }
>                 if (!no_modify && head_blk != tail_blk) {
>                         if (zap_log) {
>                                 do_warn(_(
> "ALERT: The filesystem has valuable metadata changes in a log which is being\n"
> "destroyed because the -L option was used.\n"));
>                         } else {
>                                 do_warn(_(
> "ERROR: The filesystem has valuable metadata changes in a log which needs to\n"
> "be replayed.  Mount the filesystem to replay the log, and unmount it before\n"
> "re-running xfs_repair.  If you are unable to mount the filesystem, then use\n"
> "the -L option to destroy the log and attempt a repair.\n"
> "Note that destroying the log may cause corruption -- please attempt a mount\n"
> "of the filesystem before doing this.\n"));
>                                 exit(2);
>                         }
>                 }
>         }
>
> I've marked [exit from here] for you. do_error will call exit(1). And the output
> message already tell you the reason about why it fail.
>
> You can keep reading, there's a "exit(2)" at the end of above code. I can't find
> more exit(2) from xfsprogs/repair/ . So maybe this's the only one place which
> can return 2. From the information above that exit(2), you can see that
> xfs_repair will return 2 when it find there're some valuable metadata changes in
> a log. It think a mount operation maybe can replay this log, so it return 2 and
> suggest the user try to mount the filesystem. If mount can't replay the log, -L
> is the next choice.
>
> So I think the _repair_scratch_fs function in xfstests/common/rc doesn't think
> about above situation. xfs_repair doesn't always return 2 if log corrupted.
> Only xfs_repair feel log can be replay, it'll return 2, or it'll return 1. So
> maybe we should change "if [ $res -eq 2 ]" to "if [ $res -ne 0 ]". Or we need
> to change xfs_repair to make it return 2:-P
>
> For xfs/098's problem, you can change the line#96:
> from
> _scratch_xfs_repair >> $seqres.full 2>&1
> to
> _repair_scratch_fs >> $seqres.full 2>&1
>
> And _repair_scratch_fs need to be modified as I said above. I think I should write
> a patch to describe the return value of xfs_repair(without -n). The current
> xfs_repair manpage said:
> "xfs_repair run without the -n option will always return a status code of 0."
> it's wrong.
>
> OK, I've talked too much. If anyone feel anything wrong, please corrent me:)
>
> Thanks,
> Zorro
>
Thanks for your comment, so i will rewrite this patch.

Thanks,
Xiao Yang
>> Thanks,
>> yang
>>>> Thanks
>>>> Xiao Yang.
>>>>>>  echo "+ mount image (2)"
>>>>>>  _scratch_mount
>>>>>> -- 
>>>>>> 1.8.3.1
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>> .
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> .
>>>
>>
>>
>
> .
>

Re: [PATCH v2] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

于 2016/09/09 20:28, Zorro Lang 写道:
> On Fri, Sep 09, 2016 at 05:29:37PM +0800, Xiao Yang wrote:
>> 于 2016/08/26 17:05, Zorro Lang 写道:
>>> On Fri, Aug 26, 2016 at 02:10:00PM +0800, Xiao Yang wrote:
>>>> On 2016/08/26 12:48, Zorro Lang wrote:
>>>>> On Fri, Aug 26, 2016 at 11:36:52AM +0800, Xiao Yang wrote:
>>>>>> Make sure xfs_repair can't clear the log by default when it is corrupted.
>>>>>> xfs_repair always and only clear the log when the -L parameter is specified.
>>>>>> This has updated by:
>>>>>> Commit f2053bc ("xfs_repair: don't clear the log by default")
>>>>>>
>>>>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>>>>>> ---
>>>>>>  common/rc     | 4 ++--
>>>>>>  tests/xfs/098 | 2 +-
>>>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/common/rc b/common/rc
>>>>>> index 3fb0600..c693a31 100644
>>>>>> --- a/common/rc
>>>>>> +++ b/common/rc
>>>>>> @@ -1143,9 +1143,9 @@ _repair_scratch_fs()
>>>>> Hi Xiao
>>>>>
>>>>> You should explain why you changed this function in commit log. Or
>>>>> the reviewer can't understand why you change it.
>>>>>
>>>>>>      xfs)
>>>>>>          _scratch_xfs_repair "$@" 2>&1
>>>>>>  	res=$?
>>>>>> -	if [ "$res" -eq 2 ]; then
>>>>>> +	if [ "$res" -ne 0 ]; then
>>>>> Hi Darrick,
>>>>>
>>>>> The xfs_repair manpage said:
>>>>> xfs_repair run without the -n option will always return a status code of 0.
>>>>>
>>>>> I don't understand why you think it return 2 here? (Please check below)
>>>>>
>>>> Hi Zorro
>>>>
>>>> I don't understand why it return 2 here too.  I want to change this
>>>> function because xfs_repair
>>>> without -L option return 1 when log is corrupted on newer xfsprogs-dev.
>>>>>>  		echo "xfs_repair returns $res; replay log?"
>>>>>> -		_scratch_mount
>>>>>> +		_scratch_mount 2>&1
>>>>>>  		res=$?
>>>>>>  		if [ "$res" -gt 0 ]; then
>>>>>>  			echo "mount returns $res; zap log?"
>>>>>> diff --git a/tests/xfs/098 b/tests/xfs/098
>>>>>> index d91d617..eb33bb1 100755
>>>>>> --- a/tests/xfs/098
>>>>>> +++ b/tests/xfs/098
>>>>>> @@ -93,7 +93,7 @@ echo "+ mount image"
>>>>>>  _scratch_mount 2>/dev/null&&   _fail "mount should not succeed"
>>>>>>
>>>>>>  echo "+ repair fs"
>>>>>> -_scratch_xfs_repair>>   $seqres.full 2>&1
>>>>>> +_repair_scratch_fs>>   $seqres.full
>>> You should print the stderr to $seqres.full too. Because in
>>> "_repair_scratch_fs", its code likes below:
>>>
>>>     xfs)
>>>         _scratch_xfs_repair "$@" 2>&1
>>>>>> This repair won't clear the corrupted log anymore.
>>>         res=$?
>>>         if [ "$res" -eq 2 ]; then
>>>                 echo "xfs_repair returns $res; replay log?"
>>>                 _scratch_mount
>>>>>> So this mount maybe failed if it can't deal with the corrupted log.
>>>>>> If it print some error messages, it'll break the golden image of xfs/098
>>>                 res=$?
>>>                 if [ "$res" -gt 0 ]; then
>>>                         echo "mount returns $res; zap log?"
>>>                         _scratch_xfs_repair -L 2>&1
>>>
>>>
>>>>> If just call xfs_repair without any options, the _repair_scratch_fs won't
>>>>> help to call xfs_repair -L I think.
>>>>>
>>>>> So I think this patch won't fix the problem.
>>>>>
>>>>> Feel free to correct me, if I misunderstand something:)
>>>>>
>>>>> Thanks,
>>>>> Zorro
>>>>>
>>>> If xfs_repair without any option succeed to repair filesystem when
>>>> log is corrupted,
>>>> _repair_scratch_fs don't need  to call  xfs_repair -L.  If it failed
>>>> to repair filesystem,
>>>> _repair_scratch_fs needs  to call  xfs_repair -L.
>>> Oh, sorry, I just tried to run ths case. The "_scratch_xfs_repair" really return
>>> non-zero when it try to repair a corrupted xfs...
>>>
>>> But the manpage(man xfs_repair) really said:
>>> xfs_repair run without the -n option will always return a status code of 0.
>>>
>>> Maybe we should update the manpage? I'll check it later.
>>>
>>> Any way, there's still a problem in your patch, please see above:
>>>
>>> Thanks,
>>> Zorro
>> Hi Zorro
>> Do you know why it returns 2 instead of 1 when we use xfs_repair
>> without any options.
>> I can't understand it, because it always return 1 on my machine.
> Hi Xiao,
>
> Please CC the mail list, there's no secret. And the most important
> thing is if I said something wrong, others great developers maybe
> glad to correct me:-P
>
> I've asked DJ Wong about the return value of xfs_repair, and he
> already replied:
>
> "xfs_repair returns 2 when the log is corrupted, 1 when there's corruption left
> to be fixed *or* some kind of operation error happened, and 0 if either it
> found nothing wrong or all the corruptions were fixed."
>
> I'm sure that email has been sent to you too.
>
> If you can't understand why it return 1, you can check your xfs/098.full file,
> you'll find:
>
> "Phase 1 - find and verify superblock...
> Phase 2 - using internal log
>         - zero log...
> Log inconsistent (didn't find previous header)
> failed to find log head
> zero_log: cannot find log head/tail (xlog_find_tail=5)
>
> fatal error -- ERROR: The log head and/or tail cannot be discovered. Attempt to mount the
> filesystem to replay the log or use the -L option to destroy the log and
> attempt a repair.
> xfs_repair failed, err=1"
>
> This output from below xfsprogs code:
>
>         error = xlog_find_tail(log, &head_blk, &tail_blk);
>         if (error) {
>                 do_warn(
>                 _("zero_log: cannot find log head/tail (xlog_find_tail=%d)\n"),
>                         error);
>                 if (!no_modify && !zap_log)
>>>> [exit from here] >>>    do_error(_(
> "ERROR: The log head and/or tail cannot be discovered. Attempt to mount the\n"
> "filesystem to replay the log or use the -L option to destroy the log and\n"
> "attempt a repair.\n"));
>         } else {
>                 if (verbose) {
>                         do_warn(
>         _("zero_log: head block %" PRId64 " tail block %" PRId64 "\n"),
>                                 head_blk, tail_blk);
>                 }
>                 if (!no_modify && head_blk != tail_blk) {
>                         if (zap_log) {
>                                 do_warn(_(
> "ALERT: The filesystem has valuable metadata changes in a log which is being\n"
> "destroyed because the -L option was used.\n"));
>                         } else {
>                                 do_warn(_(
> "ERROR: The filesystem has valuable metadata changes in a log which needs to\n"
> "be replayed.  Mount the filesystem to replay the log, and unmount it before\n"
> "re-running xfs_repair.  If you are unable to mount the filesystem, then use\n"
> "the -L option to destroy the log and attempt a repair.\n"
> "Note that destroying the log may cause corruption -- please attempt a mount\n"
> "of the filesystem before doing this.\n"));
>                                 exit(2);
>                         }
>                 }
>         }
>
> I've marked [exit from here] for you. do_error will call exit(1). And the output
> message already tell you the reason about why it fail.
>
> You can keep reading, there's a "exit(2)" at the end of above code. I can't find
> more exit(2) from xfsprogs/repair/ . So maybe this's the only one place which
> can return 2. From the information above that exit(2), you can see that
> xfs_repair will return 2 when it find there're some valuable metadata changes in
> a log. It think a mount operation maybe can replay this log, so it return 2 and
> suggest the user try to mount the filesystem. If mount can't replay the log, -L
> is the next choice.
>
> So I think the _repair_scratch_fs function in xfstests/common/rc doesn't think
> about above situation. xfs_repair doesn't always return 2 if log corrupted.
> Only xfs_repair feel log can be replay, it'll return 2, or it'll return 1. So
> maybe we should change "if [ $res -eq 2 ]" to "if [ $res -ne 0 ]". Or we need
> to change xfs_repair to make it return 2:-P
>
> For xfs/098's problem, you can change the line#96:
> from
> _scratch_xfs_repair >> $seqres.full 2>&1
> to
> _repair_scratch_fs >> $seqres.full 2>&1
>
> And _repair_scratch_fs need to be modified as I said above. I think I should write
> a patch to describe the return value of xfs_repair(without -n). The current
> xfs_repair manpage said:
> "xfs_repair run without the -n option will always return a status code of 0."
> it's wrong.
>
> OK, I've talked too much. If anyone feel anything wrong, please corrent me:)
>
> Thanks,
> Zorro
>
Thanks for your comment, so i will rewrite this patch.

Thanks,
Xiao Yang
>> Thanks,
>> yang
>>>> Thanks
>>>> Xiao Yang.
>>>>>>  echo "+ mount image (2)"
>>>>>>  _scratch_mount
>>>>>> -- 
>>>>>> 1.8.3.1
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>> .
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> .
>>>
>>
>>
>
> .
>



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

The obsolete xfs_repair always cleared the log regardless of whether it
is corrupted.   However current xfs_repair only cleared the log when -L
option is specified, so xfs_repair without any options failed to clear log
on newer xfsprogs.  If xfs_repair failed to clear log, xfs_repair -L option
should be used to clear it.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc     | 10 ++++++----
 tests/xfs/098 |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/common/rc b/common/rc
index 04039a4..fda108c 100644
--- a/common/rc
+++ b/common/rc
@@ -1134,16 +1134,18 @@ _scratch_xfs_repair()
     $XFS_REPAIR_PROG $SCRATCH_OPTIONS $* $SCRATCH_DEV
 }
 
-# Repair scratch filesystem.  Returns 0 if the FS is good to go (either no
-# errors found or errors were fixed) and nonzero otherwise; also spits out
-# a complaint on stderr if fsck didn't tell us that the FS is good to go.
+# Repair scratch filesystem.  Return 2 if the filesystem has valuable
+# metadata changes in log which needs to be replayed, 1 if there's
+# corruption left to be fixed or can't find log head and tail or some
+# other errors happened, and 0 if nothing wrong or all the corruptions
+# were fixed.
 _repair_scratch_fs()
 {
     case $FSTYP in
     xfs)
         _scratch_xfs_repair "$@" 2>&1
 	res=$?
-	if [ "$res" -eq 2 ]; then
+	if [ "$res" -ne 0 ]; then
 		echo "xfs_repair returns $res; replay log?"
 		_scratch_mount
 		res=$?
diff --git a/tests/xfs/098 b/tests/xfs/098
index d91d617..3743e78 100755
--- a/tests/xfs/098
+++ b/tests/xfs/098
@@ -93,7 +93,7 @@ echo "+ mount image"
 _scratch_mount 2>/dev/null && _fail "mount should not succeed"
 
 echo "+ repair fs"
-_scratch_xfs_repair >> $seqres.full 2>&1
+_repair_scratch_fs >> $seqres.full 2>&1
 
 echo "+ mount image (2)"
 _scratch_mount
-- 
1.8.3.1

Re: [PATCH v3] xfs/098: fix xfs_repair on newer xfsprogs

[Eric Sandeen]

On 9/12/16 12:13 AM, Xiao Yang wrote:
> The obsolete xfs_repair always cleared the log regardless of whether it
> is corrupted.   However current xfs_repair only cleared the log when -L
> option is specified, so xfs_repair without any options failed to clear log
> on newer xfsprogs.  If xfs_repair failed to clear log, xfs_repair -L option
> should be used to clear it.
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  common/rc     | 10 ++++++----
>  tests/xfs/098 |  2 +-
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 04039a4..fda108c 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1134,16 +1134,18 @@ _scratch_xfs_repair()
>      $XFS_REPAIR_PROG $SCRATCH_OPTIONS $* $SCRATCH_DEV
>  }
>  
> -# Repair scratch filesystem.  Returns 0 if the FS is good to go (either no
> -# errors found or errors were fixed) and nonzero otherwise; also spits out
> -# a complaint on stderr if fsck didn't tell us that the FS is good to go.
> +# Repair scratch filesystem.  Return 2 if the filesystem has valuable
> +# metadata changes in log which needs to be replayed, 1 if there's
> +# corruption left to be fixed or can't find log head and tail or some
> +# other errors happened, and 0 if nothing wrong or all the corruptions
> +# were fixed.

I'm sorry to nitpick; this looks almost correct to me....

I think the problem here is really due to a bug in xfsprogs, (see patch
[PATCH] xfs_repair: exit with status 2 if log dirtiness is unknown sent
to the xfs list today), but we do need to handle binaries between 4.3.0
and if/when that fix gets applied, so catching any non-zero return value
below does make sense to me.

But the new comment above is very xfs-specific, and _repair_scratch_fs
is a generic function (it has a *) default $FSTYP case...)

And even if /xfs_repair/ returns 2, the bash function
_repair_scratch_fs() won't; $res gets overwritten by the last
call to xfs_repair, at which point there should be no dirty log.
So _repair_scratch_fs() really only returns 0 or 1, even if xfs_repair
may have a return value of 2.  So the new comment is not correct.

To fix that, I would simply leave the comment unchanged.


The other remaining problem I see is this, on a CONFIG_XFS_WARN kernel:

xfs/098 12s ... 12s
_check_dmesg: something found in dmesg (see /mnt/test2/git/xfstests/results//xfs/098.dmesg)
Ran: xfs/098
Failures: xfs/098
Failed 1 of 1 tests

because the failed mount issued warnings, at least on a CONFIG_XFS_WARN build:

[249062.871158] XFS (sdb2): log mount failed
[249063.170109] XFS (sdb2): Mounting V5 Filesystem
[249063.266522] XFS (sdb2): Log inconsistent (didn't find previous header)
[249063.273135] XFS: Assertion failed: 0, file: fs/xfs/xfs_log_recover.c, line: 540
...

so we should probably have a _disable_dmesg_check; I'm not sure if it would
be better to put it in _repair_scratch_fs in the "mount failed, zap
the log" case, or to put it in test 098 directly.  I think it would
be better to put it in 098, because we know we are dealing with a corrupt
log and can expect the message; putting it in _repair_scratch_fs may mask
problems on other tests that use it.

Thanks,
-Eric



>  _repair_scratch_fs()
>  {
>      case $FSTYP in
>      xfs)
>          _scratch_xfs_repair "$@" 2>&1
>  	res=$?
> -	if [ "$res" -eq 2 ]; then
> +	if [ "$res" -ne 0 ]; then
>  		echo "xfs_repair returns $res; replay log?"
>  		_scratch_mount
>  		res=$?
> diff --git a/tests/xfs/098 b/tests/xfs/098
> index d91d617..3743e78 100755
> --- a/tests/xfs/098
> +++ b/tests/xfs/098
> @@ -93,7 +93,7 @@ echo "+ mount image"
>  _scratch_mount 2>/dev/null && _fail "mount should not succeed"
>  
>  echo "+ repair fs"
> -_scratch_xfs_repair >> $seqres.full 2>&1
> +_repair_scratch_fs >> $seqres.full 2>&1
>  
>  echo "+ mount image (2)"
>  _scratch_mount
> 

Re: [PATCH v3] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

On 2016/09/12 20:59, Eric Sandeen wrote:
> On 9/12/16 12:13 AM, Xiao Yang wrote:
>> The obsolete xfs_repair always cleared the log regardless of whether it
>> is corrupted.   However current xfs_repair only cleared the log when -L
>> option is specified, so xfs_repair without any options failed to clear log
>> on newer xfsprogs.  If xfs_repair failed to clear log, xfs_repair -L option
>> should be used to clear it.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   common/rc     | 10 ++++++----
>>   tests/xfs/098 |  2 +-
>>   2 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 04039a4..fda108c 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -1134,16 +1134,18 @@ _scratch_xfs_repair()
>>       $XFS_REPAIR_PROG $SCRATCH_OPTIONS $* $SCRATCH_DEV
>>   }
>>
>> -# Repair scratch filesystem.  Returns 0 if the FS is good to go (either no
>> -# errors found or errors were fixed) and nonzero otherwise; also spits out
>> -# a complaint on stderr if fsck didn't tell us that the FS is good to go.
>> +# Repair scratch filesystem.  Return 2 if the filesystem has valuable
>> +# metadata changes in log which needs to be replayed, 1 if there's
>> +# corruption left to be fixed or can't find log head and tail or some
>> +# other errors happened, and 0 if nothing wrong or all the corruptions
>> +# were fixed.
> I'm sorry to nitpick; this looks almost correct to me....
>
> I think the problem here is really due to a bug in xfsprogs, (see patch
> [PATCH] xfs_repair: exit with status 2 if log dirtiness is unknown sent
> to the xfs list today), but we do need to handle binaries between 4.3.0
> and if/when that fix gets applied, so catching any non-zero return value
> below does make sense to me.
>
> But the new comment above is very xfs-specific, and _repair_scratch_fs
> is a generic function (it has a *) default $FSTYP case...)
>
> And even if /xfs_repair/ returns 2, the bash function
> _repair_scratch_fs() won't; $res gets overwritten by the last
> call to xfs_repair, at which point there should be no dirty log.
> So _repair_scratch_fs() really only returns 0 or 1, even if xfs_repair
> may have a return value of 2.  So the new comment is not correct.
>
> To fix that, I would simply leave the comment unchanged.
>
>
Hi Eric
Thanks for your suggestion, i agree with you.
I will leave the comment unchanged.
> The other remaining problem I see is this, on a CONFIG_XFS_WARN kernel:
>
> xfs/098 12s ... 12s
> _check_dmesg: something found in dmesg (see /mnt/test2/git/xfstests/results//xfs/098.dmesg)
> Ran: xfs/098
> Failures: xfs/098
> Failed 1 of 1 tests
>
> because the failed mount issued warnings, at least on a CONFIG_XFS_WARN build:
>
> [249062.871158] XFS (sdb2): log mount failed
> [249063.170109] XFS (sdb2): Mounting V5 Filesystem
> [249063.266522] XFS (sdb2): Log inconsistent (didn't find previous header)
> [249063.273135] XFS: Assertion failed: 0, file: fs/xfs/xfs_log_recover.c, line: 540
> ...
>
> so we should probably have a _disable_dmesg_check; I'm not sure if it would
> be better to put it in _repair_scratch_fs in the "mount failed, zap
> the log" case, or to put it in test 098 directly.  I think it would
> be better to put it in 098, because we know we are dealing with a corrupt
> log and can expect the message; putting it in _repair_scratch_fs may mask
> problems on other tests that use it.
>
> Thanks,
> -Eric
>
>
OK, you are right.  I got the same warnings on a CONFIG_XFS_WARN build, 
so i will add _disable_dmesg_check in 098 as you said.
Thanks,
Xiao Yang
>>   _repair_scratch_fs()
>>   {
>>       case $FSTYP in
>>       xfs)
>>           _scratch_xfs_repair "$@" 2>&1
>>   	res=$?
>> -	if [ "$res" -eq 2 ]; then
>> +	if [ "$res" -ne 0 ]; then
>>   		echo "xfs_repair returns $res; replay log?"
>>   		_scratch_mount
>>   		res=$?
>> diff --git a/tests/xfs/098 b/tests/xfs/098
>> index d91d617..3743e78 100755
>> --- a/tests/xfs/098
>> +++ b/tests/xfs/098
>> @@ -93,7 +93,7 @@ echo "+ mount image"
>>   _scratch_mount 2>/dev/null&&  _fail "mount should not succeed"
>>
>>   echo "+ repair fs"
>> -_scratch_xfs_repair>>  $seqres.full 2>&1
>> +_repair_scratch_fs>>  $seqres.full 2>&1
>>
>>   echo "+ mount image (2)"
>>   _scratch_mount
>>
>
> .
>

Re: [PATCH v3] xfs/098: fix xfs_repair on newer xfsprogs

[Zorro Lang]

On Mon, Sep 12, 2016 at 07:59:02AM -0500, Eric Sandeen wrote:
> On 9/12/16 12:13 AM, Xiao Yang wrote:
> > The obsolete xfs_repair always cleared the log regardless of whether it
> > is corrupted.   However current xfs_repair only cleared the log when -L
> > option is specified, so xfs_repair without any options failed to clear log
> > on newer xfsprogs.  If xfs_repair failed to clear log, xfs_repair -L option
> > should be used to clear it.
> > 
> > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> > ---
> >  common/rc     | 10 ++++++----
> >  tests/xfs/098 |  2 +-
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 04039a4..fda108c 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1134,16 +1134,18 @@ _scratch_xfs_repair()
> >      $XFS_REPAIR_PROG $SCRATCH_OPTIONS $* $SCRATCH_DEV
> >  }
> >  
> > -# Repair scratch filesystem.  Returns 0 if the FS is good to go (either no
> > -# errors found or errors were fixed) and nonzero otherwise; also spits out
> > -# a complaint on stderr if fsck didn't tell us that the FS is good to go.
> > +# Repair scratch filesystem.  Return 2 if the filesystem has valuable
> > +# metadata changes in log which needs to be replayed, 1 if there's
> > +# corruption left to be fixed or can't find log head and tail or some
> > +# other errors happened, and 0 if nothing wrong or all the corruptions
> > +# were fixed.
> 
> I'm sorry to nitpick; this looks almost correct to me....
> 
> I think the problem here is really due to a bug in xfsprogs, (see patch
> [PATCH] xfs_repair: exit with status 2 if log dirtiness is unknown sent
> to the xfs list today), but we do need to handle binaries between 4.3.0
> and if/when that fix gets applied, so catching any non-zero return value
> below does make sense to me.

Agree, this comment don't need to be changed.

> 
> But the new comment above is very xfs-specific, and _repair_scratch_fs
> is a generic function (it has a *) default $FSTYP case...)
> 
> And even if /xfs_repair/ returns 2, the bash function
> _repair_scratch_fs() won't; $res gets overwritten by the last
> call to xfs_repair, at which point there should be no dirty log.
> So _repair_scratch_fs() really only returns 0 or 1, even if xfs_repair
> may have a return value of 2.  So the new comment is not correct.
> 
> To fix that, I would simply leave the comment unchanged.
> 
> 
> The other remaining problem I see is this, on a CONFIG_XFS_WARN kernel:
> 
> xfs/098 12s ... 12s
> _check_dmesg: something found in dmesg (see /mnt/test2/git/xfstests/results//xfs/098.dmesg)
> Ran: xfs/098
> Failures: xfs/098
> Failed 1 of 1 tests
> 
> because the failed mount issued warnings, at least on a CONFIG_XFS_WARN build:
> 
> [249062.871158] XFS (sdb2): log mount failed
> [249063.170109] XFS (sdb2): Mounting V5 Filesystem
> [249063.266522] XFS (sdb2): Log inconsistent (didn't find previous header)
> [249063.273135] XFS: Assertion failed: 0, file: fs/xfs/xfs_log_recover.c, line: 540

Wow, I didn't find that. I'll test xfs with CONFIG_XFS_WARN in the future:)

Hi Xiao, if you want to ignore this kernel warning, you can check how
generic/095 do that.

Thanks,
Zorro

> ...
> 
> so we should probably have a _disable_dmesg_check; I'm not sure if it would
> be better to put it in _repair_scratch_fs in the "mount failed, zap
> the log" case, or to put it in test 098 directly.  I think it would
> be better to put it in 098, because we know we are dealing with a corrupt
> log and can expect the message; putting it in _repair_scratch_fs may mask
> problems on other tests that use it.
> 
> Thanks,
> -Eric
> 
> 
> 
> >  _repair_scratch_fs()
> >  {
> >      case $FSTYP in
> >      xfs)
> >          _scratch_xfs_repair "$@" 2>&1
> >  	res=$?
> > -	if [ "$res" -eq 2 ]; then
> > +	if [ "$res" -ne 0 ]; then
> >  		echo "xfs_repair returns $res; replay log?"
> >  		_scratch_mount
> >  		res=$?
> > diff --git a/tests/xfs/098 b/tests/xfs/098
> > index d91d617..3743e78 100755
> > --- a/tests/xfs/098
> > +++ b/tests/xfs/098
> > @@ -93,7 +93,7 @@ echo "+ mount image"
> >  _scratch_mount 2>/dev/null && _fail "mount should not succeed"
> >  
> >  echo "+ repair fs"
> > -_scratch_xfs_repair >> $seqres.full 2>&1
> > +_repair_scratch_fs >> $seqres.full 2>&1
> >  
> >  echo "+ mount image (2)"
> >  _scratch_mount
> > 

Re: [PATCH v3] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

On 2016/09/13 15:08, Zorro Lang wrote:
> On Mon, Sep 12, 2016 at 07:59:02AM -0500, Eric Sandeen wrote:
>> On 9/12/16 12:13 AM, Xiao Yang wrote:
>>> The obsolete xfs_repair always cleared the log regardless of whether it
>>> is corrupted.   However current xfs_repair only cleared the log when -L
>>> option is specified, so xfs_repair without any options failed to clear log
>>> on newer xfsprogs.  If xfs_repair failed to clear log, xfs_repair -L option
>>> should be used to clear it.
>>>
>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>>> ---
>>>   common/rc     | 10 ++++++----
>>>   tests/xfs/098 |  2 +-
>>>   2 files changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/common/rc b/common/rc
>>> index 04039a4..fda108c 100644
>>> --- a/common/rc
>>> +++ b/common/rc
>>> @@ -1134,16 +1134,18 @@ _scratch_xfs_repair()
>>>       $XFS_REPAIR_PROG $SCRATCH_OPTIONS $* $SCRATCH_DEV
>>>   }
>>>
>>> -# Repair scratch filesystem.  Returns 0 if the FS is good to go (either no
>>> -# errors found or errors were fixed) and nonzero otherwise; also spits out
>>> -# a complaint on stderr if fsck didn't tell us that the FS is good to go.
>>> +# Repair scratch filesystem.  Return 2 if the filesystem has valuable
>>> +# metadata changes in log which needs to be replayed, 1 if there's
>>> +# corruption left to be fixed or can't find log head and tail or some
>>> +# other errors happened, and 0 if nothing wrong or all the corruptions
>>> +# were fixed.
>> I'm sorry to nitpick; this looks almost correct to me....
>>
>> I think the problem here is really due to a bug in xfsprogs, (see patch
>> [PATCH] xfs_repair: exit with status 2 if log dirtiness is unknown sent
>> to the xfs list today), but we do need to handle binaries between 4.3.0
>> and if/when that fix gets applied, so catching any non-zero return value
>> below does make sense to me.
> Agree, this comment don't need to be changed.
>
>> But the new comment above is very xfs-specific, and _repair_scratch_fs
>> is a generic function (it has a *) default $FSTYP case...)
>>
>> And even if /xfs_repair/ returns 2, the bash function
>> _repair_scratch_fs() won't; $res gets overwritten by the last
>> call to xfs_repair, at which point there should be no dirty log.
>> So _repair_scratch_fs() really only returns 0 or 1, even if xfs_repair
>> may have a return value of 2.  So the new comment is not correct.
>>
>> To fix that, I would simply leave the comment unchanged.
>>
>>
>> The other remaining problem I see is this, on a CONFIG_XFS_WARN kernel:
>>
>> xfs/098 12s ... 12s
>> _check_dmesg: something found in dmesg (see /mnt/test2/git/xfstests/results//xfs/098.dmesg)
>> Ran: xfs/098
>> Failures: xfs/098
>> Failed 1 of 1 tests
>>
>> because the failed mount issued warnings, at least on a CONFIG_XFS_WARN build:
>>
>> [249062.871158] XFS (sdb2): log mount failed
>> [249063.170109] XFS (sdb2): Mounting V5 Filesystem
>> [249063.266522] XFS (sdb2): Log inconsistent (didn't find previous header)
>> [249063.273135] XFS: Assertion failed: 0, file: fs/xfs/xfs_log_recover.c, line: 540
> Wow, I didn't find that. I'll test xfs with CONFIG_XFS_WARN in the future:)
>
> Hi Xiao, if you want to ignore this kernel warning, you can check how
> generic/095 do that.
>
> Thanks,
> Zorro
>
Hi zorro
Thanks for your help.
I will change the following three points:
1) use _repair_scratch_fs instead of xfs_repair
2) catch non-zero return value instead of 2
3) add filter_xfs_dmesg to ignore mount releated warnings

If you have another suggestion, please tell me. :-)
Thanks,
Xiao Yang
>> ...
>>
>> so we should probably have a _disable_dmesg_check; I'm not sure if it would
>> be better to put it in _repair_scratch_fs in the "mount failed, zap
>> the log" case, or to put it in test 098 directly.  I think it would
>> be better to put it in 098, because we know we are dealing with a corrupt
>> log and can expect the message; putting it in _repair_scratch_fs may mask
>> problems on other tests that use it.
>>
>> Thanks,
>> -Eric
>>
>>
>>
>>>   _repair_scratch_fs()
>>>   {
>>>       case $FSTYP in
>>>       xfs)
>>>           _scratch_xfs_repair "$@" 2>&1
>>>   	res=$?
>>> -	if [ "$res" -eq 2 ]; then
>>> +	if [ "$res" -ne 0 ]; then
>>>   		echo "xfs_repair returns $res; replay log?"
>>>   		_scratch_mount
>>>   		res=$?
>>> diff --git a/tests/xfs/098 b/tests/xfs/098
>>> index d91d617..3743e78 100755
>>> --- a/tests/xfs/098
>>> +++ b/tests/xfs/098
>>> @@ -93,7 +93,7 @@ echo "+ mount image"
>>>   _scratch_mount 2>/dev/null&&  _fail "mount should not succeed"
>>>
>>>   echo "+ repair fs"
>>> -_scratch_xfs_repair>>  $seqres.full 2>&1
>>> +_repair_scratch_fs>>  $seqres.full 2>&1
>>>
>>>   echo "+ mount image (2)"
>>>   _scratch_mount
>>>
>
> .
>

[PATCH v4] xfs/098: fix xfs_repair on newer xfsprogs

[Xiao Yang]

1) use _repair_scratch_fs instead of xfs_repair
   The obsolete xfs_repair always cleared the log regardless of whether
   it is corrupted and current xfs_repair only cleared the log when -L
   option is specified.  xfs_repair -L option should be used to clear it
   if xfs_repair failed to clear log.
2) catch non-zero return value instead of 2
   It can be applied to both the old return value 1 and the new return
   value 2
3) add filter_xfs_dmesg to ignore mount related warnings
   If we corrupt log and mount on a CONFIG_XFS_WARN build, there will be
   mount related warnings in dmesg as expected.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 common/rc     |  2 +-
 tests/xfs/098 | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 13afc6a..655ae28 100644
--- a/common/rc
+++ b/common/rc
@@ -1152,7 +1152,7 @@ _repair_scratch_fs()
     xfs)
         _scratch_xfs_repair "$@" 2>&1
 	res=$?
-	if [ "$res" -eq 2 ]; then
+	if [ "$res" -ne 0 ]; then
 		echo "xfs_repair returns $res; replay log?"
 		_scratch_mount
 		res=$?
diff --git a/tests/xfs/098 b/tests/xfs/098
index d91d617..86ec62c 100755
--- a/tests/xfs/098
+++ b/tests/xfs/098
@@ -54,6 +54,17 @@ _require_xfs_db_blocktrash_z_command
 test -z "${FUZZ_ARGS}" && FUZZ_ARGS="-n 8 -3"
 
 rm -f $seqres.full
+
+# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related
+# WARNINGs in dmesg as expected.  We don't want to simply _disable_dmesg_check
+# which could miss other potential bugs, so filter out the intentional WARNINGs,
+# make sure test doesn't fail because of this warning and fails on other WARNINGs.
+filter_xfs_dmesg()
+{
+	local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*"
+	sed -e "s#$warn#Intentional warnings in asswarn#"
+}
+
 TESTDIR="${SCRATCH_MNT}/scratchdir"
 TESTFILE="${TESTDIR}/testfile"
 
@@ -93,7 +104,10 @@ echo "+ mount image"
 _scratch_mount 2>/dev/null && _fail "mount should not succeed"
 
 echo "+ repair fs"
-_scratch_xfs_repair >> $seqres.full 2>&1
+_repair_scratch_fs >> $seqres.full 2>&1
+
+# mount may trigger related WARNINGs, so filter them.
+_check_dmesg filter_xfs_dmesg
 
 echo "+ mount image (2)"
 _scratch_mount
-- 
1.8.3.1