[PATCH] Enable async I/O by default
Jeremy Allison
jra at samba.org
Tue Dec 12 16:45:15 UTC 2017
On Tue, Dec 12, 2017 at 10:31:55AM +0100, Volker Lendecke via samba-technical wrote:
> Hi!
>
> Review appreciated!
Oh that looks *great*. I'm especially pleased it found some bugs
in old code that hadn't been tested in years :-). Well done Volker !
Jeremy.
> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de
> From d5443f85dedee6c396928e6e71cf9136dbbd83f4 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Thu, 7 Dec 2017 18:12:28 +0100
> Subject: [PATCH 1/7] smbd: Fix async large read
>
> We also do the 128k reads asynchronously, just not the huge 24MB
> ones. smb_setlen does not work well for >64k.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/smbd/aio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
> index d3611baae6a..4fc1132be87 100644
> --- a/source3/smbd/aio.c
> +++ b/source3/smbd/aio.c
> @@ -305,7 +305,7 @@ static void aio_pread_smb1_done(struct tevent_req *req)
> (int)aio_ex->nbyte, (int)nread ) );
>
> }
> - smb_setlen(outbuf, outsize - 4);
> + _smb_setlen_large(outbuf, outsize - 4);
> show_msg(outbuf);
> if (!srv_send_smb(aio_ex->smbreq->xconn, outbuf,
> true, aio_ex->smbreq->seqnum+1,
> --
> 2.11.0
>
>
> From 53ee7ef79d3ddc4dc4c00d107366b2d45d575e69 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 8 Dec 2017 14:30:46 +0100
> Subject: [PATCH 2/7] vfs_aio_fork: Drop "volatile" from the mmap area in
> aio_fork
>
> We don't do that in tdb either, and the mmap/memcpy prototypes don't
> have it either
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/modules/vfs_aio_fork.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c
> index 8e47531ebd1..4069d935d24 100644
> --- a/source3/modules/vfs_aio_fork.c
> +++ b/source3/modules/vfs_aio_fork.c
> @@ -47,7 +47,7 @@ struct aio_fork_config {
>
> struct mmap_area {
> size_t size;
> - volatile void *ptr;
> + void *ptr;
> };
>
> static int mmap_area_destructor(struct mmap_area *area)
> --
> 2.11.0
>
>
> From 3974d118ec0179e982d24ccf3ba0a53a033acb42 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Thu, 7 Dec 2017 20:53:18 +0100
> Subject: [PATCH 3/7] vfs_aio_fork: Fix a crash in aio_fork
>
> Since the introduction of the vfs_aio_fork:erratic_testing_mode this
> crashed reliably, as we had two different structs behind
> SMB_VFS_HANDLE_SET_DATA. I had always believed that due to the fact that
> we have specific aio_fork tests in our autobuild, this would have been
> tested. But it was not, because the share definition missed the the "aio
> read/write size = 1" to actually use the async code in vfs_aio_fork.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/modules/vfs_aio_fork.c | 44 ++++++++++++++++++------------------------
> 1 file changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c
> index 4069d935d24..3eaa26774f5 100644
> --- a/source3/modules/vfs_aio_fork.c
> +++ b/source3/modules/vfs_aio_fork.c
> @@ -41,8 +41,11 @@
> #define MAP_FILE 0
> #endif
>
> +struct aio_child_list;
> +
> struct aio_fork_config {
> bool erratic_testing_mode;
> + struct aio_child_list *children;
> };
>
> struct mmap_area {
> @@ -149,11 +152,6 @@ struct aio_child_list {
> struct tevent_timer *cleanup_event;
> };
>
> -static void free_aio_children(void **p)
> -{
> - TALLOC_FREE(*p);
> -}
> -
> static ssize_t read_fd(int fd, void *ptr, size_t nbytes, int *recvfd)
> {
> struct iovec iov[1];
> @@ -267,19 +265,19 @@ static void aio_child_cleanup(struct tevent_context *event_ctx,
>
> static struct aio_child_list *init_aio_children(struct vfs_handle_struct *handle)
> {
> - struct aio_child_list *data = NULL;
> + struct aio_fork_config *config;
> + struct aio_child_list *children;
>
> - if (SMB_VFS_HANDLE_TEST_DATA(handle)) {
> - SMB_VFS_HANDLE_GET_DATA(handle, data, struct aio_child_list,
> - return NULL);
> - }
> + SMB_VFS_HANDLE_GET_DATA(handle, config, struct aio_fork_config,
> + return NULL);
>
> - if (data == NULL) {
> - data = talloc_zero(NULL, struct aio_child_list);
> - if (data == NULL) {
> + if (config->children == NULL) {
> + config->children = talloc_zero(config, struct aio_child_list);
> + if (config->children == NULL) {
> return NULL;
> }
> }
> + children = config->children;
>
> /*
> * Regardless of whether the child_list had been around or not, make
> @@ -287,22 +285,18 @@ static struct aio_child_list *init_aio_children(struct vfs_handle_struct *handle
> * delete itself when it finds that no children are around anymore.
> */
>
> - if (data->cleanup_event == NULL) {
> - data->cleanup_event = tevent_add_timer(server_event_context(), data,
> - timeval_current_ofs(30, 0),
> - aio_child_cleanup, data);
> - if (data->cleanup_event == NULL) {
> - TALLOC_FREE(data);
> + if (children->cleanup_event == NULL) {
> + children->cleanup_event =
> + tevent_add_timer(server_event_context(), children,
> + timeval_current_ofs(30, 0),
> + aio_child_cleanup, children);
> + if (children->cleanup_event == NULL) {
> + TALLOC_FREE(config->children);
> return NULL;
> }
> }
>
> - if (!SMB_VFS_HANDLE_TEST_DATA(handle)) {
> - SMB_VFS_HANDLE_SET_DATA(handle, data, free_aio_children,
> - struct aio_child_list, return False);
> - }
> -
> - return data;
> + return children;
> }
>
> static void aio_child_loop(int sockfd, struct mmap_area *map)
> --
> 2.11.0
>
>
> From 239fe6c9e509ad4e3399f8dc03844ac834a3d4b7 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 8 Dec 2017 14:07:06 +0100
> Subject: [PATCH 4/7] vfs_aio_fork: Fix vfs_aio_pread
>
> Copy the data that the child read into the caller's buffer. This can't
> have been used in half a decade at least...
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/modules/vfs_aio_fork.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c
> index 3eaa26774f5..1538ed9cf46 100644
> --- a/source3/modules/vfs_aio_fork.c
> +++ b/source3/modules/vfs_aio_fork.c
> @@ -534,6 +534,8 @@ static int get_idle_child(struct vfs_handle_struct *handle,
>
> struct aio_fork_pread_state {
> struct aio_child *child;
> + size_t n;
> + void *data;
> ssize_t ret;
> struct vfs_aio_state vfs_aio_state;
> };
> @@ -562,6 +564,8 @@ static struct tevent_req *aio_fork_pread_send(struct vfs_handle_struct *handle,
> if (req == NULL) {
> return NULL;
> }
> + state->n = n;
> + state->data = data;
>
> if (n > 128*1024) {
> /* TODO: support variable buffers */
> @@ -629,12 +633,20 @@ static void aio_fork_pread_done(struct tevent_req *subreq)
> return;
> }
>
> - state->child->busy = false;
> -
> retbuf = (struct rw_ret *)buf;
> state->ret = retbuf->size;
> state->vfs_aio_state.error = retbuf->ret_errno;
> state->vfs_aio_state.duration = retbuf->duration;
> +
> + if ((size_t)state->ret > state->n) {
> + tevent_req_error(req, EIO);
> + state->child->busy = false;
> + return;
> + }
> + memcpy(state->data, state->child->map->ptr, state->ret);
> +
> + state->child->busy = false;
> +
> tevent_req_done(req);
> }
>
> --
> 2.11.0
>
>
> From 64ae1f47129f3ba10e90d066afd87580e20033cc Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 8 Dec 2017 14:07:47 +0100
> Subject: [PATCH 5/7] vfs_aio_fork: Fix vfs_aio_pwrite
>
> Make the data to write available to the child
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/modules/vfs_aio_fork.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c
> index 1538ed9cf46..40fba4dbdc0 100644
> --- a/source3/modules/vfs_aio_fork.c
> +++ b/source3/modules/vfs_aio_fork.c
> @@ -703,6 +703,8 @@ static struct tevent_req *aio_fork_pwrite_send(
> return tevent_req_post(req, ev);
> }
>
> + memcpy(state->child->map->ptr, data, n);
> +
> ZERO_STRUCT(cmd);
> cmd.n = n;
> cmd.offset = offset;
> --
> 2.11.0
>
>
> From db0faefaeda3a98241aa088b644901ff37043838 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 11 Dec 2017 17:32:40 +0100
> Subject: [PATCH 6/7] vfs_aio_fork: Use a shorter random delay
>
> Otherwise the rw2 test takes ages for no good reason
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/modules/vfs_aio_fork.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c
> index 40fba4dbdc0..a0b1429132a 100644
> --- a/source3/modules/vfs_aio_fork.c
> +++ b/source3/modules/vfs_aio_fork.c
> @@ -331,7 +331,7 @@ static void aio_child_loop(int sockfd, struct mmap_area *map)
> * common parent state
> */
> generate_random_buffer(&randval, sizeof(randval));
> - msecs = randval + 20;
> + msecs = (randval%20)+1;
> DEBUG(10, ("delaying for %u msecs\n", msecs));
> smb_msleep(msecs);
> }
> --
> 2.11.0
>
>
> From 5a050435ab352fbc877db6c9df12cfd133537caf Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 4 Dec 2017 15:39:10 +0100
> Subject: [PATCH 7/7] smbd: Enable async I/O by default
>
> We've had this code in for long enough that we should enable it by default.
> Modern clients do overlapping I/O, we should utilize that if possible.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> docs-xml/smbdotconf/tuning/aioreadsize.xml | 4 ++--
> docs-xml/smbdotconf/tuning/aiowritesize.xml | 4 ++--
> lib/param/loadparm.c | 2 ++
> source3/param/loadparm.c | 4 ++--
> 4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/docs-xml/smbdotconf/tuning/aioreadsize.xml b/docs-xml/smbdotconf/tuning/aioreadsize.xml
> index c6028b8f9ac..4785d2abad9 100644
> --- a/docs-xml/smbdotconf/tuning/aioreadsize.xml
> +++ b/docs-xml/smbdotconf/tuning/aioreadsize.xml
> @@ -13,7 +13,7 @@
> <related>aio write size</related>
> </description>
>
> -<value type="default">0</value>
> -<value type="example">1<comment>Always do reads asynchronously
> +<value type="default">1</value>
> +<value type="example">0<comment>Always do reads synchronously
> </comment></value>
> </samba:parameter>
> diff --git a/docs-xml/smbdotconf/tuning/aiowritesize.xml b/docs-xml/smbdotconf/tuning/aiowritesize.xml
> index 8f42284111e..1d649fe7c2c 100644
> --- a/docs-xml/smbdotconf/tuning/aiowritesize.xml
> +++ b/docs-xml/smbdotconf/tuning/aiowritesize.xml
> @@ -18,7 +18,7 @@
> <related>aio read size</related>
> </description>
>
> -<value type="default">0</value>
> -<value type="example">1<comment>Always do writes asynchronously
> +<value type="default">1</value>
> +<value type="example">0<comment>Always do writes synchronously
> </comment></value>
> </samba:parameter>
> diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
> index 73b7901d7f6..508fa5a692d 100644
> --- a/lib/param/loadparm.c
> +++ b/lib/param/loadparm.c
> @@ -2590,6 +2590,8 @@ struct loadparm_context *loadparm_init(TALLOC_CTX *mem_ctx)
> lp_ctx->sDefault->force_create_mode = 0000;
> lp_ctx->sDefault->directory_mask = 0755;
> lp_ctx->sDefault->force_directory_mode = 0000;
> + lp_ctx->sDefault->aio_read_size = 1;
> + lp_ctx->sDefault->aio_write_size = 1;
>
> DEBUG(3, ("Initialising global parameters\n"));
>
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index 01c022e2889..a34b3dbd1ad 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -238,8 +238,8 @@ static const struct loadparm_service _sDefault =
> .acl_group_control = false,
> .acl_allow_execute_always = false,
> .allocation_roundup_size = SMB_ROUNDUP_ALLOCATION_SIZE,
> - .aio_read_size = 0,
> - .aio_write_size = 0,
> + .aio_read_size = 1,
> + .aio_write_size = 1,
> .map_readonly = MAP_READONLY_YES,
> .directory_name_cache_size = 100,
> .smb_encrypt = SMB_SIGNING_DEFAULT,
> --
> 2.11.0
>
More information about the samba-technical
mailing list