linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* iomap SEEK_HOLE / SEEK_DATA cleanups and generalization v2
@ 2018-05-31 18:07 Christoph Hellwig
  2018-05-31 18:07 ` [PATCH 1/3] fs: move page_cache_seek_hole_data to iomap.c Christoph Hellwig
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Christoph Hellwig @ 2018-05-31 18:07 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andreas Gruenbacher, linux-fsdevel, linux-ext4

Refactor the seek pagecache helpers to work independent of buffer_heads,
and clean up a few lose bits.

This is needed for the small blocksize buffered I/O support in iomap.

Changes since v1:
 - a trivial comment update

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] fs: move page_cache_seek_hole_data to iomap.c
  2018-05-31 18:07 iomap SEEK_HOLE / SEEK_DATA cleanups and generalization v2 Christoph Hellwig
@ 2018-05-31 18:07 ` Christoph Hellwig
  2018-05-31 18:07 ` [PATCH 2/3] fs: remove the buffer_unwritten check in page_seek_hole_data Christoph Hellwig
  2018-05-31 18:07 ` [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data Christoph Hellwig
  2 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2018-05-31 18:07 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andreas Gruenbacher, linux-fsdevel, linux-ext4

This function is only used by the iomap code, depends on being called
from it, and will soon stop poking into buffer head internals.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/buffer.c                 | 114 -----------------------------------
 fs/iomap.c                  | 116 ++++++++++++++++++++++++++++++++++++
 include/linux/buffer_head.h |   2 -
 3 files changed, 116 insertions(+), 116 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 249b83fafe48..cabc045f483d 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -3427,120 +3427,6 @@ int bh_submit_read(struct buffer_head *bh)
 }
 EXPORT_SYMBOL(bh_submit_read);
 
-/*
- * Seek for SEEK_DATA / SEEK_HOLE within @page, starting at @lastoff.
- *
- * Returns the offset within the file on success, and -ENOENT otherwise.
- */
-static loff_t
-page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
-{
-	loff_t offset = page_offset(page);
-	struct buffer_head *bh, *head;
-	bool seek_data = whence == SEEK_DATA;
-
-	if (lastoff < offset)
-		lastoff = offset;
-
-	bh = head = page_buffers(page);
-	do {
-		offset += bh->b_size;
-		if (lastoff >= offset)
-			continue;
-
-		/*
-		 * Unwritten extents that have data in the page cache covering
-		 * them can be identified by the BH_Unwritten state flag.
-		 * Pages with multiple buffers might have a mix of holes, data
-		 * and unwritten extents - any buffer with valid data in it
-		 * should have BH_Uptodate flag set on it.
-		 */
-
-		if ((buffer_unwritten(bh) || buffer_uptodate(bh)) == seek_data)
-			return lastoff;
-
-		lastoff = offset;
-	} while ((bh = bh->b_this_page) != head);
-	return -ENOENT;
-}
-
-/*
- * Seek for SEEK_DATA / SEEK_HOLE in the page cache.
- *
- * Within unwritten extents, the page cache determines which parts are holes
- * and which are data: unwritten and uptodate buffer heads count as data;
- * everything else counts as a hole.
- *
- * Returns the resulting offset on successs, and -ENOENT otherwise.
- */
-loff_t
-page_cache_seek_hole_data(struct inode *inode, loff_t offset, loff_t length,
-			  int whence)
-{
-	pgoff_t index = offset >> PAGE_SHIFT;
-	pgoff_t end = DIV_ROUND_UP(offset + length, PAGE_SIZE);
-	loff_t lastoff = offset;
-	struct pagevec pvec;
-
-	if (length <= 0)
-		return -ENOENT;
-
-	pagevec_init(&pvec);
-
-	do {
-		unsigned nr_pages, i;
-
-		nr_pages = pagevec_lookup_range(&pvec, inode->i_mapping, &index,
-						end - 1);
-		if (nr_pages == 0)
-			break;
-
-		for (i = 0; i < nr_pages; i++) {
-			struct page *page = pvec.pages[i];
-
-			/*
-			 * At this point, the page may be truncated or
-			 * invalidated (changing page->mapping to NULL), or
-			 * even swizzled back from swapper_space to tmpfs file
-			 * mapping.  However, page->index will not change
-			 * because we have a reference on the page.
-                         *
-			 * If current page offset is beyond where we've ended,
-			 * we've found a hole.
-                         */
-			if (whence == SEEK_HOLE &&
-			    lastoff < page_offset(page))
-				goto check_range;
-
-			lock_page(page);
-			if (likely(page->mapping == inode->i_mapping) &&
-			    page_has_buffers(page)) {
-				lastoff = page_seek_hole_data(page, lastoff, whence);
-				if (lastoff >= 0) {
-					unlock_page(page);
-					goto check_range;
-				}
-			}
-			unlock_page(page);
-			lastoff = page_offset(page) + PAGE_SIZE;
-		}
-		pagevec_release(&pvec);
-	} while (index < end);
-
-	/* When no page at lastoff and we are not done, we found a hole. */
-	if (whence != SEEK_HOLE)
-		goto not_found;
-
-check_range:
-	if (lastoff < offset + length)
-		goto out;
-not_found:
-	lastoff = -ENOENT;
-out:
-	pagevec_release(&pvec);
-	return lastoff;
-}
-
 void __init buffer_init(void)
 {
 	unsigned long nrpages;
diff --git a/fs/iomap.c b/fs/iomap.c
index 106720355963..f553a2d8a5fa 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -21,6 +21,7 @@
 #include <linux/mm_inline.h>
 #include <linux/swap.h>
 #include <linux/pagemap.h>
+#include <linux/pagevec.h>
 #include <linux/file.h>
 #include <linux/uio.h>
 #include <linux/backing-dev.h>
@@ -804,6 +805,121 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
 }
 EXPORT_SYMBOL_GPL(iomap_fiemap);
 
+/*
+ * Seek for SEEK_DATA / SEEK_HOLE within @page, starting at @lastoff.
+ *
+ * Returns the offset within the file on success, and -ENOENT otherwise.
+ */
+static loff_t
+page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
+{
+	loff_t offset = page_offset(page);
+	struct buffer_head *bh, *head;
+	bool seek_data = whence == SEEK_DATA;
+
+	if (lastoff < offset)
+		lastoff = offset;
+
+	bh = head = page_buffers(page);
+	do {
+		offset += bh->b_size;
+		if (lastoff >= offset)
+			continue;
+
+		/*
+		 * Unwritten extents that have data in the page cache covering
+		 * them can be identified by the BH_Unwritten state flag.
+		 * Pages with multiple buffers might have a mix of holes, data
+		 * and unwritten extents - any buffer with valid data in it
+		 * should have BH_Uptodate flag set on it.
+		 */
+
+		if ((buffer_unwritten(bh) || buffer_uptodate(bh)) == seek_data)
+			return lastoff;
+
+		lastoff = offset;
+	} while ((bh = bh->b_this_page) != head);
+	return -ENOENT;
+}
+
+/*
+ * Seek for SEEK_DATA / SEEK_HOLE in the page cache.
+ *
+ * Within unwritten extents, the page cache determines which parts are holes
+ * and which are data: unwritten and uptodate buffer heads count as data;
+ * everything else counts as a hole.
+ *
+ * Returns the resulting offset on successs, and -ENOENT otherwise.
+ */
+static loff_t
+page_cache_seek_hole_data(struct inode *inode, loff_t offset, loff_t length,
+		int whence)
+{
+	pgoff_t index = offset >> PAGE_SHIFT;
+	pgoff_t end = DIV_ROUND_UP(offset + length, PAGE_SIZE);
+	loff_t lastoff = offset;
+	struct pagevec pvec;
+
+	if (length <= 0)
+		return -ENOENT;
+
+	pagevec_init(&pvec);
+
+	do {
+		unsigned nr_pages, i;
+
+		nr_pages = pagevec_lookup_range(&pvec, inode->i_mapping, &index,
+						end - 1);
+		if (nr_pages == 0)
+			break;
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page *page = pvec.pages[i];
+
+			/*
+			 * At this point, the page may be truncated or
+			 * invalidated (changing page->mapping to NULL), or
+			 * even swizzled back from swapper_space to tmpfs file
+			 * mapping.  However, page->index will not change
+			 * because we have a reference on the page.
+                         *
+			 * If current page offset is beyond where we've ended,
+			 * we've found a hole.
+                         */
+			if (whence == SEEK_HOLE &&
+			    lastoff < page_offset(page))
+				goto check_range;
+
+			lock_page(page);
+			if (likely(page->mapping == inode->i_mapping) &&
+			    page_has_buffers(page)) {
+				lastoff = page_seek_hole_data(page, lastoff, whence);
+				if (lastoff >= 0) {
+					unlock_page(page);
+					goto check_range;
+				}
+			}
+			unlock_page(page);
+			lastoff = page_offset(page) + PAGE_SIZE;
+		}
+		pagevec_release(&pvec);
+	} while (index < end);
+
+	/* When no page at lastoff and we are not done, we found a hole. */
+	if (whence != SEEK_HOLE)
+		goto not_found;
+
+check_range:
+	if (lastoff < offset + length)
+		goto out;
+not_found:
+	lastoff = -ENOENT;
+out:
+	pagevec_release(&pvec);
+	return lastoff;
+}
+
+
 static loff_t
 iomap_seek_hole_actor(struct inode *inode, loff_t offset, loff_t length,
 		      void *data, struct iomap *iomap)
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 894e5d125de6..96225a77c112 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -205,8 +205,6 @@ void write_boundary_block(struct block_device *bdev,
 			sector_t bblock, unsigned blocksize);
 int bh_uptodate_or_lock(struct buffer_head *bh);
 int bh_submit_read(struct buffer_head *bh);
-loff_t page_cache_seek_hole_data(struct inode *inode, loff_t offset,
-				 loff_t length, int whence);
 
 extern int buffer_heads_over_limit;
 
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] fs: remove the buffer_unwritten check in page_seek_hole_data
  2018-05-31 18:07 iomap SEEK_HOLE / SEEK_DATA cleanups and generalization v2 Christoph Hellwig
  2018-05-31 18:07 ` [PATCH 1/3] fs: move page_cache_seek_hole_data to iomap.c Christoph Hellwig
@ 2018-05-31 18:07 ` Christoph Hellwig
  2018-05-31 18:07 ` [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data Christoph Hellwig
  2 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2018-05-31 18:07 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andreas Gruenbacher, linux-fsdevel, linux-ext4

We only call into this function through the iomap iterators, so we already
know the buffer is unwritten.  In addition to that we always require the
uptodate flag that is ORed with the result anyway.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 fs/iomap.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index f553a2d8a5fa..e93bd4eb7fa7 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -827,14 +827,9 @@ page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
 			continue;
 
 		/*
-		 * Unwritten extents that have data in the page cache covering
-		 * them can be identified by the BH_Unwritten state flag.
-		 * Pages with multiple buffers might have a mix of holes, data
-		 * and unwritten extents - any buffer with valid data in it
-		 * should have BH_Uptodate flag set on it.
+		 * Any buffer with valid data in it should have BH_Uptodate set.
 		 */
-
-		if ((buffer_unwritten(bh) || buffer_uptodate(bh)) == seek_data)
+		if (buffer_uptodate(bh) == seek_data)
 			return lastoff;
 
 		lastoff = offset;
@@ -846,8 +841,8 @@ page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
  * Seek for SEEK_DATA / SEEK_HOLE in the page cache.
  *
  * Within unwritten extents, the page cache determines which parts are holes
- * and which are data: unwritten and uptodate buffer heads count as data;
- * everything else counts as a hole.
+ * and which are data: uptodate buffer heads count as data; everything else
+ * counts as a hole.
  *
  * Returns the resulting offset on successs, and -ENOENT otherwise.
  */
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data
  2018-05-31 18:07 iomap SEEK_HOLE / SEEK_DATA cleanups and generalization v2 Christoph Hellwig
  2018-05-31 18:07 ` [PATCH 1/3] fs: move page_cache_seek_hole_data to iomap.c Christoph Hellwig
  2018-05-31 18:07 ` [PATCH 2/3] fs: remove the buffer_unwritten check in page_seek_hole_data Christoph Hellwig
@ 2018-05-31 18:07 ` Christoph Hellwig
  2 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2018-05-31 18:07 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andreas Gruenbacher, linux-fsdevel, linux-ext4

