2:53 p.m.
As described recently on list:
https://www.redhat.com/archives/libvir-list/2017-November/msg00043.html
and then discussed more in depth recently on OFTC #virt IRC, the
thread run after a storageVolUpload is completed to refresh the volume
and pool statistics could cause a crash if there was a storageVolCreateXML
being run in a different thread that was taking a long time to build the
volume. In the create/build case, the storage pool is unlocked while the
build occurs, but before doing so "asyncjobs" is incremented to avoid
certain other destructive operations such as PoolUndefine, PoolDestroy,
PoolDelete, and PoolRefresh. The poolRefresh will clear and repopulate
the pool volume list which is a bad thing to do since the create logic
will have added to that list and be keeping track of what was added just
in case the volume would need to be removed in the event of a build
failure.
The patches here will fix the case found and then alter the logic for
another possible miscreant that would clear the volumes in a thread that
needs to be run "later" than the normal processing for SCSI Fibre Channel
volume recognition.
John Ferlan (2):
storage: Resolve storage driver crash
scsi: Check for long running create in FCRefreshThread
src/storage/storage_backend_scsi.c | 1 +
src/storage/storage_driver.c | 9 +++++++++
2 files changed, 10 insertions(+)
--
2.13.6
2:53 p.m.
New subject: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash
Resolve a storage driver crash as a result of a long running
storageVolCreateXML when the virStorageVolPoolRefreshThread is
run as a result of a storageVolUpload complete and ran the
virStoragePoolObjClearVols without checking if the creation
code was currently processing a buildVol after incrementing
the driver->asyncjob count.
The refreshThread needs to wait until all creation threads
are completed so as to not alter the volume list while the
volume is being built.
Crash from valgrind is as follows (with a bit of editing):
==21309== Invalid read of size 8
==21309== at 0x153E47AF: storageBackendUpdateVolTargetInfo
==21309== by 0x153E4C30: virStorageBackendUpdateVolInfo
==21309== by 0x153E52DE: virStorageBackendVolRefreshLocal
==21309== by 0x153DE29E: storageVolCreateXML
==21309== by 0x562035B: virStorageVolCreateXML
==21309== by 0x147366: remoteDispatchStorageVolCreateXML
...
==21309== Address 0x2590a720 is 64 bytes inside a block of size 336 free'd
==21309== at 0x4C2F2BB: free
==21309== by 0x54CB9FA: virFree
==21309== by 0x55BC800: virStorageVolDefFree
==21309== by 0x55BF1D8: virStoragePoolObjClearVols
==21309== by 0x153D967E: virStorageVolPoolRefreshThread
...
==21309== Block was alloc'd at
==21309== at 0x4C300A5: calloc
==21309== by 0x54CB483: virAlloc
==21309== by 0x55BDC1F: virStorageVolDefParseXML
==21309== by 0x55BDC1F: virStorageVolDefParseNode
==21309== by 0x55BE5A4: virStorageVolDefParse
==21309== by 0x153DDFF1: storageVolCreateXML
==21309== by 0x562035B: virStorageVolCreateXML
==21309== by 0x147366: remoteDispatchStorageVolCreateXML
...
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_driver.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index b0edf9f885..5e920fc14c 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
virStorageBackendPtr backend;
virObjectEventPtr event = NULL;
+ retry:
storageDriverLock();
if (cbdata->vol_path) {
if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
@@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
if (!(backend = virStorageBackendForType(def->type)))
goto cleanup;
+ /* Some thread is creating a new volume in the pool, we need to retry */
+ if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
+ virStoragePoolObjUnlock(obj);
+ storageDriverUnlock();
+ usleep(100 * 1000);
+ goto retry;
+ }
+
virStoragePoolObjClearVols(obj);
if (backend->refreshPool(NULL, obj) < 0)
VIR_DEBUG("Failed to refresh storage pool");
--
2.13.6
3:18 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash
On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
Resolve a storage driver crash as a result of a long running
storageVolCreateXML when the virStorageVolPoolRefreshThread is
run as a result of a storageVolUpload complete and ran the
virStoragePoolObjClearVols without checking if the creation
code was currently processing a buildVol after incrementing
the driver->asyncjob count.
The refreshThread needs to wait until all creation threads
are completed so as to not alter the volume list while the
volume is being built.
Crash from valgrind is as follows (with a bit of editing):
==21309== Invalid read of size 8
==21309== at 0x153E47AF: storageBackendUpdateVolTargetInfo
==21309== by 0x153E4C30: virStorageBackendUpdateVolInfo
==21309== by 0x153E52DE: virStorageBackendVolRefreshLocal
==21309== by 0x153DE29E: storageVolCreateXML
==21309== by 0x562035B: virStorageVolCreateXML
==21309== by 0x147366: remoteDispatchStorageVolCreateXML
...
==21309== Address 0x2590a720 is 64 bytes inside a block of size 336 free'd
==21309== at 0x4C2F2BB: free
==21309== by 0x54CB9FA: virFree
==21309== by 0x55BC800: virStorageVolDefFree
==21309== by 0x55BF1D8: virStoragePoolObjClearVols
==21309== by 0x153D967E: virStorageVolPoolRefreshThread
...
==21309== Block was alloc'd at
==21309== at 0x4C300A5: calloc
==21309== by 0x54CB483: virAlloc
==21309== by 0x55BDC1F: virStorageVolDefParseXML
==21309== by 0x55BDC1F: virStorageVolDefParseNode
==21309== by 0x55BE5A4: virStorageVolDefParse
==21309== by 0x153DDFF1: storageVolCreateXML
==21309== by 0x562035B: virStorageVolCreateXML
==21309== by 0x147366: remoteDispatchStorageVolCreateXML
...
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_driver.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index b0edf9f885..5e920fc14c 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
virStorageBackendPtr backend;
virObjectEventPtr event = NULL;
+ retry:
storageDriverLock();
if (cbdata->vol_path) {
if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
@@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
if (!(backend = virStorageBackendForType(def->type)))
goto cleanup;
+ /* Some thread is creating a new volume in the pool, we need to retry */
+ if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
+ virStoragePoolObjUnlock(obj);
+ storageDriverUnlock();
+ usleep(100 * 1000);
+ goto retry;
+ }
+
virStoragePoolObjClearVols(obj);
if (backend->refreshPool(NULL, obj) < 0)
VIR_DEBUG("Failed to refresh storage pool");
ACK, does the job here. I'm rather surprised to see you implementing it
with sleep, while you pointed me towards virCondWait yesterday. But using
sleep is a way less intrusive change.
--
Cedric
7:43 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash
On 11/07/2017 04:18 AM, Cedric Bosdonnat wrote:
On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
> Resolve a storage driver crash as a result of a long running
> storageVolCreateXML when the virStorageVolPoolRefreshThread is
> run as a result of a storageVolUpload complete and ran the
> virStoragePoolObjClearVols without checking if the creation
> code was currently processing a buildVol after incrementing
> the driver->asyncjob count.
>
> The refreshThread needs to wait until all creation threads
> are completed so as to not alter the volume list while the
> volume is being built.
>
> Crash from valgrind is as follows (with a bit of editing):
>
> ==21309== Invalid read of size 8
> ==21309== at 0x153E47AF: storageBackendUpdateVolTargetInfo
> ==21309== by 0x153E4C30: virStorageBackendUpdateVolInfo
> ==21309== by 0x153E52DE: virStorageBackendVolRefreshLocal
> ==21309== by 0x153DE29E: storageVolCreateXML
> ==21309== by 0x562035B: virStorageVolCreateXML
> ==21309== by 0x147366: remoteDispatchStorageVolCreateXML
> ...
> ==21309== Address 0x2590a720 is 64 bytes inside a block of size 336 free'd
> ==21309== at 0x4C2F2BB: free
> ==21309== by 0x54CB9FA: virFree
> ==21309== by 0x55BC800: virStorageVolDefFree
> ==21309== by 0x55BF1D8: virStoragePoolObjClearVols
> ==21309== by 0x153D967E: virStorageVolPoolRefreshThread
> ...
> ==21309== Block was alloc'd at
> ==21309== at 0x4C300A5: calloc
> ==21309== by 0x54CB483: virAlloc
> ==21309== by 0x55BDC1F: virStorageVolDefParseXML
> ==21309== by 0x55BDC1F: virStorageVolDefParseNode
> ==21309== by 0x55BE5A4: virStorageVolDefParse
> ==21309== by 0x153DDFF1: storageVolCreateXML
> ==21309== by 0x562035B: virStorageVolCreateXML
> ==21309== by 0x147366: remoteDispatchStorageVolCreateXML
> ...
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_driver.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index b0edf9f885..5e920fc14c 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
> virStorageBackendPtr backend;
> virObjectEventPtr event = NULL;
>
> + retry:
> storageDriverLock();
> if (cbdata->vol_path) {
> if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
> @@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
> if (!(backend = virStorageBackendForType(def->type)))
> goto cleanup;
>
> + /* Some thread is creating a new volume in the pool, we need to retry */
> + if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
> + virStoragePoolObjUnlock(obj);
> + storageDriverUnlock();
> + usleep(100 * 1000);
> + goto retry;
> + }
> +
> virStoragePoolObjClearVols(obj);
> if (backend->refreshPool(NULL, obj) < 0)
> VIR_DEBUG("Failed to refresh storage pool");
ACK, does the job here. I'm rather surprised to see you implementing it
with sleep, while you pointed me towards virCondWait yesterday. But using
sleep is a way less intrusive change.
Well you only asked about an alternative mechanism and condition
variable was the first thing that popped into my mind; however, as I got
to actually doing more thinking about it - asyncjobs is not blocking
multiple creates from occurring; whereas, a condition variable would be
waiting for one thing to complete.
My response to Jan also lists 2 other alternatives. This was just the
"easiest" of the 3. If there's other ideas, I'm open to suggestions.
John
--
Cedric
9:16 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash
On Tue, 2017-11-07 at 08:43 -0500, John Ferlan wrote:
On 11/07/2017 04:18 AM, Cedric Bosdonnat wrote:
> On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
> > Resolve a storage driver crash as a result of a long running
> > storageVolCreateXML when the virStorageVolPoolRefreshThread is
> > run as a result of a storageVolUpload complete and ran the
> > virStoragePoolObjClearVols without checking if the creation
> > code was currently processing a buildVol after incrementing
> > the driver->asyncjob count.
> >
> > The refreshThread needs to wait until all creation threads
> > are completed so as to not alter the volume list while the
> > volume is being built.
> >
> > Crash from valgrind is as follows (with a bit of editing):
> >
> > ==21309== Invalid read of size 8
> > ==21309== at 0x153E47AF: storageBackendUpdateVolTargetInfo
> > ==21309== by 0x153E4C30: virStorageBackendUpdateVolInfo
> > ==21309== by 0x153E52DE: virStorageBackendVolRefreshLocal
> > ==21309== by 0x153DE29E: storageVolCreateXML
> > ==21309== by 0x562035B: virStorageVolCreateXML
> > ==21309== by 0x147366: remoteDispatchStorageVolCreateXML
> > ...
> > ==21309== Address 0x2590a720 is 64 bytes inside a block of size 336
free'd
> > ==21309== at 0x4C2F2BB: free
> > ==21309== by 0x54CB9FA: virFree
> > ==21309== by 0x55BC800: virStorageVolDefFree
> > ==21309== by 0x55BF1D8: virStoragePoolObjClearVols
> > ==21309== by 0x153D967E: virStorageVolPoolRefreshThread
> > ...
> > ==21309== Block was alloc'd at
> > ==21309== at 0x4C300A5: calloc
> > ==21309== by 0x54CB483: virAlloc
> > ==21309== by 0x55BDC1F: virStorageVolDefParseXML
> > ==21309== by 0x55BDC1F: virStorageVolDefParseNode
> > ==21309== by 0x55BE5A4: virStorageVolDefParse
> > ==21309== by 0x153DDFF1: storageVolCreateXML
> > ==21309== by 0x562035B: virStorageVolCreateXML
> > ==21309== by 0x147366: remoteDispatchStorageVolCreateXML
> > ...
> >
> > Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> > ---
> > src/storage/storage_driver.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> > index b0edf9f885..5e920fc14c 100644
> > --- a/src/storage/storage_driver.c
> > +++ b/src/storage/storage_driver.c
> > @@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
> > virStorageBackendPtr backend;
> > virObjectEventPtr event = NULL;
> >
> > + retry:
> > storageDriverLock();
> > if (cbdata->vol_path) {
> > if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
> > @@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
> > if (!(backend = virStorageBackendForType(def->type)))
> > goto cleanup;
> >
> > + /* Some thread is creating a new volume in the pool, we need to retry */
> > + if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
> > + virStoragePoolObjUnlock(obj);
> > + storageDriverUnlock();
> > + usleep(100 * 1000);
> > + goto retry;
> > + }
> > +
> > virStoragePoolObjClearVols(obj);
> > if (backend->refreshPool(NULL, obj) < 0)
> > VIR_DEBUG("Failed to refresh storage pool");
>
> ACK, does the job here. I'm rather surprised to see you implementing it
> with sleep, while you pointed me towards virCondWait yesterday. But using
> sleep is a way less intrusive change.
>
Well you only asked about an alternative mechanism and condition
variable was the first thing that popped into my mind; however, as I got
to actually doing more thinking about it - asyncjobs is not blocking
multiple creates from occurring; whereas, a condition variable would be
waiting for one thing to complete.
My response to Jan also lists 2 other alternatives. This was just the
"easiest" of the 3. If there's other ideas, I'm open to suggestions.
It looks fine to me, I was just surprised to see the sleep version ;)
--
Cedric
3:36 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash
On Mon, Nov 06, 2017 at 03:53:08PM -0500, John Ferlan wrote:
Resolve a storage driver crash as a result of a long running
storageVolCreateXML when the virStorageVolPoolRefreshThread is
run as a result of a storageVolUpload complete and ran the
virStoragePoolObjClearVols without checking if the creation
code was currently processing a buildVol after incrementing
the driver->asyncjob count.
I thought the whole point of refreshing the pool in a thread
was to have the storage driver usable for other things.
This effectively disables pool refresh during long-running
jobs.
The refreshThread needs to wait until all creation threads
are completed so as to not alter the volume list while the
volume is being built.
Crash from valgrind is as follows (with a bit of editing):
==21309== Invalid read of size 8
==21309== at 0x153E47AF: storageBackendUpdateVolTargetInfo
==21309== by 0x153E4C30: virStorageBackendUpdateVolInfo
==21309== by 0x153E52DE: virStorageBackendVolRefreshLocal
==21309== by 0x153DE29E: storageVolCreateXML
This is a fault of storageVolCreateXML for using invalid objects
after dropping the locks.
Jan
==21309== by 0x562035B: virStorageVolCreateXML
==21309== by 0x147366: remoteDispatchStorageVolCreateXML
...
==21309== Address 0x2590a720 is 64 bytes inside a block of size 336 free'd
==21309== at 0x4C2F2BB: free
==21309== by 0x54CB9FA: virFree
==21309== by 0x55BC800: virStorageVolDefFree
==21309== by 0x55BF1D8: virStoragePoolObjClearVols
==21309== by 0x153D967E: virStorageVolPoolRefreshThread
...
==21309== Block was alloc'd at
==21309== at 0x4C300A5: calloc
==21309== by 0x54CB483: virAlloc
==21309== by 0x55BDC1F: virStorageVolDefParseXML
==21309== by 0x55BDC1F: virStorageVolDefParseNode
==21309== by 0x55BE5A4: virStorageVolDefParse
==21309== by 0x153DDFF1: storageVolCreateXML
==21309== by 0x562035B: virStorageVolCreateXML
==21309== by 0x147366: remoteDispatchStorageVolCreateXML
...
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_driver.c | 9 +++++++++
1 file changed, 9 insertions(+)
7:38 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash
On 11/07/2017 04:36 AM, Ján Tomko wrote:
On Mon, Nov 06, 2017 at 03:53:08PM -0500, John Ferlan wrote:
> Resolve a storage driver crash as a result of a long running
> storageVolCreateXML when the virStorageVolPoolRefreshThread is
> run as a result of a storageVolUpload complete and ran the
> virStoragePoolObjClearVols without checking if the creation
> code was currently processing a buildVol after incrementing
> the driver->asyncjob count.
I thought the whole point of refreshing the pool in a thread
was to have the storage driver usable for other things.
This effectively disables pool refresh during long-running
jobs.
This would "disable" the refresh during a long running create/build job,
but that's already being done as other than another create, not much can
be done while a create/build is "in progress". The pool undefine,
destroy, delete, and refresh calls are all blocked (they fail) until the
create completes.
The reason for virStorageVolPoolRefreshThread is that updateVol can be
long running and once it's done (the stream is closed), it was desired
that the pool sizing values be updated. That meant either refresh the
entire pool (the easy way - theoretically) or update just the volume and
adjust the pool sizing values with the delta from the upload (similar to
how create/build alters the values).
Unfortunately the latter is not possible (yet) because only a subset of
pool types that support uploadVol also support refreshVol. But this
really is only a "problem" for the intersection of those that support
the buildVol too. That is the only reason we'd be blocking is if a pool
type supports both buildVol/buildVolFrom and uploadVol - so the fs,
logical, and vstorage pools.
The virStoragePoolFCRefreshThread used similar type logic w/r/t
refreshing the pool after a createVport completed. Here though, we're
waiting for udev to work it's magic in creating the vHBA LUN. The
asyncjob won't be > 0 for scsi pools yet, so perhaps patch 2 isn't
necessary, but it's preventative.
In any case, at this point in time I would think it's more desirable to
block during the build/upload condition as opposed to crash.
An "alternative" is to refresh the pool after a create vol rather than
just altering the pool available/allocation values from the created
volume target allocation. That way the VolPoolRefreshThread could just
"exit" knowing that volCreate will do the update. However, this refresh
could only happen when there are no asyncjobs outstanding. In the mean
time the pool sizing values probably would still need to be updated if
asyncjobs > 0 to avoid multiple create jobs from "exhausting" the pool
because the sizing values weren't properly updated.
>
> The refreshThread needs to wait until all creation threads
> are completed so as to not alter the volume list while the
> volume is being built.
>
> Crash from valgrind is as follows (with a bit of editing):
>
> ==21309== Invalid read of size 8
> ==21309== at 0x153E47AF: storageBackendUpdateVolTargetInfo
> ==21309== by 0x153E4C30: virStorageBackendUpdateVolInfo
> ==21309== by 0x153E52DE: virStorageBackendVolRefreshLocal
> ==21309== by 0x153DE29E: storageVolCreateXML
This is a fault of storageVolCreateXML for using invalid objects
after dropping the locks.
Partially, but the code also goes through the trouble of incrementing
the pool->asyncjobs count and setting the voldef->building flag for what
appears to be the reasoning that nothing else should modify the pool
volume list while we create/build a new volume, but the refresh thread
just ignores that. The asyncjobs was added in much simpler times in
commit id '2cd9b2d8'.
And this brings up a possible 3rd alternative. Perhaps the more real
problem is that virStoragePoolObjClearVols and refreshPool processing is
overly invasive with respect to free-ing the entire volume list and
rebuilding it rather than perhaps doing some sort of more intelligent
processing to "add" new volumes found in a pool that are not in the
list, "remove" ones no longer in the pool that are in the list, and
"update" those that are still in the list. That to me is outside the
scope of this change, but a noble goal at some point in time.
John
Jan
> ==21309== by 0x562035B: virStorageVolCreateXML
> ==21309== by 0x147366: remoteDispatchStorageVolCreateXML
> ...
> ==21309== Address 0x2590a720 is 64 bytes inside a block of size 336
> free'd
> ==21309== at 0x4C2F2BB: free
> ==21309== by 0x54CB9FA: virFree
> ==21309== by 0x55BC800: virStorageVolDefFree
> ==21309== by 0x55BF1D8: virStoragePoolObjClearVols
> ==21309== by 0x153D967E: virStorageVolPoolRefreshThread
> ...
> ==21309== Block was alloc'd at
> ==21309== at 0x4C300A5: calloc
> ==21309== by 0x54CB483: virAlloc
> ==21309== by 0x55BDC1F: virStorageVolDefParseXML
> ==21309== by 0x55BDC1F: virStorageVolDefParseNode
> ==21309== by 0x55BE5A4: virStorageVolDefParse
> ==21309== by 0x153DDFF1: storageVolCreateXML
> ==21309== by 0x562035B: virStorageVolCreateXML
> ==21309== by 0x147366: remoteDispatchStorageVolCreateXML
> ...
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/storage/storage_driver.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
5:21 p.m.
New subject: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash
On 11/07/2017 08:38 AM, John Ferlan wrote:
On 11/07/2017 04:36 AM, Ján Tomko wrote:
> On Mon, Nov 06, 2017 at 03:53:08PM -0500, John Ferlan wrote:
>> Resolve a storage driver crash as a result of a long running
>> storageVolCreateXML when the virStorageVolPoolRefreshThread is
>> run as a result of a storageVolUpload complete and ran the
>> virStoragePoolObjClearVols without checking if the creation
>> code was currently processing a buildVol after incrementing
>> the driver->asyncjob count.
>
> I thought the whole point of refreshing the pool in a thread
> was to have the storage driver usable for other things.
> This effectively disables pool refresh during long-running
> jobs.
>
Jan -
So I'm curious to know whether your comments amount to a NAK of the
design of the patches of if the explanation provided here was sufficient?
Tks -
John
This would "disable" the refresh during a long running create/build job,
but that's already being done as other than another create, not much can
be done while a create/build is "in progress". The pool undefine,
destroy, delete, and refresh calls are all blocked (they fail) until the
create completes.
The reason for virStorageVolPoolRefreshThread is that updateVol can be
long running and once it's done (the stream is closed), it was desired
that the pool sizing values be updated. That meant either refresh the
entire pool (the easy way - theoretically) or update just the volume and
adjust the pool sizing values with the delta from the upload (similar to
how create/build alters the values).
Unfortunately the latter is not possible (yet) because only a subset of
pool types that support uploadVol also support refreshVol. But this
really is only a "problem" for the intersection of those that support
the buildVol too. That is the only reason we'd be blocking is if a pool
type supports both buildVol/buildVolFrom and uploadVol - so the fs,
logical, and vstorage pools.
The virStoragePoolFCRefreshThread used similar type logic w/r/t
refreshing the pool after a createVport completed. Here though, we're
waiting for udev to work it's magic in creating the vHBA LUN. The
asyncjob won't be > 0 for scsi pools yet, so perhaps patch 2 isn't
necessary, but it's preventative.
In any case, at this point in time I would think it's more desirable to
block during the build/upload condition as opposed to crash.
An "alternative" is to refresh the pool after a create vol rather than
just altering the pool available/allocation values from the created
volume target allocation. That way the VolPoolRefreshThread could just
"exit" knowing that volCreate will do the update. However, this refresh
could only happen when there are no asyncjobs outstanding. In the mean
time the pool sizing values probably would still need to be updated if
asyncjobs > 0 to avoid multiple create jobs from "exhausting" the pool
because the sizing values weren't properly updated.
>>
>> The refreshThread needs to wait until all creation threads
>> are completed so as to not alter the volume list while the
>> volume is being built.
>>
>> Crash from valgrind is as follows (with a bit of editing):
>>
>> ==21309== Invalid read of size 8
>> ==21309== at 0x153E47AF: storageBackendUpdateVolTargetInfo
>> ==21309== by 0x153E4C30: virStorageBackendUpdateVolInfo
>> ==21309== by 0x153E52DE: virStorageBackendVolRefreshLocal
>> ==21309== by 0x153DE29E: storageVolCreateXML
>
> This is a fault of storageVolCreateXML for using invalid objects
> after dropping the locks.
>
Partially, but the code also goes through the trouble of incrementing
the pool->asyncjobs count and setting the voldef->building flag for what
appears to be the reasoning that nothing else should modify the pool
volume list while we create/build a new volume, but the refresh thread
just ignores that. The asyncjobs was added in much simpler times in
commit id '2cd9b2d8'.
And this brings up a possible 3rd alternative. Perhaps the more real
problem is that virStoragePoolObjClearVols and refreshPool processing is
overly invasive with respect to free-ing the entire volume list and
rebuilding it rather than perhaps doing some sort of more intelligent
processing to "add" new volumes found in a pool that are not in the
list, "remove" ones no longer in the pool that are in the list, and
"update" those that are still in the list. That to me is outside the
scope of this change, but a noble goal at some point in time.
John
> Jan
>
>> ==21309== by 0x562035B: virStorageVolCreateXML
>> ==21309== by 0x147366: remoteDispatchStorageVolCreateXML
>> ...
>> ==21309== Address 0x2590a720 is 64 bytes inside a block of size 336
>> free'd
>> ==21309== at 0x4C2F2BB: free
>> ==21309== by 0x54CB9FA: virFree
>> ==21309== by 0x55BC800: virStorageVolDefFree
>> ==21309== by 0x55BF1D8: virStoragePoolObjClearVols
>> ==21309== by 0x153D967E: virStorageVolPoolRefreshThread
>> ...
>> ==21309== Block was alloc'd at
>> ==21309== at 0x4C300A5: calloc
>> ==21309== by 0x54CB483: virAlloc
>> ==21309== by 0x55BDC1F: virStorageVolDefParseXML
>> ==21309== by 0x55BDC1F: virStorageVolDefParseNode
>> ==21309== by 0x55BE5A4: virStorageVolDefParse
>> ==21309== by 0x153DDFF1: storageVolCreateXML
>> ==21309== by 0x562035B: virStorageVolCreateXML
>> ==21309== by 0x147366: remoteDispatchStorageVolCreateXML
>> ...
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/storage/storage_driver.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:47 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash
On Wed, Nov 15, 2017 at 06:21:11PM -0500, John Ferlan wrote:
On 11/07/2017 08:38 AM, John Ferlan wrote:
>
>
> On 11/07/2017 04:36 AM, Ján Tomko wrote:
>> On Mon, Nov 06, 2017 at 03:53:08PM -0500, John Ferlan wrote:
>>> Resolve a storage driver crash as a result of a long running
>>> storageVolCreateXML when the virStorageVolPoolRefreshThread is
>>> run as a result of a storageVolUpload complete and ran the
>>> virStoragePoolObjClearVols without checking if the creation
>>> code was currently processing a buildVol after incrementing
>>> the driver->asyncjob count.
>>
>> I thought the whole point of refreshing the pool in a thread
>> was to have the storage driver usable for other things.
>> This effectively disables pool refresh during long-running
>> jobs.
>>
Jan -
So I'm curious to know whether your comments amount to a NAK of the
design of the patches of if the explanation provided here was sufficient?
NACK to the use of usleep for this.
I loathe usleep. It is reasonable for waiting on something external or racy
things like session daemon startup, but I do not see why it is necessary
here.
Tks -
John
[...]
>
> An "alternative" is to refresh the pool after a create vol rather than
> just altering the pool available/allocation values from the created
> volume target allocation. That way the VolPoolRefreshThread could just
> "exit" knowing that volCreate will do the update. However, this refresh
> could only happen when there are no asyncjobs outstanding. In the mean
> time the pool sizing values probably would still need to be updated if
> asyncjobs > 0 to avoid multiple create jobs from "exhausting" the pool
> because the sizing values weren't properly updated.
>
I did not realize VolPoolRefreshThread was only called after volume
upload. I think quietly doing nothing in this case would be reasonable,
considering that actual storagePoolRefresh errors out in that case.
As far as noble goals go, this is volUpload being more agressive
in keeping the pool allocation/capacity values up to date.
Since volCreate only changes the values based on the request by
the user, it does not take into account how much space is required
on-disk (e.g. metadata for LUKS and qcow2 volumes).
I am not sure how to reasonably solve this without refreshing the
whole pool every time (which gives us the advantage of catching
changes out of our scope - e.g. a fs pool that does not reside
on a separate partition).
Jan
2:53 p.m.
New subject: [libvirt] [PATCH 2/2] scsi: Check for long running create in FCRefreshThread
Similar to a recent patch in virStorageVolPoolRefreshThread to ensure
that there were no pool AsyncJobs (e.g. nothing being created at the
time in a long running buildVol job), modify virStoragePoolFCRefreshThread
to check for async jobs before calling virStoragePoolObjClearVols and
refreshing the volumes defined in the pool.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 02fd4b643c..63a9154102 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -159,6 +159,7 @@ virStoragePoolFCRefreshThread(void *opaque)
pool->def->allocation = pool->def->capacity =
pool->def->available = 0;
if (virStoragePoolObjIsActive(pool) &&
+ virStoragePoolObjGetAsyncjobs(pool) == 0 &&
virSCSIHostGetNumber(fchost_name, &host) == 0 &&
virStorageBackendSCSITriggerRescan(host) == 0) {
virStoragePoolObjClearVols(pool);
--
2.13.6
3:19 a.m.
New subject: [libvirt] [PATCH 2/2] scsi: Check for long running create in FCRefreshThread
On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
Similar to a recent patch in virStorageVolPoolRefreshThread to
ensure
that there were no pool AsyncJobs (e.g. nothing being created at the
time in a long running buildVol job), modify virStoragePoolFCRefreshThread
to check for async jobs before calling virStoragePoolObjClearVols and
refreshing the volumes defined in the pool.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_scsi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 02fd4b643c..63a9154102 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -159,6 +159,7 @@ virStoragePoolFCRefreshThread(void *opaque)
pool->def->allocation = pool->def->capacity =
pool->def->available = 0;
if (virStoragePoolObjIsActive(pool) &&
+ virStoragePoolObjGetAsyncjobs(pool) == 0 &&
virSCSIHostGetNumber(fchost_name, &host) == 0 &&
virStorageBackendSCSITriggerRescan(host) == 0) {
virStoragePoolObjClearVols(pool);
ACK
--
Cedric
2564
days inactive
2574
days old
10 comments
3 participants
participants (3)
-
Cedric Bosdonnat -
John Ferlan -
Ján Tomko