Re: [PATCH v7 10/10] block/iscsi: add persistent reservation in/out driver

[Stefan Hajnoczi]

On Fri, Jul 05, 2024 at 06:56:14PM +0800, Changqi Lu wrote:
> Add persistent reservation in/out operations for iscsi driver.
> The following methods are implemented: bdrv_co_pr_read_keys,
> bdrv_co_pr_read_reservation, bdrv_co_pr_register, bdrv_co_pr_reserve,
> bdrv_co_pr_release, bdrv_co_pr_clear and bdrv_co_pr_preempt.
> 
> Signed-off-by: Changqi Lu <luchangqi.123@bytedance.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  block/iscsi.c | 431 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 431 insertions(+)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 2ff14b7472..9a546f48de 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -96,6 +96,7 @@ typedef struct IscsiLun {
>      unsigned long *allocmap_valid;
>      long allocmap_size;
>      int cluster_size;
> +    uint8_t pr_cap;
>      bool use_16_for_rw;
>      bool write_protected;
>      bool lbpme;
> @@ -280,6 +281,10 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int 
> status,
>                      iTask->err_code = -error;
>                      iTask->err_str = g_strdup(iscsi_get_error(iscsi));
>                  }
> +            } else if (status == SCSI_STATUS_RESERVATION_CONFLICT) {
> +                iTask->err_code = -EBADE;
> +                error_report("iSCSI Persistent Reservation Conflict: %s",
> +                             iscsi_get_error(iscsi));
>              }
>          }
>      }
> @@ -1792,6 +1797,52 @@ static void iscsi_save_designator(IscsiLun *lun,
>      }
>  }
>  
> +static void iscsi_get_pr_cap_sync(IscsiLun *iscsilun, Error **errp)
> +{
> +    struct scsi_task *task = NULL;
> +    struct scsi_persistent_reserve_in_report_capabilities *rc = NULL;
> +    int retries = ISCSI_CMD_RETRIES;
> +    int xferlen = sizeof(struct 
> scsi_persistent_reserve_in_report_capabilities);
> +
> +    do {
> +        if (task != NULL) {
> +            scsi_free_scsi_task(task);
> +            task = NULL;
> +        }
> +
> +        task = iscsi_persistent_reserve_in_sync(iscsilun->iscsi,
> +               iscsilun->lun, SCSI_PR_IN_REPORT_CAPABILITIES, xferlen);
> +        if (task != NULL && task->status == SCSI_STATUS_GOOD) {
> +                rc = scsi_datain_unmarshall(task);
> +                if (rc == NULL) {
> +                    error_setg(errp,
> +                    "iSCSI: Failed to unmarshall report capabilities data.");
> +                } else {
> +                    iscsilun->pr_cap =
> +                    
> scsi_pr_cap_to_block(rc->persistent_reservation_type_mask);
> +                    iscsilun->pr_cap |= (rc->ptpl_a) ? BLK_PR_CAP_PTPL : 0;
> +                }
> +                break;
> +            }
> +
> +        if (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
> +            && task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
> +            break;
> +        }
> +
> +    } while (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
> +             && task->sense.key == SCSI_SENSE_UNIT_ATTENTION
> +             && retries-- > 0);

The if statement is the same condition as the while statement (except
for the retry counter)? It looks like retrying logic won't work in
practice because the if statement breaks early.

> +
> +    if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> +        error_setg(errp, "iSCSI: failed to send report capabilities 
> command");
> +    }

Did you test this function against a SCSI target that does not implement
the optional PERSISTENT RESERVE IN operation? iscsi_open() must succeed
when the target does not implement this.

> +
> +    if (task) {
> +        scsi_free_scsi_task(task);
> +    }
> +}
> +
>  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>                        Error **errp)
>  {
> @@ -2024,6 +2075,11 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
>      }
>  
> +    iscsi_get_pr_cap_sync(iscsilun, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +    }
>  out:
>      qemu_opts_del(opts);
>      g_free(initiator_name);
> @@ -2110,6 +2166,8 @@ static void iscsi_refresh_limits(BlockDriverState *bs, 
> Error **errp)
>          bs->bl.opt_transfer = pow2floor(iscsilun->bl.opt_xfer_len *
>                                          iscsilun->block_size);
>      }
> +
> +    bs->bl.pr_cap = iscsilun->pr_cap;
>  }
>  
>  /* Note that this will not re-establish a connection with an iSCSI target - 
> it
> @@ -2408,6 +2466,371 @@ out_unlock:
>      return r;
>  }
>  
> +static int coroutine_fn
> +iscsi_co_pr_read_keys(BlockDriverState *bs, uint32_t *generation,
> +                      uint32_t num_keys, uint64_t *keys)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    QEMUIOVector qiov;
> +    struct IscsiTask iTask;
> +    int xferlen = sizeof(struct scsi_persistent_reserve_in_read_keys) +
> +                  sizeof(uint64_t) * num_keys;
> +    uint8_t *buf = g_malloc0(xferlen);
> +    int32_t num_collect_keys = 0;
> +    int r = 0;
> +
> +    qemu_iovec_init_buf(&qiov, buf, xferlen);
> +    iscsi_co_init_iscsitask(iscsilun, &iTask);
> +    qemu_mutex_lock(&iscsilun->mutex);
> +retry:
> +    iTask.task = iscsi_persistent_reserve_in_task(iscsilun->iscsi,
> +                 iscsilun->lun, SCSI_PR_IN_READ_KEYS, xferlen,
> +                 iscsi_co_generic_cb, &iTask);
> +
> +    if (iTask.task == NULL) {
> +        qemu_mutex_unlock(&iscsilun->mutex);
> +        return -ENOMEM;

buf is leaked. Use g_autofree to avoid having to call g_free() manually.

> +    }
> +
> +    scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *)qiov.iov, 
> qiov.niov);
> +    iscsi_co_wait_for_task(&iTask, iscsilun);
> +
> +    if (iTask.task != NULL) {
> +        scsi_free_scsi_task(iTask.task);
> +        iTask.task = NULL;
> +    }
> +
> +    if (iTask.do_retry) {
> +        iTask.complete = 0;
> +        goto retry;
> +    }
> +
> +    if (iTask.status != SCSI_STATUS_GOOD) {
> +        error_report("iSCSI PERSISTENT_RESERVE_IN failed: %s", 
> iTask.err_str);
> +        r = iTask.err_code;
> +        goto out;
> +    }
> +
> +    memcpy(generation, &buf[0], 4);
> +    *generation = be32_to_cpu(*generation);
> +    memcpy(&num_collect_keys, &buf[4], 4);
> +    num_collect_keys = be32_to_cpu(num_collect_keys) / sizeof(uint64_t);
> +    if (num_collect_keys > num_keys) {
> +        r = -EINVAL;
> +        goto out;
> +    }
> +
> +    for (int i = 0; i < num_collect_keys; i++) {
> +        memcpy(&keys[i], &buf[8 + i * 8], 8);
> +        keys[i] = be64_to_cpu(keys[i]);
> +    }
> +    r = num_collect_keys;
> +
> +out:
> +    qemu_mutex_unlock(&iscsilun->mutex);
> +    g_free(iTask.err_str);
> +    g_free(buf);
> +    return r;
> +}
> +
> +static int coroutine_fn
> +iscsi_co_pr_read_reservation(BlockDriverState *bs, uint32_t *generation,
> +                             uint64_t *key, BlockPrType *type)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    QEMUIOVector qiov;
> +    struct IscsiTask iTask;
> +    int xferlen = sizeof(struct scsi_persistent_reserve_in_read_reservation);
> +    uint8_t *buf = g_malloc0(xferlen);
> +    uint8_t scope_type = 0;
> +    int32_t num_collect_keys = 0;
> +    int r = 0;
> +
> +    qemu_iovec_init_buf(&qiov, buf, xferlen);
> +    iscsi_co_init_iscsitask(iscsilun, &iTask);
> +    qemu_mutex_lock(&iscsilun->mutex);
> +retry:
> +    iTask.task = iscsi_persistent_reserve_in_task(iscsilun->iscsi,
> +                 iscsilun->lun, SCSI_PR_IN_READ_RESERVATION,
> +                 xferlen, iscsi_co_generic_cb, &iTask);
> +
> +    if (iTask.task == NULL) {
> +        qemu_mutex_unlock(&iscsilun->mutex);
> +        return -ENOMEM;

buf is leaked. Use g_autofree to avoid having to call g_free() manually.

> +    }
> +
> +    scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *)qiov.iov, 
> qiov.niov);
> +    iscsi_co_wait_for_task(&iTask, iscsilun);
> +
> +    if (iTask.task != NULL) {
> +        scsi_free_scsi_task(iTask.task);
> +        iTask.task = NULL;
> +    }
> +
> +    if (iTask.do_retry) {
> +        iTask.complete = 0;
> +        goto retry;
> +    }
> +
> +    if (iTask.status != SCSI_STATUS_GOOD) {
> +        error_report("iSCSI PERSISTENT_RESERVE_IN failed: %s", 
> iTask.err_str);
> +        r = iTask.err_code;
> +        goto out;
> +    }
> +
> +    memcpy(generation, &buf[0], 4);
> +    *generation = be32_to_cpu(*generation);
> +    memcpy(key, &buf[8], 8);
> +    *key = be64_to_cpu(*key);
> +    memcpy(&scope_type, &buf[21], 1);
> +    *type = scsi_pr_type_to_block(scope_type & 0xf);
> +    memcpy(&num_collect_keys, &buf[4], 4);
> +    r = be32_to_cpu(num_collect_keys) / sizeof(uint64_t);
> +out:
> +    qemu_mutex_unlock(&iscsilun->mutex);
> +    g_free(iTask.err_str);
> +    g_free(buf);
> +    return r;
> +}
> +
> +static int coroutine_fn
> +iscsi_co_pr_register(BlockDriverState *bs, uint64_t old_key,
> +                     uint64_t new_key, BlockPrType type,
> +                     bool ptpl, bool ignore_key)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct IscsiTask iTask;
> +    struct scsi_persistent_reserve_out_basic basic;
> +    SCSIPrOutAction action = ignore_key ? SCSI_PR_OUT_REG_AND_IGNORE_KEY :
> +                                          SCSI_PR_OUT_REGISTER;
> +    int r = 0;
> +
> +    basic.reservation_key = old_key;
> +    basic.service_action_reservation_key = new_key;
> +    basic.aptpl = ptpl ? 1 : 0;
> +
> +    iscsi_co_init_iscsitask(iscsilun, &iTask);
> +    qemu_mutex_lock(&iscsilun->mutex);
> +retry:
> +    iTask.task = iscsi_persistent_reserve_out_task(iscsilun->iscsi,
> +                 iscsilun->lun, action, 0, block_pr_type_to_scsi(type),
> +                 &basic, iscsi_co_generic_cb, &iTask);
> +
> +    if (iTask.task == NULL) {
> +        qemu_mutex_unlock(&iscsilun->mutex);
> +        return -ENOMEM;
> +    }
> +
> +    iscsi_co_wait_for_task(&iTask, iscsilun);
> +
> +    if (iTask.task != NULL) {
> +        scsi_free_scsi_task(iTask.task);
> +        iTask.task = NULL;
> +    }
> +
> +    if (iTask.do_retry) {
> +        iTask.complete = 0;
> +        goto retry;
> +    }
> +
> +    if (iTask.status != SCSI_STATUS_GOOD) {
> +        error_report("iSCSI PERSISTENT_RESERVE_OUT failed: %s", 
> iTask.err_str);
> +        r = iTask.err_code;
> +    }
> +
> +    qemu_mutex_unlock(&iscsilun->mutex);
> +
> +    g_free(iTask.err_str);
> +    return r;
> +}
> +
> +static int coroutine_fn
> +iscsi_co_pr_reserve(BlockDriverState *bs, uint64_t key, BlockPrType type)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct IscsiTask iTask;
> +    struct scsi_persistent_reserve_out_basic basic;
> +    int r = 0;
> +
> +    basic.reservation_key = key;
> +    iscsi_co_init_iscsitask(iscsilun, &iTask);
> +    qemu_mutex_lock(&iscsilun->mutex);
> +retry:
> +    iTask.task = iscsi_persistent_reserve_out_task(iscsilun->iscsi,
> +                 iscsilun->lun, SCSI_PR_OUT_RESERVE, 0,
> +                 block_pr_type_to_scsi(type), &basic,
> +                 iscsi_co_generic_cb, &iTask);
> +
> +    if (iTask.task == NULL) {
> +        qemu_mutex_unlock(&iscsilun->mutex);
> +        return -ENOMEM;
> +    }
> +
> +
> +    iscsi_co_wait_for_task(&iTask, iscsilun);
> +
> +    if (iTask.task != NULL) {
> +        scsi_free_scsi_task(iTask.task);
> +        iTask.task = NULL;
> +    }
> +
> +    if (iTask.do_retry) {
> +        iTask.complete = 0;
> +        goto retry;
> +    }
> +
> +    if (iTask.status != SCSI_STATUS_GOOD) {
> +        error_report("iSCSI PERSISTENT_RESERVE_OUT failed: %s", 
> iTask.err_str);
> +        r = iTask.err_code;
> +    }
> +
> +    qemu_mutex_unlock(&iscsilun->mutex);
> +
> +    g_free(iTask.err_str);
> +    return r;
> +}
> +
> +static int coroutine_fn
> +iscsi_co_pr_release(BlockDriverState *bs, uint64_t key, BlockPrType type)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct IscsiTask iTask;
> +    struct scsi_persistent_reserve_out_basic basic;
> +    int r = 0;
> +
> +    basic.reservation_key = key;
> +    iscsi_co_init_iscsitask(iscsilun, &iTask);
> +    qemu_mutex_lock(&iscsilun->mutex);
> +retry:
> +    iTask.task = iscsi_persistent_reserve_out_task(iscsilun->iscsi,
> +                 iscsilun->lun, SCSI_PR_OUT_RELEASE, 0,
> +                 block_pr_type_to_scsi(type), &basic,
> +                 iscsi_co_generic_cb, &iTask);
> +
> +    if (iTask.task == NULL) {
> +        qemu_mutex_unlock(&iscsilun->mutex);
> +        return -ENOMEM;
> +    }
> +
> +
> +    iscsi_co_wait_for_task(&iTask, iscsilun);
> +
> +    if (iTask.task != NULL) {
> +        scsi_free_scsi_task(iTask.task);
> +        iTask.task = NULL;
> +    }
> +
> +    if (iTask.do_retry) {
> +        iTask.complete = 0;
> +        goto retry;
> +    }
> +
> +    if (iTask.status != SCSI_STATUS_GOOD) {
> +        error_report("iSCSI PERSISTENT_RESERVE_OUT failed: %s", 
> iTask.err_str);
> +        r = iTask.err_code;
> +    }
> +
> +    qemu_mutex_unlock(&iscsilun->mutex);
> +
> +    g_free(iTask.err_str);
> +    return r;
> +}
> +
> +static int coroutine_fn
> +iscsi_co_pr_clear(BlockDriverState *bs, uint64_t key)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct IscsiTask iTask;
> +    struct scsi_persistent_reserve_out_basic basic;
> +    int r = 0;
> +
> +    basic.reservation_key = key;
> +    iscsi_co_init_iscsitask(iscsilun, &iTask);
> +    qemu_mutex_lock(&iscsilun->mutex);
> +retry:
> +    iTask.task = iscsi_persistent_reserve_out_task(iscsilun->iscsi,
> +                 iscsilun->lun, SCSI_PR_OUT_CLEAR, 0, 0, &basic,
> +                 iscsi_co_generic_cb, &iTask);
> +
> +    if (iTask.task == NULL) {
> +        qemu_mutex_unlock(&iscsilun->mutex);
> +        return -ENOMEM;
> +    }
> +
> +
> +    iscsi_co_wait_for_task(&iTask, iscsilun);
> +
> +    if (iTask.task != NULL) {
> +        scsi_free_scsi_task(iTask.task);
> +        iTask.task = NULL;
> +    }
> +
> +    if (iTask.do_retry) {
> +        iTask.complete = 0;
> +        goto retry;
> +    }
> +
> +    if (iTask.status != SCSI_STATUS_GOOD) {
> +        error_report("iSCSI PERSISTENT_RESERVE_OUT failed: %s", 
> iTask.err_str);
> +        r = iTask.err_code;
> +    }
> +
> +    qemu_mutex_unlock(&iscsilun->mutex);
> +
> +    g_free(iTask.err_str);
> +    return r;
> +}
> +
> +static int coroutine_fn
> +iscsi_co_pr_preempt(BlockDriverState *bs, uint64_t old_key,
> +                    uint64_t new_key, BlockPrType type, bool abort)
> +{
> +    IscsiLun *iscsilun = bs->opaque;
> +    struct IscsiTask iTask;
> +    struct scsi_persistent_reserve_out_basic basic;
> +    SCSIPrOutAction action = abort ? SCSI_PR_OUT_PREEMPT_AND_ABORT :
> +                                     SCSI_PR_OUT_PREEMPT;
> +    int r = 0;
> +
> +    basic.reservation_key = old_key;
> +    basic.service_action_reservation_key = new_key;
> +
> +    iscsi_co_init_iscsitask(iscsilun, &iTask);
> +    qemu_mutex_lock(&iscsilun->mutex);
> +retry:
> +    iTask.task = iscsi_persistent_reserve_out_task(iscsilun->iscsi,
> +                 iscsilun->lun, action, 0, block_pr_type_to_scsi(type),
> +                 &basic, iscsi_co_generic_cb, &iTask);
> +
> +    if (iTask.task == NULL) {
> +        qemu_mutex_unlock(&iscsilun->mutex);
> +        return -ENOMEM;
> +    }
> +
> +
> +    iscsi_co_wait_for_task(&iTask, iscsilun);
> +
> +    if (iTask.task != NULL) {
> +        scsi_free_scsi_task(iTask.task);
> +        iTask.task = NULL;
> +    }
> +
> +    if (iTask.do_retry) {
> +        iTask.complete = 0;
> +        goto retry;
> +    }
> +
> +    if (iTask.status != SCSI_STATUS_GOOD) {
> +        error_report("iSCSI PERSISTENT_RESERVE_OUT failed: %s", 
> iTask.err_str);
> +        r = iTask.err_code;
> +    }
> +
> +    qemu_mutex_unlock(&iscsilun->mutex);
> +
> +    g_free(iTask.err_str);
> +    return r;
> +}
> +
>  
>  static const char *const iscsi_strong_runtime_opts[] = {
>      "transport",
> @@ -2451,6 +2874,14 @@ static BlockDriver bdrv_iscsi = {
>      .bdrv_co_writev        = iscsi_co_writev,
>      .bdrv_co_flush_to_disk = iscsi_co_flush,
>  
> +    .bdrv_co_pr_read_keys     = iscsi_co_pr_read_keys,
> +    .bdrv_co_pr_read_reservation = iscsi_co_pr_read_reservation,
> +    .bdrv_co_pr_register      = iscsi_co_pr_register,
> +    .bdrv_co_pr_reserve       = iscsi_co_pr_reserve,
> +    .bdrv_co_pr_release       = iscsi_co_pr_release,
> +    .bdrv_co_pr_clear         = iscsi_co_pr_clear,
> +    .bdrv_co_pr_preempt       = iscsi_co_pr_preempt,
> +
>  #ifdef __linux__
>      .bdrv_aio_ioctl   = iscsi_aio_ioctl,
>  #endif
> -- 
> 2.20.1
>

signature.asc
Description: PGP signature