[PATCH] generic/017: skip invalid block sizes for btrfs

[PATCH] generic/017: skip invalid block sizes for btrfs

[Filipe David Borba Manana]

In btrfs the block size (called sector size in btrfs) can not be
smaller then the page size. Therefore skip block sizes smaller
then page size if the fs is btrfs, so that the test can succeed
on btrfs (testing only with block sizes of 4kb on systems with a
page size of 4Kb).

Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---
 tests/generic/017 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/generic/017 b/tests/generic/017
index 13b7254..6495be5 100755
--- a/tests/generic/017
+++ b/tests/generic/017
@@ -51,6 +51,14 @@ BLOCKS=10240
 
 for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
 
+	# btrfs doesn't support block size smaller then page size
+	if [ "$FSTYP" == "btrfs" ]; then
+		if (( $BSIZE < `getconf PAGE_SIZE` )); then
+			echo "80"
+			continue
+		fi
+	fi
+
 	length=$(($BLOCKS * $BSIZE))
 	case $FSTYP in
 	xfs)
-- 
1.9.1

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Satoru Takeuchi]

Hi Filipe,

(2014/06/23 19:28), Filipe David Borba Manana wrote:
> In btrfs the block size (called sector size in btrfs) can not be
> smaller then the page size. Therefore skip block sizes smaller
> then page size if the fs is btrfs, so that the test can succeed
> on btrfs (testing only with block sizes of 4kb on systems with a
> page size of 4Kb).
> 
> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>

I consider it doesn't work since this test is not for Btrfs.
Please see the following code.

tests/generic/017:
===
for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do

	length=$(($BLOCKS * $BSIZE))
	case $FSTYP in
	xfs)
	_scratch_mkfs -b size=$BSIZE >> $seqres.full 2>&1
	;;
	ext4)
	_scratch_mkfs -b $BSIZE >> $seqres.full 2>&1
	;;
	esac
	_scratch_mount >> $seqres.full 2>&1
===

There is no btrfs here.

This test was moved to shared/005 to generic/017
at 21723cdbf303e031d6429f67fec9768750a5db7d.

Original supported fs is here.
===============================================================================
supported_fs xfs ext4
===============================================================================

I suspect that Lukas moved this test to generic/ by mistake or forgot to
add "$FSTYP == btrfs" case.

Thanks,
Satoru

> ---
>   tests/generic/017 | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/tests/generic/017 b/tests/generic/017
> index 13b7254..6495be5 100755
> --- a/tests/generic/017
> +++ b/tests/generic/017
> @@ -51,6 +51,14 @@ BLOCKS=10240
>   
>   for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>   
> +	# btrfs doesn't support block size smaller then page size
> +	if [ "$FSTYP" == "btrfs" ]; then
> +		if (( $BSIZE < `getconf PAGE_SIZE` )); then
> +			echo "80"
> +			continue
> +		fi
> +	fi
> +
>   	length=$(($BLOCKS * $BSIZE))
>   	case $FSTYP in
>   	xfs)
> 

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Filipe David Manana]

On Mon, Jun 23, 2014 at 11:48 AM, Satoru Takeuchi
<takeuchi_satoru@jp.fujitsu.com> wrote:
> Hi Filipe,
>
> (2014/06/23 19:28), Filipe David Borba Manana wrote:
>> In btrfs the block size (called sector size in btrfs) can not be
>> smaller then the page size. Therefore skip block sizes smaller
>> then page size if the fs is btrfs, so that the test can succeed
>> on btrfs (testing only with block sizes of 4kb on systems with a
>> page size of 4Kb).
>>
>> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
>
> I consider it doesn't work since this test is not for Btrfs.

Did you try it out (together with the corresponding change for btrfs
to support collapse range) or is just code analysis?
For me it works.

