xfs
[Top] [All Lists]

Re: [PATCH RFC] xfs: combine xfs_seek_hole & xfs_seek_data

To: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Subject: Re: [PATCH RFC] xfs: combine xfs_seek_hole & xfs_seek_data
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Thu, 21 Aug 2014 20:37:54 +0800
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <53F55765.6030205@xxxxxxxxxx>
References: <53F55765.6030205@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
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

<Prev in Thread] Current Thread [Next in Thread>