xfs: Introduce a new function to find the desired type of offset from page cache
PATCH v6 2/4 xfs: Introduce a new function to find the desired type of offset from page cache
Introduce helpers to probe data or hole offset from page cache for unwritten extents.
--- fs/xfs/xfs_file.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 213 insertions(+), 0 deletions(-)
fs/xfs/xfs_file.c +213 -0
@@ -36,6 +36,7 @@#include <linux/dcache.h> #include <linux/falloc.h> +#include <linux/pagevec.h> static const struct vm_operations_struct xfs_file_vm_ops;
@@ -966,6 +967,218 @@ xfs_vm_page_mkwrite(return block_page_mkwrite (vma, vmf, xfs_get_blocks); } +/* +* This type is designed to indicate the type of offset we would like +* to search from page cache for either xfs_seek_data() or xfs_seek_hole(). +*/ +enum { +HOLE_OFF =0 , +DATA_OFF , +}; + +/* +* Lookup the desired type of offset from the given page. +* +* On success, return true and the offset argument will point to the +* searched offset. Otherwise this function will return false and +* keep the offset argument unchanged. +*/ +STATIC bool +xfs_lookup_buffer_offset ( +struct page *page, +loff_t *offset, +unsigned int type) +{ +loff_t lastoff =page_offset (page); +bool found = false; +struct buffer_head *bh, *head; + + bh = head =page_buffers (page); +do { +/* +* 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)) { +if (type ==DATA_OFF ) + found = true; + }else { +if (type ==HOLE_OFF ) + found = true; + } + +if (found) { + *offset = lastoff; +unlock_page (page); +break ; + } + lastoff += bh->b_size; + }while ((bh = bh->b_this_page) != head); + +return found; +} + +/* +* This routine is called to find out and return a data or hole offset +* from the page cache for unwritten extents according to the desired +* type for xfs_seek_data() or xfs_seek_hole(). +* +* The argument offset is used to tell where we start to search from the +* page cache. Map is used to figure out the end points of the range to +* lookup pages. +* +* Return true if the desired type of offset was found, and the argument +* offset is filled with that address. Otherwise, return false and keep +* offset unchanged. +*/ +STATIC bool +xfs_find_get_desired_pgoff ( +struct inode *inode, +struct xfs_bmbt_irec *map, +unsigned int type, +loff_t *offset) +{ +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +struct pagevec pvec; +pgoff_t index; +pgoff_t end; +loff_t endoff; +loff_t startoff = *offset; +loff_t lastoff = startoff; +bool found = false; + +pagevec_init (&pvec,0 ); + + index = startoff >>PAGE_CACHE_SHIFT ; + endoff =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount); + end = endoff >>PAGE_CACHE_SHIFT ; +do { +int want; +unsigned nr_pages; +unsigned int i; + + want =min_t (pgoff_t, end - index, (pgoff_t )PAGEVEC_SIZE ); + nr_pages =pagevec_lookup (&pvec, inode->i_mapping, index, + want); +/* +* No page mapped into given range. If we are searching holes +* and if this is the first time we got into the loop, it means +* that the given offset is landed in a hole, return it. +* +* If we have already stepped through some block buffers to find +* holes but they all contains data. In this case, the last +* offset is already updated and pointed to the end of the last +* mapped page, if it does not reach the endpoint to search, +* that means there should be a hole between them. +*/ +if (nr_pages ==0 ) { +/* Data search found nothing */ +if (type ==DATA_OFF ) +break ; + +ASSERT (type ==HOLE_OFF ); +if (lastoff == startoff || lastoff < endoff) { + found = true; + *offset = lastoff; + } +break ; + } + +/* +* At lease we found one page. If this is the first time we +* step into the loop, and if the first page index offset is +* greater than the given search offset, a hole was found. +*/ +if (type ==HOLE_OFF && lastoff == startoff && + lastoff <page_offset (pvec.pages[0 ])) { + found = true; +break ; + } + +for (i =0 ; i < nr_pages; i++) { +struct page *page = pvec.pages[i]; +loff_t b_offset; + +/* +* Page index is out of range, searching done. +* If the current offset is not reaches the end +* of the specified search range, there should +* be a hole between them. +*/ +if (page->index > end) { +if (type ==HOLE_OFF && lastoff < endoff) { + *offset = lastoff; + found = true; + } + goto out; + } + +lock_page (page); +/* +* Page truncated or invalidated(page->mapping == NULL). +* We can freely skip it and proceed to check the next +* page. +*/ +if (unlikely (page->mapping != inode->i_mapping)) { +unlock_page (page); +continue ; + } + +if (!page_has_buffers (page)) { +unlock_page (page); +continue ; + } + + found =xfs_lookup_buffer_offset (page, &b_offset, type); +if (found) { +/* +* The found offset may be less than the start +* point to search if this is the first time to +* come here. +*/ + *offset =max_t (loff_t, startoff, b_offset); + goto out; + } + +/* +* We either searching data but nothing was found, or +* searching hole but found a data buffer. In either +* case, probably the next page contains the desired +* things, update the last offset to it so. +*/ + lastoff =page_offset (page) +PAGE_SIZE ; +unlock_page (page); + } + +/* +* The number of returned pages less than our desired, search +* done. In this case, nothing was found for searching data, +* but we found a hole behind the last offset. +*/ +if (nr_pages < want) { +if (type ==HOLE_OFF ) { + *offset = lastoff; + found = true; + } +break ; + } + + index = pvec.pages[i -1 ]->index +1 ; +pagevec_release (&pvec); + }while (index < end); + +out: +pagevec_release (&pvec); +return found; +} + STATIC loff_t xfs_seek_data( struct file *file,
Signature · 6 lines
1.7.4.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 11 lines · Jeff Liu
Introduce helpers to probe data or hole offset from page cache for unwritten extents.
--- fs/xfs/xfs_file.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 213 insertions(+), 0 deletions(-)fs/xfs/xfs_file.c +0
-0
fs/xfs/xfs_file.c line 42
Quoted 7 lines
fs/xfs/xfs_file.c +4
-0@@ -36,6 +36,7 @@ +* keep the offset argument unchanged. +*/ +STATIC bool +xfs_lookup_buffer_offset(
I am fine with this helper routine.
Quoted 68 lines
fs/xfs/xfs_file.c +68
-0+ +/* +* This routine is called to find out and return a data or hole offset +* from the page cache for unwritten extents according to the desired +* type for xfs_seek_data() or xfs_seek_hole(). +* +* The argument offset is used to tell where we start to search from the +* page cache. Map is used to figure out the end points of the range to +* lookup pages. +* +* Return true if the desired type of offset was found, and the argument +* offset is filled with that address. Otherwise, return false and keep +* offset unchanged. +*/ +STATIC bool +xfs_find_get_desired_pgoff ( +struct inode *inode, +struct xfs_bmbt_irec *map, +unsigned int type, +loff_t *offset) +{ +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +struct pagevec pvec; +pgoff_t index; +pgoff_t end; +loff_t endoff; +loff_t startoff = *offset; +loff_t lastoff = startoff; +bool found = false; + +pagevec_init (&pvec,0 ); + + index = startoff>>PAGE_CACHE_SHIFT ; + endoff =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount); + end = endoff>>PAGE_CACHE_SHIFT ; + do { + int want; + unsigned nr_pages; + unsigned int i; + + want = min_t(pgoff_t, end - index, (pgoff_t)PAGEVEC_SIZE); + nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index, + want); + /* + * No page mapped into given range. If we are searching holes + * and if this is the first time we got into the loop, it means + * that the given offset is landed in a hole, return it. + * + * If we have already stepped through some block buffers to find + * holes but they all contains data. In this case, the last + * offset is already updated and pointed to the end of the last + * mapped page, if it does not reach the endpoint to search, + * that means there should be a hole between them. + */ + if (nr_pages == 0) { + /* Data search found nothing */ + if (type == DATA_OFF) + break; + + ASSERT(type == HOLE_OFF); + if (lastoff == startoff || lastoff< endoff) { + found = true; + *offset = lastoff; + } + break; + } +
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1103
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+/* +* At lease we found one page. If this is the first time we +* step into the loop, and if the first page index offset is +* greater than the given search offset, a hole was found. +*/ +if (type ==HOLE_OFF && lastoff == startoff&& + lastoff<page_offset (pvec.pages[0 ])) { + found = true; +break ; + }
I played with the code and appreciate the trickiness of the startoff. I this can be refined a bit more. see below.
Quoted 8 lines
fs/xfs/xfs_file.c +8
-0+ +for (i =0 ; i< nr_pages; i++) { +struct page *page = pvec.pages[i]; +loff_t b_offset; + +/* +* Page index is out of range, searching done. +* If the current offset is not reaches the end
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1121
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+* of the specified search range, there should +* be a hole between them. +*/ +if (page->index> end) { +if (type ==HOLE_OFF && lastoff< endoff) { + *offset = lastoff; + found = true; + } + goto out; + }
Before we take the lock, we can check to see if the page starts later than expected (a hole). The pre-loop start check can be removed and something like the follow added:
/* Skip over this page if it precedes the start offset */
if (page_offset(page) + PAGE_SIZE < startoff)
continue;
if (type == HOLE_OFF && page_offset(page) >
lastoff) {
*offset = lastoff;
found = true;
goto out;
}My experiment passes your newest, more extensive tests. I can give you the complete experiment patch if you want.
Quoted 6 lines
fs/xfs/xfs_file.c +6
-0+ +lock_page (page); +/* +* Page truncated or invalidated(page->mapping == NULL). +* We can freely skip it and proceed to check the next +* page.
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1137
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+*/ +if (unlikely (page->mapping != inode->i_mapping)) { +unlock_page (page); +continue ; + } + +if (!page_has_buffers (page)) { +unlock_page (page); +continue ; + }
Would this be a hole condition? I suppose the next iteration of the loop will figure this out.
Quoted 37 lines
fs/xfs/xfs_file.c +37
-0+ + found =xfs_lookup_buffer_offset (page,&b_offset, type); +if (found) { +/* +* The found offset may be less than the start +* point to search if this is the first time to +* come here. +*/ + *offset =max_t (loff_t, startoff, b_offset); + goto out; + } + +/* +* We either searching data but nothing was found, or +* searching hole but found a data buffer. In either +* case, probably the next page contains the desired +* things, update the last offset to it so. +*/ + lastoff =page_offset (page) +PAGE_SIZE ; +unlock_page (page); + } + +/* +* The number of returned pages less than our desired, search +* done. In this case, nothing was found for searching data, +* but we found a hole behind the last offset. +*/ +if (nr_pages< want) { +if (type ==HOLE_OFF ) { + *offset = lastoff; + found = true; + } +break ; + } + + index = pvec.pages[i -1 ]->index +1 ; +pagevec_release (&pvec);
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1184
Quoted 10 lines
fs/xfs/xfs_file.c +7
-0+ }while (index< end); + +out: +pagevec_release (&pvec); +return found; +} + STATIC loff_t xfs_seek_data( struct file *file,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
On Thu, Aug 09, 2012 at 09:41:05AM -0500, Mark Tinguely wrote:
.....
Quoted 29 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1103
Quoted 11 lines
fs/xfs/xfs_file.c +11
-0+ +/* +* At lease we found one page. If this is the first time we +* step into the loop, and if the first page index offset is +* greater than the given search offset, a hole was found. +*/ +if (type ==HOLE_OFF && lastoff == startoff&& + lastoff<page_offset (pvec.pages[0 ])) { + found = true; +break ; + }I played with the code and appreciate the trickiness of the startoff. I this can be refined a bit more. see below.
Quoted 14 lines
fs/xfs/xfs_file.c +14
-0+ +for (i =0 ; i< nr_pages; i++) { +struct page *page = pvec.pages[i]; +loff_t b_offset; + +/* +* Page index is out of range, searching done. +* If the current offset is not reaches the end +* of the specified search range, there should +* be a hole between them. +*/ +if (page->index> end) { +if (type ==HOLE_OFF && lastoff< endoff) { + *offset = lastoff;
Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1121
Quoted 4 lines
fs/xfs/xfs_file.c +4
-0+ found = true; + } + goto out; + }Before we take the lock, we can check to see if the page starts later than expected (a hole). The pre-loop start check can be removed and something like the follow added:
Really, we don't need to be that tricky or smart. This is not a performance critical operation, so we don't need to optimise away page locks, especially when the risk of getting it wrong is compromising data integrity...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 146 lines · Mark Tinguely
Quoted 13 lines · Jeff Liu
Introduce helpers to probe data or hole offset from page cache for unwritten extents.
--- fs/xfs/xfs_file.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 213 insertions(+), 0 deletions(-)fs/xfs/xfs_file.c +0
-0fs/xfs/xfs_file.c line 42
Quoted 7 lines
fs/xfs/xfs_file.c +4
-0@@ -36,6 +36,7 @@ +* keep the offset argument unchanged. +*/ +STATIC bool +xfs_lookup_buffer_offset(I am fine with this helper routine.
Quoted 73 lines
fs/xfs/xfs_file.c +7
-0+ +/* +* This routine is called to find out and return a data or hole offset +* from the page cache for unwritten extents according to the desired +* type for xfs_seek_data() or xfs_seek_hole(). +* +* The argument offset is used to tell where we start to search from the
+ * page cache. Map is used to figure out the end points of the range to
+ * lookup pages.
+ *
+ * Return true if the desired type of offset was found, and the argument
+ * offset is filled with that address. Otherwise, return false and keep
+ * offset unchanged.
+ */fs/xfs/xfs_file.c +36
-0+STATIC bool +xfs_find_get_desired_pgoff ( +struct inode *inode, +struct xfs_bmbt_irec *map, +unsigned int type, +loff_t *offset) +{ +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +struct pagevec pvec; +pgoff_t index; +pgoff_t end; +loff_t endoff; +loff_t startoff = *offset; +loff_t lastoff = startoff; +bool found = false; + +pagevec_init (&pvec,0 ); + + index = startoff>>PAGE_CACHE_SHIFT ; + endoff =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount); + end = endoff>>PAGE_CACHE_SHIFT ; + do { + int want; + unsigned nr_pages; + unsigned int i; + + want = min_t(pgoff_t, end - index, (pgoff_t)PAGEVEC_SIZE); + nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index, + want); + /* + * No page mapped into given range. If we are searching holes + * and if this is the first time we got into the loop, it means + * that the given offset is landed in a hole, return it. + * + * If we have already stepped through some block buffers tofind
fs/xfs/xfs_file.c +18
-0+* holes but they all contains data. In this case, the last +* offset is already updated and pointed to the end of the last +* mapped page, if it does not reach the endpoint to search, +* that means there should be a hole between them. +*/ +if (nr_pages ==0 ) { +/* Data search found nothing */ +if (type ==DATA_OFF ) +break ; + +ASSERT (type ==HOLE_OFF ); +if (lastoff == startoff || lastoff< endoff) { + found = true; + *offset = lastoff; + } +break ; + } +fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1103
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+/* +* At lease we found one page. If this is the first time we +* step into the loop, and if the first page index offset is +* greater than the given search offset, a hole was found. +*/ +if (type ==HOLE_OFF && lastoff == startoff&& + lastoff<page_offset (pvec.pages[0 ])) { + found = true; +break ; + }I played with the code and appreciate the trickiness of the startoff. I this can be refined a bit more. see below.
Quoted 8 lines
fs/xfs/xfs_file.c +8
-0+ +for (i =0 ; i< nr_pages; i++) { +struct page *page = pvec.pages[i]; +loff_t b_offset; + +/* +* Page index is out of range, searching done. +* If the current offset is not reaches the end fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1121
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+* of the specified search range, there should +* be a hole between them. +*/ +if (page->index> end) { +if (type ==HOLE_OFF && lastoff< endoff) { + *offset = lastoff; + found = true; + } + goto out; + }Before we take the lock, we can check to see if the page starts later than expected (a hole). The pre-loop start check can be removed and something like the follow added:
/* Skip over this page if it precedes the start offset */ if (page_offset(page) + PAGE_SIZE < startoff) continue; if (type == HOLE_OFF && page_offset(page) > lastoff) { *offset = lastoff; found = true; goto out; }
Quoted 3 lines
My experiment passes your newest, more extensive tests. I can give you the
complete experiment patch if you want.
It's better to show me your code, this really optimize the logic a bit than me.
Quoted 13 lines
Quoted 13 lines
fs/xfs/xfs_file.c +11
-0+ +lock_page (page); +/* +* Page truncated or invalidated(page->mapping == NULL). +* We can freely skip it and proceed to check the next +* page. +*/ +if (unlikely (page->mapping != inode->i_mapping)) { +unlock_page (page); +continue ; + }
Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1137
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+ +if (!page_has_buffers (page)) { +unlock_page (page); +continue ; + }Would this be a hole condition? I suppose the next iteration of the loop will figure this out.
This is a fuzzy point to me. If there is no buffer heads associated with a page, can we treat it as an invalid page like the mapping check up above?
Thanks,
-Jeff
Quoted 45 lines
Quoted 45 lines
fs/xfs/xfs_file.c +44
-0+ + found =xfs_lookup_buffer_offset (page,&b_offset, type); +if (found) { +/* +* The found offset may be less than the start +* point to search if this is the first time to +* come here. +*/ + *offset =max_t (loff_t, startoff, b_offset); + goto out; + } + +/* +* We either searching data but nothing was found, or +* searching hole but found a data buffer. In either +* case, probably the next page contains the desired +* things, update the last offset to it so. +*/ + lastoff =page_offset (page) +PAGE_SIZE ; +unlock_page (page); + } + +/* +* The number of returned pages less than our desired, search +* done. In this case, nothing was found for searching data, +* but we found a hole behind the last offset. +*/ +if (nr_pages< want) { +if (type ==HOLE_OFF ) { + *offset = lastoff; + found = true; + } +break ; + } + + index = pvec.pages[i -1 ]->index +1 ; +pagevec_release (&pvec); + }while (index< end); + +out: +pagevec_release (&pvec); +return found; +} +
Quoted 8 lines
Quoted 3 lines
STATIC
loff_t
xfs_seek_data(
struct file *file,_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 167 lines · Jie Liu
Quoted 142 lines · Mark Tinguely
Quoted 13 lines · Jeff Liu
Introduce helpers to probe data or hole offset from page cache for unwritten extents.
--- fs/xfs/xfs_file.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 213 insertions(+), 0 deletions(-)fs/xfs/xfs_file.c +0
-0fs/xfs/xfs_file.c line 41
Quoted 6 lines
fs/xfs/xfs_file.c +4
-0@@ -36,6 +36,7 @@ +* keep the offset argument unchanged. +*/ +STATIC bool +xfs_lookup_buffer_offset(I am fine with this helper routine.
Quoted 73 lines
fs/xfs/xfs_file.c +7
-0+ +/* +* This routine is called to find out and return a data or hole offset +* from the page cache for unwritten extents according to the desired +* type for xfs_seek_data() or xfs_seek_hole(). +* +* The argument offset is used to tell where we start to search from the
+ * page cache. Map is used to figure out the end points of the range to
+ * lookup pages.
+ *
+ * Return true if the desired type of offset was found, and the argument
+ * offset is filled with that address. Otherwise, return false and keep
+ * offset unchanged.
+ */fs/xfs/xfs_file.c +36
-0+STATIC bool +xfs_find_get_desired_pgoff ( +struct inode *inode, +struct xfs_bmbt_irec *map, +unsigned int type, +loff_t *offset) +{ +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +struct pagevec pvec; +pgoff_t index; +pgoff_t end; +loff_t endoff; +loff_t startoff = *offset; +loff_t lastoff = startoff; +bool found = false; + +pagevec_init (&pvec,0 ); + + index = startoff>>PAGE_CACHE_SHIFT ; + endoff =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount); + end = endoff>>PAGE_CACHE_SHIFT ; + do { + int want; + unsigned nr_pages; + unsigned int i; + + want = min_t(pgoff_t, end - index, (pgoff_t)PAGEVEC_SIZE); + nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index, + want); + /* + * No page mapped into given range. If we are searching holes + * and if this is the first time we got into the loop, it means + * that the given offset is landed in a hole, return it. + * + * If we have already stepped through some block buffers tofind
fs/xfs/xfs_file.c +18
-0+* holes but they all contains data. In this case, the last +* offset is already updated and pointed to the end of the last +* mapped page, if it does not reach the endpoint to search, +* that means there should be a hole between them. +*/ +if (nr_pages ==0 ) { +/* Data search found nothing */ +if (type ==DATA_OFF ) +break ; + +ASSERT (type ==HOLE_OFF ); +if (lastoff == startoff || lastoff< endoff) { + found = true; + *offset = lastoff; + } +break ; + } +fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1103
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+/* +* At lease we found one page. If this is the first time we +* step into the loop, and if the first page index offset is +* greater than the given search offset, a hole was found. +*/ +if (type ==HOLE_OFF && lastoff == startoff&& + lastoff<page_offset (pvec.pages[0 ])) { + found = true; +break ; + }I played with the code and appreciate the trickiness of the startoff. I this can be refined a bit more. see below.
Quoted 8 lines
fs/xfs/xfs_file.c +8
-0+ +for (i =0 ; i< nr_pages; i++) { +struct page *page = pvec.pages[i]; +loff_t b_offset; + +/* +* Page index is out of range, searching done. +* If the current offset is not reaches the end fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1121
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+* of the specified search range, there should +* be a hole between them. +*/ +if (page->index> end) { +if (type ==HOLE_OFF && lastoff< endoff) { + *offset = lastoff; + found = true; + } + goto out; + }Before we take the lock, we can check to see if the page starts later than expected (a hole). The pre-loop start check can be removed and something like the follow added:
/* Skip over this page if it precedes the start offset */ if (page_offset(page) + PAGE_SIZE < startoff) continue; if (type == HOLE_OFF && page_offset(page) > lastoff) { *offset = lastoff; found = true; goto out; }Quoted 3 lines
My experiment passes your newest, more extensive tests. I can give you the
complete experiment patch if you want.It's better to show me your code, this really optimize the logic a bit than me.
Quoted 11 lines
Quoted 11 lines
fs/xfs/xfs_file.c +10
-0+ +lock_page (page); +/* +* Page truncated or invalidated(page->mapping == NULL). +* We can freely skip it and proceed to check the next +* page. +*/ +if (unlikely (page->mapping != inode->i_mapping)) { +unlock_page (page); +continue ;Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1137
Quoted 6 lines
fs/xfs/xfs_file.c +6
-0+ } + +if (!page_has_buffers (page)) { +unlock_page (page); +continue ; + }Would this be a hole condition? I suppose the next iteration of the loop will figure this out.
Quoted 3 lines
This is a fuzzy point to me. If there is no buffer heads associated with a page, can we treat it as an invalid page like the mapping check up above?
In other words, an unwritten extent have real block numbers on disk. Hence a valid page mapping to that exent should have BHs associated with, Am I right?
Thanks,
-Jeff
Quoted 43 lines
Quoted 43 lines
Quoted 43 lines
fs/xfs/xfs_file.c +43
-0+ + found =xfs_lookup_buffer_offset (page,&b_offset, type); +if (found) { +/* +* The found offset may be less than the start +* point to search if this is the first time to +* come here. +*/ + *offset =max_t (loff_t, startoff, b_offset); + goto out; + } + +/* +* We either searching data but nothing was found, or +* searching hole but found a data buffer. In either +* case, probably the next page contains the desired +* things, update the last offset to it so. +*/ + lastoff =page_offset (page) +PAGE_SIZE ; +unlock_page (page); + } + +/* +* The number of returned pages less than our desired, search +* done. In this case, nothing was found for searching data, +* but we found a hole behind the last offset. +*/ +if (nr_pages< want) { +if (type ==HOLE_OFF ) { + *offset = lastoff; + found = true; + } +break ; + } + + index = pvec.pages[i -1 ]->index +1 ; +pagevec_release (&pvec); + }while (index< end); + +out: +pagevec_release (&pvec); +return found; +}
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
+
STATICloff_t
xfs_seek_data(
struct file *file,_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 1 line · Jeff Liu
Introduce helpers to probe data or hole offset from page cache for unwritten extents.
.....
Quoted 4 lines
+ * Lookup the desired type of offset from the given page.
+ *
+ * On success, return true and the offset argument will point to the
+ * searched offset. Otherwise this function will return false and
I'm not sure what the "searched offset" is. Is it the start of the data or hole region that has been found? If so, it woul dbe clearer to write:
* On success, return true and the offset argument will point to the
* start of the region that was found. Otherwise this function will return false and
Quoted 27 lines
+ * keep the offset argument unchanged.
+ */fs/xfs/xfs_file.c +24
-0+STATIC bool +xfs_lookup_buffer_offset ( +struct page *page, +loff_t *offset, +unsigned int type) +{ +loff_t lastoff =page_offset (page); +bool found = false; +struct buffer_head *bh, *head; + + bh = head =page_buffers (page); + do { + /* + * 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)) { + if (type == DATA_OFF)
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1019
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ found = true; + }else { +if (type ==HOLE_OFF ) + found = true; + } + +if (found) { + *offset = lastoff; +unlock_page (page); +break ;
You don't lock the page in this function, so you shouldn't unlock it, either. Let the caller handle that.
Quoted 117 lines
fs/xfs/xfs_file.c +117
-0+ } + lastoff += bh->b_size; + }while ((bh = bh->b_this_page) != head); + +return found; +} + +/* +* This routine is called to find out and return a data or hole offset +* from the page cache for unwritten extents according to the desired +* type for xfs_seek_data() or xfs_seek_hole(). +* +* The argument offset is used to tell where we start to search from the +* page cache. Map is used to figure out the end points of the range to +* lookup pages. +* +* Return true if the desired type of offset was found, and the argument +* offset is filled with that address. Otherwise, return false and keep +* offset unchanged. +*/ +STATIC bool +xfs_find_get_desired_pgoff ( +struct inode *inode, +struct xfs_bmbt_irec *map, +unsigned int type, +loff_t *offset) +{ +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +struct pagevec pvec; +pgoff_t index; +pgoff_t end; +loff_t endoff; +loff_t startoff = *offset; +loff_t lastoff = startoff; +bool found = false; + +pagevec_init (&pvec,0 ); + + index = startoff >>PAGE_CACHE_SHIFT ; + endoff =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount); + end = endoff >>PAGE_CACHE_SHIFT ; + do { + int want; + unsigned nr_pages; + unsigned int i; + + want = min_t(pgoff_t, end - index, (pgoff_t)PAGEVEC_SIZE); + nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index, + want); + /* + * No page mapped into given range. If we are searching holes + * and if this is the first time we got into the loop, it means + * that the given offset is landed in a hole, return it. + * + * If we have already stepped through some block buffers to find + * holes but they all contains data. In this case, the last + * offset is already updated and pointed to the end of the last + * mapped page, if it does not reach the endpoint to search, + * that means there should be a hole between them. + */ + if (nr_pages == 0) { + /* Data search found nothing */ + if (type == DATA_OFF) + break; + + ASSERT(type == HOLE_OFF); + if (lastoff == startoff || lastoff < endoff) { + found = true; + *offset = lastoff; + } + break; + } + + /* + * At lease we found one page. If this is the first time we + * step into the loop, and if the first page index offset is + * greater than the given search offset, a hole was found. + */ + if (type == HOLE_OFF && lastoff == startoff && + lastoff < page_offset(pvec.pages[0])) { + found = true; + break; + } + + for (i = 0; i < nr_pages; i++) { + struct page *page = pvec.pages[i]; + loff_t b_offset; + + /* + * Page index is out of range, searching done. + * If the current offset is not reaches the end + * of the specified search range, there should + * be a hole between them. + */ + if (page->index > end) { + if (type == HOLE_OFF && lastoff < endoff) { + *offset = lastoff; + found = true; + } + goto out; + } + + lock_page(page); + /* + * Page truncated or invalidated(page->mapping == NULL). + * We can freely skip it and proceed to check the next + * page. + */ + if (unlikely(page->mapping != inode->i_mapping)) { + unlock_page(page); + continue; + } + + if (!page_has_buffers(page)) { + unlock_page(page); + continue;
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1146
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ } + + found =xfs_lookup_buffer_offset (page, &b_offset, type); +if (found) { +/* +* The found offset may be less than the start +* point to search if this is the first time to +* come here. +*/ + *offset =max_t (loff_t, startoff, b_offset);
You should unlock the page here.
Quoted 17 lines
fs/xfs/xfs_file.c +17
-0+ goto out; + } + +/* +* We either searching data but nothing was found, or +* searching hole but found a data buffer. In either +* case, probably the next page contains the desired +* things, update the last offset to it so. +*/ + lastoff =page_offset (page) +PAGE_SIZE ; +unlock_page (page); + } + +/* +* The number of returned pages less than our desired, search +* done. In this case, nothing was found for searching data, +* but we found a hole behind the last offset.
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1173
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+*/ +if (nr_pages < want) { +if (type ==HOLE_OFF ) { + *offset = lastoff; + found = true; + } +break ; + } + + index = pvec.pages[i -1 ]->index +1 ;
You have to sample the index while the page is locked, otherwise it can be invalid. Hence you have to update index every time through the above page array loop.
Other than that, it looks fine.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 9 lines · Dave Chinner
Quoted 1 line · Jeff Liu
Introduce helpers to probe data or hole offset from page cache for unwritten extents.
.....
Quoted 4 lines
+ * Lookup the desired type of offset from the given page.
+ *
+ * On success, return true and the offset argument will point to the
+ * searched offset. Otherwise this function will return false andI'm not sure what the "searched offset" is. Is it the start of the data or hole region that has been found?
Yes, it is.
Quoted 5 lines
If so, it woul dbe clearer
to write:* On success, return true and the offset argument will point to the
* start of the region that was found. Otherwise this function will return false and
I'll fix the comments for it.
Quoted 160 lines
Quoted 27 lines
+ * keep the offset argument unchanged.
+ */fs/xfs/xfs_file.c +24
-0+STATIC bool +xfs_lookup_buffer_offset ( +struct page *page, +loff_t *offset, +unsigned int type) +{ +loff_t lastoff =page_offset (page); +bool found = false; +struct buffer_head *bh, *head; + + bh = head =page_buffers (page); + do { + /* + * 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)) { + if (type == DATA_OFF)fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1019
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ found = true; + }else { +if (type ==HOLE_OFF ) + found = true; + } + +if (found) { + *offset = lastoff; +unlock_page (page); +break ;You don't lock the page in this function, so you shouldn't unlock it, either. Let the caller handle that.
Quoted 120 lines
fs/xfs/xfs_file.c +120
-0+ } + lastoff += bh->b_size; + }while ((bh = bh->b_this_page) != head); + +return found; +} + +/* +* This routine is called to find out and return a data or hole offset +* from the page cache for unwritten extents according to the desired +* type for xfs_seek_data() or xfs_seek_hole(). +* +* The argument offset is used to tell where we start to search from the +* page cache. Map is used to figure out the end points of the range to +* lookup pages. +* +* Return true if the desired type of offset was found, and the argument +* offset is filled with that address. Otherwise, return false and keep +* offset unchanged. +*/ +STATIC bool +xfs_find_get_desired_pgoff ( +struct inode *inode, +struct xfs_bmbt_irec *map, +unsigned int type, +loff_t *offset) +{ +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +struct pagevec pvec; +pgoff_t index; +pgoff_t end; +loff_t endoff; +loff_t startoff = *offset; +loff_t lastoff = startoff; +bool found = false; + +pagevec_init (&pvec,0 ); + + index = startoff >>PAGE_CACHE_SHIFT ; + endoff =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount); + end = endoff >>PAGE_CACHE_SHIFT ; + do { + int want; + unsigned nr_pages; + unsigned int i; + + want = min_t(pgoff_t, end - index, (pgoff_t)PAGEVEC_SIZE); + nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index, + want); + /* + * No page mapped into given range. If we are searching holes + * and if this is the first time we got into the loop, it means + * that the given offset is landed in a hole, return it. + * + * If we have already stepped through some block buffers to find + * holes but they all contains data. In this case, the last + * offset is already updated and pointed to the end of the last + * mapped page, if it does not reach the endpoint to search, + * that means there should be a hole between them. + */ + if (nr_pages == 0) { + /* Data search found nothing */ + if (type == DATA_OFF) + break; + + ASSERT(type == HOLE_OFF); + if (lastoff == startoff || lastoff < endoff) { + found = true; + *offset = lastoff; + } + break; + } + + /* + * At lease we found one page. If this is the first time we + * step into the loop, and if the first page index offset is + * greater than the given search offset, a hole was found. + */ + if (type == HOLE_OFF && lastoff == startoff && + lastoff < page_offset(pvec.pages[0])) { + found = true; + break; + } + + for (i = 0; i < nr_pages; i++) { + struct page *page = pvec.pages[i]; + loff_t b_offset; + + /* + * Page index is out of range, searching done. + * If the current offset is not reaches the end + * of the specified search range, there should + * be a hole between them. + */ + if (page->index > end) { + if (type == HOLE_OFF && lastoff < endoff) { + *offset = lastoff; + found = true; + } + goto out; + } + + lock_page(page); + /* + * Page truncated or invalidated(page->mapping == NULL). + * We can freely skip it and proceed to check the next + * page. + */ + if (unlikely(page->mapping != inode->i_mapping)) { + unlock_page(page); + continue; + } + + if (!page_has_buffers(page)) { + unlock_page(page); + continue; + } + + found = xfs_lookup_buffer_offset(page, &b_offset, type);
Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1146
Quoted 7 lines
fs/xfs/xfs_file.c +7
-0+if (found) { +/* +* The found offset may be less than the start +* point to search if this is the first time to +* come here. +*/ + *offset =max_t (loff_t, startoff, b_offset);You should unlock the page here.
exactly.
Quoted 23 lines
Quoted 23 lines
fs/xfs/xfs_file.c +22
-0+ goto out; + } + +/* +* We either searching data but nothing was found, or +* searching hole but found a data buffer. In either +* case, probably the next page contains the desired +* things, update the last offset to it so. +*/ + lastoff =page_offset (page) +PAGE_SIZE ; +unlock_page (page); + } + +/* +* The number of returned pages less than our desired, search +* done. In this case, nothing was found for searching data, +* but we found a hole behind the last offset. +*/ +if (nr_pages < want) { +if (type ==HOLE_OFF ) { + *offset = lastoff; + found = true;
Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1173
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+ } +break ; + } + + index = pvec.pages[i -1 ]->index +1 ;You have to sample the index while the page is locked, otherwise it can be invalid. Hence you have to update index every time through the above page array loop.
I'll take care of it, thanks!
-Jeff
Quoted 6 lines
Other than that, it looks fine.
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs