Introduce SEEK_DATA/SEEK_HOLE support to XFS V7
PATCH 1/1 Introduce SEEK_DATA/SEEK_HOLE support to XFS V7
Hello,
There is one bug fix in this version, in xfs_seek_data()/xfs_seek_hole(), call xfs_bmapi_read() or xfs_bmap_first_unused() maybe failed, they should return ENXIO in this case. Thanks Mark for pointing this out!
--- fs/xfs/xfs_file.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 171 insertions(+), 1 deletions(-)
fs/xfs/xfs_file.c +171 -1
@@ -1141,8 +1141,178 @@ xfs_vm_page_mkwrite(return block_page_mkwrite (vma, vmf, xfs_get_blocks); } +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; +loff_t uninitialized_var (offset ); +xfs_fsize_t isize; +xfs_fileoff_t fsbno; +xfs_filblks_t len; +uint lock; +int error; + + lock =xfs_ilock_map_shared (ip); + + isize =i_size_read (inode); +if (start >= isize) { + error =ENXIO ; + goto out_unlock; + } + + fsbno =XFS_B_TO_FSBT (mp, start); + len =XFS_B_TO_FSB (mp, isize); +for (;;) { +struct xfs_bmbt_irec map[2 ]; +int nmap =2 ; +loff_t seekoff; + + error =xfs_bmapi_read (ip, fsbno, len - fsbno, map, &nmap, +XFS_BMAPI_ENTIRE ); +if (error) { + error =ENXIO ; + goto out_unlock; + } + +/* No extents at given offset, must be beyond EOF */ +if (nmap ==0 ) { + error =ENXIO ; + goto out_unlock; + } + + seekoff =XFS_FSB_TO_B (mp, fsbno); + +if ((map[0 ].br_state ==XFS_EXT_NORM && + !isnullstartblock (map[0 ].br_startblock)) || + map[0 ].br_startblock ==DELAYSTARTBLOCK ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ; + }else if (map[0 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ; + }else if (map[0 ].br_startblock ==HOLESTARTBLOCK ) { +if (nmap ==1 ) { + error =ENXIO ; + goto out_unlock; + } + +if ((map[1 ].br_state ==XFS_EXT_NORM && + !isnullstartblock (map[1 ].br_startblock)) || + map[1 ].br_startblock ==DELAYSTARTBLOCK ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + }else if (map[1 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + }else if (map[1 ].br_startblock ==HOLESTARTBLOCK ) { + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount; + }else { +BUG (); + } + }else { +BUG (); + } + +if (XFS_FSB_TO_B (mp, fsbno) > isize) { + error =ENXIO ; + goto out_unlock; + } + } + +if (offset < start) + offset = start; + +if (offset != file->f_pos) + file->f_pos = offset; + +out_unlock: +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; +loff_t uninitialized_var (offset ); +loff_t holeoff; +xfs_fsize_t isize; +xfs_fileoff_t fsbno; +uint lock; +int error; + + lock =xfs_ilock_map_shared (ip); + + isize =i_size_read (inode); +if (start >= isize) { + error =ENXIO ; + goto out_unlock; + } + + fsbno =XFS_B_TO_FSBT (mp, start); + error =xfs_bmap_first_unused (NULL , ip,1 , &fsbno,XFS_DATA_FORK ); +if (error) { + error =ENXIO ; + goto out_unlock; + } + + 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_unlock: +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 -EINVAL ; + } +} +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.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 18 lines · Jeff Liu
Hello,
There is one bug fix in this version, in xfs_seek_data()/xfs_seek_hole(), call
xfs_bmapi_read()orxfs_bmap_first_unused()maybe failed, they should return ENXIO in this case. Thanks Mark for pointing this out!Signed-off-by Jie Liu<[email protected]>--- fs/xfs/xfs_file.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 171 insertions(+), 1 deletions(-)fs/xfs/xfs_file.c +0
-0
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1141
Quoted 1 line
fs/xfs/xfs_file.c +0
-0@@ -1141,8 +1141,178 @@ xfs_vm_page_mkwrite(return block_page_mkwrite (vma, vmf, xfs_get_blocks);
Looks good.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 8 lines · Jeff Liu
Hello,
There is one bug fix in this version, in xfs_seek_data()/xfs_seek_hole(), call
xfs_bmapi_read()orxfs_bmap_first_unused()maybe failed, they should return ENXIO in this case. Thanks Mark for pointing this out!Signed-off-by Jie Liu <[email protected]>
Can you post a final version with the real commit message attached?
The normal way of making comments like this about a patch posting is to put the comments after the first "---" line, like the diffstat is below....
As it is,my comments are mainly about error handling and putting in some comments to explain exactly why the code ended up this way....
Quoted 28 lines
--- fs/xfs/xfs_file.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 171 insertions(+), 1 deletions(-)fs/xfs/xfs_file.c +16
-0@@ -1141,8 +1141,178 @@ xfs_vm_page_mkwrite(return block_page_mkwrite (vma, vmf, xfs_get_blocks); } +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; +loff_t uninitialized_var (offset ); +xfs_fsize_t isize; +xfs_fileoff_t fsbno; +xfs_filblks_t len; +uint lock; +int error; +
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1169
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ lock =xfs_ilock_map_shared (ip); + + isize =i_size_read (inode); +if (start >= isize) { + error =ENXIO ; + goto out_unlock; + } + + fsbno =XFS_B_TO_FSBT (mp, start); + len =XFS_B_TO_FSB (mp, isize);
It's not entirely obvious why len is based on isize rather than the (isize - start), the range being mapped. A comment might be in order so we don't make silly mistakes reading the code in a couple of years time.
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1180
Quoted 11 lines
fs/xfs/xfs_file.c +11
-0+for (;;) { +struct xfs_bmbt_irec map[2 ]; +int nmap =2 ; +loff_t seekoff; + + error =xfs_bmapi_read (ip, fsbno, len - fsbno, map, &nmap, +XFS_BMAPI_ENTIRE ); +if (error) { + error =ENXIO ; + goto out_unlock; + }
I don't think that is correct. ENXIO means "offset beyond EOF", and this will typically only return errors due to extent tree corruption or filesystem shutdown. I'd just return the error as it stands.
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1186
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+/* No extents at given offset, must be beyond EOF */ +if (nmap ==0 ) { + error =ENXIO ; + goto out_unlock; + }
But we can't be beyond EOF - we've already checked that. Hence we should *always* get a mapping back. To fail to get one back is a sign of extent tree corruption, I think, so this should probably be a XFS_WANT_CORRUPTED_GOTO() case....
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1192
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+ seekoff =XFS_FSB_TO_B (mp, fsbno); + +if ((map[0 ].br_state ==XFS_EXT_NORM && + !isnullstartblock (map[0 ].br_startblock)) || + map[0 ].br_startblock ==DELAYSTARTBLOCK ) {
So this skips holes and unwritten regions.
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1199
Quoted 7 lines
fs/xfs/xfs_file.c +7
-0+ offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ; + }else if (map[0 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ;
But unwritten regions have an identical offset caclulation to delayed and written regions. So that entire piece of logic becomes:
if (map[0].br_startblock != HOLESTARTBLOCK)) {
offset = max_t(loff_t, seekoff,
XFS_FSB_TO_B(mp, map[0].br_startoff));
break;
} else {A comment might be in order there, too, indicating why we are handling unwritten regions as data, just like written and delalloc regions....
Quoted 9 lines
fs/xfs/xfs_file.c +9
-0+ }else if (map[0 ].br_startblock ==HOLESTARTBLOCK ) { +if (nmap ==1 ) { + error =ENXIO ; + goto out_unlock; + } + +if ((map[1 ].br_state ==XFS_EXT_NORM && + !isnullstartblock (map[1 ].br_startblock)) || + map[1 ].br_startblock ==DELAYSTARTBLOCK ) {
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1218
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + }else if (map[1 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + }else if (map[1 ].br_startblock ==HOLESTARTBLOCK ) { + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount;
Same again:
if (map[1].br_startblock != HOLESTARTBLOCK) {
offset = max_t(loff_t, seekoff
XFS_FSB_TO_B(mp, map[1].br_startoff));
break;
} else {
fsbno = map[1].br_startoff +
map[1].br_blockcount;
}fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1224
Quoted 6 lines
fs/xfs/xfs_file.c +6
-0+ }else { +BUG (); + } + }else { +BUG (); + }
Panicing the machine just because the filesystem might be corrupted in not a very nice way to handle the error. Given that we don't even need to handle wierd map states here (because the xfs_bmapi_read() will have found any corruption during the lookup) i don't think this is at all necessary.
Quoted 41 lines
fs/xfs/xfs_file.c +41
-0+ +if (XFS_FSB_TO_B (mp, fsbno) > isize) { + error =ENXIO ; + goto out_unlock; + } + } + +if (offset < start) + offset = start; + +if (offset != file->f_pos) + file->f_pos = offset; + +out_unlock: +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; +loff_t uninitialized_var (offset ); +loff_t holeoff; +xfs_fsize_t isize; +xfs_fileoff_t fsbno; +uint lock; +int error; + + lock =xfs_ilock_map_shared (ip); + + isize =i_size_read (inode); +if (start >= isize) {
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1275
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ error =ENXIO ; + goto out_unlock; + } + + fsbno =XFS_B_TO_FSBT (mp, start); + error =xfs_bmap_first_unused (NULL , ip,1 , &fsbno,XFS_DATA_FORK ); +if (error) { + error =ENXIO ; + goto out_unlock; + }
Same comment here about error handling. xfs_bmap_first_unused() failing usually indicates a corruption, not a "offset beyond EOF", so we should be returning the error that the filesystem has returned rather than ENXIO.
Quoted 52 lines
fs/xfs/xfs_file.c +39
-1+ + 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_unlock: +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 -EINVAL ; + } +} +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.9 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Hi Dave,
Thanks for your review.
Quoted 12 lines · Dave Chinner
Quoted 8 lines · Jeff Liu
Hello,
There is one bug fix in this version, in xfs_seek_data()/xfs_seek_hole(), call
xfs_bmapi_read()orxfs_bmap_first_unused()maybe failed, they should return ENXIO in this case. Thanks Mark for pointing this out!Signed-off-by Jie Liu <[email protected]>Can you post a final version with the real commit message attached?
Quoted 7 lines
The normal way of making comments like this about a patch posting is to put the comments after the first "---" line, like the diffstat is below....
As it is,my comments are mainly about error handling and putting in some comments to explain exactly why the code ended up this way....
Ok, the commit message will added in next post.
But I still have a concern about error handing as below.
Quoted 45 lines
Quoted 29 lines
--- fs/xfs/xfs_file.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 171 insertions(+), 1 deletions(-)fs/xfs/xfs_file.c +16
-0@@ -1141,8 +1141,178 @@ xfs_vm_page_mkwrite(return block_page_mkwrite (vma, vmf, xfs_get_blocks); } +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; +loff_t uninitialized_var (offset ); +xfs_fsize_t isize; +xfs_fileoff_t fsbno; +xfs_filblks_t len; +uint lock; +int error; +fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1169
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ lock =xfs_ilock_map_shared (ip); + + isize =i_size_read (inode); +if (start >= isize) { + error =ENXIO ; + goto out_unlock; + } + + fsbno =XFS_B_TO_FSBT (mp, start); + len =XFS_B_TO_FSB (mp, isize);It's not entirely obvious why len is based on isize rather than the (isize - start), the range being mapped. A comment might be in order so we don't make silly mistakes reading the code in a couple of years time.
Quoted 15 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1180
Quoted 11 lines
fs/xfs/xfs_file.c +11
-0+for (;;) { +struct xfs_bmbt_irec map[2 ]; +int nmap =2 ; +loff_t seekoff; + + error =xfs_bmapi_read (ip, fsbno, len - fsbno, map, &nmap, +XFS_BMAPI_ENTIRE ); +if (error) { + error =ENXIO ; + goto out_unlock; + }I don't think that is correct. ENXIO means "offset beyond EOF", and this will typically only return errors due to extent tree corruption or filesystem shutdown. I'd just return the error as it stands.
Currently, both OCFS2 and Btrfs return ENXIO in case of their particular extent fetch routine failed. So return other internal errors in XFS will introduce incompatibility IMHO.
But just as you pointed out, as well as ENXIO semantics defined at http://linux.die.net/man/2/lseek,
return ENXIO is not proper to internal issue, and that might confuse user in such situation for return value check up.
Maybe it's better to reach a consensus with other file systems with SEEK_DATA/SEEK_HOLE supports, I'd suggest either:
1) return the error as it stands in particular file system.
or:
2) return EIO or another meaningful error number.
look all those file systems need to fix up accordingly.
Quoted 10 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1186
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+/* No extents at given offset, must be beyond EOF */ +if (nmap ==0 ) { + error =ENXIO ; + goto out_unlock; + }But we can't be beyond EOF - we've already checked that. Hence we should *always* get a mapping back. To fail to get one back is a sign of extent tree corruption, I think, so this should probably be a
XFS_WANT_CORRUPTED_GOTO()case....
checking "nmap == 0" also has another option, since there might have continuous hole extents in mapped file range, i.e, both map[0] and map[1] are holes(for now, I have not yet worked out a test case to verify this scenario). I still need some time to verify that.
Quoted 9 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1192
Quoted 6 lines
fs/xfs/xfs_file.c +5
-0+ seekoff =XFS_FSB_TO_B (mp, fsbno); + +if ((map[0 ].br_state ==XFS_EXT_NORM && + !isnullstartblock (map[0 ].br_startblock)) || + map[0 ].br_startblock ==DELAYSTARTBLOCK ) {So this skips holes and unwritten regions.
Quoted 7 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1199
Quoted 7 lines
fs/xfs/xfs_file.c +7
-0+ offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ; + }else if (map[0 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ;
Using "map[0].br_startblock != HOLESTARTBLOCK" to simplify the logic for offset calculation is cool! However, treat unwritten extent as data is temporarily for us, it will finally split up to an individual check when trying to add dirty data lookup. Given that, how about if we keep the current logic so that we don't need much code change in future, does it make sense? :)
Quoted 11 lines
But unwritten regions have an identical offset caclulation to delayed and written regions. So that entire piece of logic becomes:
if (map[0].br_startblock != HOLESTARTBLOCK)) { offset = max_t(loff_t, seekoff, XFS_FSB_TO_B(mp, map[0].br_startoff)); break; } else {
Quoted 3 lines
A comment might be in order there, too, indicating why we are handling unwritten regions as data, just like written and delalloc regions....
Ok.
Quoted 31 lines
Quoted 10 lines
fs/xfs/xfs_file.c +9
-0+ }else if (map[0 ].br_startblock ==HOLESTARTBLOCK ) { +if (nmap ==1 ) { + error =ENXIO ; + goto out_unlock; + } + +if ((map[1 ].br_state ==XFS_EXT_NORM && + !isnullstartblock (map[1 ].br_startblock)) || + map[1 ].br_startblock ==DELAYSTARTBLOCK ) {fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1218
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + }else if (map[1 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[1 ].br_startoff)); +break ; + }else if (map[1 ].br_startblock ==HOLESTARTBLOCK ) { + fsbno = map[1 ].br_startoff + + map[1 ].br_blockcount;Same again:
if (map[1].br_startblock != HOLESTARTBLOCK) { offset = max_t(loff_t, seekoff XFS_FSB_TO_B(mp, map[1].br_startoff)); break; } else { fsbno = map[1].br_startoff + map[1].br_blockcount; }
Quoted 12 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1224
Quoted 6 lines
fs/xfs/xfs_file.c +6
-0+ }else { +BUG (); + } + }else { +BUG (); + }Panicing the machine just because the filesystem might be corrupted in not a very nice way to handle the error. Given that we don't even need to handle wierd map states here (because the
xfs_bmapi_read()will have found any corruption during the lookup) i don't think this is at all necessary.
Yes, looks we can safely remove them. :)
I'll take care below things too.
Thanks,
-Jeff
Quoted 105 lines
Quoted 42 lines
fs/xfs/xfs_file.c +41
-0+ +if (XFS_FSB_TO_B (mp, fsbno) > isize) { + error =ENXIO ; + goto out_unlock; + } + } + +if (offset < start) + offset = start; + +if (offset != file->f_pos) + file->f_pos = offset; + +out_unlock: +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; +loff_t uninitialized_var (offset ); +loff_t holeoff; +xfs_fsize_t isize; +xfs_fileoff_t fsbno; +uint lock; +int error; + + lock =xfs_ilock_map_shared (ip); + + isize =i_size_read (inode); +if (start >= isize) {fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1275
Quoted 10 lines
fs/xfs/xfs_file.c +10
-0+ error =ENXIO ; + goto out_unlock; + } + + fsbno =XFS_B_TO_FSBT (mp, start); + error =xfs_bmap_first_unused (NULL , ip,1 , &fsbno,XFS_DATA_FORK ); +if (error) { + error =ENXIO ; + goto out_unlock; + }Same comment here about error handling.
xfs_bmap_first_unused()failing usually indicates a corruption, not a "offset beyond EOF", so we should be returning the error that the filesystem has returned rather than ENXIO.Quoted 47 lines
fs/xfs/xfs_file.c +39
-1+ + 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_unlock: +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 -EINVAL ; + } +} +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 · 1 line
1.7.9
Quoted 6 lines
Quoted 5 lines
_______________________________________________
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 28 lines · Jeff Liu
Hi Dave,
Thanks for your review.
Quoted 12 lines · Dave Chinner
Quoted 8 lines · Jeff Liu
Hello,
There is one bug fix in this version, in xfs_seek_data()/xfs_seek_hole(), call
xfs_bmapi_read()orxfs_bmap_first_unused()maybe failed, they should return ENXIO in this case. Thanks Mark for pointing this out!Signed-off-by Jie Liu <[email protected]>Can you post a final version with the real commit message attached?
Quoted 7 lines
The normal way of making comments like this about a patch posting is to put the comments after the first "---" line, like the diffstat is below....
As it is,my comments are mainly about error handling and putting in some comments to explain exactly why the code ended up this way....
Ok, the commit message will added in next post.
But I still have a concern about error handing as below.
.....
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
fs/xfs/xfs_file.c +7
-0+for (;;) { +struct xfs_bmbt_irec map[2 ]; +int nmap =2 ; +loff_t seekoff; + + error =xfs_bmapi_read (ip, fsbno, len - fsbno, map, &nmap, +XFS_BMAPI_ENTIRE );Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1180
Quoted 4 lines
fs/xfs/xfs_file.c +4
-0+if (error) { + error =ENXIO ; + goto out_unlock; + }I don't think that is correct. ENXIO means "offset beyond EOF", and this will typically only return errors due to extent tree corruption or filesystem shutdown. I'd just return the error as it stands.
Quoted 2 lines
Currently, both OCFS2 and Btrfs return ENXIO in case of their particular extent fetch routine failed.
That doesn't mean it is the best thing to do. The seek has not succeeded and the current file position is now undefined. We did not seek past the EOF - an extent lookup failure indicates we don't even know where the EOF or even the next hole or data area exists.
Hence the correct thing to do here is return a fatal error, not something that the application can interpret as a successful operation....
Quoted 2 lines
So return other internal errors in XFS will introduce incompatibility IMHO.
Filesystems return different errors to the same syscall in lots of places. Indeed, XFS will return EUCLEAN to just about any syscall when the filesystem has been shut down to indicate a fatal error, and that's not documented in a single syscall man page....
Quoted 4 lines
But just as you pointed out, as well as ENXIO semantics defined at
http://linux.die.net/man/2/lseek, return ENXIO is not proper to internal issue, and that might confuse user in such situation for return value check up.
Exactly, because it is valid for applications to be expecting ENXIO when trying to find a specific data or hole location in a file (e.g. is there any hole I can fill? EXNIO == no holes in the file)
Quoted 6 lines
Maybe it's better to reach a consensus with other file systems with SEEK_DATA/SEEK_HOLE supports, I'd suggest either:
1) return the error as it stands in particular file system.
or:
2) return EIO or another meaningful error number.
look all those file systems need to fix up accordingly.
See above - filesystems are free to return errors according to the problem that has occurred - it doesn't need to be defined in the man page. XFS policy is to return EUCLEAN when a corruption is detected, EIO when a read error has occurred, etc. Other filesystems are free to do treat these error conditions however they want, so I don't see any particular need for standardisation here...
Quoted 11 lines
Quoted 10 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1186
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+/* No extents at given offset, must be beyond EOF */ +if (nmap ==0 ) { + error =ENXIO ; + goto out_unlock; + }But we can't be beyond EOF - we've already checked that. Hence we should *always* get a mapping back. To fail to get one back is a sign of extent tree corruption, I think, so this should probably be a
XFS_WANT_CORRUPTED_GOTO()case....
Quoted 2 lines
checking "nmap == 0" also has another option, since there might have continuous hole extents in mapped file range, i.e, both map[0] and map[1] are
holes(for now, I have not yet worked out a test case to verify this scenario).
If it lands in a hole, a hole mapping will be returned for the given range. IOWs, xfs_bmapi_read will always return a mapping of some kind for a range that is within EOF. Hence nmap == 0 is an indication of something gone very wrong...
Quoted 1 line
I still need some time to verify that.
Create a sparse file of a petabyte size, write a block in the first 1MB, then write a block in the last MB. The see what gets returned....
Quoted 8 lines
Quoted 8 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1192
Quoted 5 lines
fs/xfs/xfs_file.c +5
-0+ seekoff =XFS_FSB_TO_B (mp, fsbno); + +if ((map[0 ].br_state ==XFS_EXT_NORM && + !isnullstartblock (map[0 ].br_startblock)) || + map[0 ].br_startblock ==DELAYSTARTBLOCK ) {So this skips holes and unwritten regions.
Quoted 12 lines
Quoted 7 lines
fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1199
Quoted 7 lines
fs/xfs/xfs_file.c +7
-0+ offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ; + }else if (map[0 ].br_state ==XFS_EXT_UNWRITTEN ) { + offset =max_t (loff_t, seekoff, +XFS_FSB_TO_B (mp, map[0 ].br_startoff)); +break ;Using "map[0].br_startblock != HOLESTARTBLOCK" to simplify the logic for offset calculation is cool! However, treat unwritten extent as data is temporarily for us, it will finally split up to an individual check when trying to add dirty data lookup. Given that, how about if we keep the current logic so that we don't need much code change in future, does it make sense? :)
We can change the code easily enough to discriminate between written and unwritten extents once we work out how best to do that. So right now I'd prefer simple code that documents the cases we support very clearly.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs