All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brian Foster <bfoster@redhat.com>
To: Eric Sandeen <sandeen@sandeen.net>
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 15:30:23 -0400	[thread overview]
Message-ID: <20150914193023.GG34083@bfoster.bfoster> (raw)
In-Reply-To: <55F71F05.90905@sandeen.net>

On Mon, Sep 14, 2015 at 02:24:53PM -0500, Eric Sandeen wrote:
> 
> 
> 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.
> 

Ah, right. Don't worry about this then.

> > 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.
> 

I figured it might (haven't got that far yet), but I'd still rather not
have regressions in the history if we can help it, even if transient.

Brian

> 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

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

  reply	other threads:[~2015-09-14 19:30 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
2015-09-14 19:30       ` Brian Foster [this message]
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=20150914193023.GG34083@bfoster.bfoster \
    --to=bfoster@redhat.com \
    --cc=sandeen@sandeen.net \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.