This way the implementation doesn't depend on buffer_head internals.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/iomap.c | 83 ++++++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 43 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index e93bd4eb7fa7..49119e64edc3 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -807,34 +807,53 @@ EXPORT_SYMBOL_GPL(iomap_fiemap);
 
 /*
  * Seek for SEEK_DATA / SEEK_HOLE within @page, starting at @lastoff.
- *
- * Returns the offset within the file on success, and -ENOENT otherwise.
+ * Returns true if found and updates @lastoff to the offset in file.
  */
-static loff_t
-page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
+static bool
+page_seek_hole_data(struct inode *inode, struct page *page, loff_t *lastoff,
+		int whence)
 {
-	loff_t offset = page_offset(page);
-	struct buffer_head *bh, *head;
+	const struct address_space_operations *ops = inode->i_mapping->a_ops;
+	unsigned int bsize = i_blocksize(inode), off;
 	bool seek_data = whence == SEEK_DATA;
+	loff_t poff = page_offset(page);
 
-	if (lastoff < offset)
-		lastoff = offset;
-
-	bh = head = page_buffers(page);
-	do {
-		offset += bh->b_size;
-		if (lastoff >= offset)
-			continue;
+	if (WARN_ON_ONCE(*lastoff >= poff + PAGE_SIZE))
+		return false;
 
+	if (*lastoff < poff) {
 		/*
-		 * Any buffer with valid data in it should have BH_Uptodate set.
+		 * Last offset smaller than the start of the page means we found
+		 * a hole:
 		 */
-		if (buffer_uptodate(bh) == seek_data)
-			return lastoff;
+		if (whence == SEEK_HOLE)
+			return true;
+		*lastoff = poff;
+	}
 
-		lastoff = offset;
-	} while ((bh = bh->b_this_page) != head);
-	return -ENOENT;
+	/*
+	 * Just check the page unless we can and should check block ranges:
+	 */
+	if (bsize == PAGE_SIZE || !ops->is_partially_uptodate)
+		return PageUptodate(page) == seek_data;
+
+	lock_page(page);
+	if (unlikely(page->mapping != inode->i_mapping))
+		goto out_unlock_not_found;
+
+	for (off = 0; off < PAGE_SIZE; off += bsize) {
+		if ((*lastoff & ~PAGE_MASK) >= off + bsize)
+			continue;
+		if (ops->is_partially_uptodate(page, off, bsize) == seek_data) {
+			unlock_page(page);
+			return true;
+		}
+		*lastoff = poff + off + bsize;
+	}
+
+out_unlock_not_found:
+	unlock_page(page);
+	return false;
 }
 
 /*
@@ -871,30 +890,8 @@ page_cache_seek_hole_data(struct inode *inode, loff_t offset, loff_t length,
 		for (i = 0; i < nr_pages; i++) {
 			struct page *page = pvec.pages[i];
 
-			/*
-			 * At this point, the page may be truncated or
-			 * invalidated (changing page->mapping to NULL), or
-			 * even swizzled back from swapper_space to tmpfs file
-			 * mapping.  However, page->index will not change
-			 * because we have a reference on the page.
-                         *
-			 * If current page offset is beyond where we've ended,
-			 * we've found a hole.
-                         */
-			if (whence == SEEK_HOLE &&
-			    lastoff < page_offset(page))
+			if (page_seek_hole_data(inode, page, &lastoff, whence))
 				goto check_range;
-
-			lock_page(page);
-			if (likely(page->mapping == inode->i_mapping) &&
-			    page_has_buffers(page)) {
-				lastoff = page_seek_hole_data(page, lastoff, whence);
-				if (lastoff >= 0) {
-					unlock_page(page);
-					goto check_range;
-				}
-			}
-			unlock_page(page);
 			lastoff = page_offset(page) + PAGE_SIZE;
 		}
 		pagevec_release(&pvec);
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data
  2018-05-30 23:00   ` Dave Chinner
@ 2018-05-31  6:20     ` Christoph Hellwig
  0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2018-05-31  6:20 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Christoph Hellwig, linux-xfs, Andreas Gruenbacher, linux-fsdevel,
	linux-ext4

