Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1483098 > unrolled thread

[PATCH 00/11] splice cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/11] splice cleanups Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 10:40 +0200
    [PATCH 09/11] splice: use get_page_for_read() Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 10:40 +0200
    [PATCH 10/11] splice: don't check i_size in generic_file_splice_read() Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 10:40 +0200
    [PATCH 11/11] splice: fold __generic_file_splice_read() into caller Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 10:40 +0200
    [PATCH 07/11] pipe: remove generic_pipe_buf_confirm() Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 10:40 +0200
      Re: [PATCH 07/11] pipe: remove generic_pipe_buf_confirm() Christoph Hellwig <hch@infradead.org> - 2016-09-16 13:30 +0200
    [PATCH 01/11] pipe: add pipe_buf_get() helper Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 10:50 +0200
    [PATCH 02/11] pipe: add pipe_buf_release() helper Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 10:50 +0200
    [PATCH 04/11] pipe: add pipe_buf_steal() helper Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 10:50 +0200
    Re: [PATCH 00/11] splice cleanups Cedric Blancher <cedric.blancher@gmail.com> - 2016-09-14 11:00 +0200
      Re: [PATCH 00/11] splice cleanups Miklos Szeredi <mszeredi@redhat.com> - 2016-09-14 11:40 +0200
    Re: [PATCH 00/11] splice cleanups Christoph Hellwig <hch@infradead.org> - 2016-09-16 13:30 +0200

#1483098 — [PATCH 00/11] splice cleanups

This contains assorted cleanups in the splice area:

 - add helpers for pipe buf ops instead of directly calling them

 - page cache buf doesn't seem to need confirming (since ages)

 - generic_file_splice_read() and generic_file_read() have lots of
   duplication

Git tree is here:

 git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git#splice

Thanks,
Miklos

---
Miklos Szeredi (11):
  pipe: add pipe_buf_get() helper
  pipe: add pipe_buf_release() helper
  pipe: add pipe_buf_confirm() helper
  pipe: add pipe_buf_steal() helper
  pipe: fix comment in pipe_buf_operations
  pipe: no need to confirm page cache buf
  pipe: remove generic_pipe_buf_confirm()
  filemap: add get_page_for_read() helper
  splice: use get_page_for_read()
  splice: don't check i_size in generic_file_splice_read()
  splice: fold __generic_file_splice_read() into caller

 drivers/char/virtio_console.c |   2 +-
 fs/fuse/dev.c                 |  15 +-
 fs/pipe.c                     |  31 +---
 fs/splice.c                   | 301 ++++++-------------------------------
 include/linux/pagemap.h       |   3 +
 include/linux/pipe_fs_i.h     |  73 ++++++---
 kernel/relay.c                |   1 -
 kernel/trace/trace.c          |   2 -
 mm/filemap.c                  | 339 ++++++++++++++++++++++--------------------
 9 files changed, 281 insertions(+), 486 deletions(-)

-- 
2.5.5

[toc] | [next] | [standalone]

#1483099 — [PATCH 09/11] splice: use get_page_for_read()

What __generic_file_splice_read() does is get a series of uptodate pages
and put them into the pipe buffer.

The get_page_for_read() helper can now be used to get the pages,
simplifying the code and making sure the splice(2) stays in sync with
read(2).

