On 02/27/2017 09:47 PM, lixiubo@xxxxxxxxxxxxxxxxxxxx wrote:
From: Xiubo Li <lixiubo@xxxxxxxxxxxxxxxxxxxx>
If there has BIDI data, its first iov[] will overwrite the last
iov[] for se_cmd->t_data_sg.
(+CCing orig BIDI and data block code authors)
Yeah. It looks like because alloc_and_scatter_data_area() (hereafter
"aasda") is called twice in the BIDI case, and both times iov_cnt is 0,
the new_iov() call doesn't increment the iov ptr and the first bidi iov
overwrites the last data iov. Maybe fix this by exiting aasda() with iov
pointing at the next unused iov in the array? Probably also want to zero
the iov.
To fix this, we can just increase the iov pointer, but this may
introuduce a new memory leakage bug: If the se_cmd->data_length
and se_cmd->t_bidi_data_sg->length are all not aligned up to the
DATA_BLOCK_SIZE, the actual length needed maybe larger than just
sum of them.
This also sounds right. But the solution below rounds up both data and
bidi lengths to DATA_BLOCK_SIZE separately. That's not quite right
either, since the current code (once aasda is fixed) allows the last
data and first bidi iovs to both have allocations from the same data
block. Shouldn't we be rounding up the sum of data and bidi data_lengths
to DATA_BLOCK_SIZE? Call this Option A.
Option B is we go with changing the implementation to always use a
separate data block for BIDI data (BIDI cmds are rare so no big deal),
but then also please look into simplifying code in aasda() and
tcmu_queue_cmd_ring that may now be overly complex.
Thanks -- Regards -- Andy
So, this could be avoided by rounding all the data lengthes up
to DATA_BLOCK_SIZE.
Signed-off-by: Xiubo Li <lixiubo@xxxxxxxxxxxxxxxxxxxx>
---
drivers/target/target_core_user.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 2e33100..59a18fd 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -429,10 +429,11 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
mb = udev->mb_addr;
cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
- data_length = se_cmd->data_length;
+ data_length = round_up(se_cmd->data_length, DATA_BLOCK_SIZE);
if (se_cmd->se_cmd_flags & SCF_BIDI) {
BUG_ON(!(se_cmd->t_bidi_data_sg && se_cmd->t_bidi_data_nents));
- data_length += se_cmd->t_bidi_data_sg->length;
+ data_length += round_up(se_cmd->t_bidi_data_sg->length,
+ DATA_BLOCK_SIZE);
}
if ((command_size > (udev->cmdr_size / 2)) ||
data_length > udev->data_size) {
@@ -503,10 +504,14 @@ static bool is_ring_space_avail(struct tcmu_dev *udev, size_t cmd_size, size_t d
entry->req.iov_dif_cnt = 0;
/* Handle BIDI commands */
- iov_cnt = 0;
- alloc_and_scatter_data_area(udev, se_cmd->t_bidi_data_sg,
- se_cmd->t_bidi_data_nents, &iov, &iov_cnt, false);
- entry->req.iov_bidi_cnt = iov_cnt;
+ if (se_cmd->se_cmd_flags & SCF_BIDI) {
+ iov_cnt = 0;
+ iov++;
+ alloc_and_scatter_data_area(udev, se_cmd->t_bidi_data_sg,
+ se_cmd->t_bidi_data_nents, &iov, &iov_cnt,
+ false);
+ entry->req.iov_bidi_cnt = iov_cnt;
+ }
/* cmd's data_bitmap is what changed in process */
bitmap_xor(tcmu_cmd->data_bitmap, old_bitmap, udev->data_bitmap,
