os/filestore: disable use of splice by default #11113
Conversation
yuyuyu101
commented
Sep 17, 2016
•
yuyuyu101
commented
- currently splice is actually buggy whatever enabling or not fiemap/seek_data feature
- like bluestore's clone_range(a931dd3) we need to zero target file when source file exist hole, but filestore's zero op maybe heavy which not equal to bluestore's zero. So I'd like pick up full copy strategy instead of fine grain handle. Since clone_range overwrite target object case is rare, I don't think this will be a problem in current clone_range users.
| @@ -61,7 +61,7 @@ GenericFileStoreBackend::GenericFileStoreBackend(FileStore *fs): | |||
| m_filestore_fiemap(g_conf->filestore_fiemap), | |||
| m_filestore_seek_data_hole(g_conf->filestore_seek_data_hole), | |||
| m_filestore_fsync_flushes_journal_data(g_conf->filestore_fsync_flushes_journal_data), | |||
| m_filestore_splice(false) {} | |||
| m_filestore_splice(g_conf->filestore_splice) {} | |||
There was a problem hiding this comment.
does this actually change the behavior? it's going from false to (tunable) false.
There was a problem hiding this comment.
I think the detection code in GenericFileStoreBackend.cc shoudl only run if it's true (instead of false) and disable if it's not detected (vs enable if it is).
There was a problem hiding this comment.
Oh, I found the detection in _detect_feature is wrong.... I update _detect_feature too. Otherwise it will enable splice when m_filestore_splice is false....
yuyuyu101
force-pushed
the
wip-fix-filestore-clone
branch
3 times, most recently
from
fed6aa2
to
144e14b
Compare
| goto out; | ||
| } | ||
|
|
||
| if ((uint64_t)st.st_size > dstoff) { |
There was a problem hiding this comment.
I don't think this condition is correct.
Example:
fromis 8KB long, and first 4KB is a holetois 1MB long and has no holes- srcoff = 0, len = 4096, dstoff = 1MB - 4KB
Such operation should have punched a 4KB hole at 1MB - 4K, however nothing will happen since the (st.size > dstoff) condition does not hold.
The problem is that this method only copies the data, however it should have also punched the same holes as in the source.
There was a problem hiding this comment.
agreed.....
@liewegas go ahead to complete the sparse copy feature, or totally fall down to full copy? I think both are ok. But I want to get an ack if we prefer to keep these freeze.
There was a problem hiding this comment.
I suspect in the hole case falling back to a full copy is fine. That path shouldn't be triggered with real workloads anyway.
Signed-off-by: Haomai Wang <haomai@xsky.com>
yuyuyu101
force-pushed
the
wip-fix-filestore-clone
branch
from
144e14b
to
114b1e3
Compare
|
@liewegas remove fiemap things, let's disable splice by default firstly. |
|
retest this please |
|
@liewegas ping? |
liewegas
changed the title
