iomap SEEK_HOLE / SEEK_DATA cleanups and generalization
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.
PATCH 1/3 fs: move page_cache_seek_hole_data to iomap.c
This function is only used by the iomap code, depends on being called from it, and will soon stop poking into buffer head internals.
--- fs/buffer.c | 114 ----------------------------------- fs/iomap.c | 116 ++++++++++++++++++++++++++++++++++++ include/linux/buffer_head.h | 2 - 3 files changed, 116 insertions(+), 116 deletions(-)
fs/buffer.c +0 -114
@@ -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;
fs/iomap.c +116 -0
128 lines not on this page · raw message
include/linux/buffer_head.h +0 -2
@@ -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;
Signature · 1 line
2.17.0
Re: [PATCH 1/3] fs: move page_cache_seek_hole_data to iomap.c
Quoted 4 lines · Christoph Hellwig
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 <[email protected]>
Looks fine.
Signature · 2 lines
Dave Chinner [email protected]
PATCH 2/3 fs: remove the buffer_unwritten check in page_seek_hole_data
In reply to Christoph HellwigWe 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.
--- fs/iomap.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
fs/iomap.c +4 -9
@@ -816,14 +816,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;
@@ -835,8 +830,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. */
Signature · 1 line
2.17.0
Re: [PATCH 2/3] fs: remove the buffer_unwritten check in page_seek_hole_data
Quoted 8 lines · Christoph Hellwig
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 <[email protected]>--- fs/iomap.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
Makes sense to me.
Signature · 2 lines
Dave Chinner [email protected]
PATCH 3/3 fs: use ->is_partially_uptodate in page_cache_seek_hole_data
In reply to Christoph HellwigThis way the implementation doesn't depend on buffer_head internals.
--- fs/iomap.c | 85 +++++++++++++++++++++++++----------------------------- 1 file changed, 39 insertions(+), 46 deletions(-)
fs/iomap.c +39 -46
@@ -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);
Signature · 1 line
2.17.0
Re: [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data
2018-05-30 12:02 GMT+02:00 Christoph Hellwig <[email protected]>:
Quoted 59 lines
This way the implementation doesn't depend on
buffer_headinternals.Signed-off-by Christoph Hellwig <[email protected]>--- fs/iomap.c | 85 +++++++++++++++++++++++++----------------------------- 1 file changed, 39 insertions(+), 46 deletions(-)fs/iomap.c +18
-20@@ -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:
fs/iomap.c int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, · line 827
Quoted 10 lines
fs/iomap.c +6
-3+*/ +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.
Quoted 50 lines
fs/iomap.c +15
-24+ +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); --
Quoted 1 line
2.17.0
Other than the above, these three patches look okay, and they have passed xfstests on gfs2.
Thanks,
Andreas
Re: [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data
In reply to Christoph HellwigQuoted 15 lines · Christoph Hellwig
This way the implementation doesn't depend on
buffer_headinternals.Signed-off-by Christoph Hellwig <[email protected]>--- fs/iomap.c | 85 +++++++++++++++++++++++++----------------------------- 1 file changed, 39 insertions(+), 46 deletions(-)fs/iomap.c +0
-0@@ -794,36 +794,51 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, }EXPORT_SYMBOL_GPL (iomap_fiemap);
fs/iomap.c int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, · line 799
Quoted 10 lines
fs/iomap.c +3
-7-/* -* 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?
fs/iomap.c int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, · line 802
Quoted 5 lines
fs/iomap.c +2
-2{ -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.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH 3/3] fs: use ->is_partially_uptodate in page_cache_seek_hole_data
Quoted 4 lines · Dave Chinner
Quoted 4 lines
-/*
- * Seek forSEEK_DATA/SEEK_HOLEwithin @page, starting at @lastoff.
- *
- * Returns the offset within the file on success, and-ENOENTotherwise.
Quoted 8 lines
fs/iomap.c int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, · line 799
Quoted 6 lines
- */
fs/iomap.c +3
-2-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.
Quoted 4 lines
fs/iomap.c int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi, · line 802
Quoted 2 lines
fs/iomap.c +2
-0+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..
Quoted 4 lines · Christoph Hellwig
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.
I added RVB tags to the previous version of this, but I guess they fell off? Have these three patches changed considerably since v3?
--D
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 8 lines · Darrick J. Wong
Quoted 4 lines · Christoph Hellwig
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.
I added RVB tags to the previous version of this, but I guess they fell off? Have these three patches changed considerably since v3?
They haven't changed at all. So with the review from Andreas this might be a good time to pull them in and get these patches off the backlog :)
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Darrick J. Wong
Quoted 3 lines · Christoph Hellwig
Refactor the seek pagecache helpers to work independent of
buffer_heads, and clean up a few lose bits.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
This is needed for the small blocksize buffered I/O support in iomap.
I added RVB tags to the previous version of this, but I guess they fell off? Have these three patches changed considerably since v3?
They haven't changed at all. So with the review from Andreas this might be a good time to pull them in and get these patches off the backlog :)
I know, but I skip the patches that already have my RVBs attached, so dropping them just slows down the review because I will go reread the whole thing assuming that there's some critical difference from the previous submission.
--D
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html