On Thu, May 31, 2018 at 09:00:38AM +1000, Dave Chinner wrote:
> > -/*
> > - * Seek for SEEK_DATA / SEEK_HOLE within @page, starting at @lastoff.
> > - *
> > - * Returns the offset within the file on success, and -ENOENT otherwise.
> > - */
> > -static loff_t
> > -page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
> > +static bool
> > +page_seek_hole_data(struct inode *inode, struct page *page, loff_t *lastoff,
> > +		int whence)
> 
> Can we keep the comment explaining the return value?

The old comment isn't going to be correct.  I've added back a changed
comment, akthough I don't really think it helps.

> > +	const struct address_space_operations *ops = inode->i_mapping->a_ops;
> > +	unsigned int bsize = i_blocksize(inode), off;
> 
> Split this into two lines.

We've got a pattern here - I really like it that way..

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data
  2018-05-30 10:02 ` [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data Christoph Hellwig
  2018-05-30 13:48   ` Andreas Grünbacher
@ 2018-05-30 23:00   ` Dave Chinner
  2018-05-31  6:20     ` Christoph Hellwig
  1 sibling, 1 reply; 8+ messages in thread
From: Dave Chinner @ 2018-05-30 23:00 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-xfs, Andreas Gruenbacher, linux-fsdevel, linux-ext4

On Wed, May 30, 2018 at 12:02:08PM +0200, Christoph Hellwig wrote:
> This way the implementation doesn't depend on buffer_head internals.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap.c | 85 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 39 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index e75153f52748..5b92243808d3 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -794,36 +794,51 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
>  }
>  EXPORT_SYMBOL_GPL(iomap_fiemap);
>  
> -/*
> - * Seek for SEEK_DATA / SEEK_HOLE within @page, starting at @lastoff.
> - *
> - * Returns the offset within the file on success, and -ENOENT otherwise.
> - */
> -static loff_t
> -page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
> +static bool
> +page_seek_hole_data(struct inode *inode, struct page *page, loff_t *lastoff,
> +		int whence)

Can we keep the comment explaining the return value?