> Please see the following code.
>
> tests/generic/017:
> ===
> for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>
>         length=$(($BLOCKS * $BSIZE))
>         case $FSTYP in
>         xfs)
>         _scratch_mkfs -b size=$BSIZE >> $seqres.full 2>&1
>         ;;
>         ext4)
>         _scratch_mkfs -b $BSIZE >> $seqres.full 2>&1
>         ;;
>         esac
>         _scratch_mount >> $seqres.full 2>&1
> ===
>
> There is no btrfs here.

Yes...
That means it doesn't create a brand new fs and just uses the currently one.

Thanks.

>
> This test was moved to shared/005 to generic/017
> at 21723cdbf303e031d6429f67fec9768750a5db7d.
>
> Original supported fs is here.
> ===============================================================================
> supported_fs xfs ext4
> ===============================================================================
>
> I suspect that Lukas moved this test to generic/ by mistake or forgot to
> add "$FSTYP == btrfs" case.
>
> Thanks,
> Satoru
>
>> ---
>>   tests/generic/017 | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/tests/generic/017 b/tests/generic/017
>> index 13b7254..6495be5 100755
>> --- a/tests/generic/017
>> +++ b/tests/generic/017
>> @@ -51,6 +51,14 @@ BLOCKS=10240
>>
>>   for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>>
>> +     # btrfs doesn't support block size smaller then page size
>> +     if [ "$FSTYP" == "btrfs" ]; then
>> +             if (( $BSIZE < `getconf PAGE_SIZE` )); then
>> +                     echo "80"
>> +                     continue
>> +             fi
>> +     fi
>> +
>>       length=$(($BLOCKS * $BSIZE))
>>       case $FSTYP in
>>       xfs)
>>
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Satoru Takeuchi]

Hi Filipe,

(2014/06/23 19:55), Filipe David Manana wrote:
> On Mon, Jun 23, 2014 at 11:48 AM, Satoru Takeuchi
> <takeuchi_satoru@jp.fujitsu.com> wrote:
>> Hi Filipe,
>>
>> (2014/06/23 19:28), Filipe David Borba Manana wrote:
>>> In btrfs the block size (called sector size in btrfs) can not be
>>> smaller then the page size. Therefore skip block sizes smaller
>>> then page size if the fs is btrfs, so that the test can succeed
>>> on btrfs (testing only with block sizes of 4kb on systems with a
>>> page size of 4Kb).
>>>
>>> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
>>
>> I consider it doesn't work since this test is not for Btrfs.
>
> Did you try it out (together with the corresponding change for btrfs
> to support collapse range) or is just code analysis?

I commented it just by code analysis.

> For me it works.
>
>> Please see the following code.
>>
>> tests/generic/017:
>> ===
>> for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>>
>>          length=$(($BLOCKS * $BSIZE))
>>          case $FSTYP in
>>          xfs)
>>          _scratch_mkfs -b size=$BSIZE >> $seqres.full 2>&1
>>          ;;
>>          ext4)
>>          _scratch_mkfs -b $BSIZE >> $seqres.full 2>&1
>>          ;;
>>          esac
>>          _scratch_mount >> $seqres.full 2>&1
>> ===
>>
>> There is no btrfs here.
>
> Yes...
> That means it doesn't create a brand new fs and just uses the currently one.

Yes. I think so too.

Thanks,
Satoru

>
> Thanks.
>
>>
>> This test was moved to shared/005 to generic/017
>> at 21723cdbf303e031d6429f67fec9768750a5db7d.
>>
>> Original supported fs is here.
>> ===============================================================================
>> supported_fs xfs ext4
>> ===============================================================================
>>
>> I suspect that Lukas moved this test to generic/ by mistake or forgot to
>> add "$FSTYP == btrfs" case.
>>
>> Thanks,
>> Satoru
>>
>>> ---
>>>    tests/generic/017 | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/tests/generic/017 b/tests/generic/017
>>> index 13b7254..6495be5 100755
>>> --- a/tests/generic/017
>>> +++ b/tests/generic/017
>>> @@ -51,6 +51,14 @@ BLOCKS=10240
>>>
>>>    for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>>>
>>> +     # btrfs doesn't support block size smaller then page size
>>> +     if [ "$FSTYP" == "btrfs" ]; then
>>> +             if (( $BSIZE < `getconf PAGE_SIZE` )); then
>>> +                     echo "80"
>>> +                     continue
>>> +             fi
>>> +     fi
>>> +
>>>        length=$(($BLOCKS * $BSIZE))
>>>        case $FSTYP in
>>>        xfs)
>>>
>>
>
>
>

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Brendan Hide]

Not subscribed to fstests so not sure if this will reach that mailing 
list...

I feel Takeuchi's instincts are right, even if the analysis *may* be 
wrong. As is it looks like there should be a btrfs) selector inside the 
case.

On 23/06/14 12:48, Satoru Takeuchi wrote:
> Hi Filipe,
>
> (2014/06/23 19:28), Filipe David Borba Manana wrote:
>> In btrfs the block size (called sector size in btrfs) can not be
>> smaller then the page size. Therefore skip block sizes smaller
>> then page size if the fs is btrfs, so that the test can succeed
>> on btrfs (testing only with block sizes of 4kb on systems with a
>> page size of 4Kb).
>>
>> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
> I consider it doesn't work since this test is not for Btrfs.
> Please see the following code.
>
> tests/generic/017:
> ===
> for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>
> 	length=$(($BLOCKS * $BSIZE))
> 	case $FSTYP in
> 	xfs)
> 	_scratch_mkfs -b size=$BSIZE >> $seqres.full 2>&1
> 	;;
> 	ext4)
> 	_scratch_mkfs -b $BSIZE >> $seqres.full 2>&1
> 	;;
> 	esac
> 	_scratch_mount >> $seqres.full 2>&1
> ===
>
> There is no btrfs here.
>
> This test was moved to shared/005 to generic/017
> at 21723cdbf303e031d6429f67fec9768750a5db7d.
>
> Original supported fs is here.
> ===============================================================================
> supported_fs xfs ext4
> ===============================================================================
>
> I suspect that Lukas moved this test to generic/ by mistake or forgot to
> add "$FSTYP == btrfs" case.
>
> Thanks,
> Satoru
>
>> ---
>>    tests/generic/017 | 8 ++++++++
>>    1 file changed, 8 insertions(+)
>>
>> diff --git a/tests/generic/017 b/tests/generic/017
>> index 13b7254..6495be5 100755
>> --- a/tests/generic/017
>> +++ b/tests/generic/017
>> @@ -51,6 +51,14 @@ BLOCKS=10240
>>    
>>    for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>>    
>> +	# btrfs doesn't support block size smaller then page size
>> +	if [ "$FSTYP" == "btrfs" ]; then
>> +		if (( $BSIZE < `getconf PAGE_SIZE` )); then
>> +			echo "80"
>> +			continue
>> +		fi
>> +	fi
>> +
>>    	length=$(($BLOCKS * $BSIZE))
>>    	case $FSTYP in
>>    	xfs)
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
__________
Brendan Hide
http://swiftspirit.co.za/
http://www.webafrica.co.za/?AFF1E97

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Lukáš Czerner]

On Mon, 23 Jun 2014, Filipe David Borba Manana wrote:

> Date: Mon, 23 Jun 2014 11:28:00 +0100
> From: Filipe David Borba Manana <fdmanana@gmail.com>
> To: fstests@vger.kernel.org
> Cc: linux-btrfs@vger.kernel.org,
>     Filipe David Borba Manana <fdmanana@gmail.com>
> Subject: [PATCH] generic/017: skip invalid block sizes for btrfs
> 
> In btrfs the block size (called sector size in btrfs) can not be
> smaller then the page size. Therefore skip block sizes smaller
> then page size if the fs is btrfs, so that the test can succeed
> on btrfs (testing only with block sizes of 4kb on systems with a
> page size of 4Kb).

