Re: [PATCH 1/3 v2] chunkd: remove sendfile(2) zero-copy support

[Steven Dake <sdake@redhat.com>]

On 07/16/2010 10:46 PM, Jeff Garzik wrote:
>
>   chunkd/be-fs.c  |   60 --------------------------------------------------------
>   chunkd/chunkd.h |   14 -------------
>   chunkd/object.c |   31 ++++++++++++----------------
>   chunkd/server.c |   28 --------------------------
>   configure.ac    |    3 --
>   5 files changed, 15 insertions(+), 121 deletions(-)
> commit e87d06526fa2052a1cecbed65ec2fac263c5e7d8
> Author: Jeff Garzik<jeff@garzik.org>
> Date:   Sat Jul 17 00:26:41 2010 -0400
>
>      chunkd: remove sendfile(2) zero-copy support
>
>      chunkd will be soon checksumming data in main memory.  That removes
>      the utility of a zero-copy interface which bypasses the on-heap
>      data requirement.
>
>      Signed-off-by: Jeff Garzik<jgarzik@redhat.com>
>
> diff --git a/chunkd/be-fs.c b/chunkd/be-fs.c
> index 4b851a7..0a81134 100644
> --- a/chunkd/be-fs.c
> +++ b/chunkd/be-fs.c
> @@ -25,9 +25,6 @@
>   #include<sys/stat.h>
>   #include<sys/socket.h>
>   #include<sys/uio.h>
> -#if defined(HAVE_SYS_SENDFILE_H)
> -#include<sys/sendfile.h>
> -#endif
>   #include<stdlib.h>
>   #include<unistd.h>
>   #include<stdio.h>
> @@ -52,7 +49,6 @@ struct fs_obj {
>
>   	int			in_fd;
>   	char			*in_fn;
> -	off_t			sendfile_ofs;
>   };
>
>   struct be_fs_obj_hdr {
> @@ -547,62 +543,6 @@ ssize_t fs_obj_write(struct backend_obj *bo, const void *ptr, size_t len)
>   	return rc;
>   }
>
> -#if defined(HAVE_SENDFILE)&&  defined(__linux__)
> -
> -ssize_t fs_obj_sendfile(struct backend_obj *bo, int out_fd, size_t len)
> -{
> -	struct fs_obj *obj = bo->private;
> -	ssize_t rc;
> -
> -	if (obj->sendfile_ofs == 0) {
> -		obj->sendfile_ofs += sizeof(struct be_fs_obj_hdr);
> -		obj->sendfile_ofs += bo->key_len;
> -	}
> -
> -	rc = sendfile(out_fd, obj->in_fd,&obj->sendfile_ofs, len);
> -	if (rc<  0)
> -		applog(LOG_ERR, "obj sendfile(%s) failed: %s",
> -		       obj->in_fn, strerror(errno));
> -
> -	return rc;
> -}
> -
> -#elif defined(HAVE_SENDFILE)&&  defined(__FreeBSD__)
> -
> -ssize_t fs_obj_sendfile(struct backend_obj *bo, int out_fd, size_t len)
> -{
> -	struct fs_obj *obj = bo->private;
> -	ssize_t rc;
> -	off_t sbytes = 0;
> -
> -	if (obj->sendfile_ofs == 0) {
> -		obj->sendfile_ofs += sizeof(struct be_fs_obj_hdr);
> -		obj->sendfile_ofs += bo->key_len;
> -	}
> -
> -	rc = sendfile(obj->in_fd, out_fd, obj->sendfile_ofs, len,
> -		      NULL,&sbytes, 0);
> -	if (rc<  0) {
> -		applog(LOG_ERR, "obj sendfile(%s) failed: %s",
> -		       obj->in_fn, strerror(errno));
> -		return rc;
> -	}
> -
> -	obj->sendfile_ofs += sbytes;
> -
> -	return sbytes;
> -}
> -
> -#else
> -
> -ssize_t fs_obj_sendfile(struct backend_obj *bo, int out_fd, size_t len)
> -{
> -	applog(LOG_ERR, "BUG: sendfile used but not supported");
> -	return -EOPNOTSUPP;
> -}
> -
> -#endif /* HAVE_SENDFILE&&  HAVE_SYS_SENDFILE_H */
> -
>   bool fs_obj_write_commit(struct backend_obj *bo, const char *user,
>   			 unsigned char *md, bool sync_data)
>   {
> diff --git a/chunkd/chunkd.h b/chunkd/chunkd.h
> index 72833f7..716704b 100644
> --- a/chunkd/chunkd.h
> +++ b/chunkd/chunkd.h
> @@ -39,8 +39,6 @@ enum {
>   	CLI_DATA_BUF_SZ		= 16 * 1024,
>
>   	CHD_TRASH_MAX		= 1000,
> -
> -	CLI_MAX_SENDFILE_SZ	= 512 * 1024,
>   };
>
>   struct client;
> @@ -54,7 +52,6 @@ struct client_write {
>   	uint64_t		len;		/* write buffer length */
>   	cli_write_func		cb;		/* callback */
>   	void			*cb_data;	/* data passed to cb */
> -	bool			sendfile;	/* using sendfile? */
>
>   	struct list_head	node;
>   };
> @@ -268,7 +265,6 @@ extern bool fs_obj_delete(uint32_t table_id, const char *user,
>   		          const void *kbuf, size_t klen,
>   			  enum chunk_errcode *err_code);
>   extern int fs_obj_disable(const char *fn);
> -extern ssize_t fs_obj_sendfile(struct backend_obj *bo, int out_fd, size_t len);
>   extern int fs_list_objs_open(struct fs_obj_lister *t,
>   			     const char *root_path, uint32_t table_id);
>   extern int fs_list_objs_next(struct fs_obj_lister *t, char **fnp);
> @@ -323,7 +319,6 @@ extern void applog(int prio, const char *fmt, ...);
>   extern bool cli_err(struct client *cli, enum chunk_errcode code, bool recycle_ok);
>   extern int cli_writeq(struct client *cli, const void *buf, unsigned int buflen,
>   		     cli_write_func cb, void *cb_data);
> -extern bool cli_wr_sendfile(struct client *, cli_write_func);
>   extern bool cli_rd_set_poll(struct client *cli, bool readable);
>   extern void cli_wr_set_poll(struct client *cli, bool writable);
>   extern bool cli_cb_free(struct client *cli, struct client_write *wr,
> @@ -342,15 +337,6 @@ extern void read_config(void);
>   /* selfcheck.c */
>   extern int chk_spawn(TCHDB *hdb);
>
> -static inline bool use_sendfile(struct client *cli)
> -{
> -#if defined(HAVE_SENDFILE)&&  defined(HAVE_SYS_SENDFILE_H)
> -	return cli->ssl ? false : true;
> -#else
> -	return false;
> -#endif
> -}
> -
>   #ifndef HAVE_STRNLEN
>   extern size_t strnlen(const char *s, size_t maxlen);
>   #endif
> diff --git a/chunkd/object.c b/chunkd/object.c
> index d7d3cb6..a517558 100644
> --- a/chunkd/object.c
> +++ b/chunkd/object.c
> @@ -253,28 +253,23 @@ void cli_in_end(struct client *cli)
>
>   static bool object_read_bytes(struct client *cli)
>   {
> -	if (use_sendfile(cli)) {
> -		if (!cli_wr_sendfile(cli, object_get_more))
> -			return false;
> -	} else {
> -		ssize_t bytes;
> +	ssize_t bytes;
>
> -		bytes = fs_obj_read(cli->in_obj, cli->netbuf_out,
> -				    MIN(cli->in_len, CLI_DATA_BUF_SZ));
> -		if (bytes<  0)
> -			return false;
> -		if (bytes == 0&&  cli->in_len != 0)
> -			return false;
> +	bytes = fs_obj_read(cli->in_obj, cli->netbuf_out,
> +			    MIN(cli->in_len, CLI_DATA_BUF_SZ));
> +	if (bytes<  0)
> +		return false;
> +	if (bytes == 0&&  cli->in_len != 0)
> +		return false;
>
> -		cli->in_len -= bytes;
> +	cli->in_len -= bytes;
>
> -		if (!cli->in_len)
> -			cli_in_end(cli);
> +	if (!cli->in_len)
> +		cli_in_end(cli);
>
> -		if (cli_writeq(cli, cli->netbuf_out, bytes,
> -			       cli->in_len ? object_get_more : NULL, NULL))
> -			return false;
> -	}
> +	if (cli_writeq(cli, cli->netbuf_out, bytes,
> +		       cli->in_len ? object_get_more : NULL, NULL))
> +		return false;
>
>   	return true;
>   }
> diff --git a/chunkd/server.c b/chunkd/server.c
> index 3115592..833dcf8 100644
> --- a/chunkd/server.c
> +++ b/chunkd/server.c
> @@ -504,14 +504,7 @@ restart:
>
>   	/* execute non-blocking write */
>   do_write:
> -	if (tmp->sendfile) {
> -		rc = fs_obj_sendfile(cli->in_obj, cli->fd,
> -				     MIN(cli->in_len, CLI_MAX_SENDFILE_SZ));
> -		if (rc<  0)
> -			goto err_out;
> -
> -		cli->in_len -= rc;
> -	} else if (cli->ssl) {
> +	if (cli->ssl) {
>   		rc = SSL_write(cli->ssl, tmp->buf, tmp->len);
>   		if (rc<= 0) {
>   			rc = SSL_get_error(cli->ssl, rc);
> @@ -612,31 +605,12 @@ int cli_writeq(struct client *cli, const void *buf, unsigned int buflen,
>   	wr->len = buflen;
>   	wr->cb = cb;
>   	wr->cb_data = cb_data;
> -	wr->sendfile = false;
>
>   	list_add_tail(&wr->node,&cli->write_q);
>
>   	return 0;
>   }
>
> -bool cli_wr_sendfile(struct client *cli, cli_write_func cb)
> -{
> -	struct client_write *wr;
> -
> -	wr = calloc(1, sizeof(struct client_write));
> -	if (!wr)
> -		return false;
> -
> -	wr->len = cli->in_len;
> -	wr->cb = cb;
> -	wr->sendfile = true;
> -	INIT_LIST_HEAD(&wr->node);
> -
> -	list_add_tail(&wr->node,&cli->write_q);
> -
> -	return true;
> -}
> -
>   static int cli_read_data(struct client *cli, void *buf, size_t buflen)
>   {
>   	ssize_t rc;
> diff --git a/configure.ac b/configure.ac
> index 6b48f1e..db10242 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -65,7 +65,6 @@ AM_PROG_LIBTOOL
>   dnl Checks for header files.
>   AC_HEADER_STDC
>   dnl AC_CHECK_HEADERS(sys/ioctl.h unistd.h)
> -AC_CHECK_HEADERS(sys/sendfile.h sys/filio.h)
>   AC_CHECK_HEADER(db.h,[],exit 1)
>
>   dnl Checks for typedefs, structures, and compiler characteristics.
> @@ -97,7 +96,7 @@ PKG_CHECK_MODULES(TOKYOCABINET, tokyocabinet)
>   dnl -------------------------------------
>   dnl Checks for optional library functions
>   dnl -------------------------------------
> -AC_CHECK_FUNCS(strnlen daemon memmem memrchr sendfile)
> +AC_CHECK_FUNCS(strnlen daemon memmem memrchr)
>   AC_CHECK_FUNC(xdr_sizeof,
>   	[AC_DEFINE([HAVE_XDR_SIZEOF], [1],
>   		[Define to 1 if you have xdr_sizeof.])],
> --
> To unsubscribe from this list: send the line "unsubscribe hail-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff,

May be able to use vmsplice with sendfile (if linux is only target 
platform).  Haven't tried it myself, but the operations look interesting 
at achieving zero copy with sockets from memory addresses.

Regards
-steve