We flush a cache buffer once it's filled. When the write
for that cache buffer has completed, we look to see if
there's more data to flush. Currently if there's *any*
more data to flush, we will flush it, even if it's not
a full buffer.
That can hurt performance though. Ideally we only want
to flush a partial buffer if there's been an explicit
sync operation that requires that partial buffer to be
flushed. Otherwise we will end up writing the partial
buffer to disk, and then come back and write that data
again later when the buffer is full.
Add a new unit test to test for this condition. This
patch breaks one of the existing unit tests which was
designed specifically around a RocksDB failure condition.
Change that file_length unit test to now write exactly
one CACHE_BUFFER, which still tests the general logic
making sure that we don't confuse the amount of data
flushed with the value written to the file's length
xattr.
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: I83795fb45afe854b38648d0e0c1a7928219307a2
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/455698
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Data can get implicitly flushed as cache buffers are filled. But
the length xattr is only written in response to a sync or close
operation. So we cannot just look at the amount of data flushed,
and ignore the sync operation if all of the data written has been
flushed - we still need to write the length xattr.
This also adds a unit test which reproduces the original problem.
Fixes issue #297.
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: Icca6ef4d1544f72e9bc31c4ee77d26b4b7f0cce4
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/455692
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Replaced (uint64_t *) dereference of (char *) variable with memcpy to
avoid unaligned memory access. It fixes the following error reported by
ubsan:
spdk/lib/blobfs/blobfs.c:1492:2: runtime error: load of misaligned
address 0x00000047a136 for type 'uint64_t', which requires 8 byte
alignment
Change-Id: I5f3f28a52b8cbe3eb30394ae7ebd0e64b683db57
Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/454886
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-by: Paul Luse <paul.e.luse@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
This avoids to get the first sync_req in the list, instead of
make sure which sync_req we want to make it complete.
Also release the request early.
Change-Id: If6f9b2ab10f0a93e6c45088d33f2aa78d25f66cb
Signed-off-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452136
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
According to my code understanding, should be assigned
itself instead of req.
Change-Id: I0af4d56f8029269e0632bb6be6c9d912c7106cf8
Signed-off-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452120
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
They were removed from the trace API and BlobFS doesn't
compile right now.
Change-Id: Ie98339f19353e4db901af551c706f57c18b89fc5
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452863
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
For now, just do events for open, close and start/end
for writing the length xattr. These will be useful for
debugging the intermittent RocksDB failures.
Note these events may only be recorded on the polling
SPDK thread. SPDK events cannot be recorded on
non-SPDK threads (i.e. RocksDB user or background threads).
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: I72fb302db23b399f583f56194c237340587f3027
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452262
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
This will be used in next patch to ensure synchronous
threads wait until any background operations are done
before freeing its request memory, and also to print
errors if internal channels have outstanding
requests when freed.
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: I24bc8096f84b70270f87b051f9034ed08e639500
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/451780
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
blob_delete_cb() takes spdk_fs_request as the input
parameter.
Change-Id: Ie6150e7d31d187296a448e82784e2ac2fecfe52c
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/450895
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
fs_truncate_complete_cb() takes spdk_fs_request as
the input parameter.
Change-Id: I0413fecd30a1f7cb6528b502848fa999454ba638
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/450891
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
For existing filesystem APIs, SPDK can only support
1 data buffer, we add the iovec data structure here
so that it can support multiple buffer vectors in
following patches.
Change-Id: I26984f3ea985f349a5016060e0801e3989ce2fc6
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/450722
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
For now all the spdk_fs_cb_args used in blobfs are in the
spdk_fs_request context, so the field 'from_request' in
spdk_fs_cb_args can be removed.
Change-Id: I9b40cffeac8a673a87406ee89ad248939593806b
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/450721
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Data structure spdk_fs_request has internal spdk_fs_cb_args
for each request, and for some APIs such as truncate they
all use spdk_fs_request as the parameter, so here we also
change flush to use the request parameter too.
Change-Id: Ic70b98cafe53d42c234f74b3703dc7797f210569
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/450718
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
After the write data has been appended to cache buffer, the
allocation of flush request can return error, so we move this
allocation before appending to the cache buffer.
Change-Id: Ia8baae8f3d88596561633bd8a5c4f83e554c72d7
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/450981
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
This also means that the RocksDB threads don't need
to be SPDK threads any more.
Change-Id: Icadd6dd446958ebf470ad8ab8239f5390942eb87
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/449465
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
In the next step, this won't be a channel at all.
Change-Id: Ia8fe4da5b0b283e8dfc5c6477b84cfdd346d89a0
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/449464
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
spdk_dma_*malloc() is about to be deprecated.
Change-Id: Ib9fb94463a69ffd9660ab2c7fbe02826a7e06741
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/449796
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Our blobfs allocates fixed number of reqs on three different channels
(the default size is 512), if the req cannot be allocated, our blobfs
related function will do nothing.
With this debug info added, we can easily know the issues.
If the fixed number of requests are not enough, we need to change the
design, using malloc/free together with the fixed number of requests.
During high presure test, I think that if the reqs are not allocated,
it will cause the sync, flush related issues.
Change-Id: Ied8cd5197c4f87b861bd7dc09edd5a095fdcc802
Signed-off-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/448596
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reason: We need to detect this issue. Otherwise, it will
cause null pointer issue later
Change-Id: I4d9aec0570e0b46274ebf3b9642b4727049a4a3b
Signed-off-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/448594
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
The caller function needs to judge whether the buf is
allocated. Putting assert here is easy for debugging,
but not be friendly for real use.
Change-Id: I3c8e66bc95cbd2f9bc87f26335f249c32e339da4
Signed-off-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/448697
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
In function spdk_fs_file_stat_async, the
stat.size = f->append_pos >= f->length ? f->append_pos : f->length;
but in spdk_file_get_length, we return f->length.
So generally, it should all use the same method to return the file
length, and this patch will fix this issue.
Change-Id: Idb9aa9e737711fcd48ac0075c7f7ffed825fe3b0
Signed-off-by: Ziye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/c/439627
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Previously the alignment was to 4k due to page size of blobstore being minmal
size to write to blobs. Now that io_unit is used, alignment can be changed to that.
Change-Id: Ib48182e299dad9d9587ae20b97afe683d53933cb
Signed-off-by: Piotr Pelplinski <piotr.pelplinski@intel.com>
Signed-off-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-on: https://review.gerrithub.io/425788
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Signed-off-by: Piotr Pelplinski <piotr.pelplinski@intel.com>
Change-Id: I40d8532494f75f8175aa4730e723c4d0905a486c
Reviewed-on: https://review.gerrithub.io/425356
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
This is a string name used for debugging only.
Change-Id: I9827f0e6c83be7bc13951c7b5f0951ce6c2a1ece
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/424127
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Change-Id: I066085a0606d64a0d95ab2d28340aa35d83efdf7
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/423504
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Change-Id: I9fe730e5a34608dbc7def3a5c19a891fbcc0d9e9
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/423195
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Xiaodong Liu <xiaodong.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
When testing blobfs, the allocation of cache buffer may fail
without any error information, users may get segment fault
error after that.
Change-Id: I6dc8ee4b93c6a9109aba193f599197caa4c4c383
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/422943
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Xiaodong Liu <xiaodong.liu@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Though the buffer can be allocated in the normal case,
however we still need to check it.
Change-Id: I483c7bd083c24590295e44473674c7b8ed30e9da
Signed-off-by: Ziye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/420462
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Some functions should be executed togther, so
group it into a single function which will avoid
the potential issues.
Change-Id: Ie57be20aa76d9bece82b5534c3c5fc6c9faeebad
Signed-off-by: Ziye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/420233
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
We should check the value of error info. If it
is error, we should not do the read anymore.
Change-Id: Ie3527766779a1f4ed98dde7d9881fe66720badc4
Signed-off-by: Ziye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/420335
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
If it fails to allocate the buf, we should conduct
the error handling.
Change-Id: If36b2dd84236052c777f5007ecbf7295e1c1d3eb
Signed-off-by: Ziye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/420334
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
The two values are all default values.
Change-Id: I117eb217b71c3f079a4450c6f716f9bb70762399
Signed-off-by: Ziye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/420333
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reason:It will be helpful for the debug, and
for some places we need to handle no memory
case.
Change-Id: Id221a856b1f65fe7b946dec246bb53532a29db78
Signed-off-by: Ziye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/419923
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Seth Howell <seth.howell5141@gmail.com>
Reviewed-by: John Kariuki <John.K.Kariuki@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Change-Id: I6babd4cf990bf19b510db88bdfb0ca81e29d9252
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/414700
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Madhu Pai <mpai@netapp.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
Intermittent failures with the RocksDB tests pointed to
corruption in the MANIFEST file. Further debug showed that
the MANIFEST file would be corrupted when its cache was
evicted. Blobfs was partly handling append after cache
eviction - it would write the data to the correct position
within the blob. But the sync path would not write the
updated file length xattr when the cache had been evicted.
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: Ie5cf273f8d6511548146a756a7b63c73fee12101
Reviewed-on: https://review.gerrithub.io/407232
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
strncpy is going to be added to the list of banned functions because
it does not guarantee strings are null terminated.
Change-Id: I14e2ce65401bbdb77260627e94aa936797c20ce0
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/407020
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
To support resize operations during I/O, we will need
to send messages to each thread to quiesce I/O while
the resize operation is in progress to guard against
the cluster map memory changing while another thread
is accessing the cluster map.
Therefore, spdk_blob_resize needs to be asynchronous.
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: Ida037334739b4b80a1dbc76e8f1c70bca8b73582
Reviewed-on: https://review.gerrithub.io/404616
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: I30675083dcb7dda47ac62eaf124614033ff9c587
Reviewed-on: https://review.gerrithub.io/401702
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
These new names are much more clear and are aligned with other
functions such as spdk_blob_close.
Keep the old names around for now but deprecate them. We will
remove them in next release.
Signed-off-by: Jim Harris <james.r.harris@intel.com>
Change-Id: Idc60fd0b19fa2a8b0247a1f5835774d342e721f9
Reviewed-on: https://review.gerrithub.io/400884
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
If the file have cache data and not flushed, the file length will be
not correct.
Change-Id: Idde23e158c1d010e67579b5d6a8d87d3cfbfed2d
Signed-off-by: Cunyin Chang <cunyin.chang@intel.com>
Reviewed-on: https://review.gerrithub.io/393784
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
