xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
From: Jie Liu <jeff.liu@xxxxxxxxxx>
Date: Tue, 31 Jul 2012 16:03:09 +0800
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120731000606.GM2877@dastard>
References: <50110630.7020902@xxxxxxxxxx> <50116322.2020005@xxxxxxxxxx> <20120731000606.GM2877@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120714 Thunderbird/14.0
On 07/31/12 08:06, Dave Chinner wrote:
On Thu, Jul 26, 2012 at 11:32:50PM +0800, Jeff Liu wrote:
Search data buffer offset for given range from page cache.

Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
Reviewed-by: Mark Tinguely <tinguely@xxxxxxx>
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

---
  fs/xfs/xfs_file.c |   88 ++++++++++++++++++++++++++++++++++++++++++-----------
  1 files changed, 70 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 69965a4..b1158b3 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1182,8 +1182,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;
@@ -1199,34 +1197,88 @@ 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 {
-               if (nmap == 1) {
+               /* No extents at given offset, must be beyond EOF */
+               if (nmap == 0) {
                        error = ENXIO;
                        goto out_unlock;
                }
offset = max_t(loff_t, start,
-                              XFS_FSB_TO_B(mp, map[1].br_startoff));
+                              XFS_FSB_TO_B(mp, map[0].br_startoff));
+               if (map[0].br_state == XFS_EXT_NORM &&
+                   !isnullstartblock(map[0].br_startblock))
+                       break;
+               else {
+                       /*
+                        * Landed in an unwritten extent, try to lookup data
Not correct - hole, delay or unwritten land here.
Will fix it.

+                        * buffer from the page cache before proceeding to
+                        * check the next extent map.  It's a hole if nothing
+                        * was found.
+                        */
+                       if (map[0].br_startblock == DELAYSTARTBLOCK ||
+                           map[0].br_state == XFS_EXT_UNWRITTEN) {
+                               /* Probing page cache start from offset */
+                               if (xfs_find_get_desired_pgoff(inode, &map[0],
+                                                       DATA_OFF, &offset))
+                                       break;
+                       }
If is is a DELAYSTARTBLOCK, and it is within EOF, then it is
guaranteed to contain data. There is no need for a page cache lookup
to decide where the data starts - by definition the data starts at
map[0].br_startblock. I think you can treat DELAYSTARTBLOCK exactly
the same as allocated XFS_EXT_NORM extents.

That means the logic is:

                if (map[0].br_state == XFS_EXT_NORM &&
                    (!isnullstartblock(map[0].br_startblock) ||
                     map[0].br_startblock == DELAYSTARTBLOCK)) {
                        break;
                } else if (map[0].br_state == XFS_EXT_UNWRITTEN) {
                        /* Probing page cache start from offset */
                        if (xfs_find_get_desired_pgoff(inode, &map[0],
                                                DATA_OFF, &offset))
                                break;
                } else {
                        /*
                         * If we find a hole in map[0] and nothing in map[1] it
                         * probably means that we are reading after EOF.
                         */
                         .....
                }

Which kills a level of indentation in the code....
Just done a quick test with above modifications, it works to me, thanks.


+                       /*
+                        * Found a hole in map[0] and nothing in map[1].
+                        * Probably means that we are reading after EOF.
+                        */
+                       if (nmap == 1) {
+                               error = ENXIO;
+                               goto out_unlock;
+                       }
+
+                       /*
+                        * We have two mappings, proceed to check map[1].
+                        */
+                       offset = max_t(loff_t, start,
+                                      XFS_FSB_TO_B(mp, map[1].br_startoff));
+                       if (map[1].br_state == XFS_EXT_NORM &&
+                           !isnullstartblock(map[1].br_startblock))
+                               break;
+                       else {
+                               /*
+                                * map[1] is also an unwritten extent, lookup
+                                * data buffer from page cache now.
+                                */
+                               if (map[1].br_startblock == DELAYSTARTBLOCK ||
+                                   map[1].br_state == XFS_EXT_UNWRITTEN) {
+                                       if (xfs_find_get_desired_pgoff(inode,
+                                               &map[1], DATA_OFF, &offset))
+                                               break;
+                               }
+                       }
And the if/elseif/else logic above can be repeated here.

+               }
+
+               /*
+                * 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;
+               start = XFS_FSB_TO_B(mp, fsbno);
+               if (start >= isize) {
+                       error = ENXIO;
+                       goto out_unlock;
+               }
Shouldn't this check be done at the start of the loop?
If put this check at the beginning of the loop, the code block would looks like below:

    isize = i_size_read(inode);
        if (start >= isize) {
                error = ENXIO;
                goto out_unlock;
        }

    .......
    for (;;) {
                struct xfs_bmbt_irec    map[2];
                int                     nmap = 2;

                if (start >= isize) {
                        error = ENXIO;
                        goto out_unlock;
                }

                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;
                }

                ..........
                .....
                /*
* 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;
                start = XFS_FSB_TO_B(mp, fsbno);
    }

But we have a similar check up at the begnning of xfs_seek_data(), will it looks a bit weird?

Thanks,
-Jeff

Cheers,

Dave.

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