[PATCH] xfs/194: fix the exception when run on 4k sector drives

[PATCH] xfs/194: fix the exception when run on 4k sector drives

[Zorro Lang]

The below command in "Test 4":

    xfs_io -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512"

will run failed on 4k sector drives. So I use min_alignment size
to replace the hard-code 512.

Also I make sure the blksize won't less than min_alignment size,
after blksize=`expr $pgsize / 8`.

If blksize really less than min_alignment size, I set blksize =
min_alignment size, and for sure the consistency of test result,
I repair pgsize(already not real page size) number according to
the new blksize.

Because IRIX can't use _min_dio_alignment(), so remove it from
supported os list.

At last, make the crc flag be disabled only when blksize=512.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 tests/xfs/194 | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/tests/xfs/194 b/tests/xfs/194
index e11b459..4491dd4 100755
--- a/tests/xfs/194
+++ b/tests/xfs/194
@@ -42,7 +42,7 @@ _cleanup()
 
 # only xfs supported due to use of xfs_bmap
 _supported_fs xfs
-_supported_os IRIX Linux
+_supported_os Linux
 
 # real QA test starts here
 rm -f $seqres.full
@@ -50,6 +50,16 @@ rm -f $seqres.full
 # For this test we use block size = 1/8 page size
 pgsize=`$here/src/feature -s`
 blksize=`expr $pgsize / 8`
+secsize=`_min_dio_alignment $SCRATCH_DEV`
+
+# The minimal blksize can't less than sector size, So if
+# blksize < secsize, we should adjust blksize and pgsize number.
+# Of course, if we adjust pgsize, pgsize won't equal to the
+# real page size of system.
+if [ $blksize -lt $secsize ];then
+        blksize=$secsize
+        pgsize=`expr $blksize \* 8`
+fi
 
 # Filter out file mountpoint and physical location info
 # Input:
@@ -84,8 +94,13 @@ _require_scratch
 unset MKFS_OPTIONS
 unset XFS_MKFS_OPTIONS
 
-# we need 512 byte block size, so crc's are turned off
-_scratch_mkfs_xfs -m crc=0 -b size=$blksize >/dev/null 2>&1
+# If we use 512 byte block size, can't use crc
+if [ $blksize -eq 512 ];then
+        crcflag=0
+else
+        crcflag=1
+fi
+_scratch_mkfs_xfs -m crc=$crcflag -b size=$blksize >/dev/null 2>&1
 _scratch_mount
 
 # 512b block / 4k page example:
@@ -209,7 +224,7 @@ xfs_io \
 -c "truncate `expr $blksize / 2`" \
 -c "truncate `expr $blksize + 1`" \
 -c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \
--c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \
+-c "pwrite -S 0x33 -b $secsize `expr $blksize \* 2` $secsize" \
 -t -d -f $SCRATCH_MNT/testfile4 >> $seqres.full
 
 xfs_bmap -v $SCRATCH_MNT/testfile4 | _filter_bmap
-- 
1.9.3

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Dave Chinner]

On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
> The below command in "Test 4":
> 
>     xfs_io -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512"
> 
> will run failed on 4k sector drives. So I use min_alignment size
> to replace the hard-code 512.
> 
> Also I make sure the blksize won't less than min_alignment size,
> after blksize=`expr $pgsize / 8`.
> 
> If blksize really less than min_alignment size, I set blksize =
> min_alignment size, and for sure the consistency of test result,
> I repair pgsize(already not real page size) number according to
> the new blksize.
> 
> Because IRIX can't use _min_dio_alignment(), so remove it from
> supported os list.

Not true - that's what the 'feature -s' branch in
_min_dio_alignment() is supposed to be for. Just add another check
for "$HOSTOS" == "Linux"....


> At last, make the crc flag be disabled only when blksize=512.

That makes things unnecessarily complex. I'd prefer to leave it as
it is (i.e. with -m crc=0), as CRCs do not affect what is being
tested at all.

> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>  tests/xfs/194 | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/xfs/194 b/tests/xfs/194
> index e11b459..4491dd4 100755
> --- a/tests/xfs/194
> +++ b/tests/xfs/194
> @@ -42,7 +42,7 @@ _cleanup()
>  
>  # only xfs supported due to use of xfs_bmap
>  _supported_fs xfs
> -_supported_os IRIX Linux
> +_supported_os Linux
>  
>  # real QA test starts here
>  rm -f $seqres.full
> @@ -50,6 +50,16 @@ rm -f $seqres.full
>  # For this test we use block size = 1/8 page size
>  pgsize=`$here/src/feature -s`
>  blksize=`expr $pgsize / 8`
> +secsize=`_min_dio_alignment $SCRATCH_DEV`
> +
> +# The minimal blksize can't less than sector size, So if
> +# blksize < secsize, we should adjust blksize and pgsize number.
> +# Of course, if we adjust pgsize, pgsize won't equal to the
> +# real page size of system.
> +if [ $blksize -lt $secsize ];then
> +        blksize=$secsize
> +        pgsize=`expr $blksize \* 8`
> +fi

