Introduce SEEK_DATA/SEEK_HOLE to XFS V3
PATCH 1/1 Introduce SEEK_DATA/SEEK_HOLE to XFS V3
Hello,
Now the V3 was coming out.
Major changes:
==============
1. Isolate SEEK_DATA/SEEK_HOLE to individual functions, call them directly from xfs_llseek().
2. Probing page cache for unwritten extents.
Tests:
======
1. General tests I have mentioned before.
2. Cover look up DIRTY pages through fallocate(2).
The issue is I have not yet successfully worked out a test case can cover look up WRITEBACK pages easily, I'll continue to try out some other methods in this case.
In the meantime, I'd like to post the revised patch for your guys review, to make sure that there is no obvious mistake in my current implementation, especially for the page cache look up approach.
Also, I took care of the function parameters and variables in TAB-align code style this time, for variables defined at conditions code blocks(i.e, "{}"), I also adhere to this rule, but not sure if it is properly, since I found some code snippets in xfs_file.c does not defined in this way.
Thank you!
-Jeff
--- fs/xfs/xfs_file.c | 308 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 307 insertions(+), 1 deletions(-)
fs/xfs/xfs_file.c +307 -1
@@ -38,6 +38,7 @@#include <linux/dcache.h> #include <linux/falloc.h> +#include <linux/pagevec.h> static const struct vm_operations_struct xfs_file_vm_ops;
@@ -1141,8 +1142,313 @@ xfs_vm_page_mkwrite(return block_page_mkwrite (vma, vmf, xfs_get_blocks); } +/* +* Try to find out the data buffer offset in page cache for unwritten +* extents. Firstly, try to probe the DIRTY pages in current extent range, +* and iterate each page to lookup all theirs data buffers, if a buffer +* head status is unwritten, return its offset. If there is no DIRTY pages +* found or lookup done, we need to lookup the WRITEBACK pages again and +* perform the same operation as previously to avoid data loss. +*/ +STATIC loff_t +xfs_probe_unwritten_buffer ( +struct inode *inode, +struct xfs_bmbt_irec *map, +int *found) +{ +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 offset =0 ; +int tag =PAGECACHE_TAG_DIRTY ; + +pagevec_init (&pvec,0 ); + +probe_writeback_pages: + index =XFS_FSB_TO_B (mp, map->br_startoff) >>PAGE_CACHE_SHIFT ; + end =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount) + >>PAGE_CACHE_SHIFT ; + +do { +unsigned nr_pages; +unsigned int i; +int want =min_t (pgoff_t, end - index, +PAGEVEC_SIZE -1 ) +1 ; + nr_pages =pagevec_lookup_tag (&pvec, inode->i_mapping, + &index, tag, want); +if (nr_pages ==0 ) { +/* +* No dirty pages returns for this extent, try +* to lookup the writeback pages again. +* FIXME: If this is the first time for probing +* DIRTY pages but nothing returned, we need to +* search the WRITEBACK pages from the extent +* beginning offset, but if we have found out +* some DIRTY pages before, maybe we should +* continue to probe the WRITEBACK pages from +* the current page index rather than beginning? +*/ +if (tag ==PAGECACHE_TAG_DIRTY ) { + tag =PAGECACHE_TAG_WRITEBACK ; + goto probe_writeback_pages; + } +break ; + } + +for (i =0 ; i < nr_pages; i++) { +struct page *page = pvec.pages[i]; +struct buffer_head *bh; +struct buffer_head *head; +xfs_fileoff_t last; + +if (!page_has_buffers (page)) + goto out; + + last =XFS_B_TO_FSBT (mp, + page->index <<PAGE_CACHE_SHIFT ); + bh = head =page_buffers (page); +do { +/* +* In XFS, if an extent in XFS_EXT_UNWRITTEN +* state, that means the disk blocks were +* already mapped for it, but the data is +* still lived at page caches. For buffers +* resides at DIRTY pages, their BH state +* should be in (dirty && mapped && unwritten +* && uptodate) status. For buffers resides +* at WRITEBACK pages, their BH state should +* be in (mapped && unwritten && uptodate) +* status. So we only need to check unwritten +* buffer status here. +*/ +if (buffer_unwritten (bh)) { + offset =XFS_FSB_TO_B (mp, last); + *found =1 ; + goto out; + } + last++; + }while ((bh = bh->b_this_page) != head); + } + + index = pvec.pages[i]->index +1 ; +pagevec_release (&pvec); + }while (index < end); + +out: +pagevec_release (&pvec); +return offset; +} + +STATIC loff_t +xfs_seek_data ( +struct file *file, +loff_t start, +u32 type) +{ +struct inode *inode = file->f_mapping->host; +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +xfs_fsize_t isize =i_size_read (inode); +loff_t offset =0 ; +struct xfs_ifork *ifp; +xfs_fileoff_t fsbno; +xfs_filblks_t len; +int lock; +int error; + +if (start >= isize) +return -ENXIO ; + + lock =xfs_ilock_map_shared (ip); + + fsbno =XFS_B_TO_FSBT (mp, start); + ifp =XFS_IFORK_PTR (ip,XFS_DATA_FORK ); + len =XFS_B_TO_FSB (mp, isize); + +for (;;) { +struct xfs_bmbt_irec map[2 ]; +int nmap =2 ; +int found =0 ; +loff_t seekoff; + + error =xfs_bmapi_read (ip, fsbno, len - fsbno, map, &nmap, +XFS_BMAPI_ENTIRE ); +if (error) + goto out_lock; + +/* No extents at given offset, must be beyond EOF */ +if (!nmap) { + error =ENXIO ; + goto out_lock; + } + + seekoff =XFS_FSB_TO_B (mp, fsbno); +/* +* Landed in a hole, skip to check the next extent. +* If the next extent landed in an in-memory data extent, +* or it is a normal extent, its fine to return. +* If the next extent landed in a hole extent, calculate +* the start file system block number for next bmapi read. +* If the next extent landed in an unwritten extent, we +* need to probe the page cache to find out the data buffer +* offset, if nothing found, treat it as a hole extent too. +*/ +if (map[0 ].br_startblock ==HOLESTARTBLOCK ) { +if (map[1 ].br_startblock ==HOLESTARTBLOCK ) { + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount; + }else if (map[1 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =xfs_probe_unwritten_buffer (inode, + &map[1 ], + &found); +if (found) { + offset =max_t (loff_t, seekoff, offset); +break ; + } +/* +* No data buffer found in pagecache, treate it +* as a hole. +*/ + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount; + }else { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + } + } + +/* +* Landed in an unwritten extent, try to find out the data +* buffer offset from page cache firstly. if nothing was +* found, treat it as a hole, and skip to check the next +* extent, something just like above. +*/ +if (map[0 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =xfs_probe_unwritten_buffer (inode, &map[0 ], + &found); +if (found) { + offset =max_t (loff_t, seekoff, offset); +break ; + } +if (map[1 ].br_startblock ==HOLESTARTBLOCK ) { + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount; + }else if (map[1 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =xfs_probe_unwritten_buffer (inode, + &map[1 ], + &found); +if (found) { + offset =max_t (loff_t, seekoff, + offset); +break ; + } + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount; + }else { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + } + } + +/* Landed in a delay allocated extent or a read data extent */ +if (map[0 ].br_startblock ==DELAYSTARTBLOCK || + map[0 ].br_state ==XFS_EXT_NORM ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ; + } + +/* return ENXIO if beyond eof */ +if (XFS_FSB_TO_B (mp, fsbno) > isize) { + error =ENXIO ; + goto out_lock; + } + } + +if (offset < start) + offset = start; + +if (offset != file->f_pos) + file->f_pos = offset; + +out_lock: +xfs_iunlock_map_shared (ip, lock); + +if (error) +return -error; + +return offset; +} + +STATIC loff_t +xfs_seek_hole ( +struct file *file, +loff_t start, +u32 type) +{ +struct inode *inode = file->f_mapping->host; +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +xfs_fsize_t isize =i_size_read (inode); +xfs_fileoff_t fsbno; +loff_t holeoff; +loff_t offset =0 ; +int lock; +int error; + +if (start >= isize) +return -ENXIO ; + + lock =xfs_ilock_map_shared (ip); + + fsbno =XFS_B_TO_FSBT (mp, start); + error =xfs_bmap_first_unused (NULL , ip,1 , &fsbno,XFS_DATA_FORK ); +if (error) + goto out_lock; + + holeoff =XFS_FSB_TO_B (mp, fsbno); +if (holeoff <= start) + offset = start; +else + offset =min_t (loff_t, holeoff, isize); + +if (offset != file->f_pos) + file->f_pos = offset; + +out_lock: +xfs_iunlock_map_shared (ip, lock); + +if (error) +return -error; +return offset; +} + +STATIC loff_t +xfs_file_llseek ( +struct file *file, +loff_t offset, +int origin) +{ +switch (origin) { +case SEEK_END : +case SEEK_CUR : +case SEEK_SET : +return generic_file_llseek (file, offset, origin); +case SEEK_DATA : +return xfs_seek_data (file, offset, origin); +case SEEK_HOLE : +return xfs_seek_hole (file, offset, origin); +default : +return -EOPNOTSUPP ; + } +} +const struct file_operations xfs_file_operations = { - .llseek = generic_file_llseek, + .llseek = xfs_file_llseek, .read = do_sync_read, .write = do_sync_write, .aio_read = xfs_file_aio_read,
Signature · 6 lines
1.7.4.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Hi Jeff,
sorry for the delay getting back to this - last week has been very busy for me.
Quoted 4 lines
Tests:
======
1. General tests I have mentioned before.
2. Cover look up DIRTY pages through fallocate(2).
Can you send your actual test cases to the list? Preferably wired up to xfstests, and also verified against btrfs and ocfs2 which already have the feature.
Quoted 2 lines
The issue is I have not yet successfully worked out a test case can cover look up WRITEBACK pages easily,
It's probably fairly hard to hit, as you'd need a relatively slow device to hit it reliably. Doing a sync_file_range call with SYNC_FILE_RANGE_WRITE as the flags argument just before the lseek call probably is the best way to get it.
Quoted 42 lines
fs/xfs/xfs_file.c +42
-0+/* +* Try to find out the data buffer offset in page cache for unwritten +* extents. Firstly, try to probe the DIRTY pages in current extent range, +* and iterate each page to lookup all theirs data buffers, if a buffer +* head status is unwritten, return its offset. If there is no DIRTY pages +* found or lookup done, we need to lookup the WRITEBACK pages again and +* perform the same operation as previously to avoid data loss. +*/ +STATIC loff_t +xfs_probe_unwritten_buffer ( +struct inode *inode, +struct xfs_bmbt_irec *map, +int *found) +{ +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 offset =0 ; +int tag =PAGECACHE_TAG_DIRTY ; + +pagevec_init (&pvec,0 ); + +probe_writeback_pages: + index =XFS_FSB_TO_B (mp, map->br_startoff) >>PAGE_CACHE_SHIFT ; + end =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount) + >>PAGE_CACHE_SHIFT ; + + do { + unsigned nr_pages; + unsigned int i; + int want = min_t(pgoff_t, end - index, + PAGEVEC_SIZE - 1) + 1; + nr_pages = pagevec_lookup_tag(&pvec, inode->i_mapping, + &index, tag, want); + if (nr_pages == 0) { + /* + * No dirty pages returns for this extent, try + * to lookup the writeback pages again. + * FIXME: If this is the first time for probing + * DIRTY pages but nothing returned, we need to
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1196
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+* search the WRITEBACK pages from the extent +* beginning offset, but if we have found out +* some DIRTY pages before, maybe we should +* continue to probe the WRITEBACK pages from +* the current page index rather than beginning? +*/ +if (tag ==PAGECACHE_TAG_DIRTY ) { + tag =PAGECACHE_TAG_WRITEBACK ; + goto probe_writeback_pages; + }
I don't think this is correct. Even if we have dirty pages we migt have writeback pages at an lower index. Given that the we can't look for multiple tags at the same time it seems like we need a normal pagecache lookup and iterate over all pages. Later we could optimize this by adding a multiple tag lookup helper, but let's get the code functional for now. Btw, did you look what btrfs and ocfs2 do here?
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1224
Quoted 12 lines
fs/xfs/xfs_file.c +12
-0+/* +* In XFS, if an extent in XFS_EXT_UNWRITTEN +* state, that means the disk blocks were +* already mapped for it, but the data is +* still lived at page caches. For buffers +* resides at DIRTY pages, their BH state +* should be in (dirty && mapped && unwritten +* && uptodate) status. For buffers resides +* at WRITEBACK pages, their BH state should +* be in (mapped && unwritten && uptodate) +* status. So we only need to check unwritten +* buffer status here.
Remove the "In XFS" - this is XFS code so that part is redudant.
XFS_EXT_UNWRITTEN do not need to have data at all, in fact they most likely don't. So I'd reword this to:
An extent in XFS_EXT_UNWRITTEN has disk blocks already mapped to it, but no data has been commiteed to them yet. If it has dirty data in the pagecache it can be identified by having BH_Unwritten set in each buffer.
Quoted 11 lines
fs/xfs/xfs_file.c +11
-0+STATIC loff_t +xfs_seek_data ( +struct file *file, +loff_t start, +u32 type) +{ +struct inode *inode = file->f_mapping->host; +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +xfs_fsize_t isize =i_size_read (inode); +loff_t offset =0 ;
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1264
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+struct xfs_ifork *ifp; +xfs_fileoff_t fsbno; +xfs_filblks_t len; +int lock; +int error; + +if (start >= isize) +return -ENXIO ; + + lock =xfs_ilock_map_shared (ip);
I'd move the check after acquiring the lock just to be sure.
Quoted 45 lines
fs/xfs/xfs_file.c +45
-0+ fsbno =XFS_B_TO_FSBT (mp, start); + ifp =XFS_IFORK_PTR (ip,XFS_DATA_FORK ); + len =XFS_B_TO_FSB (mp, isize); + +for (;;) { +struct xfs_bmbt_irec map[2 ]; +int nmap =2 ; +int found =0 ; +loff_t seekoff; + + error =xfs_bmapi_read (ip, fsbno, len - fsbno, map, &nmap, +XFS_BMAPI_ENTIRE ); +if (error) + goto out_lock; + +/* No extents at given offset, must be beyond EOF */ +if (!nmap) { + error =ENXIO ; + goto out_lock; + } + + seekoff =XFS_FSB_TO_B (mp, fsbno); +/* +* Landed in a hole, skip to check the next extent. +* If the next extent landed in an in-memory data extent, +* or it is a normal extent, its fine to return. +* If the next extent landed in a hole extent, calculate +* the start file system block number for next bmapi read. +* If the next extent landed in an unwritten extent, we +* need to probe the page cache to find out the data buffer +* offset, if nothing found, treat it as a hole extent too. +*/ +if (map[0 ].br_startblock ==HOLESTARTBLOCK ) { +if (map[1 ].br_startblock ==HOLESTARTBLOCK ) { + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount; + }else if (map[1 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =xfs_probe_unwritten_buffer (inode, + &map[1 ], + &found); +if (found) { + offset =max_t (loff_t, seekoff, offset); +break ; + } +/*
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1320
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+* No data buffer found in pagecache, treate it +* as a hole. +*/ + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount; + }else { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + }
It seems like the hole handling is the same for this case and what we handle below.
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1357
Quoted 1 line
fs/xfs/xfs_file.c +1
-0+/* Landed in a delay allocated extent or a read data extent */
s/read/real/
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+STATIC loff_t +xfs_seek_hole ( +struct file *file, +loff_t start, +u32 type) +{ +struct inode *inode = file->f_mapping->host; +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +xfs_fsize_t isize =i_size_read (inode);
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1406
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+xfs_fileoff_t fsbno; +loff_t holeoff; +loff_t offset =0 ; +int lock; +int error; + +if (start >= isize) +return -ENXIO ; + + lock =xfs_ilock_map_shared (ip);
I'd move the check after acquiring the lock just to be sure.
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1411
Quoted 3 lines
fs/xfs/xfs_file.c +3
-0+ error =xfs_bmap_first_unused (NULL , ip,1 , &fsbno,XFS_DATA_FORK ); +if (error) + goto out_lock;
Hmm - this misses the unwritten cases that we handle in SEEK_DATA. But if we want to handle it we probably can't use the simple xfs_bmap_first_unused call that Dave suggested but need to use the xfs_bmapi_read loop, too.
Sorry for not finding this earlier.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Hi Christoph,
Thanks for your feedback.
Quoted 5 lines · Christoph Hellwig
Hi Jeff,
sorry for the delay getting back to this - last week has been very busy for me.
Quoted 8 lines
Quoted 4 lines
Tests:
======
1. General tests I have mentioned before.
2. Cover look up DIRTY pages through fallocate(2).Can you send your actual test cases to the list? Preferably wired up to xfstests, and also verified against btrfs and ocfs2 which already have the feature.
I'll includes the test cases together in next post.
Quoted 3 lines
Quoted 3 lines
The issue is I have not yet successfully worked out a test case can cover look up WRITEBACK pages easily,
Yes! Finally, I got a few hits by tuning the dirty_background_bytes to a very little size, but it not a reasonable approach for our tests :(
Quoted 5 lines
It's probably fairly hard to hit, as you'd need a relatively slow device to hit it reliably. Doing a
sync_file_rangecall withSYNC_FILE_RANGE_WRITEas the flags argument just before the lseek call probably is the best way to get it.
Coool, I'm play with it now.
Quoted 52 lines
Quoted 52 lines
fs/xfs/xfs_file.c +51
-0+/* +* Try to find out the data buffer offset in page cache for unwritten +* extents. Firstly, try to probe the DIRTY pages in current extent range, +* and iterate each page to lookup all theirs data buffers, if a buffer +* head status is unwritten, return its offset. If there is no DIRTY pages +* found or lookup done, we need to lookup the WRITEBACK pages again and +* perform the same operation as previously to avoid data loss. +*/ +STATIC loff_t +xfs_probe_unwritten_buffer ( +struct inode *inode, +struct xfs_bmbt_irec *map, +int *found) +{ +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 offset =0 ; +int tag =PAGECACHE_TAG_DIRTY ; + +pagevec_init (&pvec,0 ); + +probe_writeback_pages: + index =XFS_FSB_TO_B (mp, map->br_startoff) >>PAGE_CACHE_SHIFT ; + end =XFS_FSB_TO_B (mp, map->br_startoff + map->br_blockcount) + >>PAGE_CACHE_SHIFT ; + + do { + unsigned nr_pages; + unsigned int i; + int want = min_t(pgoff_t, end - index, + PAGEVEC_SIZE - 1) + 1; + nr_pages = pagevec_lookup_tag(&pvec, inode->i_mapping, + &index, tag, want); + if (nr_pages == 0) { + /* + * No dirty pages returns for this extent, try + * to lookup the writeback pages again. + * FIXME: If this is the first time for probing + * DIRTY pages but nothing returned, we need to + * search the WRITEBACK pages from the extent + * beginning offset, but if we have found out + * some DIRTY pages before, maybe we should + * continue to probe the WRITEBACK pages from + * the current page index rather than beginning? + */ + if (tag == PAGECACHE_TAG_DIRTY) { + tag = PAGECACHE_TAG_WRITEBACK; + goto probe_writeback_pages;
Quoted 8 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ }I don't think this is correct. Even if we have dirty pages we migt have writeback pages at an lower index. Given that the we can't look for multiple tags at the same time it seems like we need a normal pagecache lookup and iterate over all pages. Later we could optimize this by adding a multiple tag lookup helper, but let's get the code functional for now.
The idea which I have mentioned is definitely wrong, we have to lookup writeback pages from the beginning offset again.
Quoted 1 line
Btw, did you look what btrfs and ocfs2 do here?
Looks they don't check page caches for unwritten extent, in other worlds, they treat it as data return to user.
Quoted 16 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1224
Quoted 13 lines
fs/xfs/xfs_file.c +12
-0+/* +* In XFS, if an extent in XFS_EXT_UNWRITTEN +* state, that means the disk blocks were +* already mapped for it, but the data is +* still lived at page caches. For buffers +* resides at DIRTY pages, their BH state +* should be in (dirty && mapped && unwritten +* && uptodate) status. For buffers resides +* at WRITEBACK pages, their BH state should +* be in (mapped && unwritten && uptodate) +* status. So we only need to check unwritten +* buffer status here. Remove the "In XFS" - this is XFS code so that part is redudant.
Quoted 7 lines
XFS_EXT_UNWRITTENdo not need to have data at all, in fact they most likely don't. So I'd reword this to:An extent in
XFS_EXT_UNWRITTENhas disk blocks already mapped to it, but no data has been commiteed to them yet. If it has dirty data in the pagecache it can be identified by havingBH_Unwrittenset in each buffer.
Thanks again!
Quoted 16 lines
Quoted 16 lines
fs/xfs/xfs_file.c +15
-0+STATIC loff_t +xfs_seek_data ( +struct file *file, +loff_t start, +u32 type) +{ +struct inode *inode = file->f_mapping->host; +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +xfs_fsize_t isize =i_size_read (inode); +loff_t offset =0 ; +struct xfs_ifork *ifp; +xfs_fileoff_t fsbno; +xfs_filblks_t len; +int lock;
Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1264
Quoted 6 lines
fs/xfs/xfs_file.c +6
-0+int error; + +if (start >= isize) +return -ENXIO ; + + lock =xfs_ilock_map_shared (ip);I'd move the check after acquiring the lock just to be sure.
yes, it should be present after
Quoted 51 lines
Quoted 51 lines
fs/xfs/xfs_file.c +50
-0+ fsbno =XFS_B_TO_FSBT (mp, start); + ifp =XFS_IFORK_PTR (ip,XFS_DATA_FORK ); + len =XFS_B_TO_FSB (mp, isize); + +for (;;) { +struct xfs_bmbt_irec map[2 ]; +int nmap =2 ; +int found =0 ; +loff_t seekoff; + + error =xfs_bmapi_read (ip, fsbno, len - fsbno, map, &nmap, +XFS_BMAPI_ENTIRE ); +if (error) + goto out_lock; + +/* No extents at given offset, must be beyond EOF */ +if (!nmap) { + error =ENXIO ; + goto out_lock; + } + + seekoff =XFS_FSB_TO_B (mp, fsbno); +/* +* Landed in a hole, skip to check the next extent. +* If the next extent landed in an in-memory data extent, +* or it is a normal extent, its fine to return. +* If the next extent landed in a hole extent, calculate +* the start file system block number for next bmapi read. +* If the next extent landed in an unwritten extent, we +* need to probe the page cache to find out the data buffer +* offset, if nothing found, treat it as a hole extent too. +*/ +if (map[0 ].br_startblock ==HOLESTARTBLOCK ) { +if (map[1 ].br_startblock ==HOLESTARTBLOCK ) { + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount; + }else if (map[1 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =xfs_probe_unwritten_buffer (inode, + &map[1 ], + &found); +if (found) { + offset =max_t (loff_t, seekoff, offset); +break ; + } +/* +* No data buffer found in pagecache, treate it +* as a hole. +*/ + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount;
Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1320
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+ }else { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + }It seems like the hole handling is the same for this case and what we handle below.
looks we only need to add unwritten extent lookup for hole handing. Return the offset if nothing found, for HOLESTARTBLOCK, return the offset. for DELAYSTARTBLOCK and NORM_EXTENT, examine the index fsbno for next xfs_bmapi_read().
Quoted 10 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1357
Quoted 2 lines
fs/xfs/xfs_file.c +1
-0+/* Landed in a delay allocated extent or a read data extent */ s/read/real/
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1391
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+STATIC loff_t +xfs_seek_hole ( +struct file *file, +loff_t start, +u32 type)
Just realized that we can omit 'type' argument here as well as xfs_seek_data(). both interface should be xfs_seek_xxx(struct file *file, loff_t start). :)
Quoted 18 lines
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+{ +struct inode *inode = file->f_mapping->host; +struct xfs_inode *ip =XFS_I (inode); +struct xfs_mount *mp = ip->i_mount; +xfs_fsize_t isize =i_size_read (inode);fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1406
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+xfs_fileoff_t fsbno; +loff_t holeoff; +loff_t offset =0 ; +int lock; +int error; + +if (start >= isize) +return -ENXIO ; + + lock =xfs_ilock_map_shared (ip);I'd move the check after acquiring the lock just to be sure.
Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1411
Quoted 3 lines
fs/xfs/xfs_file.c +3
-0+ error =xfs_bmap_first_unused (NULL , ip,1 , &fsbno,XFS_DATA_FORK ); +if (error) + goto out_lock;Hmm - this misses the unwritten cases that we handle in
SEEK_DATA. But if we want to handle it we probably can't use the simplexfs_bmap_first_unusedcall that Dave suggested but need to use thexfs_bmapi_readloop, too.
Oops! sorry, I have not took care this function when revising the patch last time. just as your above comments, in most cases, the unwritten extent fetched but without data committed to page cache, in this case, it should be treated as holes too. :)
Thanks,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs