Closed parkvibes closed 1 month ago
Can you add a test case for this?
Can you add a test case for this?
Sure.
Did you intend the test case to be included in t/nvmep_fdp.py
?
Can you add a test case for this?
Sure. Did you intend the test case to be included in
t/nvmep_fdp.py
?
Yes that's fine.
Looks good. Thanks.
@parkvibes The iolog changes triggered the issues below from Coverity. Could you take a look?
Hi,
Please find the latest report on new defect(s) introduced to axboe/fio found with Coverity Scan.
2 new defect(s) introduced to axboe/fio found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)
** CID 494151: Error handling issues (NEGATIVE_RETURNS)
/iolog.c: 148 in ipo_special()
________________________________________________________________________________________________________
*** CID 494151: Error handling issues (NEGATIVE_RETURNS)
/iolog.c: 148 in ipo_special()
142 ret = td_io_open_file(td, f);
143 if (!ret) {
144 if (td->o.dp_type != FIO_DP_NONE) {
145 int dp_init_ret = dp_init(td);
146
147 if (dp_init_ret != 0) {
>>> CID 494151: Error handling issues (NEGATIVE_RETURNS)
>>> "dp_init_ret" is passed to a parameter that cannot be negative.
148 td_verror(td, dp_init_ret, "dp_init");
149 return -1;
150 }
151 }
152 break;
153 }
** CID 494150: Null pointer dereferences (FORWARD_NULL)
________________________________________________________________________________________________________
*** CID 494150: Null pointer dereferences (FORWARD_NULL)
/io_u.c: 1877 in get_io_u()
1871 goto out;
1872
1873 /*
1874 * If using an iolog, grab next piece if any available.
1875 */
1876 if (td->flags & TD_F_READ_IOLOG) {
>>> CID 494150: Null pointer dereferences (FORWARD_NULL)
>>> Passing "io_u" to "read_iolog_get", which dereferences null "io_u->file".
1877 if (read_iolog_get(td, io_u))
1878 goto err_put;
1879 } else if (set_io_u_file(td, io_u)) {
1880 ret = -EBUSY;
1881 dprint(FD_IO, "io_u %p, setting file failed\n", io_u);
1882 goto err_put;
________________________________________________________________________________________________________
To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=u001.AxU2LYlgjL6eX23u9ErQy-2BKADyCpvUKOL6EWmZljiu4yVEXfoZtNI9aDIN5OOuYjE8SHJnwGeWwjfdVs9s-2BYB3ZEVB97u-2BLmRNT80zQw85Y-3Db7sa_8kI-2BKaI4kRTN8bowTGN-2FIv-2B5XERDW2ihNq7DYuDzDpmmvahLn3JajrxA3i5l5DGyDhhzLSTpTQ-2BJ2sl2nUUaOh87USSlaD9P5rFbqOiOAvnH6OSpFV-2Fc8BKi6mMESwLHUeXVPyb0HpmtEJ03mcYdYqmBXxlvj0UkBSWKzLtyQWImKUzg3AXuv07Yw-2BQDc5ZpFmoMO-2FrNuMNNecVzSxCC4A-3D-3D
@vincentkfu Analyzing the issues, I indentified the causes that could be reasons of issue.
Here is my study.
This issue seems to be not originated from my commits. But I guess that the code that checking whether io_u
is null or not in read_iolog_get()
can resolve the issue.
dp_init_ret
(int
) will be casted into unsigned int
in __td_verror()
macro.
To handle this issue, the declaration of dp_init()
should be changed to return unsigned int
. It seems like this change should come with changes of all functions in datplacement.c/h
But I wonder why similar cases below are not detected as issues.
td_verror()
of line 154 in iolog.c
set_io_u_file()
of line 1879 in io_u.c
As far as I know, these came from https://scan.coverity.com/projects/axboe-fio. After adding me to the project(I already requested), I can see the detail. (to view defects or help fix defects...)
_CID 494150: Null pointer dereferences (FORWARDNULL)
Inspecting the code and Coverity's annotations suggest that there will be a null pointer dereference of io_u->file
in dp_fill_dspec_data()
when ipo->ddir == DDIR_WAIT
. We know that io_u->file
is NULL when read_iolog_get()
is called, so when ipo->ddir == DDIR_WAIT
we should not try to fill dspec data for this operation.
I can confirm that adding /dev/ng0n1 wait 100 100
to iolog
for your new test case does result in a segfault.
One possible fix would be to put
if (td->o.dp_type != FIO_DP_NONE)
dp_fill_dspec_data(td, io_u);
inside the if (iop->ddir !+ DDIR_WAIT) {
block.
_CID 494151: Error handling issues (NEGATIVERETURNS)
It looks like td_verror()
expects a positive errno as the second parameter but dp_init()
returns a negative errno on error. For td_verror()
on line 154 in iolog.c
, td_io_open_file()
only returns 0 or 1.
Thanks for the cross check.
Putting the codes related to dp_fill_dspec_data()
inside if (iop->ddir !+ DDIR_WAIT) {
block sounds good.
I'm willing to update dp_init()
not to return negative value. Before carrying it out, I have some question.
EINVAL
but -EINVAL
)? Inspecting fio codes overally, Some codes return error number itself but others do it adding -
.int
→unsigned int
) in dataplacement.c/h
additionally?The least intrusive change to handle the return code of dp_init would be to leave the function unchanged but simply negate the return code.
fio: enable dataplacement(fdp) while replaying I/Os
Add initialization and dataplacement logic to enable dataplacement(fdp) while fio replays I/Os with read_iolog.
Signed-off-by: Hyunwoo Park dshw.park@samsung.com
t/nvmept_fdp: add a test(402)
A test(402) checks whether dataplacement(fdp) works fine while replaying iologs
Signed-off-by: Hyunwoo Park dshw.park@samsung.com