xfs
[Top] [All Lists]

Re: [PATCH 3/3] xfstests: fix wrong number of the required devices and a

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/3] xfstests: fix wrong number of the required devices and add independent device check for case 265
From: Miao Xie <miaox@xxxxxxxxxxxxxx>
Date: Fri, 24 Aug 2012 14:55:13 +0800
Cc: Linux Btrfs <linux-btrfs@xxxxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, anand.jain@xxxxxxxxxx
In-reply-to: <50371623.9040505@xxxxxxxxxxxxxx>
References: <5036F1FB.80205@xxxxxxxxxxxxxx> <20120824041804.GB19235@dastard> <50371623.9040505@xxxxxxxxxxxxxx>
Reply-to: miaox@xxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0
On fri, 24 Aug 2012 13:50:27 +0800, Miao Xie wrote:
> On fri, 24 Aug 2012 14:18:04 +1000, Dave Chinner wrote:
>> On Fri, Aug 24, 2012 at 11:16:11AM +0800, Miao Xie wrote:
>>> Case 265 need 4 devices to test RAID10, so we need 4 or more devices not 2.
>>> and it is better that these 4 devices are independent devices, especially
>>> the 2nd last one, so we add independent device check to check the devices
>>> in SCRATCH_DEV_POOL.
>>
>> I don't see any reason for requiring the devices to be independent.
> 
> README said we need independent devices. I think the reason is:
> Case 265 will remove/add the 2nd last device in SCRATCH_DEV_POOL, if this 
> device
> is a partition of a device, not a independent device, it is easy to make a 
> mistake
> for the users that the other partitions are used while doing the test. If so,
> the name of the device will be changed, and it will make the next test cases 
> fail.

I find all the partitions and the virtual devices don't have the delete 
entry-point
in sysfs, so we can avoid the above problem by checking the delete entry-point.

>> You're basically checking if the devices are on an MD device, which
>> isn't really a check for indpendent devices. e.g. my 4 devices could
>> be loopback devices with files all the in the same filesystem, or on
>> a VM using images at that are all hosted on the same device, or LVM
>> volumes on top of a single MD device, hardware lun, etc. They are
>> most certainly not independent, but your test won't pick up any of
>> them.
> 
> The check _require_deletable_scratch_dev_pool will make sure the device is not
> a virtual device. My check just make sure the devices are not partitions.
> Maybe I should change the name of the my check.
> 
> P.S. I made a mistake, I needn't take the soft raid into account because
> the soft raid devices are also virtual disks.
> 
>> Hence the test does not require the devices to be independent to run
>> correctly.  Sure, the test will run faster if each device is on an
>> independent spindle, but it's not a requirement for test success or
>> failure....

If the 2nd last device in SCRATCH_DEV_POOL is a partition, case 265 will
fail because case 265 is designed for independent device test and doesn't
take the partitions into account.

Thanks
Miao

>>
>>> diff --git a/common.rc b/common.rc
>>> index 602513a..ede25fe 100644
>>> --- a/common.rc
>>> +++ b/common.rc
>>> @@ -1699,12 +1699,14 @@ _require_scratch_dev_pool()
>>>             _notrun "this test requires a valid \$SCRATCH_DEV_POOL"
>>>     fi
>>>  
>>> -   # btrfs test case needs 2 or more scratch_dev_pool; other FS not sure
>>> +   # btrfs test case needs 4 or more scratch_dev_pool; other FS not sure
>>>     # so fail it
>>> +   # common.config has moved the first device to SCRATCH_DEV, so
>>> +   # SCRATCH_DEV_POOL should have 3 or more disks.
>>>     case $FSTYP in
>>>     btrfs)
>>> -           if [ "`echo $SCRATCH_DEV_POOL|wc -w`" -lt 2 ]; then
>>> -                   _notrun "btrfs and this test needs 2 or more disks in 
>>> SCRATCH_DEV_POOL"
>>> +           if [ "`echo $SCRATCH_DEV_POOL|wc -w`" -lt 3 ]; then
>>> +                   _notrun "btrfs and this test needs 4 or more disks in 
>>> SCRATCH_DEV_POOL"
>>>             fi
>>>     ;;
>>>     *)
>>
>> Rather than changing this every time a new number of disks is
>> required, change it so that the number of devices required by the
>> test is passed as a parameter to _require_scratch_dev_pool.
> 
> Yes, I'll update my patch.
> 
> Thanks
> Miao
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


<Prev in Thread] Current Thread [Next in Thread>