various improvements for the iSCSI driver
v2 cover various improvements for the iSCSI driver
v1->v2: - removed the requirement for libiscsi 1.10.0 [Paolo]
- reworked to force_next_flush logic [Paolo]
Peter Lieven (9):
block/iscsi: do not forget to logout from target block/iscsi: change all iscsilun properties from uint8_t to bool block/iscsi: rename iscsi_write_protected and let it return void block/iscsi: store DPOFUA bit from the modesense command block/iscsi: optimize WRITE10/16 if cache.writeback is not set block/iscsi: increase retry count
block/iscsi: handle SCSI_STATUS_TASK_SET_FULL block/iscsi: bump year in copyright notice block/iscsi: use the allocationmap also if cache.direct=on
block/iscsi.c | 64 ++++++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 20 deletions(-)
Signature · 1 line
1.9.1
PATCH v2 1/9 block/iscsi: do not forget to logout from target
We actually were always impolitely dropping the connection and not cleanly logging out.
--- block/iscsi.c | 6 ++++++ 1 file changed, 6 insertions(+)
block/iscsi.c +6 -0
@@ -1501,6 +1501,9 @@ out:if (ret) {if (iscsi !=NULL ) { +if (iscsi_is_logged_in (iscsi)) { +iscsi_logout_sync (iscsi); + }iscsi_destroy_context (iscsi); }memset (iscsilun,0 ,sizeof (IscsiLun));
@@ -1514,6 +1517,9 @@ static void iscsi_close(BlockDriverState *bs)struct iscsi_context *iscsi = iscsilun->iscsi;iscsi_detach_aio_context (bs); +if (iscsi_is_logged_in (iscsi)) { +iscsi_logout_sync (iscsi); + }iscsi_destroy_context (iscsi);g_free (iscsilun->zeroblock);g_free (iscsilun->allocationmap);
Signature · 1 line
1.9.1
PATCH v2 2/9 block/iscsi: change all iscsilun properties from uint8_t to bool
In reply to Peter Lieven--- block/iscsi.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
block/iscsi.c +7 -7
@@ -57,9 +57,6 @@ typedef struct IscsiLun {int events;QEMUTimer *nop_timer;QEMUTimer *event_timer; -uint8_t lbpme; -uint8_t lbprz; -uint8_t has_write_same;struct scsi_inquiry_logical_block_provisioning lbp;struct scsi_inquiry_block_limits bl;unsigned char *zeroblock;
@@ -67,6 +64,9 @@ typedef struct IscsiLun {int cluster_sectors;bool use_16_for_rw;bool write_protected; +bool lbpme; +bool lbprz; +bool has_write_same; } IscsiLun; typedef struct IscsiTask {
@@ -460,7 +460,7 @@ static int64_t coroutine_fn iscsi_co_get_block_status(BlockDriverState *bs, *pnum = nb_sectors;/* LUN does not support logical block provisioning */ -if (iscsilun->lbpme ==0 ) { +if (!iscsilun->lbpme) { goto out; }
@@ -1121,8 +1121,8 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun, Error **errp) }else { iscsilun->block_size = rc16->block_length; iscsilun->num_blocks = rc16->returned_lba +1 ; - iscsilun->lbpme = rc16->lbpme; - iscsilun->lbprz = rc16->lbprz; + iscsilun->lbpme = !!rc16->lbpme; + iscsilun->lbprz = !!rc16->lbprz; iscsilun->use_16_for_rw = (rc16->returned_lba >0xffffffff ); } }
@@ -1655,7 +1655,7 @@ out:static int iscsi_get_info (BlockDriverState *bs,BlockDriverInfo *bdi) {IscsiLun *iscsilun = bs->opaque; - bdi->unallocated_blocks_are_zero = !!iscsilun->lbprz; + bdi->unallocated_blocks_are_zero = iscsilun->lbprz; bdi->can_write_zeroes_with_unmap = iscsilun->lbprz && iscsilun->lbp.lbpws; bdi->cluster_size = iscsilun->cluster_sectors *BDRV_SECTOR_SIZE ;return 0 ;
Signature · 1 line
1.9.1
PATCH v2 3/9 block/iscsi: rename iscsi_write_protected and let it return void
In reply to Peter Lieven--- block/iscsi.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
block/iscsi.c +5 -5
@@ -1253,11 +1253,11 @@ static void iscsi_attach_aio_context(BlockDriverState *bs, iscsi_timed_set_events, iscsilun); } -static bool iscsi_is_write_protected (IscsiLun *iscsilun) +static void iscsi_modesense_sync (IscsiLun *iscsilun) {struct scsi_task *task;struct scsi_mode_sense *ms =NULL ; -bool wrprotected = false; + iscsilun->write_protected = false; task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun, 1, SCSI_MODESENSE_PC_CURRENT,
@@ -1278,13 +1278,12 @@ static bool iscsi_is_write_protected(IscsiLun *iscsilun) iscsi_get_error(iscsilun->iscsi)); goto out; } - wrprotected = ms->device_specific_parameter &0x80 ; + iscsilun->write_protected = ms->device_specific_parameter &0x80 ; out:if (task) {scsi_free_scsi_task (task); } -return wrprotected; }/*
@@ -1403,7 +1402,8 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,scsi_free_scsi_task (task); task =NULL ; - iscsilun->write_protected =iscsi_is_write_protected (iscsilun); +iscsi_modesense_sync (iscsilun); +/* Check the write protect flag of the LUN if we want to write */ if (iscsilun->type ==TYPE_DISK && (flags &BDRV_O_RDWR ) && iscsilun->write_protected) {
Signature · 1 line
1.9.1
PATCH v2 4/9 block/iscsi: store DPOFUA bit from the modesense command
In reply to Peter Lieven--- block/iscsi.c | 3 +++ 1 file changed, 3 insertions(+)
block/iscsi.c +3 -0
@@ -66,6 +66,7 @@ typedef struct IscsiLun {bool write_protected;bool lbpme;bool lbprz; +bool dpofua;bool has_write_same; } IscsiLun;
@@ -1258,6 +1259,7 @@ static void iscsi_modesense_sync(IscsiLun *iscsilun)struct scsi_task *task;struct scsi_mode_sense *ms =NULL ; iscsilun->write_protected = false; + iscsilun->dpofua = false; task = iscsi_modesense6_sync(iscsilun->iscsi, iscsilun->lun, 1, SCSI_MODESENSE_PC_CURRENT,
@@ -1279,6 +1281,7 @@ static void iscsi_modesense_sync(IscsiLun *iscsilun) goto out; } iscsilun->write_protected = ms->device_specific_parameter &0x80 ; + iscsilun->dpofua = ms->device_specific_parameter &0x10 ; out:if (task) {
Signature · 1 line
1.9.1
PATCH v2 5/9 block/iscsi: optimize WRITE10/16 if cache.writeback is not set
In reply to Peter LievenSCSI allowes to tell the target to not return from a write command if the date is not written to the disk. Use this so called FUA bit if it is supported to optimize WRITE commands if writeback is not allowed.
In this case qemu always issues a WRITE followed by a FLUSH. This is 2 round trip times. If we set the FUA bit we can ignore the following FLUSH.
--- block/iscsi.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
block/iscsi.c +15 -3
@@ -68,6 +68,7 @@ typedef struct IscsiLun {bool lbprz;bool dpofua;bool has_write_same; +bool force_next_flush; } IscsiLun; typedef struct IscsiTask {
@@ -80,6 +81,7 @@ typedef struct IscsiTask {QEMUBH *bh;IscsiLun *iscsilun;QEMUTimer retry_timer; +bool force_next_flush; } IscsiTask; typedef struct IscsiAIOCB {
@@ -200,6 +202,8 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, } }error_report ("iSCSI Failure: %s" ,iscsi_get_error (iscsi)); + }else { + iTask->iscsilun->force_next_flush |= iTask->force_next_flush; } out:
@@ -370,6 +374,7 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,struct IscsiTask iTask;uint64_t lba;uint32_t num_sectors; +int fua;if (!is_request_lun_aligned (sector_num, nb_sectors, iscsilun)) {return -EINVAL ;
@@ -385,15 +390,17 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs, num_sectors =sector_qemu2lun (nb_sectors, iscsilun);iscsi_co_init_iscsitask (iscsilun, &iTask); retry: + fua = iscsilun->dpofua && !bs->enable_write_cache; + iTask.force_next_flush = !fua;if (iscsilun->use_16_for_rw) { iTask.task =iscsi_write16_task (iscsilun->iscsi, iscsilun->lun, lba,NULL , num_sectors * iscsilun->block_size, - iscsilun->block_size,0 ,0 ,0 ,0 ,0 , + iscsilun->block_size,0 ,0 , fua,0 ,0 , iscsi_co_generic_cb, &iTask); }else { iTask.task =iscsi_write10_task (iscsilun->iscsi, iscsilun->lun, lba,NULL , num_sectors * iscsilun->block_size, - iscsilun->block_size,0 ,0 ,0 ,0 ,0 , + iscsilun->block_size,0 ,0 , fua,0 ,0 , iscsi_co_generic_cb, &iTask); }if (iTask.task ==NULL ) {
@@ -621,8 +628,12 @@ static int coroutine_fn iscsi_co_flush(BlockDriverState *bs)return 0 ; } -iscsi_co_init_iscsitask (iscsilun, &iTask); +if (!iscsilun->force_next_flush) { +return 0 ; + } + iscsilun->force_next_flush = false; +iscsi_co_init_iscsitask (iscsilun, &iTask); retry:if (iscsi_synchronizecache10_task (iscsilun->iscsi, iscsilun->lun,0 ,0 ,0 ,0 , iscsi_co_generic_cb, &iTask) ==NULL ) {
@@ -918,6 +929,7 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState *bs, int64_t sector_num, }iscsi_co_init_iscsitask (iscsilun, &iTask); + iTask.force_next_flush = true; retry:if (use_16_for_ws) { iTask.task = iscsi_writesame16_task(iscsilun->iscsi, iscsilun->lun, lba,
Signature · 1 line
1.9.1
PATCH v2 6/9 block/iscsi: increase retry count
In reply to Peter LievenThe idea is that a command is retried in a BUSY condition up a time of approx. 60 seconds before it is failed. This should be far higher than any command timeout in the guest.
--- block/iscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
block/iscsi.c +1 -1
@@ -103,7 +103,7 @@ typedef struct IscsiAIOCB {#define NOP_INTERVAL 5000#define MAX_NOP_FAILURES 3#define ISCSI_CMD_RETRIES ARRAY_SIZE(iscsi_retry_times) -static const unsigned iscsi_retry_times[] = {8 ,32 ,128 ,512 ,2048 }; +static const unsigned iscsi_retry_times[] = {8 ,32 ,128 ,512 ,2048 ,8192 ,32768 };/* this threshold is a trade-off knob to choose between * the potential additional overhead of an extra GET_LBA_STATUS request
Signature · 1 line
1.9.1
PATCH v2 7/9 block/iscsi: handle SCSI_STATUS_TASK_SET_FULL
In reply to Peter Lievena target may issue a SCSI_STATUS_TASK_SET_FULL status if there is more than one "BUSY" command queued already.
--- block/iscsi.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
block/iscsi.c +5 -2
@@ -186,10 +186,13 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, iTask->do_retry =1 ; goto out; } -if (status ==SCSI_STATUS_BUSY ) { +/* status 0x28 is SCSI_TASK_SET_FULL. It was first introduced +* in libiscsi 1.10.0. Hardcode this value here to avoid +* the need to bump the libiscsi requirement to 1.10.0 */ +if (status ==SCSI_STATUS_BUSY || status ==0x28 ) {unsigned retry_time =exp_random (iscsi_retry_times[iTask->retries -1 ]); -error_report ("iSCSI Busy (retry #%u in %u ms): %s" , +error_report ("iSCSI Busy/TaskSetFull (retry #%u in %u ms): %s" , iTask->retries, retry_time,iscsi_get_error (iscsi)); aio_timer_init(iTask->iscsilun->aio_context,
Signature · 1 line
1.9.1
PATCH v2 8/9 block/iscsi: bump year in copyright notice
In reply to Peter Lieven--- block/iscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
block/iscsi.c +1 -1
@@ -2,7 +2,7 @@* QEMU Block driver for iSCSI images * * Copyright (c) 2010-2011 Ronnie Sahlberg <[email protected]> -* Copyright (c) 2012-2014 Peter Lieven <[email protected]> +* Copyright (c) 2012-2015 Peter Lieven <[email protected]> * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal
Signature · 1 line
1.9.1
PATCH v2 9/9 block/iscsi: use the allocationmap also if cache.direct=on
In reply to Peter Lieventhe allocationmap has only a hint character. The driver always double checks that blocks marked unallocated in the cache are still unallocated before taking the fast path and return zeroes. So using the allocationmap is migration safe and can also be enabled with cache.direct=on.
--- block/iscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
block/iscsi.c +1 -1
@@ -1499,7 +1499,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags, iscsilun->bl.opt_unmap_gran * iscsilun->block_size <= 16 * 1024 * 1024) { iscsilun->cluster_sectors = (iscsilun->bl.opt_unmap_gran * iscsilun->block_size) >>BDRV_SECTOR_BITS ; -if (iscsilun->lbprz && !(bs->open_flags &BDRV_O_NOCACHE )) { +if (iscsilun->lbprz) { iscsilun->allocationmap =iscsi_allocationmap_init (iscsilun);if (iscsilun->allocationmap ==NULL ) { ret = -ENOMEM ;
Signature · 1 line
1.9.1
Stefan Hajnoczi
AppliedQuoted 14 lines · Peter Lieven
v1->v2: - removed the requirement for libiscsi 1.10.0 [Paolo]
- reworked toforce_next_flushlogic [Paolo]Peter Lieven (9):
block/iscsi: do not forget to logout from target block/iscsi: change all iscsilun properties fromuint8_tto bool block/iscsi: renameiscsi_write_protectedand let it return void block/iscsi: store DPOFUA bit from the modesense command block/iscsi: optimize WRITE10/16 if cache.writeback is not set block/iscsi: increase retry count
block/iscsi: handleSCSI_STATUS_TASK_SET_FULLblock/iscsi: bump year in copyright notice block/iscsi: use the allocationmap also if cache.direct=on
Quoted 7 lines
block/iscsi.c | 64 ++++++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 20 deletions(-)Signature · 1 line
1.9.1
Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next
Stefan