On Thu, Feb 25, 2016 at 6:03 PM, Sheng Yang <sheng@xxxxxxxxxx> wrote:
> On Thu, Feb 25, 2016 at 5:07 PM, Sheng Yang <sheng@xxxxxxxxxx> wrote:
>> On Thu, Feb 25, 2016 at 4:29 PM, Andy Grover <agrover@xxxxxxxxxx> wrote:
>>> Hi, comments inline.
>>>
>>
>> Thanks for reviewing!
>>
>>>
>>> On 02/24/2016 04:27 PM, Sheng Yang wrote:
>>>>
>>>> The data_bitmap was introduced to support asynchornization accessing of
>>>> data area.
>>>>
>>>> We would divide mailbox data area into blocks, and use data_bitmap to
>>>> track
>>>> the usage of data area. All the new command's data would start with a
>>>> block,
>>>> and may left unusable space after it end. But it's easy to track using
>>>> data_bitmap.
>>>>
>>>> So we can allocate data_are for asynchronization accessing from userspace,
>>>> since we can track the allocation using data_bitmap. The userspace part
>>>> would
>>>> be the same as Maxim's previous asynchronized implementation.
>>>>
>>>> Signed-off-by: Sheng Yang <sheng@xxxxxxxxxx>
>>>> ---
>>>> drivers/target/target_core_user.c | 245
>>>> ++++++++++++++++++++++----------------
>>>> 1 file changed, 145 insertions(+), 100 deletions(-)
>>>>
>>>> diff --git a/drivers/target/target_core_user.c
>>>> b/drivers/target/target_core_user.c
>>>> index baa8720..f039d48 100644
>>>> --- a/drivers/target/target_core_user.c
>>>> +++ b/drivers/target/target_core_user.c
>>>> @@ -26,6 +26,7 @@
>>>> #include <linux/vmalloc.h>
>>>> #include <linux/uio_driver.h>
>>>> #include <linux/stringify.h>
>>>> +#include <linux/bitops.h>
>>>> #include <net/genetlink.h>
>>>> #include <scsi/scsi_common.h>
>>>> #include <scsi/scsi_proto.h>
>>>> @@ -63,8 +64,11 @@
>>>>
>>>> #define TCMU_TIME_OUT (30 * MSEC_PER_SEC)
>>>>
>>>> +#define DATA_BLOCK_BITS 257
>>>> +#define DATA_BLOCK_SIZE 4096
>>>> +
>>>> #define CMDR_SIZE (16 * 4096)
>>>> -#define DATA_SIZE (257 * 4096)
>>>> +#define DATA_SIZE (DATA_BLOCK_BITS * DATA_BLOCK_SIZE)
>>>
>>>
>>> So the reason this was 25*7* was because in testing I saw repeated lengths
>>> of size 512KiB, and due to the ring not being able to be completely used (a
>>> limitation of the circular buffer logic) this would result in only one
>>> request and the second having to wait. Another page allowed 2 to fit at once
>>> :) so now that we're not using a ring any more this can probably just be 256
>>> and still fit 2 512KiB allocations.
>>>
>>
>> Sure. Would update.
>
> In fact I just run another test again 256 * 4096, and found there are
> many requests just 1MB long. So the following warning is triggered:
>
> "TCMU: Request of size 4160/1048576 may be too big for 65408/1048576
> cmd/data ring buffers."
>
> Probably we want to increase the buffer size a bit?
>
> But also it shows that we cannot accommodate all the upper layer
> request in the buffer... There is no way we can allocate 1MB buffer
> for each request.
In fact we should able to limit that data size per request through
max_sectors setting. So it's still ideal for ring buffer to
accommodate all pending commands.
--Sheng
>
> --Sheng
>>
>>>>
>>>> #define TCMU_RING_SIZE (CMDR_SIZE + DATA_SIZE)
>>>>
>>>> @@ -93,12 +97,11 @@ struct tcmu_dev {
>>>> u32 cmdr_size;
>>>> u32 cmdr_last_cleaned;
>>>> /* Offset of data ring from start of mb */
>>>> + /* Must add data_off and mb_addr to get the address */
>>>> size_t data_off;
>>>> size_t data_size;
>>>> - /* Ring head + tail values. */
>>>> - /* Must add data_off and mb_addr to get the address */
>>>> - size_t data_head;
>>>> - size_t data_tail;
>>>> +
>>>> + unsigned long *data_bitmap;
>>>>
>>>> wait_queue_head_t wait_cmdr;
>>>> /* TODO should this be a mutex? */
>>>> @@ -122,9 +125,9 @@ struct tcmu_cmd {
>>>>
>>>> uint16_t cmd_id;
>>>>
>>>> - /* Can't use se_cmd->data_length when cleaning up expired cmds,
>>>> because if
>>>> + /* Can't use se_cmd when cleaning up expired cmds, because if
>>>> cmd has been completed then accessing se_cmd is off limits */
>>>> - size_t data_length;
>>>> + unsigned long *data_bitmap;
>>>
>>>
>>> Can we just include the data_bitmap array directly in tcmu_cmd and tcmu_dev?
>>
>> Sure, would update. I was indeed asking for more mantainence troubles... :)
>>
>>>
>>>>
>>>> unsigned long deadline;
>>>>
>>>> @@ -168,12 +171,11 @@ static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd
>>>> *se_cmd)
>>>>
>>>> tcmu_cmd->se_cmd = se_cmd;
>>>> tcmu_cmd->tcmu_dev = udev;
>>>> - tcmu_cmd->data_length = se_cmd->data_length;
>>>>
>>>> - if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>>> - BUG_ON(!(se_cmd->t_bidi_data_sg &&
>>>> se_cmd->t_bidi_data_nents));
>>>> - tcmu_cmd->data_length += se_cmd->t_bidi_data_sg->length;
>>>> - }
>>>> + tcmu_cmd->data_bitmap = kzalloc(sizeof(unsigned long) *
>>>> + BITS_TO_LONGS(DATA_BLOCK_BITS), GFP_KERNEL);
>>>> + if (!udev->data_bitmap)
>>>> + goto err_bitmap;
>>>
>>>
>>> This should be "if (!tcmu_cmd->data_bitmap)" (but prefer not to allocate at
>>> all if possible, see previous comment)
>>
>> Yeah...
>>>
>>>
>>>>
>>>> tcmu_cmd->deadline = jiffies + msecs_to_jiffies(TCMU_TIME_OUT);
>>>>
>>>> @@ -185,12 +187,17 @@ static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd
>>>> *se_cmd)
>>>> idr_preload_end();
>>>>
>>>> if (cmd_id < 0) {
>>>> - kmem_cache_free(tcmu_cmd_cache, tcmu_cmd);
>>>> - return NULL;
>>>> + goto err_cmdid;
>>>> }
>>>> tcmu_cmd->cmd_id = cmd_id;
>>>>
>>>> return tcmu_cmd;
>>>> +
>>>> +err_cmdid:
>>>> + kfree(tcmu_cmd->data_bitmap);
>>>> +err_bitmap:
>>>> + kmem_cache_free(tcmu_cmd_cache, tcmu_cmd);
>>>> + return NULL;
>>>> }
>>>>
>>>> static inline void tcmu_flush_dcache_range(void *vaddr, size_t size)
>>>> @@ -242,111 +249,115 @@ static inline void new_iov(struct iovec **iov, int
>>>> *iov_cnt,
>>>>
>>>> iovec = *iov;
>>>> memset(iovec, 0, sizeof(struct iovec));
>>>> -
>>>> - /* Even iov_base is relative to mb_addr */
>>>> - iovec->iov_base = (void __user *) udev->data_off +
>>>> - udev->data_head;
>>>> }
>>>>
>>>> #define UPDATE_HEAD(head, used, size) smp_store_release(&head, ((head %
>>>> size) + used) % size)
>>>>
>>>> +/* offset is relative to mb_addr */
>>>> +static inline size_t get_block_offset(struct tcmu_dev *dev,
>>>> + int block, int remaining)
>>>> +{
>>>> + return dev->data_off + block * DATA_BLOCK_SIZE +
>>>> + DATA_BLOCK_SIZE - remaining;
>>>> +}
>>>> +
>>>> +static inline size_t iov_tail(struct tcmu_dev *udev, struct iovec *iov)
>>>> +{
>>>> + return (size_t)iov->iov_base + iov->iov_len;
>>>> +}
>>>> +
>>>> static void alloc_and_scatter_data_area(struct tcmu_dev *udev,
>>>> struct scatterlist *data_sg, unsigned int data_nents,
>>>> struct iovec **iov, int *iov_cnt, bool copy_data)
>>>> {
>>>> - int i;
>>>> + int i, block;
>>>> + int block_remaining = 0;
>>>> void *from, *to;
>>>> - size_t copy_bytes;
>>>> + size_t copy_bytes, to_offset;
>>>> struct scatterlist *sg;
>>>>
>>>> - if (data_nents == 0)
>>>> - return;
>>>> -
>>>> - new_iov(iov, iov_cnt, udev);
>>>> for_each_sg(data_sg, sg, data_nents, i) {
>>>> - copy_bytes = min_t(size_t, sg->length,
>>>> - head_to_end(udev->data_head,
>>>> udev->data_size));
>>>> + int sg_remaining = sg->length;
>>>> from = kmap_atomic(sg_page(sg)) + sg->offset;
>>>> - to = (void *) udev->mb_addr + udev->data_off +
>>>> udev->data_head;
>>>> -
>>>> - if (copy_data) {
>>>> - memcpy(to, from, copy_bytes);
>>>> - tcmu_flush_dcache_range(to, copy_bytes);
>>>> - }
>>>> -
>>>> - (*iov)->iov_len += copy_bytes;
>>>> -
>>>> - UPDATE_HEAD(udev->data_head, copy_bytes, udev->data_size);
>>>> -
>>>> - /* Uh oh, we wrapped the buffer. Must split sg across 2
>>>> iovs. */
>>>> - if (sg->length != copy_bytes) {
>>>> - void *from_skip = from + copy_bytes;
>>>> -
>>>> - copy_bytes = sg->length - copy_bytes;
>>>> -
>>>> - new_iov(iov, iov_cnt, udev);
>>>> - (*iov)->iov_len = copy_bytes;
>>>> -
>>>> + while (sg_remaining > 0) {
>>>> + if (block_remaining == 0) {
>>>> + block =
>>>> find_first_zero_bit(udev->data_bitmap,
>>>> + DATA_BLOCK_BITS);
>>>> + block_remaining = DATA_BLOCK_SIZE;
>>>> + set_bit(block, udev->data_bitmap);
>>>> + }
>>>> + copy_bytes = min_t(size_t, sg_remaining,
>>>> + block_remaining);
>>>> + to_offset = get_block_offset(udev, block,
>>>> + block_remaining);
>>>> + to = (void *)udev->mb_addr + to_offset;
>>>> + if (*iov_cnt != 0 &&
>>>> + to_offset == iov_tail(udev, *iov)) {
>>>> + (*iov)->iov_len += copy_bytes;
>>>> + } else {
>>>> + new_iov(iov, iov_cnt, udev);
>>>> + (*iov)->iov_base = (void __user *)
>>>> to_offset;
>>>> + (*iov)->iov_len = copy_bytes;
>>>> + }
>>>> if (copy_data) {
>>>> - to = (void *) udev->mb_addr +
>>>> - udev->data_off + udev->data_head;
>>>> - memcpy(to, from_skip, copy_bytes);
>>>> + memcpy(to, from + sg->length -
>>>> sg_remaining,
>>>> + copy_bytes);
>>>> tcmu_flush_dcache_range(to, copy_bytes);
>>>> }
>>>> -
>>>> -
>>>> - UPDATE_HEAD(udev->data_head,
>>>> - copy_bytes, udev->data_size);
>>>> + sg_remaining -= copy_bytes;
>>>> + block_remaining -= copy_bytes;
>>>> }
>>>> -
>>>> kunmap_atomic(from - sg->offset);
>>>> }
>>>> }
>>>>
>>>> -static void free_data_area(struct tcmu_dev *udev, size_t length)
>>>> +static void free_data_area(struct tcmu_dev *udev, struct tcmu_cmd *cmd)
>>>> {
>>>> - UPDATE_HEAD(udev->data_tail, length, udev->data_size);
>>>> + bitmap_xor(udev->data_bitmap, udev->data_bitmap, cmd->data_bitmap,
>>>> + DATA_BLOCK_BITS);
>>>> }
>>>>
>>>> -static void gather_and_free_data_area(struct tcmu_dev *udev,
>>>> - struct scatterlist *data_sg, unsigned int data_nents)
>>>> +static void gather_data_area(struct tcmu_dev *udev, unsigned long
>>>> *cmd_bitmap,
>>>> + struct scatterlist *data_sg, unsigned int data_nents)
>>>> {
>>>> - int i;
>>>> + int i, block;
>>>> + int block_remaining = 0;
>>>> void *from, *to;
>>>> - size_t copy_bytes;
>>>> + size_t copy_bytes, from_offset;
>>>> struct scatterlist *sg;
>>>>
>>>> - /* It'd be easier to look at entry's iovec again, but UAM */
>>>> for_each_sg(data_sg, sg, data_nents, i) {
>>>> - copy_bytes = min_t(size_t, sg->length,
>>>> - head_to_end(udev->data_tail,
>>>> udev->data_size));
>>>> -
>>>> + int sg_remaining = sg->length;
>>>> to = kmap_atomic(sg_page(sg)) + sg->offset;
>>>> - WARN_ON(sg->length + sg->offset > PAGE_SIZE);
>>>> - from = (void *) udev->mb_addr +
>>>> - udev->data_off + udev->data_tail;
>>>> - tcmu_flush_dcache_range(from, copy_bytes);
>>>> - memcpy(to, from, copy_bytes);
>>>> -
>>>> - free_data_area(udev, copy_bytes);
>>>> -
>>>> - /* Uh oh, wrapped the data buffer for this sg's data */
>>>> - if (sg->length != copy_bytes) {
>>>> - void *to_skip = to + copy_bytes;
>>>> -
>>>> - from = (void *) udev->mb_addr +
>>>> - udev->data_off + udev->data_tail;
>>>> - WARN_ON(udev->data_tail);
>>>> - copy_bytes = sg->length - copy_bytes;
>>>> + while (sg_remaining > 0) {
>>>> + if (block_remaining == 0) {
>>>> + block = find_first_bit(cmd_bitmap,
>>>> + DATA_BLOCK_BITS);
>>>> + block_remaining = DATA_BLOCK_SIZE;
>>>> + clear_bit(block, cmd_bitmap);
>>>> + }
>>>> + copy_bytes = min_t(size_t, sg_remaining,
>>>> + block_remaining);
>>>> + from_offset = get_block_offset(udev, block,
>>>> + block_remaining);
>>>> + from = (void *) udev->mb_addr + from_offset;
>>>> tcmu_flush_dcache_range(from, copy_bytes);
>>>> - memcpy(to_skip, from, copy_bytes);
>>>> + memcpy(to + sg->length - sg_remaining, from,
>>>> + copy_bytes);
>>>>
>>>> - free_data_area(udev, copy_bytes);
>>>> + sg_remaining -= copy_bytes;
>>>> + block_remaining -= copy_bytes;
>>>> }
>>>> kunmap_atomic(to - sg->offset);
>>>> }
>>>> }
>>>>
>>>> +static inline size_t spc_bitmap_free(unsigned long *bitmap)
>>>> +{
>>>> + return DATA_BLOCK_SIZE * (DATA_BLOCK_BITS -
>>>> + bitmap_weight(bitmap, DATA_BLOCK_BITS));
>>>> +}
>>>> +
>>>> /*
>>>> * We can't queue a command until we have space available on the cmd
>>>> ring *and*
>>>> * space available on the data ring.
>>>> @@ -380,10 +391,10 @@ static bool is_ring_space_avail(struct tcmu_dev
>>>> *udev, size_t cmd_size, size_t d
>>>> return false;
>>>> }
>>>>
>>>> - space = spc_free(udev->data_head, udev->data_tail,
>>>> udev->data_size);
>>>> + space = spc_bitmap_free(udev->data_bitmap);
>>>> if (space < data_needed) {
>>>> - pr_debug("no data space: %zu %zu %zu\n", udev->data_head,
>>>> - udev->data_tail, udev->data_size);
>>>> + pr_debug("no data space: only %lu available, but ask for
>>>> %lu\n",
>>>> + space, data_needed);
>>>> return false;
>>>> }
>>>>
>>>> @@ -402,6 +413,8 @@ static int tcmu_queue_cmd_ring(struct tcmu_cmd
>>>> *tcmu_cmd)
>>>> uint32_t cmd_head;
>>>> uint64_t cdb_off;
>>>> bool copy_to_data_area;
>>>> + size_t data_length;
>>>> + DECLARE_BITMAP(old_bitmap, DATA_BLOCK_BITS);
>>>>
>>>> if (test_bit(TCMU_DEV_BIT_BROKEN, &udev->flags))
>>>> return -EINVAL;
>>>> @@ -410,10 +423,12 @@ static int tcmu_queue_cmd_ring(struct tcmu_cmd
>>>> *tcmu_cmd)
>>>> * Must be a certain minimum size for response sense info, but
>>>> * also may be larger if the iov array is large.
>>>> *
>>>> - * 3 iovs since we can describe the whole continuous are using one
>>>> - * for data, one for bidi and one more in the case of wrap.
>>>> + * We prepare way too many iovs for potential uses here, because
>>>> it's
>>>> + * expensive to tell how many regions are freed in the bitmap
>>>> */
>>>> - base_command_size = max(offsetof(struct tcmu_cmd_entry,
>>>> req.iov[3]),
>>>> + base_command_size = max(offsetof(struct tcmu_cmd_entry,
>>>> + req.iov[se_cmd->t_bidi_data_nents +
>>>> + se_cmd->t_data_nents]),
>>>> sizeof(struct tcmu_cmd_entry));
>>>> command_size = base_command_size
>>>> + round_up(scsi_command_size(se_cmd->t_task_cdb),
>>>> TCMU_OP_ALIGN_SIZE);
>>>> @@ -424,13 +439,18 @@ static int tcmu_queue_cmd_ring(struct tcmu_cmd
>>>> *tcmu_cmd)
>>>>
>>>> mb = udev->mb_addr;
>>>> cmd_head = mb->cmd_head % udev->cmdr_size; /* UAM */
>>>> + data_length = se_cmd->data_length;
>>>> + 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;
>>>> + }
>>>> if ((command_size > (udev->cmdr_size / 2))
>>>> - || tcmu_cmd->data_length > (udev->data_size - 1))
>>>> + || data_length > (udev->data_size - 1))
>>>> pr_warn("TCMU: Request of size %zu/%zu may be too big for
>>>> %u/%zu "
>>>> - "cmd/data ring buffers\n", command_size,
>>>> tcmu_cmd->data_length,
>>>> + "cmd/data ring buffers\n", command_size,
>>>> data_length,
>>>> udev->cmdr_size, udev->data_size);
>>>>
>>>> - while (!is_ring_space_avail(udev, command_size,
>>>> tcmu_cmd->data_length)) {
>>>> + while (!is_ring_space_avail(udev, command_size, data_length)) {
>>>> int ret;
>>>> DEFINE_WAIT(__wait);
>>>>
>>>> @@ -477,6 +497,8 @@ static int tcmu_queue_cmd_ring(struct tcmu_cmd
>>>> *tcmu_cmd)
>>>> entry->hdr.kflags = 0;
>>>> entry->hdr.uflags = 0;
>>>>
>>>> + bitmap_copy(old_bitmap, udev->data_bitmap, DATA_BLOCK_BITS);
>>>> +
>>>> /*
>>>> * Fix up iovecs, and handle if allocation in data ring wrapped.
>>>> */
>>>> @@ -495,6 +517,10 @@ static int tcmu_queue_cmd_ring(struct tcmu_cmd
>>>> *tcmu_cmd)
>>>> 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,
>>>> + DATA_BLOCK_BITS);
>>>> +
>>>> /* All offsets relative to mb_addr, not start of entry! */
>>>> cdb_off = CMDR_OFF + cmd_head + base_command_size;
>>>> memcpy((void *) mb + cdb_off, se_cmd->t_task_cdb,
>>>> scsi_command_size(se_cmd->t_task_cdb));
>>>> @@ -533,6 +559,7 @@ static int tcmu_queue_cmd(struct se_cmd *se_cmd)
>>>> idr_remove(&udev->commands, tcmu_cmd->cmd_id);
>>>> spin_unlock_irq(&udev->commands_lock);
>>>>
>>>> + kfree(tcmu_cmd->data_bitmap);
>>>> kmem_cache_free(tcmu_cmd_cache, tcmu_cmd);
>>>> }
>>>>
>>>> @@ -547,31 +574,36 @@ static void tcmu_handle_completion(struct tcmu_cmd
>>>> *cmd, struct tcmu_cmd_entry *
>>>> if (test_bit(TCMU_CMD_BIT_EXPIRED, &cmd->flags)) {
>>>> /* cmd has been completed already from timeout, just
>>>> reclaim data
>>>> ring space */
>>>> - free_data_area(udev, cmd->data_length);
>>>> + free_data_area(udev, cmd);
>>>> return;
>>>> }
>>>>
>>>> if (entry->hdr.uflags & TCMU_UFLAG_UNKNOWN_OP) {
>>>> - free_data_area(udev, cmd->data_length);
>>>> + free_data_area(udev, cmd);
>>>> pr_warn("TCMU: Userspace set UNKNOWN_OP flag on se_cmd
>>>> %p\n",
>>>> cmd->se_cmd);
>>>> entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
>>>> } else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
>>>> memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
>>>> se_cmd->scsi_sense_length);
>>>> - free_data_area(udev, cmd->data_length);
>>>> + free_data_area(udev, cmd);
>>>> } else if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>>> - /* Discard data_out buffer */
>>>> - free_data_area(udev, (size_t)se_cmd->t_data_sg->length);
>>>> + DECLARE_BITMAP(bitmap, DATA_BLOCK_BITS);
>>>>
>>>> - /* Get Data-In buffer */
>>>> - gather_and_free_data_area(udev,
>>>> + /* Get Data-In buffer before clean up */
>>>> + bitmap_copy(bitmap, cmd->data_bitmap, DATA_BLOCK_BITS);
>>>> + gather_data_area(udev, bitmap,
>>>> se_cmd->t_bidi_data_sg,
>>>> se_cmd->t_bidi_data_nents);
>>>> + free_data_area(udev, cmd);
>>>> } else if (se_cmd->data_direction == DMA_FROM_DEVICE) {
>>>> - gather_and_free_data_area(udev,
>>>> + DECLARE_BITMAP(bitmap, DATA_BLOCK_BITS);
>>>> +
>>>> + bitmap_copy(bitmap, cmd->data_bitmap, DATA_BLOCK_BITS);
>>>> + gather_data_area(udev, bitmap,
>>>> se_cmd->t_data_sg, se_cmd->t_data_nents);
>>>> + free_data_area(udev, cmd);
>>>> } else if (se_cmd->data_direction == DMA_TO_DEVICE) {
>>>> - free_data_area(udev, cmd->data_length);
>>>> + free_data_area(udev, cmd);
>>>> } else if (se_cmd->data_direction != DMA_NONE) {
>>>> pr_warn("TCMU: data direction was %d!\n",
>>>> se_cmd->data_direction);
>>>> @@ -580,6 +612,7 @@ static void tcmu_handle_completion(struct tcmu_cmd
>>>> *cmd, struct tcmu_cmd_entry *
>>>> target_complete_cmd(cmd->se_cmd, entry->rsp.scsi_status);
>>>> cmd->se_cmd = NULL;
>>>>
>>>> + kfree(cmd->data_bitmap);
>>>> kmem_cache_free(tcmu_cmd_cache, cmd);
>>>> }
>>>>
>>>> @@ -659,6 +692,7 @@ static int tcmu_check_expired_cmd(int id, void *p,
>>>> void *data)
>>>> target_complete_cmd(cmd->se_cmd, SAM_STAT_CHECK_CONDITION);
>>>> cmd->se_cmd = NULL;
>>>>
>>>> + kfree(cmd->data_bitmap);
>>>> kmem_cache_free(tcmu_cmd_cache, cmd);
>>>>
>>>> return 0;
>>>> @@ -905,6 +939,13 @@ static int tcmu_configure_device(struct se_device
>>>> *dev)
>>>> udev->data_off = CMDR_SIZE;
>>>> udev->data_size = TCMU_RING_SIZE - CMDR_SIZE;
>>>>
>>>> + udev->data_bitmap = kzalloc(sizeof(unsigned long) *
>>>> + BITS_TO_LONGS(DATA_BLOCK_BITS), GFP_KERNEL);
>>>> + if (!udev->data_bitmap) {
>>>> + ret = -ENOMEM;
>>>> + goto err_bitmap;
>>>> + }
>>>> +
>>>> mb = udev->mb_addr;
>>>> mb->version = TCMU_MAILBOX_VERSION;
>>>> mb->cmdr_off = CMDR_OFF;
>>>> @@ -912,6 +953,7 @@ static int tcmu_configure_device(struct se_device
>>>> *dev)
>>>>
>>>> WARN_ON(!PAGE_ALIGNED(udev->data_off));
>>>> WARN_ON(udev->data_size % PAGE_SIZE);
>>>> + WARN_ON(udev->data_size % DATA_BLOCK_SIZE);
>>>>
>>>> info->version = __stringify(TCMU_MAILBOX_VERSION);
>>>>
>>>> @@ -948,6 +990,8 @@ static int tcmu_configure_device(struct se_device
>>>> *dev)
>>>> err_netlink:
>>>> uio_unregister_device(&udev->uio_info);
>>>> err_register:
>>>> + kfree(udev->data_bitmap);
>>>> +err_bitmap:
>>>> vfree(udev->mb_addr);
>>>> err_vzalloc:
>>>> kfree(info->name);
>>>> @@ -979,6 +1023,7 @@ static void tcmu_free_device(struct se_device *dev)
>>>>
>>>> del_timer_sync(&udev->timeout);
>>>>
>>>> + kfree(udev->data_bitmap);
>>>> vfree(udev->mb_addr);
>>>>
>>>> /* Upper layer should drain all requests before calling this */
>>>>
>>>
>>> Overall I really like the approach you've taken. This is different (better!)
>>> than what I originally envisioned.
>>
>> Thank you Andy!
>>
>> Maybe in the future we can reduce the block size and increase the
>> bitmap, to increase space efficiency. Though we probably want to make
>> more measurement first.
>>
>> --Sheng
>>>
>>> Regards -- Andy
>>>
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
