Re: [PATCHv4 0/4] tcmu: bug fix and dynamic growing data area support

[Ilias Tsitsimpis]

On Fri, Mar 24, 2017 at 02:45AM, 李秀波 wrote:
> IliasTsitsimpis 写到：
> >Could you please validate the above snippet, and update your patches to
> >include it?
> 
> They are actually two separate issues, but all for BIDI case.
> 
> I'll merged into my patch.

Since these are two separate issues, I would create a new patch.
Also, feel free to add 'Tested-by' me to the first two patches.

-- 
Ilias

Re: [PATCHv4 0/4] tcmu: bug fix and dynamic growing data area support

[Ilias Tsitsimpis]

Hi Xiubo,

On Tue, Mar 21, 2017 at 04:36PM, lixi...@cmss.chinamobile.com wrote:
> [...]
>   tcmu: Fix possible overwrite of t_data_sg's last iov[]
>   tcmu: Fix wrongly calculating of the base_command_size

I tested these two patches, which try to fix the broken support for
BIDI commands in target/user.

Both look good to me, but unfortunately, there is also another
regression which got introduced with the use of the data_bitmap. More
specifically, in case of BIDI commands, the data bitmap records both the
Data-Out and the Data-In buffer, and so when gathering the data in the
tcmu_handle_completion() function, care should be taken in order to
discard the Data-Out buffer before gathering the Data-In buffer.
Something like this should do the trick:

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 1108bf5..7075161 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -610,10 +610,22 @@ static void tcmu_handle_completion(struct tcmu_cmd *cmd, 
struct tcmu_cmd_entry *
   se_cmd->scsi_sense_length);
free_data_area(udev, cmd);
} else if (se_cmd->se_cmd_flags & SCF_BIDI) {
+   struct scatterlist *sg;
+   int n, block, sg_remaining = 0;
DECLARE_BITMAP(bitmap, DATA_BLOCK_BITS);

-   /* Get Data-In buffer before clean up */
bitmap_copy(bitmap, cmd->data_bitmap, DATA_BLOCK_BITS);
+
+   /* Discard Data-Out buffer */
+   for_each_sg(se_cmd->t_data_sg, sg, se_cmd->t_data_nents, n) {
+   sg_remaining += sg->length;
+   while (sg_remaining > 0) {
+   block = find_first_bit(bitmap, DATA_BLOCK_BITS);
+   clear_bit(block, bitmap);
+   sg_remaining -= DATA_BLOCK_SIZE;
+   }
+   }
+
+   /* Get Data-In buffer before clean up */
gather_data_area(udev, bitmap,
se_cmd->t_bidi_data_sg, se_cmd->t_bidi_data_nents);
free_data_area(udev, cmd);

With your patches, and the above diff, support for BIDI commands in the
target/user seems to be working again.

Could you please validate the above snippet, and update your patches to
include it?

-- 
Ilias

Re: [PATCHv4 0/4] tcmu: bug fix and dynamic growing data area support

[Bryant G. Ly]

On 3/21/17 3:36 AM, lixi...@cmss.chinamobile.com wrote:

From: Xiubo Li 

Changed for V4:
- re-order the #3, #4 at the head.
- merge most of the #5 to others.


Changed for V3:
- [PATCHv2 2/5] fix double usage of blocks and possible page fault
   call trace.
- [PATCHv2 5/5] fix a mistake.

Changed for V2:
- [PATCHv2 1/5] just fixes some small spelling and other mistakes.
   And as the initial patch, here sets cmd area to 8M and data area to
   1G(1M fixed and 1023M growing)
- [PATCHv2 2/5] is a new one, adding global data block pool support.
   The max total size of the pool is 2G and all the targets will get
   growing blocks from here.
   Test this using multi-targets at the same time.
- [PATCHv2 3/5] changed nothing, respin it to avoid the conflict.
- [PATCHv2 4/5] and [PATCHv2 5/5] are new ones.



Xiubo Li (4):
   tcmu: Fix possible overwrite of t_data_sg's last iov[]
   tcmu: Fix wrongly calculating of the base_command_size
   tcmu: Add dynamic growing data area feature support
   tcmu: Add global data block pool support

  drivers/target/target_core_user.c | 605 +++---
  1 file changed, 504 insertions(+), 101 deletions(-)


I have built this patch into our kernel and will get back to you.

We will be doing larger scale testing to make sure everything still works.

-Bryant

[PATCHv4 0/4] tcmu: bug fix and dynamic growing data area support

[lixiubo]

From: Xiubo Li 

Changed for V4:
- re-order the #3, #4 at the head.
- merge most of the #5 to others.


Changed for V3:
- [PATCHv2 2/5] fix double usage of blocks and possible page fault
  call trace.
- [PATCHv2 5/5] fix a mistake.

Changed for V2:
- [PATCHv2 1/5] just fixes some small spelling and other mistakes.
  And as the initial patch, here sets cmd area to 8M and data area to
  1G(1M fixed and 1023M growing)
- [PATCHv2 2/5] is a new one, adding global data block pool support.
  The max total size of the pool is 2G and all the targets will get
  growing blocks from here.
  Test this using multi-targets at the same time.
- [PATCHv2 3/5] changed nothing, respin it to avoid the conflict.
- [PATCHv2 4/5] and [PATCHv2 5/5] are new ones.



Xiubo Li (4):
  tcmu: Fix possible overwrite of t_data_sg's last iov[]
  tcmu: Fix wrongly calculating of the base_command_size
  tcmu: Add dynamic growing data area feature support
  tcmu: Add global data block pool support

 drivers/target/target_core_user.c | 605 +++---
 1 file changed, 504 insertions(+), 101 deletions(-)

-- 
1.8.3.1