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@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
