[PATCH v2 0/6] xfs-4.19: various fixes

[PATCH v2 0/6] xfs-4.19: various fixes

[Darrick J. Wong]

Hi all,

Here are some late fixes for 4.19.

The first patch teaches xfs to trigger a sb counter recalculation if the
icount is bad; when I added the icount check to the write verifier, I
forgot to add the corresponding check to the read side.

The second patch fixes the online repair "find AG btree root" function
to ignore btree blocks that have siblings and to ignore a btree level if
multiple sibling-less blocks are found.

The third patch fixes a problem when allocating a transaction for online
repairs where if the AGF block or AGI inode counts are totally garbage
we can end up ENOSPC'ing out of online repair after an attempt to
reserve a ridiculous number of blocks.  This is unfortunate,
particularly if we were setting up for an AGF/AGI counter repair.

The fourth patch fixes some buffer state management bugs so that we
don't accidentally clobber b_ops on buffers that were already in-core
when we try to find an AG header's btree root blocks.

The fifth patch fixes an uninitialized variable usage in the iomap code.

The sixth patch avoids a crash in the VFS readlink routines if we have
an inline symlink that's corrupted such that if_data is NULL.

Comments and questions are, as always, welcome.

--D

[PATCH 1/6] xfs: recalculate summary counters at mount time if icount is bad

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Since the sb write verifier trips on bad icounts, we should also force a
mount time recalculation of the summary counters if the icount is bad.
This helps us avoid blowing up at freeze/unmount time when the bad
counter gets written back out.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_mount.c |    1 +
 1 file changed, 1 insertion(+)


diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 99db27d6ac8a..02d15098dbee 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -637,6 +637,7 @@ xfs_check_summary_counts(
 	 */
 	if (XFS_LAST_UNMOUNT_WAS_CLEAN(mp) &&
 	    (mp->m_sb.sb_fdblocks > mp->m_sb.sb_dblocks ||
+	     !xfs_verify_icount(mp, mp->m_sb.sb_icount) ||
 	     mp->m_sb.sb_ifree > mp->m_sb.sb_icount))
 		mp->m_flags |= XFS_MOUNT_BAD_SUMMARY;
 

Re: [PATCH 1/6] xfs: recalculate summary counters at mount time if icount is bad

[Allison Henderson]

On 08/11/2018 08:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since the sb write verifier trips on bad icounts, we should also force a
> mount time recalculation of the summary counters if the icount is bad.
> This helps us avoid blowing up at freeze/unmount time when the bad
> counter gets written back out.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_mount.c |    1 +
>   1 file changed, 1 insertion(+)
> 
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 99db27d6ac8a..02d15098dbee 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -637,6 +637,7 @@ xfs_check_summary_counts(
>   	 */
>   	if (XFS_LAST_UNMOUNT_WAS_CLEAN(mp) &&
>   	    (mp->m_sb.sb_fdblocks > mp->m_sb.sb_dblocks ||
> +	     !xfs_verify_icount(mp, mp->m_sb.sb_icount) ||
>   	     mp->m_sb.sb_ifree > mp->m_sb.sb_icount))
>   		mp->m_flags |= XFS_MOUNT_BAD_SUMMARY;
>   
> 
Ok, looks good

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Re: [PATCH 1/6] xfs: recalculate summary counters at mount time if icount is bad

[Carlos Maiolino]

On Sat, Aug 11, 2018 at 08:35:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Since the sb write verifier trips on bad icounts, we should also force a
> mount time recalculation of the summary counters if the icount is bad.
> This helps us avoid blowing up at freeze/unmount time when the bad
> counter gets written back out.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_mount.c |    1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 99db27d6ac8a..02d15098dbee 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -637,6 +637,7 @@ xfs_check_summary_counts(
>  	 */
>  	if (XFS_LAST_UNMOUNT_WAS_CLEAN(mp) &&
>  	    (mp->m_sb.sb_fdblocks > mp->m_sb.sb_dblocks ||
> +	     !xfs_verify_icount(mp, mp->m_sb.sb_icount) ||
>  	     mp->m_sb.sb_ifree > mp->m_sb.sb_icount))
>  		mp->m_flags |= XFS_MOUNT_BAD_SUMMARY;
>  
> 

-- 
Carlos

[PATCH 2/6] xfs: xrep_findroot_block should reject root blocks with siblings

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In xrep_findroot_block, if we find a candidate root block with sibling
pointers or sibling blocks on the same tree level, we should not return
that block as a tree root because root blocks cannot have siblings.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/repair.c |   47 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)


diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 17cf48564390..42d8c798ce7d 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -690,6 +690,7 @@ xrep_findroot_block(
 	struct xfs_buf			*bp;
 	struct xfs_btree_block		*btblock;
 	xfs_daddr_t			daddr;
+	int				block_level;
 	int				error;
 
 	daddr = XFS_AGB_TO_DADDR(mp, ri->sc->sa.agno, agbno);
@@ -727,17 +728,51 @@ xrep_findroot_block(
 		goto out;
 	bp->b_ops = fab->buf_ops;
 
-	/* Ignore this block if it's lower in the tree than we've seen. */
-	if (fab->root != NULLAGBLOCK &&
-	    xfs_btree_get_level(btblock) < fab->height)
-		goto out;
-
 	/* Make sure we pass the verifiers. */
 	bp->b_ops->verify_read(bp);
 	if (bp->b_error)
 		goto out;
+
+	/* If we've recorded a root candidate... */
+	block_level = xfs_btree_get_level(btblock);
+	if (fab->root != NULLAGBLOCK) {
+		/*
+		 * ...and this no-sibling root block candidate has the same
+		 * level as the recorded candidate, there's no way we're going
+		 * to accept any candidates at this tree level.  Stash a root
+		 * block of zero because the height is still valid, but no
+		 * AG btree can root at agblock 0.  Callers should verify the
+		 * root agbno with xfs_verify_agbno...
+		 */
+		if (block_level + 1 == fab->height) {
+			fab->root = 0;
+			goto out;
+		}
+
+		/*
+		 * ...and this no-sibling root block is lower in the tree than
+		 * the recorded root block candidate, just ignore it.  There's
+		 * still a strong chance that something is wrong with the btree
+		 * itself, but that's not what we're fixing right now.
+		 */
+		if (block_level < fab->height)
+			goto out;
+	}
+
+	/*
+	 * Root blocks can't have siblings.  This level can't be the root, so
+	 * record the tree height (but not the ag block pointer) to force us to
+	 * look for a higher level in the tree.
+	 */
+	if (btblock->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK) ||
+	    btblock->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK)) {
+		fab->root = 0;
+		fab->height = block_level + 1;
+		goto out;
+	}
+
 	fab->root = agbno;
-	fab->height = xfs_btree_get_level(btblock) + 1;
+	fab->height = block_level + 1;
 	*found_it = true;
 
 	trace_xrep_findroot_block(mp, ri->sc->sa.agno, agbno,

Re: [PATCH 2/6] xfs: xrep_findroot_block should reject root blocks with siblings

[Allison Henderson]

On 08/11/2018 08:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xrep_findroot_block, if we find a candidate root block with sibling
> pointers or sibling blocks on the same tree level, we should not return
> that block as a tree root because root blocks cannot have siblings.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/scrub/repair.c |   47 +++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 41 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 17cf48564390..42d8c798ce7d 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -690,6 +690,7 @@ xrep_findroot_block(
>   	struct xfs_buf			*bp;
>   	struct xfs_btree_block		*btblock;
>   	xfs_daddr_t			daddr;
> +	int				block_level;
>   	int				error;
>   
>   	daddr = XFS_AGB_TO_DADDR(mp, ri->sc->sa.agno, agbno);
> @@ -727,17 +728,51 @@ xrep_findroot_block(
>   		goto out;
>   	bp->b_ops = fab->buf_ops;
>   
> -	/* Ignore this block if it's lower in the tree than we've seen. */
> -	if (fab->root != NULLAGBLOCK &&
> -	    xfs_btree_get_level(btblock) < fab->height)
> -		goto out;
> -
>   	/* Make sure we pass the verifiers. */
>   	bp->b_ops->verify_read(bp);
>   	if (bp->b_error)
>   		goto out;
> +
> +	/* If we've recorded a root candidate... */
> +	block_level = xfs_btree_get_level(btblock);
> +	if (fab->root != NULLAGBLOCK) {
> +		/*
> +		 * ...and this no-sibling root block candidate has the same
> +		 * level as the recorded candidate, there's no way we're going
> +		 * to accept any candidates at this tree level.  Stash a root
> +		 * block of zero because the height is still valid, but no
> +		 * AG btree can root at agblock 0.  Callers should verify the
> +		 * root agbno with xfs_verify_agbno...
> +		 */
> +		if (block_level + 1 == fab->height) {
> +			fab->root = 0;
> +			goto out;
> +		}
> +
> +		/*
> +		 * ...and this no-sibling root block is lower in the tree than
> +		 * the recorded root block candidate, just ignore it.  There's
> +		 * still a strong chance that something is wrong with the btree
> +		 * itself, but that's not what we're fixing right now.
> +		 */
> +		if (block_level < fab->height)
> +			goto out;
> +	}
> +
> +	/*
> +	 * Root blocks can't have siblings.  This level can't be the root, so
> +	 * record the tree height (but not the ag block pointer) to force us to
> +	 * look for a higher level in the tree.
> +	 */
> +	if (btblock->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK) ||
> +	    btblock->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK)) {
> +		fab->root = 0;
> +		fab->height = block_level + 1;
> +		goto out;
> +	}
> +
>   	fab->root = agbno;
> -	fab->height = xfs_btree_get_level(btblock) + 1;
> +	fab->height = block_level + 1;
>   	*found_it = true;
>   
>   	trace_xrep_findroot_block(mp, ri->sc->sa.agno, agbno,
> 
Looks ok, thank you for the comments!
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Re: [PATCH 2/6] xfs: xrep_findroot_block should reject root blocks with siblings

[Carlos Maiolino]

On Sat, Aug 11, 2018 at 08:35:09AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xrep_findroot_block, if we find a candidate root block with sibling
> pointers or sibling blocks on the same tree level, we should not return
> that block as a tree root because root blocks cannot have siblings.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/repair.c |   47 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 6 deletions(-)

Looks fine

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 17cf48564390..42d8c798ce7d 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -690,6 +690,7 @@ xrep_findroot_block(
>  	struct xfs_buf			*bp;
>  	struct xfs_btree_block		*btblock;
>  	xfs_daddr_t			daddr;
> +	int				block_level;
>  	int				error;
>  
>  	daddr = XFS_AGB_TO_DADDR(mp, ri->sc->sa.agno, agbno);
> @@ -727,17 +728,51 @@ xrep_findroot_block(
>  		goto out;
>  	bp->b_ops = fab->buf_ops;
>  
> -	/* Ignore this block if it's lower in the tree than we've seen. */
> -	if (fab->root != NULLAGBLOCK &&
> -	    xfs_btree_get_level(btblock) < fab->height)
> -		goto out;
> -
>  	/* Make sure we pass the verifiers. */
>  	bp->b_ops->verify_read(bp);
>  	if (bp->b_error)
>  		goto out;
> +
> +	/* If we've recorded a root candidate... */
> +	block_level = xfs_btree_get_level(btblock);
> +	if (fab->root != NULLAGBLOCK) {
> +		/*
> +		 * ...and this no-sibling root block candidate has the same
> +		 * level as the recorded candidate, there's no way we're going
> +		 * to accept any candidates at this tree level.  Stash a root
> +		 * block of zero because the height is still valid, but no
> +		 * AG btree can root at agblock 0.  Callers should verify the
> +		 * root agbno with xfs_verify_agbno...
> +		 */
> +		if (block_level + 1 == fab->height) {
> +			fab->root = 0;
> +			goto out;
> +		}
> +
> +		/*
> +		 * ...and this no-sibling root block is lower in the tree than
> +		 * the recorded root block candidate, just ignore it.  There's
> +		 * still a strong chance that something is wrong with the btree
> +		 * itself, but that's not what we're fixing right now.
> +		 */
> +		if (block_level < fab->height)
> +			goto out;
> +	}
> +
> +	/*
> +	 * Root blocks can't have siblings.  This level can't be the root, so
> +	 * record the tree height (but not the ag block pointer) to force us to
> +	 * look for a higher level in the tree.
> +	 */
> +	if (btblock->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK) ||
> +	    btblock->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK)) {
> +		fab->root = 0;
> +		fab->height = block_level + 1;
> +		goto out;
> +	}
> +
>  	fab->root = agbno;
> -	fab->height = xfs_btree_get_level(btblock) + 1;
> +	fab->height = block_level + 1;
>  	*found_it = true;
>  
>  	trace_xrep_findroot_block(mp, ri->sc->sa.agno, agbno,
> 

-- 
Carlos

Re: [PATCH 2/6] xfs: xrep_findroot_block should reject root blocks with siblings

[Brian Foster]

On Sat, Aug 11, 2018 at 08:35:09AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xrep_findroot_block, if we find a candidate root block with sibling
> pointers or sibling blocks on the same tree level, we should not return
> that block as a tree root because root blocks cannot have siblings.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/repair.c |   47 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 17cf48564390..42d8c798ce7d 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -690,6 +690,7 @@ xrep_findroot_block(
>  	struct xfs_buf			*bp;
>  	struct xfs_btree_block		*btblock;
>  	xfs_daddr_t			daddr;
> +	int				block_level;
>  	int				error;
>  
>  	daddr = XFS_AGB_TO_DADDR(mp, ri->sc->sa.agno, agbno);
> @@ -727,17 +728,51 @@ xrep_findroot_block(
>  		goto out;
>  	bp->b_ops = fab->buf_ops;
>  
> -	/* Ignore this block if it's lower in the tree than we've seen. */
> -	if (fab->root != NULLAGBLOCK &&
> -	    xfs_btree_get_level(btblock) < fab->height)
> -		goto out;
> -
>  	/* Make sure we pass the verifiers. */
>  	bp->b_ops->verify_read(bp);
>  	if (bp->b_error)
>  		goto out;
> +
> +	/* If we've recorded a root candidate... */
> +	block_level = xfs_btree_get_level(btblock);
> +	if (fab->root != NULLAGBLOCK) {
> +		/*
> +		 * ...and this no-sibling root block candidate has the same
> +		 * level as the recorded candidate, there's no way we're going
> +		 * to accept any candidates at this tree level.  Stash a root
> +		 * block of zero because the height is still valid, but no
> +		 * AG btree can root at agblock 0.  Callers should verify the
> +		 * root agbno with xfs_verify_agbno...
> +		 */
> +		if (block_level + 1 == fab->height) {
> +			fab->root = 0;
> +			goto out;
> +		}
> +
> +		/*
> +		 * ...and this no-sibling root block is lower in the tree than
> +		 * the recorded root block candidate, just ignore it.  There's
> +		 * still a strong chance that something is wrong with the btree
> +		 * itself, but that's not what we're fixing right now.
> +		 */
> +		if (block_level < fab->height)
> +			goto out;
> +	}
> +
> +	/*
> +	 * Root blocks can't have siblings.  This level can't be the root, so
> +	 * record the tree height (but not the ag block pointer) to force us to
> +	 * look for a higher level in the tree.
> +	 */
> +	if (btblock->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK) ||
> +	    btblock->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK)) {
> +		fab->root = 0;
> +		fab->height = block_level + 1;
> +		goto out;
> +	}
> +

Hmm, this looks technically correct but this still seems more involved
than necessary. I don't really like how we define yet another magic
value of 0, for example, and differentiate that from NULLAGBLOCK when it
seems like we could just check the height.

Could we do something like the following (which assumes ->height is
initialized to 0)?

        /*
         * If current height exceeds the block level, we've already seen at
         * least one block at this level or higher. Skip it and invalidate the
         * root if this block matches the current root level because a root
         * block has no siblings.
         */
        block_level = xfs_btree_get_level(btblock);
        if (fab->height > block_level) {
                if (fab->height - 1 == block_level)
                        fab->root = NULLAGBLOCK;
                goto out;
        }

        /*
         * Found a block with a new max height. Track it as a root candidate if
         * it has no siblings. Otherwise invalidate root since we know the root
         * can't be at this level.
         */
        if (btblock->bb_u.s.bb_leftsib == cpu_to_be32(NULLAGBLOCK) &&
            btblock->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK))
                fab->root = agbno;
        else
                fab->root = NULLAGBLOCK;
        fab->height = block_level + 1;

... and then ->root should either be valid or NULLAGBLOCK at the end..?

Also, shouldn't we set *found_it a bit earlier in this function? It
looks like that simply controls the fab iteration and we have
technically matched the block to a btree type at this point, regardless
of whether we update root/height. (I wonder if something like "matched"
or "found_match" might be a better name than found_it...).

Brian

>  	fab->root = agbno;
> -	fab->height = xfs_btree_get_level(btblock) + 1;
> +	fab->height = block_level + 1;
>  	*found_it = true;
>  
>  	trace_xrep_findroot_block(mp, ri->sc->sa.agno, agbno,
> 

Re: [PATCH 2/6] xfs: xrep_findroot_block should reject root blocks with siblings

[Darrick J. Wong]

On Mon, Aug 13, 2018 at 12:56:36PM -0400, Brian Foster wrote:
> On Sat, Aug 11, 2018 at 08:35:09AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In xrep_findroot_block, if we find a candidate root block with sibling
> > pointers or sibling blocks on the same tree level, we should not return
> > that block as a tree root because root blocks cannot have siblings.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/repair.c |   47 +++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 41 insertions(+), 6 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> > index 17cf48564390..42d8c798ce7d 100644
> > --- a/fs/xfs/scrub/repair.c
> > +++ b/fs/xfs/scrub/repair.c
> > @@ -690,6 +690,7 @@ xrep_findroot_block(
> >  	struct xfs_buf			*bp;
> >  	struct xfs_btree_block		*btblock;
> >  	xfs_daddr_t			daddr;
> > +	int				block_level;
> >  	int				error;
> >  
> >  	daddr = XFS_AGB_TO_DADDR(mp, ri->sc->sa.agno, agbno);
> > @@ -727,17 +728,51 @@ xrep_findroot_block(
> >  		goto out;
> >  	bp->b_ops = fab->buf_ops;
> >  
> > -	/* Ignore this block if it's lower in the tree than we've seen. */
> > -	if (fab->root != NULLAGBLOCK &&
> > -	    xfs_btree_get_level(btblock) < fab->height)
> > -		goto out;
> > -
> >  	/* Make sure we pass the verifiers. */
> >  	bp->b_ops->verify_read(bp);
> >  	if (bp->b_error)
> >  		goto out;
> > +
> > +	/* If we've recorded a root candidate... */
> > +	block_level = xfs_btree_get_level(btblock);
> > +	if (fab->root != NULLAGBLOCK) {
> > +		/*
> > +		 * ...and this no-sibling root block candidate has the same
> > +		 * level as the recorded candidate, there's no way we're going
> > +		 * to accept any candidates at this tree level.  Stash a root
> > +		 * block of zero because the height is still valid, but no
> > +		 * AG btree can root at agblock 0.  Callers should verify the
> > +		 * root agbno with xfs_verify_agbno...
> > +		 */
> > +		if (block_level + 1 == fab->height) {
> > +			fab->root = 0;
> > +			goto out;
> > +		}
> > +
> > +		/*
> > +		 * ...and this no-sibling root block is lower in the tree than
> > +		 * the recorded root block candidate, just ignore it.  There's
> > +		 * still a strong chance that something is wrong with the btree
> > +		 * itself, but that's not what we're fixing right now.
> > +		 */
> > +		if (block_level < fab->height)
> > +			goto out;
> > +	}
> > +
> > +	/*
> > +	 * Root blocks can't have siblings.  This level can't be the root, so
> > +	 * record the tree height (but not the ag block pointer) to force us to
> > +	 * look for a higher level in the tree.
> > +	 */
> > +	if (btblock->bb_u.s.bb_leftsib != cpu_to_be32(NULLAGBLOCK) ||
> > +	    btblock->bb_u.s.bb_rightsib != cpu_to_be32(NULLAGBLOCK)) {
> > +		fab->root = 0;
> > +		fab->height = block_level + 1;
> > +		goto out;
> > +	}
> > +

[Finally getting to this after weeks...]

> Hmm, this looks technically correct but this still seems more involved
> than necessary. I don't really like how we define yet another magic
> value of 0, for example, and differentiate that from NULLAGBLOCK when it
> seems like we could just check the height.
>
> Could we do something like the following (which assumes ->height is
> initialized to 0)?

Yes, ->height is initialized to zero.

>         /*
>          * If current height exceeds the block level, we've already seen at
>          * least one block at this level or higher. Skip it and invalidate the
>          * root if this block matches the current root level because a root
>          * block has no siblings.
>          */
>         block_level = xfs_btree_get_level(btblock);
>         if (fab->height > block_level) {
>                 if (fab->height - 1 == block_level)
>                         fab->root = NULLAGBLOCK;
>                 goto out;
>         }

Yes, I think that will work.  I might be a little more explicit that
we're handling two cases here.

>         /*
>          * Found a block with a new max height. Track it as a root candidate if
>          * it has no siblings. Otherwise invalidate root since we know the root
>          * can't be at this level.
>          */
>         if (btblock->bb_u.s.bb_leftsib == cpu_to_be32(NULLAGBLOCK) &&
>             btblock->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK))
>                 fab->root = agbno;
>         else
>                 fab->root = NULLAGBLOCK;
>         fab->height = block_level + 1;
> 
> ... and then ->root should either be valid or NULLAGBLOCK at the end..?

Correct.  This is much better. :)

> Also, shouldn't we set *found_it a bit earlier in this function? It
> looks like that simply controls the fab iteration and we have
> technically matched the block to a btree type at this point, regardless
> of whether we update root/height. (I wonder if something like "matched"
> or "found_match" might be a better name than found_it...).

If the block passes the magic number/uuid/verifier tests then we're
certain that the block belongs to this btree type and we don't need to
try the other btree types.  Therefore, *found_it should indeed be set
earlier in the function.  It should probably be renamed *done or
something.

Ok so here's the new code, starting right after we finish the
magic/uuid/verifier tests:

	/*
	 * This block passes the magic number and verifier test for this tree
	 * type.  We don't need the caller to try the other tree types.
	 */
	*done_with_block = true;

	block_level = xfs_btree_get_level(btblock);
	if (block_level + 1 == fab->height) {
		/*
		 * This block claims to be at the same level as the root we
		 * found previously.  There can't be two candidate roots, so
		 * we'll throw away both of them and hope we later find a block
		 * even higher in the tree.
		 */
		fab->root = NULLAGBLOCK;
		goto out;
	} else if (block_level < fab->height) {
		/*
		 * This block is lower in the tree than the root we found
		 * previously, so just ignore it.
		 */
		goto out;
	}

	/*
	 * This is the highest block in the tree that we've found so far.
	 * Update the btree height to reflect what we've learned from this
	 * block.
	 */
	fab->height = block_level + 1;

	/*
	 * If this block doesn't have sibling pointers, then it's the new root
	 * block candidate.  Otherwise, the root will be found farther up the
	 * tree.
	 */
	if (btblock->bb_u.s.bb_leftsib == cpu_to_be32(NULLAGBLOCK) &&
	    btblock->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK))
		fab->root = agbno;
	else
		fab->root = NULLAGBLOCK;

Will ship this out for testing and see what happens. :)

--D

> 
> Brian
> 
> >  	fab->root = agbno;
> > -	fab->height = xfs_btree_get_level(btblock) + 1;
> > +	fab->height = block_level + 1;
> >  	*found_it = true;
> >  
> >  	trace_xrep_findroot_block(mp, ri->sc->sa.agno, agbno,
> > 

[PATCH 3/6] xfs: sanity check ag header values in xrep_calc_ag_resblks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Check the values we read in from the AG headers when calculating the
block reservations for a repair transaction.  If they're obviously
wrong, substitute worst case assumptions (rather than ENOSPC on a bogus
reservatoin request).

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/repair.c |   46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 42d8c798ce7d..97c3077fb005 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -195,8 +195,8 @@ xrep_calc_ag_resblks(
 	struct xfs_scrub_metadata	*sm = sc->sm;
 	struct xfs_perag		*pag;
 	struct xfs_buf			*bp;
-	xfs_agino_t			icount = 0;
-	xfs_extlen_t			aglen = 0;
+	xfs_agino_t			icount = NULLAGINO;
+	xfs_extlen_t			aglen = NULLAGBLOCK;
 	xfs_extlen_t			usedlen;
 	xfs_extlen_t			freelen;
 	xfs_extlen_t			bnobt_sz;
@@ -208,20 +208,14 @@ xrep_calc_ag_resblks(
 	if (!(sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR))
 		return 0;
 
-	/* Use in-core counters if possible. */
 	pag = xfs_perag_get(mp, sm->sm_agno);
-	if (pag->pagi_init)
+	if (pag->pagi_init) {
+		/* Use in-core icount if possible. */
 		icount = pag->pagi_count;
-
-	/*
-	 * Otherwise try to get the actual counters from disk; if not, make
-	 * some worst case assumptions.
-	 */
-	if (icount == 0) {
+	} else {
+		/* Try to get the actual counters from disk. */
 		error = xfs_ialloc_read_agi(mp, NULL, sm->sm_agno, &bp);
-		if (error) {
-			icount = mp->m_sb.sb_agblocks / mp->m_sb.sb_inopblock;
-		} else {
+		if (!error) {
 			icount = pag->pagi_count;
 			xfs_buf_relse(bp);
 		}
@@ -229,18 +223,32 @@ xrep_calc_ag_resblks(
 
 	/* Now grab the block counters from the AGF. */
 	error = xfs_alloc_read_agf(mp, NULL, sm->sm_agno, 0, &bp);
-	if (error) {
-		aglen = mp->m_sb.sb_agblocks;
-		freelen = aglen;
-		usedlen = aglen;
-	} else {
+	if (!error) {
 		aglen = be32_to_cpu(XFS_BUF_TO_AGF(bp)->agf_length);
-		freelen = pag->pagf_freeblks;
+		freelen = be32_to_cpu(XFS_BUF_TO_AGF(bp)->agf_freeblks);
 		usedlen = aglen - freelen;
 		xfs_buf_relse(bp);
 	}
 	xfs_perag_put(pag);
 
+	/* If the icount is impossible, make some worst-case assumptions. */
+	if (icount == NULLAGINO ||
+	    !xfs_verify_agino(mp, sm->sm_agno, icount)) {
+		xfs_agino_t	first, last;
+
+		xfs_agino_range(mp, sm->sm_agno, &first, &last);
+		icount = last - first + 1;
+	}
+
+	/* If the block counts are impossible, make worst-case assumptions. */
+	if (aglen == NULLAGBLOCK ||
+	    aglen != xfs_ag_block_count(mp, sm->sm_agno) ||
+	    freelen >= aglen) {
+		aglen = xfs_ag_block_count(mp, sm->sm_agno);
+		freelen = aglen;
+		usedlen = aglen;
+	}
+
 	trace_xrep_calc_ag_resblks(mp, sm->sm_agno, icount, aglen,
 			freelen, usedlen);
 

Re: [PATCH 3/6] xfs: sanity check ag header values in xrep_calc_ag_resblks

[Allison Henderson]

On 08/11/2018 08:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the values we read in from the AG headers when calculating the
> block reservations for a repair transaction.  If they're obviously
> wrong, substitute worst case assumptions (rather than ENOSPC on a bogus
> reservatoin request).
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/scrub/repair.c |   46 +++++++++++++++++++++++++++-------------------
>   1 file changed, 27 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 42d8c798ce7d..97c3077fb005 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -195,8 +195,8 @@ xrep_calc_ag_resblks(
>   	struct xfs_scrub_metadata	*sm = sc->sm;
>   	struct xfs_perag		*pag;
>   	struct xfs_buf			*bp;
> -	xfs_agino_t			icount = 0;
> -	xfs_extlen_t			aglen = 0;
> +	xfs_agino_t			icount = NULLAGINO;
> +	xfs_extlen_t			aglen = NULLAGBLOCK;
>   	xfs_extlen_t			usedlen;
>   	xfs_extlen_t			freelen;
>   	xfs_extlen_t			bnobt_sz;
> @@ -208,20 +208,14 @@ xrep_calc_ag_resblks(
>   	if (!(sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR))
>   		return 0;
>   
> -	/* Use in-core counters if possible. */
>   	pag = xfs_perag_get(mp, sm->sm_agno);
> -	if (pag->pagi_init)
> +	if (pag->pagi_init) {
> +		/* Use in-core icount if possible. */
>   		icount = pag->pagi_count;
> -
> -	/*
> -	 * Otherwise try to get the actual counters from disk; if not, make
> -	 * some worst case assumptions.
> -	 */
> -	if (icount == 0) {
> +	} else {
> +		/* Try to get the actual counters from disk. */
>   		error = xfs_ialloc_read_agi(mp, NULL, sm->sm_agno, &bp);
> -		if (error) {
> -			icount = mp->m_sb.sb_agblocks / mp->m_sb.sb_inopblock;
> -		} else {
> +		if (!error) {
>   			icount = pag->pagi_count;
>   			xfs_buf_relse(bp);
>   		}
> @@ -229,18 +223,32 @@ xrep_calc_ag_resblks(
>   
>   	/* Now grab the block counters from the AGF. */
>   	error = xfs_alloc_read_agf(mp, NULL, sm->sm_agno, 0, &bp);
> -	if (error) {
> -		aglen = mp->m_sb.sb_agblocks;
> -		freelen = aglen;
> -		usedlen = aglen;
> -	} else {
> +	if (!error) {
>   		aglen = be32_to_cpu(XFS_BUF_TO_AGF(bp)->agf_length);
> -		freelen = pag->pagf_freeblks;
> +		freelen = be32_to_cpu(XFS_BUF_TO_AGF(bp)->agf_freeblks);
>   		usedlen = aglen - freelen;
>   		xfs_buf_relse(bp);
>   	}
>   	xfs_perag_put(pag);
>   
> +	/* If the icount is impossible, make some worst-case assumptions. */
> +	if (icount == NULLAGINO ||
> +	    !xfs_verify_agino(mp, sm->sm_agno, icount)) {
> +		xfs_agino_t	first, last;
> +
> +		xfs_agino_range(mp, sm->sm_agno, &first, &last);
> +		icount = last - first + 1;
> +	}
> +
> +	/* If the block counts are impossible, make worst-case assumptions. */
> +	if (aglen == NULLAGBLOCK ||
> +	    aglen != xfs_ag_block_count(mp, sm->sm_agno) ||
> +	    freelen >= aglen) {
> +		aglen = xfs_ag_block_count(mp, sm->sm_agno);
> +		freelen = aglen;
> +		usedlen = aglen;
> +	}
> +
>   	trace_xrep_calc_ag_resblks(mp, sm->sm_agno, icount, aglen,
>   			freelen, usedlen);
>   
> 
Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Re: [PATCH 3/6] xfs: sanity check ag header values in xrep_calc_ag_resblks

[Carlos Maiolino]

On Sat, Aug 11, 2018 at 08:35:15AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the values we read in from the AG headers when calculating the
> block reservations for a repair transaction.  If they're obviously
> wrong, substitute worst case assumptions (rather than ENOSPC on a bogus
> reservatoin request).

reservation ^

Other than that:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/repair.c |   46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 42d8c798ce7d..97c3077fb005 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -195,8 +195,8 @@ xrep_calc_ag_resblks(
>  	struct xfs_scrub_metadata	*sm = sc->sm;
>  	struct xfs_perag		*pag;
>  	struct xfs_buf			*bp;
> -	xfs_agino_t			icount = 0;
> -	xfs_extlen_t			aglen = 0;
> +	xfs_agino_t			icount = NULLAGINO;
> +	xfs_extlen_t			aglen = NULLAGBLOCK;
>  	xfs_extlen_t			usedlen;
>  	xfs_extlen_t			freelen;
>  	xfs_extlen_t			bnobt_sz;
> @@ -208,20 +208,14 @@ xrep_calc_ag_resblks(
>  	if (!(sm->sm_flags & XFS_SCRUB_IFLAG_REPAIR))
>  		return 0;
>  
> -	/* Use in-core counters if possible. */
>  	pag = xfs_perag_get(mp, sm->sm_agno);
> -	if (pag->pagi_init)
> +	if (pag->pagi_init) {
> +		/* Use in-core icount if possible. */
>  		icount = pag->pagi_count;
> -
> -	/*
> -	 * Otherwise try to get the actual counters from disk; if not, make
> -	 * some worst case assumptions.
> -	 */
> -	if (icount == 0) {
> +	} else {
> +		/* Try to get the actual counters from disk. */
>  		error = xfs_ialloc_read_agi(mp, NULL, sm->sm_agno, &bp);
> -		if (error) {
> -			icount = mp->m_sb.sb_agblocks / mp->m_sb.sb_inopblock;
> -		} else {
> +		if (!error) {
>  			icount = pag->pagi_count;
>  			xfs_buf_relse(bp);
>  		}
> @@ -229,18 +223,32 @@ xrep_calc_ag_resblks(
>  
>  	/* Now grab the block counters from the AGF. */
>  	error = xfs_alloc_read_agf(mp, NULL, sm->sm_agno, 0, &bp);
> -	if (error) {
> -		aglen = mp->m_sb.sb_agblocks;
> -		freelen = aglen;
> -		usedlen = aglen;
> -	} else {
> +	if (!error) {
>  		aglen = be32_to_cpu(XFS_BUF_TO_AGF(bp)->agf_length);
> -		freelen = pag->pagf_freeblks;
> +		freelen = be32_to_cpu(XFS_BUF_TO_AGF(bp)->agf_freeblks);
>  		usedlen = aglen - freelen;
>  		xfs_buf_relse(bp);
>  	}
>  	xfs_perag_put(pag);
>  
> +	/* If the icount is impossible, make some worst-case assumptions. */
> +	if (icount == NULLAGINO ||
> +	    !xfs_verify_agino(mp, sm->sm_agno, icount)) {
> +		xfs_agino_t	first, last;
> +
> +		xfs_agino_range(mp, sm->sm_agno, &first, &last);
> +		icount = last - first + 1;
> +	}
> +
> +	/* If the block counts are impossible, make worst-case assumptions. */
> +	if (aglen == NULLAGBLOCK ||
> +	    aglen != xfs_ag_block_count(mp, sm->sm_agno) ||
> +	    freelen >= aglen) {
> +		aglen = xfs_ag_block_count(mp, sm->sm_agno);
> +		freelen = aglen;
> +		usedlen = aglen;
> +	}
> +
>  	trace_xrep_calc_ag_resblks(mp, sm->sm_agno, icount, aglen,
>  			freelen, usedlen);
>  
> 

-- 
Carlos

[PATCH 4/6] xfs: fix buffer state management in xrep_findroot_block

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

We don't quite handle buffer state properly in online repair's findroot
routine.  If the buffer is already in-core we don't want to trash its
b_ops and state, so first we should try _get_buf to grab the buffer.  If
the buffer is loaded, we only want to verify the structure of the buffer
since it could be dirty and the crc hasn't yet been recalculated.

Only if the buffer hasn't been read in should try _read_buf, and if we
were the ones who read the buffer then we must be careful to oneshot the
buffer so that a subsequent _read_buf won't find a buffer with no ops.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/repair.c |   67 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 13 deletions(-)


diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 97c3077fb005..fae50dced8bc 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -697,6 +697,7 @@ xrep_findroot_block(
 	struct xfs_mount		*mp = ri->sc->mp;
 	struct xfs_buf			*bp;
 	struct xfs_btree_block		*btblock;
+	xfs_failaddr_t			fa;
 	xfs_daddr_t			daddr;
 	int				block_level;
 	int				error;
@@ -718,28 +719,68 @@ xrep_findroot_block(
 			return error;
 	}
 
-	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp, daddr,
-			mp->m_bsize, 0, &bp, NULL);
-	if (error)
-		return error;
-
 	/*
-	 * Does this look like a block matching our fs and higher than any
-	 * other block we've found so far?  If so, reattach buffer verifiers
-	 * so the AIL won't complain if the buffer is also dirty.
+	 * Try to grab the buffer, on the off chance it's already in memory.
+	 * If the buffer doesn't have the DONE flag set it hasn't been read
+	 * into memory yet.  Drop the buffer and read the buffer with NULL
+	 * b_ops.  (This could race with another read_buf.)  If we get the
+	 * buffer back with NULL b_ops then we know that there weren't any
+	 * other readers.  There's a risk we won't match the buffer with any
+	 * of the findroot prototypes, so we want to encourage the buffer layer
+	 * to drop the buffer as soon as possible.
 	 */
+	bp = xfs_trans_get_buf(ri->sc->tp, mp->m_ddev_targp, daddr,
+			mp->m_bsize, 0);
+	if (!bp)
+		return -ENOMEM;
+	if (!(bp->b_flags & XBF_DONE)) {
+		xfs_trans_brelse(ri->sc->tp, bp);
+
+		error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
+				daddr, mp->m_bsize, 0, &bp, NULL);
+		if (error)
+			return error;
+		if (!bp->b_ops)
+			xfs_buf_oneshot(bp);
+	}
+
+	/* Does this look like a block matching our fs? */
 	btblock = XFS_BUF_TO_BLOCK(bp);
 	if (be32_to_cpu(btblock->bb_magic) != fab->magic)
 		goto out;
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    !uuid_equal(&btblock->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))
 		goto out;
-	bp->b_ops = fab->buf_ops;
 
-	/* Make sure we pass the verifiers. */
-	bp->b_ops->verify_read(bp);
-	if (bp->b_error)
-		goto out;
+	/*
+	 * We've matched this buffer by magic number to this findroot
+	 * prototype.  If there are no buffer ops attached, attach the one
+	 * specified by the prototype.  Otherwise, the buffer ops must match
+	 * the prototype.   We don't want to disturb existing b_ops.
+	 */
+	if (bp->b_ops) {
+		if (bp->b_ops != fab->buf_ops)
+			goto out;
+		/*
+		 * If the buffer was already incore (on a v5 fs) then it should
+		 * already have had b_ops assigned.  Call ->verify_struct to
+		 * check the structure.  Avoid checking the CRC because we
+		 * don't calculate CRCs until the buffer is written by the log.
+		 */
+		fa = bp->b_ops->verify_struct(bp);
+		if (fa)
+			goto out;
+	} else {
+		/*
+		 * If we have to assign buffer ops, that means that nobody's
+		 * checked the buffer structure or its CRC.  Do both now by
+		 * calling ->verify_read.
+		 */
+		bp->b_ops = fab->buf_ops;
+		bp->b_ops->verify_read(bp);
+		if (bp->b_error)
+			goto out;
+	}
 
 	/* If we've recorded a root candidate... */
 	block_level = xfs_btree_get_level(btblock);

Re: [PATCH 4/6] xfs: fix buffer state management in xrep_findroot_block

[Allison Henderson]

On 08/11/2018 08:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We don't quite handle buffer state properly in online repair's findroot
> routine.  If the buffer is already in-core we don't want to trash its
> b_ops and state, so first we should try _get_buf to grab the buffer.  If
> the buffer is loaded, we only want to verify the structure of the buffer
> since it could be dirty and the crc hasn't yet been recalculated.
> 
> Only if the buffer hasn't been read in should try _read_buf, and if we
> were the ones who read the buffer then we must be careful to oneshot the
> buffer so that a subsequent _read_buf won't find a buffer with no ops.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/scrub/repair.c |   67 +++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 54 insertions(+), 13 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 97c3077fb005..fae50dced8bc 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -697,6 +697,7 @@ xrep_findroot_block(
>   	struct xfs_mount		*mp = ri->sc->mp;
>   	struct xfs_buf			*bp;
>   	struct xfs_btree_block		*btblock;
> +	xfs_failaddr_t			fa;
>   	xfs_daddr_t			daddr;
>   	int				block_level;
>   	int				error;
> @@ -718,28 +719,68 @@ xrep_findroot_block(
>   			return error;
>   	}
>   
> -	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp, daddr,
> -			mp->m_bsize, 0, &bp, NULL);
> -	if (error)
> -		return error;
> -
>   	/*
> -	 * Does this look like a block matching our fs and higher than any
> -	 * other block we've found so far?  If so, reattach buffer verifiers
> -	 * so the AIL won't complain if the buffer is also dirty.
> +	 * Try to grab the buffer, on the off chance it's already in memory.
> +	 * If the buffer doesn't have the DONE flag set it hasn't been read
> +	 * into memory yet.  Drop the buffer and read the buffer with NULL
> +	 * b_ops.  (This could race with another read_buf.)  If we get the
> +	 * buffer back with NULL b_ops then we know that there weren't any
> +	 * other readers.  There's a risk we won't match the buffer with any
> +	 * of the findroot prototypes, so we want to encourage the buffer layer
> +	 * to drop the buffer as soon as possible.
>   	 */
> +	bp = xfs_trans_get_buf(ri->sc->tp, mp->m_ddev_targp, daddr,
> +			mp->m_bsize, 0);
> +	if (!bp)
> +		return -ENOMEM;
> +	if (!(bp->b_flags & XBF_DONE)) {
> +		xfs_trans_brelse(ri->sc->tp, bp);
> +
> +		error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> +				daddr, mp->m_bsize, 0, &bp, NULL);
> +		if (error)
> +			return error;
> +		if (!bp->b_ops)
> +			xfs_buf_oneshot(bp);
> +	}
> +
> +	/* Does this look like a block matching our fs? */
>   	btblock = XFS_BUF_TO_BLOCK(bp);
>   	if (be32_to_cpu(btblock->bb_magic) != fab->magic)
>   		goto out;
>   	if (xfs_sb_version_hascrc(&mp->m_sb) &&
>   	    !uuid_equal(&btblock->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))
>   		goto out;
> -	bp->b_ops = fab->buf_ops;
>   
> -	/* Make sure we pass the verifiers. */
> -	bp->b_ops->verify_read(bp);
> -	if (bp->b_error)
> -		goto out;
> +	/*
> +	 * We've matched this buffer by magic number to this findroot
> +	 * prototype.  If there are no buffer ops attached, attach the one
> +	 * specified by the prototype.  Otherwise, the buffer ops must match
> +	 * the prototype.   We don't want to disturb existing b_ops.
> +	 */
> +	if (bp->b_ops) {
> +		if (bp->b_ops != fab->buf_ops)
> +			goto out;
> +		/*
> +		 * If the buffer was already incore (on a v5 fs) then it should
> +		 * already have had b_ops assigned.  Call ->verify_struct to
> +		 * check the structure.  Avoid checking the CRC because we
> +		 * don't calculate CRCs until the buffer is written by the log.
> +		 */
> +		fa = bp->b_ops->verify_struct(bp);
> +		if (fa)
> +			goto out;
> +	} else {
> +		/*
> +		 * If we have to assign buffer ops, that means that nobody's
> +		 * checked the buffer structure or its CRC.  Do both now by
> +		 * calling ->verify_read.
> +		 */
> +		bp->b_ops = fab->buf_ops;
> +		bp->b_ops->verify_read(bp);
> +		if (bp->b_error)
> +			goto out;
> +	}
>   
>   	/* If we've recorded a root candidate... */
>   	block_level = xfs_btree_get_level(btblock);
> 
Ok, thanks for the commentary, it helps!
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Re: [PATCH 4/6] xfs: fix buffer state management in xrep_findroot_block

[Carlos Maiolino]

On Sat, Aug 11, 2018 at 08:35:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We don't quite handle buffer state properly in online repair's findroot
> routine.  If the buffer is already in-core we don't want to trash its
> b_ops and state, so first we should try _get_buf to grab the buffer.  If
> the buffer is loaded, we only want to verify the structure of the buffer
> since it could be dirty and the crc hasn't yet been recalculated.
> 
> Only if the buffer hasn't been read in should try _read_buf, and if we
> were the ones who read the buffer then we must be careful to oneshot the
> buffer so that a subsequent _read_buf won't find a buffer with no ops.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

>  fs/xfs/scrub/repair.c |   67 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 54 insertions(+), 13 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 97c3077fb005..fae50dced8bc 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -697,6 +697,7 @@ xrep_findroot_block(
>  	struct xfs_mount		*mp = ri->sc->mp;
>  	struct xfs_buf			*bp;
>  	struct xfs_btree_block		*btblock;
> +	xfs_failaddr_t			fa;
>  	xfs_daddr_t			daddr;
>  	int				block_level;
>  	int				error;
> @@ -718,28 +719,68 @@ xrep_findroot_block(
>  			return error;
>  	}
>  
> -	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp, daddr,
> -			mp->m_bsize, 0, &bp, NULL);
> -	if (error)
> -		return error;
> -
>  	/*
> -	 * Does this look like a block matching our fs and higher than any
> -	 * other block we've found so far?  If so, reattach buffer verifiers
> -	 * so the AIL won't complain if the buffer is also dirty.
> +	 * Try to grab the buffer, on the off chance it's already in memory.
> +	 * If the buffer doesn't have the DONE flag set it hasn't been read
> +	 * into memory yet.  Drop the buffer and read the buffer with NULL
> +	 * b_ops.  (This could race with another read_buf.)  If we get the
> +	 * buffer back with NULL b_ops then we know that there weren't any
> +	 * other readers.  There's a risk we won't match the buffer with any
> +	 * of the findroot prototypes, so we want to encourage the buffer layer
> +	 * to drop the buffer as soon as possible.
>  	 */
> +	bp = xfs_trans_get_buf(ri->sc->tp, mp->m_ddev_targp, daddr,
> +			mp->m_bsize, 0);
> +	if (!bp)
> +		return -ENOMEM;
> +	if (!(bp->b_flags & XBF_DONE)) {
> +		xfs_trans_brelse(ri->sc->tp, bp);
> +
> +		error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> +				daddr, mp->m_bsize, 0, &bp, NULL);
> +		if (error)
> +			return error;
> +		if (!bp->b_ops)
> +			xfs_buf_oneshot(bp);
> +	}
> +
> +	/* Does this look like a block matching our fs? */
>  	btblock = XFS_BUF_TO_BLOCK(bp);
>  	if (be32_to_cpu(btblock->bb_magic) != fab->magic)
>  		goto out;
>  	if (xfs_sb_version_hascrc(&mp->m_sb) &&
>  	    !uuid_equal(&btblock->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))
>  		goto out;
> -	bp->b_ops = fab->buf_ops;
>  
> -	/* Make sure we pass the verifiers. */
> -	bp->b_ops->verify_read(bp);
> -	if (bp->b_error)
> -		goto out;
> +	/*
> +	 * We've matched this buffer by magic number to this findroot
> +	 * prototype.  If there are no buffer ops attached, attach the one
> +	 * specified by the prototype.  Otherwise, the buffer ops must match
> +	 * the prototype.   We don't want to disturb existing b_ops.
> +	 */
> +	if (bp->b_ops) {
> +		if (bp->b_ops != fab->buf_ops)
> +			goto out;
> +		/*
> +		 * If the buffer was already incore (on a v5 fs) then it should
> +		 * already have had b_ops assigned.  Call ->verify_struct to
> +		 * check the structure.  Avoid checking the CRC because we
> +		 * don't calculate CRCs until the buffer is written by the log.
> +		 */
> +		fa = bp->b_ops->verify_struct(bp);
> +		if (fa)
> +			goto out;
> +	} else {
> +		/*
> +		 * If we have to assign buffer ops, that means that nobody's
> +		 * checked the buffer structure or its CRC.  Do both now by
> +		 * calling ->verify_read.
> +		 */
> +		bp->b_ops = fab->buf_ops;
> +		bp->b_ops->verify_read(bp);
> +		if (bp->b_error)
> +			goto out;
> +	}
>  
>  	/* If we've recorded a root candidate... */
>  	block_level = xfs_btree_get_level(btblock);
> 

-- 
Carlos

Re: [PATCH 4/6] xfs: fix buffer state management in xrep_findroot_block

[Brian Foster]

On Sat, Aug 11, 2018 at 08:35:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We don't quite handle buffer state properly in online repair's findroot
> routine.  If the buffer is already in-core we don't want to trash its
> b_ops and state, so first we should try _get_buf to grab the buffer.  If
> the buffer is loaded, we only want to verify the structure of the buffer
> since it could be dirty and the crc hasn't yet been recalculated.
> 
> Only if the buffer hasn't been read in should try _read_buf, and if we
> were the ones who read the buffer then we must be careful to oneshot the
> buffer so that a subsequent _read_buf won't find a buffer with no ops.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/repair.c |   67 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 54 insertions(+), 13 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 97c3077fb005..fae50dced8bc 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -697,6 +697,7 @@ xrep_findroot_block(
>  	struct xfs_mount		*mp = ri->sc->mp;
>  	struct xfs_buf			*bp;
>  	struct xfs_btree_block		*btblock;
> +	xfs_failaddr_t			fa;
>  	xfs_daddr_t			daddr;
>  	int				block_level;
>  	int				error;
> @@ -718,28 +719,68 @@ xrep_findroot_block(
>  			return error;
>  	}
>  
> -	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp, daddr,
> -			mp->m_bsize, 0, &bp, NULL);
> -	if (error)
> -		return error;
> -
>  	/*
> -	 * Does this look like a block matching our fs and higher than any
> -	 * other block we've found so far?  If so, reattach buffer verifiers
> -	 * so the AIL won't complain if the buffer is also dirty.
> +	 * Try to grab the buffer, on the off chance it's already in memory.
> +	 * If the buffer doesn't have the DONE flag set it hasn't been read
> +	 * into memory yet.  Drop the buffer and read the buffer with NULL
> +	 * b_ops.  (This could race with another read_buf.)  If we get the
> +	 * buffer back with NULL b_ops then we know that there weren't any
> +	 * other readers.  There's a risk we won't match the buffer with any
> +	 * of the findroot prototypes, so we want to encourage the buffer layer
> +	 * to drop the buffer as soon as possible.
>  	 */
> +	bp = xfs_trans_get_buf(ri->sc->tp, mp->m_ddev_targp, daddr,
> +			mp->m_bsize, 0);
> +	if (!bp)
> +		return -ENOMEM;
> +	if (!(bp->b_flags & XBF_DONE)) {
> +		xfs_trans_brelse(ri->sc->tp, bp);
> +
> +		error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> +				daddr, mp->m_bsize, 0, &bp, NULL);

Maybe I'm missing something, but I thought buf_ops should only be
attached to the buffer if the call actually read the buffer from disk.
Doesn't that mean we could continue to call xfs_trans_read_buf() here
and do the oneshot thing below to cover the case where we read it (and
don't want to leave around a buf with ->b_ops == NULL)?

> +		if (error)
> +			return error;
> +		if (!bp->b_ops)
> +			xfs_buf_oneshot(bp);
> +	}
> +

It's also kind of painful that we have to re-read the same buffer
multiple times to compare it to each fab. It seems like this could use
some refactoring to read each buffer once, check it against each fab
until we get through the verifier, then run the root/level processing on
the fab that matched. Perhaps that's something for another patch though
(make it work vs. make it fast :P)...

Brian

> +	/* Does this look like a block matching our fs? */
>  	btblock = XFS_BUF_TO_BLOCK(bp);
>  	if (be32_to_cpu(btblock->bb_magic) != fab->magic)
>  		goto out;
>  	if (xfs_sb_version_hascrc(&mp->m_sb) &&
>  	    !uuid_equal(&btblock->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))
>  		goto out;
> -	bp->b_ops = fab->buf_ops;
>  
> -	/* Make sure we pass the verifiers. */
> -	bp->b_ops->verify_read(bp);
> -	if (bp->b_error)
> -		goto out;
> +	/*
> +	 * We've matched this buffer by magic number to this findroot
> +	 * prototype.  If there are no buffer ops attached, attach the one
> +	 * specified by the prototype.  Otherwise, the buffer ops must match
> +	 * the prototype.   We don't want to disturb existing b_ops.
> +	 */
> +	if (bp->b_ops) {
> +		if (bp->b_ops != fab->buf_ops)
> +			goto out;
> +		/*
> +		 * If the buffer was already incore (on a v5 fs) then it should
> +		 * already have had b_ops assigned.  Call ->verify_struct to
> +		 * check the structure.  Avoid checking the CRC because we
> +		 * don't calculate CRCs until the buffer is written by the log.
> +		 */
> +		fa = bp->b_ops->verify_struct(bp);
> +		if (fa)
> +			goto out;
> +	} else {
> +		/*
> +		 * If we have to assign buffer ops, that means that nobody's
> +		 * checked the buffer structure or its CRC.  Do both now by
> +		 * calling ->verify_read.
> +		 */
> +		bp->b_ops = fab->buf_ops;
> +		bp->b_ops->verify_read(bp);
> +		if (bp->b_error)
> +			goto out;
> +	}
>  
>  	/* If we've recorded a root candidate... */
>  	block_level = xfs_btree_get_level(btblock);
> 

Re: [PATCH 4/6] xfs: fix buffer state management in xrep_findroot_block

[Darrick J. Wong]

On Mon, Aug 13, 2018 at 12:56:59PM -0400, Brian Foster wrote:
> On Sat, Aug 11, 2018 at 08:35:22AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > We don't quite handle buffer state properly in online repair's findroot
> > routine.  If the buffer is already in-core we don't want to trash its
> > b_ops and state, so first we should try _get_buf to grab the buffer.  If
> > the buffer is loaded, we only want to verify the structure of the buffer
> > since it could be dirty and the crc hasn't yet been recalculated.
> > 
> > Only if the buffer hasn't been read in should try _read_buf, and if we
> > were the ones who read the buffer then we must be careful to oneshot the
> > buffer so that a subsequent _read_buf won't find a buffer with no ops.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/repair.c |   67 +++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 54 insertions(+), 13 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> > index 97c3077fb005..fae50dced8bc 100644
> > --- a/fs/xfs/scrub/repair.c
> > +++ b/fs/xfs/scrub/repair.c
> > @@ -697,6 +697,7 @@ xrep_findroot_block(
> >  	struct xfs_mount		*mp = ri->sc->mp;
> >  	struct xfs_buf			*bp;
> >  	struct xfs_btree_block		*btblock;
> > +	xfs_failaddr_t			fa;
> >  	xfs_daddr_t			daddr;
> >  	int				block_level;
> >  	int				error;
> > @@ -718,28 +719,68 @@ xrep_findroot_block(
> >  			return error;
> >  	}
> >  
> > -	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp, daddr,
> > -			mp->m_bsize, 0, &bp, NULL);
> > -	if (error)
> > -		return error;
> > -
> >  	/*
> > -	 * Does this look like a block matching our fs and higher than any
> > -	 * other block we've found so far?  If so, reattach buffer verifiers
> > -	 * so the AIL won't complain if the buffer is also dirty.
> > +	 * Try to grab the buffer, on the off chance it's already in memory.
> > +	 * If the buffer doesn't have the DONE flag set it hasn't been read
> > +	 * into memory yet.  Drop the buffer and read the buffer with NULL
> > +	 * b_ops.  (This could race with another read_buf.)  If we get the
> > +	 * buffer back with NULL b_ops then we know that there weren't any
> > +	 * other readers.  There's a risk we won't match the buffer with any
> > +	 * of the findroot prototypes, so we want to encourage the buffer layer
> > +	 * to drop the buffer as soon as possible.
> >  	 */
> > +	bp = xfs_trans_get_buf(ri->sc->tp, mp->m_ddev_targp, daddr,
> > +			mp->m_bsize, 0);
> > +	if (!bp)
> > +		return -ENOMEM;
> > +	if (!(bp->b_flags & XBF_DONE)) {
> > +		xfs_trans_brelse(ri->sc->tp, bp);
> > +
> > +		error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> > +				daddr, mp->m_bsize, 0, &bp, NULL);
> 
> Maybe I'm missing something, but I thought buf_ops should only be
> attached to the buffer if the call actually read the buffer from disk.
> Doesn't that mean we could continue to call xfs_trans_read_buf() here
> and do the oneshot thing below to cover the case where we read it (and
> don't want to leave around a buf with ->b_ops == NULL)?
> 
> > +		if (error)
> > +			return error;
> > +		if (!bp->b_ops)
> > +			xfs_buf_oneshot(bp);
> > +	}
> > +
> 
> It's also kind of painful that we have to re-read the same buffer
> multiple times to compare it to each fab. It seems like this could use
> some refactoring to read each buffer once, check it against each fab
> until we get through the verifier, then run the root/level processing on
> the fab that matched. Perhaps that's something for another patch though
> (make it work vs. make it fast :P)...

FWIW I /think/ Dave's suggestion will greatly simplify the code while
reducing the amount of thrashing we do here.  I'll give it a try and see
what happens.

--D

> Brian
> 
> > +	/* Does this look like a block matching our fs? */
> >  	btblock = XFS_BUF_TO_BLOCK(bp);
> >  	if (be32_to_cpu(btblock->bb_magic) != fab->magic)
> >  		goto out;
> >  	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> >  	    !uuid_equal(&btblock->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))
> >  		goto out;
> > -	bp->b_ops = fab->buf_ops;
> >  
> > -	/* Make sure we pass the verifiers. */
> > -	bp->b_ops->verify_read(bp);
> > -	if (bp->b_error)
> > -		goto out;
> > +	/*
> > +	 * We've matched this buffer by magic number to this findroot
> > +	 * prototype.  If there are no buffer ops attached, attach the one
> > +	 * specified by the prototype.  Otherwise, the buffer ops must match
> > +	 * the prototype.   We don't want to disturb existing b_ops.
> > +	 */
> > +	if (bp->b_ops) {
> > +		if (bp->b_ops != fab->buf_ops)
> > +			goto out;
> > +		/*
> > +		 * If the buffer was already incore (on a v5 fs) then it should
> > +		 * already have had b_ops assigned.  Call ->verify_struct to
> > +		 * check the structure.  Avoid checking the CRC because we
> > +		 * don't calculate CRCs until the buffer is written by the log.
> > +		 */
> > +		fa = bp->b_ops->verify_struct(bp);
> > +		if (fa)
> > +			goto out;
> > +	} else {
> > +		/*
> > +		 * If we have to assign buffer ops, that means that nobody's
> > +		 * checked the buffer structure or its CRC.  Do both now by
> > +		 * calling ->verify_read.
> > +		 */
> > +		bp->b_ops = fab->buf_ops;
> > +		bp->b_ops->verify_read(bp);
> > +		if (bp->b_error)
> > +			goto out;
> > +	}
> >  
> >  	/* If we've recorded a root candidate... */
> >  	block_level = xfs_btree_get_level(btblock);
> > 

Re: [PATCH 4/6] xfs: fix buffer state management in xrep_findroot_block

[Dave Chinner]

On Sat, Aug 11, 2018 at 08:35:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We don't quite handle buffer state properly in online repair's findroot
> routine.  If the buffer is already in-core we don't want to trash its
> b_ops and state, so first we should try _get_buf to grab the buffer.  If
> the buffer is loaded, we only want to verify the structure of the buffer
> since it could be dirty and the crc hasn't yet been recalculated.
> 
> Only if the buffer hasn't been read in should try _read_buf, and if we
> were the ones who read the buffer then we must be careful to oneshot the
> buffer so that a subsequent _read_buf won't find a buffer with no ops.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I don't know the history of how this came about, but IMO this isn't
a particularly nice solution.

> ---
>  fs/xfs/scrub/repair.c |   67 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 54 insertions(+), 13 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 97c3077fb005..fae50dced8bc 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -697,6 +697,7 @@ xrep_findroot_block(
>  	struct xfs_mount		*mp = ri->sc->mp;
>  	struct xfs_buf			*bp;
>  	struct xfs_btree_block		*btblock;
> +	xfs_failaddr_t			fa;
>  	xfs_daddr_t			daddr;
>  	int				block_level;
>  	int				error;
> @@ -718,28 +719,68 @@ xrep_findroot_block(
>  			return error;
>  	}
>  
> -	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp, daddr,
> -			mp->m_bsize, 0, &bp, NULL);
> -	if (error)
> -		return error;
> -
>  	/*
> -	 * Does this look like a block matching our fs and higher than any
> -	 * other block we've found so far?  If so, reattach buffer verifiers
> -	 * so the AIL won't complain if the buffer is also dirty.
> +	 * Try to grab the buffer, on the off chance it's already in memory.
> +	 * If the buffer doesn't have the DONE flag set it hasn't been read
> +	 * into memory yet.  Drop the buffer and read the buffer with NULL
> +	 * b_ops.  (This could race with another read_buf.)  If we get the
> +	 * buffer back with NULL b_ops then we know that there weren't any
> +	 * other readers.  There's a risk we won't match the buffer with any
> +	 * of the findroot prototypes, so we want to encourage the buffer layer
> +	 * to drop the buffer as soon as possible.
>  	 */
> +	bp = xfs_trans_get_buf(ri->sc->tp, mp->m_ddev_targp, daddr,
> +			mp->m_bsize, 0);
> +	if (!bp)
> +		return -ENOMEM;
> +	if (!(bp->b_flags & XBF_DONE)) {
> +		xfs_trans_brelse(ri->sc->tp, bp);
> +
> +		error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> +				daddr, mp->m_bsize, 0, &bp, NULL);
> +		if (error)
> +			return error;
> +		if (!bp->b_ops)
> +			xfs_buf_oneshot(bp);
> +	}

Let's look a little closer. xfs_trans_read_buf() ends up in
xfs_buf_read_map(), which does:

....
        bp = xfs_buf_get_map(target, map, nmaps, flags);
        if (bp) {
                trace_xfs_buf_read(bp, flags, _RET_IP_);

                if (!(bp->b_flags & XBF_DONE)) {
                        XFS_STATS_INC(target->bt_mount, xb_get_read);
                        bp->b_ops = ops;
                        _xfs_buf_read(bp, flags);
		} else if (flags & XBF_ASYNC) {
.....

But what you are doing in the code above is trying to do is
determine if we needed to call _xfs_buf_read() on the buffer, and if
we do we use a different verify procedure on it.

So isn't there a simpler way to do this? e.g. pass a flag down to
xfs_buf_read_map() that says "use these ops for just this read".

	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
				daddr, mp->m_bsize, XBF_ONESHOT_OPS,
				&bp, NULL);

and change xfs_buf_read_map() to do:


....
        bp = xfs_buf_get_map(target, map, nmaps, flags);
        if (bp) {
                trace_xfs_buf_read(bp, flags, _RET_IP_);

                if (!(bp->b_flags & XBF_DONE)) {
                        XFS_STATS_INC(target->bt_mount, xb_get_read);
			if (flags & XBF_ONESHOT_OPS)
				orig_ops = bp->b_ops;
                        bp->b_ops = ops;
                        _xfs_buf_read(bp, flags);
			if (flags & XBF_ONESHOT_OPS)
				bp->b_ops = orig_ops;
		} else if (flags & XBF_ASYNC) {
			ASSERT(!(flags & XBF_ONESHOT_OPS));
....

Now you get back the buffer with it's original ops on it even if had
to be read from disk and you used a different verifier. Hence you
know how to treat it after this because the ops will be null if it
was not in core and had to be read from disk.

That also means you could supply fab->buf_ops to the
xfs_trans_read_buf() call, knowing that they'll be used on read and
you'll get a null bp->b_ops back despite the buffer already having
been fully verified. i.e. if it fails verification, you'll get an
error rather than having to having to run the verification yourself.
That means you only need to run the ->verify_struct() op if you get
back a non-null bp->b_ops, which further simplifies the function...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/6] xfs: fix buffer state management in xrep_findroot_block

[Darrick J. Wong]

On Tue, Aug 14, 2018 at 08:56:38AM +1000, Dave Chinner wrote:
> On Sat, Aug 11, 2018 at 08:35:22AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > We don't quite handle buffer state properly in online repair's findroot
> > routine.  If the buffer is already in-core we don't want to trash its
> > b_ops and state, so first we should try _get_buf to grab the buffer.  If
> > the buffer is loaded, we only want to verify the structure of the buffer
> > since it could be dirty and the crc hasn't yet been recalculated.
> > 
> > Only if the buffer hasn't been read in should try _read_buf, and if we
> > were the ones who read the buffer then we must be careful to oneshot the
> > buffer so that a subsequent _read_buf won't find a buffer with no ops.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I don't know the history of how this came about, but IMO this isn't
> a particularly nice solution.

Ugh, yes.

> > ---
> >  fs/xfs/scrub/repair.c |   67 +++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 54 insertions(+), 13 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> > index 97c3077fb005..fae50dced8bc 100644
> > --- a/fs/xfs/scrub/repair.c
> > +++ b/fs/xfs/scrub/repair.c
> > @@ -697,6 +697,7 @@ xrep_findroot_block(
> >  	struct xfs_mount		*mp = ri->sc->mp;
> >  	struct xfs_buf			*bp;
> >  	struct xfs_btree_block		*btblock;
> > +	xfs_failaddr_t			fa;
> >  	xfs_daddr_t			daddr;
> >  	int				block_level;
> >  	int				error;
> > @@ -718,28 +719,68 @@ xrep_findroot_block(
> >  			return error;
> >  	}
> >  
> > -	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp, daddr,
> > -			mp->m_bsize, 0, &bp, NULL);
> > -	if (error)
> > -		return error;
> > -
> >  	/*
> > -	 * Does this look like a block matching our fs and higher than any
> > -	 * other block we've found so far?  If so, reattach buffer verifiers
> > -	 * so the AIL won't complain if the buffer is also dirty.
> > +	 * Try to grab the buffer, on the off chance it's already in memory.
> > +	 * If the buffer doesn't have the DONE flag set it hasn't been read
> > +	 * into memory yet.  Drop the buffer and read the buffer with NULL
> > +	 * b_ops.  (This could race with another read_buf.)  If we get the
> > +	 * buffer back with NULL b_ops then we know that there weren't any
> > +	 * other readers.  There's a risk we won't match the buffer with any
> > +	 * of the findroot prototypes, so we want to encourage the buffer layer
> > +	 * to drop the buffer as soon as possible.
> >  	 */
> > +	bp = xfs_trans_get_buf(ri->sc->tp, mp->m_ddev_targp, daddr,
> > +			mp->m_bsize, 0);
> > +	if (!bp)
> > +		return -ENOMEM;
> > +	if (!(bp->b_flags & XBF_DONE)) {
> > +		xfs_trans_brelse(ri->sc->tp, bp);
> > +
> > +		error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> > +				daddr, mp->m_bsize, 0, &bp, NULL);
> > +		if (error)
> > +			return error;
> > +		if (!bp->b_ops)
> > +			xfs_buf_oneshot(bp);
> > +	}
> 
> Let's look a little closer. xfs_trans_read_buf() ends up in
> xfs_buf_read_map(), which does:
> 
> ....
>         bp = xfs_buf_get_map(target, map, nmaps, flags);
>         if (bp) {
>                 trace_xfs_buf_read(bp, flags, _RET_IP_);
> 
>                 if (!(bp->b_flags & XBF_DONE)) {
>                         XFS_STATS_INC(target->bt_mount, xb_get_read);
>                         bp->b_ops = ops;
>                         _xfs_buf_read(bp, flags);
> 		} else if (flags & XBF_ASYNC) {
> .....
> 
> But what you are doing in the code above is trying to do is
> determine if we needed to call _xfs_buf_read() on the buffer, and if
> we do we use a different verify procedure on it.
> 
> So isn't there a simpler way to do this? e.g. pass a flag down to
> xfs_buf_read_map() that says "use these ops for just this read".
> 
> 	error = xfs_trans_read_buf(mp, ri->sc->tp, mp->m_ddev_targp,
> 				daddr, mp->m_bsize, XBF_ONESHOT_OPS,
> 				&bp, NULL);
> 
> and change xfs_buf_read_map() to do:
> 
> 
> ....
>         bp = xfs_buf_get_map(target, map, nmaps, flags);
>         if (bp) {
>                 trace_xfs_buf_read(bp, flags, _RET_IP_);
> 
>                 if (!(bp->b_flags & XBF_DONE)) {
>                         XFS_STATS_INC(target->bt_mount, xb_get_read);
> 			if (flags & XBF_ONESHOT_OPS)
> 				orig_ops = bp->b_ops;
>                         bp->b_ops = ops;
>                         _xfs_buf_read(bp, flags);
> 			if (flags & XBF_ONESHOT_OPS)
> 				bp->b_ops = orig_ops;
> 		} else if (flags & XBF_ASYNC) {
> 			ASSERT(!(flags & XBF_ONESHOT_OPS));
> ....
> 
> Now you get back the buffer with it's original ops on it even if had
> to be read from disk and you used a different verifier. Hence you
> know how to treat it after this because the ops will be null if it
> was not in core and had to be read from disk.
> 
> That also means you could supply fab->buf_ops to the
> xfs_trans_read_buf() call, knowing that they'll be used on read and
> you'll get a null bp->b_ops back despite the buffer already having
> been fully verified. i.e. if it fails verification, you'll get an
> error rather than having to having to run the verification yourself.
> That means you only need to run the ->verify_struct() op if you get
> back a non-null bp->b_ops, which further simplifies the function...

Hmm, that seems like a much better solution.  I'll look into it.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH 5/6] iomap: fix WARN_ON_ONCE on uninitialized variable

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In commit 9dc55f1389f9569 ("iomap: add support for sub-pagesize buffered
I/O without buffer heads") we moved the initialization of poff (it's
computed from pos) into a separate helper function.  Inline data only
ever deals with pos == 0, hence the WARN_ON_ONCE, but now we're testing
an uninitialized variable.

Therefore, change the test to check the parameter directly.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/iomap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/iomap.c b/fs/iomap.c
index 8bd54c08deee..8a18163dc432 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -290,7 +290,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 	sector_t sector;
 
 	if (iomap->type == IOMAP_INLINE) {
-		WARN_ON_ONCE(poff);
+		WARN_ON_ONCE(pos);
 		iomap_read_inline_data(inode, page, iomap);
 		return PAGE_SIZE;
 	}

Re: [PATCH 5/6] iomap: fix WARN_ON_ONCE on uninitialized variable

[Allison Henderson]

On 08/11/2018 08:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit 9dc55f1389f9569 ("iomap: add support for sub-pagesize buffered
> I/O without buffer heads") we moved the initialization of poff (it's
> computed from pos) into a separate helper function.  Inline data only
> ever deals with pos == 0, hence the WARN_ON_ONCE, but now we're testing
> an uninitialized variable.
> 
> Therefore, change the test to check the parameter directly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/iomap.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 8bd54c08deee..8a18163dc432 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -290,7 +290,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
>   	sector_t sector;
>   
>   	if (iomap->type == IOMAP_INLINE) {
> -		WARN_ON_ONCE(poff);
> +		WARN_ON_ONCE(pos);
>   		iomap_read_inline_data(inode, page, iomap);
>   		return PAGE_SIZE;
>   	}
> 
Ok, looks good.
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Re: [PATCH 5/6] iomap: fix WARN_ON_ONCE on uninitialized variable

[Carlos Maiolino]

On Sat, Aug 11, 2018 at 08:35:28AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit 9dc55f1389f9569 ("iomap: add support for sub-pagesize buffered
> I/O without buffer heads") we moved the initialization of poff (it's
> computed from pos) into a separate helper function.  Inline data only
> ever deals with pos == 0, hence the WARN_ON_ONCE, but now we're testing
> an uninitialized variable.
> 
> Therefore, change the test to check the parameter directly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

>  fs/iomap.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 8bd54c08deee..8a18163dc432 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -290,7 +290,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
>  	sector_t sector;
>  
>  	if (iomap->type == IOMAP_INLINE) {
> -		WARN_ON_ONCE(poff);
> +		WARN_ON_ONCE(pos);
>  		iomap_read_inline_data(inode, page, iomap);
>  		return PAGE_SIZE;
>  	}
> 

-- 
Carlos

[PATCH 6/6] xfs: don't crash the vfs on a garbage inline symlink

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The VFS routine that calls ->get_link blindly copies whatever's returned
into the user's buffer.  If we return a NULL pointer, the vfs will
crash on the null pointer.  Therefore, return -EFSCORRUPTED instead of
blowing up the kernel.

Reported-by: wen.xu@gatech.edu
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_iops.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0ef5ad7fb851..26007a9db49d 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -471,8 +471,16 @@ xfs_vn_get_link_inline(
 	struct inode		*inode,
 	struct delayed_call	*done)
 {
+	char			*ptr;
+
 	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
-	return XFS_I(inode)->i_df.if_u1.if_data;
+
+	/*
+	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
+	 * if_data is junk.
+	 */
+	ptr = XFS_I(inode)->i_df.if_u1.if_data;
+	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
 }
 
 STATIC int

Re: [PATCH 6/6] xfs: don't crash the vfs on a garbage inline symlink

[Allison Henderson]

On 08/11/2018 08:35 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The VFS routine that calls ->get_link blindly copies whatever's returned
> into the user's buffer.  If we return a NULL pointer, the vfs will
> crash on the null pointer.  Therefore, return -EFSCORRUPTED instead of
> blowing up the kernel.
> 
> Reported-by: wen.xu@gatech.edu
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_iops.c |   10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 0ef5ad7fb851..26007a9db49d 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -471,8 +471,16 @@ xfs_vn_get_link_inline(
>   	struct inode		*inode,
>   	struct delayed_call	*done)
>   {
> +	char			*ptr;
> +
>   	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> -	return XFS_I(inode)->i_df.if_u1.if_data;
> +
> +	/*
> +	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
> +	 * if_data is junk.
> +	 */
> +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> +	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
>   }
>   
>   STATIC int
> 
Ok, looks fine.
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Re: [PATCH 6/6] xfs: don't crash the vfs on a garbage inline symlink

[Christoph Hellwig]

>  	struct delayed_call	*done)
>  {
> +	char			*ptr;
> +
>  	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> -	return XFS_I(inode)->i_df.if_u1.if_data;
> +
> +	/*
> +	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
> +	 * if_data is junk.
> +	 */
> +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> +	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
>  }
>  
>  STATIC int

Please simplify this to:

>  	struct delayed_call	*done)
>  {
	char			*link = XFS_I(inode)->i_df.if_u1.if_data;

  	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> +	ptr = XFS_I(inode)->i_df.if_u1.if_data;

	if (!link)
		return ERR_PTR(-EFSCORRUPTED);
	return link;

But be honest I'd much rather fix this in the caller than every fs.
Can you send a patch to Al to do that instead?

Re: [PATCH 6/6] xfs: don't crash the vfs on a garbage inline symlink

[Darrick J. Wong]

On Mon, Aug 13, 2018 at 12:23:13AM -0700, Christoph Hellwig wrote:
> >  	struct delayed_call	*done)
> >  {
> > +	char			*ptr;
> > +
> >  	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> > -	return XFS_I(inode)->i_df.if_u1.if_data;
> > +
> > +	/*
> > +	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
> > +	 * if_data is junk.
> > +	 */
> > +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> > +	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
> >  }
> >  
> >  STATIC int
> 
> Please simplify this to:
> 
> >  	struct delayed_call	*done)
> >  {
> 	char			*link = XFS_I(inode)->i_df.if_u1.if_data;
> 
>   	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> > +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> 
> 	if (!link)
> 		return ERR_PTR(-EFSCORRUPTED);
> 	return link;
> 
> But be honest I'd much rather fix this in the caller than every fs.
> Can you send a patch to Al to do that instead?

Ok, I'll do that.

--D

Re: [PATCH 6/6] xfs: don't crash the vfs on a garbage inline symlink

[Xu, Wen]

Hi Darrick,

Could I know what bugzilla bug I reported this patch corresponds to?

Thanks,
Wen

> On Aug 11, 2018, at 11:35 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The VFS routine that calls ->get_link blindly copies whatever's returned
> into the user's buffer.  If we return a NULL pointer, the vfs will
> crash on the null pointer.  Therefore, return -EFSCORRUPTED instead of
> blowing up the kernel.
> 
> Reported-by: wen.xu@gatech.edu
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_iops.c |   10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 0ef5ad7fb851..26007a9db49d 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -471,8 +471,16 @@ xfs_vn_get_link_inline(
> 	struct inode		*inode,
> 	struct delayed_call	*done)
> {
> +	char			*ptr;
> +
> 	ASSERT(XFS_I(inode)->i_df.if_flags & XFS_IFINLINE);
> -	return XFS_I(inode)->i_df.if_u1.if_data;
> +
> +	/*
> +	 * The VFS crashes on a NULL pointer, so return -EFSCORRUPTED if
> +	 * if_data is junk.
> +	 */
> +	ptr = XFS_I(inode)->i_df.if_u1.if_data;
> +	return ptr ? ptr : ERR_PTR(-EFSCORRUPTED);
> }
> 
> STATIC int
>