[Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Peter Lieven]

ensure that there are no pending I/Os before calling
the sync readcapacity commands. the block_resize monitor
command will also flush all I/O, but double check in
case iscsi_truncate() is called from elsewhere in the
future.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
  block/iscsi.c |    4 ++++
  1 file changed, 4 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 3d52921..de20d53 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1167,6 +1167,10 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
          return -ENOTSUP;
      }

+    /* ensure all async requests are completed before executing
+     * a sync readcapacity */
+    bdrv_drain_all();
+
      if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
          return ret;
      }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Paolo Bonzini]

Il 11/03/2013 11:05, Peter Lieven ha scritto:
> ensure that there are no pending I/Os before calling
> the sync readcapacity commands. the block_resize monitor
> command will also flush all I/O, but double check in
> case iscsi_truncate() is called from elsewhere in the
> future.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 3d52921..de20d53 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1167,6 +1167,10 @@ static int iscsi_truncate(BlockDriverState *bs,
> int64_t offset)
>          return -ENOTSUP;
>      }
> 
> +    /* ensure all async requests are completed before executing
> +     * a sync readcapacity */
> +    bdrv_drain_all();
> +
>      if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
>          return ret;
>      }

NACK to this patch.  It would be a bug, let's fix it properly.

The other two are fine, however.

Paolo

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Peter Lieven]

Am 11.03.2013 um 11:16 schrieb Paolo Bonzini <pbonzini@redhat.com>:

> Il 11/03/2013 11:05, Peter Lieven ha scritto:
>> ensure that there are no pending I/Os before calling
>> the sync readcapacity commands. the block_resize monitor
>> command will also flush all I/O, but double check in
>> case iscsi_truncate() is called from elsewhere in the
>> future.
>> 
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> block/iscsi.c |    4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 3d52921..de20d53 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1167,6 +1167,10 @@ static int iscsi_truncate(BlockDriverState *bs,
>> int64_t offset)
>>         return -ENOTSUP;
>>     }
>> 
>> +    /* ensure all async requests are completed before executing
>> +     * a sync readcapacity */
>> +    bdrv_drain_all();
>> +
>>     if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
>>         return ret;
>>     }
> 
> NACK to this patch.  It would be a bug, let's fix it properly.

ok, are you sure that never ever will some new feature call bdrv_truncate() on an iscsi
device?

otherwise the real fix would be to implement async read capacity commands like it
the first patch version for iscsi_truncate().

Peter


> 
> The other two are fine, however.
> 
> Paolo

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Paolo Bonzini]

Il 11/03/2013 11:19, Peter Lieven ha scritto:
> 
> Am 11.03.2013 um 11:16 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
>> Il 11/03/2013 11:05, Peter Lieven ha scritto:
>>> ensure that there are no pending I/Os before calling
>>> the sync readcapacity commands. the block_resize monitor
>>> command will also flush all I/O, but double check in
>>> case iscsi_truncate() is called from elsewhere in the
>>> future.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>> block/iscsi.c |    4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index 3d52921..de20d53 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -1167,6 +1167,10 @@ static int iscsi_truncate(BlockDriverState *bs,
>>> int64_t offset)
>>>         return -ENOTSUP;
>>>     }
>>>
>>> +    /* ensure all async requests are completed before executing
>>> +     * a sync readcapacity */
>>> +    bdrv_drain_all();
>>> +
>>>     if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
>>>         return ret;
>>>     }
>>
>> NACK to this patch.  It would be a bug, let's fix it properly.
> 
> ok, are you sure that never ever will some new feature call bdrv_truncate() on an iscsi
> device?

I'm sure some new feature _will_ call bdrv_truncate().  But as things
stand now, it would be a bug to do so on an iSCSI device.

For example, qcow1 (and VHDX) will already call it, but that's a bug and
should be fixed otherwise.  Your patch will just cause an assertion failure.

Paolo

> otherwise the real fix would be to implement async read capacity commands like it
> the first patch version for iscsi_truncate().
> 
> Peter
> 
> 
>>
>> The other two are fine, however.
>>
>> Paolo
> 

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Peter Lieven]

