Re: [Qemu-devel] [RFC PATCH v2 5/7] iscsi: Implement copy offloading

2018-05-09 Thread Fam Zheng 
On Thu, 05/03 11:27, Stefan Hajnoczi wrote:
> On Wed, Apr 18, 2018 at 11:04:22AM +0800, Fam Zheng wrote:
> > +static void iscsi_save_designator(IscsiLun *lun,
> > +  struct 
> > scsi_inquiry_device_identification *inq_di)
> > +{
> > +struct scsi_inquiry_device_designator *desig, *copy = NULL;
> > +
> > +for (desig = inq_di->designators; desig; desig = desig->next) {
> > +if (desig->association ||
> > +desig->designator_type > SCSI_DESIGNATOR_TYPE_NAA) {
> > +continue;
> > +}
> > +/* NAA works better than T10 vendor ID based designator. */
> > +if (!copy || copy->designator_type < desig->designator_type) {
> > +copy = desig;
> > +}
> > +}
> > +if (copy) {
> > +lun->dd = g_new(struct scsi_inquiry_device_designator, 1);
> > +*lun->dd = *copy;
> 
> Just in case:
> 
>   lun->dd->next = NULL;

Sure.

> 
> > +lun->dd->designator = g_malloc(copy->designator_length);
> > +memcpy(lun->dd->designator, copy->designator, 
> > copy->designator_length);
> > +}
> > +}
> > +
> >  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
> >Error **errp)
> >  {
> > @@ -1922,6 +1946,7 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >  struct scsi_task *inq_task;
> >  struct scsi_inquiry_logical_block_provisioning *inq_lbp;
> >  struct scsi_inquiry_block_limits *inq_bl;
> > +struct scsi_inquiry_device_identification *inq_di;
> >  switch (inq_vpd->pages[i]) {
> >  case SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING:
> >  inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> > @@ -1947,6 +1972,17 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> > sizeof(struct scsi_inquiry_block_limits));
> >  scsi_free_scsi_task(inq_task);
> >  break;
> > +case SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION:
> 
> The device designator part should probably be a separate patch.

Good idea, I'll do the split.

> 
> > +inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> > +
> > SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION,
> > +(void **) _di, errp);
> > +if (inq_task == NULL) {
> > +ret = -EINVAL;
> > +goto out;
> > +}
> > +iscsi_save_designator(iscsilun, inq_di);
> > +scsi_free_scsi_task(inq_task);
> > +break;
> >  default:
> >  break;
> >  }
> > @@ -2003,6 +2039,8 @@ static void iscsi_close(BlockDriverState *bs)
> >  iscsi_logout_sync(iscsi);
> >  }
> >  iscsi_destroy_context(iscsi);
> > +g_free(iscsilun->dd->designator);
> 
> Can iscsilun->dd be NULL?

Yes, if the INQUIRY response isn't typical. Will add an if here.

> 
> > +static void iscsi_xcopy_data(struct iscsi_data *data,
> > + IscsiLun *src, int64_t src_lba,
> > + IscsiLun *dst, int64_t dst_lba,
> > + int num_blocks)
> 
> It's not obvious that int is the appropriate type for num_blocks.  The
> caller had a uint64_t value.  Also, IscsiLun->num_blocks is uint64_t.

This is limited by the field size in the command data (2 bytes) so int is big
enough, although unsigned is better. I can change the type, add a comment and an
assert.

