On Wed, Jan 3, 2018 at 4:25 PM, Chengguang Xu <cgxu519@xxxxxxxxxx> wrote:
>>
>> 在 2018年1月3日，下午8:58，Amir Goldstein <amir73il@xxxxxxxxx> 写道：
>>
>> On Wed, Jan 3, 2018 at 2:44 PM, Chengguang Xu <cgxu519@xxxxxxxxxx> wrote:
>>>
>> [...]
>>> In order to add overlay support in some requirement checks like _require_metadata_journaling,
>>> I think it’s better save underlying filesystem type to $OVL_BASE_FSTYP and doing proper check based
>>> on it.
>>>
>>> Currently $OVL_BASE_FSTYP and $FSTYP all set to “overlay”, Is there any specific reason for it?
>>>
>>
>> In _overlay_config_override():
>> # Config file may specify base fs type, but we obay -overlay flag
>> export OVL_BASE_FSTYP="$FSTYP"
>> export FSTYP=overlay
>>
>> So either your setup is wrong or there is a bug.
>> $OVL_BASE_FSTYP *should* contain the base fs type,
>> but only if you defined FSTYPE in your config file when running ./check -overlay
>
> Yeah, I didn’t specify by hand in config file and just supposed to detect automatically.
> I didn’t check very carefully but seems just slight modification like below could let it
> support auto detection.
>
Sorry, I do not wish invest time to review this change because:
1. I don't see the need to support auto detect of base fs type (feel
free to explain)
2. I ran a lot of test to sanitize the new overlay config option with many
configurations and this adds a lot more variants. I you wish to push this
forward and have a good claim for the need, please specify which tests
you ran to sanitize your change before requesting review.
If you wish to contribute to the xfstests overlay infrastructure there
are other features
that would bring more gain IMO, for example:
- Support mkfs and MKFS_OPTIONS for base fs
- Try to increase the number of generic tests that can be run with
basefs+overlay
similar to your effort with the shutdown group tests
Thanks!
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