No, this is wrong. the page size stays fixed at the machine page
size. We are testing *sub-page block sizes* here and the sector size
must be <= page size. Increasing the "page size" to larger than the
machine page size does not make the kernel use larger page sizes.

IOWs, if you've got sector size = page size (e.g. 4k sector device)
then no matter what you say $pgsize is, the kernel will see a block
size = page size test.

This whole chunk of code can simply be replaced with:

blksize=`_min_dio_alignment $SCRATCH_DEV`

Because that's what we actually need to test...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Eric Sandeen]

On 8/18/15 5:28 PM, Dave Chinner wrote:
> On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
>> The below command in "Test 4":
>>
>>     xfs_io -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512"
>>
>> will run failed on 4k sector drives. So I use min_alignment size
>> to replace the hard-code 512.
>>
>> Also I make sure the blksize won't less than min_alignment size,
>> after blksize=`expr $pgsize / 8`.
>>
>> If blksize really less than min_alignment size, I set blksize =
>> min_alignment size, and for sure the consistency of test result,
>> I repair pgsize(already not real page size) number according to
>> the new blksize.
>>
>> Because IRIX can't use _min_dio_alignment(), so remove it from
>> supported os list.
> 
> Not true - that's what the 'feature -s' branch in
> _min_dio_alignment() is supposed to be for. Just add another check
> for "$HOSTOS" == "Linux"....
> 
> 
>> At last, make the crc flag be disabled only when blksize=512.
> 
> That makes things unnecessarily complex. I'd prefer to leave it as
> it is (i.e. with -m crc=0), as CRCs do not affect what is being
> tested at all.
> 
>> Signed-off-by: Zorro Lang <zlang@redhat.com>
>> ---
>>  tests/xfs/194 | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/xfs/194 b/tests/xfs/194
>> index e11b459..4491dd4 100755
>> --- a/tests/xfs/194
>> +++ b/tests/xfs/194
>> @@ -42,7 +42,7 @@ _cleanup()
>>  
>>  # only xfs supported due to use of xfs_bmap
>>  _supported_fs xfs
>> -_supported_os IRIX Linux
>> +_supported_os Linux
>>  
>>  # real QA test starts here
>>  rm -f $seqres.full
>> @@ -50,6 +50,16 @@ rm -f $seqres.full
>>  # For this test we use block size = 1/8 page size
>>  pgsize=`$here/src/feature -s`
>>  blksize=`expr $pgsize / 8`
>> +secsize=`_min_dio_alignment $SCRATCH_DEV`
>> +
>> +# The minimal blksize can't less than sector size, So if
>> +# blksize < secsize, we should adjust blksize and pgsize number.
>> +# Of course, if we adjust pgsize, pgsize won't equal to the
>> +# real page size of system.
>> +if [ $blksize -lt $secsize ];then
>> +        blksize=$secsize
>> +        pgsize=`expr $blksize \* 8`
>> +fi
> 
> No, this is wrong. the page size stays fixed at the machine page
> size. We are testing *sub-page block sizes* here and the sector size
> must be <= page size. Increasing the "page size" to larger than the
> machine page size does not make the kernel use larger page sizes.
> 
> IOWs, if you've got sector size = page size (e.g. 4k sector device)
> then no matter what you say $pgsize is, the kernel will see a block
> size = page size test.
> 
> This whole chunk of code can simply be replaced with:
> 
> blksize=`_min_dio_alignment $SCRATCH_DEV`
> 
> Because that's what we actually need to test...

That won't work either, because we could easily get 512 from that.

and then this test:

# Now try the same thing but write a sector in the middle of that hole
# If things go badly stale data will be exposed either side.
# This is most interesting for block size > 512 (page size > 4096)

# We *should* get:
# |1100|HHHH|33HH|HHHH|2222|----|----|----|

echo "== Test 4 =="
xfs_io \
-c "pwrite -S 0x11 -b $pgsize 0 $pgsize" \
-c "mmap -r 0 $blksize" -c "mread 0 $blksize" -c "munmap" \
-c "truncate `expr $blksize / 2`" \
-c "truncate `expr $blksize + 1`" \
-c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \
-c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \
-t -d -f $SCRATCH_MNT/testfile4 >> $seqres.full

will be impossible.

AFAICT everything works except for that explicit 512-byte IO.

All we really need here is a sub-block-size IO, but at least as
large as the logical sector size.

So we want sub-page-size blocks, an sub-block-sized IOS (here).

Can't be *that* tricky to work out the scaling for various pages
and sector sizes, I hope? :)  Famous last words!

-Eric

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Dave Chinner]