The test itself is wrong, it's trying to do _scratch_mkfs with
different block size, but the block size might already be specified
by the user (in fact it should be user responsibility to test
different block sizes). In the case that mkfs can not handle
multiple of the same option like mkfs.xfs for example it will fail,
but the test will go on with the original file system.

The test needs to be fixed to just test the file system with options
specified by the user. Also we should change _scratch_mkfs() to fail
the test if the mkfs failed (no one is actually testing mkfs_status
variable anyway.

Once we do that this patch will no longer be needed.

Thanks!
-Lukas

> 
> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
> ---
>  tests/generic/017 | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tests/generic/017 b/tests/generic/017
> index 13b7254..6495be5 100755
> --- a/tests/generic/017
> +++ b/tests/generic/017
> @@ -51,6 +51,14 @@ BLOCKS=10240
>  
>  for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>  
> +	# btrfs doesn't support block size smaller then page size
> +	if [ "$FSTYP" == "btrfs" ]; then
> +		if (( $BSIZE < `getconf PAGE_SIZE` )); then
> +			echo "80"
> +			continue
> +		fi
> +	fi
> +
>  	length=$(($BLOCKS * $BSIZE))
>  	case $FSTYP in
>  	xfs)
> 

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2884 bytes --]

On Mon, 23 Jun 2014, Lukáš Czerner wrote:

> Date: Mon, 23 Jun 2014 14:35:50 +0200 (CEST)
> From: Lukáš Czerner <lczerner@redhat.com>
> To: Filipe David Borba Manana <fdmanana@gmail.com>
> Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH] generic/017: skip invalid block sizes for btrfs
> 
> On Mon, 23 Jun 2014, Filipe David Borba Manana wrote:
> 
> > Date: Mon, 23 Jun 2014 11:28:00 +0100
> > From: Filipe David Borba Manana <fdmanana@gmail.com>
> > To: fstests@vger.kernel.org
> > Cc: linux-btrfs@vger.kernel.org,
> >     Filipe David Borba Manana <fdmanana@gmail.com>
> > Subject: [PATCH] generic/017: skip invalid block sizes for btrfs
> > 
> > In btrfs the block size (called sector size in btrfs) can not be
> > smaller then the page size. Therefore skip block sizes smaller
> > then page size if the fs is btrfs, so that the test can succeed
> > on btrfs (testing only with block sizes of 4kb on systems with a
> > page size of 4Kb).
> 
> The test itself is wrong, it's trying to do _scratch_mkfs with
> different block size, but the block size might already be specified
> by the user (in fact it should be user responsibility to test
> different block sizes). In the case that mkfs can not handle
> multiple of the same option like mkfs.xfs for example it will fail,
> but the test will go on with the original file system.
> 
> The test needs to be fixed to just test the file system with options
> specified by the user. Also we should change _scratch_mkfs() to fail
> the test if the mkfs failed (no one is actually testing mkfs_status
> variable anyway.

Correction, _scratch_mkfs_xfs() is actually testing mkfs_status and
will attempt to re-run mkfs only with provided options if it failed
before. But my point remains the same, block size to test should be
in users hands and we should run all tests with different block
sizes, if supported.

Thanks!
-Lukas

> 
> Once we do that this patch will no longer be needed.
> 
> Thanks!
> -Lukas
> 
> > 
> > Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
> > ---
> >  tests/generic/017 | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/tests/generic/017 b/tests/generic/017
> > index 13b7254..6495be5 100755
> > --- a/tests/generic/017
> > +++ b/tests/generic/017
> > @@ -51,6 +51,14 @@ BLOCKS=10240
> >  
> >  for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
> >  
> > +	# btrfs doesn't support block size smaller then page size
> > +	if [ "$FSTYP" == "btrfs" ]; then
> > +		if (( $BSIZE < `getconf PAGE_SIZE` )); then
> > +			echo "80"
> > +			continue
> > +		fi
> > +	fi
> > +
> >  	length=$(($BLOCKS * $BSIZE))
> >  	case $FSTYP in
> >  	xfs)
> > 
> --
> 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] generic/017: skip invalid block sizes for btrfs

