fio: enable dataplacement(fdp) while replaying I/Os

[parkvibes]

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 [email protected]

t/nvmept_fdp: add a test(402)

A test(402) checks whether dataplacement(fdp) works fine while replaying iologs

Signed-off-by: Hyunwoo Park [email protected]

[vincentkfu]

Can you add a test case for this?

[vincentkfu]

space between ) and {

[vincentkfu]

If we return 0 here this fails silently. Why not print out an error message and abort on error? See the td_verror() + return -1; construction below.

[parkvibes]

Following the construction below, I'll update it.

[parkvibes]

Can you add a test case for this?

Sure.
Did you intend the test case to be included in t/nvmep_fdp.py?

[vincentkfu]

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.

[vincentkfu]

Looks good. Thanks.

[vincentkfu]

@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

[parkvibes]

@vincentkfu
Analyzing the issues, I indentified the causes that could be reasons of issue.

Here is my study.

CID 494150: Null pointer dereferences (FORWARD_NULL)

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.

CID 494151: Error handling issues (NEGATIVE_RETURNS)

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...)

[vincentkfu]

CID 494150: Null pointer dereferences (FORWARD_NULL)

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 (NEGATIVE_RETURNS)

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.

[parkvibes]

Thanks for the cross check.

CID 494150: Null pointer dereferences (FORWARD_NULL)

Putting the codes related to dp_fill_dspec_data() inside if (iop->ddir !+ DDIR_WAIT) { block sounds good.

CID 494151: Error handling issues (NEGATIVE_RETURNS)

I'm willing to update dp_init() not to return negative value. Before carrying it out, I have some question.

• Is there any special purpose to return a negative error number(not EINVAL but -EINVAL)? Inspecting fio codes overally, Some codes return error number itself but others do it adding -.
• Would it be okay to change function prototype(int→unsigned int) in dataplacement.c/h additionally?

[vincentkfu]

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.