public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Eric Sandeen <sandeen@sandeen.net>
To: Brian Foster <bfoster@redhat.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 04/13] xfs_repair: use multibuffer read routines in attr_repair.c
Date: Mon, 14 Sep 2015 14:24:53 -0500	[thread overview]
Message-ID: <55F71F05.90905@sandeen.net> (raw)
In-Reply-To: <20150914191816.GD34083@bfoster.bfoster>



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@redhat.com>
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>> ---
>>  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@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

  reply	other threads:[~2015-09-14 19:24 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-09 19:33 [PATCH 0/13] xfs_repair: recombine cut&waste code in dir2.c/attr_repair.c Eric Sandeen
2015-09-09 19:33 ` [PATCH 01/13] xfs_repair: remove trace-only 'n' member from da_level_state Eric Sandeen
2015-09-14 19:17   ` Brian Foster
2015-09-09 19:34 ` [PATCH 02/13] xfs_repair: remove type from da & dir2 cursors Eric Sandeen
2015-09-14 19:18   ` Brian Foster
2015-09-09 19:34 ` [PATCH 03/13] xfs_repair: make CRC checking consistent in path verification Eric Sandeen
2015-09-14 19:18   ` Brian Foster
2015-09-09 19:34 ` [PATCH 04/13] xfs_repair: use multibuffer read routines in attr_repair.c Eric Sandeen
2015-09-14 19:18   ` Brian Foster
2015-09-14 19:24     ` Eric Sandeen [this message]
2015-09-14 19:30       ` Brian Foster
2015-09-09 19:34 ` [PATCH 05/13] xfs_repair: fix use-after-free in verify_final_dir2_path Eric Sandeen
2015-09-14 19:18   ` Brian Foster
2015-09-09 19:34 ` [PATCH 06/13] xfs_repair: add XR_DIR_TRACE to dir2.c Eric Sandeen
2015-09-14 19:18   ` Brian Foster
2015-09-09 19:34 ` [PATCH 07/13] xfs_repair: Remove BUF_PTR from attr_repair.c Eric Sandeen
2015-09-14 19:44   ` Brian Foster
2015-09-09 19:34 ` [PATCH 08/13] xfs_repair: catch bad level/depth in da node Eric Sandeen
2015-09-14 19:44   ` Brian Foster
2015-09-09 19:34 ` [PATCH 09/13] xfs_repair: better checking of v5 attributes Eric Sandeen
2015-09-14 19:44   ` Brian Foster
2015-09-23 17:53     ` Eric Sandeen
2015-09-09 19:34 ` [PATCH 10/13] xfs_repair: Remove more differences between attr & dir2 Eric Sandeen
2015-09-14 19:55   ` Brian Foster
2015-09-09 19:34 ` [PATCH 11/13] xfs_repair: whitespace & comments Eric Sandeen
2015-09-14 19:56   ` Brian Foster
2015-09-09 19:34 ` [PATCH 12/13] xfs_repair: move common dir2 and attr_repair code to da_util.c Eric Sandeen
2015-09-09 19:34 ` [PATCH 13/13] xfs_repair: Fix up warning strings in da_util.c Eric Sandeen
2015-09-14 20:06   ` Brian Foster
2015-09-14 20:11     ` Eric Sandeen
2015-09-10  9:22 ` [PATCH 0/13] xfs_repair: recombine cut&waste code in dir2.c/attr_repair.c Carlos Maiolino
2015-09-10 16:51   ` Eric Sandeen
2015-09-11  8:20     ` Carlos Maiolino

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55F71F05.90905@sandeen.net \
    --to=sandeen@sandeen.net \
    --cc=bfoster@redhat.com \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox