[Cluster-devel] [PATCH] gfs2: hole_size improvement

[Andreas Gruenbacher <agruenba@redhat.com>]

Hi Andy,

On 11 April 2018 at 15:00, Andrew Price <anprice@redhat.com> wrote:
> 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 <agruenba@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.

Right, it can't go negative (anymore).

>> +
>> +       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 :)

Ok.

>> +               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?

No, I need to think the directory case through some more. The only
"hole" in directories is at the end of the file though.

> Reviewed-by: Andrew Price <anprice@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;
>>         }

Thanks,
Andreas