For example get_page_for_read() can handle partially uptodate pages and now
splice can take advantage of these as well.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/splice.c | 169 +++++-------------------------------------------------------
 1 file changed, 12 insertions(+), 157 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index dc4648ba6e8d..e7757b363b6c 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -265,14 +265,12 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 			   struct pipe_inode_info *pipe, size_t len,
 			   unsigned int flags)
 {
-	struct address_space *mapping = in->f_mapping;
 	unsigned int loff, nr_pages, req_pages;
 	struct page *pages[PIPE_DEF_BUFFERS];
 	struct partial_page partial[PIPE_DEF_BUFFERS];
 	struct page *page;
-	pgoff_t index, end_index;
-	loff_t isize;
-	int error, page_nr;
+	pgoff_t index;
+	int error;
 	struct splice_pipe_desc spd = {
 		.pages = pages,
 		.partial = partial,
@@ -290,168 +288,25 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 	req_pages = (len + loff + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	nr_pages = min(req_pages, spd.nr_pages_max);
 
-	/*
-	 * Lookup the (hopefully) full range of pages we need.
-	 */
-	spd.nr_pages = find_get_pages_contig(mapping, index, nr_pages, spd.pages);
-	index += spd.nr_pages;
-
-	/*
-	 * If find_get_pages_contig() returned fewer pages than we needed,
-	 * readahead/allocate the rest and fill in the holes.
-	 */
-	if (spd.nr_pages < nr_pages)
-		page_cache_sync_readahead(mapping, &in->f_ra, in,
-				index, req_pages - spd.nr_pages);
-
 	error = 0;
-	while (spd.nr_pages < nr_pages) {
-		/*
-		 * Page could be there, find_get_pages_contig() breaks on
-		 * the first hole.
-		 */
-		page = find_get_page(mapping, index);
-		if (!page) {
-			/*
-			 * page didn't exist, allocate one.
-			 */
-			page = page_cache_alloc_cold(mapping);
-			if (!page)
-				break;
+	while (spd.nr_pages < nr_pages && len) {
+		long ret;
 
-			error = add_to_page_cache_lru(page, mapping, index,
-				   mapping_gfp_constraint(mapping, GFP_KERNEL));
-			if (unlikely(error)) {
-				put_page(page);
-				if (error == -EEXIST)
-					continue;
-				break;
-			}
-			/*
-			 * add_to_page_cache() locks the page, unlock it
-			 * to avoid convoluting the logic below even more.
-			 */
-			unlock_page(page);
-		}
-
-		spd.pages[spd.nr_pages++] = page;
-		index++;
-	}
-
-	/*
-	 * Now loop over the map and see if we need to start IO on any
-	 * pages, fill in the partial map, etc.
-	 */
-	index = *ppos >> PAGE_SHIFT;
-	nr_pages = spd.nr_pages;
-	spd.nr_pages = 0;
-	for (page_nr = 0; page_nr < nr_pages; page_nr++) {
-		unsigned int this_len;
-
-		if (!len)
+		ret = get_page_for_read(in, loff, len, index, &page);
+		if (ret <= 0) {
+			error = ret;
 			break;
-
-		/*
-		 * this_len is the max we'll use from this page
-		 */
-		this_len = min_t(unsigned long, len, PAGE_SIZE - loff);
-		page = spd.pages[page_nr];
-
-		if (PageReadahead(page))
-			page_cache_async_readahead(mapping, &in->f_ra, in,
-					page, index, req_pages - page_nr);
-
-		/*
-		 * If the page isn't uptodate, we may need to start io on it
-		 */
-		if (!PageUptodate(page)) {
-			lock_page(page);
-
-			/*
-			 * Page was truncated, or invalidated by the
-			 * filesystem.  Redo the find/create, but this time the
-			 * page is kept locked, so there's no chance of another
-			 * race with truncate/invalidate.
-			 */
-			if (!page->mapping) {
-				unlock_page(page);
-retry_lookup:
-				page = find_or_create_page(mapping, index,
-						mapping_gfp_mask(mapping));
-
-				if (!page) {
-					error = -ENOMEM;
-					break;
-				}
-				put_page(spd.pages[page_nr]);
-				spd.pages[page_nr] = page;
-			}
-			/*
-			 * page was already under io and is now done, great
-			 */
-			if (PageUptodate(page)) {
-				unlock_page(page);
-				goto fill_it;
-			}
-
-			/*
-			 * need to read in the page
-			 */
-			error = mapping->a_ops->readpage(in, page);
-			if (unlikely(error)) {
-				/*
-				 * Re-lookup the page
-				 */
-				if (error == AOP_TRUNCATED_PAGE)
-					goto retry_lookup;
-
-				break;
-			}
-		}
-fill_it:
-		/*
-		 * i_size must be checked after PageUptodate.
-		 */
-		isize = i_size_read(mapping->host);
-		end_index = (isize - 1) >> PAGE_SHIFT;
-		if (unlikely(!isize || index > end_index))
-			break;
-
-		/*
-		 * if this is the last page, see if we need to shrink
-		 * the length and stop
-		 */
-		if (end_index == index) {
-			unsigned int plen;
-
-			/*
-			 * max good bytes in this page
-			 */
-			plen = ((isize - 1) & ~PAGE_MASK) + 1;
-			if (plen <= loff)
-				break;
-
-			/*
-			 * force quit after adding this page
-			 */
-			this_len = min(this_len, plen - loff);
-			len = this_len;
 		}
 
-		spd.partial[page_nr].offset = loff;
-		spd.partial[page_nr].len = this_len;
-		len -= this_len;
-		loff = 0;
+		spd.pages[spd.nr_pages] = page;
+		spd.partial[spd.nr_pages].offset = loff;
+		spd.partial[spd.nr_pages].len = ret;
 		spd.nr_pages++;
 		index++;
+		len -= ret;
+		loff = 0;
 	}
 
-	/*
-	 * Release any pages at the end, if we quit early. 'page_nr' is how far
-	 * we got, 'nr_pages' is how many pages are in the map.
-	 */
-	while (page_nr < nr_pages)
-		put_page(spd.pages[page_nr++]);
 	in->f_ra.prev_pos = (loff_t)index << PAGE_SHIFT;
 
 	if (spd.nr_pages)
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1483100 — [PATCH 10/11] splice: don't check i_size in generic_file_splice_read()

This is handled in get_page_for_read() already.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/splice.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index e7757b363b6c..bee282803ccf 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -334,20 +334,11 @@ ssize_t generic_file_splice_read(struct file *in, loff_t *ppos,
 				 struct pipe_inode_info *pipe, size_t len,
 				 unsigned int flags)
 {
-	loff_t isize, left;
 	int ret;
 
 	if (IS_DAX(in->f_mapping->host))
 		return default_file_splice_read(in, ppos, pipe, len, flags);
 
-	isize = i_size_read(in->f_mapping->host);
-	if (unlikely(*ppos >= isize))
-		return 0;
-
-	left = isize - *ppos;
-	if (unlikely(left < len))
-		len = left;
-
 	ret = __generic_file_splice_read(in, ppos, pipe, len, flags);
 	if (ret > 0) {
 		*ppos += ret;
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1483101 — [PATCH 11/11] splice: fold __generic_file_splice_read() into caller

generic_file_splice_read() does so little that it makes no sense to keep
__generic_file_splice_read() a separate function.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/splice.c | 70 +++++++++++++++++++++++++------------------------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index bee282803ccf..395cbb6b4926 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -260,17 +260,30 @@ void splice_shrink_spd(struct splice_pipe_desc *spd)
 	kfree(spd->partial);
 }
 
-static int
-__generic_file_splice_read(struct file *in, loff_t *ppos,
-			   struct pipe_inode_info *pipe, size_t len,
-			   unsigned int flags)
+/**
+ * generic_file_splice_read - splice data from file to a pipe
+ * @in:		file to splice from
+ * @ppos:	position in @in
+ * @pipe:	pipe to splice to
+ * @len:	number of bytes to splice
+ * @flags:	splice modifier flags
+ *
+ * Description:
+ *    Will read pages from given file and fill them into a pipe. Can be
+ *    used as long as the address_space operations for the source implements
+ *    a readpage() hook.
+ *
+ */
+ssize_t generic_file_splice_read(struct file *in, loff_t *ppos,
+				 struct pipe_inode_info *pipe, size_t len,
+				 unsigned int flags)
 {
 	unsigned int loff, nr_pages, req_pages;
 	struct page *pages[PIPE_DEF_BUFFERS];
 	struct partial_page partial[PIPE_DEF_BUFFERS];
 	struct page *page;
 	pgoff_t index;
-	int error;
+	int ret = 0;
 	struct splice_pipe_desc spd = {
 		.pages = pages,
 		.partial = partial,
@@ -280,6 +293,9 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 		.spd_release = spd_release_page,
 	};
 
+	if (IS_DAX(in->f_mapping->host))
+		return default_file_splice_read(in, ppos, pipe, len, flags);
+
 	if (splice_grow_spd(pipe, &spd))
 		return -ENOMEM;
 
@@ -288,62 +304,34 @@ __generic_file_splice_read(struct file *in, loff_t *ppos,
 	req_pages = (len + loff + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	nr_pages = min(req_pages, spd.nr_pages_max);
 
-	error = 0;
 	while (spd.nr_pages < nr_pages && len) {
-		long ret;
+		int nr;
 
-		ret = get_page_for_read(in, loff, len, index, &page);
-		if (ret <= 0) {
-			error = ret;
+		nr = get_page_for_read(in, loff, len, index, &page);
+		if (nr <= 0) {
+			ret = nr;
 			break;
 		}
 
 		spd.pages[spd.nr_pages] = page;
 		spd.partial[spd.nr_pages].offset = loff;
-		spd.partial[spd.nr_pages].len = ret;
+		spd.partial[spd.nr_pages].len = nr;
 		spd.nr_pages++;
 		index++;
-		len -= ret;
+		len -= nr;
 		loff = 0;
 	}
 
 	in->f_ra.prev_pos = (loff_t)index << PAGE_SHIFT;
 
 	if (spd.nr_pages)
-		error = splice_to_pipe(pipe, &spd);
-
-	splice_shrink_spd(&spd);
-	return error;
-}
-
-/**
- * generic_file_splice_read - splice data from file to a pipe
- * @in:		file to splice from
- * @ppos:	position in @in
- * @pipe:	pipe to splice to
- * @len:	number of bytes to splice
- * @flags:	splice modifier flags
- *
- * Description:
- *    Will read pages from given file and fill them into a pipe. Can be
- *    used as long as the address_space operations for the source implements
- *    a readpage() hook.
- *
- */
-ssize_t generic_file_splice_read(struct file *in, loff_t *ppos,
-				 struct pipe_inode_info *pipe, size_t len,
-				 unsigned int flags)
-{
-	int ret;
-
-	if (IS_DAX(in->f_mapping->host))
-		return default_file_splice_read(in, ppos, pipe, len, flags);
+		ret = splice_to_pipe(pipe, &spd);
 
-	ret = __generic_file_splice_read(in, ppos, pipe, len, flags);
 	if (ret > 0) {
 		*ppos += ret;
 		file_accessed(in);
 	}
+	splice_shrink_spd(&spd);
 
 	return ret;
 }
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1483102 — [PATCH 07/11] pipe: remove generic_pipe_buf_confirm()

Now all instances of .confirm are set to generic_pipe_buf_confirm().  The
method and the helper can be removed.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/pipe.c                 | 18 ------------------
 fs/splice.c               |  4 ----
 include/linux/pipe_fs_i.h | 18 +-----------------
 kernel/relay.c            |  1 -
 kernel/trace/trace.c      |  2 --
 5 files changed, 1 insertion(+), 42 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 4fc422f0dea8..47b41568b3fc 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -200,22 +200,6 @@ void generic_pipe_buf_get(struct pipe_inode_info *pipe, struct pipe_buffer *buf)
 EXPORT_SYMBOL(generic_pipe_buf_get);
 
 /**
- * generic_pipe_buf_confirm - verify contents of the pipe buffer
- * @info:	the pipe that the buffer belongs to
- * @buf:	the buffer to confirm
- *
- * Description:
- *	This function does nothing, because the generic pipe code uses
- *	pages that are always good when inserted into the pipe.
- */
-int generic_pipe_buf_confirm(struct pipe_inode_info *info,
-			     struct pipe_buffer *buf)
-{
-	return 0;
-}
-EXPORT_SYMBOL(generic_pipe_buf_confirm);
-
-/**
  * generic_pipe_buf_release - put a reference to a &struct pipe_buffer
  * @pipe:	the pipe that the buffer belongs to
  * @buf:	the buffer to put a reference to
@@ -232,7 +216,6 @@ EXPORT_SYMBOL(generic_pipe_buf_release);
 
 static const struct pipe_buf_operations anon_pipe_buf_ops = {
 	.can_merge = 1,
-	.confirm = generic_pipe_buf_confirm,
 	.release = anon_pipe_buf_release,
 	.steal = anon_pipe_buf_steal,
 	.get = generic_pipe_buf_get,
@@ -240,7 +223,6 @@ static const struct pipe_buf_operations anon_pipe_buf_ops = {
 
 static const struct pipe_buf_operations packet_pipe_buf_ops = {
 	.can_merge = 0,
-	.confirm = generic_pipe_buf_confirm,
 	.release = anon_pipe_buf_release,
 	.steal = anon_pipe_buf_steal,
 	.get = generic_pipe_buf_get,
diff --git a/fs/splice.c b/fs/splice.c
index 0ecbe3011796..dc4648ba6e8d 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -95,7 +95,6 @@ static void page_cache_pipe_buf_release(struct pipe_inode_info *pipe,
 
 const struct pipe_buf_operations page_cache_pipe_buf_ops = {
 	.can_merge = 0,
-	.confirm = generic_pipe_buf_confirm,
 	.release = page_cache_pipe_buf_release,
 	.steal = page_cache_pipe_buf_steal,
 	.get = generic_pipe_buf_get,
@@ -113,7 +112,6 @@ static int user_page_pipe_buf_steal(struct pipe_inode_info *pipe,
 
 static const struct pipe_buf_operations user_page_pipe_buf_ops = {
 	.can_merge = 0,
-	.confirm = generic_pipe_buf_confirm,
 	.release = page_cache_pipe_buf_release,
 	.steal = user_page_pipe_buf_steal,
 	.get = generic_pipe_buf_get,
@@ -507,7 +505,6 @@ EXPORT_SYMBOL(generic_file_splice_read);
 
 static const struct pipe_buf_operations default_pipe_buf_ops = {
 	.can_merge = 0,
-	.confirm = generic_pipe_buf_confirm,
 	.release = generic_pipe_buf_release,
 	.steal = generic_pipe_buf_steal,
 	.get = generic_pipe_buf_get,
@@ -522,7 +519,6 @@ static int generic_pipe_buf_nosteal(struct pipe_inode_info *pipe,
 /* Pipe buffer operations for a socket and similar. */
 const struct pipe_buf_operations nosteal_pipe_buf_ops = {
 	.can_merge = 0,
-	.confirm = generic_pipe_buf_confirm,
 	.release = generic_pipe_buf_release,
 	.steal = generic_pipe_buf_nosteal,
 	.get = generic_pipe_buf_get,
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index e7497c9dde7f..ae9d87ea95cb 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -62,12 +62,6 @@ struct pipe_inode_info {
 };
 
 /*
- * Note on the nesting of these functions:
- *
- * ->confirm()
- *	->steal()
- *
- * That is, ->steal() must be called on a confirmed buffer.
  * See below for the meaning of each operation. Also see kerneldoc
  * in fs/pipe.c for the pipe and generic variants of these hooks.
  */
@@ -80,15 +74,6 @@ struct pipe_buf_operations {
 	int can_merge;
 
 	/*
-	 * ->confirm() verifies that the data in the pipe buffer is there
-	 * and that the contents are good. If the pages in the pipe belong
-	 * to a file system, we may need to wait for IO completion in this
-	 * hook. Returns 0 for good, or a negative error value in case of
-	 * error.
-	 */
-	int (*confirm)(struct pipe_inode_info *, struct pipe_buffer *);
-
-	/*
 	 * When the contents of this pipe buffer has been completely
 	 * consumed by a reader, ->release() is called.
 	 */
@@ -143,7 +128,7 @@ static inline void pipe_buf_release(struct pipe_inode_info *pipe,
 static inline int pipe_buf_confirm(struct pipe_inode_info *pipe,
 				   struct pipe_buffer *buf)
 {
-	return buf->ops->confirm(pipe, buf);
+	return 0;
 }
 
 /**
@@ -179,7 +164,6 @@ void free_pipe_info(struct pipe_inode_info *);
 
 /* Generic pipe buffer ops functions */
 void generic_pipe_buf_get(struct pipe_inode_info *, struct pipe_buffer *);
-int generic_pipe_buf_confirm(struct pipe_inode_info *, struct pipe_buffer *);
 int generic_pipe_buf_steal(struct pipe_inode_info *, struct pipe_buffer *);
 void generic_pipe_buf_release(struct pipe_inode_info *, struct pipe_buffer *);
 
diff --git a/kernel/relay.c b/kernel/relay.c
index d797502140b9..da954ce520b2 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -1223,7 +1223,6 @@ static void relay_pipe_buf_release(struct pipe_inode_info *pipe,
 
 static const struct pipe_buf_operations relay_pipe_buf_ops = {
 	.can_merge = 0,
-	.confirm = generic_pipe_buf_confirm,
 	.release = relay_pipe_buf_release,
 	.steal = generic_pipe_buf_steal,
 	.get = generic_pipe_buf_get,
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index dade4c9559cc..bdf891c78708 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5219,7 +5219,6 @@ static void tracing_spd_release_pipe(struct splice_pipe_desc *spd,
 
 static const struct pipe_buf_operations tracing_pipe_buf_ops = {
 	.can_merge		= 0,
-	.confirm		= generic_pipe_buf_confirm,
 	.release		= generic_pipe_buf_release,
 	.steal			= generic_pipe_buf_steal,
 	.get			= generic_pipe_buf_get,
@@ -6114,7 +6113,6 @@ static void buffer_pipe_buf_get(struct pipe_inode_info *pipe,
 /* Pipe buffer operations for a buffer. */
 static const struct pipe_buf_operations buffer_pipe_buf_ops = {
 	.can_merge		= 0,
-	.confirm		= generic_pipe_buf_confirm,
 	.release		= buffer_pipe_buf_release,
 	.steal			= generic_pipe_buf_steal,
 	.get			= buffer_pipe_buf_get,
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1484906 — Re: [PATCH 07/11] pipe: remove generic_pipe_buf_confirm()

>  static inline int pipe_buf_confirm(struct pipe_inode_info *pipe,
>  				   struct pipe_buffer *buf)
>  {
> -	return buf->ops->confirm(pipe, buf);
> +	return 0;
>  }

Why do you keep pipe_buf_confirm around?

[toc] | [prev] | [next] | [standalone]

#1483105 — [PATCH 01/11] pipe: add pipe_buf_get() helper

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dev.c             |  2 +-
 fs/splice.c               |  4 ++--
 include/linux/pipe_fs_i.h | 11 +++++++++++
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index a94d2ed81ab4..5cb20600bc9e 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1993,7 +1993,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 			pipe->curbuf = (pipe->curbuf + 1) & (pipe->buffers - 1);
 			pipe->nrbufs--;
 		} else {
-			ibuf->ops->get(pipe, ibuf);
+			pipe_buf_get(pipe, ibuf);
 			*obuf = *ibuf;
 			obuf->flags &= ~PIPE_BUF_FLAG_GIFT;
 			obuf->len = rem;
diff --git a/fs/splice.c b/fs/splice.c
index dd9bf7e410d2..36457a192c13 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1876,7 +1876,7 @@ retry:
 			 * Get a reference to this pipe buffer,
 			 * so we can copy the contents over.
 			 */
-			ibuf->ops->get(ipipe, ibuf);
+			pipe_buf_get(ipipe, ibuf);
 			*obuf = *ibuf;
 
 			/*
@@ -1948,7 +1948,7 @@ static int link_pipe(struct pipe_inode_info *ipipe,
 		 * Get a reference to this pipe buffer,
 		 * so we can copy the contents over.
 		 */
-		ibuf->ops->get(ipipe, ibuf);
+		pipe_buf_get(ipipe, ibuf);
 
 		obuf = opipe->bufs + nbuf;
 		*obuf = *ibuf;
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 24f5470d3944..10876f3cb3da 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -115,6 +115,17 @@ struct pipe_buf_operations {
 	void (*get)(struct pipe_inode_info *, struct pipe_buffer *);
 };
 
+/**
+ * pipe_buf_get - get a reference to a pipe_buffer
+ * @pipe:	the pipe that the buffer belongs to
+ * @buf:	the buffer to get a reference to
+ */
+static inline void pipe_buf_get(struct pipe_inode_info *pipe,
+				struct pipe_buffer *buf)
+{
+	buf->ops->get(pipe, buf);
+}
+
 /* Differs from PIPE_BUF in that PIPE_SIZE is the length of the actual
    memory allocation, whereas PIPE_BUF makes atomicity guarantees.  */
 #define PIPE_SIZE		PAGE_SIZE
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1483106 — [PATCH 02/11] pipe: add pipe_buf_release() helper

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dev.c             |  7 +++----
 fs/pipe.c                 |  5 ++---
 fs/splice.c               | 14 ++++----------
 include/linux/pipe_fs_i.h | 14 ++++++++++++++
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 5cb20600bc9e..13be2fddcace 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2015,10 +2015,9 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 
 	ret = fuse_dev_do_write(fud, &cs, len);
 
-	for (idx = 0; idx < nbuf; idx++) {
-		struct pipe_buffer *buf = &bufs[idx];
-		buf->ops->release(pipe, buf);
-	}
+	for (idx = 0; idx < nbuf; idx++)
+		pipe_buf_release(pipe, &bufs[idx]);
+
 out:
 	kfree(bufs);
 	return ret;
diff --git a/fs/pipe.c b/fs/pipe.c
index 4ebe6b2e5217..67b5f1923835 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -299,8 +299,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
 			}
 
 			if (!buf->len) {
-				buf->ops = NULL;
-				ops->release(pipe, buf);
+				pipe_buf_release(pipe, buf);
 				curbuf = (curbuf + 1) & (pipe->buffers - 1);
 				pipe->curbuf = curbuf;
 				pipe->nrbufs = --bufs;
@@ -664,7 +663,7 @@ void free_pipe_info(struct pipe_inode_info *pipe)
 	for (i = 0; i < pipe->buffers; i++) {
 		struct pipe_buffer *buf = pipe->bufs + i;
 		if (buf->ops)
-			buf->ops->release(pipe, buf);
+			pipe_buf_release(pipe, buf);
 	}
 	if (pipe->tmp_page)
 		__free_page(pipe->tmp_page);
diff --git a/fs/splice.c b/fs/splice.c
index 36457a192c13..e4dc45926494 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -757,7 +757,6 @@ static int splice_from_pipe_feed(struct pipe_inode_info *pipe, struct splice_des
 
 	while (pipe->nrbufs) {
 		struct pipe_buffer *buf = pipe->bufs + pipe->curbuf;
-		const struct pipe_buf_operations *ops = buf->ops;
 
 		sd->len = buf->len;
 		if (sd->len > sd->total_len)
@@ -783,8 +782,7 @@ static int splice_from_pipe_feed(struct pipe_inode_info *pipe, struct splice_des
 		sd->total_len -= ret;
 
 		if (!buf->len) {
-			buf->ops = NULL;
-			ops->release(pipe, buf);
+			pipe_buf_release(pipe, buf);
 			pipe->curbuf = (pipe->curbuf + 1) & (pipe->buffers - 1);
 			pipe->nrbufs--;
 			if (pipe->files)
@@ -1030,11 +1028,9 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
 		while (ret) {
 			struct pipe_buffer *buf = pipe->bufs + pipe->curbuf;
 			if (ret >= buf->len) {
-				const struct pipe_buf_operations *ops = buf->ops;
 				ret -= buf->len;
 				buf->len = 0;
-				buf->ops = NULL;
-				ops->release(pipe, buf);
+				pipe_buf_release(pipe, buf);
 				pipe->curbuf = (pipe->curbuf + 1) & (pipe->buffers - 1);
 				pipe->nrbufs--;
 				if (pipe->files)
@@ -1273,10 +1269,8 @@ out_release:
 	for (i = 0; i < pipe->buffers; i++) {
 		struct pipe_buffer *buf = pipe->bufs + i;
 
-		if (buf->ops) {
-			buf->ops->release(pipe, buf);
-			buf->ops = NULL;
-		}
+		if (buf->ops)
+			pipe_buf_release(pipe, buf);
 	}
 
 	if (!bytes)
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 10876f3cb3da..d24fa6da6ae3 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -126,6 +126,20 @@ static inline void pipe_buf_get(struct pipe_inode_info *pipe,
 	buf->ops->get(pipe, buf);
 }
 
+/**
+ * pipe_buf_release - put a reference to a pipe_buffer
+ * @pipe:	the pipe that the buffer belongs to
+ * @buf:	the buffer to put a reference to
+ */
+static inline void pipe_buf_release(struct pipe_inode_info *pipe,
+				    struct pipe_buffer *buf)
+{
+	const struct pipe_buf_operations *ops = buf->ops;
+
+	buf->ops = NULL;
+	ops->release(pipe, buf);
+}
+
 /* Differs from PIPE_BUF in that PIPE_SIZE is the length of the actual
    memory allocation, whereas PIPE_BUF makes atomicity guarantees.  */
 #define PIPE_SIZE		PAGE_SIZE
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1483107 — [PATCH 04/11] pipe: add pipe_buf_steal() helper

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 drivers/char/virtio_console.c |  2 +-
 fs/fuse/dev.c                 |  2 +-
 include/linux/pipe_fs_i.h     | 11 +++++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 5da47e26a012..8114744bf30c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -889,7 +889,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
 		return 0;
 
 	/* Try lock this page */
-	if (buf->ops->steal(pipe, buf) == 0) {
+	if (pipe_buf_steal(pipe, buf) == 0) {
 		/* Get reference and unlock page for moving */
 		get_page(buf->page);
 		unlock_page(buf->page);
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 9aa757f283e5..6830b3ca1bbb 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -841,7 +841,7 @@ static int fuse_try_move_page(struct fuse_copy_state *cs, struct page **pagep)
 	if (cs->len != PAGE_SIZE)
 		goto out_fallback;
 
-	if (buf->ops->steal(cs->pipe, buf) != 0)
+	if (pipe_buf_steal(cs->pipe, buf) != 0)
 		goto out_fallback;
 
 	newpage = buf->page;
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 654413334537..bddccf0159bb 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -151,6 +151,17 @@ static inline int pipe_buf_confirm(struct pipe_inode_info *pipe,
 	return buf->ops->confirm(pipe, buf);
 }
 
+/**
+ * pipe_buf_steal - attempt to take ownership of a pipe_buffer
+ * @pipe:	the pipe that the buffer belongs to
+ * @buf:	the buffer to attempt to steal
+ */
+static inline int pipe_buf_steal(struct pipe_inode_info *pipe,
+				 struct pipe_buffer *buf)
+{
+	return buf->ops->steal(pipe, buf);
+}
+
 /* Differs from PIPE_BUF in that PIPE_SIZE is the length of the actual
    memory allocation, whereas PIPE_BUF makes atomicity guarantees.  */
 #define PIPE_SIZE		PAGE_SIZE
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1483112

Is there any shell which uses pipe splicing?

Ced

On 14 September 2016 at 10:37, Miklos Szeredi <mszeredi@redhat.com> wrote:
> This contains assorted cleanups in the splice area:
>
>  - add helpers for pipe buf ops instead of directly calling them
>
>  - page cache buf doesn't seem to need confirming (since ages)
>
>  - generic_file_splice_read() and generic_file_read() have lots of
>    duplication
>
> Git tree is here:
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git#splice
>
> Thanks,
> Miklos
>
> ---
> Miklos Szeredi (11):
>   pipe: add pipe_buf_get() helper
>   pipe: add pipe_buf_release() helper
>   pipe: add pipe_buf_confirm() helper
>   pipe: add pipe_buf_steal() helper
>   pipe: fix comment in pipe_buf_operations
>   pipe: no need to confirm page cache buf
>   pipe: remove generic_pipe_buf_confirm()
>   filemap: add get_page_for_read() helper
>   splice: use get_page_for_read()
>   splice: don't check i_size in generic_file_splice_read()
>   splice: fold __generic_file_splice_read() into caller
>
>  drivers/char/virtio_console.c |   2 +-
>  fs/fuse/dev.c                 |  15 +-
>  fs/pipe.c                     |  31 +---
>  fs/splice.c                   | 301 ++++++-------------------------------
>  include/linux/pagemap.h       |   3 +
>  include/linux/pipe_fs_i.h     |  73 ++++++---
>  kernel/relay.c                |   1 -
>  kernel/trace/trace.c          |   2 -
>  mm/filemap.c                  | 339 ++++++++++++++++++++++--------------------
>  9 files changed, 281 insertions(+), 486 deletions(-)
>
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Cedric Blancher <cedric.blancher@gmail.com>
[https://plus.google.com/u/0/+CedricBlancher/]
Institute Pasteur

[toc] | [prev] | [next] | [standalone]

#1483129

[Multipart message — attachments visible in raw view] — view raw

On Wed, Sep 14, 2016 at 10:55 AM, Cedric Blancher
<cedric.blancher@gmail.com> wrote:
> Is there any shell which uses pipe splicing?

for i in /usr/bin/*; do if file $i | grep -q ELF; then if nm -D $i |
grep -q splice; then echo $i; fi; fi; done

For me it does not yield anything by which you could easily try out splicing.

Attaching a simple cp-like program which uses splice if you want to try it out.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1484901

On Wed, Sep 14, 2016 at 10:37:05AM +0200, Miklos Szeredi wrote:
> This contains assorted cleanups in the splice area:
> 
>  - add helpers for pipe buf ops instead of directly calling them
> 
>  - page cache buf doesn't seem to need confirming (since ages)
> 
>  - generic_file_splice_read() and generic_file_read() have lots of
>    duplication

The whole series looks great to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web