Re: [PATCH v2] fstests: btrfs: Add regression test for reserved space leak.

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

Tsutomu Itoh wrote on 2015/08/05 11:52 +0900:
> On 2015/08/05 10:57, Dave Chinner wrote:
>> On Wed, Aug 05, 2015 at 09:39:35AM +0800, Qu Wenruo wrote:
>>> Tsutomu Itoh wrote on 2015/08/05 10:26 +0900:
>>>> On 2015/08/05 10:08, Qu Wenruo wrote:
>>>>> +# As the reserved space freeing happens at commit_transaction time,
>>>>> +# without a transaction commit, no reserved space needs freeing and
>>>>> +# won't trigger the bug.
>>>>> +sync
>>>>
>>>> Isn't '$BTRFS_UTIL_PROG filesystem sync' better instead of 'sync'?
>>>>
>>>> Thanks,
>>>> Tsutomu
>>> Hi, Tsutomu-san,
>>>
>>> Yes, I did use such method before, but Dave said it's better to use
>>> unified interface to sync a filesystem other than the specialized
>>> one.
>>>
>>> So I still use sync as Dave said.
>>
>> Mainly because "sync" is what users will use to make sure their data
>> is safe. filesystem specific tools have a habit of doing "special
>> stuff" to sync a filesystem, so it may not reflect the way users
>> expect the system to behaviour when they run sync.
>>
>> The other option is this:
>>
>> _syncfs()
>> {
>>     mntpt=$1
>>
>>     $XFS_IO_PROG -c syncfs $mntpt
>> }
>>
>> _sync_test()
>> {
>>     _syncfs $TEST_DIR
>> }
>>
>> _sync_scratch()
>> {
>>     _syncfs $SCRATCH_MNT
>> }
>>
>> which only runs sync on the filesystem that needs syncing (via the
>> syncfs() syscall)
>
> I think that syncfs is better instead of sync because the syncing of
> the specified filesystem is necessary.
>
> Thanks,
> Tsutomu
>
I'm OK changing the sync to _sync_scratch().

But no one in btrfs testcase uses that function and I didn't see the 
reason to specifically call _sync_scratch() instead of sync() BTW.

Thanks,
Qu

>>
>> Cheers,
>>
>> Dave.
>>
>
>