New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kv/RocksDBStore: Table options for indexing and filtering #16450

Merged
merged 1 commit into from Jul 27, 2017

Conversation

Projects
None yet
5 participants
@markhpc
Member

markhpc commented Jul 20, 2017

This PR exposes a number of table options for RocksDB backed key/value stores related to filtering and indexing. It also enables bloom filters by default with 20 bits per key. In testing, this resulted in a significant performance increase (possibly orders of magnitude) when writing out new objects to HDD vs no bloom filters, and about a 20% increase in performance vs setting 10 bits per key. The other newly exposed options may be valuable on clusters where the k/v store has grown big enough that indexes/filters can no longer fit into cache all at once, but the partition index filters can not yet be used since they are still experimental.

@markhpc

This comment has been minimized.

Member

markhpc commented Jul 20, 2017

Note to self: Update doc/rados/configuration/osd-config-ref.rst

@liewegas liewegas added the needs-qa label Jul 20, 2017

OPTION(kstore_rocksdb_index_type, OPT_STR, "binary_search")
OPTION(kstore_rocksdb_partition_filters, OPT_BOOL, false)
OPTION(kstore_rocksdb_metadata_block_size, OPT_U64, 0)
OPTION(kstore_rocksdb_pin_l0_filter_and_index_blocks_in_cache, OPT_BOOL, false)

This comment has been minimized.

@liewegas

liewegas Jul 20, 2017

Member

everything from kstore_rocksdb_options should just be rocksdb_*.. none of this is related in any way to os/KStore (except that kstore might use rocksdb as its backend)

This comment has been minimized.

@markhpc

markhpc Jul 20, 2017

Member

So we are already using kstore_bloom_bits_per_key to set the bloom bits generally for rocksdb in kv/RocksDBStore.cc. That's why I used the kstore prefix to match what we were already doing. Should we change that as well?

This comment has been minimized.

@liu-chunmei

liu-chunmei Jul 20, 2017

Contributor

will the other BlockBasedTableOptions structure members influence the performance? Do we need set one by one?

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 20, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jul 21, 2017

@markhpc how about the performance improvement in SSD?

@yuriw

This comment has been minimized.

Contributor

yuriw commented Jul 24, 2017

@markhpc
may need rebase

From https://github.com/markhpc/ceph

  • branch wip-rocksdb-bloomsettings -> FETCH_HEAD
    CONFLICT (modify/delete): src/common/config_opts.h deleted in HEAD and modified in bc0d7fe. Version bc0d7fe of src/common/config_opts.h left in tree.
    Automatic merge failed; fix conflicts and then commit the result.
@markhpc

This comment has been minimized.

Member

markhpc commented Jul 25, 2017

liupan: need to run new tests across the board since the partition filters are experimental so we can't use them. Going to rerun tests now with only the 20 bits/key bloomfilters with high priority caching and L0 pinning. I expect it will work great until we run out of block cache and then we'll start seeing slow downs. We may need to revisit the default block cache sizes when there are many keys in the database (maybe high object count RGW).

@liewegas liewegas added this to the luminous milestone Jul 25, 2017

@markhpc

This comment has been minimized.

Member

markhpc commented Jul 26, 2017

The commits got a little messy after integrating with #16527 , so I ended up just folding everything into one as it's not that huge anyway.

@markhpc

This comment has been minimized.

Member

markhpc commented Jul 26, 2017

retest this please

kv/RocksDBStore: Add table options for filter and index tuning.
Signed-off-by: Mark Nelson <mnelson@redhat.com>
@markhpc

This comment has been minimized.

Member

markhpc commented Jul 27, 2017

retest this please

@liewegas liewegas merged commit cb12a81 into ceph:master Jul 27, 2017

4 of 5 checks passed

ceph-volume tox testing ceph-volume tox failed
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment