| Message ID | 20190219125044.5416-3-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: improve error handling when luks creation fails | expand |
On 2/19/19 6:50 AM, Daniel P. Berrangé wrote: > During creation we write a minimal qcow2 header and then update it with > extra features. If the updating fails for some reason we might still be > left with a valid qcow2 image that will be mistakenly used for I/O. We > cannot delete the image, since we don't know if we created the > underlying storage or not. Thus we mark the header as corrupt to > prevents its later usage. Should we unconditionally mark the image as corrupt at the time we write the minimal qcow2 header, and then update the image to non-corrupt on the final update? > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > block/qcow2.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/block/qcow2.c b/block/qcow2.c > index ecc577175f..338513e652 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) > > ret = 0; > out: > + if (ret < 0) { > + qcow2_mark_corrupt(blk_bs(blk)); > + } If ret < 0 because of an EIO error, this may also fail to write the change to the header. Hence my question as to whether this is too late.
On Tue, Feb 19, 2019 at 10:11:58AM -0600, Eric Blake wrote: > On 2/19/19 6:50 AM, Daniel P. Berrangé wrote: > > During creation we write a minimal qcow2 header and then update it with > > extra features. If the updating fails for some reason we might still be > > left with a valid qcow2 image that will be mistakenly used for I/O. We > > cannot delete the image, since we don't know if we created the > > underlying storage or not. Thus we mark the header as corrupt to > > prevents its later usage. > > Should we unconditionally mark the image as corrupt at the time we write > the minimal qcow2 header, and then update the image to non-corrupt on > the final update? That's a nice idea, but we call blk_new_open() half way through to qcow2_co_create method to open the minimal image. If we mark it corrupt upfront we'll never be able to open this minimal image. Adding a flag to allow blk_new_open to ignore the "corrupt" marker feels unplesant to me. > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > block/qcow2.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/block/qcow2.c b/block/qcow2.c > > index ecc577175f..338513e652 100644 > > --- a/block/qcow2.c > > +++ b/block/qcow2.c > > @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) > > > > ret = 0; > > out: > > + if (ret < 0) { > > + qcow2_mark_corrupt(blk_bs(blk)); > > + } > > If ret < 0 because of an EIO error, this may also fail to write the > change to the header. Hence my question as to whether this is too late. Regards, Daniel
On 19.02.19 13:50, Daniel P. Berrangé wrote: > During creation we write a minimal qcow2 header and then update it with > extra features. If the updating fails for some reason we might still be > left with a valid qcow2 image that will be mistakenly used for I/O. We > cannot delete the image, since we don't know if we created the > underlying storage or not. Thus we mark the header as corrupt to > prevents its later usage. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > block/qcow2.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/block/qcow2.c b/block/qcow2.c > index ecc577175f..338513e652 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) > > ret = 0; > out: > + if (ret < 0) { > + qcow2_mark_corrupt(blk_bs(blk)); First, blk_bs(blk) may be the qcow2 BDS, or it may the protocol BDS here (it is the latter before the first blk_new_open() call). Calling qcow2_mark_corrupt() unconditionally may mean an invalid access to bs->opaque (which is not going to be of type BDRVQcow2State if the BDS is not a qcow2 BDS). Second, blk may be NULL (at various points, e.g. after blk_new_open() failed). Then this would yield a segfault in in blk_bs(). Third, blk_bs(blk) may be NULL (if blk_insert_bs() failed). Then this would yield a segfault in qcow2_mark_corrupt(). On a minor note, it is rather probably that blk_new_open() fails. In that case, there is currently no way to mark the image corrupt. Would it be useful and possible to have a function to mark a qcow2 image corrupt without relying on qcow2 features, i.e. by writing directly to the protocol layer (which is always @bs)? This would be unsafe to use as long as the protocol layer is opened by the qcow2 driver in some other node, but we could invoke this function safely after @blk has been freed. Or maybe Eric's suggestion really is for the best, i.e. mark the image corrupt from the start and then clean that after we're all done. You don't need a new flag for that, we already have BDRV_O_CHECK. Max > + } > blk_unref(blk); > bdrv_unref(bs); > return ret; >
On Fri, Feb 22, 2019 at 08:21:26PM +0100, Max Reitz wrote: > On 19.02.19 13:50, Daniel P. Berrangé wrote: > > During creation we write a minimal qcow2 header and then update it with > > extra features. If the updating fails for some reason we might still be > > left with a valid qcow2 image that will be mistakenly used for I/O. We > > cannot delete the image, since we don't know if we created the > > underlying storage or not. Thus we mark the header as corrupt to > > prevents its later usage. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > block/qcow2.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/block/qcow2.c b/block/qcow2.c > > index ecc577175f..338513e652 100644 > > --- a/block/qcow2.c > > +++ b/block/qcow2.c > > @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) > > > > ret = 0; > > out: > > + if (ret < 0) { > > + qcow2_mark_corrupt(blk_bs(blk)); ...snip... > Or maybe Eric's suggestion really is for the best, i.e. mark the image > corrupt from the start and then clean that after we're all done. You > don't need a new flag for that, we already have BDRV_O_CHECK. Ah, I didn't realize that is what BDRV_O_CHECK could do. I'll try this approach as it is nicer. Regards, Daniel
diff --git a/block/qcow2.c b/block/qcow2.c index ecc577175f..338513e652 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) ret = 0; out: + if (ret < 0) { + qcow2_mark_corrupt(blk_bs(blk)); + } blk_unref(blk); bdrv_unref(bs); return ret;
During creation we write a minimal qcow2 header and then update it with extra features. If the updating fails for some reason we might still be left with a valid qcow2 image that will be mistakenly used for I/O. We cannot delete the image, since we don't know if we created the underlying storage or not. Thus we mark the header as corrupt to prevents its later usage. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- block/qcow2.c | 3 +++ 1 file changed, 3 insertions(+)