xfs
[Top] [All Lists]

Re: [PATCH v6 3/4] xfs: xfs_seek_data() refinement with lookup data buff

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v6 3/4] xfs: xfs_seek_data() refinement with lookup data buffer offset from page cache
From: Jie Liu <jeff.liu@xxxxxxxxxx>
Date: Fri, 10 Aug 2012 16:17:56 +0800
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120809233747.GC2877@dastard>
References: <501B6B7B.5040602@xxxxxxxxxx> <20120809233747.GC2877@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120714 Thunderbird/14.0
On 08/10/12 07:37, Dave Chinner wrote:
> On Fri, Aug 03, 2012 at 02:11:07PM +0800, Jeff Liu wrote:
>> Refine xfs_seek_data() to check up data buffer offset from page cache for 
>> unwritten extents.
>>
>>
>> Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
>>
>> ---
>>  fs/xfs/xfs_file.c |   77 
>> ++++++++++++++++++++++++++++++++++++++++------------
>>  1 files changed, 59 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index aff0c30..e852e52 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -1187,8 +1187,6 @@ xfs_seek_data(
>>      struct inode            *inode = file->f_mapping->host;
>>      struct xfs_inode        *ip = XFS_I(inode);
>>      struct xfs_mount        *mp = ip->i_mount;
>> -    struct xfs_bmbt_irec    map[2];
>> -    int                     nmap = 2;
>>      loff_t                  uninitialized_var(offset);
>>      xfs_fsize_t             isize;
>>      xfs_fileoff_t           fsbno;
>> @@ -1204,34 +1202,77 @@ xfs_seek_data(
>>              goto out_unlock;
>>      }
>>  
>> -    fsbno = XFS_B_TO_FSBT(mp, start);
>> -
>>      /*
>>       * Try to read extents from the first block indicated
>>       * by fsbno to the end block of the file.
>>       */
>> +    fsbno = XFS_B_TO_FSBT(mp, start);
>>      end = XFS_B_TO_FSB(mp, isize);
>> +    for (;;) {
>> +            struct xfs_bmbt_irec    map[2];
>> +            int                     nmap = 2;
>>  
>> -    error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
>> -                           XFS_BMAPI_ENTIRE);
>> -    if (error)
>> -            goto out_unlock;
>> +            error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
>> +                                   XFS_BMAPI_ENTIRE);
>> +            if (error)
>> +                    goto out_unlock;
>>  
>> -    /*
>> -     * Treat unwritten extent as data extent since it might
>> -     * contains dirty data in page cache.
>> -     */
>> -    if (map[0].br_startblock != HOLESTARTBLOCK) {
>> -            offset = max_t(loff_t, start,
>> -                           XFS_FSB_TO_B(mp, map[0].br_startoff));
>> -    } else {
>> +            /* No extents at given offset, must be beyond EOF */
>> +            if (nmap == 0) {
>> +                    error = ENXIO;
>> +                    goto out_unlock;
>> +            }
>> +
>> +            offset = start;
>> +            /* Landed in a data extent */
>> +            if (map[0].br_startblock == DELAYSTARTBLOCK ||
>> +                (map[0].br_state == XFS_EXT_NORM &&
>> +                 !isnullstartblock(map[0].br_startblock)))
>> +                    break;
>> +
>> +            /*
>> +             * Landed in an unwritten extent, try to search data
>> +             * from page cache.
>> +             */
>> +            if (map[0].br_state == XFS_EXT_UNWRITTEN) {
>> +                    if (xfs_find_get_desired_pgoff(inode, &map[0],
>> +                                                   DATA_OFF, &offset))
>> +                            break;
>> +            }
>> +
>> +            /*
>> +             * 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) {
>>                      error = ENXIO;
>>                      goto out_unlock;
>>              }
>>  
>> -            offset = max_t(loff_t, start,
>> -                           XFS_FSB_TO_B(mp, map[1].br_startoff));
>> +            /* We have two mappings, proceed to check map[1] */
>> +            offset = XFS_FSB_TO_B(mp, map[1].br_startoff);
>> +            if (map[1].br_startblock == DELAYSTARTBLOCK ||
>> +                (map[1].br_state == XFS_EXT_NORM &&
>> +                 !isnullstartblock(map[1].br_startblock)))
>> +                    break;
>> +
>> +            if (map[1].br_state == XFS_EXT_UNWRITTEN) {
>> +                    if (xfs_find_get_desired_pgoff(inode, &map[1],
>> +                                            DATA_OFF, &offset))
>> +                            break;
>> +            }
> That's now duplicate code, so a loop would be much better to make it
> simpler to maintain an enhance in future:
>
>               for (i = 0; i < nmap; i++) {
>                       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)))
>                               break;
>
>                       /*
>                        * Landed in an unwritten extent, try to search data
>                        * from page cache.
>                        */
>                       if (map[i].br_state == XFS_EXT_UNWRITTEN) {
>                               if (xfs_find_get_desired_pgoff(inode, &map[i],
>                                                              DATA_OFF, 
> &offset))
>                                       break;
>                       }
>               }
>
>               /*
>                * 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) {
>                       error = ENXIO;
>                       goto out_unlock;
>               }
It looks very nice to wrap up those check ups in loop.

Thanks,
-Jeff
>
>> +
>> +            /*
>> +             * Nothing was found, proceed to the next round of search
>> +             * if reading offset not beyond or hit EOF.
>> +             */
>> +            fsbno = map[1].br_startoff + map[1].br_blockcount;
>               fsbno = map[i].br_startoff + map[i].br_blockcount;
>
>> +            start = XFS_FSB_TO_B(mp, fsbno);
>> +            if (start >= isize) {
>> +                    error = ENXIO;
>> +                    goto out_unlock;
>> +            }
>>      }
> Otherwise this looks just fine.
>
> Cheers,
>
> Dave.

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