>  {
> -	loff_t offset = page_offset(page);
> -	struct buffer_head *bh, *head;
> +	const struct address_space_operations *ops = inode->i_mapping->a_ops;
> +	unsigned int bsize = i_blocksize(inode), off;

Split this into two lines.

Otherwise looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data
  2018-05-30 10:02 ` [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data Christoph Hellwig
@ 2018-05-30 13:48   ` Andreas Grünbacher
  2018-05-30 23:00   ` Dave Chinner
  1 sibling, 0 replies; 8+ messages in thread
From: Andreas Grünbacher @ 2018-05-30 13:48 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-xfs, Andreas Gruenbacher, Linux FS-devel Mailing List, linux-ext4

2018-05-30 12:02 GMT+02:00 Christoph Hellwig <hch@lst.de>:
> This way the implementation doesn't depend on buffer_head internals.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap.c | 85 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 39 insertions(+), 46 deletions(-)
>
> diff --git a/fs/iomap.c b/fs/iomap.c
> index e75153f52748..5b92243808d3 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -794,36 +794,51 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
>  }
>  EXPORT_SYMBOL_GPL(iomap_fiemap);
>
> -/*
> - * Seek for SEEK_DATA / SEEK_HOLE within @page, starting at @lastoff.
> - *
> - * Returns the offset within the file on success, and -ENOENT otherwise.
> - */
> -static loff_t
> -page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
> +static bool
> +page_seek_hole_data(struct inode *inode, struct page *page, loff_t *lastoff,
> +               int whence)
>  {
> -       loff_t offset = page_offset(page);
> -       struct buffer_head *bh, *head;
> +       const struct address_space_operations *ops = inode->i_mapping->a_ops;
> +       unsigned int bsize = i_blocksize(inode), off;
>         bool seek_data = whence == SEEK_DATA;
> +       loff_t poff = page_offset(page);
>
> -       if (lastoff < offset)
> -               lastoff = offset;
> -
> -       bh = head = page_buffers(page);
> -       do {
> -               offset += bh->b_size;
> -               if (lastoff >= offset)
> -                       continue;
> +       if (WARN_ON_ONCE(*lastoff >= poff + PAGE_SIZE))
> +               return false;
>
> +       if (*lastoff < poff) {
>                 /*
> -                * Any buffer with valid data in it should have BH_Uptodate set.
> +                * Last offset smaller than the start of the page means we found
> +                * a hole:
>                  */
> -               if (buffer_uptodate(bh) == seek_data)
> -                       return lastoff;
> +               if (whence == SEEK_HOLE)
> +                       return true;
> +               *lastoff = poff;
> +       }
> +
> +       /*
> +        * Just check the page unless we can and should check block ranges:
> +        */
> +       if (bsize == PAGE_SIZE || !ops->is_partially_uptodate)
> +               return PageUptodate(page) == seek_data;
>
> -               lastoff = offset;
> -       } while ((bh = bh->b_this_page) != head);
> -       return -ENOENT;
> +       lock_page(page);
> +       if (unlikely(page->mapping != inode->i_mapping))
> +               goto out_unlock_not_found;

Not a regression, but I guess it would make more sense to treat the
above as a hole.

> +
> +       for (off = 0; off < PAGE_SIZE; off += bsize) {
> +               if ((*lastoff & ~PAGE_MASK) >= off + bsize)
> +                       continue;
> +               if (ops->is_partially_uptodate(page, off, bsize) == seek_data) {
> +                       unlock_page(page);
> +                       return true;
> +               }
> +               *lastoff = poff + off + bsize;
> +       }
> +
> +out_unlock_not_found:
> +       unlock_page(page);
> +       return false;
>  }
>
>  /*
> @@ -860,30 +875,8 @@ page_cache_seek_hole_data(struct inode *inode, loff_t offset, loff_t length,
>                 for (i = 0; i < nr_pages; i++) {
>                         struct page *page = pvec.pages[i];
>
> -                       /*
> -                        * At this point, the page may be truncated or
> -                        * invalidated (changing page->mapping to NULL), or
> -                        * even swizzled back from swapper_space to tmpfs file
> -                        * mapping.  However, page->index will not change
> -                        * because we have a reference on the page.
> -                         *
> -                        * If current page offset is beyond where we've ended,
> -                        * we've found a hole.
> -                         */
> -                       if (whence == SEEK_HOLE &&
> -                           lastoff < page_offset(page))
> +                       if (page_seek_hole_data(inode, page, &lastoff, whence))
>                                 goto check_range;
> -
> -                       lock_page(page);
> -                       if (likely(page->mapping == inode->i_mapping) &&
> -                           page_has_buffers(page)) {
> -                               lastoff = page_seek_hole_data(page, lastoff, whence);
> -                               if (lastoff >= 0) {
> -                                       unlock_page(page);
> -                                       goto check_range;
> -                               }
> -                       }
> -                       unlock_page(page);
>                         lastoff = page_offset(page) + PAGE_SIZE;
>                 }
>                 pagevec_release(&pvec);
> --
> 2.17.0

Other than the above, these three patches look okay, and they have
passed xfstests on gfs2.

Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>

Thanks,
Andreas

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data
  2018-05-30 10:02 iomap SEEK_HOLE / SEEK_DATA cleanups and generalization Christoph Hellwig
@ 2018-05-30 10:02 ` Christoph Hellwig
  2018-05-30 13:48   ` Andreas Grünbacher
  2018-05-30 23:00   ` Dave Chinner
  0 siblings, 2 replies; 8+ messages in thread
From: Christoph Hellwig @ 2018-05-30 10:02 UTC (permalink / raw)
  To: linux-xfs; +Cc: Andreas Gruenbacher, linux-fsdevel, linux-ext4

This way the implementation doesn't depend on buffer_head internals.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap.c | 85 +++++++++++++++++++++++++-----------------------------
 1 file changed, 39 insertions(+), 46 deletions(-)

diff --git a/fs/iomap.c b/fs/iomap.c
index e75153f52748..5b92243808d3 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -794,36 +794,51 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
 }
 EXPORT_SYMBOL_GPL(iomap_fiemap);
 
-/*
- * Seek for SEEK_DATA / SEEK_HOLE within @page, starting at @lastoff.
- *
- * Returns the offset within the file on success, and -ENOENT otherwise.
- */
-static loff_t
-page_seek_hole_data(struct page *page, loff_t lastoff, int whence)
+static bool
+page_seek_hole_data(struct inode *inode, struct page *page, loff_t *lastoff,
+		int whence)
 {
-	loff_t offset = page_offset(page);
-	struct buffer_head *bh, *head;
+	const struct address_space_operations *ops = inode->i_mapping->a_ops;
+	unsigned int bsize = i_blocksize(inode), off;
 	bool seek_data = whence == SEEK_DATA;
+	loff_t poff = page_offset(page);
 
-	if (lastoff < offset)
-		lastoff = offset;
-
-	bh = head = page_buffers(page);
-	do {
-		offset += bh->b_size;
-		if (lastoff >= offset)
-			continue;
+	if (WARN_ON_ONCE(*lastoff >= poff + PAGE_SIZE))
+		return false;
 
+	if (*lastoff < poff) {
 		/*
-		 * Any buffer with valid data in it should have BH_Uptodate set.
+		 * Last offset smaller than the start of the page means we found
+		 * a hole:
 		 */
-		if (buffer_uptodate(bh) == seek_data)
-			return lastoff;
+		if (whence == SEEK_HOLE)
+			return true;
+		*lastoff = poff;
+	}
+
+	/*
+	 * Just check the page unless we can and should check block ranges:
+	 */
+	if (bsize == PAGE_SIZE || !ops->is_partially_uptodate)
+		return PageUptodate(page) == seek_data;
 
-		lastoff = offset;
-	} while ((bh = bh->b_this_page) != head);
-	return -ENOENT;
+	lock_page(page);
+	if (unlikely(page->mapping != inode->i_mapping))
+		goto out_unlock_not_found;
+
+	for (off = 0; off < PAGE_SIZE; off += bsize) {
+		if ((*lastoff & ~PAGE_MASK) >= off + bsize)
+			continue;
+		if (ops->is_partially_uptodate(page, off, bsize) == seek_data) {
+			unlock_page(page);
+			return true;
+		}
+		*lastoff = poff + off + bsize;
+	}
+
+out_unlock_not_found:
+	unlock_page(page);
+	return false;
 }
 
 /*
@@ -860,30 +875,8 @@ page_cache_seek_hole_data(struct inode *inode, loff_t offset, loff_t length,
 		for (i = 0; i < nr_pages; i++) {
 			struct page *page = pvec.pages[i];
 
-			/*
-			 * At this point, the page may be truncated or
-			 * invalidated (changing page->mapping to NULL), or
-			 * even swizzled back from swapper_space to tmpfs file
-			 * mapping.  However, page->index will not change
-			 * because we have a reference on the page.
-                         *
-			 * If current page offset is beyond where we've ended,
-			 * we've found a hole.
-                         */
-			if (whence == SEEK_HOLE &&
-			    lastoff < page_offset(page))
+			if (page_seek_hole_data(inode, page, &lastoff, whence))
 				goto check_range;
-
-			lock_page(page);
-			if (likely(page->mapping == inode->i_mapping) &&
-			    page_has_buffers(page)) {
-				lastoff = page_seek_hole_data(page, lastoff, whence);
-				if (lastoff >= 0) {
-					unlock_page(page);
-					goto check_range;
-				}
-			}
-			unlock_page(page);
 			lastoff = page_offset(page) + PAGE_SIZE;
 		}
 		pagevec_release(&pvec);
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-05-31 18:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-31 18:07 iomap SEEK_HOLE / SEEK_DATA cleanups and generalization v2 Christoph Hellwig
2018-05-31 18:07 ` [PATCH 1/3] fs: move page_cache_seek_hole_data to iomap.c Christoph Hellwig
2018-05-31 18:07 ` [PATCH 2/3] fs: remove the buffer_unwritten check in page_seek_hole_data Christoph Hellwig
2018-05-31 18:07 ` [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2018-05-30 10:02 iomap SEEK_HOLE / SEEK_DATA cleanups and generalization Christoph Hellwig
2018-05-30 10:02 ` [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data Christoph Hellwig
2018-05-30 13:48   ` Andreas Grünbacher
2018-05-30 23:00   ` Dave Chinner
2018-05-31  6:20     ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).