* Re: [PATCH RFC] xfs: combine xfs_seek_hole & xfs_seek_data
2014-08-21 2:20 [PATCH RFC] xfs: combine xfs_seek_hole & xfs_seek_data Eric Sandeen
@ 2014-08-21 12:37 ` Jeff Liu
2014-08-21 14:20 ` Eric Sandeen
2014-08-21 14:33 ` Brian Foster
2014-08-21 19:23 ` [PATCH V2] " Eric Sandeen
2 siblings, 1 reply; 10+ messages in thread
From: Jeff Liu @ 2014-08-21 12:37 UTC (permalink / raw)
To: Eric Sandeen, xfs-oss
Hi Eric,
On 08/21/2014 10:20 AM, Eric Sandeen wrote:
> xfs_seek_hole & xfs_seek_data are remarkably similar;
> so much so that they could be combined, saving a fair
> bit of semi-complex code duplication.
>
> The following patch passes generic/285 and generic/286; E
> is this worth doing, (maybe cleaning up a bit), or
> is it too convoluted & confusing?
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> xfs_file.c | 174 +++++++++++++++++--------------------------------------------
> 1 file changed, 50 insertions(+), 124 deletions(-)
Significant code reduction :)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 076b170..321dde6 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -964,7 +964,7 @@ xfs_vm_page_mkwrite(
>
> /*
> * 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().
> + * to search from page cache for xfs_seek_hole_data().
> */
> enum {
> HOLE_OFF = 0,
> @@ -1021,7 +1021,7 @@ xfs_lookup_buffer_offset(
> /*
> * 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().
> + * type for xfs_seek_hole_data().
> *
> * 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
> @@ -1181,9 +1181,10 @@ out:
> }
>
> STATIC loff_t
> -xfs_seek_data(
> +xfs_seek_hole_data(
> struct file *file,
> - loff_t start)
> + loff_t start,
> + int whence)
> {
> struct inode *inode = file->f_mapping->host;
> struct xfs_inode *ip = XFS_I(inode);
> @@ -1195,6 +1196,9 @@ xfs_seek_data(
> uint lock;
> int error;
>
> + if (XFS_FORCED_SHUTDOWN(mp))
> + return -EIO;
Currently, the forced shutdown check up is only enabled for SEEK_HOLE.
But looks we should add it for SEEK_DATA as well, no matter the RFC
patch would be applied or not.
> +
> lock = xfs_ilock_data_map_shared(ip);
>
> isize = i_size_read(inode);
> @@ -1209,6 +1213,7 @@ xfs_seek_data(
> */
> fsbno = XFS_B_TO_FSBT(mp, start);
> end = XFS_B_TO_FSB(mp, isize);
> +
> for (;;) {
> struct xfs_bmbt_irec map[2];
> int nmap = 2;
> @@ -1229,29 +1234,46 @@ xfs_seek_data(
> offset = max_t(loff_t, start,
> XFS_FSB_TO_B(mp, map[i].br_startoff));
>
> - /* Landed in a data extent */
> - if (map[i].br_startblock == DELAYSTARTBLOCK ||
> - (map[i].br_state == XFS_EXT_NORM &&
> - !isnullstartblock(map[i].br_startblock)))
> + /* Landed in the hole we wanted? */
> + if (whence == SEEK_HOLE &&
> + map[i].br_startblock == HOLESTARTBLOCK)
> + goto out;
> +
> + /* Landed in the data extent we wanted? */
> + if (whence == SEEK_DATA &&
> + (map[i].br_startblock == DELAYSTARTBLOCK ||
> + (map[i].br_state == XFS_EXT_NORM &&
> + !isnullstartblock(map[i].br_startblock))))
> goto out;
>
> /*
> - * Landed in an unwritten extent, try to search data
> - * from page cache.
> + * Landed in an unwritten extent, try to search
> + * for hole or data from page cache.
> */
> if (map[i].br_state == XFS_EXT_UNWRITTEN) {
> if (xfs_find_get_desired_pgoff(inode, &map[i],
> - DATA_OFF, &offset))
> + whence == SEEK_HOLE ? HOLE_OFF : DATA_OFF,
> + &offset))
> goto out;
> }
> }
>
> - /*
> - * map[0] is hole or its an unwritten extent but
> - * without data in page cache. Probably means that
> - * we are reading after EOF if nothing in map[1].
> - */
> if (nmap == 1) {
> + /*
> + * The single map didn't have what we were looking for.
> + * If we were looking for a hole, we are probably
> + * looking past EOF. We should fix offset to point
> + * to the end of the file (i.e., there is an implicit
> + * hole at the end of any file).
> + */
> + if (whence == SEEK_HOLE) {
> + offset = isize;
> + break;
> + }
> + /*
> + * If we were looking for data, it's nowhere to be found
> + */
> + ASSERT(whence == SEEK_DATA);
> error = -ENXIO;
> goto out_unlock;
> }
> @@ -1260,125 +1282,30 @@ xfs_seek_data(
>
> /*
> * Nothing was found, proceed to the next round of search
> - * if reading offset not beyond or hit EOF.
> + * if the next reading offset is not at or beyond EOF.
> */
> fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
> start = XFS_FSB_TO_B(mp, fsbno);
> if (start >= isize) {
> + if (whence == SEEK_HOLE) {
> + offset = isize;
> + break;
> + }
> + ASSERT(whence == SEEK_DATA);
> error = -ENXIO;
> goto out_unlock;
> }
> }
>
> out:
> - offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
> -
> -out_unlock:
> - xfs_iunlock(ip, lock);
> -
> - if (error)
> - return error;
> - return offset;
> -}
> -
> -STATIC loff_t
> -xfs_seek_hole(
> - struct file *file,
> - loff_t start)
> -{
> - 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 end;
> - uint lock;
> - int error;
> -
> - if (XFS_FORCED_SHUTDOWN(mp))
> - return -EIO;
> -
> - lock = xfs_ilock_data_map_shared(ip);
> -
> - isize = i_size_read(inode);
> - if (start >= isize) {
> - error = -ENXIO;
> - goto out_unlock;
> - }
> -
> - fsbno = XFS_B_TO_FSBT(mp, start);
> - end = XFS_B_TO_FSB(mp, isize);
> -
> - for (;;) {
> - struct xfs_bmbt_irec map[2];
> - int nmap = 2;
> - unsigned int i;
> -
> - error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
> - XFS_BMAPI_ENTIRE);
> - if (error)
> - goto out_unlock;
> -
> - /* No extents at given offset, must be beyond EOF */
> - if (nmap == 0) {
> - error = -ENXIO;
> - goto out_unlock;
> - }
> -
> - for (i = 0; i < nmap; i++) {
> - offset = max_t(loff_t, start,
> - XFS_FSB_TO_B(mp, map[i].br_startoff));
> -
> - /* Landed in a hole */
> - if (map[i].br_startblock == HOLESTARTBLOCK)
> - goto out;
> -
> - /*
> - * Landed in an unwritten extent, try to search hole
> - * from page cache.
> - */
> - if (map[i].br_state == XFS_EXT_UNWRITTEN) {
> - if (xfs_find_get_desired_pgoff(inode, &map[i],
> - HOLE_OFF, &offset))
> - goto out;
> - }
> - }
> -
> - /*
> - * map[0] contains data or its unwritten but contains
> - * data in page cache, probably means that we are
> - * reading after EOF. We should fix offset to point
> - * to the end of the file(i.e., there is an implicit
> - * hole at the end of any file).
> - */
> - if (nmap == 1) {
> - offset = isize;
> - break;
> - }
> -
> - ASSERT(i > 1);
> -
> - /*
> - * Both mappings contains data, proceed to the next round of
> - * search if the current reading offset not beyond or hit EOF.
> - */
> - fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
> - start = XFS_FSB_TO_B(mp, fsbno);
> - if (start >= isize) {
> - offset = isize;
> - break;
> - }
> - }
> -
> -out:
> /*
> - * At this point, we must have found a hole. However, the returned
> + * If at this point we have found the hole we wanted, the returned
> * offset may be bigger than the file size as it may be aligned to
> - * page boundary for unwritten extents, we need to deal with this
> + * page boundary for unwritten extents. We need to deal with this
> * situation in particular.
> */
> - offset = min_t(loff_t, offset, isize);
> + if (whence == SEEK_HOLE)
> + offset = min_t(loff_t, offset, isize);
> offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
>
> out_unlock:
> @@ -1400,10 +1327,9 @@ xfs_file_llseek(
> case SEEK_CUR:
> case SEEK_SET:
> return generic_file_llseek(file, offset, origin);
> - case SEEK_DATA:
> - return xfs_seek_data(file, offset);
> case SEEK_HOLE:
> - return xfs_seek_hole(file, offset);
> + case SEEK_DATA:
> + return xfs_seek_hole_data(file, offset, origin);
> default:
> return -EINVAL;
> }
With the current refactoring, the code logic still looks easy to understand,
so I personally vote this change.
BTW, originally I have also tried to implement SEEK_HOLE/DATA in one routine
in my 1st round of patch which was shown as following. However, I failed to
make the code looks readable and works correctly at that time.
http://oss.sgi.com/archives/xfs/2011-11/msg00364.html
http://oss.sgi.com/archives/xfs/2011-11/msg00395.html
Cheers,
-Jeff
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH RFC] xfs: combine xfs_seek_hole & xfs_seek_data
2014-08-21 12:37 ` Jeff Liu
@ 2014-08-21 14:20 ` Eric Sandeen
0 siblings, 0 replies; 10+ messages in thread
From: Eric Sandeen @ 2014-08-21 14:20 UTC (permalink / raw)
To: Jeff Liu, Eric Sandeen, xfs-oss
On 8/21/14, 7:37 AM, Jeff Liu wrote:
> With the current refactoring, the code logic still looks easy to understand,
> so I personally vote this change.
>
> BTW, originally I have also tried to implement SEEK_HOLE/DATA in one routine
> in my 1st round of patch which was shown as following. However, I failed to
> make the code looks readable and works correctly at that time.
>
> http://oss.sgi.com/archives/xfs/2011-11/msg00364.html
> http://oss.sgi.com/archives/xfs/2011-11/msg00395.html
>
>
> Cheers,
> -Jeff
Ah, I had forgotten about that! Good idea! ;)
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] xfs: combine xfs_seek_hole & xfs_seek_data
2014-08-21 2:20 [PATCH RFC] xfs: combine xfs_seek_hole & xfs_seek_data Eric Sandeen
2014-08-21 12:37 ` Jeff Liu
@ 2014-08-21 14:33 ` Brian Foster
2014-08-21 19:23 ` [PATCH V2] " Eric Sandeen
2 siblings, 0 replies; 10+ messages in thread
From: Brian Foster @ 2014-08-21 14:33 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Jeff Liu, xfs-oss
On Wed, Aug 20, 2014 at 09:20:21PM -0500, Eric Sandeen wrote:
> xfs_seek_hole & xfs_seek_data are remarkably similar;
> so much so that they could be combined, saving a fair
> bit of semi-complex code duplication.
>
> The following patch passes generic/285 and generic/286;
> is this worth doing, (maybe cleaning up a bit), or
> is it too convoluted & confusing?
>
I agree with Jeff. I think the comments clarify what's going on and the
general flow is the same between both types of seek. Just a comment nit
below...
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> xfs_file.c | 174 +++++++++++++++++--------------------------------------------
> 1 file changed, 50 insertions(+), 124 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 076b170..321dde6 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -964,7 +964,7 @@ xfs_vm_page_mkwrite(
>
> /*
> * 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().
> + * to search from page cache for xfs_seek_hole_data().
> */
> enum {
> HOLE_OFF = 0,
> @@ -1021,7 +1021,7 @@ xfs_lookup_buffer_offset(
> /*
> * 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().
> + * type for xfs_seek_hole_data().
> *
> * 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
> @@ -1181,9 +1181,10 @@ out:
> }
>
> STATIC loff_t
> -xfs_seek_data(
> +xfs_seek_hole_data(
> struct file *file,
> - loff_t start)
> + loff_t start,
> + int whence)
> {
> struct inode *inode = file->f_mapping->host;
> struct xfs_inode *ip = XFS_I(inode);
> @@ -1195,6 +1196,9 @@ xfs_seek_data(
> uint lock;
> int error;
>
> + if (XFS_FORCED_SHUTDOWN(mp))
> + return -EIO;
> +
> lock = xfs_ilock_data_map_shared(ip);
>
> isize = i_size_read(inode);
> @@ -1209,6 +1213,7 @@ xfs_seek_data(
> */
> fsbno = XFS_B_TO_FSBT(mp, start);
> end = XFS_B_TO_FSB(mp, isize);
> +
> for (;;) {
> struct xfs_bmbt_irec map[2];
> int nmap = 2;
> @@ -1229,29 +1234,46 @@ xfs_seek_data(
> offset = max_t(loff_t, start,
> XFS_FSB_TO_B(mp, map[i].br_startoff));
>
> - /* Landed in a data extent */
> - if (map[i].br_startblock == DELAYSTARTBLOCK ||
> - (map[i].br_state == XFS_EXT_NORM &&
> - !isnullstartblock(map[i].br_startblock)))
> + /* Landed in the hole we wanted? */
> + if (whence == SEEK_HOLE &&
> + map[i].br_startblock == HOLESTARTBLOCK)
> + goto out;
> +
> + /* Landed in the data extent we wanted? */
> + if (whence == SEEK_DATA &&
> + (map[i].br_startblock == DELAYSTARTBLOCK ||
> + (map[i].br_state == XFS_EXT_NORM &&
> + !isnullstartblock(map[i].br_startblock))))
> goto out;
>
> /*
> - * Landed in an unwritten extent, try to search data
> - * from page cache.
> + * Landed in an unwritten extent, try to search
> + * for hole or data from page cache.
> */
> if (map[i].br_state == XFS_EXT_UNWRITTEN) {
> if (xfs_find_get_desired_pgoff(inode, &map[i],
> - DATA_OFF, &offset))
> + whence == SEEK_HOLE ? HOLE_OFF : DATA_OFF,
> + &offset))
> goto out;
> }
> }
>
> - /*
> - * map[0] is hole or its an unwritten extent but
> - * without data in page cache. Probably means that
> - * we are reading after EOF if nothing in map[1].
> - */
I think the comment could be slightly improved to explicitly point out
why we care about nmap == 1 rather than implicitly via rehashing the
logic above. For example:
/*
* We only received one extent out of the two requested. This
* means we've hit EOF and didn't find what we are looking for.
*/
> if (nmap == 1) {
> + /*
> + * The single map didn't have what we were looking for.
> + * If we were looking for a hole, we are probably
> + * looking past EOF. We should fix offset to point
> + * to the end of the file (i.e., there is an implicit
> + * hole at the end of any file).
> + */
/*
* If we were looking for a hole, set offset to the
* implicit hole at EOF.
*/
Brian
> + if (whence == SEEK_HOLE) {
> + offset = isize;
> + break;
> + }
> + /*
> + * If we were looking for data, it's nowhere to be found
> + */
> + ASSERT(whence == SEEK_DATA);
> error = -ENXIO;
> goto out_unlock;
> }
> @@ -1260,125 +1282,30 @@ xfs_seek_data(
>
> /*
> * Nothing was found, proceed to the next round of search
> - * if reading offset not beyond or hit EOF.
> + * if the next reading offset is not at or beyond EOF.
> */
> fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
> start = XFS_FSB_TO_B(mp, fsbno);
> if (start >= isize) {
> + if (whence == SEEK_HOLE) {
> + offset = isize;
> + break;
> + }
> + ASSERT(whence == SEEK_DATA);
> error = -ENXIO;
> goto out_unlock;
> }
> }
>
> out:
> - offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
> -
> -out_unlock:
> - xfs_iunlock(ip, lock);
> -
> - if (error)
> - return error;
> - return offset;
> -}
> -
> -STATIC loff_t
> -xfs_seek_hole(
> - struct file *file,
> - loff_t start)
> -{
> - 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 end;
> - uint lock;
> - int error;
> -
> - if (XFS_FORCED_SHUTDOWN(mp))
> - return -EIO;
> -
> - lock = xfs_ilock_data_map_shared(ip);
> -
> - isize = i_size_read(inode);
> - if (start >= isize) {
> - error = -ENXIO;
> - goto out_unlock;
> - }
> -
> - fsbno = XFS_B_TO_FSBT(mp, start);
> - end = XFS_B_TO_FSB(mp, isize);
> -
> - for (;;) {
> - struct xfs_bmbt_irec map[2];
> - int nmap = 2;
> - unsigned int i;
> -
> - error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
> - XFS_BMAPI_ENTIRE);
> - if (error)
> - goto out_unlock;
> -
> - /* No extents at given offset, must be beyond EOF */
> - if (nmap == 0) {
> - error = -ENXIO;
> - goto out_unlock;
> - }
> -
> - for (i = 0; i < nmap; i++) {
> - offset = max_t(loff_t, start,
> - XFS_FSB_TO_B(mp, map[i].br_startoff));
> -
> - /* Landed in a hole */
> - if (map[i].br_startblock == HOLESTARTBLOCK)
> - goto out;
> -
> - /*
> - * Landed in an unwritten extent, try to search hole
> - * from page cache.
> - */
> - if (map[i].br_state == XFS_EXT_UNWRITTEN) {
> - if (xfs_find_get_desired_pgoff(inode, &map[i],
> - HOLE_OFF, &offset))
> - goto out;
> - }
> - }
> -
> - /*
> - * map[0] contains data or its unwritten but contains
> - * data in page cache, probably means that we are
> - * reading after EOF. We should fix offset to point
> - * to the end of the file(i.e., there is an implicit
> - * hole at the end of any file).
> - */
> - if (nmap == 1) {
> - offset = isize;
> - break;
> - }
> -
> - ASSERT(i > 1);
> -
> - /*
> - * Both mappings contains data, proceed to the next round of
> - * search if the current reading offset not beyond or hit EOF.
> - */
> - fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
> - start = XFS_FSB_TO_B(mp, fsbno);
> - if (start >= isize) {
> - offset = isize;
> - break;
> - }
> - }
> -
> -out:
> /*
> - * At this point, we must have found a hole. However, the returned
> + * If at this point we have found the hole we wanted, the returned
> * offset may be bigger than the file size as it may be aligned to
> - * page boundary for unwritten extents, we need to deal with this
> + * page boundary for unwritten extents. We need to deal with this
> * situation in particular.
> */
> - offset = min_t(loff_t, offset, isize);
> + if (whence == SEEK_HOLE)
> + offset = min_t(loff_t, offset, isize);
> offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
>
> out_unlock:
> @@ -1400,10 +1327,9 @@ xfs_file_llseek(
> case SEEK_CUR:
> case SEEK_SET:
> return generic_file_llseek(file, offset, origin);
> - case SEEK_DATA:
> - return xfs_seek_data(file, offset);
> case SEEK_HOLE:
> - return xfs_seek_hole(file, offset);
> + case SEEK_DATA:
> + return xfs_seek_hole_data(file, offset, origin);
> default:
> return -EINVAL;
> }
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH V2] xfs: combine xfs_seek_hole & xfs_seek_data
2014-08-21 2:20 [PATCH RFC] xfs: combine xfs_seek_hole & xfs_seek_data Eric Sandeen
2014-08-21 12:37 ` Jeff Liu
2014-08-21 14:33 ` Brian Foster
@ 2014-08-21 19:23 ` Eric Sandeen
2014-08-21 19:30 ` [PATCH] xfs: lseek: the "whence" argument is called "whence" Eric Sandeen
` (2 more replies)
2 siblings, 3 replies; 10+ messages in thread
From: Eric Sandeen @ 2014-08-21 19:23 UTC (permalink / raw)
To: Eric Sandeen, xfs-oss, Jeff Liu
xfs_seek_hole & xfs_seek_data are remarkably similar;
so much so that they can be combined, saving a fair
bit of semi-complex code duplication.
The following patch passes generic/285 and generic/286,
which specifically test seek behavior.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: comment update as suggested by Brian
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 076b170..1da3b7d 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -964,7 +964,7 @@ xfs_vm_page_mkwrite(
/*
* 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().
+ * to search from page cache for xfs_seek_hole_data().
*/
enum {
HOLE_OFF = 0,
@@ -1021,7 +1021,7 @@ xfs_lookup_buffer_offset(
/*
* 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().
+ * type for xfs_seek_hole_data().
*
* 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
@@ -1181,9 +1181,10 @@ out:
}
STATIC loff_t
-xfs_seek_data(
+xfs_seek_hole_data(
struct file *file,
- loff_t start)
+ loff_t start,
+ int whence)
{
struct inode *inode = file->f_mapping->host;
struct xfs_inode *ip = XFS_I(inode);
@@ -1195,6 +1196,9 @@ xfs_seek_data(
uint lock;
int error;
+ if (XFS_FORCED_SHUTDOWN(mp))
+ return -EIO;
+
lock = xfs_ilock_data_map_shared(ip);
isize = i_size_read(inode);
@@ -1209,6 +1213,7 @@ xfs_seek_data(
*/
fsbno = XFS_B_TO_FSBT(mp, start);
end = XFS_B_TO_FSB(mp, isize);
+
for (;;) {
struct xfs_bmbt_irec map[2];
int nmap = 2;
@@ -1229,29 +1234,48 @@ xfs_seek_data(
offset = max_t(loff_t, start,
XFS_FSB_TO_B(mp, map[i].br_startoff));
- /* Landed in a data extent */
- if (map[i].br_startblock == DELAYSTARTBLOCK ||
- (map[i].br_state == XFS_EXT_NORM &&
- !isnullstartblock(map[i].br_startblock)))
+ /* Landed in the hole we wanted? */
+ if (whence == SEEK_HOLE &&
+ map[i].br_startblock == HOLESTARTBLOCK)
+ goto out;
+
+ /* Landed in the data extent we wanted? */
+ if (whence == SEEK_DATA &&
+ (map[i].br_startblock == DELAYSTARTBLOCK ||
+ (map[i].br_state == XFS_EXT_NORM &&
+ !isnullstartblock(map[i].br_startblock))))
goto out;
/*
- * Landed in an unwritten extent, try to search data
- * from page cache.
+ * Landed in an unwritten extent, try to search
+ * for hole or data from page cache.
*/
if (map[i].br_state == XFS_EXT_UNWRITTEN) {
if (xfs_find_get_desired_pgoff(inode, &map[i],
- DATA_OFF, &offset))
+ whence == SEEK_HOLE ? HOLE_OFF : DATA_OFF,
+ &offset))
goto out;
}
}
/*
- * map[0] is hole or its an unwritten extent but
- * without data in page cache. Probably means that
- * we are reading after EOF if nothing in map[1].
+ * We only received one extent out of the two requested. This
+ * means we've hit EOF and didn't find what we are looking for.
*/
if (nmap == 1) {
+ /*
+ * If we were looking for a hole, set offset to
+ * the end of the file (i.e., there is an implicit
+ * hole at the end of any file).
+ */
+ if (whence == SEEK_HOLE) {
+ offset = isize;
+ break;
+ }
+ /*
+ * If we were looking for data, it's nowhere to be found
+ */
+ ASSERT(whence == SEEK_DATA);
error = -ENXIO;
goto out_unlock;
}
@@ -1260,125 +1284,30 @@ xfs_seek_data(
/*
* Nothing was found, proceed to the next round of search
- * if reading offset not beyond or hit EOF.
+ * if the next reading offset is not at or beyond EOF.
*/
fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
start = XFS_FSB_TO_B(mp, fsbno);
if (start >= isize) {
+ if (whence == SEEK_HOLE) {
+ offset = isize;
+ break;
+ }
+ ASSERT(whence == SEEK_DATA);
error = -ENXIO;
goto out_unlock;
}
}
out:
- offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
-
-out_unlock:
- xfs_iunlock(ip, lock);
-
- if (error)
- return error;
- return offset;
-}
-
-STATIC loff_t
-xfs_seek_hole(
- struct file *file,
- loff_t start)
-{
- 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 end;
- uint lock;
- int error;
-
- if (XFS_FORCED_SHUTDOWN(mp))
- return -EIO;
-
- lock = xfs_ilock_data_map_shared(ip);
-
- isize = i_size_read(inode);
- if (start >= isize) {
- error = -ENXIO;
- goto out_unlock;
- }
-
- fsbno = XFS_B_TO_FSBT(mp, start);
- end = XFS_B_TO_FSB(mp, isize);
-
- for (;;) {
- struct xfs_bmbt_irec map[2];
- int nmap = 2;
- unsigned int i;
-
- error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
- XFS_BMAPI_ENTIRE);
- if (error)
- goto out_unlock;
-
- /* No extents at given offset, must be beyond EOF */
- if (nmap == 0) {
- error = -ENXIO;
- goto out_unlock;
- }
-
- for (i = 0; i < nmap; i++) {
- offset = max_t(loff_t, start,
- XFS_FSB_TO_B(mp, map[i].br_startoff));
-
- /* Landed in a hole */
- if (map[i].br_startblock == HOLESTARTBLOCK)
- goto out;
-
- /*
- * Landed in an unwritten extent, try to search hole
- * from page cache.
- */
- if (map[i].br_state == XFS_EXT_UNWRITTEN) {
- if (xfs_find_get_desired_pgoff(inode, &map[i],
- HOLE_OFF, &offset))
- goto out;
- }
- }
-
- /*
- * map[0] contains data or its unwritten but contains
- * data in page cache, probably means that we are
- * reading after EOF. We should fix offset to point
- * to the end of the file(i.e., there is an implicit
- * hole at the end of any file).
- */
- if (nmap == 1) {
- offset = isize;
- break;
- }
-
- ASSERT(i > 1);
-
- /*
- * Both mappings contains data, proceed to the next round of
- * search if the current reading offset not beyond or hit EOF.
- */
- fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
- start = XFS_FSB_TO_B(mp, fsbno);
- if (start >= isize) {
- offset = isize;
- break;
- }
- }
-
-out:
/*
- * At this point, we must have found a hole. However, the returned
+ * If at this point we have found the hole we wanted, the returned
* offset may be bigger than the file size as it may be aligned to
- * page boundary for unwritten extents, we need to deal with this
+ * page boundary for unwritten extents. We need to deal with this
* situation in particular.
*/
- offset = min_t(loff_t, offset, isize);
+ if (whence == SEEK_HOLE)
+ offset = min_t(loff_t, offset, isize);
offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
out_unlock:
@@ -1400,10 +1329,9 @@ xfs_file_llseek(
case SEEK_CUR:
case SEEK_SET:
return generic_file_llseek(file, offset, origin);
- case SEEK_DATA:
- return xfs_seek_data(file, offset);
case SEEK_HOLE:
- return xfs_seek_hole(file, offset);
+ case SEEK_DATA:
+ return xfs_seek_hole_data(file, offset, origin);
default:
return -EINVAL;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH] xfs: lseek: the "whence" argument is called "whence"
2014-08-21 19:23 ` [PATCH V2] " Eric Sandeen
@ 2014-08-21 19:30 ` Eric Sandeen
2014-08-22 11:35 ` Brian Foster
2014-08-22 11:54 ` Jeff Liu
2014-08-22 11:35 ` [PATCH V2] xfs: combine xfs_seek_hole & xfs_seek_data Brian Foster
2014-08-22 11:50 ` Jeff Liu
2 siblings, 2 replies; 10+ messages in thread
From: Eric Sandeen @ 2014-08-21 19:30 UTC (permalink / raw)
To: Eric Sandeen, xfs-oss, Jeff Liu
For some reason, the older commit:
965c8e5 lseek: the "whence" argument is called "whence"
lseek: the "whence" argument is called "whence"
But the kernel decided to call it "origin" instead.
Fix most of the sites.
left out xfs. So fix xfs.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 1da3b7d..0fe36e4 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1322,16 +1322,16 @@ STATIC loff_t
xfs_file_llseek(
struct file *file,
loff_t offset,
- int origin)
+ int whence)
{
- switch (origin) {
+ switch (whence) {
case SEEK_END:
case SEEK_CUR:
case SEEK_SET:
- return generic_file_llseek(file, offset, origin);
+ return generic_file_llseek(file, offset, whence);
case SEEK_HOLE:
case SEEK_DATA:
- return xfs_seek_hole_data(file, offset, origin);
+ return xfs_seek_hole_data(file, offset, whence);
default:
return -EINVAL;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] xfs: lseek: the "whence" argument is called "whence"
2014-08-21 19:30 ` [PATCH] xfs: lseek: the "whence" argument is called "whence" Eric Sandeen
@ 2014-08-22 11:35 ` Brian Foster
2014-08-22 11:54 ` Jeff Liu
1 sibling, 0 replies; 10+ messages in thread
From: Brian Foster @ 2014-08-22 11:35 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Jeff Liu, Eric Sandeen, xfs-oss
On Thu, Aug 21, 2014 at 02:30:20PM -0500, Eric Sandeen wrote:
> For some reason, the older commit:
>
> 965c8e5 lseek: the "whence" argument is called "whence"
>
> lseek: the "whence" argument is called "whence"
>
> But the kernel decided to call it "origin" instead.
> Fix most of the sites.
>
> left out xfs. So fix xfs.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 1da3b7d..0fe36e4 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1322,16 +1322,16 @@ STATIC loff_t
> xfs_file_llseek(
> struct file *file,
> loff_t offset,
> - int origin)
> + int whence)
> {
> - switch (origin) {
> + switch (whence) {
> case SEEK_END:
> case SEEK_CUR:
> case SEEK_SET:
> - return generic_file_llseek(file, offset, origin);
> + return generic_file_llseek(file, offset, whence);
> case SEEK_HOLE:
> case SEEK_DATA:
> - return xfs_seek_hole_data(file, offset, origin);
> + return xfs_seek_hole_data(file, offset, whence);
> default:
> return -EINVAL;
> }
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] xfs: lseek: the "whence" argument is called "whence"
2014-08-21 19:30 ` [PATCH] xfs: lseek: the "whence" argument is called "whence" Eric Sandeen
2014-08-22 11:35 ` Brian Foster
@ 2014-08-22 11:54 ` Jeff Liu
1 sibling, 0 replies; 10+ messages in thread
From: Jeff Liu @ 2014-08-22 11:54 UTC (permalink / raw)
To: Eric Sandeen, Eric Sandeen; +Cc: xfs-oss
On 08/22/2014 03:30 AM, Eric Sandeen wrote:
> For some reason, the older commit:
>
> 965c8e5 lseek: the "whence" argument is called "whence"
>
> lseek: the "whence" argument is called "whence"
>
> But the kernel decided to call it "origin" instead.
> Fix most of the sites.
>
> left out xfs. So fix xfs.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Looks good to me too.
Reviewed-by: Jie Liu <jeff.liu@oracle.com>
Cheers,
-Jeff
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] xfs: combine xfs_seek_hole & xfs_seek_data
2014-08-21 19:23 ` [PATCH V2] " Eric Sandeen
2014-08-21 19:30 ` [PATCH] xfs: lseek: the "whence" argument is called "whence" Eric Sandeen
@ 2014-08-22 11:35 ` Brian Foster
2014-08-22 11:50 ` Jeff Liu
2 siblings, 0 replies; 10+ messages in thread
From: Brian Foster @ 2014-08-22 11:35 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Jeff Liu, Eric Sandeen, xfs-oss
On Thu, Aug 21, 2014 at 02:23:15PM -0500, Eric Sandeen wrote:
> xfs_seek_hole & xfs_seek_data are remarkably similar;
> so much so that they can be combined, saving a fair
> bit of semi-complex code duplication.
>
> The following patch passes generic/285 and generic/286,
> which specifically test seek behavior.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
Looks good to me:
Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> V2: comment update as suggested by Brian
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 076b170..1da3b7d 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -964,7 +964,7 @@ xfs_vm_page_mkwrite(
>
> /*
> * 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().
> + * to search from page cache for xfs_seek_hole_data().
> */
> enum {
> HOLE_OFF = 0,
> @@ -1021,7 +1021,7 @@ xfs_lookup_buffer_offset(
> /*
> * 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().
> + * type for xfs_seek_hole_data().
> *
> * 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
> @@ -1181,9 +1181,10 @@ out:
> }
>
> STATIC loff_t
> -xfs_seek_data(
> +xfs_seek_hole_data(
> struct file *file,
> - loff_t start)
> + loff_t start,
> + int whence)
> {
> struct inode *inode = file->f_mapping->host;
> struct xfs_inode *ip = XFS_I(inode);
> @@ -1195,6 +1196,9 @@ xfs_seek_data(
> uint lock;
> int error;
>
> + if (XFS_FORCED_SHUTDOWN(mp))
> + return -EIO;
> +
> lock = xfs_ilock_data_map_shared(ip);
>
> isize = i_size_read(inode);
> @@ -1209,6 +1213,7 @@ xfs_seek_data(
> */
> fsbno = XFS_B_TO_FSBT(mp, start);
> end = XFS_B_TO_FSB(mp, isize);
> +
> for (;;) {
> struct xfs_bmbt_irec map[2];
> int nmap = 2;
> @@ -1229,29 +1234,48 @@ xfs_seek_data(
> offset = max_t(loff_t, start,
> XFS_FSB_TO_B(mp, map[i].br_startoff));
>
> - /* Landed in a data extent */
> - if (map[i].br_startblock == DELAYSTARTBLOCK ||
> - (map[i].br_state == XFS_EXT_NORM &&
> - !isnullstartblock(map[i].br_startblock)))
> + /* Landed in the hole we wanted? */
> + if (whence == SEEK_HOLE &&
> + map[i].br_startblock == HOLESTARTBLOCK)
> + goto out;
> +
> + /* Landed in the data extent we wanted? */
> + if (whence == SEEK_DATA &&
> + (map[i].br_startblock == DELAYSTARTBLOCK ||
> + (map[i].br_state == XFS_EXT_NORM &&
> + !isnullstartblock(map[i].br_startblock))))
> goto out;
>
> /*
> - * Landed in an unwritten extent, try to search data
> - * from page cache.
> + * Landed in an unwritten extent, try to search
> + * for hole or data from page cache.
> */
> if (map[i].br_state == XFS_EXT_UNWRITTEN) {
> if (xfs_find_get_desired_pgoff(inode, &map[i],
> - DATA_OFF, &offset))
> + whence == SEEK_HOLE ? HOLE_OFF : DATA_OFF,
> + &offset))
> goto out;
> }
> }
>
> /*
> - * map[0] is hole or its an unwritten extent but
> - * without data in page cache. Probably means that
> - * we are reading after EOF if nothing in map[1].
> + * We only received one extent out of the two requested. This
> + * means we've hit EOF and didn't find what we are looking for.
> */
> if (nmap == 1) {
> + /*
> + * If we were looking for a hole, set offset to
> + * the end of the file (i.e., there is an implicit
> + * hole at the end of any file).
> + */
> + if (whence == SEEK_HOLE) {
> + offset = isize;
> + break;
> + }
> + /*
> + * If we were looking for data, it's nowhere to be found
> + */
> + ASSERT(whence == SEEK_DATA);
> error = -ENXIO;
> goto out_unlock;
> }
> @@ -1260,125 +1284,30 @@ xfs_seek_data(
>
> /*
> * Nothing was found, proceed to the next round of search
> - * if reading offset not beyond or hit EOF.
> + * if the next reading offset is not at or beyond EOF.
> */
> fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
> start = XFS_FSB_TO_B(mp, fsbno);
> if (start >= isize) {
> + if (whence == SEEK_HOLE) {
> + offset = isize;
> + break;
> + }
> + ASSERT(whence == SEEK_DATA);
> error = -ENXIO;
> goto out_unlock;
> }
> }
>
> out:
> - offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
> -
> -out_unlock:
> - xfs_iunlock(ip, lock);
> -
> - if (error)
> - return error;
> - return offset;
> -}
> -
> -STATIC loff_t
> -xfs_seek_hole(
> - struct file *file,
> - loff_t start)
> -{
> - 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 end;
> - uint lock;
> - int error;
> -
> - if (XFS_FORCED_SHUTDOWN(mp))
> - return -EIO;
> -
> - lock = xfs_ilock_data_map_shared(ip);
> -
> - isize = i_size_read(inode);
> - if (start >= isize) {
> - error = -ENXIO;
> - goto out_unlock;
> - }
> -
> - fsbno = XFS_B_TO_FSBT(mp, start);
> - end = XFS_B_TO_FSB(mp, isize);
> -
> - for (;;) {
> - struct xfs_bmbt_irec map[2];
> - int nmap = 2;
> - unsigned int i;
> -
> - error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
> - XFS_BMAPI_ENTIRE);
> - if (error)
> - goto out_unlock;
> -
> - /* No extents at given offset, must be beyond EOF */
> - if (nmap == 0) {
> - error = -ENXIO;
> - goto out_unlock;
> - }
> -
> - for (i = 0; i < nmap; i++) {
> - offset = max_t(loff_t, start,
> - XFS_FSB_TO_B(mp, map[i].br_startoff));
> -
> - /* Landed in a hole */
> - if (map[i].br_startblock == HOLESTARTBLOCK)
> - goto out;
> -
> - /*
> - * Landed in an unwritten extent, try to search hole
> - * from page cache.
> - */
> - if (map[i].br_state == XFS_EXT_UNWRITTEN) {
> - if (xfs_find_get_desired_pgoff(inode, &map[i],
> - HOLE_OFF, &offset))
> - goto out;
> - }
> - }
> -
> - /*
> - * map[0] contains data or its unwritten but contains
> - * data in page cache, probably means that we are
> - * reading after EOF. We should fix offset to point
> - * to the end of the file(i.e., there is an implicit
> - * hole at the end of any file).
> - */
> - if (nmap == 1) {
> - offset = isize;
> - break;
> - }
> -
> - ASSERT(i > 1);
> -
> - /*
> - * Both mappings contains data, proceed to the next round of
> - * search if the current reading offset not beyond or hit EOF.
> - */
> - fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
> - start = XFS_FSB_TO_B(mp, fsbno);
> - if (start >= isize) {
> - offset = isize;
> - break;
> - }
> - }
> -
> -out:
> /*
> - * At this point, we must have found a hole. However, the returned
> + * If at this point we have found the hole we wanted, the returned
> * offset may be bigger than the file size as it may be aligned to
> - * page boundary for unwritten extents, we need to deal with this
> + * page boundary for unwritten extents. We need to deal with this
> * situation in particular.
> */
> - offset = min_t(loff_t, offset, isize);
> + if (whence == SEEK_HOLE)
> + offset = min_t(loff_t, offset, isize);
> offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
>
> out_unlock:
> @@ -1400,10 +1329,9 @@ xfs_file_llseek(
> case SEEK_CUR:
> case SEEK_SET:
> return generic_file_llseek(file, offset, origin);
> - case SEEK_DATA:
> - return xfs_seek_data(file, offset);
> case SEEK_HOLE:
> - return xfs_seek_hole(file, offset);
> + case SEEK_DATA:
> + return xfs_seek_hole_data(file, offset, origin);
> default:
> return -EINVAL;
> }
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V2] xfs: combine xfs_seek_hole & xfs_seek_data
2014-08-21 19:23 ` [PATCH V2] " Eric Sandeen
2014-08-21 19:30 ` [PATCH] xfs: lseek: the "whence" argument is called "whence" Eric Sandeen
2014-08-22 11:35 ` [PATCH V2] xfs: combine xfs_seek_hole & xfs_seek_data Brian Foster
@ 2014-08-22 11:50 ` Jeff Liu
2 siblings, 0 replies; 10+ messages in thread
From: Jeff Liu @ 2014-08-22 11:50 UTC (permalink / raw)
To: Eric Sandeen, Eric Sandeen, xfs-oss
On 08/22/2014 03:23 AM, Eric Sandeen wrote:
> xfs_seek_hole & xfs_seek_data are remarkably similar;
> so much so that they can be combined, saving a fair
> bit of semi-complex code duplication.
>
> The following patch passes generic/285 and generic/286,
> which specifically test seek behavior.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Looks good to me.
Reviewed-by: Jie Liu <jeff.liu@oracle.com>
Cheers,
-Jeff
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 10+ messages in thread