extfree log recovery and owner (rmapbt) updates

extfree log recovery and owner (rmapbt) updates

[Brian Foster]

Hi,

I've been playing around with deferring AGFL block frees and noticed
something funky going on with xfs_bmap_add_free() and owner info. We
pass the extent owner info to xfs_bmap_add_free() in various places to
transfer this info to the deferred free processing context.
Subsequently, xfs_trans_free_extent() -> ... -> xfs_free_ag_extent()
uses the owner info to remove the rmapbt entry associated with the
extent. The rmapbt update occurs at the time the extent is freed.

If the system crashes immediately after the initial transaction commits,
however, EFI recovery calls xfs_trans_free_extent() with an "unknown"
owner. I see a reference to using a "wildcard" in this context in some
of the old rmapbt commits, but it looks like this codepath skips the
rmapbt update altogether if oi_owner == XFS_RMAP_OWN_UNKNOWN. A quick
test that shuts down the fs immediately after a transaction commits that
logs an EFI for a freed inode chunk leaves the fs inconsistent after log
recovery, with a bit of a cryptic message from repair:

  unknown block state, ag 0, block 24

It does look like the associated rmapbt entry still exists in the tree
even though the extent has been freed (from xfs_db -c fsmap):

5: 0/24 len 8 owner -7 offset 0 bmbt 0 attrfork 0 extflag 0                                                            

So what is supposed to be going on here? Should the rmapbt entry be
removed unconditionally during recovery, or should a separate rmapbt
update item be deferred (as in the bunmapi case, for example) rather
than passing oinfo along with the TYPE_FREE deferred item? Or something
else entirely..?

Brian

Re: extfree log recovery and owner (rmapbt) updates

[Darrick J. Wong]

On Tue, Dec 05, 2017 at 01:55:24PM -0500, Brian Foster wrote:
> Hi,
> 
> I've been playing around with deferring AGFL block frees and noticed
> something funky going on with xfs_bmap_add_free() and owner info. We
> pass the extent owner info to xfs_bmap_add_free() in various places to
> transfer this info to the deferred free processing context.
> Subsequently, xfs_trans_free_extent() -> ... -> xfs_free_ag_extent()
> uses the owner info to remove the rmapbt entry associated with the
> extent. The rmapbt update occurs at the time the extent is freed.
> 
> If the system crashes immediately after the initial transaction commits,
> however, EFI recovery calls xfs_trans_free_extent() with an "unknown"
> owner. I see a reference to using a "wildcard" in this context in some
> of the old rmapbt commits, but it looks like this codepath skips the
> rmapbt update altogether if oi_owner == XFS_RMAP_OWN_UNKNOWN. A quick
> test that shuts down the fs immediately after a transaction commits that
> logs an EFI for a freed inode chunk leaves the fs inconsistent after log
> recovery, with a bit of a cryptic message from repair:
> 
>   unknown block state, ag 0, block 24
> 
> It does look like the associated rmapbt entry still exists in the tree
> even though the extent has been freed (from xfs_db -c fsmap):
> 
> 5: 0/24 len 8 owner -7 offset 0 bmbt 0 attrfork 0 extflag 0
> 
> So what is supposed to be going on here? Should the rmapbt entry be
> removed unconditionally during recovery, or should a separate rmapbt
> update item be deferred (as in the bunmapi case, for example) rather
> than passing oinfo along with the TYPE_FREE deferred item? Or something
> else entirely..?

Ooops, this whole thing is kind of a mess... so right now we have two
mechanisms to modify an rmap -- xfs_rmap_{map,unmap,alloc,free}_extent,
which uses the deferred ops mechanism; or xfs_rmap_{alloc,free}, which
makes the change directly in the current transaction.  The first is
used in the bmapping code for file blocks, and the second are used
everywhere else, particularly if we aren't set up for a transaction
roll, like when we're doing non-bmapi allocations.

So the simple fix to this is that we fix xfs_free_extent and
xfs_rmap_unmap so that OWN_UNKNOWN truly is a wildcard "remove rmap,
don't care who owns it" operation.  So long as the wildcard operation
comes after any other attempt to remove the rmap, this will be fine.

Note that xfs_bmap_add_free passes owner info into the in-core EFI,
which means that freeing an extent can also remove the rmap.  For
operations where we explicitly free the rmap via deferred ops (bmapi and
cow) we don't want this, so we need to be able to pass a NULL owner info
to xfs_bmap_add_free.  The EFI can record this as an OWN_NULL owner and
skip freeing the rmap, though this is potentially fraught because log
recovery won't know this and will try the wildcard rmap deletion anyway,
exposing another bug.

When we're cleaning up CoW extents, we create a CUI to remove the CoW
extent from the refcountbt and then create an EFI to free the extent.
The CUI finish routine queues an RUI to delete the rmap, which is then
placed after the EFI.  CUI -> EFI -> RUI, which is the wrong order.
This causes a double-free of the rmap, first with the wildcard and again
with OWN_COW, and the OWN_COW blows asserts.  So, that piece has to be
rearranged to log CUI -> RUI -> EFI.

I'll send an RFC patch as a reply to this one...

---

The other solution, of course, is to separate rmap and allocation
completely -- every allocation requires a dfops structure, and the
deferred rmaps must be queued directly by whomever calls the allocator.
This has the disadvantage that now everything in the allocation path is
a deferred op, whereas right now we can pack some of those into a single
transaction.

--D

> Brian
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH] xfs: always honor OWN_UNKNOWN rmap removal requests

[Darrick J. Wong]

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

Calling xfs_rmap_free with an unknown owner is supposed to remove any
rmaps covering that range regardless of owner.  This is used by the EFI
recovery code to say "we're freeing this, it mustn't be owned by
anything anymore", but for whatever reason xfs_free_ag_extent filters
them out.

Therefore, remove the filter and make xfs_rmap_unmap actually treat it
as a wildcard owner -- free anything that's already there, and if
there's no owner at all then that's fine too.

There are two existing callers of bmap_add_free that take care the rmap
deferred ops themselves and use OWN_UNKNOWN to skip the EFI-based rmap
cleanup; convert these to use OWN_NULL, and ensure that the RUI gets
added to the defer ops ahead of any EFI.