> 
> > +{
> > +uint8_t *buf;
> > +int offset;
> > +int tgt_desc_len, seg_desc_len;
> > +
> > +data->size = XCOPY_DESC_OFFSET +
> 
> struct iscsi_data->size is size_t.  Why does this function and the
> populate function use int for memory offsets/lengths?
> 
> > + 32 * 2 +/* IDENT_DESCR_TGT_DESCR */
> > + 28; /* BLK_TO_BLK_SEG_DESCR */
> > +data->data = g_malloc0(data->size);
> > +buf = data->data;
> > +
> > +/* Initialise CSCD list with one src + one dst descriptor */
> > +offset = XCOPY_DESC_OFFSET;
> > +offset += iscsi_populate_target_desc(buf + offset, src);
> > +offset += iscsi_populate_target_desc(buf + offset, dst);
> > +tgt_desc_len = offset - XCOPY_DESC_OFFSET;
> > +
> > +/* Initialise one segment descriptor */
> > +seg_desc_len = iscsi_xcopy_populate_desc(buf + offset, 0, 0,
> > + 0, 1, num_blocks,
> > + src_lba, dst_lba);
> > +offset += seg_desc_len;
> > +
> > +/* Initialise the parameter list header */
> > +iscsi_xcopy_populate_header(buf, 1, 0, 2 /* LIST_ID_USAGE_DISCARD */,
> > +0, tgt_desc_len, seg_desc_len, 0);
> 
> offset, tgt_desc_len, and seg_desc_len are not necessary:
> 
> 1. We already hardcoded exact size for g_malloc0(), so

Re: [Qemu-devel] [RFC PATCH v2 5/7] iscsi: Implement copy offloading

2018-05-03 Thread Stefan Hajnoczi 
On Wed, Apr 18, 2018 at 11:04:22AM +0800, Fam Zheng wrote:
> +static void iscsi_save_designator(IscsiLun *lun,
> +  struct scsi_inquiry_device_identification 
> *inq_di)
> +{
> +struct scsi_inquiry_device_designator *desig, *copy = NULL;
> +
> +for (desig = inq_di->designators; desig; desig = desig->next) {
> +if (desig->association ||
> +desig->designator_type > SCSI_DESIGNATOR_TYPE_NAA) {
> +continue;
> +}
> +/* NAA works better than T10 vendor ID based designator. */
> +if (!copy || copy->designator_type < desig->designator_type) {
> +copy = desig;
> +}
> +}
> +if (copy) {
> +lun->dd = g_new(struct scsi_inquiry_device_designator, 1);
> +*lun->dd = *copy;

Just in case:

  lun->dd->next = NULL;

> +lun->dd->designator = g_malloc(copy->designator_length);
> +memcpy(lun->dd->designator, copy->designator, 
> copy->designator_length);
> +}
> +}
> +
>  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>Error **errp)
>  {
> @@ -1922,6 +1946,7 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  struct scsi_task *inq_task;
>  struct scsi_inquiry_logical_block_provisioning *inq_lbp;
>  struct scsi_inquiry_block_limits *inq_bl;
> +struct scsi_inquiry_device_identification *inq_di;
>  switch (inq_vpd->pages[i]) {
>  case SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING:
>  inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> @@ -1947,6 +1972,17 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> *options, int flags,
> sizeof(struct scsi_inquiry_block_limits));
>  scsi_free_scsi_task(inq_task);
>  break;
> +case SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION:

The device designator part should probably be a separate patch.

> +inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
> +
> SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION,
> +(void **) _di, errp);
> +if (inq_task == NULL) {
> +ret = -EINVAL;
> +goto out;
> +}
> +iscsi_save_designator(iscsilun, inq_di);
> +scsi_free_scsi_task(inq_task);
> +break;
>  default:
>  break;
>  }
> @@ -2003,6 +2039,8 @@ static void iscsi_close(BlockDriverState *bs)
>  iscsi_logout_sync(iscsi);
>  }
>  iscsi_destroy_context(iscsi);
> +g_free(iscsilun->dd->designator);

Can iscsilun->dd be NULL?

> +static void iscsi_xcopy_data(struct iscsi_data *data,
> + IscsiLun *src, int64_t src_lba,
> + IscsiLun *dst, int64_t dst_lba,
> + int num_blocks)

It's not obvious that int is the appropriate type for num_blocks.  The
caller had a uint64_t value.  Also, IscsiLun->num_blocks is uint64_t.