On 11.03.2013 12:44, Paolo Bonzini wrote:
> Il 11/03/2013 11:19, Peter Lieven ha scritto:
>>
>> Am 11.03.2013 um 11:16 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>>
>>> Il 11/03/2013 11:05, Peter Lieven ha scritto:
>>>> ensure that there are no pending I/Os before calling
>>>> the sync readcapacity commands. the block_resize monitor
>>>> command will also flush all I/O, but double check in
>>>> case iscsi_truncate() is called from elsewhere in the
>>>> future.
>>>>
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>> block/iscsi.c |    4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>> index 3d52921..de20d53 100644
>>>> --- a/block/iscsi.c
>>>> +++ b/block/iscsi.c
>>>> @@ -1167,6 +1167,10 @@ static int iscsi_truncate(BlockDriverState *bs,
>>>> int64_t offset)
>>>>          return -ENOTSUP;
>>>>      }
>>>>
>>>> +    /* ensure all async requests are completed before executing
>>>> +     * a sync readcapacity */
>>>> +    bdrv_drain_all();
>>>> +
>>>>      if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
>>>>          return ret;
>>>>      }
>>>
>>> NACK to this patch.  It would be a bug, let's fix it properly.
>>
>> ok, are you sure that never ever will some new feature call bdrv_truncate() on an iscsi
>> device?
>
> I'm sure some new feature _will_ call bdrv_truncate().  But as things
> stand now, it would be a bug to do so on an iSCSI device.
>
> For example, qcow1 (and VHDX) will already call it, but that's a bug and
> should be fixed otherwise.  Your patch will just cause an assertion failure.

In which case can qcow1 (and VHDX) be used in conjunction with an iSCSI Target?

Would it be ok for you to assert if iscsi_truncate() is called when there
are aio's in flight? How can this be checked?

I just want to make sure that nothing nasty happens in that case (such as
nested event loops).

Peter

>
> Paolo
>
>> otherwise the real fix would be to implement async read capacity commands like it
>> the first patch version for iscsi_truncate().
>>
>> Peter
>>
>>
>>>
>>> The other two are fine, however.
>>>
>>> Paolo
>>
>

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Paolo Bonzini]

Il 11/03/2013 12:52, Peter Lieven ha scritto:
>>
>> For example, qcow1 (and VHDX) will already call it, but that's a bug and
>> should be fixed otherwise.  Your patch will just cause an assertion
>> failure.
> 
> In which case can qcow1 (and VHDX) be used in conjunction with an iSCSI
> Target?

No.  It is a bug that they are accepted, but we have time to fix it
before 1.5.

Paolo

> Would it be ok for you to assert if iscsi_truncate() is called when there
> are aio's in flight? How can this be checked?
> 
> I just want to make sure that nothing nasty happens in that case (such as
> nested event loops).

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Kevin Wolf]

Am 11.03.2013 um 12:52 hat Peter Lieven geschrieben:
> On 11.03.2013 12:44, Paolo Bonzini wrote:
> >Il 11/03/2013 11:19, Peter Lieven ha scritto:
> >>
> >>Am 11.03.2013 um 11:16 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> >>
> >>>Il 11/03/2013 11:05, Peter Lieven ha scritto:
> >>>>ensure that there are no pending I/Os before calling
> >>>>the sync readcapacity commands. the block_resize monitor
> >>>>command will also flush all I/O, but double check in
> >>>>case iscsi_truncate() is called from elsewhere in the
> >>>>future.
> >>>>
> >>>>Signed-off-by: Peter Lieven <pl@kamp.de>
> >>>>---
> >>>>block/iscsi.c |    4 ++++
> >>>>1 file changed, 4 insertions(+)
> >>>>
> >>>>diff --git a/block/iscsi.c b/block/iscsi.c
> >>>>index 3d52921..de20d53 100644
> >>>>--- a/block/iscsi.c
> >>>>+++ b/block/iscsi.c
> >>>>@@ -1167,6 +1167,10 @@ static int iscsi_truncate(BlockDriverState *bs,
> >>>>int64_t offset)
> >>>>         return -ENOTSUP;
> >>>>     }
> >>>>
> >>>>+    /* ensure all async requests are completed before executing
> >>>>+     * a sync readcapacity */
> >>>>+    bdrv_drain_all();
> >>>>+
> >>>>     if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
> >>>>         return ret;
> >>>>     }
> >>>
> >>>NACK to this patch.  It would be a bug, let's fix it properly.
> >>
> >>ok, are you sure that never ever will some new feature call bdrv_truncate() on an iscsi
> >>device?
> >
> >I'm sure some new feature _will_ call bdrv_truncate().  But as things
> >stand now, it would be a bug to do so on an iSCSI device.
> >
> >For example, qcow1 (and VHDX) will already call it, but that's a bug and
> >should be fixed otherwise.  Your patch will just cause an assertion failure.
> 
> In which case can qcow1 (and VHDX) be used in conjunction with an iSCSI Target?
> 
> Would it be ok for you to assert if iscsi_truncate() is called when there
> are aio's in flight? How can this be checked?
> 
> I just want to make sure that nothing nasty happens in that case (such as
> nested event loops).