[Filipe David Manana]

On Mon, Jun 23, 2014 at 3:09 PM, Lukáš Czerner <lczerner@redhat.com> wrote:
> On Mon, 23 Jun 2014, Lukáš Czerner wrote:
>
>> Date: Mon, 23 Jun 2014 14:35:50 +0200 (CEST)
>> From: Lukáš Czerner <lczerner@redhat.com>
>> To: Filipe David Borba Manana <fdmanana@gmail.com>
>> Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
>> Subject: Re: [PATCH] generic/017: skip invalid block sizes for btrfs
>>
>> On Mon, 23 Jun 2014, Filipe David Borba Manana wrote:
>>
>> > Date: Mon, 23 Jun 2014 11:28:00 +0100
>> > From: Filipe David Borba Manana <fdmanana@gmail.com>
>> > To: fstests@vger.kernel.org
>> > Cc: linux-btrfs@vger.kernel.org,
>> >     Filipe David Borba Manana <fdmanana@gmail.com>
>> > Subject: [PATCH] generic/017: skip invalid block sizes for btrfs
>> >
>> > In btrfs the block size (called sector size in btrfs) can not be
>> > smaller then the page size. Therefore skip block sizes smaller
>> > then page size if the fs is btrfs, so that the test can succeed
>> > on btrfs (testing only with block sizes of 4kb on systems with a
>> > page size of 4Kb).
>>
>> The test itself is wrong, it's trying to do _scratch_mkfs with
>> different block size, but the block size might already be specified
>> by the user (in fact it should be user responsibility to test
>> different block sizes). In the case that mkfs can not handle
>> multiple of the same option like mkfs.xfs for example it will fail,
>> but the test will go on with the original file system.
>>
>> The test needs to be fixed to just test the file system with options
>> specified by the user. Also we should change _scratch_mkfs() to fail
>> the test if the mkfs failed (no one is actually testing mkfs_status
>> variable anyway.
>
> Correction, _scratch_mkfs_xfs() is actually testing mkfs_status and
> will attempt to re-run mkfs only with provided options if it failed
> before. But my point remains the same, block size to test should be
> in users hands and we should run all tests with different block
> sizes, if supported.

Ok, so in other words, get rid of the block size loop and no more
specific mkfs calls for each fs type?

Thanks Lukas

>
> Thanks!
> -Lukas
>
>>
>> Once we do that this patch will no longer be needed.
>>
>> Thanks!
>> -Lukas
>>
>> >
>> > Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
>> > ---
>> >  tests/generic/017 | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/tests/generic/017 b/tests/generic/017
>> > index 13b7254..6495be5 100755
>> > --- a/tests/generic/017
>> > +++ b/tests/generic/017
>> > @@ -51,6 +51,14 @@ BLOCKS=10240
>> >
>> >  for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
>> >
>> > +   # btrfs doesn't support block size smaller then page size
>> > +   if [ "$FSTYP" == "btrfs" ]; then
>> > +           if (( $BSIZE < `getconf PAGE_SIZE` )); then
>> > +                   echo "80"
>> > +                   continue
>> > +           fi
>> > +   fi
>> > +
>> >     length=$(($BLOCKS * $BSIZE))
>> >     case $FSTYP in
>> >     xfs)
>> >
>> --
>> 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
>>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Dave Chinner]