On Tue, Aug 18, 2015 at 05:33:05PM -0500, Eric Sandeen wrote:
> On 8/18/15 5:28 PM, Dave Chinner wrote:
> > On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
> >> @@ -50,6 +50,16 @@ rm -f $seqres.full
> >>  # For this test we use block size = 1/8 page size
> >>  pgsize=`$here/src/feature -s`
> >>  blksize=`expr $pgsize / 8`
> >> +secsize=`_min_dio_alignment $SCRATCH_DEV`
> >> +
> >> +# The minimal blksize can't less than sector size, So if
> >> +# blksize < secsize, we should adjust blksize and pgsize number.
> >> +# Of course, if we adjust pgsize, pgsize won't equal to the
> >> +# real page size of system.
> >> +if [ $blksize -lt $secsize ];then
> >> +        blksize=$secsize
> >> +        pgsize=`expr $blksize \* 8`
> >> +fi
> > 
> > No, this is wrong. the page size stays fixed at the machine page
> > size. We are testing *sub-page block sizes* here and the sector size
> > must be <= page size. Increasing the "page size" to larger than the
> > machine page size does not make the kernel use larger page sizes.
> > 
> > IOWs, if you've got sector size = page size (e.g. 4k sector device)
> > then no matter what you say $pgsize is, the kernel will see a block
> > size = page size test.
> > 
> > This whole chunk of code can simply be replaced with:
> > 
> > blksize=`_min_dio_alignment $SCRATCH_DEV`
> > 
> > Because that's what we actually need to test...
> 
> That won't work either, because we could easily get 512 from that.

If 'blockdev --getss $dev' returns 512, then the device supports 512
byte IOs and so it is fine to do 512 byte IOs in the test.

> and then this test:
> 
> # Now try the same thing but write a sector in the middle of that hole
> # If things go badly stale data will be exposed either side.
> # This is most interesting for block size > 512 (page size > 4096)
> 
> # We *should* get:
> # |1100|HHHH|33HH|HHHH|2222|----|----|----|
> 
> echo "== Test 4 =="
> xfs_io \
> -c "pwrite -S 0x11 -b $pgsize 0 $pgsize" \
> -c "mmap -r 0 $blksize" -c "mread 0 $blksize" -c "munmap" \
> -c "truncate `expr $blksize / 2`" \
> -c "truncate `expr $blksize + 1`" \
> -c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \
> -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \
> -t -d -f $SCRATCH_MNT/testfile4 >> $seqres.full
> 
> will be impossible.
> 
> AFAICT everything works except for that explicit 512-byte IO.

Right. That hard coded 512 needs to change to $blksize, because
blksize is now equal to the sector size. I thought this would be
obvious to the reader, so I didn't comment on it.

> All we really need here is a sub-block-size IO, but at least as
> large as the logical sector size.

Yes. See above.

> So we want sub-page-size blocks, an sub-block-sized IOS (here).

Yes. See above.

> Can't be *that* tricky to work out the scaling for various pages
> and sector sizes, I hope? :)  Famous last words!

It's not. See above. :)

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Eric Sandeen]

On 8/18/15 5:43 PM, Dave Chinner wrote:
> On Tue, Aug 18, 2015 at 05:33:05PM -0500, Eric Sandeen wrote:
>> On 8/18/15 5:28 PM, Dave Chinner wrote:
>>> On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
>>>> @@ -50,6 +50,16 @@ rm -f $seqres.full
>>>>  # For this test we use block size = 1/8 page size
>>>>  pgsize=`$here/src/feature -s`
>>>>  blksize=`expr $pgsize / 8`
>>>> +secsize=`_min_dio_alignment $SCRATCH_DEV`
>>>> +
>>>> +# The minimal blksize can't less than sector size, So if
>>>> +# blksize < secsize, we should adjust blksize and pgsize number.
>>>> +# Of course, if we adjust pgsize, pgsize won't equal to the
>>>> +# real page size of system.
>>>> +if [ $blksize -lt $secsize ];then
>>>> +        blksize=$secsize
>>>> +        pgsize=`expr $blksize \* 8`
>>>> +fi
>>>
>>> No, this is wrong. the page size stays fixed at the machine page
>>> size. We are testing *sub-page block sizes* here and the sector size
>>> must be <= page size. Increasing the "page size" to larger than the
>>> machine page size does not make the kernel use larger page sizes.
>>>
>>> IOWs, if you've got sector size = page size (e.g. 4k sector device)
>>> then no matter what you say $pgsize is, the kernel will see a block
>>> size = page size test.
>>>
>>> This whole chunk of code can simply be replaced with:
>>>
>>> blksize=`_min_dio_alignment $SCRATCH_DEV`
>>>
>>> Because that's what we actually need to test...
>>
>> That won't work either, because we could easily get 512 from that.
> 
> If 'blockdev --getss $dev' returns 512, then the device supports 512
> byte IOs and so it is fine to do 512 byte IOs in the test.
> 
>> and then this test:
>>
>> # Now try the same thing but write a sector in the middle of that hole
>> # If things go badly stale data will be exposed either side.
>> # This is most interesting for block size > 512 (page size > 4096)
>>
>> # We *should* get:
>> # |1100|HHHH|33HH|HHHH|2222|----|----|----|
>>
>> echo "== Test 4 =="
>> xfs_io \
>> -c "pwrite -S 0x11 -b $pgsize 0 $pgsize" \
>> -c "mmap -r 0 $blksize" -c "mread 0 $blksize" -c "munmap" \
>> -c "truncate `expr $blksize / 2`" \
>> -c "truncate `expr $blksize + 1`" \
>> -c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \
>> -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \
>> -t -d -f $SCRATCH_MNT/testfile4 >> $seqres.full
>>
>> will be impossible.
>>
>> AFAICT everything works except for that explicit 512-byte IO.
> 
> Right. That hard coded 512 needs to change to $blksize, because
> blksize is now equal to the sector size. I thought this would be
> obvious to the reader, so I didn't comment on it.

if that last IO is $blksize, and blocksize == sector size, then the
test won't be testing what it's designed to test here, i.e. a
sub-block direct IO write.

# We *should* get:
# |1100|HHHH|33HH|HHHH|2222|----|----|----|
             ^^
             this

-Eric

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Zirong Lang]

----- 原始邮件 -----
> 发件人: "Eric Sandeen" <sandeen@sandeen.net>
> 收件人: "Dave Chinner" <david@fromorbit.com>, "Eric Sandeen" <sandeen@redhat.com>
> 抄送: "Zorro Lang" <zlang@redhat.com>, fstests@vger.kernel.org
> 发送时间: 星期三, 2015年 8 月 19日 上午 7:03:45
> 主题: Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives
> 
> On 8/18/15 5:43 PM, Dave Chinner wrote:
> > On Tue, Aug 18, 2015 at 05:33:05PM -0500, Eric Sandeen wrote:
> >> On 8/18/15 5:28 PM, Dave Chinner wrote:
> >>> On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
> >>>> @@ -50,6 +50,16 @@ rm -f $seqres.full
> >>>>  # For this test we use block size = 1/8 page size
> >>>>  pgsize=`$here/src/feature -s`
> >>>>  blksize=`expr $pgsize / 8`
> >>>> +secsize=`_min_dio_alignment $SCRATCH_DEV`
> >>>> +
> >>>> +# The minimal blksize can't less than sector size, So if
> >>>> +# blksize < secsize, we should adjust blksize and pgsize number.
> >>>> +# Of course, if we adjust pgsize, pgsize won't equal to the
> >>>> +# real page size of system.
> >>>> +if [ $blksize -lt $secsize ];then
> >>>> +        blksize=$secsize
> >>>> +        pgsize=`expr $blksize \* 8`
> >>>> +fi
> >>>
> >>> No, this is wrong. the page size stays fixed at the machine page
> >>> size. We are testing *sub-page block sizes* here and the sector size
> >>> must be <= page size. Increasing the "page size" to larger than the
> >>> machine page size does not make the kernel use larger page sizes.
> >>>
> >>> IOWs, if you've got sector size = page size (e.g. 4k sector device)
> >>> then no matter what you say $pgsize is, the kernel will see a block
> >>> size = page size test.

Yes, I know I can't change page size. So I said "for sure the consistency
of test result, I repair pgsize(already not real page size) number according
to the new blksize." in git commit message.

Eric said in xfs/194 "Test mapping around/over holes for sub-page blocks",
If the sector size = page size, I have no idea how to make sub-page blocks.
So in this situation, I set pgsize=`expr $blksize \* 8` just for the test
output suit for 194.out.

Or we don't care pgsize, just set blksize=`_min_dio_alignment $SCRATCH_DEV`,
and then replace all $pgsize to `expr $blksize \* 8`. If page size > blksize,
that's good. If not, we can't do more things about that, so just make it run pass.

> >>>
> >>> This whole chunk of code can simply be replaced with:
> >>>
> >>> blksize=`_min_dio_alignment $SCRATCH_DEV`
> >>>
> >>> Because that's what we actually need to test...
> >>
> >> That won't work either, because we could easily get 512 from that.
> > 
> > If 'blockdev --getss $dev' returns 512, then the device supports 512
> > byte IOs and so it is fine to do 512 byte IOs in the test.
> > 
> >> and then this test:
> >>
> >> # Now try the same thing but write a sector in the middle of that hole
> >> # If things go badly stale data will be exposed either side.
> >> # This is most interesting for block size > 512 (page size > 4096)
> >>
> >> # We *should* get:
> >> # |1100|HHHH|33HH|HHHH|2222|----|----|----|
> >>
> >> echo "== Test 4 =="
> >> xfs_io \
> >> -c "pwrite -S 0x11 -b $pgsize 0 $pgsize" \
> >> -c "mmap -r 0 $blksize" -c "mread 0 $blksize" -c "munmap" \
> >> -c "truncate `expr $blksize / 2`" \
> >> -c "truncate `expr $blksize + 1`" \
> >> -c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \
> >> -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \
> >> -t -d -f $SCRATCH_MNT/testfile4 >> $seqres.full
> >>
> >> will be impossible.
> >>
> >> AFAICT everything works except for that explicit 512-byte IO.
> > 
> > Right. That hard coded 512 needs to change to $blksize, because
> > blksize is now equal to the sector size. I thought this would be
> > obvious to the reader, so I didn't comment on it.
> 
> if that last IO is $blksize, and blocksize == sector size, then the
> test won't be testing what it's designed to test here, i.e. a
> sub-block direct IO write.

What do you think about loop device? If page size(4k) = sector size(4k), how
about use loop device? I think it will be 512 sector size as default?

Thanks,
Zorro

> 
> # We *should* get:
> # |1100|HHHH|33HH|HHHH|2222|----|----|----|
>              ^^
>              this
> 
> -Eric
> 
> 

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Dave Chinner]

On Tue, Aug 18, 2015 at 06:03:45PM -0500, Eric Sandeen wrote:
> On 8/18/15 5:43 PM, Dave Chinner wrote:
> > On Tue, Aug 18, 2015 at 05:33:05PM -0500, Eric Sandeen wrote:
> >> On 8/18/15 5:28 PM, Dave Chinner wrote:
> >>> On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
> >>>> @@ -50,6 +50,16 @@ rm -f $seqres.full
> >>>>  # For this test we use block size = 1/8 page size
> >>>>  pgsize=`$here/src/feature -s`
> >>>>  blksize=`expr $pgsize / 8`
> >>>> +secsize=`_min_dio_alignment $SCRATCH_DEV`
> >>>> +
> >>>> +# The minimal blksize can't less than sector size, So if
> >>>> +# blksize < secsize, we should adjust blksize and pgsize number.
> >>>> +# Of course, if we adjust pgsize, pgsize won't equal to the
> >>>> +# real page size of system.
> >>>> +if [ $blksize -lt $secsize ];then
> >>>> +        blksize=$secsize
> >>>> +        pgsize=`expr $blksize \* 8`
> >>>> +fi
> >>>
> >>> No, this is wrong. the page size stays fixed at the machine page
> >>> size. We are testing *sub-page block sizes* here and the sector size
> >>> must be <= page size. Increasing the "page size" to larger than the
> >>> machine page size does not make the kernel use larger page sizes.
> >>>
> >>> IOWs, if you've got sector size = page size (e.g. 4k sector device)
> >>> then no matter what you say $pgsize is, the kernel will see a block
> >>> size = page size test.
> >>>
> >>> This whole chunk of code can simply be replaced with:
> >>>
> >>> blksize=`_min_dio_alignment $SCRATCH_DEV`
> >>>
> >>> Because that's what we actually need to test...
> >>
> >> That won't work either, because we could easily get 512 from that.
> > 
> > If 'blockdev --getss $dev' returns 512, then the device supports 512
> > byte IOs and so it is fine to do 512 byte IOs in the test.
> > 
> >> and then this test:
> >>
> >> # Now try the same thing but write a sector in the middle of that hole
> >> # If things go badly stale data will be exposed either side.
> >> # This is most interesting for block size > 512 (page size > 4096)
> >>
> >> # We *should* get:
> >> # |1100|HHHH|33HH|HHHH|2222|----|----|----|
> >>
> >> echo "== Test 4 =="
> >> xfs_io \
> >> -c "pwrite -S 0x11 -b $pgsize 0 $pgsize" \
> >> -c "mmap -r 0 $blksize" -c "mread 0 $blksize" -c "munmap" \
> >> -c "truncate `expr $blksize / 2`" \
> >> -c "truncate `expr $blksize + 1`" \
> >> -c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \
> >> -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \
> >> -t -d -f $SCRATCH_MNT/testfile4 >> $seqres.full
> >>
> >> will be impossible.
> >>
> >> AFAICT everything works except for that explicit 512-byte IO.
> > 
> > Right. That hard coded 512 needs to change to $blksize, because
> > blksize is now equal to the sector size. I thought this would be
> > obvious to the reader, so I didn't comment on it.
> 
> if that last IO is $blksize, and blocksize == sector size, then the
> test won't be testing what it's designed to test here, i.e. a
> sub-block direct IO write.

That's not what the test is exercising:

# Test mapping around/over holes for sub-page blocks

it's testing *sub-page block behaviour*, not sub-block direct IO.

> 
> # We *should* get:
> # |1100|HHHH|33HH|HHHH|2222|----|----|----|
>              ^^
>              this

That implies a sub-block sized direct IO, on a single page that has
8 blocks. On a 4k page size machine, that is impossible and so most
of the time we are not doing what the comment implies.

With a 4k page, 512 byte block size:

