| Message ID | 513DAC5B.5000607@dlhnet.de |
|---|---|
| State | New |
| Headers | show |
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
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
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 >
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 >> >
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).
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
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
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 >
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
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; }
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(+)