| Message ID | 20210721012134.792845-5-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migrations: Fix potential rare race of migration-test after yank | expand |
* Peter Xu (peterx@redhat.com) wrote: > migration uses QIOChannel typed qemufiles. In follow up patches, we'll need > the capability to identify this fact, so that we can get the backing QIOChannel > from a QEMUFile. > > We can also define types for QEMUFile but so far since we only need to be able > to identify QIOChannel, introduce a boolean which is simpler. > > No functional change. > > Signed-off-by: Peter Xu <peterx@redhat.com> This is messy but I can't see another quick way; the better way would be to add an OBJECT or QIOCHannel wrapper for BlockDriverState. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/qemu-file-channel.c | 4 ++-- > migration/qemu-file.c | 5 ++++- > migration/qemu-file.h | 2 +- > migration/ram.c | 2 +- > migration/savevm.c | 4 ++-- > 5 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c > index 867a5ed0c3..2f8b1fcd46 100644 > --- a/migration/qemu-file-channel.c > +++ b/migration/qemu-file-channel.c > @@ -187,11 +187,11 @@ static const QEMUFileOps channel_output_ops = { > QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc) > { > object_ref(OBJECT(ioc)); > - return qemu_fopen_ops(ioc, &channel_input_ops); > + return qemu_fopen_ops(ioc, &channel_input_ops, true); > } > > QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc) > { > object_ref(OBJECT(ioc)); > - return qemu_fopen_ops(ioc, &channel_output_ops); > + return qemu_fopen_ops(ioc, &channel_output_ops, true); > } > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index 1eacf9e831..ada58c94dd 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -55,6 +55,8 @@ struct QEMUFile { > Error *last_error_obj; > /* has the file has been shutdown */ > bool shutdown; > + /* Whether opaque points to a QIOChannel */ > + bool has_ioc; > }; > > /* > @@ -101,7 +103,7 @@ bool qemu_file_mode_is_not_valid(const char *mode) > return false; > } > > -QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops) > +QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc) > { > QEMUFile *f; > > @@ -109,6 +111,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops) > > f->opaque = opaque; > f->ops = ops; > + f->has_ioc = has_ioc; > return f; > } > > diff --git a/migration/qemu-file.h b/migration/qemu-file.h > index a9b6d6ccb7..80d0e79fd1 100644 > --- a/migration/qemu-file.h > +++ b/migration/qemu-file.h > @@ -119,7 +119,7 @@ typedef struct QEMUFileHooks { > QEMURamSaveFunc *save_page; > } QEMUFileHooks; > > -QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops); > +QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc); > void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks); > int qemu_get_fd(QEMUFile *f); > int qemu_fclose(QEMUFile *f); > diff --git a/migration/ram.c b/migration/ram.c > index b5fc454b2f..f2a86f9971 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -550,7 +550,7 @@ static int compress_threads_save_setup(void) > /* comp_param[i].file is just used as a dummy buffer to save data, > * set its ops to empty. > */ > - comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops); > + comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops, false); > comp_param[i].done = true; > comp_param[i].quit = false; > qemu_mutex_init(&comp_param[i].mutex); > diff --git a/migration/savevm.c b/migration/savevm.c > index 72848b946c..96b5e5d639 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -168,9 +168,9 @@ static const QEMUFileOps bdrv_write_ops = { > static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable) > { > if (is_writable) { > - return qemu_fopen_ops(bs, &bdrv_write_ops); > + return qemu_fopen_ops(bs, &bdrv_write_ops, false); > } > - return qemu_fopen_ops(bs, &bdrv_read_ops); > + return qemu_fopen_ops(bs, &bdrv_read_ops, false); > } > > > -- > 2.31.1 >
On Wed, Jul 21, 2021 at 11:27:44AM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > migration uses QIOChannel typed qemufiles. In follow up patches, we'll need > > the capability to identify this fact, so that we can get the backing QIOChannel > > from a QEMUFile. > > > > We can also define types for QEMUFile but so far since we only need to be able > > to identify QIOChannel, introduce a boolean which is simpler. > > > > No functional change. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > This is messy but I can't see another quick way; the better way would be > to add an OBJECT or QIOCHannel wrapper for BlockDriverState. I looked at making a QIOChannel for BlockDriverState but it was not as easy as it might seem. The problem is that the QEMUFile get_buffer / write_buffer methods take a offset at which the I/O operation is required to be applied. For the existing QIOChannel impl for migration, we simply ignore the 'pos' argument entirely, since it is irrelevant for the main migration channel doing streaming. For a BlockDriverState based impl though I think we need to honour "pos" in some manner. I think it ought to be possible to rewrite the savevm code so that it uses 'seek' in the few places it needs to, and then we can drop "pos" from get_buffer/write_buffer, but that requires careful consideration. Regards, Daniel
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c index 867a5ed0c3..2f8b1fcd46 100644 --- a/migration/qemu-file-channel.c +++ b/migration/qemu-file-channel.c @@ -187,11 +187,11 @@ static const QEMUFileOps channel_output_ops = { QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc) { object_ref(OBJECT(ioc)); - return qemu_fopen_ops(ioc, &channel_input_ops); + return qemu_fopen_ops(ioc, &channel_input_ops, true); } QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc) { object_ref(OBJECT(ioc)); - return qemu_fopen_ops(ioc, &channel_output_ops); + return qemu_fopen_ops(ioc, &channel_output_ops, true); } diff --git a/migration/qemu-file.c b/migration/qemu-file.c index 1eacf9e831..ada58c94dd 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -55,6 +55,8 @@ struct QEMUFile { Error *last_error_obj; /* has the file has been shutdown */ bool shutdown; + /* Whether opaque points to a QIOChannel */ + bool has_ioc; }; /* @@ -101,7 +103,7 @@ bool qemu_file_mode_is_not_valid(const char *mode) return false; } -QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops) +QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc) { QEMUFile *f; @@ -109,6 +111,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops) f->opaque = opaque; f->ops = ops; + f->has_ioc = has_ioc; return f; } diff --git a/migration/qemu-file.h b/migration/qemu-file.h index a9b6d6ccb7..80d0e79fd1 100644 --- a/migration/qemu-file.h +++ b/migration/qemu-file.h @@ -119,7 +119,7 @@ typedef struct QEMUFileHooks { QEMURamSaveFunc *save_page; } QEMUFileHooks; -QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops); +QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc); void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks); int qemu_get_fd(QEMUFile *f); int qemu_fclose(QEMUFile *f); diff --git a/migration/ram.c b/migration/ram.c index b5fc454b2f..f2a86f9971 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -550,7 +550,7 @@ static int compress_threads_save_setup(void) /* comp_param[i].file is just used as a dummy buffer to save data, * set its ops to empty. */ - comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops); + comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops, false); comp_param[i].done = true; comp_param[i].quit = false; qemu_mutex_init(&comp_param[i].mutex); diff --git a/migration/savevm.c b/migration/savevm.c index 72848b946c..96b5e5d639 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -168,9 +168,9 @@ static const QEMUFileOps bdrv_write_ops = { static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable) { if (is_writable) { - return qemu_fopen_ops(bs, &bdrv_write_ops); + return qemu_fopen_ops(bs, &bdrv_write_ops, false); } - return qemu_fopen_ops(bs, &bdrv_read_ops); + return qemu_fopen_ops(bs, &bdrv_read_ops, false); }
migration uses QIOChannel typed qemufiles. In follow up patches, we'll need the capability to identify this fact, so that we can get the backing QIOChannel from a QEMUFile. We can also define types for QEMUFile but so far since we only need to be able to identify QIOChannel, introduce a boolean which is simpler. No functional change. Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/qemu-file-channel.c | 4 ++-- migration/qemu-file.c | 5 ++++- migration/qemu-file.h | 2 +- migration/ram.c | 2 +- migration/savevm.c | 4 ++-- 5 files changed, 10 insertions(+), 7 deletions(-)