| xfs_io \
| -c "pwrite -S 0x11 -b $pgsize 0 $pgsize" \

Write an entire page (4k)

# |1111|1111|1111|1111|1111|1111|1111|1111|

| -c "mmap -r 0 $blksize" -c "mread 0 $blksize" -c "munmap" \

map the first block (0-511 bytes - one sector)

| -c "truncate `expr $blksize / 2`" \
| -c "truncate `expr $blksize + 1`" \

sub-block truncate down, sub-block truncate up, make sure page cache
is correctly zeroed.

# |1100|HHHH|----|----|----|----|----|----|

| -c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \

DIO write of a single block half way through the original page, make
sure page cache is flushed correctly before DIO.

# |1100|HHHH|HHHH|HHHH|2222|----|----|----|

FWIW, this write will fail on a 4k sector device on a 4k page size
platform, because the IO is not sector aligned, and is why the
original patch needed to multiply pgsize out to 8 * sector size....

| -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \

do a -minimum sized write- to the *3rd* block in the page.

# |1100|HHHH|3333|HHHH|2222|----|----|----|

And that matches the expected output. We do not get this output with
a block size that is anything other than pgsize / 8, regardless of
whether the last write is a sub-block DIO or not.

IOWs, this test assumes that there are at least 8 blocks to page
because to exercise the appropriate paths it needs a hole between
each region that is written.  4k sector/4k page means the kernel
cannot do sub-page block size operations, and the test does not
exercise the code paths we're expecting it to. Hence it may simply
be best to do this:

if [ $sector_size > $page_size / 8 ]; then
	_not_run "sector size too large for platform page size"
fi

and replace the hard coded 512 with $sector_size.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Eric Sandeen]

On 8/18/15 9:42 PM, Dave Chinner wrote:

> if [ $sector_size > $page_size / 8 ]; then
> 	_not_run "sector size too large for platform page size"
> fi
> 
> and replace the hard coded 512 with $sector_size.

I agree.

-Eric

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Zirong Lang]

----- 原始邮件 -----
> 发件人: "Dave Chinner" <david@fromorbit.com>
> 收件人: "Eric Sandeen" <sandeen@sandeen.net>
> 抄送: "Eric Sandeen" <sandeen@redhat.com>, "Zorro Lang" <zlang@redhat.com>, fstests@vger.kernel.org
> 发送时间: 星期三, 2015年 8 月 19日 上午 10:42:16
> 主题: Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives
> 
> On Tue, Aug 18, 2015 at 06:03:45PM -0500, Eric Sandeen wrote:
> > On 8/18/15 5:43 PM, Dave Chinner wrote:
> > > On Tue, Aug 18, 2015 at 05:33:05PM -0500, Eric Sandeen wrote:
> > >> On 8/18/15 5:28 PM, Dave Chinner wrote:
> > >>> On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
> > >>>> @@ -50,6 +50,16 @@ rm -f $seqres.full
> > >>>>  # For this test we use block size = 1/8 page size
> > >>>>  pgsize=`$here/src/feature -s`
> > >>>>  blksize=`expr $pgsize / 8`
> > >>>> +secsize=`_min_dio_alignment $SCRATCH_DEV`
> > >>>> +
> > >>>> +# The minimal blksize can't less than sector size, So if
> > >>>> +# blksize < secsize, we should adjust blksize and pgsize number.
> > >>>> +# Of course, if we adjust pgsize, pgsize won't equal to the
> > >>>> +# real page size of system.
> > >>>> +if [ $blksize -lt $secsize ];then
> > >>>> +        blksize=$secsize
> > >>>> +        pgsize=`expr $blksize \* 8`
> > >>>> +fi
> > >>>
> > >>> No, this is wrong. the page size stays fixed at the machine page
> > >>> size. We are testing *sub-page block sizes* here and the sector size
> > >>> must be <= page size. Increasing the "page size" to larger than the
> > >>> machine page size does not make the kernel use larger page sizes.
> > >>>
> > >>> IOWs, if you've got sector size = page size (e.g. 4k sector device)
> > >>> then no matter what you say $pgsize is, the kernel will see a block
> > >>> size = page size test.
> > >>>
> > >>> This whole chunk of code can simply be replaced with:
> > >>>
> > >>> blksize=`_min_dio_alignment $SCRATCH_DEV`
> > >>>
> > >>> Because that's what we actually need to test...
> > >>
> > >> That won't work either, because we could easily get 512 from that.
> > > 
> > > If 'blockdev --getss $dev' returns 512, then the device supports 512
> > > byte IOs and so it is fine to do 512 byte IOs in the test.
> > > 
> > >> and then this test:
> > >>
> > >> # Now try the same thing but write a sector in the middle of that hole
> > >> # If things go badly stale data will be exposed either side.
> > >> # This is most interesting for block size > 512 (page size > 4096)
> > >>
> > >> # We *should* get:
> > >> # |1100|HHHH|33HH|HHHH|2222|----|----|----|
> > >>
> > >> echo "== Test 4 =="
> > >> xfs_io \
> > >> -c "pwrite -S 0x11 -b $pgsize 0 $pgsize" \
> > >> -c "mmap -r 0 $blksize" -c "mread 0 $blksize" -c "munmap" \
> > >> -c "truncate `expr $blksize / 2`" \
> > >> -c "truncate `expr $blksize + 1`" \
> > >> -c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \
> > >> -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \
> > >> -t -d -f $SCRATCH_MNT/testfile4 >> $seqres.full
> > >>
> > >> will be impossible.
> > >>
> > >> AFAICT everything works except for that explicit 512-byte IO.
> > > 
> > > Right. That hard coded 512 needs to change to $blksize, because
> > > blksize is now equal to the sector size. I thought this would be
> > > obvious to the reader, so I didn't comment on it.
> > 
> > if that last IO is $blksize, and blocksize == sector size, then the
> > test won't be testing what it's designed to test here, i.e. a
> > sub-block direct IO write.
> 
> That's not what the test is exercising:
> 
> # Test mapping around/over holes for sub-page blocks
> 
> it's testing *sub-page block behaviour*, not sub-block direct IO.
> 
> > 
> > # We *should* get:
> > # |1100|HHHH|33HH|HHHH|2222|----|----|----|
> >              ^^
> >              this
> 
> That implies a sub-block sized direct IO, on a single page that has
> 8 blocks. On a 4k page size machine, that is impossible and so most
> of the time we are not doing what the comment implies.
> 
> With a 4k page, 512 byte block size:
> 
> | xfs_io \
> | -c "pwrite -S 0x11 -b $pgsize 0 $pgsize" \
> 
> Write an entire page (4k)
> 
> # |1111|1111|1111|1111|1111|1111|1111|1111|
> 
> | -c "mmap -r 0 $blksize" -c "mread 0 $blksize" -c "munmap" \
> 
> map the first block (0-511 bytes - one sector)
> 
> | -c "truncate `expr $blksize / 2`" \
> | -c "truncate `expr $blksize + 1`" \
> 
> sub-block truncate down, sub-block truncate up, make sure page cache
> is correctly zeroed.
> 
> # |1100|HHHH|----|----|----|----|----|----|
> 
> | -c "pwrite -S 0x22 -b $blksize `expr $pgsize / 2` $blksize" \
> 
> DIO write of a single block half way through the original page, make
> sure page cache is flushed correctly before DIO.
> 
> # |1100|HHHH|HHHH|HHHH|2222|----|----|----|
> 
> FWIW, this write will fail on a 4k sector device on a 4k page size
> platform, because the IO is not sector aligned, and is why the
> original patch needed to multiply pgsize out to 8 * sector size....
> 
> | -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512" \
> 
> do a -minimum sized write- to the *3rd* block in the page.
> 
> # |1100|HHHH|3333|HHHH|2222|----|----|----|

