On Mon, 18 Feb 2019 13:06:57 +0000
Jean-Philippe Brucker <jean-philippe.brucker@xxxxxxx> wrote:
> Move all AIO code to a separate file, disk/aio.c, to remove as much
> #ifdefs as possible. Split the raw read/write disk ops into async and
> sync, and choose which ones to use depending on CONFIG_HAS_AIO. Note that
> we fix raw_image__close() which incorrectly checked CONFIG_HAS_VIRTIO
> instead of CONFIG_HAS_AIO, and closed an unitialized disk->evt. A
> subsequent commit will complete this refactoring by fixing use of the
> 'async' disk attribute.
So I chased every function and line down from where it left to where it
went, and it seems like a valid transformation to me.
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@xxxxxxx>
Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
Thanks!
Andre.
> ---
> Makefile | 2 +
> disk/aio.c | 111 +++++++++++++++++++++++++++++++++++++++
> disk/core.c | 52 +++++-------------
> disk/raw.c | 39 +++-----------
> include/kvm/disk-image.h | 41 ++++++++++++---
> include/kvm/read-write.h | 11 ----
> util/read-write.c | 36 -------------
> 7 files changed, 167 insertions(+), 125 deletions(-)
> create mode 100644 disk/aio.c
>
> diff --git a/Makefile b/Makefile
> index ec75cd999..89acec2d5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -275,10 +275,12 @@ endif
> ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio),y)
> CFLAGS_DYNOPT += -DCONFIG_HAS_AIO
> LIBS_DYNOPT += -laio
> + OBJS += disk/aio.o
> else
> ifeq ($(call try-build,$(SOURCE_AIO),$(CFLAGS),$(LDFLAGS) -laio -static),y)
> CFLAGS_STATOPT += -DCONFIG_HAS_AIO
> LIBS_STATOPT += -laio
> + OBJS += disk/aio.o
> else
> NOTFOUND += aio
> endif
> diff --git a/disk/aio.c b/disk/aio.c
> new file mode 100644
> index 000000000..6afcffe5a
> --- /dev/null
> +++ b/disk/aio.c
> @@ -0,0 +1,111 @@
> +#include <libaio.h>
> +#include <pthread.h>
> +#include <sys/eventfd.h>
> +
> +#include "kvm/disk-image.h"
> +#include "kvm/kvm.h"
> +#include "linux/list.h"
> +
> +#define AIO_MAX 256
> +
> +static int aio_pwritev(io_context_t ctx, struct iocb *iocb, int fd,
> + const struct iovec *iov, int iovcnt, off_t offset,
> + int ev, void *param)
> +{
> + struct iocb *ios[1] = { iocb };
> + int ret;
> +
> + io_prep_pwritev(iocb, fd, iov, iovcnt, offset);
> + io_set_eventfd(iocb, ev);
> + iocb->data = param;
> +
> +restart:
> + ret = io_submit(ctx, 1, ios);
> + if (ret == -EAGAIN)
> + goto restart;
> + return ret;
> +}
> +
> +static int aio_preadv(io_context_t ctx, struct iocb *iocb, int fd,
> + const struct iovec *iov, int iovcnt, off_t offset,
> + int ev, void *param)
> +{
> + struct iocb *ios[1] = { iocb };
> + int ret;
> +
> + io_prep_preadv(iocb, fd, iov, iovcnt, offset);
> + io_set_eventfd(iocb, ev);
> + iocb->data = param;
> +
> +restart:
> + ret = io_submit(ctx, 1, ios);
> + if (ret == -EAGAIN)
> + goto restart;
> + return ret;
> +}
> +
> +ssize_t raw_image__read_async(struct disk_image *disk, u64 sector,
> + const struct iovec *iov, int iovcount,
> + void *param)
> +{
> + u64 offset = sector << SECTOR_SHIFT;
> + struct iocb iocb;
> +
> + return aio_preadv(disk->ctx, &iocb, disk->fd, iov, iovcount,
> + offset, disk->evt, param);
> +}
> +
> +ssize_t raw_image__write_async(struct disk_image *disk, u64 sector,
> + const struct iovec *iov, int iovcount,
> + void *param)
> +{
> + u64 offset = sector << SECTOR_SHIFT;
> + struct iocb iocb;
> +
> + return aio_pwritev(disk->ctx, &iocb, disk->fd, iov, iovcount,
> + offset, disk->evt, param);
> +}
> +
> +static void *disk_aio_thread(void *param)
> +{
> + struct disk_image *disk = param;
> + struct io_event event[AIO_MAX];
> + struct timespec notime = {0};
> + int nr, i;
> + u64 dummy;
> +
> + kvm__set_thread_name("disk-image-io");
> +
> + while (read(disk->evt, &dummy, sizeof(dummy)) > 0) {
> + nr = io_getevents(disk->ctx, 1, ARRAY_SIZE(event), event, ¬ime);
> + for (i = 0; i < nr; i++)
> + disk->disk_req_cb(event[i].data, event[i].res);
> + }
> +
> + return NULL;
> +}
> +
> +int disk_aio_setup(struct disk_image *disk)
> +{
> + int r;
> + pthread_t thread;
> +
> + disk->evt = eventfd(0, 0);
> + if (disk->evt < 0)
> + return -errno;
> +
> + io_setup(AIO_MAX, &disk->ctx);
> + r = pthread_create(&thread, NULL, disk_aio_thread, disk);
> + if (r) {
> + r = -errno;
> + close(disk->evt);
> + return r;
> + }
> + return 0;
> +}
> +
> +void disk_aio_destroy(struct disk_image *disk)
> +{
> + close(disk->evt);
> + io_destroy(disk->ctx);
> +}
> diff --git a/disk/core.c b/disk/core.c
> index 4c7c4f030..89880703e 100644
> --- a/disk/core.c
> +++ b/disk/core.c
> @@ -4,11 +4,8 @@
> #include "kvm/kvm.h"
>
> #include <linux/err.h>
> -#include <sys/eventfd.h>
> #include <poll.h>
>
> -#define AIO_MAX 256
> -
> int debug_iodelay;
>
> static int disk_image__close(struct disk_image *disk);
> @@ -54,27 +51,6 @@ int disk_img_name_parser(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> -#ifdef CONFIG_HAS_AIO
> -static void *disk_image__thread(void *param)
> -{
> - struct disk_image *disk = param;
> - struct io_event event[AIO_MAX];
> - struct timespec notime = {0};
> - int nr, i;
> - u64 dummy;
> -
> - kvm__set_thread_name("disk-image-io");
> -
> - while (read(disk->evt, &dummy, sizeof(dummy)) > 0) {
> - nr = io_getevents(disk->ctx, 1, ARRAY_SIZE(event), event, ¬ime);
> - for (i = 0; i < nr; i++)
> - disk->disk_req_cb(event[i].data, event[i].res);
> - }
> -
> - return NULL;
> -}
> -#endif
> -
> struct disk_image *disk_image__new(int fd, u64 size,
> struct disk_image_operations *ops,
> int use_mmap)
> @@ -99,26 +75,22 @@ struct disk_image *disk_image__new(int fd, u64 size,
> disk->priv = mmap(NULL, size, PROT_RW, MAP_PRIVATE | MAP_NORESERVE, fd, 0);
> if (disk->priv == MAP_FAILED) {
> r = -errno;
> - free(disk);
> - return ERR_PTR(r);
> + goto err_free_disk;
> }
> }
>
> -#ifdef CONFIG_HAS_AIO
> - {
> - pthread_t thread;
> + r = disk_aio_setup(disk);
> + if (r)
> + goto err_unmap_disk;
>
> - disk->evt = eventfd(0, 0);
> - io_setup(AIO_MAX, &disk->ctx);
> - r = pthread_create(&thread, NULL, disk_image__thread, disk);
> - if (r) {
> - r = -errno;
> - free(disk);
> - return ERR_PTR(r);
> - }
> - }
> -#endif
> return disk;
> +
> +err_unmap_disk:
> + if (disk->priv)
> + munmap(disk->priv, size);
> +err_free_disk:
> + free(disk);
> + return ERR_PTR(r);
> }
>
> static struct disk_image *disk_image__open(const char *filename, bool readonly, bool direct)
> @@ -243,6 +215,8 @@ static int disk_image__close(struct disk_image *disk)
> if (!disk)
> return 0;
>
> + disk_aio_destroy(disk);
> +
> if (disk->ops->close)
> return disk->ops->close(disk);
>
> diff --git a/disk/raw.c b/disk/raw.c
> index 93b2b4e8d..09da7e081 100644
> --- a/disk/raw.c
> +++ b/disk/raw.c
> @@ -2,38 +2,17 @@
>
> #include <linux/err.h>
>
> -#ifdef CONFIG_HAS_AIO
> -#include <libaio.h>
> -#endif
> -
> -ssize_t raw_image__read(struct disk_image *disk, u64 sector, const struct iovec *iov,
> +ssize_t raw_image__read_sync(struct disk_image *disk, u64 sector, const struct iovec *iov,
> int iovcount, void *param)
> {
> - u64 offset = sector << SECTOR_SHIFT;
> -
> -#ifdef CONFIG_HAS_AIO
> - struct iocb iocb;
> -
> - return aio_preadv(disk->ctx, &iocb, disk->fd, iov, iovcount, offset,
> - disk->evt, param);
> -#else
> - return preadv_in_full(disk->fd, iov, iovcount, offset);
> -#endif
> + return preadv_in_full(disk->fd, iov, iovcount, sector << SECTOR_SHIFT);
> }
>
> -ssize_t raw_image__write(struct disk_image *disk, u64 sector, const struct iovec *iov,
> - int iovcount, void *param)
> +ssize_t raw_image__write_sync(struct disk_image *disk, u64 sector,
> + const struct iovec *iov, int iovcount,
> + void *param)
> {
> - u64 offset = sector << SECTOR_SHIFT;
> -
> -#ifdef CONFIG_HAS_AIO
> - struct iocb iocb;
> -
> - return aio_pwritev(disk->ctx, &iocb, disk->fd, iov, iovcount, offset,
> - disk->evt, param);
> -#else
> - return pwritev_in_full(disk->fd, iov, iovcount, offset);
> -#endif
> + return pwritev_in_full(disk->fd, iov, iovcount, sector << SECTOR_SHIFT);
> }
>
> ssize_t raw_image__read_mmap(struct disk_image *disk, u64 sector, const struct iovec *iov,
> @@ -79,12 +58,6 @@ int raw_image__close(struct disk_image *disk)
> if (disk->priv != MAP_FAILED)
> ret = munmap(disk->priv, disk->size);
>
> - close(disk->evt);
> -
> -#ifdef CONFIG_HAS_VIRTIO
> - io_destroy(disk->ctx);
> -#endif
> -
> return ret;
> }
>
> diff --git a/include/kvm/disk-image.h b/include/kvm/disk-image.h
> index 4746e88c9..953beb2d5 100644
> --- a/include/kvm/disk-image.h
> +++ b/include/kvm/disk-image.h
> @@ -19,6 +19,10 @@
> #include <unistd.h>
> #include <fcntl.h>
>
> +#ifdef CONFIG_HAS_AIO
> +#include <libaio.h>
> +#endif
> +
> #define SECTOR_SHIFT 9
> #define SECTOR_SIZE (1UL << SECTOR_SHIFT)
>
> @@ -61,10 +65,10 @@ struct disk_image {
> void (*disk_req_cb)(void *param, long len);
> bool readonly;
> bool async;
> - int evt;
> #ifdef CONFIG_HAS_AIO
> io_context_t ctx;
> -#endif
> + int evt;
> +#endif /* CONFIG_HAS_AIO */
> const char *wwpn;
> const char *tpgt;
> int debug_iodelay;
> @@ -84,14 +88,39 @@ ssize_t disk_image__get_serial(struct disk_image *disk, void *buffer, ssize_t *l
> struct disk_image *raw_image__probe(int fd, struct stat *st, bool readonly);
> struct disk_image *blkdev__probe(const char *filename, int flags, struct stat *st);
>
> -ssize_t raw_image__read(struct disk_image *disk, u64 sector,
> - const struct iovec *iov, int iovcount, void *param);
> -ssize_t raw_image__write(struct disk_image *disk, u64 sector,
> - const struct iovec *iov, int iovcount, void *param);
> +ssize_t raw_image__read_sync(struct disk_image *disk, u64 sector,
> + const struct iovec *iov, int iovcount, void *param);
> +ssize_t raw_image__write_sync(struct disk_image *disk, u64 sector,
> + const struct iovec *iov, int iovcount, void *param);
> ssize_t raw_image__read_mmap(struct disk_image *disk, u64 sector,
> const struct iovec *iov, int iovcount, void *param);
> ssize_t raw_image__write_mmap(struct disk_image *disk, u64 sector,
> const struct iovec *iov, int iovcount, void *param);
> int raw_image__close(struct disk_image *disk);
> void disk_image__set_callback(struct disk_image *disk, void (*disk_req_cb)(void *param, long len));
> +
> +#ifdef CONFIG_HAS_AIO
> +int disk_aio_setup(struct disk_image *disk);
> +void disk_aio_destroy(struct disk_image *disk);
> +ssize_t raw_image__read_async(struct disk_image *disk, u64 sector,
> + const struct iovec *iov, int iovcount, void *param);
> +ssize_t raw_image__write_async(struct disk_image *disk, u64 sector,
> + const struct iovec *iov, int iovcount, void *param);
> +
> +#define raw_image__read raw_image__read_async
> +#define raw_image__write raw_image__write_async
> +
> +#else /* !CONFIG_HAS_AIO */
> +static inline int disk_aio_setup(struct disk_image *disk)
> +{
> + /* No-op */
> + return 0;
> +}
> +static inline void disk_aio_destroy(struct disk_image *disk)
> +{
> +}
> +#define raw_image__read raw_image__read_sync
> +#define raw_image__write raw_image__write_sync
> +#endif /* CONFIG_HAS_AIO */
> +
> #endif /* KVM__DISK_IMAGE_H */
> diff --git a/include/kvm/read-write.h b/include/kvm/read-write.h
> index acbd6f0b1..8375d7c7d 100644
> --- a/include/kvm/read-write.h
> +++ b/include/kvm/read-write.h
> @@ -5,10 +5,6 @@
> #include <sys/uio.h>
> #include <unistd.h>
>
> -#ifdef CONFIG_HAS_AIO
> -#include <libaio.h>
> -#endif
> -
> ssize_t xread(int fd, void *buf, size_t count);
> ssize_t xwrite(int fd, const void *buf, size_t count);
>
> @@ -35,11 +31,4 @@ ssize_t xpwritev(int fd, const struct iovec *iov, int iovcnt, off_t offset);
> ssize_t preadv_in_full(int fd, const struct iovec *iov, int iovcnt, off_t offset);
> ssize_t pwritev_in_full(int fd, const struct iovec *iov, int iovcnt, off_t offset);
>
> -#ifdef CONFIG_HAS_AIO
> -int aio_preadv(io_context_t ctx, struct iocb *iocb, int fd, const struct iovec *iov, int iovcnt,
> - off_t offset, int ev, void *param);
> -int aio_pwritev(io_context_t ctx, struct iocb *iocb, int fd, const struct iovec *iov, int iovcnt,
> - off_t offset, int ev, void *param);
> -#endif
> -
> #endif /* KVM_READ_WRITE_H */
> diff --git a/util/read-write.c b/util/read-write.c
> index bf6fb2fc2..06fc0dfff 100644
> --- a/util/read-write.c
> +++ b/util/read-write.c
> @@ -337,39 +337,3 @@ ssize_t pwritev_in_full(int fd, const struct iovec *iov, int iovcnt, off_t offse
>
> return total;
> }
> -
> -#ifdef CONFIG_HAS_AIO
> -int aio_pwritev(io_context_t ctx, struct iocb *iocb, int fd, const struct iovec *iov, int iovcnt,
> - off_t offset, int ev, void *param)
> -{
> - struct iocb *ios[1] = { iocb };
> - int ret;
> -
> - io_prep_pwritev(iocb, fd, iov, iovcnt, offset);
> - io_set_eventfd(iocb, ev);
> - iocb->data = param;
> -
> -restart:
> - ret = io_submit(ctx, 1, ios);
> - if (ret == -EAGAIN)
> - goto restart;
> - return ret;
> -}
> -
> -int aio_preadv(io_context_t ctx, struct iocb *iocb, int fd, const struct iovec *iov, int iovcnt,
> - off_t offset, int ev, void *param)
> -{
> - struct iocb *ios[1] = { iocb };
> - int ret;
> -
> - io_prep_preadv(iocb, fd, iov, iovcnt, offset);
> - io_set_eventfd(iocb, ev);
> - iocb->data = param;
> -
> -restart:
> - ret = io_submit(ctx, 1, ios);
> - if (ret == -EAGAIN)
> - goto restart;
> - return ret;
> -}
> -#endif