Isn't the real problem that I/O requests for _this_specific_ iscsi BDS
must not be in flight? So what you reall need is bdrv_drain(iscsi_bs)?

If I understand the code correctly, this boils down to:

    while (iscsi_process_flush(iscsilun)) {
        qemu_aio_wait();
    }

Kevin

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Paolo Bonzini]

Il 13/03/2013 10:31, Kevin Wolf ha scritto:
> Isn't the real problem that I/O requests for _this_specific_ iscsi BDS
> must not be in flight? So what you reall need is bdrv_drain(iscsi_bs)?
> 
> If I understand the code correctly, this boils down to:
> 
>     while (iscsi_process_flush(iscsilun)) {
>         qemu_aio_wait();
>     }

Yes, but this function is not doing a truncate at all.  It is simply
updating the cached result of bdrv_getlength.  I think it makes sense
that this function (which should not be truncate, but something else)
expects to be called with no pending requests.  It shouldn't be its task
to drain them.

I'll look at fixing this as mentioned in the earlier thread.

Paolo

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Peter Lieven]

On 11.03.2013 11:16, Paolo Bonzini wrote:
> Il 11/03/2013 11:05, Peter Lieven ha scritto:
>> ensure that there are no pending I/Os before calling
>> the sync readcapacity commands. the block_resize monitor
>> command will also flush all I/O, but double check in
>> case iscsi_truncate() is called from elsewhere in the
>> future.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/iscsi.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 3d52921..de20d53 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1167,6 +1167,10 @@ static int iscsi_truncate(BlockDriverState *bs,
>> int64_t offset)
>>           return -ENOTSUP;
>>       }
>>
>> +    /* ensure all async requests are completed before executing
>> +     * a sync readcapacity */
>> +    bdrv_drain_all();
>> +
>>       if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
>>           return ret;
>>       }
>
> NACK to this patch.  It would be a bug, let's fix it properly.
>
> The other two are fine, however.

Paolo, can you ensure that Patch 1+2 get merged before qemu 1.5.0
to fix the regression.

Thanks,
Peter


>
> Paolo
>

Re: [Qemu-devel] [PATCH 3/3] iscsi_truncate: ensure there are no requests in flight

[Paolo Bonzini]

Il 19/03/2013 08:29, Peter Lieven ha scritto:
> On 11.03.2013 11:16, Paolo Bonzini wrote:
>> Il 11/03/2013 11:05, Peter Lieven ha scritto:
>>> ensure that there are no pending I/Os before calling
>>> the sync readcapacity commands. the block_resize monitor
>>> command will also flush all I/O, but double check in
>>> case iscsi_truncate() is called from elsewhere in the
>>> future.
>>>
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>   block/iscsi.c |    4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index 3d52921..de20d53 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -1167,6 +1167,10 @@ static int iscsi_truncate(BlockDriverState *bs,
>>> int64_t offset)
>>>           return -ENOTSUP;
>>>       }
>>>
>>> +    /* ensure all async requests are completed before executing
>>> +     * a sync readcapacity */
>>> +    bdrv_drain_all();
>>> +
>>>       if ((ret = iscsi_readcapacity_sync(iscsilun)) != 0) {
>>>           return ret;
>>>       }
>>
>> NACK to this patch.  It would be a bug, let's fix it properly.
>>
>> The other two are fine, however.
> 
> Paolo, can you ensure that Patch 1+2 get merged before qemu 1.5.0
> to fix the regression.

No, I cannot :) because I'm not the block maintainer.  But I'm sure that
they are on Kevin and Jeff's radar.

Paolo