Yes, that's true. if sector size and block size all 512b, we will get this.

In my test machine(64k page size, and 4k sector size), this case will
mkfs with blksize=8k, and we can get:

|1100|HHHH|33HH|HHHH|2222|----|----|----| (one |----| means 8k block size at here:)

> 
> And that matches the expected output. We do not get this output with
> a block size that is anything other than pgsize / 8, regardless of
> whether the last write is a sub-block DIO or not.
> 
> IOWs, this test assumes that there are at least 8 blocks to page
> because to exercise the appropriate paths it needs a hole between
> each region that is written.  4k sector/4k page means the kernel
> cannot do sub-page block size operations, and the test does not
> exercise the code paths we're expecting it to. Hence it may simply
> be best to do this:
> 
> if [ $sector_size > $page_size / 8 ]; then
> 	_not_run "sector size too large for platform page size"
> fi

If so, that'll be simple. I will send V2 patch for review;)

Thanks,
Zorro

> 
> and replace the hard coded 512 with $sector_size.
> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Zirong Lang]

----- 原始邮件 -----
> 发件人: "Dave Chinner" <david@fromorbit.com>
> 收件人: "Zorro Lang" <zlang@redhat.com>
> 抄送: fstests@vger.kernel.org, sandeen@redhat.com
> 发送时间: 星期三, 2015年 8 月 19日 上午 6:28:32
> 主题: Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives
> 
> On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
> > The below command in "Test 4":
> > 
> >     xfs_io -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512"
> > 
> > will run failed on 4k sector drives. So I use min_alignment size
> > to replace the hard-code 512.
> > 
> > Also I make sure the blksize won't less than min_alignment size,
> > after blksize=`expr $pgsize / 8`.
> > 
> > If blksize really less than min_alignment size, I set blksize =
> > min_alignment size, and for sure the consistency of test result,
> > I repair pgsize(already not real page size) number according to
> > the new blksize.
> > 
> > Because IRIX can't use _min_dio_alignment(), so remove it from
> > supported os list.
> 
> Not true - that's what the 'feature -s' branch in
> _min_dio_alignment() is supposed to be for. Just add another check
> for "$HOSTOS" == "Linux"....

Do you mean change _min_dio_alignment() to:
_min_dio_alignment()
{
    dev=$1

    if [ -b "$dev" -a $HOSTOS" == "Linux" ]; then
        blockdev --getss $dev
    else
        $here/src/feature -s
    fi
}

I really don't understand why page size will be the minimum dio alignment?
Do you mean in other OS(except linux), the sector size = page size?

Thanks,
Zorro


> 
> 
> > At last, make the crc flag be disabled only when blksize=512.
> 
> That makes things unnecessarily complex. I'd prefer to leave it as
> it is (i.e. with -m crc=0), as CRCs do not affect what is being
> tested at all.

Yes, you're right:)

> 
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> >  tests/xfs/194 | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/xfs/194 b/tests/xfs/194
> > index e11b459..4491dd4 100755
> > --- a/tests/xfs/194
> > +++ b/tests/xfs/194
> > @@ -42,7 +42,7 @@ _cleanup()
> >  
> >  # only xfs supported due to use of xfs_bmap
> >  _supported_fs xfs
> > -_supported_os IRIX Linux
> > +_supported_os Linux
> >  
> >  # real QA test starts here
> >  rm -f $seqres.full
> > @@ -50,6 +50,16 @@ rm -f $seqres.full
> >  # For this test we use block size = 1/8 page size
> >  pgsize=`$here/src/feature -s`
> >  blksize=`expr $pgsize / 8`
> > +secsize=`_min_dio_alignment $SCRATCH_DEV`
> > +
> > +# The minimal blksize can't less than sector size, So if
> > +# blksize < secsize, we should adjust blksize and pgsize number.
> > +# Of course, if we adjust pgsize, pgsize won't equal to the
> > +# real page size of system.
> > +if [ $blksize -lt $secsize ];then
> > +        blksize=$secsize
> > +        pgsize=`expr $blksize \* 8`
> > +fi
> 
> No, this is wrong. the page size stays fixed at the machine page
> size. We are testing *sub-page block sizes* here and the sector size
> must be <= page size. Increasing the "page size" to larger than the
> machine page size does not make the kernel use larger page sizes.
> 
> IOWs, if you've got sector size = page size (e.g. 4k sector device)
> then no matter what you say $pgsize is, the kernel will see a block
> size = page size test.
> 
> This whole chunk of code can simply be replaced with:
> 
> blksize=`_min_dio_alignment $SCRATCH_DEV`
> 
> Because that's what we actually need to test...
> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives

[Dave Chinner]

On Tue, Aug 18, 2015 at 10:24:59PM -0400, Zirong Lang wrote:
> 
> 
> ----- 原始邮件 -----
> > 发件人: "Dave Chinner" <david@fromorbit.com>
> > 收件人: "Zorro Lang" <zlang@redhat.com>
> > 抄送: fstests@vger.kernel.org, sandeen@redhat.com
> > 发送时间: 星期三, 2015年 8 月 19日 上午 6:28:32
> > 主题: Re: [PATCH] xfs/194: fix the exception when run on 4k sector drives
> > 
> > On Wed, Aug 19, 2015 at 01:21:51AM +0800, Zorro Lang wrote:
> > > The below command in "Test 4":
> > > 
> > >     xfs_io -c "pwrite -S 0x33 -b 512 `expr $blksize \* 2` 512"
> > > 
> > > will run failed on 4k sector drives. So I use min_alignment size
> > > to replace the hard-code 512.
> > > 
> > > Also I make sure the blksize won't less than min_alignment size,
> > > after blksize=`expr $pgsize / 8`.
> > > 
> > > If blksize really less than min_alignment size, I set blksize =
> > > min_alignment size, and for sure the consistency of test result,
> > > I repair pgsize(already not real page size) number according to
> > > the new blksize.
> > > 
> > > Because IRIX can't use _min_dio_alignment(), so remove it from
> > > supported os list.
> > 
> > Not true - that's what the 'feature -s' branch in
> > _min_dio_alignment() is supposed to be for. Just add another check
> > for "$HOSTOS" == "Linux"....
> 
> Do you mean change _min_dio_alignment() to:
> _min_dio_alignment()
> {
>     dev=$1
> 
>     if [ -b "$dev" -a $HOSTOS" == "Linux" ]; then
>         blockdev --getss $dev
>     else
>         $here/src/feature -s
>     fi
> }

Yes.

> I really don't understand why page size will be the minimum dio alignment?
> Do you mean in other OS(except linux), the sector size = page size?

No. It means that on other platforms the page size will be used as
alignment restrictions. If those platforms need anything different,
then they can add a similar 'elif [ "$HOSTOS" == "foo" ];' branch in
there to call the appropriate function. You don't need to worry
about that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com