On 07/11/2017 09:15 AM, David Sterba wrote:
> On Wed, Jun 28, 2017 at 09:57:00PM -0600, Edmund Nadolski wrote:
>> It's been known for a while that the use of multiple lists
>> that are periodically merged was an algorithmic problem within
>> btrfs. There are several workloads that don't complete in any
>> reasonable amount of time (e.g. btrfs/130) and others that cause
>> soft lockups.
>>
>> The solution is to use a pair of rbtrees that do insertion merging
>> for both indirect and direct refs, with the former converting
>> refs into the latter. The result is a btrfs/130 workload that
>> used to take several hours now takes about half of that. This
>> runtime still isn't acceptable and a future patch will address that
>> by moving the rbtrees higher in the stack so the lookups can be
>> shared across multiple calls to find_parent_nodes.
>>
>> Signed-off-by: Edmund Nadolski <enadol...@suse.com>
>> Signed-off-by: Jeff Mahoney <je...@suse.com>
>
> I've bisected to this patch, the self-tests run at module load time
> fail:
>
> tests/qgroup-tests.c:272
>
> 270 if (btrfs_verify_qgroup_counts(fs_info, BTRFS_FS_TREE_OBJECTID,
> 271 nodesize, nodesize)) {
> 272 test_msg("Qgroup counts didn't match expected values\n");
> 273 return -EINVAL;
> 274 }
>
> 245 int btrfs_verify_qgroup_counts(struct btrfs_fs_info *fs_info, u64
> qgroupid,
> 246 u64 rfer, u64 excl)
> 247 {
> 248 struct btrfs_qgroup *qgroup;
> 249
> 250 qgroup = find_qgroup_rb(fs_info, qgroupid);
> 251 if (!qgroup)
> 252 return -EINVAL;
> 253 if (qgroup->rfer != rfer || qgroup->excl != excl)
> 254 return -EINVAL;
> 255 return 0;
> 256 }
>
> the second if fails, with 0 != 4096 || 0 != 4096
>
> Tested branch was current for-next-test (top commit
> 8d73f8348287a3d3be10795f45d313f63cdcd72c), with
> CONFIG_BTRFS_FS_RUN_SANITY_TESTS=y
This looks like a consequence of an existing check in __resolve_indirect_ref():
if (btrfs_is_testing(fs_info)) {
srcu_read_unlock(&fs_info->subvol_srcu, index);
ret = -ENOENT;
goto out;
}
The existing code simply leaves the ref on the pref list, to be picked up later
in find_parent_nodes(), which will ulist_add() an entry onto the roots list for
it. The patch otoh when it sees -ENOENT just frees the ref so no entry is
ever added to the ulist.
The patch can be fixed to behave similarly to the existing code by
inserting the ref into the direct tree instead of freeing it. This seems
a bit odd since technically the ref isn't actually 'resolved'. Considering
that this code path is really just a special case for the sanity check when
the fs_info is in a BTRFS_FS_STATE_DUMMY_FS_INFO state, perhaps that's not
too great a concern. Thoughts?
Thanks,
Ed
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
