Skip to content
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

Add missing fsync calls in the snapshot module #4850

Merged
merged 12 commits into from Jun 6, 2019

Conversation

Projects
None yet
4 participants
@benpigchu
Copy link
Contributor

commented Jun 6, 2019

Signed-off-by: Ben Pig Chu benpichu@gmail.com

What have you changed? (mandatory)

Fix #4846 by add some fsync calls in the snapshot module. Also add util used to call fsync by path

What are the type of the changes? (mandatory)

  • Bug fix

How has this PR been tested? (mandatory)

Unit test

Does this PR affect documentation (docs) or release note? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

benpigchu added some commits Jun 6, 2019

raftstore,util: add missing fsync calls in snap
Also add a fsync_by_path function to util.

Signed-off-by: Ben Pig Chu <benpichu@gmail.com>
@BusyJay

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

How about calling sync only when the snap is about to be saved?

Show resolved Hide resolved src/raftstore/store/snap.rs Outdated
Show resolved Hide resolved src/raftstore/store/snap.rs Outdated
util: add a test for fsync_by_path
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

@benpigchu benpigchu force-pushed the benpigchu:snapshot-fsync branch from 743293e to a6183d9 Jun 6, 2019

raftstore,util: address some review comments
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

@benpigchu benpigchu force-pushed the benpigchu:snapshot-fsync branch from 821c8d5 to 4870bad Jun 6, 2019

util: rename fsync_by_path to sync_dir
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

@benpigchu benpigchu force-pushed the benpigchu:snapshot-fsync branch from b080a26 to d2f00d9 Jun 6, 2019

raftstore: remove the not needed fsync call
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>
@benpigchu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@zhangjinpeng1987
Copy link
Member

left a comment

LGTM
Please cherry pick to release-2.1 branch.

@BusyJay

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

I think sync them in fn save function is more understandable and not easy to make mistake.

zhangjinpeng1987 and others added some commits Jun 6, 2019

Revert "raftstore: remove the not needed fsync call"
This reverts commit c40e3bc.

Signed-off-by: Ben Pig Chu <benpichu@gmail.com>
@benpigchu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I reverted the commit that removes the sync in the save functions. PTAL @zhangjinpeng1987 @BusyJay

