xfs_copy: Fix meta UUID handling on multiple copies
diff mbox

Message ID 510aeea2-6f26-aeee-d899-57e55049958d@redhat.com
State Accepted
Headers show

Commit Message

Eric Sandeen Sept. 20, 2016, 3:50 p.m. UTC
Zorro reported that when making multiple copies of a V5
filesystem with xfs_copy while generating new UUIDs, all
but the first copy were corrupt.

Upon inspection, the corruption was related to incorrect UUIDs;
the original UUID, as stamped into every metadata structure,
was not preserved in the sb_meta_uuid field of the superblock
on any but the first copy.

This happened because sb_update_uuid was using the UUID present in
the ag_hdr structure as the unchanging meta-uuid which is to match
existing structures, but it also /updates/ that UUID with the
new identifying UUID present in tcarg.  So the newly-generated
UUIDs moved transitively from tcarg->uuid to ag_hdr->xfs_sb->sb_uuid
to ag_hdr->xfs_sb->sb_meta_uuid each time the function got called.

Fix this by looking instead to the unchanging, original UUID
present in the xfs_sb_t we are given, which reflects the original
filesystem's metadata UUID, and copy /that/ UUID into each target
filesystem's meta_uuid field.

Most of this patch is changing comments and re-ordering tests
to match; the functional change is to simply use the *sb rather
than the *ag_hdr to identify the proper metadata UUID.

Reported-by: Zorro Lang <zlang@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Comments

Zorro Lang Sept. 20, 2016, 4:28 p.m. UTC | #1
On Tue, Sep 20, 2016 at 10:50:39AM -0500, Eric Sandeen wrote:
> Zorro reported that when making multiple copies of a V5
> filesystem with xfs_copy while generating new UUIDs, all
> but the first copy were corrupt.
> 
> Upon inspection, the corruption was related to incorrect UUIDs;
> the original UUID, as stamped into every metadata structure,
> was not preserved in the sb_meta_uuid field of the superblock
> on any but the first copy.
> 
> This happened because sb_update_uuid was using the UUID present in
> the ag_hdr structure as the unchanging meta-uuid which is to match
> existing structures, but it also /updates/ that UUID with the
> new identifying UUID present in tcarg.  So the newly-generated
> UUIDs moved transitively from tcarg->uuid to ag_hdr->xfs_sb->sb_uuid
> to ag_hdr->xfs_sb->sb_meta_uuid each time the function got called.
> 
> Fix this by looking instead to the unchanging, original UUID
> present in the xfs_sb_t we are given, which reflects the original
> filesystem's metadata UUID, and copy /that/ UUID into each target
> filesystem's meta_uuid field.
> 
> Most of this patch is changing comments and re-ordering tests
> to match; the functional change is to simply use the *sb rather
> than the *ag_hdr to identify the proper metadata UUID.
> 
> Reported-by: Zorro Lang <zlang@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

I can't reproduce that bug again after merge this patch. V4 and V5
XFS all test passed, different block size test passed too.

I didn't do regression test for this patch.

Thanks,
Zorro

> 
> 
> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> index 3c8998c..d8edfdd 100644
> --- a/copy/xfs_copy.c
> +++ b/copy/xfs_copy.c
> @@ -494,27 +494,29 @@ write_wbuf(void)
>  
>  void
>  sb_update_uuid(
> -	xfs_sb_t	*sb,
> -	ag_header_t	*ag_hdr,
> -	thread_args	*tcarg)
> +	xfs_sb_t	*sb,		/* Original fs superblock */
> +	ag_header_t	*ag_hdr,	/* AG hdr to update for this copy */
> +	thread_args	*tcarg)		/* Args for this thread, with UUID */
>  {
>  	/*
>  	 * If this filesystem has CRCs, the original UUID is stamped into
> -	 * all metadata.  If we are changing the UUID in the copy, we need
> -	 * to copy the original UUID into the meta_uuid slot and set the
> -	 * set the incompat flag if that hasn't already been done.
> +	 * all metadata.  If we don't have an existing meta_uuid field in the
> +	 * the original filesystem and we are changing the UUID in this copy,
> +	 * we must copy the original sb_uuid to the sb_meta_uuid slot and set
> +	 * the incompat flag for the feature on this copy.
>  	 */
> -	if (!uuid_equal(&tcarg->uuid, &ag_hdr->xfs_sb->sb_uuid) &&
> -	    xfs_sb_version_hascrc(sb) && !xfs_sb_version_hasmetauuid(sb)) {
> +	if (xfs_sb_version_hascrc(sb) && !xfs_sb_version_hasmetauuid(sb) &&
> +	    !uuid_equal(&tcarg->uuid, &sb->sb_uuid)) {
>  		__be32 feat;
>  
>  		feat = be32_to_cpu(ag_hdr->xfs_sb->sb_features_incompat);
>  		feat |= XFS_SB_FEAT_INCOMPAT_META_UUID;
>  		ag_hdr->xfs_sb->sb_features_incompat = cpu_to_be32(feat);
>  		platform_uuid_copy(&ag_hdr->xfs_sb->sb_meta_uuid,
> -				   &ag_hdr->xfs_sb->sb_uuid);
> +				   &sb->sb_uuid);
>  	}
>  
> +	/* Copy the (possibly new) fs-identifier UUID into sb_uuid */
>  	platform_uuid_copy(&ag_hdr->xfs_sb->sb_uuid, &tcarg->uuid);
>  
>  	/* We may have changed the UUID, so update the superblock CRC */
> 
> 
> --
> 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
Christoph Hellwig Sept. 25, 2016, 2:33 p.m. UTC | #2
Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Patch
diff mbox

diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index 3c8998c..d8edfdd 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -494,27 +494,29 @@  write_wbuf(void)
 
 void
 sb_update_uuid(
-	xfs_sb_t	*sb,
-	ag_header_t	*ag_hdr,
-	thread_args	*tcarg)
+	xfs_sb_t	*sb,		/* Original fs superblock */
+	ag_header_t	*ag_hdr,	/* AG hdr to update for this copy */
+	thread_args	*tcarg)		/* Args for this thread, with UUID */
 {
 	/*
 	 * If this filesystem has CRCs, the original UUID is stamped into
-	 * all metadata.  If we are changing the UUID in the copy, we need
-	 * to copy the original UUID into the meta_uuid slot and set the
-	 * set the incompat flag if that hasn't already been done.
+	 * all metadata.  If we don't have an existing meta_uuid field in the
+	 * the original filesystem and we are changing the UUID in this copy,
+	 * we must copy the original sb_uuid to the sb_meta_uuid slot and set
+	 * the incompat flag for the feature on this copy.
 	 */
-	if (!uuid_equal(&tcarg->uuid, &ag_hdr->xfs_sb->sb_uuid) &&
-	    xfs_sb_version_hascrc(sb) && !xfs_sb_version_hasmetauuid(sb)) {
+	if (xfs_sb_version_hascrc(sb) && !xfs_sb_version_hasmetauuid(sb) &&
+	    !uuid_equal(&tcarg->uuid, &sb->sb_uuid)) {
 		__be32 feat;
 
 		feat = be32_to_cpu(ag_hdr->xfs_sb->sb_features_incompat);
 		feat |= XFS_SB_FEAT_INCOMPAT_META_UUID;
 		ag_hdr->xfs_sb->sb_features_incompat = cpu_to_be32(feat);
 		platform_uuid_copy(&ag_hdr->xfs_sb->sb_meta_uuid,
-				   &ag_hdr->xfs_sb->sb_uuid);
+				   &sb->sb_uuid);
 	}
 
+	/* Copy the (possibly new) fs-identifier UUID into sb_uuid */
 	platform_uuid_copy(&ag_hdr->xfs_sb->sb_uuid, &tcarg->uuid);
 
 	/* We may have changed the UUID, so update the superblock CRC */