dio / splice fixups

dio / splice fixups

[Christoph Hellwig]

Hi all,

this series has a small fix and a bunch of cleanups on top of the
splice and direct I/O rework in the block tree.

 block/blk.h               |    2 --
 fs/splice.c               |   15 +++++++--------
 include/linux/bio.h       |    3 +--
 include/linux/blk_types.h |    1 -
 include/linux/uio.h       |    6 ------
 lib/iov_iter.c            |   35 +++++++----------------------------
 6 files changed, 15 insertions(+), 47 deletions(-)

[PATCH 1/4] splice: don't call file_accessed in copy_splice_read

[Christoph Hellwig]

copy_splice_read calls into ->read_iter to read the data, which already
calls file_accessed.

Fixes: 33b3b041543e ("splice: Add a func to do a splice from an O_DIRECT file without ITER_PIPE")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/splice.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/splice.c b/fs/splice.c
index 2420ead610a727..87c69fdb333dab 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -368,7 +368,6 @@ ssize_t copy_splice_read(struct file *in, loff_t *ppos,
 	if (ret > 0) {
 		keep = DIV_ROUND_UP(ret, PAGE_SIZE);
 		*ppos = kiocb.ki_pos;
-		file_accessed(in);
 	} else if (ret < 0) {
 		/*
 		 * callers of ->splice_read() expect -EAGAIN on
-- 
2.39.2

Re: [PATCH 1/4] splice: don't call file_accessed in copy_splice_read

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/4] splice: don't call file_accessed in copy_splice_read

[Christian Brauner]

On Wed, Jun 14, 2023 at 04:03:38PM +0200, Christoph Hellwig wrote:
> copy_splice_read calls into ->read_iter to read the data, which already
> calls file_accessed.
> 
> Fixes: 33b3b041543e ("splice: Add a func to do a splice from an O_DIRECT file without ITER_PIPE")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

[PATCH 2/4] splice: simplify a conditional in copy_splice_read

[Christoph Hellwig]

Check for -EFAULT instead of wrapping the check in an ret < 0 block.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/splice.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index 87c69fdb333dab..7a9565d8ec4f9d 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -368,15 +368,15 @@ ssize_t copy_splice_read(struct file *in, loff_t *ppos,
 	if (ret > 0) {
 		keep = DIV_ROUND_UP(ret, PAGE_SIZE);
 		*ppos = kiocb.ki_pos;
-	} else if (ret < 0) {
-		/*
-		 * callers of ->splice_read() expect -EAGAIN on
-		 * "can't put anything in there", rather than -EFAULT.
-		 */
-		if (ret == -EFAULT)
-			ret = -EAGAIN;
 	}
 
+	/*
+	 * Callers of ->splice_read() expect -EAGAIN on "can't put anything in
+	 * there", rather than -EFAULT.
+	 */
+	if (ret == -EFAULT)
+		ret = -EAGAIN;
+
 	/* Free any pages that didn't get touched at all. */
 	if (keep < npages)
 		release_pages(pages + keep, npages - keep);
-- 
2.39.2

Re: [PATCH 2/4] splice: simplify a conditional in copy_splice_read

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 2/4] splice: simplify a conditional in copy_splice_read

[Christian Brauner]

On Wed, Jun 14, 2023 at 04:03:39PM +0200, Christoph Hellwig wrote:
> Check for -EFAULT instead of wrapping the check in an ret < 0 block.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

[PATCH 3/4] block: remove BIO_PAGE_REFFED

[Christoph Hellwig]

Now that all block direct I/O helpers use page pinning, this flag is
unused.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk.h               | 2 --
 include/linux/bio.h       | 3 +--
 include/linux/blk_types.h | 1 -
 3 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/block/blk.h b/block/blk.h
index 768852a84fefb3..608c5dcc516b55 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -445,8 +445,6 @@ static inline void bio_release_page(struct bio *bio, struct page *page)
 {
 	if (bio_flagged(bio, BIO_PAGE_PINNED))
 		unpin_user_page(page);
-	else if (bio_flagged(bio, BIO_PAGE_REFFED))
-		put_page(page);
 }
 
 struct request_queue *blk_alloc_queue(int node_id);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 61752292896494..c4f5b5228105fe 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -492,8 +492,7 @@ void zero_fill_bio(struct bio *bio);
 
 static inline void bio_release_pages(struct bio *bio, bool mark_dirty)
 {
-	if (bio_flagged(bio, BIO_PAGE_REFFED) ||
-	    bio_flagged(bio, BIO_PAGE_PINNED))
+	if (bio_flagged(bio, BIO_PAGE_PINNED))
 		__bio_release_pages(bio, mark_dirty);
 }
 
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index deb69eeab6bd7b..752a54e3284b27 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -326,7 +326,6 @@ struct bio {
  */
 enum {
 	BIO_PAGE_PINNED,	/* Unpin pages in bio_release_pages() */
-	BIO_PAGE_REFFED,	/* put pages in bio_release_pages() */
 	BIO_CLONED,		/* doesn't own data */
 	BIO_BOUNCED,		/* bio is a bounce bio */
 	BIO_QUIET,		/* Make BIO Quiet */
-- 
2.39.2

Re: [PATCH 3/4] block: remove BIO_PAGE_REFFED

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 3/4] block: remove BIO_PAGE_REFFED

[Christian Brauner]

On Wed, Jun 14, 2023 at 04:03:40PM +0200, Christoph Hellwig wrote:
> Now that all block direct I/O helpers use page pinning, this flag is
> unused.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

[PATCH 4/4] iov_iter: remove iov_iter_get_pages and iov_iter_get_pages_alloc

[Christoph Hellwig]

Now that the direct I/O helpers have switched to use
iov_iter_extract_pages, these helpers are unused.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/uio.h |  6 ------
 lib/iov_iter.c      | 35 +++++++----------------------------
 2 files changed, 7 insertions(+), 34 deletions(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 60c342bb7ab89f..8e7d2c425340c5 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -277,14 +277,8 @@ void iov_iter_bvec(struct iov_iter *i, unsigned int direction, const struct bio_
 void iov_iter_discard(struct iov_iter *i, unsigned int direction, size_t count);
 void iov_iter_xarray(struct iov_iter *i, unsigned int direction, struct xarray *xarray,
 		     loff_t start, size_t count);
-ssize_t iov_iter_get_pages(struct iov_iter *i, struct page **pages,
-		size_t maxsize, unsigned maxpages, size_t *start,
-		iov_iter_extraction_t extraction_flags);
 ssize_t iov_iter_get_pages2(struct iov_iter *i, struct page **pages,
 			size_t maxsize, unsigned maxpages, size_t *start);
-ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
-		struct page ***pages, size_t maxsize, size_t *start,
-		iov_iter_extraction_t extraction_flags);
 ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i, struct page ***pages,
 			size_t maxsize, size_t *start);
 int iov_iter_npages(const struct iov_iter *i, int maxpages);
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index f18138e0292ae4..b667b1e2f68866 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1082,8 +1082,7 @@ static struct page *first_bvec_segment(const struct iov_iter *i,
 
 static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
 		   struct page ***pages, size_t maxsize,
-		   unsigned int maxpages, size_t *start,
-		   iov_iter_extraction_t extraction_flags)
+		   unsigned int maxpages, size_t *start)
 {
 	unsigned int n, gup_flags = 0;
 
@@ -1093,8 +1092,6 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
 		return 0;
 	if (maxsize > MAX_RW_COUNT)
 		maxsize = MAX_RW_COUNT;
-	if (extraction_flags & ITER_ALLOW_P2PDMA)
-		gup_flags |= FOLL_PCI_P2PDMA;
 
 	if (likely(user_backed_iter(i))) {
 		unsigned long addr;
@@ -1144,49 +1141,31 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
 	return -EFAULT;
 }
 
-ssize_t iov_iter_get_pages(struct iov_iter *i,
-		   struct page **pages, size_t maxsize, unsigned maxpages,
-		   size_t *start, iov_iter_extraction_t extraction_flags)
+ssize_t iov_iter_get_pages2(struct iov_iter *i, struct page **pages,
+		size_t maxsize, unsigned maxpages, size_t *start)
 {
 	if (!maxpages)
 		return 0;
 	BUG_ON(!pages);
 
-	return __iov_iter_get_pages_alloc(i, &pages, maxsize, maxpages,
-					  start, extraction_flags);
-}
-EXPORT_SYMBOL_GPL(iov_iter_get_pages);
-
-ssize_t iov_iter_get_pages2(struct iov_iter *i, struct page **pages,
-		size_t maxsize, unsigned maxpages, size_t *start)
-{
-	return iov_iter_get_pages(i, pages, maxsize, maxpages, start, 0);
+	return __iov_iter_get_pages_alloc(i, &pages, maxsize, maxpages, start);
 }
 EXPORT_SYMBOL(iov_iter_get_pages2);
 
-ssize_t iov_iter_get_pages_alloc(struct iov_iter *i,
-		   struct page ***pages, size_t maxsize,
-		   size_t *start, iov_iter_extraction_t extraction_flags)
+ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i,
+		struct page ***pages, size_t maxsize, size_t *start)
 {
 	ssize_t len;
 
 	*pages = NULL;
 
-	len = __iov_iter_get_pages_alloc(i, pages, maxsize, ~0U, start,
-					 extraction_flags);
+	len = __iov_iter_get_pages_alloc(i, pages, maxsize, ~0U, start);
 	if (len <= 0) {
 		kvfree(*pages);
 		*pages = NULL;
 	}
 	return len;
 }