> +{
> +uint8_t *buf;
> +int offset;
> +int tgt_desc_len, seg_desc_len;
> +
> +data->size = XCOPY_DESC_OFFSET +

struct iscsi_data->size is size_t.  Why does this function and the
populate function use int for memory offsets/lengths?

> + 32 * 2 +/* IDENT_DESCR_TGT_DESCR */
> + 28; /* BLK_TO_BLK_SEG_DESCR */
> +data->data = g_malloc0(data->size);
> +buf = data->data;
> +
> +/* Initialise CSCD list with one src + one dst descriptor */
> +offset = XCOPY_DESC_OFFSET;
> +offset += iscsi_populate_target_desc(buf + offset, src);
> +offset += iscsi_populate_target_desc(buf + offset, dst);
> +tgt_desc_len = offset - XCOPY_DESC_OFFSET;
> +
> +/* Initialise one segment descriptor */
> +seg_desc_len = iscsi_xcopy_populate_desc(buf + offset, 0, 0,
> + 0, 1, num_blocks,
> + src_lba, dst_lba);
> +offset += seg_desc_len;
> +
> +/* Initialise the parameter list header */
> +iscsi_xcopy_populate_header(buf, 1, 0, 2 /* LIST_ID_USAGE_DISCARD */,
> +0, tgt_desc_len, seg_desc_len, 0);

offset, tgt_desc_len, and seg_desc_len are not necessary:

1. We already hardcoded exact size for g_malloc0(), so why are we
   calculating the size again dynamically?

2. The populate functions assume the caller already knows the size
   since there is no buffer length argument to prevent overflows.

It's cleaner to use hardcoded offsets:

  iscsi_xcopy_populate_header(buf, 1, 0, 2 /* LIST_ID_USAGE_DISCARD */,
  0, 2 * XCOPY_TGT_DESC_LEN,
  XCOPY_SEG_DESC_LEN, 0);
  iscsi_populate_target_desc([XCOPY_SRC_OFFSET], src);
  iscsi_populate_target_desc([XCOPY_DST_OFFSET], dst);

[Qemu-devel] [RFC PATCH v2 5/7] iscsi: Implement copy offloading

2018-04-17 Thread Fam Zheng 
Issue EXTENDED COPY (LID1) command to implement the copy_range API.

The parameter data construction code is ported from libiscsi's
iscsi-dd.c.

Signed-off-by: Fam Zheng 
---
 block/iscsi.c| 266 +++
 include/scsi/constants.h |   3 +
 2 files changed, 269 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index f5aecfc883..7d17e03ad3 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -68,6 +68,7 @@ typedef struct IscsiLun {
 QemuMutex mutex;
 struct scsi_inquiry_logical_block_provisioning lbp;
 struct scsi_inquiry_block_limits bl;
+struct scsi_inquiry_device_designator *dd;
 unsigned char *zeroblock;
 /* The allocmap tracks which clusters (pages) on the iSCSI target are
  * allocated and which are not. In case a target returns zeros for
@@ -1740,6 +1741,29 @@ static QemuOptsList runtime_opts = {
 },
 };
 
+static void iscsi_save_designator(IscsiLun *lun,
+  struct scsi_inquiry_device_identification 
*inq_di)
+{
+struct scsi_inquiry_device_designator *desig, *copy = NULL;
+
+for (desig = inq_di->designators; desig; desig = desig->next) {
+if (desig->association ||
+desig->designator_type > SCSI_DESIGNATOR_TYPE_NAA) {
+continue;
+}
+/* NAA works better than T10 vendor ID based designator. */
+if (!copy || copy->designator_type < desig->designator_type) {
+copy = desig;
+}
+}
+if (copy) {
+lun->dd = g_new(struct scsi_inquiry_device_designator, 1);
+*lun->dd = *copy;
+lun->dd->designator = g_malloc(copy->designator_length);
+memcpy(lun->dd->designator, copy->designator, copy->designator_length);
+}
+}
+
 static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
@@ -1922,6 +1946,7 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 struct scsi_task *inq_task;
 struct scsi_inquiry_logical_block_provisioning *inq_lbp;
 struct scsi_inquiry_block_limits *inq_bl;
+struct scsi_inquiry_device_identification *inq_di;
 switch (inq_vpd->pages[i]) {
 case SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING:
 inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
@@ -1947,6 +1972,17 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
sizeof(struct scsi_inquiry_block_limits));
 scsi_free_scsi_task(inq_task);
 break;
+case SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION:
+inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
+
SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION,
+(void **) _di, errp);
+if (inq_task == NULL) {
+ret = -EINVAL;
+goto out;
+}
+iscsi_save_designator(iscsilun, inq_di);
+scsi_free_scsi_task(inq_task);
+break;
 default:
 break;
 }
@@ -2003,6 +2039,8 @@ static void iscsi_close(BlockDriverState *bs)
 iscsi_logout_sync(iscsi);
 }
 iscsi_destroy_context(iscsi);
+g_free(iscsilun->dd->designator);
+g_free(iscsilun->dd);
 g_free(iscsilun->zeroblock);
 iscsi_allocmap_free(iscsilun);
 qemu_mutex_destroy(>mutex);
@@ -2184,6 +,230 @@ static void coroutine_fn 
iscsi_co_invalidate_cache(BlockDriverState *bs,
 iscsi_allocmap_invalidate(iscsilun);
 }
 
+static int coroutine_fn iscsi_co_copy_range_from(BlockDriverState *bs,
+ BdrvChild *src,
+ uint64_t src_offset,
+ BdrvChild *dst,
+ uint64_t dst_offset,
+ uint64_t bytes,
+ BdrvRequestFlags flags)
+{
+return bdrv_co_copy_range_to(src, src_offset, dst, dst_offset, bytes, 
flags);
+}
+
+static struct scsi_task *iscsi_xcopy_task(int param_len)
+{
+struct scsi_task *task;
+
+task = g_new0(struct scsi_task, 1);
+
+task->cdb[0] = EXTENDED_COPY;
+task->cdb[10]= (param_len >> 24) & 0xFF;
+task->cdb[11]= (param_len >> 16) & 0xFF;
+task->cdb[12]= (param_len >> 8) & 0xFF;
+task->cdb[13]= param_len & 0xFF;
+task->cdb_size   = 16;
+task->xfer_dir   = SCSI_XFER_WRITE;
+task->expxferlen = param_len;
+
+return task;
+}
+
+static int iscsi_populate_target_desc(unsigned char *desc, IscsiLun *lun)
+{
+struct scsi_inquiry_device_designator *dd = lun->dd;
+
+memset(desc, 0, 32);
+desc[0] = IDENT_DESCR_TGT_DESCR;
+desc[4] = dd->code_set;
+desc[5] = (dd->designator_type & 0xF)
+|