Lastly, now that xfs_free_extent filters out OWN_NULL rmap free requests,
growfs will have to consult directly with the rmap to ensure that there
aren't any rmaps in the grown region.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c    |    2 +-
 fs/xfs/libxfs/xfs_bmap.c     |    2 +-
 fs/xfs/libxfs/xfs_refcount.c |   52 +++++++++++++++---------------------------
 fs/xfs/libxfs/xfs_rmap.c     |   15 +++++++++---
 fs/xfs/xfs_fsops.c           |    5 ++++
 5 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index a840028..0f260eeb 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -1696,7 +1696,7 @@ xfs_free_ag_extent(
 	bno_cur = cnt_cur = NULL;
 	mp = tp->t_mountp;
 
-	if (oinfo->oi_owner != XFS_RMAP_OWN_UNKNOWN) {
+	if (oinfo->oi_owner != XFS_RMAP_OWN_NULL) {
 		error = xfs_rmap_free(tp, agbp, agno, bno, len, oinfo);
 		if (error)
 			goto error0;
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 16df627..89bb3d9 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -573,7 +573,7 @@ xfs_bmap_add_free(
 	if (oinfo)
 		new->xefi_oinfo = *oinfo;
 	else
-		xfs_rmap_skip_owner_update(&new->xefi_oinfo);
+		xfs_rmap_ag_owner(&new->xefi_oinfo, XFS_RMAP_OWN_NULL);
 	trace_xfs_bmap_free_defer(mp, XFS_FSB_TO_AGNO(mp, bno), 0,
 			XFS_FSB_TO_AGBNO(mp, bno), len);
 	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_FREE, &new->xefi_list);
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 73f8058..9103be0 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1505,27 +1505,12 @@ __xfs_refcount_cow_alloc(
 	xfs_extlen_t		aglen,
 	struct xfs_defer_ops	*dfops)
 {
-	int			error;
-
 	trace_xfs_refcount_cow_increase(rcur->bc_mp, rcur->bc_private.a.agno,
 			agbno, aglen);
 
 	/* Add refcount btree reservation */
-	error = xfs_refcount_adjust_cow(rcur, agbno, aglen,
+	return xfs_refcount_adjust_cow(rcur, agbno, aglen,
 			XFS_REFCOUNT_ADJUST_COW_ALLOC, dfops);
-	if (error)
-		return error;
-
-	/* Add rmap entry */
-	if (xfs_sb_version_hasrmapbt(&rcur->bc_mp->m_sb)) {
-		error = xfs_rmap_alloc_extent(rcur->bc_mp, dfops,
-				rcur->bc_private.a.agno,
-				agbno, aglen, XFS_RMAP_OWN_COW);
-		if (error)
-			return error;
-	}
-
-	return error;
 }
 
 /*
@@ -1538,27 +1523,12 @@ __xfs_refcount_cow_free(
 	xfs_extlen_t		aglen,
 	struct xfs_defer_ops	*dfops)
 {
-	int			error;
-
 	trace_xfs_refcount_cow_decrease(rcur->bc_mp, rcur->bc_private.a.agno,
 			agbno, aglen);
 
 	/* Remove refcount btree reservation */
-	error = xfs_refcount_adjust_cow(rcur, agbno, aglen,
+	return xfs_refcount_adjust_cow(rcur, agbno, aglen,
 			XFS_REFCOUNT_ADJUST_COW_FREE, dfops);
-	if (error)
-		return error;
-
-	/* Remove rmap entry */
-	if (xfs_sb_version_hasrmapbt(&rcur->bc_mp->m_sb)) {
-		error = xfs_rmap_free_extent(rcur->bc_mp, dfops,
-				rcur->bc_private.a.agno,
-				agbno, aglen, XFS_RMAP_OWN_COW);
-		if (error)
-			return error;
-	}
-
-	return error;
 }
 
 /* Record a CoW staging extent in the refcount btree. */
@@ -1569,11 +1539,19 @@ xfs_refcount_alloc_cow_extent(
 	xfs_fsblock_t			fsb,
 	xfs_extlen_t			len)
 {
+	int				error;
+
 	if (!xfs_sb_version_hasreflink(&mp->m_sb))
 		return 0;
 
-	return __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_ALLOC_COW,
+	error = __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_ALLOC_COW,
 			fsb, len);
+	if (error)
+		return error;
+
+	/* Add rmap entry */
+	return xfs_rmap_alloc_extent(mp, dfops, XFS_FSB_TO_AGNO(mp, fsb),
+			XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
 }
 
 /* Forget a CoW staging event in the refcount btree. */
@@ -1584,9 +1562,17 @@ xfs_refcount_free_cow_extent(
 	xfs_fsblock_t			fsb,
 	xfs_extlen_t			len)
 {
+	int				error;
+
 	if (!xfs_sb_version_hasreflink(&mp->m_sb))
 		return 0;
 
+	/* Remove rmap entry */
+	error = xfs_rmap_free_extent(mp, dfops, XFS_FSB_TO_AGNO(mp, fsb),
+			XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
+	if (error)
+		return error;
+
 	return __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_FREE_COW,
 			fsb, len);
 }
diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index 5f3a3d9..fd0e630 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -484,10 +484,17 @@ xfs_rmap_unmap(
 	XFS_WANT_CORRUPTED_GOTO(mp, (flags & XFS_RMAP_UNWRITTEN) ==
 			(ltrec.rm_flags & XFS_RMAP_UNWRITTEN), out_error);
 
-	/* Make sure the extent we found covers the entire freeing range. */
-	XFS_WANT_CORRUPTED_GOTO(mp, ltrec.rm_startblock <= bno &&
-		ltrec.rm_startblock + ltrec.rm_blockcount >=
-		bno + len, out_error);
+	/*
+	 * Make sure the extent we found covers the entire freeing range.
+	 * If this is a wildcard free, we're already done, otherwise there's
+	 * something wrong with the rmapbt.
+	 */
+	if (ltrec.rm_startblock > bno ||
+	    ltrec.rm_startblock + ltrec.rm_blockcount < bno + len) {
+		if (owner == XFS_RMAP_OWN_UNKNOWN)
+			goto out_done;
+		XFS_WANT_CORRUPTED_GOTO(mp, false, out_error);
+	}
 
 	/* Make sure the owner matches what we expect to find in the tree. */
 	XFS_WANT_CORRUPTED_GOTO(mp, owner == ltrec.rm_owner ||
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 8f22fc5..60a2e12 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -571,6 +571,11 @@ xfs_growfs_data_private(
 		 * this doesn't actually exist in the rmap btree.
 		 */
 		xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_NULL);
+		error = xfs_rmap_free(tp, bp, agno,
+				be32_to_cpu(agf->agf_length) - new,
+				new, &oinfo);
+		if (error)
+			goto error0;
 		error = xfs_free_extent(tp,
 				XFS_AGB_TO_FSB(mp, agno,
 					be32_to_cpu(agf->agf_length) - new),

Re: extfree log recovery and owner (rmapbt) updates

[Dave Chinner]

On Tue, Dec 05, 2017 at 01:55:24PM -0500, Brian Foster wrote:
> Hi,
> 
> I've been playing around with deferring AGFL block frees and noticed
> something funky going on with xfs_bmap_add_free() and owner info. We
> pass the extent owner info to xfs_bmap_add_free() in various places to
> transfer this info to the deferred free processing context.
> Subsequently, xfs_trans_free_extent() -> ... -> xfs_free_ag_extent()
> uses the owner info to remove the rmapbt entry associated with the
> extent. The rmapbt update occurs at the time the extent is freed.
> 
> If the system crashes immediately after the initial transaction commits,
> however, EFI recovery calls xfs_trans_free_extent() with an "unknown"
> owner. I see a reference to using a "wildcard" in this context in some
> of the old rmapbt commits,

Yup, that was my original intent - to deal with log recovery not
knowing who the owner of the extent being freed is as it's not
recorded in the EFI.

IOWs, XFS_RMAP_OWN_UNKNOWN was supposed to match any owner in
the rmapbt record and allow it to be removed instead of throwing a
corruption error because a the owner field didn't match the rmap
extent record we found on lookup.

It seems that this morphed during the refcount updates to the rmapbt
record structure and I didn't catch that during review. i.e. once
cow and unsharing are added into the picture, the rmapbt updates are
no longer a 1:1 mapping with extent freeing....

> but it looks like this codepath skips the
> rmapbt update altogether if oi_owner == XFS_RMAP_OWN_UNKNOWN.

Yup, that looks like a bug at first glance, but I think what we
really need to determine here is whether there is a deferred rmap
update for that extent being freed or not. That will determine what
needs to happen when processing the EFI - if there's a deferred
rmap update, then we don't want to free the rmap record here, but
if there isn't an rmap update, then we need to free it from EFI
context...

> A quick
> test that shuts down the fs immediately after a transaction commits that
> logs an EFI for a freed inode chunk leaves the fs inconsistent after log
> recovery, with a bit of a cryptic message from repair:
> 
>   unknown block state, ag 0, block 24

That would explain the failures I occasionally see from an
fstest whose number I can't recall right now.

> It does look like the associated rmapbt entry still exists in the tree
> even though the extent has been freed (from xfs_db -c fsmap):
> 
> 5: 0/24 len 8 owner -7 offset 0 bmbt 0 attrfork 0 extflag 0                                                            
> 
> So what is supposed to be going on here? Should the rmapbt entry be
> removed unconditionally during recovery, or should a separate rmapbt
> update item be deferred (as in the bunmapi case, for example) rather
> than passing oinfo along with the TYPE_FREE deferred item? Or something
> else entirely..?

I'm thinking it depends on whether we've got a deferred RUI for the
extent being freed pending in the recovery list or not...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH] xfs: always honor OWN_UNKNOWN rmap removal requests

[Brian Foster]

On Tue, Dec 05, 2017 at 03:34:20PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Calling xfs_rmap_free with an unknown owner is supposed to remove any
> rmaps covering that range regardless of owner.  This is used by the EFI
> recovery code to say "we're freeing this, it mustn't be owned by
> anything anymore", but for whatever reason xfs_free_ag_extent filters
> them out.
> 
> Therefore, remove the filter and make xfs_rmap_unmap actually treat it
> as a wildcard owner -- free anything that's already there, and if
> there's no owner at all then that's fine too.
> 
> There are two existing callers of bmap_add_free that take care the rmap
> deferred ops themselves and use OWN_UNKNOWN to skip the EFI-based rmap
> cleanup; convert these to use OWN_NULL, and ensure that the RUI gets
> added to the defer ops ahead of any EFI.
> 
> Lastly, now that xfs_free_extent filters out OWN_NULL rmap free requests,
> growfs will have to consult directly with the rmap to ensure that there
> aren't any rmaps in the grown region.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Thanks... this resolves the log recovery problem on a quick test.

>  fs/xfs/libxfs/xfs_alloc.c    |    2 +-
>  fs/xfs/libxfs/xfs_bmap.c     |    2 +-
>  fs/xfs/libxfs/xfs_refcount.c |   52 +++++++++++++++---------------------------
>  fs/xfs/libxfs/xfs_rmap.c     |   15 +++++++++---
>  fs/xfs/xfs_fsops.c           |    5 ++++
>  5 files changed, 37 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index a840028..0f260eeb 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -1696,7 +1696,7 @@ xfs_free_ag_extent(
>  	bno_cur = cnt_cur = NULL;
>  	mp = tp->t_mountp;
>  
> -	if (oinfo->oi_owner != XFS_RMAP_OWN_UNKNOWN) {
> +	if (oinfo->oi_owner != XFS_RMAP_OWN_NULL) {
>  		error = xfs_rmap_free(tp, agbp, agno, bno, len, oinfo);
>  		if (error)
>  			goto error0;
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 16df627..89bb3d9 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -573,7 +573,7 @@ xfs_bmap_add_free(
>  	if (oinfo)
>  		new->xefi_oinfo = *oinfo;
>  	else
> -		xfs_rmap_skip_owner_update(&new->xefi_oinfo);
> +		xfs_rmap_ag_owner(&new->xefi_oinfo, XFS_RMAP_OWN_NULL);

So what is the difference now between xfs_rmap_skip_owner_update(),
which sets OWN_UNKNOWN, and OWN_NULL, which skips owner updates in
certain cases? Should we be using OWN_NULL consistently to skip owner
updates (not that UNKNOWN makes much sense in some of the other cases,
like allocation).

>  	trace_xfs_bmap_free_defer(mp, XFS_FSB_TO_AGNO(mp, bno), 0,
>  			XFS_FSB_TO_AGBNO(mp, bno), len);
>  	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_FREE, &new->xefi_list);
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 73f8058..9103be0 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -1505,27 +1505,12 @@ __xfs_refcount_cow_alloc(
>  	xfs_extlen_t		aglen,
>  	struct xfs_defer_ops	*dfops)
>  {
> -	int			error;
> -
>  	trace_xfs_refcount_cow_increase(rcur->bc_mp, rcur->bc_private.a.agno,
>  			agbno, aglen);
>  
>  	/* Add refcount btree reservation */
> -	error = xfs_refcount_adjust_cow(rcur, agbno, aglen,
> +	return xfs_refcount_adjust_cow(rcur, agbno, aglen,
>  			XFS_REFCOUNT_ADJUST_COW_ALLOC, dfops);
> -	if (error)
> -		return error;
> -
> -	/* Add rmap entry */
> -	if (xfs_sb_version_hasrmapbt(&rcur->bc_mp->m_sb)) {
> -		error = xfs_rmap_alloc_extent(rcur->bc_mp, dfops,
> -				rcur->bc_private.a.agno,
> -				agbno, aglen, XFS_RMAP_OWN_COW);
> -		if (error)
> -			return error;
> -	}
> -
> -	return error;
>  }

I think the refcount fixup probably warrants an independent patch with a
more detailed commit log around the ordering requirement and how this
changes behavior.

>  
>  /*
> @@ -1538,27 +1523,12 @@ __xfs_refcount_cow_free(
>  	xfs_extlen_t		aglen,
>  	struct xfs_defer_ops	*dfops)
>  {
> -	int			error;
> -
>  	trace_xfs_refcount_cow_decrease(rcur->bc_mp, rcur->bc_private.a.agno,
>  			agbno, aglen);
>  
>  	/* Remove refcount btree reservation */
> -	error = xfs_refcount_adjust_cow(rcur, agbno, aglen,
> +	return xfs_refcount_adjust_cow(rcur, agbno, aglen,
>  			XFS_REFCOUNT_ADJUST_COW_FREE, dfops);

xfs_refcount_finish_one() -> xfs_refcount_cow_[alloc|free]() ->
xfs_refcount_adjust_cow() -> ...

Hmm, seems like there's opportunity for more cleanup here. Do we really
need separate xfs_refcount_cow_*() functions just for tracepoints? Seems
like we could just fold these into xfs_refcount_finish_one().

> -	if (error)
> -		return error;
> -
> -	/* Remove rmap entry */
> -	if (xfs_sb_version_hasrmapbt(&rcur->bc_mp->m_sb)) {
> -		error = xfs_rmap_free_extent(rcur->bc_mp, dfops,
> -				rcur->bc_private.a.agno,
> -				agbno, aglen, XFS_RMAP_OWN_COW);
> -		if (error)
> -			return error;
> -	}
> -
> -	return error;
>  }
>  
>  /* Record a CoW staging extent in the refcount btree. */
> @@ -1569,11 +1539,19 @@ xfs_refcount_alloc_cow_extent(
>  	xfs_fsblock_t			fsb,
>  	xfs_extlen_t			len)
>  {
> +	int				error;
> +
>  	if (!xfs_sb_version_hasreflink(&mp->m_sb))
>  		return 0;
>  
> -	return __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_ALLOC_COW,
> +	error = __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_ALLOC_COW,
>  			fsb, len);
> +	if (error)
> +		return error;
> +
> +	/* Add rmap entry */
> +	return xfs_rmap_alloc_extent(mp, dfops, XFS_FSB_TO_AGNO(mp, fsb),
> +			XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
>  }
>  
>  /* Forget a CoW staging event in the refcount btree. */
> @@ -1584,9 +1562,17 @@ xfs_refcount_free_cow_extent(
>  	xfs_fsblock_t			fsb,
>  	xfs_extlen_t			len)
>  {
> +	int				error;
> +
>  	if (!xfs_sb_version_hasreflink(&mp->m_sb))
>  		return 0;
>  
> +	/* Remove rmap entry */
> +	error = xfs_rmap_free_extent(mp, dfops, XFS_FSB_TO_AGNO(mp, fsb),
> +			XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
> +	if (error)
> +		return error;
> +
>  	return __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_FREE_COW,
>  			fsb, len);
>  }
> diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> index 5f3a3d9..fd0e630 100644
> --- a/fs/xfs/libxfs/xfs_rmap.c
> +++ b/fs/xfs/libxfs/xfs_rmap.c
> @@ -484,10 +484,17 @@ xfs_rmap_unmap(
>  	XFS_WANT_CORRUPTED_GOTO(mp, (flags & XFS_RMAP_UNWRITTEN) ==
>  			(ltrec.rm_flags & XFS_RMAP_UNWRITTEN), out_error);
>  
> -	/* Make sure the extent we found covers the entire freeing range. */
> -	XFS_WANT_CORRUPTED_GOTO(mp, ltrec.rm_startblock <= bno &&
> -		ltrec.rm_startblock + ltrec.rm_blockcount >=
> -		bno + len, out_error);
> +	/*
> +	 * Make sure the extent we found covers the entire freeing range.
> +	 * If this is a wildcard free, we're already done, otherwise there's
> +	 * something wrong with the rmapbt.
> +	 */

What does this mean by "we're already done?" This logic appears to mean
that we don't do anything (as opposed to throwing an error). I think the
comment would be more clear if it pointed out that/why we have nothing
to do here (due to OWN_UNKNOWN). I.e., caller passed in a wildcard and
we essentially didn't find a match..?

> +	if (ltrec.rm_startblock > bno ||
> +	    ltrec.rm_startblock + ltrec.rm_blockcount < bno + len) {
> +		if (owner == XFS_RMAP_OWN_UNKNOWN)
> +			goto out_done;
> +		XFS_WANT_CORRUPTED_GOTO(mp, false, out_error);
> +	}
>  

Also... unrelated, but is this check immediately below really intending
to ignore owner inconsistencies for all !inode owners?

>  	/* Make sure the owner matches what we expect to find in the tree. */
>  	XFS_WANT_CORRUPTED_GOTO(mp, owner == ltrec.rm_owner ||
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 8f22fc5..60a2e12 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -571,6 +571,11 @@ xfs_growfs_data_private(
>  		 * this doesn't actually exist in the rmap btree.
>  		 */
>  		xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_NULL);
> +		error = xfs_rmap_free(tp, bp, agno,
> +				be32_to_cpu(agf->agf_length) - new,
> +				new, &oinfo);
> +		if (error)
> +			goto error0;

OWN_NULL makes sense from the perspective of needing to avoid some error
down in the free code where we need to free some space without needing
to remove an owner, but what is the purpose of the above? It doesn't
look like this really does anything beyond checking that the associated
space is beyond the end of the rmapbt. If that's the intent, then it
probably makes sense to update this comment as well.

Brian

>  		error = xfs_free_extent(tp,
>  				XFS_AGB_TO_FSB(mp, agno,
>  					be32_to_cpu(agf->agf_length) - new),
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] xfs: always honor OWN_UNKNOWN rmap removal requests

[Darrick J. Wong]

On Wed, Dec 06, 2017 at 09:14:07AM -0500, Brian Foster wrote:
> On Tue, Dec 05, 2017 at 03:34:20PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Calling xfs_rmap_free with an unknown owner is supposed to remove any
> > rmaps covering that range regardless of owner.  This is used by the EFI
> > recovery code to say "we're freeing this, it mustn't be owned by
> > anything anymore", but for whatever reason xfs_free_ag_extent filters
> > them out.
> > 
> > Therefore, remove the filter and make xfs_rmap_unmap actually treat it
> > as a wildcard owner -- free anything that's already there, and if
> > there's no owner at all then that's fine too.
> > 
> > There are two existing callers of bmap_add_free that take care the rmap
> > deferred ops themselves and use OWN_UNKNOWN to skip the EFI-based rmap
> > cleanup; convert these to use OWN_NULL, and ensure that the RUI gets
> > added to the defer ops ahead of any EFI.
> > 
> > Lastly, now that xfs_free_extent filters out OWN_NULL rmap free requests,
> > growfs will have to consult directly with the rmap to ensure that there
> > aren't any rmaps in the grown region.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> Thanks... this resolves the log recovery problem on a quick test.
> 
> >  fs/xfs/libxfs/xfs_alloc.c    |    2 +-
> >  fs/xfs/libxfs/xfs_bmap.c     |    2 +-
> >  fs/xfs/libxfs/xfs_refcount.c |   52 +++++++++++++++---------------------------
> >  fs/xfs/libxfs/xfs_rmap.c     |   15 +++++++++---
> >  fs/xfs/xfs_fsops.c           |    5 ++++
> >  5 files changed, 37 insertions(+), 39 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index a840028..0f260eeb 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -1696,7 +1696,7 @@ xfs_free_ag_extent(
> >  	bno_cur = cnt_cur = NULL;
> >  	mp = tp->t_mountp;
> >  
> > -	if (oinfo->oi_owner != XFS_RMAP_OWN_UNKNOWN) {
> > +	if (oinfo->oi_owner != XFS_RMAP_OWN_NULL) {
> >  		error = xfs_rmap_free(tp, agbp, agno, bno, len, oinfo);
> >  		if (error)
> >  			goto error0;
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 16df627..89bb3d9 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -573,7 +573,7 @@ xfs_bmap_add_free(
> >  	if (oinfo)
> >  		new->xefi_oinfo = *oinfo;
> >  	else
> > -		xfs_rmap_skip_owner_update(&new->xefi_oinfo);
> > +		xfs_rmap_ag_owner(&new->xefi_oinfo, XFS_RMAP_OWN_NULL);
> 
> So what is the difference now between xfs_rmap_skip_owner_update(),
> which sets OWN_UNKNOWN, and OWN_NULL, which skips owner updates in
> certain cases? Should we be using OWN_NULL consistently to skip owner
> updates (not that UNKNOWN makes much sense in some of the other cases,
> like allocation).

Yeah, there's a bunch of cleanups that I was intending to do (most of
which you've caught below) prior to making a non-RFC submission.

> >  	trace_xfs_bmap_free_defer(mp, XFS_FSB_TO_AGNO(mp, bno), 0,
> >  			XFS_FSB_TO_AGBNO(mp, bno), len);
> >  	xfs_defer_add(dfops, XFS_DEFER_OPS_TYPE_FREE, &new->xefi_list);
> > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > index 73f8058..9103be0 100644
> > --- a/fs/xfs/libxfs/xfs_refcount.c
> > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > @@ -1505,27 +1505,12 @@ __xfs_refcount_cow_alloc(
> >  	xfs_extlen_t		aglen,
> >  	struct xfs_defer_ops	*dfops)
> >  {
> > -	int			error;
> > -
> >  	trace_xfs_refcount_cow_increase(rcur->bc_mp, rcur->bc_private.a.agno,
> >  			agbno, aglen);
> >  
> >  	/* Add refcount btree reservation */
> > -	error = xfs_refcount_adjust_cow(rcur, agbno, aglen,
> > +	return xfs_refcount_adjust_cow(rcur, agbno, aglen,
> >  			XFS_REFCOUNT_ADJUST_COW_ALLOC, dfops);
> > -	if (error)
> > -		return error;
> > -
> > -	/* Add rmap entry */
> > -	if (xfs_sb_version_hasrmapbt(&rcur->bc_mp->m_sb)) {
> > -		error = xfs_rmap_alloc_extent(rcur->bc_mp, dfops,
> > -				rcur->bc_private.a.agno,
> > -				agbno, aglen, XFS_RMAP_OWN_COW);
> > -		if (error)
> > -			return error;
> > -	}
> > -
> > -	return error;
> >  }
> 
> I think the refcount fixup probably warrants an independent patch with a
> more detailed commit log around the ordering requirement and how this
> changes behavior.

Yep.

> >  
> >  /*
> > @@ -1538,27 +1523,12 @@ __xfs_refcount_cow_free(
> >  	xfs_extlen_t		aglen,
> >  	struct xfs_defer_ops	*dfops)
> >  {
> > -	int			error;
> > -
> >  	trace_xfs_refcount_cow_decrease(rcur->bc_mp, rcur->bc_private.a.agno,
> >  			agbno, aglen);
> >  
> >  	/* Remove refcount btree reservation */
> > -	error = xfs_refcount_adjust_cow(rcur, agbno, aglen,
> > +	return xfs_refcount_adjust_cow(rcur, agbno, aglen,
> >  			XFS_REFCOUNT_ADJUST_COW_FREE, dfops);
> 
> xfs_refcount_finish_one() -> xfs_refcount_cow_[alloc|free]() ->
> xfs_refcount_adjust_cow() -> ...
> 
> Hmm, seems like there's opportunity for more cleanup here. Do we really
> need separate xfs_refcount_cow_*() functions just for tracepoints? Seems
> like we could just fold these into xfs_refcount_finish_one().

Yep.

> > -	if (error)
> > -		return error;
> > -
> > -	/* Remove rmap entry */
> > -	if (xfs_sb_version_hasrmapbt(&rcur->bc_mp->m_sb)) {
> > -		error = xfs_rmap_free_extent(rcur->bc_mp, dfops,
> > -				rcur->bc_private.a.agno,
> > -				agbno, aglen, XFS_RMAP_OWN_COW);
> > -		if (error)
> > -			return error;
> > -	}
> > -
> > -	return error;
> >  }
> >  
> >  /* Record a CoW staging extent in the refcount btree. */
> > @@ -1569,11 +1539,19 @@ xfs_refcount_alloc_cow_extent(
> >  	xfs_fsblock_t			fsb,
> >  	xfs_extlen_t			len)
> >  {
> > +	int				error;
> > +
> >  	if (!xfs_sb_version_hasreflink(&mp->m_sb))
> >  		return 0;
> >  
> > -	return __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_ALLOC_COW,
> > +	error = __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_ALLOC_COW,
> >  			fsb, len);
> > +	if (error)
> > +		return error;
> > +
> > +	/* Add rmap entry */
> > +	return xfs_rmap_alloc_extent(mp, dfops, XFS_FSB_TO_AGNO(mp, fsb),
> > +			XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
> >  }
> >  
> >  /* Forget a CoW staging event in the refcount btree. */
> > @@ -1584,9 +1562,17 @@ xfs_refcount_free_cow_extent(
> >  	xfs_fsblock_t			fsb,
> >  	xfs_extlen_t			len)
> >  {
> > +	int				error;
> > +
> >  	if (!xfs_sb_version_hasreflink(&mp->m_sb))
> >  		return 0;
> >  
> > +	/* Remove rmap entry */
> > +	error = xfs_rmap_free_extent(mp, dfops, XFS_FSB_TO_AGNO(mp, fsb),
> > +			XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
> > +	if (error)
> > +		return error;
> > +
> >  	return __xfs_refcount_add(mp, dfops, XFS_REFCOUNT_FREE_COW,
> >  			fsb, len);
> >  }
> > diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> > index 5f3a3d9..fd0e630 100644
> > --- a/fs/xfs/libxfs/xfs_rmap.c
> > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > @@ -484,10 +484,17 @@ xfs_rmap_unmap(
> >  	XFS_WANT_CORRUPTED_GOTO(mp, (flags & XFS_RMAP_UNWRITTEN) ==
> >  			(ltrec.rm_flags & XFS_RMAP_UNWRITTEN), out_error);
> >  
> > -	/* Make sure the extent we found covers the entire freeing range. */
> > -	XFS_WANT_CORRUPTED_GOTO(mp, ltrec.rm_startblock <= bno &&
> > -		ltrec.rm_startblock + ltrec.rm_blockcount >=
> > -		bno + len, out_error);
> > +	/*
> > +	 * Make sure the extent we found covers the entire freeing range.
> > +	 * If this is a wildcard free, we're already done, otherwise there's
> > +	 * something wrong with the rmapbt.
> > +	 */
> 
> What does this mean by "we're already done?" This logic appears to mean
> that we don't do anything (as opposed to throwing an error). I think the
> comment would be more clear if it pointed out that/why we have nothing
> to do here (due to OWN_UNKNOWN). I.e., caller passed in a wildcard and
> we essentially didn't find a match..?

"Make sure the extent we found covers the entire freeing range.  Passing
in an owner of OWN_UNKNOWN means that the caller wants to remove any
reverse mapping that may exist for this range of blocks regardless of
owner; if there are no mappings at all, we're done."

> > +	if (ltrec.rm_startblock > bno ||
> > +	    ltrec.rm_startblock + ltrec.rm_blockcount < bno + len) {
> > +		if (owner == XFS_RMAP_OWN_UNKNOWN)
> > +			goto out_done;
> > +		XFS_WANT_CORRUPTED_GOTO(mp, false, out_error);
> > +	}
> >  
> 
> Also... unrelated, but is this check immediately below really intending
> to ignore owner inconsistencies for all !inode owners?

I had my eye on that one too, though I think that could be a
freestanding cleanup.

> >  	/* Make sure the owner matches what we expect to find in the tree. */
> >  	XFS_WANT_CORRUPTED_GOTO(mp, owner == ltrec.rm_owner ||
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index 8f22fc5..60a2e12 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -571,6 +571,11 @@ xfs_growfs_data_private(
> >  		 * this doesn't actually exist in the rmap btree.
> >  		 */
> >  		xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_NULL);
> > +		error = xfs_rmap_free(tp, bp, agno,
> > +				be32_to_cpu(agf->agf_length) - new,
> > +				new, &oinfo);
> > +		if (error)
> > +			goto error0;
> 
> OWN_NULL makes sense from the perspective of needing to avoid some error
> down in the free code where we need to free some space without needing
> to remove an owner, but what is the purpose of the above? It doesn't
> look like this really does anything beyond checking that the associated
> space is beyond the end of the rmapbt. If that's the intent, then it
> probably makes sense to update this comment as well.

Yes, that's exactly the intent.

Hmm, come to think of it, the rmap xref patch adds a
xfs_rmap_has_record helper that does exactly what we want here (decides
if there are any records covering this range).

--D

> Brian
> 
> >  		error = xfs_free_extent(tp,
> >  				XFS_AGB_TO_FSB(mp, agno,
> >  					be32_to_cpu(agf->agf_length) - new),
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] xfs: always honor OWN_UNKNOWN rmap removal requests

[Brian Foster]

On Wed, Dec 06, 2017 at 09:53:00AM -0800, Darrick J. Wong wrote:
> On Wed, Dec 06, 2017 at 09:14:07AM -0500, Brian Foster wrote:
> > On Tue, Dec 05, 2017 at 03:34:20PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Calling xfs_rmap_free with an unknown owner is supposed to remove any
> > > rmaps covering that range regardless of owner.  This is used by the EFI
> > > recovery code to say "we're freeing this, it mustn't be owned by
> > > anything anymore", but for whatever reason xfs_free_ag_extent filters
> > > them out.
> > > 
> > > Therefore, remove the filter and make xfs_rmap_unmap actually treat it
> > > as a wildcard owner -- free anything that's already there, and if
> > > there's no owner at all then that's fine too.
> > > 
> > > There are two existing callers of bmap_add_free that take care the rmap
> > > deferred ops themselves and use OWN_UNKNOWN to skip the EFI-based rmap
> > > cleanup; convert these to use OWN_NULL, and ensure that the RUI gets
> > > added to the defer ops ahead of any EFI.
> > > 
> > > Lastly, now that xfs_free_extent filters out OWN_NULL rmap free requests,
> > > growfs will have to consult directly with the rmap to ensure that there
> > > aren't any rmaps in the grown region.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > 
...
> > > diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> > > index 5f3a3d9..fd0e630 100644
> > > --- a/fs/xfs/libxfs/xfs_rmap.c
> > > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > > @@ -484,10 +484,17 @@ xfs_rmap_unmap(
> > >  	XFS_WANT_CORRUPTED_GOTO(mp, (flags & XFS_RMAP_UNWRITTEN) ==
> > >  			(ltrec.rm_flags & XFS_RMAP_UNWRITTEN), out_error);
> > >  
> > > -	/* Make sure the extent we found covers the entire freeing range. */
> > > -	XFS_WANT_CORRUPTED_GOTO(mp, ltrec.rm_startblock <= bno &&
> > > -		ltrec.rm_startblock + ltrec.rm_blockcount >=
> > > -		bno + len, out_error);
> > > +	/*
> > > +	 * Make sure the extent we found covers the entire freeing range.
> > > +	 * If this is a wildcard free, we're already done, otherwise there's
> > > +	 * something wrong with the rmapbt.
> > > +	 */
> > 
> > What does this mean by "we're already done?" This logic appears to mean
> > that we don't do anything (as opposed to throwing an error). I think the
> > comment would be more clear if it pointed out that/why we have nothing
> > to do here (due to OWN_UNKNOWN). I.e., caller passed in a wildcard and
> > we essentially didn't find a match..?
> 
> "Make sure the extent we found covers the entire freeing range.  Passing
> in an owner of OWN_UNKNOWN means that the caller wants to remove any
> reverse mapping that may exist for this range of blocks regardless of
> owner; if there are no mappings at all, we're done."
> 

Looking at this again, I find it a bit confusing how this check seems to
double as a "nothing to do" in the unknown case and a corruption error
otherwise. For example, is something still technically wrong if we get
an UNKNOWN unmap request (aka an extent free) to unmap a range that
overlaps with but starts before or extends past the range in the rmapbt?
I could easily be missing something here, but otherwise I wonder if this
would be better as separate checks so we don't lose some of the error
checking coverage.

Brian

> > > +	if (ltrec.rm_startblock > bno ||
> > > +	    ltrec.rm_startblock + ltrec.rm_blockcount < bno + len) {
> > > +		if (owner == XFS_RMAP_OWN_UNKNOWN)
> > > +			goto out_done;
> > > +		XFS_WANT_CORRUPTED_GOTO(mp, false, out_error);
> > > +	}
> > >  
> > 
> > Also... unrelated, but is this check immediately below really intending
> > to ignore owner inconsistencies for all !inode owners?
> 
> I had my eye on that one too, though I think that could be a
> freestanding cleanup.
> 
> > >  	/* Make sure the owner matches what we expect to find in the tree. */
> > >  	XFS_WANT_CORRUPTED_GOTO(mp, owner == ltrec.rm_owner ||
> > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > index 8f22fc5..60a2e12 100644
> > > --- a/fs/xfs/xfs_fsops.c
> > > +++ b/fs/xfs/xfs_fsops.c
> > > @@ -571,6 +571,11 @@ xfs_growfs_data_private(
> > >  		 * this doesn't actually exist in the rmap btree.
> > >  		 */
> > >  		xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_NULL);
> > > +		error = xfs_rmap_free(tp, bp, agno,
> > > +				be32_to_cpu(agf->agf_length) - new,
> > > +				new, &oinfo);
> > > +		if (error)
> > > +			goto error0;
> > 
> > OWN_NULL makes sense from the perspective of needing to avoid some error
> > down in the free code where we need to free some space without needing
> > to remove an owner, but what is the purpose of the above? It doesn't
> > look like this really does anything beyond checking that the associated
> > space is beyond the end of the rmapbt. If that's the intent, then it
> > probably makes sense to update this comment as well.
> 
> Yes, that's exactly the intent.
> 
> Hmm, come to think of it, the rmap xref patch adds a
> xfs_rmap_has_record helper that does exactly what we want here (decides
> if there are any records covering this range).
> 
> --D
> 
> > Brian
> > 
> > >  		error = xfs_free_extent(tp,
> > >  				XFS_AGB_TO_FSB(mp, agno,
> > >  					be32_to_cpu(agf->agf_length) - new),
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] xfs: always honor OWN_UNKNOWN rmap removal requests

[Darrick J. Wong]

On Wed, Dec 06, 2017 at 03:49:46PM -0500, Brian Foster wrote:
> On Wed, Dec 06, 2017 at 09:53:00AM -0800, Darrick J. Wong wrote:
> > On Wed, Dec 06, 2017 at 09:14:07AM -0500, Brian Foster wrote:
> > > On Tue, Dec 05, 2017 at 03:34:20PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Calling xfs_rmap_free with an unknown owner is supposed to remove any
> > > > rmaps covering that range regardless of owner.  This is used by the EFI
> > > > recovery code to say "we're freeing this, it mustn't be owned by
> > > > anything anymore", but for whatever reason xfs_free_ag_extent filters
> > > > them out.
> > > > 
> > > > Therefore, remove the filter and make xfs_rmap_unmap actually treat it
> > > > as a wildcard owner -- free anything that's already there, and if
> > > > there's no owner at all then that's fine too.
> > > > 
> > > > There are two existing callers of bmap_add_free that take care the rmap
> > > > deferred ops themselves and use OWN_UNKNOWN to skip the EFI-based rmap
> > > > cleanup; convert these to use OWN_NULL, and ensure that the RUI gets
> > > > added to the defer ops ahead of any EFI.
> > > > 
> > > > Lastly, now that xfs_free_extent filters out OWN_NULL rmap free requests,
> > > > growfs will have to consult directly with the rmap to ensure that there
> > > > aren't any rmaps in the grown region.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > 
> ...
> > > > diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> > > > index 5f3a3d9..fd0e630 100644
> > > > --- a/fs/xfs/libxfs/xfs_rmap.c
> > > > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > > > @@ -484,10 +484,17 @@ xfs_rmap_unmap(
> > > >  	XFS_WANT_CORRUPTED_GOTO(mp, (flags & XFS_RMAP_UNWRITTEN) ==
> > > >  			(ltrec.rm_flags & XFS_RMAP_UNWRITTEN), out_error);
> > > >  
> > > > -	/* Make sure the extent we found covers the entire freeing range. */
> > > > -	XFS_WANT_CORRUPTED_GOTO(mp, ltrec.rm_startblock <= bno &&
> > > > -		ltrec.rm_startblock + ltrec.rm_blockcount >=
> > > > -		bno + len, out_error);
> > > > +	/*
> > > > +	 * Make sure the extent we found covers the entire freeing range.
> > > > +	 * If this is a wildcard free, we're already done, otherwise there's
> > > > +	 * something wrong with the rmapbt.
> > > > +	 */
> > > 
> > > What does this mean by "we're already done?" This logic appears to mean
> > > that we don't do anything (as opposed to throwing an error). I think the
> > > comment would be more clear if it pointed out that/why we have nothing
> > > to do here (due to OWN_UNKNOWN). I.e., caller passed in a wildcard and
> > > we essentially didn't find a match..?
> > 
> > "Make sure the extent we found covers the entire freeing range.  Passing
> > in an owner of OWN_UNKNOWN means that the caller wants to remove any
> > reverse mapping that may exist for this range of blocks regardless of
> > owner; if there are no mappings at all, we're done."
> > 
> 
> Looking at this again, I find it a bit confusing how this check seems to
> double as a "nothing to do" in the unknown case and a corruption error
> otherwise. For example, is something still technically wrong if we get
> an UNKNOWN unmap request (aka an extent free) to unmap a range that
> overlaps with but starts before or extends past the range in the rmapbt?

I .... hmm, bear with my stream of consciousness:

Start with the assumption that for each rmap free request there is
exactly one rmap that overlaps with the request (X and Y are existing
rmaps, R is the range we want to remove):

XXXXX....YYYY
   RR

XXXXX....YYYY
         RR

XXXXX....YYYY
 RR

XXXXX....YYYY
RRRRR

(Note that we also assume that the rmapbt never has two mergeable
records.  rm_blockcount is 32-bit which is large enough to cover an
entire AG if need be.)

On the regular logging path, we have two choices: (1) pass the owner to
xfs_free_extent and let the EFI-finish remove the rmap, or (2) queue our
own deferred rmap unmap and pass OWN_NULL to bmap_add_free in which case
the EFI-finish will not try to remove an rmap.

If we have to log-recover (1), then neither the rmap nor the free space
btrees got updated, so there should be a single rmap covering the whole
EFI extent.  The EFI recovery will do an OWN_UNKNOWN rmap free, which
will find that full rmap and remove as necessary.

If we have to log-recover (2) then (assuming we queued the RUI before
the EFI like we're supposed to) the RUI recovery will remove the
rmapping and the EFI recovery's OWN_UNKNOWN rmap free will find nothing.

So far so good?  Let's see what happens if our assumption is false:

XXXXX....YYYY
  RRRRR

XXXXX....YYYY
      RRRRR

XXXXX....YYYY
   RRRRRRRR

Then we started with a filesystem that only had a partial rmapping for a
mapped region.  For a known-owner removal we'll only ever see rmap X and
trigger this corruption report.  This holds true even for an
unknown-owner removal, so I think you're definitely onto something here.
The rework of this hunk was incorrect; I should have left it alone.

XXXXX....YYYY
      RR

So this is this case where EFI recovery needs to delete an rmap and
there's already nothing there, ideally because a previous RUI already
did the work for us.

I /think/ the proper fix is that if ltrec precedes the range being
removed then we increment to check the next extent, and if there's no
overlap with the right extent (or no right extent), then the rmappings
were already cleared out and therefore we can just exit:

/*
 * If we're doing an unknown-owner removal and there are no rmaps
 * overlapping the other end of range, we're done.
 */
if (owner == UNKNOWN && ltrec.rm_startblock + ltrec.rm_blockcount <= bno) {
	struct xfs_rmap_irec	rtrec;

	xfs_btree_increment(rmap_cur, &has);
	if (!has)
		return 0;
	xfs_btree_getrec(rmap_cur, &rtrec);
	if (rtrec.rm_startblock > bno + len)
		return 0;
}

/* Make sure the extent we found covers the entire freeing range. */
XFS_WANT_CORRUPTED_GOTO(mp, ltrec.rm_startblock <= bno &&
	ltrec.rm_startblock + ltrec.rm_blockcount >=
	bno + len, out_error);

Good catch!  This whole thing has turned my brain into a lump of
spaghetti.  Well that and the three simultaneous internal bug hunts...

--D

> I could easily be missing something here, but otherwise I wonder if this
> would be better as separate checks so we don't lose some of the error
> checking coverage.
> 
> Brian
> 
> > > > +	if (ltrec.rm_startblock > bno ||
> > > > +	    ltrec.rm_startblock + ltrec.rm_blockcount < bno + len) {
> > > > +		if (owner == XFS_RMAP_OWN_UNKNOWN)
> > > > +			goto out_done;
> > > > +		XFS_WANT_CORRUPTED_GOTO(mp, false, out_error);
> > > > +	}
> > > >  
> > > 
> > > Also... unrelated, but is this check immediately below really intending
> > > to ignore owner inconsistencies for all !inode owners?
> > 
> > I had my eye on that one too, though I think that could be a
> > freestanding cleanup.
> > 
> > > >  	/* Make sure the owner matches what we expect to find in the tree. */
> > > >  	XFS_WANT_CORRUPTED_GOTO(mp, owner == ltrec.rm_owner ||
> > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > > index 8f22fc5..60a2e12 100644
> > > > --- a/fs/xfs/xfs_fsops.c
> > > > +++ b/fs/xfs/xfs_fsops.c
> > > > @@ -571,6 +571,11 @@ xfs_growfs_data_private(
> > > >  		 * this doesn't actually exist in the rmap btree.
> > > >  		 */
> > > >  		xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_NULL);
> > > > +		error = xfs_rmap_free(tp, bp, agno,
> > > > +				be32_to_cpu(agf->agf_length) - new,
> > > > +				new, &oinfo);
> > > > +		if (error)
> > > > +			goto error0;
> > > 
> > > OWN_NULL makes sense from the perspective of needing to avoid some error
> > > down in the free code where we need to free some space without needing
> > > to remove an owner, but what is the purpose of the above? It doesn't
> > > look like this really does anything beyond checking that the associated
> > > space is beyond the end of the rmapbt. If that's the intent, then it
> > > probably makes sense to update this comment as well.
> > 
> > Yes, that's exactly the intent.
> > 
> > Hmm, come to think of it, the rmap xref patch adds a
> > xfs_rmap_has_record helper that does exactly what we want here (decides
> > if there are any records covering this range).
> > 
> > --D
> > 
> > > Brian
> > > 
> > > >  		error = xfs_free_extent(tp,
> > > >  				XFS_AGB_TO_FSB(mp, agno,
> > > >  					be32_to_cpu(agf->agf_length) - new),
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] xfs: always honor OWN_UNKNOWN rmap removal requests

[Brian Foster]

On Wed, Dec 06, 2017 at 02:06:05PM -0800, Darrick J. Wong wrote:
> On Wed, Dec 06, 2017 at 03:49:46PM -0500, Brian Foster wrote:
> > On Wed, Dec 06, 2017 at 09:53:00AM -0800, Darrick J. Wong wrote:
> > > On Wed, Dec 06, 2017 at 09:14:07AM -0500, Brian Foster wrote:
> > > > On Tue, Dec 05, 2017 at 03:34:20PM -0800, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > 
> > > > > Calling xfs_rmap_free with an unknown owner is supposed to remove any
> > > > > rmaps covering that range regardless of owner.  This is used by the EFI
> > > > > recovery code to say "we're freeing this, it mustn't be owned by
> > > > > anything anymore", but for whatever reason xfs_free_ag_extent filters
> > > > > them out.
> > > > > 
> > > > > Therefore, remove the filter and make xfs_rmap_unmap actually treat it
> > > > > as a wildcard owner -- free anything that's already there, and if
> > > > > there's no owner at all then that's fine too.
> > > > > 
> > > > > There are two existing callers of bmap_add_free that take care the rmap
> > > > > deferred ops themselves and use OWN_UNKNOWN to skip the EFI-based rmap
> > > > > cleanup; convert these to use OWN_NULL, and ensure that the RUI gets
> > > > > added to the defer ops ahead of any EFI.
> > > > > 
> > > > > Lastly, now that xfs_free_extent filters out OWN_NULL rmap free requests,
> > > > > growfs will have to consult directly with the rmap to ensure that there
> > > > > aren't any rmaps in the grown region.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > 
> > ...
> > > > > diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> > > > > index 5f3a3d9..fd0e630 100644
> > > > > --- a/fs/xfs/libxfs/xfs_rmap.c
> > > > > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > > > > @@ -484,10 +484,17 @@ xfs_rmap_unmap(
> > > > >  	XFS_WANT_CORRUPTED_GOTO(mp, (flags & XFS_RMAP_UNWRITTEN) ==
> > > > >  			(ltrec.rm_flags & XFS_RMAP_UNWRITTEN), out_error);
> > > > >  
> > > > > -	/* Make sure the extent we found covers the entire freeing range. */
> > > > > -	XFS_WANT_CORRUPTED_GOTO(mp, ltrec.rm_startblock <= bno &&
> > > > > -		ltrec.rm_startblock + ltrec.rm_blockcount >=
> > > > > -		bno + len, out_error);
> > > > > +	/*
> > > > > +	 * Make sure the extent we found covers the entire freeing range.
> > > > > +	 * If this is a wildcard free, we're already done, otherwise there's
> > > > > +	 * something wrong with the rmapbt.
> > > > > +	 */
> > > > 
> > > > What does this mean by "we're already done?" This logic appears to mean
> > > > that we don't do anything (as opposed to throwing an error). I think the
> > > > comment would be more clear if it pointed out that/why we have nothing
> > > > to do here (due to OWN_UNKNOWN). I.e., caller passed in a wildcard and
> > > > we essentially didn't find a match..?
> > > 
> > > "Make sure the extent we found covers the entire freeing range.  Passing
> > > in an owner of OWN_UNKNOWN means that the caller wants to remove any
> > > reverse mapping that may exist for this range of blocks regardless of
> > > owner; if there are no mappings at all, we're done."
> > > 
> > 
> > Looking at this again, I find it a bit confusing how this check seems to
> > double as a "nothing to do" in the unknown case and a corruption error
> > otherwise. For example, is something still technically wrong if we get
> > an UNKNOWN unmap request (aka an extent free) to unmap a range that
> > overlaps with but starts before or extends past the range in the rmapbt?
> 
> I .... hmm, bear with my stream of consciousness:
> 
> Start with the assumption that for each rmap free request there is
> exactly one rmap that overlaps with the request (X and Y are existing
> rmaps, R is the range we want to remove):
> 
> XXXXX....YYYY
>    RR
> 
> XXXXX....YYYY
>          RR
> 
> XXXXX....YYYY
>  RR
> 
> XXXXX....YYYY
> RRRRR
> 
> (Note that we also assume that the rmapbt never has two mergeable
> records.  rm_blockcount is 32-bit which is large enough to cover an
> entire AG if need be.)
> 
> On the regular logging path, we have two choices: (1) pass the owner to
> xfs_free_extent and let the EFI-finish remove the rmap, or (2) queue our
> own deferred rmap unmap and pass OWN_NULL to bmap_add_free in which case
> the EFI-finish will not try to remove an rmap.
> 
> If we have to log-recover (1), then neither the rmap nor the free space
> btrees got updated, so there should be a single rmap covering the whole
> EFI extent.  The EFI recovery will do an OWN_UNKNOWN rmap free, which
> will find that full rmap and remove as necessary.
> 
> If we have to log-recover (2) then (assuming we queued the RUI before
> the EFI like we're supposed to) the RUI recovery will remove the
> rmapping and the EFI recovery's OWN_UNKNOWN rmap free will find nothing.
> 
> So far so good?  Let's see what happens if our assumption is false:
> 
> XXXXX....YYYY
>   RRRRR
> 
> XXXXX....YYYY
>       RRRRR
> 
> XXXXX....YYYY
>    RRRRRRRR
> 
> Then we started with a filesystem that only had a partial rmapping for a
> mapped region.  For a known-owner removal we'll only ever see rmap X and
> trigger this corruption report.  This holds true even for an
> unknown-owner removal, so I think you're definitely onto something here.
> The rework of this hunk was incorrect; I should have left it alone.
> 
> XXXXX....YYYY
>       RR
> 
> So this is this case where EFI recovery needs to delete an rmap and
> there's already nothing there, ideally because a previous RUI already
> did the work for us.
> 
> I /think/ the proper fix is that if ltrec precedes the range being
> removed then we increment to check the next extent, and if there's no
> overlap with the right extent (or no right extent), then the rmappings
> were already cleared out and therefore we can just exit:
> 

ACK, that is much more clear. A nit..

> /*
>  * If we're doing an unknown-owner removal and there are no rmaps
>  * overlapping the other end of range, we're done.
>  */

To me, this implies that the overlap case is a valid one. I'd prefer
something a bit more explicit, e.g.:

"On unknown-owner removal, we expect to find the full range in the
rmapbt or not at all. If no rmaps overlap the range, we're done.
Otherwise, all sanity checks apply except for the rmap owner."

Thanks for the breakdown!

Brian

> if (owner == UNKNOWN && ltrec.rm_startblock + ltrec.rm_blockcount <= bno) {
> 	struct xfs_rmap_irec	rtrec;
> 
> 	xfs_btree_increment(rmap_cur, &has);
> 	if (!has)
> 		return 0;
> 	xfs_btree_getrec(rmap_cur, &rtrec);
> 	if (rtrec.rm_startblock > bno + len)
> 		return 0;
> }
> 
> /* Make sure the extent we found covers the entire freeing range. */
> XFS_WANT_CORRUPTED_GOTO(mp, ltrec.rm_startblock <= bno &&
> 	ltrec.rm_startblock + ltrec.rm_blockcount >=
> 	bno + len, out_error);
> 
> Good catch!  This whole thing has turned my brain into a lump of
> spaghetti.  Well that and the three simultaneous internal bug hunts...
> 
> --D
> 
> > I could easily be missing something here, but otherwise I wonder if this
> > would be better as separate checks so we don't lose some of the error
> > checking coverage.
> > 
> > Brian
> > 
> > > > > +	if (ltrec.rm_startblock > bno ||
> > > > > +	    ltrec.rm_startblock + ltrec.rm_blockcount < bno + len) {
> > > > > +		if (owner == XFS_RMAP_OWN_UNKNOWN)
> > > > > +			goto out_done;
> > > > > +		XFS_WANT_CORRUPTED_GOTO(mp, false, out_error);
> > > > > +	}
> > > > >  
> > > > 
> > > > Also... unrelated, but is this check immediately below really intending
> > > > to ignore owner inconsistencies for all !inode owners?
> > > 
> > > I had my eye on that one too, though I think that could be a
> > > freestanding cleanup.
> > > 
> > > > >  	/* Make sure the owner matches what we expect to find in the tree. */
> > > > >  	XFS_WANT_CORRUPTED_GOTO(mp, owner == ltrec.rm_owner ||
> > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > > > index 8f22fc5..60a2e12 100644
> > > > > --- a/fs/xfs/xfs_fsops.c
> > > > > +++ b/fs/xfs/xfs_fsops.c
> > > > > @@ -571,6 +571,11 @@ xfs_growfs_data_private(
> > > > >  		 * this doesn't actually exist in the rmap btree.
> > > > >  		 */
> > > > >  		xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_NULL);
> > > > > +		error = xfs_rmap_free(tp, bp, agno,
> > > > > +				be32_to_cpu(agf->agf_length) - new,
> > > > > +				new, &oinfo);
> > > > > +		if (error)
> > > > > +			goto error0;
> > > > 
> > > > OWN_NULL makes sense from the perspective of needing to avoid some error
> > > > down in the free code where we need to free some space without needing
> > > > to remove an owner, but what is the purpose of the above? It doesn't
> > > > look like this really does anything beyond checking that the associated
> > > > space is beyond the end of the rmapbt. If that's the intent, then it
> > > > probably makes sense to update this comment as well.
> > > 
> > > Yes, that's exactly the intent.
> > > 
> > > Hmm, come to think of it, the rmap xref patch adds a
> > > xfs_rmap_has_record helper that does exactly what we want here (decides
> > > if there are any records covering this range).
> > > 
> > > --D
> > > 
> > > > Brian
> > > > 
> > > > >  		error = xfs_free_extent(tp,
> > > > >  				XFS_AGB_TO_FSB(mp, agno,
> > > > >  					be32_to_cpu(agf->agf_length) - new),
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html