On Mon, Jun 23, 2014 at 04:09:18PM +0200, Lukáš Czerner wrote:
> On Mon, 23 Jun 2014, Lukáš Czerner wrote:
> 
> > Date: Mon, 23 Jun 2014 14:35:50 +0200 (CEST)
> > From: Lukáš Czerner <lczerner@redhat.com>
> > To: Filipe David Borba Manana <fdmanana@gmail.com>
> > Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
> > Subject: Re: [PATCH] generic/017: skip invalid block sizes for btrfs
> > 
> > On Mon, 23 Jun 2014, Filipe David Borba Manana wrote:
> > 
> > > Date: Mon, 23 Jun 2014 11:28:00 +0100
> > > From: Filipe David Borba Manana <fdmanana@gmail.com>
> > > To: fstests@vger.kernel.org
> > > Cc: linux-btrfs@vger.kernel.org,
> > >     Filipe David Borba Manana <fdmanana@gmail.com>
> > > Subject: [PATCH] generic/017: skip invalid block sizes for btrfs
> > > 
> > > In btrfs the block size (called sector size in btrfs) can not be
> > > smaller then the page size. Therefore skip block sizes smaller
> > > then page size if the fs is btrfs, so that the test can succeed
> > > on btrfs (testing only with block sizes of 4kb on systems with a
> > > page size of 4Kb).
> > 
> > The test itself is wrong, it's trying to do _scratch_mkfs with
> > different block size, but the block size might already be specified
> > by the user (in fact it should be user responsibility to test
> > different block sizes). In the case that mkfs can not handle
> > multiple of the same option like mkfs.xfs for example it will fail,
> > but the test will go on with the original file system.
> > 
> > The test needs to be fixed to just test the file system with options
> > specified by the user. Also we should change _scratch_mkfs() to fail
> > the test if the mkfs failed (no one is actually testing mkfs_status
> > variable anyway.
> 
> Correction, _scratch_mkfs_xfs() is actually testing mkfs_status and
> will attempt to re-run mkfs only with provided options if it failed
> before. But my point remains the same, block size to test should be
> in users hands and we should run all tests with different block
> sizes, if supported.

Follow that line of reasoning to other options. If we take that
argument to it's logical conclusion, no test should be able to set
any mount or mkfs option because that's for the user to control.
This implies we can't even have quota specific tests because they
need to override the mount options (and perhaps mkfs options) the
user has specified to be properly tested.

Some tests only work on specific configurations and therefore they
need to be able to control the execution environment directly. Hence
the behaviour of _scratch_mkfs_xfs(), where it will *attempt* to use
the user provided options. However, if they conflict with what the
test requires it will drop the user options and use what the test
requires.

It's better that the test overrides user provided options than fail
due to incompatible configuration. It makes maintenance of the tests
much easier because we don't have to declare and maintain the
supported list of user options for every test. Either the test works
for all configurations (i.e. whatever the user sets in
MKFS/MOUNT_OPTIONS), or it specifically defines the configuration
it is testing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4105 bytes --]

On Tue, 24 Jun 2014, Dave Chinner wrote:

