| Message ID | 1443515898-3594-17-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 29, 2015 at 09:37:40AM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Postcopy needs a method to send messages from the destination back to > the source, this is the 'return path'. > > Wire it up for 'socket' QEMUFile's. I find this to be a pretty wierd approach to the problem. THe underlying transport is bi-directional, so I would expect to have a single QEMUFile object that allowed bi-directional I/O on it, rather than creating a second QEMUFile for the back channel, which was forbidden from closing the shared FD. I can understand why you've done this though - since we only have a single buffer embedded in QEMUFile. I wonder though if we'd be better off changing QEMUFile to have a 'inbuf' and 'outbuf' intead of just 'buf' and likewise iniov & outiov. Then we can allow bi-directional I/O on the single QEMUFile object which is a more natural fit. Regards, Daniel
* Daniel P. Berrange (berrange@redhat.com) wrote: > On Tue, Sep 29, 2015 at 09:37:40AM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Postcopy needs a method to send messages from the destination back to > > the source, this is the 'return path'. > > > > Wire it up for 'socket' QEMUFile's. > > I find this to be a pretty wierd approach to the problem. THe underlying > transport is bi-directional, so I would expect to have a single QEMUFile > object that allowed bi-directional I/O on it, rather than creating a > second QEMUFile for the back channel, which was forbidden from closing > the shared FD. > > I can understand why you've done this though - since we only have a > single buffer embedded in QEMUFile. I wonder though if we'd be better > off changing QEMUFile to have a 'inbuf' and 'outbuf' intead of just > 'buf' and likewise iniov & outiov. Then we can allow bi-directional > I/O on the single QEMUFile object which is a more natural fit. The 'c' FILE* is one directional, and I just took it that the QEMUFile* is like that; i.e. a buffered layer on top of an underlying one directional transport. stdin,stdout are two separate FILE*'s. Your iniov, outiov would be basically the same, so you'd end up duplicating code for the in and out parts; where as what you really have is two of the same thing wired up in opposite directions. Having said that, for things like RDMA, they have to do special stuff for each direction and the QEMUFile is really a shim on top of that. Dave > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Oct 02, 2015 at 05:32:18PM +0100, Dr. David Alan Gilbert wrote: > * Daniel P. Berrange (berrange@redhat.com) wrote: > > On Tue, Sep 29, 2015 at 09:37:40AM +0100, Dr. David Alan Gilbert (git) wrote: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > Postcopy needs a method to send messages from the destination back to > > > the source, this is the 'return path'. > > > > > > Wire it up for 'socket' QEMUFile's. > > > > I find this to be a pretty wierd approach to the problem. THe underlying > > transport is bi-directional, so I would expect to have a single QEMUFile > > object that allowed bi-directional I/O on it, rather than creating a > > second QEMUFile for the back channel, which was forbidden from closing > > the shared FD. > > > > I can understand why you've done this though - since we only have a > > single buffer embedded in QEMUFile. I wonder though if we'd be better > > off changing QEMUFile to have a 'inbuf' and 'outbuf' intead of just > > 'buf' and likewise iniov & outiov. Then we can allow bi-directional > > I/O on the single QEMUFile object which is a more natural fit. > > The 'c' FILE* is one directional, and I just took it that the QEMUFile* is > like that; i.e. a buffered layer on top of an underlying one directional > transport. stdin,stdout are two separate FILE*'s. Yep, QEMUFile was really designed as a FILE* alternative, so makes sense from that POV. > Your iniov, outiov would be basically the same, so you'd end up duplicating > code for the in and out parts; where as what you really have is two of the same > thing wired up in opposite directions. I don't think it'd actually end up duplicating any code - mostly just updating which variable was accessed in each existing method, depending on whether it was a read or write related method. > Having said that, for things like RDMA, they have to do special stuff for > each direction and the QEMUFile is really a shim on top of that. Similarly when we add TLS into the mix, there is a single shared TLS session context that is used by both I/O directionals. Now this would not be visible to the QEMUFile regardless, since its hidden in the QIOChannel object I'm defining, so its not a show stopper either but I guess my general thought is that there is a mixture of state that we maintain some different for read vs write and some shared. You workaround the fact that the FD is shared by having a comment saying we should not call close() on the FD kept by the QEMUFile for the return path. All that said, I don't think it is too critical to change this right now. It would be fine to leave it to a later date, unless there's a more pressing reason. Regards, Daniel
diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h index 865f897..4c89a2c 100644 --- a/include/migration/qemu-file.h +++ b/include/migration/qemu-file.h @@ -89,6 +89,11 @@ typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque, uint64_t *bytes_sent); /* + * Return a QEMUFile for comms in the opposite direction + */ +typedef QEMUFile *(QEMURetPathFunc)(void *opaque); + +/* * Stop any read or write (depending on flags) on the underlying * transport on the QEMUFile. * Existing blocking reads/writes must be woken @@ -106,6 +111,7 @@ typedef struct QEMUFileOps { QEMURamHookFunc *after_ram_iterate; QEMURamHookFunc *hook_ram_load; QEMURamSaveFunc *save_page; + QEMURetPathFunc *get_return_path; QEMUFileShutdownFunc *shut_down; } QEMUFileOps; @@ -196,6 +202,7 @@ int64_t qemu_file_get_rate_limit(QEMUFile *f); int qemu_file_get_error(QEMUFile *f); void qemu_file_set_error(QEMUFile *f, int ret); int qemu_file_shutdown(QEMUFile *f); +QEMUFile *qemu_file_get_return_path(QEMUFile *f); void qemu_fflush(QEMUFile *f); void qemu_file_set_blocking(QEMUFile *f, bool block); diff --git a/migration/qemu-file-unix.c b/migration/qemu-file-unix.c index bfbc086..dd463ff 100644 --- a/migration/qemu-file-unix.c +++ b/migration/qemu-file-unix.c @@ -96,6 +96,56 @@ static int socket_shutdown(void *opaque, bool rd, bool wr) } } +static int socket_return_close(void *opaque) +{ + QEMUFileSocket *s = opaque; + /* + * Note: We don't close the socket, that should be done by the forward + * path. + */ + g_free(s); + return 0; +} + +static const QEMUFileOps socket_return_read_ops = { + .get_fd = socket_get_fd, + .get_buffer = socket_get_buffer, + .close = socket_return_close, + .shut_down = socket_shutdown, +}; + +static const QEMUFileOps socket_return_write_ops = { + .get_fd = socket_get_fd, + .writev_buffer = socket_writev_buffer, + .close = socket_return_close, + .shut_down = socket_shutdown, +}; + +/* + * Give a QEMUFile* off the same socket but data in the opposite + * direction. + */ +static QEMUFile *socket_get_return_path(void *opaque) +{ + QEMUFileSocket *forward = opaque; + QEMUFileSocket *reverse; + + if (qemu_file_get_error(forward->file)) { + /* If the forward file is in error, don't try and open a return */ + return NULL; + } + + reverse = g_malloc0(sizeof(QEMUFileSocket)); + reverse->fd = forward->fd; + /* I don't think there's a better way to tell which direction 'this' is */ + if (forward->file->ops->get_buffer != NULL) { + /* being called from the read side, so we need to be able to write */ + return qemu_fopen_ops(reverse, &socket_return_write_ops); + } else { + return qemu_fopen_ops(reverse, &socket_return_read_ops); + } +} + static ssize_t unix_writev_buffer(void *opaque, struct iovec *iov, int iovcnt, int64_t pos) { @@ -204,18 +254,19 @@ QEMUFile *qemu_fdopen(int fd, const char *mode) } static const QEMUFileOps socket_read_ops = { - .get_fd = socket_get_fd, - .get_buffer = socket_get_buffer, - .close = socket_close, - .shut_down = socket_shutdown - + .get_fd = socket_get_fd, + .get_buffer = socket_get_buffer, + .close = socket_close, + .shut_down = socket_shutdown, + .get_return_path = socket_get_return_path }; static const QEMUFileOps socket_write_ops = { - .get_fd = socket_get_fd, - .writev_buffer = socket_writev_buffer, - .close = socket_close, - .shut_down = socket_shutdown + .get_fd = socket_get_fd, + .writev_buffer = socket_writev_buffer, + .close = socket_close, + .shut_down = socket_shutdown, + .get_return_path = socket_get_return_path }; QEMUFile *qemu_fopen_socket(int fd, const char *mode) diff --git a/migration/qemu-file.c b/migration/qemu-file.c index 3c64a9c..e188b69 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -44,6 +44,18 @@ int qemu_file_shutdown(QEMUFile *f) return f->ops->shut_down(f->opaque, true, true); } +/* + * Result: QEMUFile* for a 'return path' for comms in the opposite direction + * NULL if not available + */ +QEMUFile *qemu_file_get_return_path(QEMUFile *f) +{ + if (!f->ops->get_return_path) { + return NULL; + } + return f->ops->get_return_path(f->opaque); +} + bool qemu_file_mode_is_not_valid(const char *mode) { if (mode == NULL ||