iscsi: support for is_allocated and inproved has_zero_init
cover iscsi: support for is_allocated and inproved has_zero_init
These two patches add the possibility for qemu-img convert to reliably skip zero blocks when writing to an iscsi target.
Peter Lieven (2):
iscsi: add support for bdrv_co_is_allocated()
iscsi: add intelligent has_zero_init check
block/iscsi.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-)
Signature · 1 line
1.7.9.5
PATCH 1/2 iscsi: add support for bdrv_co_is_allocated()
--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)
block/iscsi.c +57 -0
@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {
@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; + + *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + } + +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + } + + task =iscsi_get_lba_status_sync (iscsilun->iscsi, iscsilun->lun, +sector_qemu2lun (sector_num, iscsilun), +8 +16 ); + +if (task ==NULL || task->status !=SCSI_STATUS_GOOD ) { +scsi_free_scsi_task (task); +return 1 ; + } + + lbas =scsi_datain_unmarshall (task); +if (lbas ==NULL ) { +scsi_free_scsi_task (task); +return 1 ; + } + + lbasd = &lbas->descriptors[0 ]; + +if (sector_qemu2lun (sector_num, iscsilun) != lbasd->lba) { +return 1 ; + } + + *pnum = lbasd->num_blocks * (iscsilun->block_size /BDRV_SECTOR_SIZE ); +if (*pnum > nb_sectors) { + *pnum = nb_sectors; + } + + ret = (lbasd->provisioning ==SCSI_PROVISIONING_TYPE_MAPPED ) ?1 :0 ; + +scsi_free_scsi_task (task); + +return ret; +} +static int parse_chap (struct iscsi_context *iscsi,const char *target) {QemuOptsList *list;
@@ -948,6 +1003,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun) }else { iscsilun->block_size = rc16->block_length; iscsilun->num_blocks = rc16->returned_lba +1 ; + iscsilun->lbpme = rc16->lbpme; } }break ;
@@ -1274,6 +1330,7 @@ static BlockDriver bdrv_iscsi = { .bdrv_aio_discard = iscsi_aio_discard, .bdrv_has_zero_init = iscsi_has_zero_init, + .bdrv_co_is_allocated = iscsi_co_is_allocated, #ifdef __linux__ .bdrv_ioctl = iscsi_ioctl,
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 31 lines
Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)block/iscsi.c +11
-0@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; +
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 10 lines
block/iscsi.c +10
-0+ *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + } + +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }
Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 827
Quoted 4 lines
block/iscsi.c +4
-0+ + task =iscsi_get_lba_status_sync (iscsilun->iscsi, iscsilun->lun, +sector_qemu2lun (sector_num, iscsilun), +8 +16 );
Spacing around operators (8 + 16)
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 831
Quoted 4 lines
block/iscsi.c +4
-0+ +if (task ==NULL || task->status !=SCSI_STATUS_GOOD ) { +scsi_free_scsi_task (task); +return 1 ;
Error cases should set *pnum = 0 and return 0.
Kevin
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 21.06.2013 11:18, schrieb Kevin Wolf:
Quoted 37 lines
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 36 lines
Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)block/iscsi.c +16
-0@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; + + *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + }
Quoted 8 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 5 lines
block/iscsi.c +5
-0+ +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Is there a document how to use it? Or can you help here?
Quoted 6 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 827
Quoted 5 lines
block/iscsi.c +4
-0+ + task =iscsi_get_lba_status_sync (iscsilun->iscsi, iscsilun->lun, +sector_qemu2lun (sector_num, iscsilun), +8 +16 );Spacing around operators (8 + 16)
Thanks, the checkpatch script didn't catch this.
Quoted 6 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 831
Quoted 5 lines
block/iscsi.c +4
-0+ +if (task ==NULL || task->status !=SCSI_STATUS_GOOD ) { +scsi_free_scsi_task (task); +return 1 ;Error cases should set *pnum = 0 and return 0.
In this special case, the target might not implement get_lba_status or it might return SCSI_STATUS_BUSY. We shouldn't generate an error or should we?
Peter
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 21.06.2013 um 11:45 hat Peter Lieven geschrieben:
Quoted 46 lines
Am 21.06.2013 11:18, schrieb Kevin Wolf:
Quoted 37 lines
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 36 lines
Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)block/iscsi.c +16
-0@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; + + *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + }Quoted 8 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 5 lines
block/iscsi.c +5
-0+ +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Quoted 1 line
Is there a document how to use it? Or can you help here?
The idea would be to take a read lock while any request is in flight (i.e. qemu_co_rwlock_rdlock() before it's started and qemu_co_rwlock_unlock() when it completes), and to take a write lock (qemu_co_rwlock_wrlock) for the part of iscsi_co_is_allocated() that requires that no other request runs in parallel.
Quoted 7 lines
Quoted 4 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 831
Quoted 3 lines
block/iscsi.c +3
-0+if (task ==NULL || task->status !=SCSI_STATUS_GOOD ) { +scsi_free_scsi_task (task); +return 1 ;Error cases should set *pnum = 0 and return 0.
In this special case, the target might not implement
get_lba_statusor it might returnSCSI_STATUS_BUSY. We shouldn't generate an error or should we?
If you have reasons to not return an error, do so. Maybe adding a comment wouldn't hurt.
I just saw more than one of these return 1; lines which looked like error handling to me. If you don't want any of them to result in an error, that's okay with me, I just wanted to mention how you would correctly signal an error.
Kevin
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 21.06.2013 13:07, schrieb Kevin Wolf:
Quoted 47 lines
Am 21.06.2013 um 11:45 hat Peter Lieven geschrieben:
Quoted 46 lines
Am 21.06.2013 11:18, schrieb Kevin Wolf:
Quoted 45 lines
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 31 lines
Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)block/iscsi.c +11
-0@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; +block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 10 lines
block/iscsi.c +10
-0+ *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + } + +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Quoted 6 lines
Quoted 1 line
Is there a document how to use it? Or can you help here?
The idea would be to take a read lock while any request is in flight (i.e.
qemu_co_rwlock_rdlock()before it's started andqemu_co_rwlock_unlock()when it completes), and to take a write lock (qemu_co_rwlock_wrlock) for the part ofiscsi_co_is_allocated()that requires that no other request runs in parallel.
Wouldn't this require that all the other operations in iscsi.c would also take these lock? Currently there are only aio requests implemented as it seems.
Would it help here to add sth like this?
while (iscsi_process_flush(iscsilun)) {
if (qemu_in_coroutine()) {
qemu_coroutine_yield();
} else {
qemu_aio_wait();
}
}Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 831
Quoted 3 lines
block/iscsi.c +3
-0+if (task ==NULL || task->status !=SCSI_STATUS_GOOD ) { +scsi_free_scsi_task (task); +return 1 ;Error cases should set *pnum = 0 and return 0.
In this special case, the target might not implement
get_lba_statusor it might returnSCSI_STATUS_BUSY. We shouldn't generate an error or should we?If you have reasons to not return an error, do so. Maybe adding a comment wouldn't hurt.
Will do ;-)
Quoted 5 lines
I just saw more than one of these return 1; lines which looked like error handling to me. If you don't want any of them to result in an error, that's okay with me, I just wanted to mention how you would correctly signal an error.
Ah okay. Thank you.
Peter
Quoted 2 lines
Kevin
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 21.06.2013 um 13:42 hat Peter Lieven geschrieben:
Quoted 49 lines
Am 21.06.2013 13:07, schrieb Kevin Wolf:
Quoted 48 lines
Am 21.06.2013 um 11:45 hat Peter Lieven geschrieben:
Quoted 47 lines
Am 21.06.2013 11:18, schrieb Kevin Wolf:
Quoted 37 lines
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 36 lines
Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)block/iscsi.c +16
-0@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; + + *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + }Quoted 8 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 5 lines
block/iscsi.c +5
-0+ +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Is there a document how to use it? Or can you help here?
Quoted 8 lines
Quoted 5 lines
The idea would be to take a read lock while any request is in flight (i.e.
qemu_co_rwlock_rdlock()before it's started andqemu_co_rwlock_unlock()when it completes), and to take a write lock (qemu_co_rwlock_wrlock) for the part ofiscsi_co_is_allocated()that requires that no other request runs in parallel.Wouldn't this require that all the other operations in
iscsi.cwould also take these lock? Currently there are only aio requests implemented as it seems.
Hm, okay, that makes it a bit harder because AIO callbacks aren't called in coroutine context. So taking the lock would be easy, but releasing them could turn out somewhat tricky.
Quoted 9 lines
Would it help here to add sth like this?
while (iscsi_process_flush(iscsilun)) { if (qemu_in_coroutine()) { qemu_coroutine_yield(); } else { qemu_aio_wait(); } }
You're always in a coroutine here.
The problem is that if you yield, you need to reenter the coroutine at some point or the is_allocated request would never complete. That's basically what the coroutine locks do for you: They yield and only reenter when the lock can be taken.
Kevin
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 21.06.2013 15:14, schrieb Kevin Wolf:
Quoted 71 lines
Am 21.06.2013 um 13:42 hat Peter Lieven geschrieben:
Quoted 49 lines
Am 21.06.2013 13:07, schrieb Kevin Wolf:
Quoted 48 lines
Am 21.06.2013 um 11:45 hat Peter Lieven geschrieben:
Quoted 47 lines
Am 21.06.2013 11:18, schrieb Kevin Wolf:
Quoted 37 lines
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 36 lines
Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)block/iscsi.c +16
-0@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; + + *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + }Quoted 8 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 5 lines
block/iscsi.c +5
-0+ +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Is there a document how to use it? Or can you help here?
Quoted 8 lines
Quoted 5 lines
The idea would be to take a read lock while any request is in flight (i.e.
qemu_co_rwlock_rdlock()before it's started andqemu_co_rwlock_unlock()when it completes), and to take a write lock (qemu_co_rwlock_wrlock) for the part ofiscsi_co_is_allocated()that requires that no other request runs in parallel.Wouldn't this require that all the other operations in
iscsi.cwould also take these lock? Currently there are only aio requests implemented as it seems.Hm, okay, that makes it a bit harder because AIO callbacks aren't called in coroutine context. So taking the lock would be easy, but releasing them could turn out somewhat tricky.
Quoted 9 lines
Would it help here to add sth like this?
while (iscsi_process_flush(iscsilun)) { if (qemu_in_coroutine()) { qemu_coroutine_yield(); } else { qemu_aio_wait(); } }
Quoted 6 lines
You're always in a coroutine here.
The problem is that if you yield, you need to reenter the coroutine at some point or the
is_allocatedrequest would never complete. That's basically what the coroutine locks do for you: They yield and only reenter when the lock can be taken.
would it ok for you to stay with qemu_aio_wait() and add a remark that it blocks and its a TODO. i need the is_allocated support in iscsi only to improve bdrv_has_zero_init().
Peter
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
In reply to Kevin WolfIl 21/06/2013 13:07, Kevin Wolf ha scritto:
Quoted 10 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Is there a document how to use it? Or can you help here?
The idea would be to take a read lock while any request is in flight (i.e.
qemu_co_rwlock_rdlock()before it's started andqemu_co_rwlock_unlock()when it completes), and to take a write lock (qemu_co_rwlock_wrlock) for the part ofiscsi_co_is_allocated()that requires that no other request runs in parallel.
You can just send the SCSI command asynchronously and wait for the result. There is an example in block/qed.c, the same would apply for iscsi.
Paolo
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 21.06.2013 18:31, schrieb Paolo Bonzini:
Quoted 5 lines
Il 21/06/2013 13:07, Kevin Wolf ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Is there a document how to use it? Or can you help here?
Quoted 8 lines
Quoted 6 lines
The idea would be to take a read lock while any request is in flight (i.e.
qemu_co_rwlock_rdlock()before it's started andqemu_co_rwlock_unlock()when it completes), and to take a write lock (qemu_co_rwlock_wrlock) for the part ofiscsi_co_is_allocated()that requires that no other request runs in parallel.You can just send the SCSI command asynchronously and wait for the result. There is an example in
block/qed.c, the same would apply for iscsi.
thanks for the pointer paolo, this was i was looking for. this here seems to work:
static void
iscsi_co_is_allocated_cb(struct iscsi_context *iscsi, int status,
void *command_data, void *opaque)
{
struct IscsiTask *iTask = opaque;
struct scsi_task *task = command_data;
struct scsi_get_lba_status *lbas = NULL;
iTask->complete = 1;
if (status != 0) {
error_report("iSCSI: Failed to get_lba_status on iSCSI lun. %s",
iscsi_get_error(iscsi));
iTask->status = 1;
goto out;
}
lbas = scsi_datain_unmarshall(task);
if (lbas == NULL) {
iTask->status = 1;
goto out;
}
memcpy(&iTask->lbasd, &lbas->descriptors[0],
sizeof(struct scsi_lba_status_descriptor));
iTask->status = 0;out:
scsi_free_scsi_task(task);
if (iTask->co) {
qemu_coroutine_enter(iTask->co, NULL);
}
}static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, int *pnum)
{
IscsiLun *iscsilun = bs->opaque;
struct IscsiTask iTask;
int ret;
*pnum = nb_sectors;
if (iscsilun->lbpme == 0) {
return 1;
}
iTask.iscsilun = iscsilun;
iTask.status = 0;
iTask.complete = 0;
iTask.bs = bs;
if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun,
sector_qemu2lun(sector_num, iscsilun),
8 + 16, iscsi_co_is_allocated_cb,
&iTask) == NULL) {
*pnum = 0;
return 0;
}
while (!iTask.complete) {
iscsi_set_events(iscsilun);
iTask.co = qemu_coroutine_self();
qemu_coroutine_yield();
}
if (iTask.status != 0) {
/* in case the get_lba_status_callout fails (i.e.
* because the device is busy or the cmd is not
* supported) we pretend all blocks are allocated
* for backwards compatiblity */
return 1;
}
if (sector_qemu2lun(sector_num, iscsilun) != iTask.lbasd.lba) {
*pnum = 0;
return 0;
}
*pnum = iTask.lbasd.num_blocks * (iscsilun->block_size / BDRV_SECTOR_SIZE);
if (*pnum > nb_sectors) {
*pnum = nb_sectors;
}
return (iTask.lbasd.provisioning == SCSI_PROVISIONING_TYPE_MAPPED) ? 1 : 0;
return ret;
}Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Quoted 34 lines · Peter Lieven
Am 21.06.2013 18:31, schrieb Paolo Bonzini:
Quoted 5 lines
Il 21/06/2013 13:07, Kevin Wolf ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Is there a document how to use it? Or can you help here?
Quoted 8 lines
Quoted 6 lines
The idea would be to take a read lock while any request is in flight (i.e.
qemu_co_rwlock_rdlock()before it's started andqemu_co_rwlock_unlock()when it completes), and to take a write lock (qemu_co_rwlock_wrlock) for the part ofiscsi_co_is_allocated()that requires that no other request runs in parallel.You can just send the SCSI command asynchronously and wait for the result. There is an example in
block/qed.c, the same would apply for iscsi.thanks for the pointer paolo, this was i was looking for. this here seems to work:
static void
iscsi_co_is_allocated_cb(structiscsi_context*iscsi, int status,
void*command_data, void *opaque){ struct IscsiTask *iTask = opaque; struct scsi_task *task = command_data; struct scsi_get_lba_status *lbas = NULL; iTask->complete = 1; if (status != 0) { error_report("iSCSI: Failed to get_lba_status on iSCSI lun. %s", iscsi_get_error(iscsi)); iTask->status = 1; goto out; }
Quoted 8 lines
lbas = scsi_datain_unmarshall(task);
if (lbas == NULL) {
iTask->status = 1;
goto out;
}
memcpy(&iTask->lbasd, &lbas->descriptors[0],
sizeof(struct scsi_lba_status_descriptor));Only the first descriptor?sector_num -> sector_num+nb_sectors could be partially allocated in which case you get multiple descriptors
Quoted 66 lines
iTask->status = 0;out:
scsi_free_scsi_task(task); if (iTask->co) { qemu_coroutine_enter(iTask->co, NULL); } }static int
coroutine_fniscsi_co_is_allocated(BlockDriverState *bs,
int64_tsector_num, intnb_sectors, int *pnum){ IscsiLun *iscsilun = bs->opaque; struct IscsiTask iTask; int ret; *pnum = nb_sectors; if (iscsilun->lbpme == 0) { return 1; } iTask.iscsilun = iscsilun; iTask.status = 0; iTask.complete = 0; iTask.bs = bs; if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun, sector_qemu2lun(sector_num, iscsilun), 8 + 16, iscsi_co_is_allocated_cb, &iTask) == NULL) { *pnum = 0; return 0; } while (!iTask.complete) { iscsi_set_events(iscsilun); iTask.co = qemu_coroutine_self(); qemu_coroutine_yield(); } if (iTask.status != 0) { /* in case the get_lba_status_callout fails (i.e. * because the device is busy or the cmd is not * supported) we pretend all blocks are allocated * for backwards compatiblity */ return 1; } if (sector_qemu2lun(sector_num, iscsilun) != iTask.lbasd.lba) { *pnum = 0; return 0; } *pnum = iTask.lbasd.num_blocks * (iscsilun->block_size / BDRV_SECTOR_SIZE); if (*pnum > nb_sectors) { *pnum = nb_sectors; } return (iTask.lbasd.provisioning == SCSI_PROVISIONING_TYPE_MAPPED) ? 1 : 0; return ret; }
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 21.06.2013 19:13, schrieb ronnie sahlberg:
Quoted 40 lines · Peter Lieven
Quoted 39 lines
Am 21.06.2013 18:31, schrieb Paolo Bonzini:
Quoted 5 lines
Il 21/06/2013 13:07, Kevin Wolf ha scritto:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Is there a document how to use it? Or can you help here?
Quoted 8 lines
Quoted 6 lines
The idea would be to take a read lock while any request is in flight (i.e.
qemu_co_rwlock_rdlock()before it's started andqemu_co_rwlock_unlock()when it completes), and to take a write lock (qemu_co_rwlock_wrlock) for the part ofiscsi_co_is_allocated()that requires that no other request runs in parallel.You can just send the SCSI command asynchronously and wait for the result. There is an example in
block/qed.c, the same would apply for iscsi.thanks for the pointer paolo, this was i was looking for. this here seems to work:
static void
iscsi_co_is_allocated_cb(structiscsi_context*iscsi, int status,
void*command_data, void *opaque){ struct IscsiTask *iTask = opaque; struct scsi_task *task = command_data; struct scsi_get_lba_status *lbas = NULL; iTask->complete = 1; if (status != 0) { error_report("iSCSI: Failed to get_lba_status on iSCSI lun. %s", iscsi_get_error(iscsi)); iTask->status = 1; goto out; } lbas = scsi_datain_unmarshall(task); if (lbas == NULL) { iTask->status = 1; goto out; }
Quoted 5 lines
Quoted 2 lines
memcpy(&iTask->lbasd, &lbas->descriptors[0], sizeof(struct scsi_lba_status_descriptor));Only the first descriptor?
sector_num-> sector_num+nb_sectors could be partially allocated in which case you get multiple descriptors.
The number of sectors for which the returned provisioning state holds true is set in *pnum. If it is only partly allocated the return value of iscsi_co_is_allocated will be 1 or 0 and pnum returns the number of sectors for which this is true.
Quoted 65 lines
Quoted 65 lines
iTask->status = 0;out:
scsi_free_scsi_task(task); if (iTask->co) { qemu_coroutine_enter(iTask->co, NULL); } }static int
coroutine_fniscsi_co_is_allocated(BlockDriverState *bs,
int64_tsector_num, intnb_sectors, int *pnum){ IscsiLun *iscsilun = bs->opaque; struct IscsiTask iTask; int ret; *pnum = nb_sectors; if (iscsilun->lbpme == 0) { return 1; } iTask.iscsilun = iscsilun; iTask.status = 0; iTask.complete = 0; iTask.bs = bs; if (iscsi_get_lba_status_task(iscsilun->iscsi, iscsilun->lun, sector_qemu2lun(sector_num, iscsilun), 8 + 16, iscsi_co_is_allocated_cb, &iTask) == NULL) { *pnum = 0; return 0; } while (!iTask.complete) { iscsi_set_events(iscsilun); iTask.co = qemu_coroutine_self(); qemu_coroutine_yield(); } if (iTask.status != 0) { /* in case the get_lba_status_callout fails (i.e. * because the device is busy or the cmd is not * supported) we pretend all blocks are allocated * for backwards compatiblity */ return 1; } if (sector_qemu2lun(sector_num, iscsilun) != iTask.lbasd.lba) { *pnum = 0; return 0; } *pnum = iTask.lbasd.num_blocks * (iscsilun->block_size / BDRV_SECTOR_SIZE); if (*pnum > nb_sectors) { *pnum = nb_sectors; } return (iTask.lbasd.provisioning == SCSI_PROVISIONING_TYPE_MAPPED) ? 1 : 0; return ret; }
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
In reply to Kevin WolfShould we really mix co-routines and AIO in the same backend?
Would it not be better to instead add a new bdrb_aio_is_allocaed and use non-blocking async calls to libiscsi ?
Quoted 62 lines · Kevin Wolf
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 31 lines
Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)block/iscsi.c +11
-0@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; +block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 10 lines
block/iscsi.c +10
-0+ *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + } + +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 827
Quoted 4 lines
block/iscsi.c +4
-0+ + task =iscsi_get_lba_status_sync (iscsilun->iscsi, iscsilun->lun, +sector_qemu2lun (sector_num, iscsilun), +8 +16 );Spacing around operators (8 + 16)
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 831
Quoted 4 lines
block/iscsi.c +4
-0+ +if (task ==NULL || task->status !=SCSI_STATUS_GOOD ) { +scsi_free_scsi_task (task); +return 1 ;Error cases should set *pnum = 0 and return 0.
Kevin
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Il 21/06/2013 18:06, ronnie sahlberg ha scritto:
Quoted 4 lines
Should we really mix co-routines and AIO in the same backend?
Would it not be better to instead add a new
bdrb_aio_is_allocaedand use non-blocking async calls to libiscsi ?
Certainly, but is_allocated's code is not the tidiest.
I'm going to replace the is_allocated callback with a more generic one (Peter knows, and Kevin might have suspected it too), and I will do the cleanups at the time I do that.
Paolo
Quoted 66 lines · Kevin Wolf
Quoted 62 lines
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 31 lines
Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)block/iscsi.c +11
-0@@ -49,6 +49,7 @@ typedef struct IscsiLun {uint64_t num_blocks;int events;QEMUTimer *nop_timer; +uint8_t lbpme; } IscsiLun; typedef struct IscsiAIOCB {@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; +block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 10 lines
block/iscsi.c +10
-0+ *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + } + +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 827
Quoted 4 lines
block/iscsi.c +4
-0+ + task =iscsi_get_lba_status_sync (iscsilun->iscsi, iscsilun->lun, +sector_qemu2lun (sector_num, iscsilun), +8 +16 );Spacing around operators (8 + 16)
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 831
Quoted 4 lines
block/iscsi.c +4
-0+ +if (task ==NULL || task->status !=SCSI_STATUS_GOOD ) { +scsi_free_scsi_task (task); +return 1 ;Error cases should set *pnum = 0 and return 0.
Kevin
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
In reply to Kevin WolfQuoted 21 lines · Kevin Wolf
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 20 lines
block/iscsi.c +16
-0@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; + + *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + } +
Quoted 8 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 4 lines
block/iscsi.c +4
-0+/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
The other option is to avoid synchronization here and instead process bs->tracked_requests so that any in-flight writes count as allocated.
Stefan
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Il 24/06/2013 10:13, Stefan Hajnoczi ha scritto:
Quoted 31 lines · Kevin Wolf
Quoted 21 lines
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 20 lines
block/iscsi.c +16
-0@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; + + *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + } +Quoted 8 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 4 lines
block/iscsi.c +4
-0+/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Quoted 2 lines
The other option is to avoid synchronization here and instead process
bs->tracked_requestsso that any in-flight writes count as allocated.
I think it's a bug if the caller doesn't take into account in-flight requests. For example mirroring expects writes to mark sectors as dirty, which will pick up everything that is_allocated fails to pick up.
If all else fails, you can always add a bdrv_drain_all before the query.
Hence, this check is not needed. In fact, raw-posix does not perform it.
Paolo
Re: [Qemu-devel] [PATCH 1/2] iscsi: add support for bdrv_co_is_allocated()
Am 24.06.2013 15:49, schrieb Paolo Bonzini:
Quoted 36 lines
Il 24/06/2013 10:13, Stefan Hajnoczi ha scritto:
Quoted 29 lines · Kevin Wolf
Quoted 20 lines
Am 20.06.2013 um 20:20 hat Peter Lieven geschrieben:
Quoted 19 lines
block/iscsi.c +15
-0@@ -800,6 +801,60 @@ iscsi_getlength(BlockDriverState *bs)return len; } +static int coroutine_fn iscsi_co_is_allocated(BlockDriverState *bs, +int64_t sector_num, +int nb_sectors,int *pnum) +{ +IscsiLun *iscsilun = bs->opaque; +struct scsi_task *task =NULL ; +struct scsi_get_lba_status *lbas =NULL ; +struct scsi_lba_status_descriptor *lbasd =NULL ; +int ret; + + *pnum = nb_sectors; + +if (iscsilun->lbpme ==0 ) { +return 1 ; + }Quoted 8 lines
block/iscsi.c iscsi_getlength(BlockDriverState *bs) · line 823
Quoted 5 lines
block/iscsi.c +5
-0+ +/* in-flight requests could invalidate the lba status result */ +while (iscsi_process_flush (iscsilun)) { +qemu_aio_wait (); + }Note that you're blocking here. The preferred way would be something involving a yield from the coroutine and a reenter as soon as all requests are done. Maybe a CoRwLock does what you need?
Quoted 2 lines
The other option is to avoid synchronization here and instead process
bs->tracked_requestsso that any in-flight writes count as allocated.I think it's a bug if the caller doesn't take into account in-flight requests. For example mirroring expects writes to mark sectors as dirty, which will pick up everything that
is_allocatedfails to pick up.
Quoted 5 lines
If all else fails, you can always add a
bdrv_drain_allbefore the query.Hence, this check is not needed. In fact, raw-posix does not perform it.
Paolo
Please ignore this patch and look at the async version in the series which also contained the write zero optimizations.
thanks,
Peter
PATCH 2/2 iscsi: add intelligent has_zero_init check
In reply to Peter Lieveniscsi targets are not created by bdrv_create and thus we cannot blindly assume that a target is empty. to avoid writing and allocating blocks of zeroes we now check if all blocks of an existing target are unallocated and return 1 for bdrv_has_zero_init if the target is completely unalloacted and unallocated blocks read as zeroes.
--- block/iscsi.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
block/iscsi.c +24 -1
@@ -50,6 +50,7 @@ typedef struct IscsiLun {int events;QEMUTimer *nop_timer;uint8_t lbpme; +uint8_t lbprz; } IscsiLun; typedef struct IscsiAIOCB {
@@ -1004,6 +1005,7 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun) iscsilun->block_size = rc16->block_length; iscsilun->num_blocks = rc16->returned_lba +1 ; iscsilun->lbpme = rc16->lbpme; + iscsilun->lbprz = rc16->lbprz; } }break ;
@@ -1249,7 +1251,28 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)static int iscsi_has_zero_init (BlockDriverState *bs) { -return 0 ; +IscsiLun *iscsilun = bs->opaque; +uint64_t lba; +int n, ret, nb_sectors; + +if (iscsilun->lbprz ==0 ) { +return 0 ; + } + +for (lba =0 ; lba < iscsilun->num_blocks; lba +=1 <<26 ) { + nb_sectors =1 <<26 ; +if (lba + nb_sectors > iscsilun->num_blocks) { + nb_sectors = iscsilun->num_blocks - lba; + } + nb_sectors *= (iscsilun->block_size /BDRV_SECTOR_SIZE ); + n =0 ; + ret =iscsi_co_is_allocated (bs, lba, nb_sectors, &n); +if (ret || n != nb_sectors) { +return 0 ; + } + } + +return 1 ; }static int iscsi_create (const char *filename,QEMUOptionParameter *options)
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCH 2/2] iscsi: add intelligent has_zero_init check
Il 20/06/2013 20:20, Peter Lieven ha scritto:
block/iscsi.c static int iscsi_truncate(BlockDriverState *bs, int64_t offset) · line 1272
Quoted 11 lines
block/iscsi.c +11
-0+for (lba =0 ; lba < iscsilun->num_blocks; lba +=1 <<26 ) { + nb_sectors =1 <<26 ; +if (lba + nb_sectors > iscsilun->num_blocks) { + nb_sectors = iscsilun->num_blocks - lba; + } + nb_sectors *= (iscsilun->block_size /BDRV_SECTOR_SIZE ); + n =0 ; + ret =iscsi_co_is_allocated (bs, lba, nb_sectors, &n); +if (ret || n != nb_sectors) { +return 0 ; + }
I would just do lba += n in the for loop, and only exit if n == 0. The SCSI spec does not forbid splitting a single allocated area into multiple descriptors, or only returning part of an allocated area into the last descriptor.
Otherwise looks good, but you may want to cache the result. It would not be 1 anymore after the first write.
Paolo
Re: [Qemu-devel] [PATCH 2/2] iscsi: add intelligent has_zero_init check
Von meinem iPhone gesendet
Am 21.06.2013 um 22:00 schrieb Paolo Bonzini <[email protected]>:
Quoted 13 lines
Il 20/06/2013 20:20, Peter Lieven ha scritto:
block/iscsi.c static int iscsi_truncate(BlockDriverState *bs, int64_t offset) · line 1272
Quoted 11 lines
block/iscsi.c +11
-0+for (lba =0 ; lba < iscsilun->num_blocks; lba +=1 <<26 ) { + nb_sectors =1 <<26 ; +if (lba + nb_sectors > iscsilun->num_blocks) { + nb_sectors = iscsilun->num_blocks - lba; + } + nb_sectors *= (iscsilun->block_size /BDRV_SECTOR_SIZE ); + n =0 ; + ret =iscsi_co_is_allocated (bs, lba, nb_sectors, &n); +if (ret || n != nb_sectors) { +return 0 ; + }
Quoted 8 lines
I would just do lba += n in the for loop, and only exit if n == 0. The SCSI spec does not forbid splitting a single allocated area into multiple descriptors, or only returning part of an allocated area into the last descriptor.
Otherwise looks good, but you may want to cache the result. It would not be 1 anymore after the first write.
Of course, but after some discussions with Kevin we might have found a better Solution than extending has_zero_init this far.
Let you know soon :-)
Quoted 1 line
Paolo