> Date: Tue, 24 Jun 2014 14:26:46 +1000
> From: Dave Chinner <david@fromorbit.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Filipe David Borba Manana <fdmanana@gmail.com>, fstests@vger.kernel.org,
>     linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH] generic/017: skip invalid block sizes for btrfs
> 
> On Mon, Jun 23, 2014 at 04:09:18PM +0200, Lukáš Czerner wrote:
> > On Mon, 23 Jun 2014, Lukáš Czerner wrote:
> > 
> > > Date: Mon, 23 Jun 2014 14:35:50 +0200 (CEST)
> > > From: Lukáš Czerner <lczerner@redhat.com>
> > > To: Filipe David Borba Manana <fdmanana@gmail.com>
> > > Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
> > > Subject: Re: [PATCH] generic/017: skip invalid block sizes for btrfs
> > > 
> > > On Mon, 23 Jun 2014, Filipe David Borba Manana wrote:
> > > 
> > > > Date: Mon, 23 Jun 2014 11:28:00 +0100
> > > > From: Filipe David Borba Manana <fdmanana@gmail.com>
> > > > To: fstests@vger.kernel.org
> > > > Cc: linux-btrfs@vger.kernel.org,
> > > >     Filipe David Borba Manana <fdmanana@gmail.com>
> > > > Subject: [PATCH] generic/017: skip invalid block sizes for btrfs
> > > > 
> > > > In btrfs the block size (called sector size in btrfs) can not be
> > > > smaller then the page size. Therefore skip block sizes smaller
> > > > then page size if the fs is btrfs, so that the test can succeed
> > > > on btrfs (testing only with block sizes of 4kb on systems with a
> > > > page size of 4Kb).
> > > 
> > > The test itself is wrong, it's trying to do _scratch_mkfs with
> > > different block size, but the block size might already be specified
> > > by the user (in fact it should be user responsibility to test
> > > different block sizes). In the case that mkfs can not handle
> > > multiple of the same option like mkfs.xfs for example it will fail,
> > > but the test will go on with the original file system.
> > > 
> > > The test needs to be fixed to just test the file system with options
> > > specified by the user. Also we should change _scratch_mkfs() to fail
> > > the test if the mkfs failed (no one is actually testing mkfs_status
> > > variable anyway.
> > 
> > Correction, _scratch_mkfs_xfs() is actually testing mkfs_status and
> > will attempt to re-run mkfs only with provided options if it failed
> > before. But my point remains the same, block size to test should be
> > in users hands and we should run all tests with different block
> > sizes, if supported.
> 
> Follow that line of reasoning to other options. If we take that
> argument to it's logical conclusion, no test should be able to set
> any mount or mkfs option because that's for the user to control.
> This implies we can't even have quota specific tests because they
> need to override the mount options (and perhaps mkfs options) the
> user has specified to be properly tested.

I acknowledge that there are tests that actually needs to set its
own options. That's usually in the nature of the test, but it's not
the case with this particular test. Here, it's not needed and in my
view it's wrong because it brings unnecessary limitations and
problems.

> 
> Some tests only work on specific configurations and therefore they
> need to be able to control the execution environment directly. Hence
> the behaviour of _scratch_mkfs_xfs(), where it will *attempt* to use
> the user provided options. However, if they conflict with what the
> test requires it will drop the user options and use what the test
> requires.
> 
> It's better that the test overrides user provided options than fail
> due to incompatible configuration. It makes maintenance of the tests
> much easier because we don't have to declare and maintain the
> supported list of user options for every test. Either the test works
> for all configurations (i.e. whatever the user sets in
> MKFS/MOUNT_OPTIONS), or it specifically defines the configuration
> it is testing.

I do agree with that, I do not think I've ever said otherwise. I was
addressing this particular test where the specific configuration is
not needed.

-Lukas

> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Zach Brown]

On Mon, Jun 23, 2014 at 11:28:00AM +0100, Filipe David Borba Manana wrote:
> In btrfs the block size (called sector size in btrfs) can not be
> smaller then the page size.

Nor larger.

commit 8d082fb727ac11930ea20bf1612e334ea7c2b697
Author: Liu Bo <liubo2009@cn.fujitsu.com>
Date:   Tue Apr 3 09:56:53 2012 +0800

    Btrfs: do not mount when we have a sectorsize unequal to PAGE_SIZE
    
    Our code is not ready to cope with a sectorsize that's not equal to PAGE_SIZE.
    It will lead to hanging-on while writing something.

- z

Re: [PATCH] generic/017: skip invalid block sizes for btrfs

[Roman Mamedov]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Mon, 23 Jun 2014 11:28:00 +0100
Filipe David Borba Manana <fdmanana@gmail.com> wrote:

> In btrfs the block size (called sector size in btrfs) can not be
> smaller then the page size.

Just in case anyone misses this, there is some work to address this limitation:
http://www.spinics.net/lists/linux-btrfs/msg34814.html

So perhaps keep in mind to roll-back the proposed patch after the above gets
finalized and merged.

-- 
With respect,
Roman

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]