Hi Andreas,

I only have a few nitpicks...

On 10/04/18 18:39, Andreas Gruenbacher wrote:
Rewrite function hole_size so that it computes the entire size of a
hole.  Previously, the hole size returned wasn't guaranteed to span the
entire hole and multiple invocations of hole_size could be needed to
reach the next bit of data.

Signed-off-by: Andreas Gruenbacher <agrue...@redhat.com>
---
  fs/gfs2/bmap.c | 106 ++++++++++++++++++++++++++++++++++++---------------------
  1 file changed, 67 insertions(+), 39 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 8c25a64d8ae0..14c21825b890 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -279,6 +279,12 @@ static inline __be64 *metapointer(unsigned int height, 
const struct metapath *mp
        return p + mp->mp_list[height];
  }
+static inline const __be64 *metaend(unsigned int height, const struct metapath *mp)
+{
+       const struct buffer_head *bh = mp->mp_bh[height];
+       return (const __be64 *)(bh->b_data + bh->b_size);
+}
+
  static void gfs2_metapath_ra(struct gfs2_glock *gl, __be64 *start, __be64 
*end)
  {
        const __be64 *t;
@@ -631,56 +637,78 @@ static int gfs2_iomap_alloc(struct inode *inode, struct 
iomap *iomap,
   * hole_size - figure out the size of a hole
   * @inode: The inode
   * @lblock: The logical starting block number
- * @mp: The metapath
+ * @mp: The metapath at lblock
+ * @pholesz: The hole size in bytes (output)
+ *
+ * This function modifies @mp.
   *
- * Returns: The hole size in bytes
+ * Returns: errno on error
   *
   */
-static u64 hole_size(struct inode *inode, sector_t lblock, struct metapath *mp)
+static int hole_size(struct inode *inode, sector_t lblock, struct metapath *mp,
+                    u64 *pholesz)
  {
+       sector_t lblock_end = (i_size_read(inode) + i_blocksize(inode) - 1) >>
+                             inode->i_blkbits;
        struct gfs2_inode *ip = GFS2_I(inode);
        struct gfs2_sbd *sdp = GFS2_SB(inode);
-       struct metapath mp_eof;
-       u64 factor = 1;
-       int hgt;
-       u64 holesz = 0;
        const __be64 *first, *end, *ptr;
-       const struct buffer_head *bh;
-       u64 lblock_stop = (i_size_read(inode) - 1) >> inode->i_blkbits;
-       int zeroptrs;
-       bool done = false;
-
-       /* Get another metapath, to the very last byte */
-       find_metapath(sdp, lblock_stop, &mp_eof, ip->i_height);
-       for (hgt = ip->i_height - 1; hgt >= 0 && !done; hgt--) {
-               bh = mp->mp_bh[hgt];
-               if (bh) {
-                       zeroptrs = 0;
-                       first = metapointer(hgt, mp);
-                       end = (const __be64 *)(bh->b_data + bh->b_size);
-
-                       for (ptr = first; ptr < end; ptr++) {
-                               if (*ptr) {
-                                       done = true;
-                                       break;
-                               } else {
-                                       zeroptrs++;
-                               }
+       u64 holesz = 0, factor = 1;
+       int hgt, ret;

hgt could be unsigned for consistency and checking.

+
+       for (hgt = ip->i_height - 1; hgt >= mp->mp_aheight; hgt--)
+               factor *= sdp->sd_inptrs;
+
+       for (;;) {
+               /* Count the number of zero pointers.  */
+               first = metapointer(hgt, mp);
+               end = metaend(hgt, mp);
+               if (end - first > lblock_end - lblock - holesz)
+                       end = first + lblock_end - lblock - holesz;
+               for (ptr = first; ptr < end; ptr++) {
+                       if (*ptr) {
+                               holesz += (ptr - first) * factor;
+                               if (hgt == ip->i_height - 1)
+                                       goto out;
+                               mp->mp_list[hgt] += (ptr - first);
+                               goto fill_up_metapath;
                        }
-               } else {
-                       zeroptrs = sdp->sd_inptrs;
                }
-               if (factor * zeroptrs >= lblock_stop - lblock + 1) {
-                       holesz = lblock_stop - lblock + 1;
-                       break;
+               holesz += (ptr - first) * factor;
+
+lower_metapath:
+               /* Decrease height of metapath. */
+               brelse(mp->mp_bh[hgt]);
+               mp->mp_bh[hgt] = NULL;
+               mp->mp_list[hgt] = 0;
+               if (!hgt)
+                       goto out;
+               hgt--;
+               factor *= sdp->sd_inptrs;
+
+               /* Advance in metadata tree. */
+               (mp->mp_list[hgt])++;
+               first = metapointer(hgt, mp);
+               end = metaend(hgt, mp);
+               if (first >= end) {
+                       if (!hgt)
+                               goto out;
+                       goto lower_metapath;
                }
-               holesz += factor * zeroptrs;
- factor *= sdp->sd_inptrs;
-               if (hgt && (mp->mp_list[hgt - 1] < mp_eof.mp_list[hgt - 1]))
-                       (mp->mp_list[hgt - 1])++;
+fill_up_metapath:
+               /* Fill up metapath. */

Comment could be removed as the function name and the label are the same as the comment :)

+               ret = fillup_metapath(ip, mp, ip->i_height - 1);
+               if (ret < 0)
+                       return ret;
+               while (ret--) {
+                       hgt++;

Could be taken out of the loop as hgt += ret

The flow is a little hard to follow but I don't see any obvious problems. Has it been tested with directories as well as files?

Reviewed-by: Andrew Price <anpr...@redhat.com>

Andy

+                       factor /= sdp->sd_inptrs;
+               }
        }
-       return holesz << inode->i_blkbits;
+out:
+       *pholesz = holesz << inode->i_blkbits;
+       return 0;
  }
static void gfs2_stuffed_iomap(struct inode *inode, struct iomap *iomap)
@@ -804,7 +832,7 @@ int gfs2_iomap_begin(struct inode *inode, loff_t pos, 
loff_t length,
                if (pos >= size)
                        ret = -ENOENT;
                else if (height <= ip->i_height)
-                       iomap->length = hole_size(inode, lblock, &mp);
+                       ret = hole_size(inode, lblock, &mp, &iomap->length);
                else
                        iomap->length = size - pos;
        }


Reply via email to