xfs
[Top] [All Lists]

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

To: jeff.liu@xxxxxxxxxx
Subject: Re: [PATCH v5 3/4]xfs: xfs_seek_data() refinements with lookup data buffer offset from page cache.
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Fri, 27 Jul 2012 16:32:22 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <50116322.2020005@xxxxxxxxxx>
References: <50110630.7020902@xxxxxxxxxx> <50116322.2020005@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 07/26/12 10:32, Jeff Liu wrote:
Search data buffer offset for given range from page cache.

Signed-off-by: Jie Liu<jeff.liu@xxxxxxxxxx>


...

This is the same as version 4.

It looks good.

There are a couple places the comment precedes the test and is not yet valid:


                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
+                        * buffer from the page cache before proceeding to
+                        * check the next extent map.  It's a hole if nothing
+                        * was found.
+                        */

or it could be a hole
+                       if (map[0].br_startblock == DELAYSTARTBLOCK ||
+                           map[0].br_state == XFS_EXT_UNWRITTEN) {

here is where it is unwritten extent
+                               /* Probing page cache start from offset */
+                               if (xfs_find_get_desired_pgoff(inode,&map[0],
+                                                       DATA_OFF,&offset))
+                                       break;
+                       }
+
+                       /*
+                        * Found a hole in map[0] and nothing in map[1].
+                        * Probably means that we are reading after EOF.
+                        */

here we did find a hole
+                       if (nmap == 1) {
here is where there is nothing in map[1] and ...
+                               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.
+                                */

it could be unwritten or a hole
+                               if (map[1].br_startblock == DELAYSTARTBLOCK ||
+                                   map[1].br_state == XFS_EXT_UNWRITTEN) {

here is where we know it is unwritten.
+                                       if (xfs_find_get_desired_pgoff(inode,
+                                               &map[1], DATA_OFF,&offset))
+                                               break;
+                               }
+                       }
+               }

I am being really picky, but comments really help a quick read of the code.

Reviewed-by: Mark Tinguely <tinguely@xxxxxxx>

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