[GIT PULL] Qgroup rework with other Fujitsu fix.

[GIT PULL] Qgroup rework with other Fujitsu fix.

[Qu Wenruo]

Hi Chris,

Please pull the 19 patchset from my branch for_chris_4.2.
We have tested it in a week.

Although it is originally based on 4.1-rc5, not the integration branch.
Quick tests shows no new bugs, although we will rerun the full test,
I'll send the patchset first for your reviewing:

https://github.com/adam900710/linux.git for_chris_4.2

This contains the following patches.

1. Qgroup rework (first 18 commits)
These commits rework the qgroup framework.
Now, quota won't need to do per-delayed-ref accounting.
But only need to record dirty delayed-ref, and account quota at 
transaction time.

The good thing is, at transaction time, we have no other interruption or
concurrency, account can be quite accurate and only need to account once
for every dirty extent.(especially faster for shared extents)

And clearer codes and logic. Codes changes from 1K to 0.5K, even a lot
of comments are added.

With the patchset, btrfs can pass all qgroup test in fstests.
No longer minus number now.

The only problem left is, we need a new mechanism to account subvolume 
deletion. But this is the long-existing problem, I'd prefer to address
it in next merge windows if we have a pretty method to solve it.

Or maybe a small patch to mark qgroup inconsistent when delete subvolume 
with level higher than 0.

2. write-rm-loop fixes from Zhao Lei.
Other patches from Zhao Lei and Forrest Liu have already been merged
into mainline, but this is the one still unmerged.

This patch fixes the last super rare problem we found in write-rm-loop
case.
And the patch will only modify the minor routine, so it won't affect the 
normal routine.

Thanks,
Qu

Re: [GIT PULL] Qgroup rework with other Fujitsu fix.

[Wang Shilong]

Hi Qu,

> Hi Chris,
> 
> Please pull the 19 patchset from my branch for_chris_4.2.
> We have tested it in a week.
> 
> Although it is originally based on 4.1-rc5, not the integration branch.
> Quick tests shows no new bugs, although we will rerun the full test,
> I'll send the patchset first for your reviewing:
> 
> https://github.com/adam900710/linux.git for_chris_4.2
> 
> This contains the following patches.
> 
> 1. Qgroup rework (first 18 commits)
> These commits rework the qgroup framework.
> Now, quota won't need to do per-delayed-ref accounting.
> But only need to record dirty delayed-ref, and account quota at transaction time.

Can you share perfomaces results with/without patches with quota enabled.
Especially, if there are thounds of snapshots, how much performaces down or up
with these patches applied.

Regards,
Wang Shilong

> 
> The good thing is, at transaction time, we have no other interruption or
> concurrency, account can be quite accurate and only need to account once
> for every dirty extent.(especially faster for shared extents)
> 
> And clearer codes and logic. Codes changes from 1K to 0.5K, even a lot
> of comments are added.
> 
> With the patchset, btrfs can pass all qgroup test in fstests.
> No longer minus number now.
> 
> The only problem left is, we need a new mechanism to account subvolume deletion. But this is the long-existing problem, I'd prefer to address
> it in next merge windows if we have a pretty method to solve it.
> 
> Or maybe a small patch to mark qgroup inconsistent when delete subvolume with level higher than 0.
> 
> 2. write-rm-loop fixes from Zhao Lei.
> Other patches from Zhao Lei and Forrest Liu have already been merged
> into mainline, but this is the one still unmerged.
> 
> This patch fixes the last super rare problem we found in write-rm-loop
> case.
> And the patch will only modify the minor routine, so it won't affect the normal routine.
> 
> Thanks,
> Qu
> -- 
> 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

Re: [GIT PULL] Qgroup rework with other Fujitsu fix.

[Qu Wenruo]

> Hi Qu,
>
>> Hi Chris,
>>
>> Please pull the 19 patchset from my branch for_chris_4.2.
>> We have tested it in a week.
>>
>> Although it is originally based on 4.1-rc5, not the integration branch.
>> Quick tests shows no new bugs, although we will rerun the full test,
>> I'll send the patchset first for your reviewing:
>>
>> https://github.com/adam900710/linux.git for_chris_4.2
>>
>> This contains the following patches.
>>
>> 1. Qgroup rework (first 18 commits)
>> These commits rework the qgroup framework.
>> Now, quota won't need to do per-delayed-ref accounting.
>> But only need to record dirty delayed-ref, and account quota at transaction time.
>
> Can you share perfomaces results with/without patches with quota enabled.
> Especially, if there are thounds of snapshots, how much performaces down or up
> with these patches applied.
>
> Regards,
> Wang Shilong
For multi-snapshot case, the test is still running and I'll post the
result ASAP.