-EXPORT_SYMBOL_GPL(iov_iter_get_pages_alloc);
-
-ssize_t iov_iter_get_pages_alloc2(struct iov_iter *i,
-		struct page ***pages, size_t maxsize, size_t *start)
-{
-	return iov_iter_get_pages_alloc(i, pages, maxsize, start, 0);
-}
 EXPORT_SYMBOL(iov_iter_get_pages_alloc2);
 
 size_t csum_and_copy_from_iter(void *addr, size_t bytes, __wsum *csum,
-- 
2.39.2

Re: [PATCH 4/4] iov_iter: remove iov_iter_get_pages and iov_iter_get_pages_alloc

[Christian Brauner]

On Wed, Jun 14, 2023 at 04:03:41PM +0200, Christoph Hellwig wrote:
> Now that the direct I/O helpers have switched to use
> iov_iter_extract_pages, these helpers are unused.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 2/4] splice: simplify a conditional in copy_splice_read

[David Howells]

Christoph Hellwig <hch@lst.de> wrote:

> +	if (ret == -EFAULT)

I wonder if that should be prefaced with "else".  On the other hand, the
optimiser ought to spot that it's mutually exclusive with the prior "if".

David

Re: dio / splice fixups

[David Howells]

Reviewed-by: David Howells <dhowells@redhat.com>

Re: dio / splice fixups

[Jens Axboe]

On Wed, 14 Jun 2023 16:03:37 +0200, Christoph Hellwig wrote:
> this series has a small fix and a bunch of cleanups on top of the
> splice and direct I/O rework in the block tree.
> 
> block/blk.h               |    2 --
>  fs/splice.c               |   15 +++++++--------
>  include/linux/bio.h       |    3 +--
>  include/linux/blk_types.h |    1 -
>  include/linux/uio.h       |    6 ------
>  lib/iov_iter.c            |   35 +++++++----------------------------
>  6 files changed, 15 insertions(+), 47 deletions(-)
> 
> [...]

Applied, thanks!

[1/4] splice: don't call file_accessed in copy_splice_read
      commit: 0b24be4691c9e6ea13ca70050d42a9f9032fa788
[2/4] splice: simplify a conditional in copy_splice_read
      commit: 2e82f6c3bfd1acde2610dd9feb4f2b264c4ef742
[3/4] block: remove BIO_PAGE_REFFED
      commit: e4cc64657becbd073c3ecc9d5938a1fe0d59913f
[4/4] iov_iter: remove iov_iter_get_pages and iov_iter_get_pages_alloc
      commit: 84bd06c632c6d5279849f5f8ab47d9517d259422

Best regards,
-- 
Jens Axboe