| Message ID | 20240523190548.23977-2-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration/mapped-ram: Add direct-io support | expand |
On Fri, 24 May 2024 at 00:38, Fabiano Rosas <farosas@suse.de> wrote: > This is further indicated by the presence of the 'offset' > argument, which indicates the start of the region where QEMU is > allowed to write. > > Fix the issue by replacing the O_TRUNC flag on open by an ftruncate > call, which will take the offset into consideration. > > + if (ftruncate(fioc->fd, offset)) { > + error_setg_errno(errp, errno, > + "failed to truncate migration file to offset %" PRIx64, > + offset); > + object_unref(OBJECT(fioc)); > + return; > + } > + * Should 'offset' be checked for > zero while ftruncating? Else it'll be same as O_TRUNC. Otherwise it looks fine. Reviewed-by: Prasad Pandit <pjp@fedoraproject.org> Thank you. --- - Prasad
Prasad Pandit <ppandit@redhat.com> writes: > On Fri, 24 May 2024 at 00:38, Fabiano Rosas <farosas@suse.de> wrote: >> This is further indicated by the presence of the 'offset' >> argument, which indicates the start of the region where QEMU is >> allowed to write. >> >> Fix the issue by replacing the O_TRUNC flag on open by an ftruncate >> call, which will take the offset into consideration. >> >> + if (ftruncate(fioc->fd, offset)) { >> + error_setg_errno(errp, errno, >> + "failed to truncate migration file to offset %" PRIx64, >> + offset); >> + object_unref(OBJECT(fioc)); >> + return; >> + } >> + > > * Should 'offset' be checked for > zero while ftruncating? Else it'll > be same as O_TRUNC. Otherwise it looks fine. That's the point. If offset==0 we truncate all the way, if not, we truncate to the offset. > > Reviewed-by: Prasad Pandit <pjp@fedoraproject.org> Thanks!
On Fri, 24 May 2024 at 18:00, Fabiano Rosas <farosas@suse.de> wrote:
> That's the point. If offset==0 we truncate all the way, if not, we truncate to the offset.
* Yes, I was wondering if the migration file has some data, but still
'offset' ends up being zero(0). If that's unlikely to happen, then we
are good.
Thank you.
---
- Prasad
On Thu, May 23, 2024 at 04:05:31PM -0300, Fabiano Rosas wrote: > When the "file:" migration support was added we missed the special > case in the qemu_open_old implementation that allows for a particular > file name format to be used to refer to a set of file descriptors that > have been previously provided to QEMU via the add-fd QMP command. > > When using this fdset feature, we should not truncate the migration > file because being given an fd means that the management layer is in > control of the file and will likely already have some data written to > it. This is further indicated by the presence of the 'offset' > argument, which indicates the start of the region where QEMU is > allowed to write. > > Fix the issue by replacing the O_TRUNC flag on open by an ftruncate > call, which will take the offset into consideration. > > Fixes: 385f510df5 ("migration: file URI offset") > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Peter Xu <peterx@redhat.com> Right below the change, did we forget to free the ioc if qio_channel_io_seek() failed? > --- > migration/file.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/migration/file.c b/migration/file.c > index ab18ba505a..ba5b5c44ff 100644 > --- a/migration/file.c > +++ b/migration/file.c > @@ -84,12 +84,19 @@ void file_start_outgoing_migration(MigrationState *s, > > trace_migration_file_outgoing(filename); > > - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, > - 0600, errp); > + fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY, 0600, errp); > if (!fioc) { > return; > } > > + if (ftruncate(fioc->fd, offset)) { > + error_setg_errno(errp, errno, > + "failed to truncate migration file to offset %" PRIx64, > + offset); > + object_unref(OBJECT(fioc)); > + return; > + } > + > outgoing_args.fname = g_strdup(filename); > > ioc = QIO_CHANNEL(fioc); > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Thu, May 23, 2024 at 04:05:31PM -0300, Fabiano Rosas wrote: >> When the "file:" migration support was added we missed the special >> case in the qemu_open_old implementation that allows for a particular >> file name format to be used to refer to a set of file descriptors that >> have been previously provided to QEMU via the add-fd QMP command. >> >> When using this fdset feature, we should not truncate the migration >> file because being given an fd means that the management layer is in >> control of the file and will likely already have some data written to >> it. This is further indicated by the presence of the 'offset' >> argument, which indicates the start of the region where QEMU is >> allowed to write. >> >> Fix the issue by replacing the O_TRUNC flag on open by an ftruncate >> call, which will take the offset into consideration. >> >> Fixes: 385f510df5 ("migration: file URI offset") >> Suggested-by: Daniel P. Berrangé <berrange@redhat.com> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> > > Reviewed-by: Peter Xu <peterx@redhat.com> Thanks > > Right below the change, did we forget to free the ioc if > qio_channel_io_seek() failed? > Yep, looks like it. I'll fix it. >> --- >> migration/file.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/migration/file.c b/migration/file.c >> index ab18ba505a..ba5b5c44ff 100644 >> --- a/migration/file.c >> +++ b/migration/file.c >> @@ -84,12 +84,19 @@ void file_start_outgoing_migration(MigrationState *s, >> >> trace_migration_file_outgoing(filename); >> >> - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, >> - 0600, errp); >> + fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY, 0600, errp); >> if (!fioc) { >> return; >> } >> >> + if (ftruncate(fioc->fd, offset)) { >> + error_setg_errno(errp, errno, >> + "failed to truncate migration file to offset %" PRIx64, >> + offset); >> + object_unref(OBJECT(fioc)); >> + return; >> + } >> + >> outgoing_args.fname = g_strdup(filename); >> >> ioc = QIO_CHANNEL(fioc); >> -- >> 2.35.3 >>
On Thu, May 23, 2024 at 04:05:31PM -0300, Fabiano Rosas wrote: > When the "file:" migration support was added we missed the special > case in the qemu_open_old implementation that allows for a particular > file name format to be used to refer to a set of file descriptors that > have been previously provided to QEMU via the add-fd QMP command. > > When using this fdset feature, we should not truncate the migration > file because being given an fd means that the management layer is in > control of the file and will likely already have some data written to > it. This is further indicated by the presence of the 'offset' > argument, which indicates the start of the region where QEMU is > allowed to write. > > Fix the issue by replacing the O_TRUNC flag on open by an ftruncate > call, which will take the offset into consideration. > > Fixes: 385f510df5 ("migration: file URI offset") > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/file.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
diff --git a/migration/file.c b/migration/file.c index ab18ba505a..ba5b5c44ff 100644 --- a/migration/file.c +++ b/migration/file.c @@ -84,12 +84,19 @@ void file_start_outgoing_migration(MigrationState *s, trace_migration_file_outgoing(filename); - fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, - 0600, errp); + fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY, 0600, errp); if (!fioc) { return; } + if (ftruncate(fioc->fd, offset)) { + error_setg_errno(errp, errno, + "failed to truncate migration file to offset %" PRIx64, + offset); + object_unref(OBJECT(fioc)); + return; + } + outgoing_args.fname = g_strdup(filename); ioc = QIO_CHANNEL(fioc);
When the "file:" migration support was added we missed the special case in the qemu_open_old implementation that allows for a particular file name format to be used to refer to a set of file descriptors that have been previously provided to QEMU via the add-fd QMP command. When using this fdset feature, we should not truncate the migration file because being given an fd means that the management layer is in control of the file and will likely already have some data written to it. This is further indicated by the presence of the 'offset' argument, which indicates the start of the region where QEMU is allowed to write. Fix the issue by replacing the O_TRUNC flag on open by an ftruncate call, which will take the offset into consideration. Fixes: 385f510df5 ("migration: file URI offset") Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/file.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)