@@ -1054,6 +1060,7 @@ impl Write for Snap {
file.write_all(&next_buf[0..left])?;
digest.write(&next_buf[0..left]);
cf_file.written_size += left as u64;
cf_file.file.as_mut().unwrap().sync_all()?;

This comment has been minimized.

Copy link
@hicqu

hicqu Jun 6, 2019

Contributor

This line can be removed.

benpigchu added some commits Jun 6, 2019

Revert "Revert "raftstore: remove the not needed fsync call""
This reverts commit a11de59.

Signed-off-by: Ben Pig Chu <benpichu@gmail.com>
raftstore: move the sync_all in impl Write to save
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>
@benpigchu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I misunderstand the previous comment, now I move the sync_all call in impl Write block to fn save().
PTAL @zhangjinpeng1987 @BusyJay

@hicqu

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

lgtm.

@@ -924,6 +926,7 @@ impl Snapshot for Snap {
{
let mut file = cf_file.file.take().unwrap();
file.flush()?;
file.sync_all()?;

This comment has been minimized.

Copy link
@BusyJay

BusyJay Jun 6, 2019

Contributor

If the size is 0 like L920, will there be any file generated?

This comment has been minimized.

Copy link
@benpigchu

benpigchu Jun 6, 2019

Author Contributor

Seems no, see L459

raftstore: only `sync_all` in `save` when recv
Also add a is_sending field to Snap.

Signed-off-by: Ben Pig Chu <benpichu@gmail.com>
@zhangjinpeng1987
Copy link
Member

left a comment

LGTM

@hicqu

hicqu approved these changes Jun 6, 2019

@hicqu hicqu merged commit 41054db into tikv:master Jun 6, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

benpigchu added a commit to benpigchu/tikv that referenced this pull request Jun 6, 2019

Add missing fsync calls in the snapshot module (tikv#4850)
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

benpigchu added a commit to benpigchu/tikv that referenced this pull request Jun 6, 2019

Add missing fsync calls in the snapshot module (tikv#4850)
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

hicqu added a commit that referenced this pull request Jun 7, 2019

Add missing fsync calls in the snapshot module (#4850) (#4853)
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

sticnarf added a commit to sticnarf/tikv that referenced this pull request Jun 10, 2019

Remove all Box of RPN functions
Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Makefile: make sure gdb is installed before sse4.2 check (tikv#4832)

Signed-off-by: Kaige Ye <ye@kaige.org>

Upgrade sys-info (tikv#4760)

* *: upgrade sys-info crate

This fixes a problem with the next toolchain upgrade
where rust fails to link the native components of the crate.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: bump sys-info to 0.5.7

Signed-off-by: Brian Anderson <andersrb@gmail.com>

Batch Top N Executor (tikv#4825)

Signed-off-by: Breezewish <breezewish@pingcap.com>

Add help message in doc:go-client-api.md (tikv#4763)

* add help message in doc:go-client-api.md

Signed-off-by: yy <cacheyy@qq.com>

* update go-client-api.md

Signed-off-by: yy <cacheyy@qq.com>

Modify Makefile to distinguish between developer and packaging use cases (tikv#4687)

* make: Add new "dist_release" rules

To make the optimized build faster the existing "release" rules are going to
changed such that they are not identical to the actual releases. Primarily they
will not have debuginfo by default and will use thinLTO instead of LTO.

This adds new "dist_release", etc rules for the CI/CD system to use.

For now they are identical to the existing rules. After CI is updated
the "release" rules will be changed.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* make: Document release rules

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Makefile: indicate use of fail_release

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Clarify the distinction in instruction set for release targets

Signed-off-by: Brian Anderson <andersrb@gmail.com>

Makefile: fix gdb check (tikv#4840)

Signed-off-by: Kaige Ye <ye@kaige.org>

pessimistic-txn: solve non-pessimistic-lock conflict (tikv#4801)

* txn: replace is_pessimistic_lock to for_update_ts in Lock

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* pessimistic-txn: overwrite optimistic lock in pessimistic_prewrite if
request's for_update_ts is greater than lock's for_update_ts

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* modify comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* add comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* return Error let TiDB to resolve lock

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

coprocessor: add batch aggregate function BitAnd/BitOr/BitXor (tikv#4824)

Batch Top N Layered Benchmarks (tikv#4827)

* Add Top N benchmarks

Signed-off-by: Breezewish <breezewish@pingcap.com>

* Address some comments in previous PRs

Signed-off-by: Breezewish <breezewish@pingcap.com>

coprocessor: add batch aggregate function Max/Min (tikv#4837)

Implement RpnFunction MultiplyDecimal (tikv#4849)

Signed-off-by: Breezewish <breezewish@pingcap.com>

Add missing fsync calls in the snapshot module (tikv#4850)

Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

use HTTP to enable jemalloc profile (tikv#4600)

use HTTP to enable jemalloc profile

Signed-off-by: Yang Keao <keao.yang@yahoo.com>

coprocessor: use servo_arc in BatchTopNExecutor (tikv#4854)

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Fix clippy warnings

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Fix test

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Add docs to function.rs

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Add example output of the macro in the test of the macro.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

fix broken url for configuration options (tikv#4856)

Signed-off-by: Yukang <moorekang@gmail.com>

shrink the latch waiting list (tikv#4844)

Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>

Fix clippy

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

scheduler use spin::Mutex (tikv#4829)

* scheduler use spinlock

Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>

Better panic info

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

hicqu added a commit to hicqu/tikv that referenced this pull request Jun 19, 2019

Add missing fsync calls in the snapshot module (tikv#4850)
Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

hicqu added a commit to hicqu/tikv that referenced this pull request Jun 19, 2019

Add missing fsync calls in the snapshot module (tikv#4850)
Signed-off-by: qupeng <qupeng@pingcap.com>

zhangjinpeng1987 added a commit that referenced this pull request Jun 20, 2019

cherry pick some commits from master to release 3.0 (#4925)
* raftstore: clear stale reads after role change (#4810)

Signed-off-by: qupeng <qupeng@pingcap.com>

* improve bad-regions and tombstone subcommands for tikv-ctl (#4862)

Signed-off-by: qupeng <qupeng@pingcap.com>

* add 2 metrics. (#4830)

Signed-off-by: qupeng <qupeng@pingcap.com>

* Add missing fsync calls in the snapshot module (#4850)

Signed-off-by: qupeng <qupeng@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.