But we already have the performance(sysbench) result between quota
disabled and quota enabled with the pull.

Note: we have already done tests on the pull branch, and it shows no
performance regression compared to 4.1-rc5 without quota.
So the result without quota should be a correct baseline.

Overall, the performance drop is below 5% and I think it's completely
acceptable for the complex of btrfs quota compared to other filesystems.

Only one tests shows a performance drop over 5%:
1. Random read with DIO on small files, single thread, 4K block size.

The drop is about 11%.

And some result may not be stable enough, we will double check but it 
needs a lot of time to do performance test.

For the full result, please refer to the google docs URL:
https://docs.google.com/spreadsheets/d/1m5c96PrxigtLl_m5OlMdrTUxr82CxJuOGDbjtDrv_I8/edit?usp=sharing

Thanks,
Qu

>
>>
>> The good thing is, at transaction time, we have no other interruption or
>> concurrency, account can be quite accurate and only need to account once
>> for every dirty extent.(especially faster for shared extents)
>>
>> And clearer codes and logic. Codes changes from 1K to 0.5K, even a lot
>> of comments are added.
>>
>> With the patchset, btrfs can pass all qgroup test in fstests.
>> No longer minus number now.
>>
>> The only problem left is, we need a new mechanism to account subvolume deletion. But this is the long-existing problem, I'd prefer to address
>> it in next merge windows if we have a pretty method to solve it.
>>
>> Or maybe a small patch to mark qgroup inconsistent when delete subvolume with level higher than 0.
>>
>> 2. write-rm-loop fixes from Zhao Lei.
>> Other patches from Zhao Lei and Forrest Liu have already been merged
>> into mainline, but this is the one still unmerged.
>>
>> This patch fixes the last super rare problem we found in write-rm-loop
>> case.
>> And the patch will only modify the minor routine, so it won't affect the normal routine.
>>
>> Thanks,
>> Qu
>> --
>> 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

Re: [GIT PULL] Qgroup rework with other Fujitsu fix.

[Qu Wenruo]

Wang Shilong wrote on 2015/06/09 14:03 +0800:
> Hi Qu,
>
>> Hi Chris,
>>
>> Please pull the 19 patchset from my branch for_chris_4.2.
>> We have tested it in a week.
>>
>> Although it is originally based on 4.1-rc5, not the integration branch.
>> Quick tests shows no new bugs, although we will rerun the full test,
>> I'll send the patchset first for your reviewing:
>>
>> https://github.com/adam900710/linux.git for_chris_4.2
>>
>> This contains the following patches.
>>
>> 1. Qgroup rework (first 18 commits)
>> These commits rework the qgroup framework.
>> Now, quota won't need to do per-delayed-ref accounting.
>> But only need to record dirty delayed-ref, and account quota at transaction time.
>
> Can you share perfomaces results with/without patches with quota enabled.
> Especially, if there are thounds of snapshots, how much performaces down or up
> with these patches applied.
Our test is done but the test result doesn't show much improvement nor 
regression on our multi subvolume test.

Since there is not such indicated performance test for multi snapshot 
case, we use the following script body to generate such environment.
---
btrfs subvolume create $MNT/orig_subv
for i in `seq 1 50`; do
	$FSSTRESS -s $i -n 500 -w -d $MNT/orig_subv
	btrfs subvolume snapshot $MNT/orig_subv $MNT/snap_$i
done
---

And then do sysbench in snap_25.

The result can be get from the following URL:
https://drive.google.com/file/d/0BxpkL3ehzX3pVElpTjAtTE52Rm8/view?usp=sharing

The above result maybe not accurate since we don't have enough time to 
run them several times, and we reuse the data from our internal 4.0-rc1 
result.
So the changes from 4.0-rc1 to the integration branch may also have some 
impact.
But should gives a overall view on performance change, which is on par 
with previous kernel.

Although I'm pretty sure for muliti snapshot case number of calling 
btrfs_find_all_roots() can be greatly reduced with the patchset, but it 
seems that's not the hotspot....

