| Message ID | 20180207162920.28386-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: early check for blockers on drive-mirror | expand |
On Wed, 02/07 17:29, Paolo Bonzini wrote: > Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE, > it is checked a bit late and the result is that the target is > created even if drive-mirror subsequently fails. Add an early > check to avoid this. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > blockdev.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index 8e977eef11..c7e2e0a00e 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) > return; > } > > + /* Early check to avoid creating target */ > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { > + return; > + } > + > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > > -- > 2.14.3 > > Reviewed-by: Fam Zheng <famz@redhat.com>
On Wed 07 Feb 2018 05:29:20 PM CET, Paolo Bonzini wrote: > Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE, > it is checked a bit late and the result is that the target is > created even if drive-mirror subsequently fails. Add an early > check to avoid this. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > blockdev.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index 8e977eef11..c7e2e0a00e 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) > return; > } > > + /* Early check to avoid creating target */ > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { > + return; > + } > + > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); Do we need to hold the AioContext in order to check for op blockers? Berto
On 08/02/2018 11:10, Alberto Garcia wrote: > On Wed 07 Feb 2018 05:29:20 PM CET, Paolo Bonzini wrote: >> Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE, >> it is checked a bit late and the result is that the target is >> created even if drive-mirror subsequently fails. Add an early >> check to avoid this. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> blockdev.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/blockdev.c b/blockdev.c >> index 8e977eef11..c7e2e0a00e 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) >> return; >> } >> >> + /* Early check to avoid creating target */ >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { >> + return; >> + } >> + >> aio_context = bdrv_get_aio_context(bs); >> aio_context_acquire(aio_context); > > Do we need to hold the AioContext in order to check for op blockers? In include/block/block_int.h, they are not in the "Protected by AioContext lock" section. Paolo
On Wed 07 Feb 2018 05:29:20 PM CET, Paolo Bonzini wrote: > Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE, > it is checked a bit late and the result is that the target is > created even if drive-mirror subsequently fails. Add an early > check to avoid this. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
Am 07.02.2018 um 17:29 hat Paolo Bonzini geschrieben: > Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE, > it is checked a bit late and the result is that the target is > created even if drive-mirror subsequently fails. Add an early > check to avoid this. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, applied to the block branch. Kevin
On 02/07/2018 11:29 AM, Paolo Bonzini wrote: > Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE, > it is checked a bit late and the result is that the target is > created even if drive-mirror subsequently fails. Add an early > check to avoid this. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > blockdev.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index 8e977eef11..c7e2e0a00e 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) > return; > } > > + /* Early check to avoid creating target */ > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { > + return; > + } > + > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > > What's the implication of the temporarily-extant target node that it needs to be avoided so strictly?
On 10/02/2018 00:07, John Snow wrote: >> + /* Early check to avoid creating target */ >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { >> + return; >> + } >> + >> aio_context = bdrv_get_aio_context(bs); >> aio_context_acquire(aio_context); >> >> > What's the implication of the temporarily-extant target node that it > needs to be avoided so strictly? > Creating a file on disk, that no one will ever remvoe. :) Paolo
On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote: > On 10/02/2018 00:07, John Snow wrote: > >> + /* Early check to avoid creating target */ > >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { > >> + return; > >> + } > >> + > >> aio_context = bdrv_get_aio_context(bs); > >> aio_context_acquire(aio_context); > >> > >> > > What's the implication of the temporarily-extant target node that it > > needs to be avoided so strictly? > > > > Creating a file on disk, that no one will ever remvoe. :) Fortunately libvirt's SELinux policy will probably prevent QEMU creating it in the first place :-) Regards, Daniel
Am 12.02.2018 um 11:02 hat Daniel P. Berrangé geschrieben: > On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote: > > On 10/02/2018 00:07, John Snow wrote: > > >> + /* Early check to avoid creating target */ > > >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { > > >> + return; > > >> + } > > >> + > > >> aio_context = bdrv_get_aio_context(bs); > > >> aio_context_acquire(aio_context); > > >> > > >> > > > What's the implication of the temporarily-extant target node that it > > > needs to be avoided so strictly? > > > > > > > Creating a file on disk, that no one will ever remvoe. :) > > Fortunately libvirt's SELinux policy will probably prevent QEMU creating > it in the first place :-) Well, calling drive-mirror without allowing QEMU to create the target image would be a bit pointless, so I think we can assume that libvirt did set up the file permission so that QEMU can create it. (Unless mode=existing is used, but I understand that libvirt doesn't want to create images with qemu-img, so that doesn't seem to be the case...) I don't know if libvirt takes care to remove a potentially already created file if the command then fails, but hopefully it does and the patch is not actually needed with libvirt. Kevin
On Mon, Feb 12, 2018 at 01:42:11PM +0100, Kevin Wolf wrote: > Am 12.02.2018 um 11:02 hat Daniel P. Berrangé geschrieben: > > On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote: > > > On 10/02/2018 00:07, John Snow wrote: > > > >> + /* Early check to avoid creating target */ > > > >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { > > > >> + return; > > > >> + } > > > >> + > > > >> aio_context = bdrv_get_aio_context(bs); > > > >> aio_context_acquire(aio_context); > > > >> > > > >> > > > > What's the implication of the temporarily-extant target node that it > > > > needs to be avoided so strictly? > > > > > > > > > > Creating a file on disk, that no one will ever remvoe. :) > > > > Fortunately libvirt's SELinux policy will probably prevent QEMU creating > > it in the first place :-) > > Well, calling drive-mirror without allowing QEMU to create the target > image would be a bit pointless, so I think we can assume that libvirt > did set up the file permission so that QEMU can create it. (Unless > mode=existing is used, but I understand that libvirt doesn't want to > create images with qemu-img, so that doesn't seem to be the case...) We use either mode=existing or mode=absolute-paths depending on what the mgmt app asked for in the API call to libvirt. I'm still kind of suprised if mode=absolute-paths will work because we ought to be blocking the creation of the file AFAIK and we can't pre-label a file that doesn't exist yet. Regards, Daniel
diff --git a/blockdev.c b/blockdev.c index 8e977eef11..c7e2e0a00e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3565,6 +3565,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) return; } + /* Early check to avoid creating target */ + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) { + return; + } + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context);
Even if an op blocker is present for BLOCK_OP_TYPE_MIRROR_SOURCE, it is checked a bit late and the result is that the target is created even if drive-mirror subsequently fails. Add an early check to avoid this. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- blockdev.c | 5 +++++ 1 file changed, 5 insertions(+)