xfs
[Top] [All Lists]

Re: [PATCH 04/13] xfs_repair: use multibuffer read routines in attr_repa

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 04/13] xfs_repair: use multibuffer read routines in attr_repair.c
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 14 Sep 2015 14:24:53 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150914191816.GD34083@xxxxxxxxxxxxxxx>
References: <1441827251-13128-1-git-send-email-sandeen@xxxxxxxxxxx> <1441827251-13128-5-git-send-email-sandeen@xxxxxxxxxxx> <20150914191816.GD34083@xxxxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:38.0) Gecko/20100101 Thunderbird/38.2.0

On 9/14/15 2:18 PM, Brian Foster wrote:
> On Wed, Sep 09, 2015 at 02:34:02PM -0500, Eric Sandeen wrote:
>> verify_da_path and traverse_int_dablock are similar to
>> verify_dir2_path and traverse_int_dir2block, but one
>> difference is that the dir2 code reads using the
>> multibuffer capable da_read_buf() routine, whereas
>> the attr code doesn't need to, and just calls
>> libxfs_readbuf.
>>
>> The multibuffer code falls back just fine when the
>> geometry indicates that it's not needed, so use that
>> same code in the attribute routines, and remove
>> another dir2 / da difference.  We make da_read_buf()
>> non-static to facilitate this.
>>
>> Finally, add a local *geo to these routines,
>> to make the code even more similar at this point.
>> The geometry will get passed in later in the series.
>>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxxx>
>> ---
>>  repair/attr_repair.c |   49 
>> +++++++++++++++++++++++++++++++++----------------
>>  repair/dir2.c        |   22 +++++++++++++---------
>>  repair/dir2.h        |    8 ++++++++
>>  3 files changed, 54 insertions(+), 25 deletions(-)
>>
>> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
>> index aba0782..fe81df4 100644
>> --- a/repair/attr_repair.c
>> +++ b/repair/attr_repair.c
>> @@ -138,14 +138,20 @@ traverse_int_dablock(xfs_mount_t       *mp,
>>              xfs_dablk_t             *rbno,
>>              int                     whichfork)
>>  {
>> +    bmap_ext_t              *bmp;

Well, yeah - I thought about that.  The goal was to make it as close
as possible to the other code, so I decided against the struct foo's.

I could do a "get rid of typedefs" as the last patch to the new util
code if you like.

> struct bmap_ext
> 
> (and several other places below)
> 
>>      xfs_dablk_t             bno;
>>      int                     i;
>> +    int                     nex;
>>      xfs_da_intnode_t        *node;
>> +    bmap_ext_t              lbmp;
>>      xfs_fsblock_t           fsbno;
>>      xfs_buf_t               *bp;
>> +    struct xfs_da_geometry  *geo;
>>      struct xfs_da_node_entry *btree;
>>      struct xfs_da3_icnode_hdr nodehdr;
>>  
>> +    geo = mp->m_attr_geo;
>> +
>>      /*
>>       * traverse down left-side of tree until we hit the
>>       * left-most leaf block setting up the btree cursor along
>> @@ -160,13 +166,16 @@ traverse_int_dablock(xfs_mount_t       *mp,
>>              /*
>>               * read in each block along the way and set up cursor
>>               */
>> -            fsbno = blkmap_get(da_cursor->blkmap, bno);
>> +            nex = blkmap_getn(da_cursor->blkmap, bno,
>> +                            geo->fsbcount, &bmp, &lbmp);
>>  
> 
> ...
> attr_repair.c: In function âprocess_attributesâ:
> attr_repair.c:185:5: warning: âfsbnoâ may be used uninitialized in this 
> function [-Wmaybe-uninitialized]

hm, whoops.  Well, it goes away in the long run, but I can still fix
that up.

Thanks,
-Eric

>      do_warn(
> ...
> 
> Brian
> 
>> -            if (fsbno == NULLFSBLOCK)
>> +            if (nex == 0)
>>                      goto error_out;
>>  
>> -            bp = libxfs_readbuf(mp->m_dev, XFS_FSB_TO_DADDR(mp, fsbno),
>> -                            XFS_FSB_TO_BB(mp, 1), 0, &xfs_da3_node_buf_ops);
>> +            bp = da_read_buf(mp, nex, bmp, &xfs_da3_node_buf_ops);
>> +            if (bmp != &lbmp)
>> +                    free(bmp);
>> +
>>              if (!bp) {
>>                      if (whichfork == XFS_DATA_FORK)
>>                              do_warn(
>> @@ -192,12 +201,10 @@ traverse_int_dablock(xfs_mount_t       *mp,
>>                      goto error_out;
>>              }
>>  
>> -            if (nodehdr.count > mp->m_attr_geo->node_ents)  {
>> +            if (nodehdr.count > geo->node_ents)  {
>>                      do_warn(_("bad record count in inode %" PRIu64 ", "
>>                                "count = %d, max = %d\n"),
>> -                            da_cursor->ino,
>> -                            nodehdr.count,
>> -                            mp->m_attr_geo->node_ents);
>> +                            da_cursor->ino, nodehdr.count, geo->node_ents);
>>                      libxfs_putbuf(bp);
>>                      goto error_out;
>>              }
>> @@ -492,9 +499,15 @@ verify_da_path(xfs_mount_t      *mp,
>>      int                     bad;
>>      int                     entry;
>>      int                     this_level = p_level + 1;
>> +    bmap_ext_t              *bmp;
>> +    int                     nex;
>> +    bmap_ext_t              lbmp;
>> +    struct xfs_da_geometry  *geo;
>>      struct xfs_da_node_entry *btree;
>>      struct xfs_da3_icnode_hdr nodehdr;
>>  
>> +    geo = mp->m_attr_geo;
>> +
>>      /*
>>       * index is currently set to point to the entry that
>>       * should be processed now in this level.
>> @@ -536,17 +549,21 @@ verify_da_path(xfs_mount_t     *mp,
>>               */
>>              dabno = nodehdr.forw;
>>              ASSERT(dabno != 0);
>> -            fsbno = blkmap_get(cursor->blkmap, dabno);
>> -
>> -            if (fsbno == NULLFSBLOCK) {
>> -                    do_warn(_("can't get map info for block %u "
>> -                              "of directory inode %" PRIu64 "\n"),
>> +            nex = blkmap_getn(cursor->blkmap, dabno, geo->fsbcount,
>> +                    &bmp, &lbmp);
>> +            if (nex == 0) {
>> +                    do_warn(
>> +_("can't get map info for block %u of directory inode %" PRIu64 "\n"),
>>                              dabno, cursor->ino);
>>                      return(1);
>>              }
>>  
>> -            bp = libxfs_readbuf(mp->m_dev, XFS_FSB_TO_DADDR(mp, fsbno),
>> -                            XFS_FSB_TO_BB(mp, 1), 0, &xfs_da3_node_buf_ops);
>> +            fsbno = bmp[0].startblock;
>> +
>> +            bp = da_read_buf(mp, nex, bmp, &xfs_da3_node_buf_ops);
>> +            if (bmp != &lbmp)
>> +                    free(bmp);
>> +
>>              if (!bp) {
>>                      do_warn(
>>      _("can't read block %u (%" PRIu64 ") for directory inode %" PRIu64 
>> "\n"),
>> @@ -577,7 +594,7 @@ verify_da_path(xfs_mount_t       *mp,
>>                              dabno, fsbno, cursor->ino);
>>                      bad++;
>>              }
>> -            if (nodehdr.count > mp->m_attr_geo->node_ents) {
>> +            if (nodehdr.count > geo->node_ents) {
>>                      do_warn(
>>      _("entry count %d too large in block %u (%" PRIu64 ") for directory 
>> inode %" PRIu64 "\n"),
>>                              nodehdr.count,
>> diff --git a/repair/dir2.c b/repair/dir2.c
>> index 54c49eb..44367c6 100644
>> --- a/repair/dir2.c
>> +++ b/repair/dir2.c
>> @@ -92,7 +92,7 @@ namecheck(char *name, int length)
>>   * Multibuffer handling.
>>   * V2 directory blocks can be noncontiguous, needing multiple buffers.
>>   */
>> -static struct xfs_buf *
>> +struct xfs_buf *
>>  da_read_buf(
>>      xfs_mount_t     *mp,
>>      int             nex,
>> @@ -143,9 +143,12 @@ traverse_int_dir2block(xfs_mount_t      *mp,
>>      int                     nex;
>>      xfs_da_intnode_t        *node;
>>      bmap_ext_t              lbmp;
>> +    struct xfs_da_geometry  *geo;
>>      struct xfs_da_node_entry *btree;
>>      struct xfs_da3_icnode_hdr nodehdr;
>>  
>> +    geo = mp->m_dir_geo;
>> +
>>      /*
>>       * traverse down left-side of tree until we hit the
>>       * left-most leaf block setting up the btree cursor along
>> @@ -161,7 +164,7 @@ traverse_int_dir2block(xfs_mount_t       *mp,
>>               * read in each block along the way and set up cursor
>>               */
>>              nex = blkmap_getn(da_cursor->blkmap, bno,
>> -                            mp->m_dir_geo->fsbcount, &bmp, &lbmp);
>> +                            geo->fsbcount, &bmp, &lbmp);
>>  
>>              if (nex == 0)
>>                      goto error_out;
>> @@ -207,13 +210,11 @@ _("corrupt tree block %u for directory inode %" PRIu64 
>> "\n"),
>>                      goto error_out;
>>              }
>>              btree = M_DIROPS(mp)->node_tree_p(node);
>> -            if (nodehdr.count > mp->m_dir_geo->node_ents)  {
>> -                    libxfs_putbuf(bp);
>> +            if (nodehdr.count > geo->node_ents)  {
>>                      do_warn(
>>  _("bad record count in inode %" PRIu64 ", count = %d, max = %d\n"),
>> -                            da_cursor->ino,
>> -                            nodehdr.count,
>> -                            mp->m_dir_geo->node_ents);
>> +                            da_cursor->ino, nodehdr.count, geo->node_ents);
>> +                    libxfs_putbuf(bp);
>>                      goto error_out;
>>              }
>>              /*
>> @@ -488,9 +489,12 @@ verify_dir2_path(xfs_mount_t    *mp,
>>      bmap_ext_t              *bmp;
>>      int                     nex;
>>      bmap_ext_t              lbmp;
>> +    struct xfs_da_geometry  *geo;
>>      struct xfs_da_node_entry *btree;
>>      struct xfs_da3_icnode_hdr nodehdr;
>>  
>> +    geo = mp->m_dir_geo;
>> +
>>      /*
>>       * index is currently set to point to the entry that
>>       * should be processed now in this level.
>> @@ -532,7 +536,7 @@ verify_dir2_path(xfs_mount_t     *mp,
>>               */
>>              dabno = nodehdr.forw;
>>              ASSERT(dabno != 0);
>> -            nex = blkmap_getn(cursor->blkmap, dabno, 
>> mp->m_dir_geo->fsbcount,
>> +            nex = blkmap_getn(cursor->blkmap, dabno, geo->fsbcount,
>>                      &bmp, &lbmp);
>>              if (nex == 0) {
>>                      do_warn(
>> @@ -574,7 +578,7 @@ _("bad back pointer in block %u for directory inode %" 
>> PRIu64 "\n"),
>>                              dabno, cursor->ino);
>>                      bad++;
>>              }
>> -            if (nodehdr.count > mp->m_dir_geo->node_ents)  {
>> +            if (nodehdr.count > geo->node_ents)  {
>>                      do_warn(
>>  _("entry count %d too large in block %u for directory inode %" PRIu64 "\n"),
>>                              nodehdr.count,
>> diff --git a/repair/dir2.h b/repair/dir2.h
>> index 3cc1941..186633f 100644
>> --- a/repair/dir2.h
>> +++ b/repair/dir2.h
>> @@ -58,6 +58,14 @@ typedef struct dir2_bt_cursor  {
>>      struct blkmap           *blkmap;
>>  } dir2_bt_cursor_t;
>>  
>> +#include "bmap.h"   /* Goes away in later refactoring */
>> +struct xfs_buf *
>> +da_read_buf(
>> +    xfs_mount_t     *mp,
>> +    int             nex,
>> +    bmap_ext_t      *bmp,
>> +    const struct xfs_buf_ops *ops);
>> +
>>  int
>>  process_dir2(
>>      xfs_mount_t     *mp,
>> -- 
>> 1.7.1
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@xxxxxxxxxxx
>> http://oss.sgi.com/mailman/listinfo/xfs
> 

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