So this patchset is better considered as a bug fix. :(

Thanks,
Qu
>
> Regards,
> Wang Shilong
>
>>
>> The good thing is, at transaction time, we have no other interruption or
>> concurrency, account can be quite accurate and only need to account once
>> for every dirty extent.(especially faster for shared extents)
>>
>> And clearer codes and logic. Codes changes from 1K to 0.5K, even a lot
>> of comments are added.
>>
>> With the patchset, btrfs can pass all qgroup test in fstests.
>> No longer minus number now.
>>
>> The only problem left is, we need a new mechanism to account subvolume deletion. But this is the long-existing problem, I'd prefer to address
>> it in next merge windows if we have a pretty method to solve it.
>>
>> Or maybe a small patch to mark qgroup inconsistent when delete subvolume with level higher than 0.
>>
>> 2. write-rm-loop fixes from Zhao Lei.
>> Other patches from Zhao Lei and Forrest Liu have already been merged
>> into mainline, but this is the one still unmerged.
>>
>> This patch fixes the last super rare problem we found in write-rm-loop
>> case.
>> And the patch will only modify the minor routine, so it won't affect the normal routine.
>>
>> Thanks,
>> Qu
>> --
>> 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

Re: [GIT PULL] Qgroup rework with other Fujitsu fix.

[Josef Bacik]

On 06/07/2015 11:06 PM, Qu Wenruo wrote:
> Hi Chris,
>
> Please pull the 19 patchset from my branch for_chris_4.2.
> We have tested it in a week.
>
> Although it is originally based on 4.1-rc5, not the integration branch.
> Quick tests shows no new bugs, although we will rerun the full test,
> I'll send the patchset first for your reviewing:
>
> https://github.com/adam900710/linux.git for_chris_4.2
>
> This contains the following patches.
>
> 1. Qgroup rework (first 18 commits)
> These commits rework the qgroup framework.
> Now, quota won't need to do per-delayed-ref accounting.
> But only need to record dirty delayed-ref, and account quota at
> transaction time.
>
> The good thing is, at transaction time, we have no other interruption or
> concurrency, account can be quite accurate and only need to account once
> for every dirty extent.(especially faster for shared extents)
>
> And clearer codes and logic. Codes changes from 1K to 0.5K, even a lot
> of comments are added.
>
> With the patchset, btrfs can pass all qgroup test in fstests.
> No longer minus number now.
>
> The only problem left is, we need a new mechanism to account subvolume
> deletion. But this is the long-existing problem, I'd prefer to address
> it in next merge windows if we have a pretty method to solve it.
>
> Or maybe a small patch to mark qgroup inconsistent when delete subvolume
> with level higher than 0.
>
> 2. write-rm-loop fixes from Zhao Lei.
> Other patches from Zhao Lei and Forrest Liu have already been merged
> into mainline, but this is the one still unmerged.
>
> This patch fixes the last super rare problem we found in write-rm-loop
> case.
> And the patch will only modify the minor routine, so it won't affect the
> normal routine.
>

btrfs: delayed-ref: Use list to replace the ref_root in ref_head.

The merge is there for when we can't merge refs because somebody did a 
backref search and jacked up the sequence number, this will completely 
break the backref searching code since now you are allowing merge's of 
delayed refs that have different sequence numbers.  Thanks,

Josef

Re: [GIT PULL] Qgroup rework with other Fujitsu fix.

[Josef Bacik]

On 06/09/2015 10:23 AM, adam900710 wrote:
> Sorry, forgot to reply to all.
>
> To Josef,
>
> I am afraid that your conclusion is not right, as in the patch, it will
> only merge delayed refs when they are in same sequence number.
>
> In fact, in the patch, conditions for merge is even more restrict than
> before, it will not allow merging delayed refs for the same extent if
> there are other delayed refs between them.
>

You're right, I missed this bit

+	if (exist->type != ref->type || exist->no_quota != ref->no_quota ||
+	    exist->seq != ref->seq)
+		goto add_tail;

I don't want the merging more strict, I want to merge as much as 
possible, this was the problem with the original qgroup work and that's 
why I reworked all this sequence number crap.  Thanks,

Josef

Re: [GIT PULL] Qgroup rework with other Fujitsu fix.

[Qu Wenruo]

-------- Original Message  --------
Subject: Re: [GIT PULL] Qgroup rework with other Fujitsu fix.
From: Josef Bacik <jbacik@fb.com>
To: adam900710 <adam900710@gmail.com>
Date: 2015年06月09日 22:54

> On 06/09/2015 10:23 AM, adam900710 wrote:
>> Sorry, forgot to reply to all.
>>
>> To Josef,
>>
>> I am afraid that your conclusion is not right, as in the patch, it will
>> only merge delayed refs when they are in same sequence number.
>>
>> In fact, in the patch, conditions for merge is even more restrict than
>> before, it will not allow merging delayed refs for the same extent if
>> there are other delayed refs between them.
>>
>
> You're right, I missed this bit
>
> +    if (exist->type != ref->type || exist->no_quota != ref->no_quota ||
> +        exist->seq != ref->seq)
> +        goto add_tail;
>
> I don't want the merging more strict, I want to merge as much as
> possible, this was the problem with the original qgroup work and that's
> why I reworked all this sequence number crap.  Thanks,
>
> Josef
The old problem of qgroup is addressed by later patches, which reworks
the whole qgroup framework again.
So qgroup will not be a problem.

This patch is mainly used to cleanup the delayed-ref codes, at a little
cost of restrict merging condition, but delayed-refs will still be
mergeable unlike the very codes before your rework.

It's possible not to include this patch in this pull, but I still prefer
to include this one.
As old implements is not so clear and easy to cause bug, the new
straight-forward data structure with less codes will definitely make
sense for maintaining.

Thanks,
Qu

Re: [GIT PULL] Qgroup rework with other Fujitsu fix.

[Qu Wenruo]

-------- Original Message  --------
Subject: Re: [GIT PULL] Qgroup rework with other Fujitsu fix.
From: Josef Bacik <jbacik@fb.com>
To: adam900710 <adam900710@gmail.com>
Date: 2015年06月09日 22:54

> On 06/09/2015 10:23 AM, adam900710 wrote:
>> Sorry, forgot to reply to all.
>>
>> To Josef,
>>
>> I am afraid that your conclusion is not right, as in the patch, it will
>> only merge delayed refs when they are in same sequence number.
>>
>> In fact, in the patch, conditions for merge is even more restrict than
>> before, it will not allow merging delayed refs for the same extent if
>> there are other delayed refs between them.
>>
>
> You're right, I missed this bit
>
> +    if (exist->type != ref->type || exist->no_quota != ref->no_quota ||
> +        exist->seq != ref->seq)
> +        goto add_tail;
>
> I don't want the merging more strict, I want to merge as much as
> possible, this was the problem with the original qgroup work and that's
> why I reworked all this sequence number crap.  Thanks,
>
> Josef
The old problem of qgroup is addressed by later patches, which reworks
the whole qgroup framework again.
So qgroup will not be a problem.

This patch is mainly used to cleanup the delayed-ref codes, at a little
cost of restrict merging condition, but delayed-refs will still be
mergeable unlike the very codes before your rework.

It's possible not to include this patch in this pull, but I still prefer
to include this one.
As old implements is not so clear and easy to cause bug, the new
straight-forward data structure with less codes will definitely make
sense for maintaining.

Thanks,
Qu

Re: [GIT PULL] Qgroup rework with other Fujitsu fix.

[Chris Mason]

On 06/07/2015 11:06 PM, Qu Wenruo wrote:
> Hi Chris,
> 
> Please pull the 19 patchset from my branch for_chris_4.2.
> We have tested it in a week.
> 
> Although it is originally based on 4.1-rc5, not the integration branch.
> Quick tests shows no new bugs, although we will rerun the full test,
> I'll send the patchset first for your reviewing:
> 
> https://github.com/adam900710/linux.git for_chris_4.2
> 
> This contains the following patches.
> 
> 1. Qgroup rework (first 18 commits)
> These commits rework the qgroup framework.
> Now, quota won't need to do per-delayed-ref accounting.
> But only need to record dirty delayed-ref, and account quota at
> transaction time.
> 
> The good thing is, at transaction time, we have no other interruption or
> concurrency, account can be quite accurate and only need to account once
> for every dirty extent.(especially faster for shared extents)
> 
> And clearer codes and logic. Codes changes from 1K to 0.5K, even a lot
> of comments are added.
> 

Thanks for preparing all of this! I especially love the comments added.

> With the patchset, btrfs can pass all qgroup test in fstests.
> No longer minus number now.

And the passing xfstests ;)

While I review and test these, I have them at the top of the
integration-4.2 branch.  integration-4.2 may get rebased if I have to
take these out, but I'm trying to use the extra time (Linus plans on an
rc8) to test them as much as possible.

-chris