[PATCH 0/4] xfs: sb_meta_uuid fixes

[PATCH 0/4] xfs: sb_meta_uuid fixes

[Dave Chinner]

Hi folks,

These are fixes to problems I found int eh sb_meta_uuid changes I
recently pushed to for-next. They were all found by augmenting
_scratch_mkfs_xfs in xfstests with an extra command to change the
UUID after the filesystem had been created. I think we should
probably do that randomly in xfstests to get coverage of both
cases. No-one shoul dhave been exposed to these yet, because I
haven't released a userspace that can change the UUID yet on v5
filesystems.

Cheers,

Dave.

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

[PATCH 1/4] xfs: fix sb_meta_uuid usage

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

After changing the UUID on a v5 filesystem, xfstests fails
immediately on a debug kernel with:

XFS: Assertion failed: uuid_equal(&ip->i_d.di_uuid, &mp->m_sb.sb_uuid), file: fs/xfs/xfs_inode.c, line: 799

This needs to check against the sb_meta_uuid, not the user visible
UUID that was changed.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index fb52ff0..4156e37 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -796,7 +796,7 @@ xfs_ialloc(
 
 	if (ip->i_d.di_version == 3) {
 		ASSERT(ip->i_d.di_ino == ino);
-		ASSERT(uuid_equal(&ip->i_d.di_uuid, &mp->m_sb.sb_uuid));
+		ASSERT(uuid_equal(&ip->i_d.di_uuid, &mp->m_sb.sb_meta_uuid));
 		ip->i_d.di_crc = 0;
 		ip->i_d.di_changecount = 1;
 		ip->i_d.di_lsn = 0;
-- 
2.1.4

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

[PATCH 2/4] xfs: growfs not aware of sb_meta_uuid

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Adding this simple change to xfstests:common/rc::_scratch_mkfs_xfs:

+       if [ $mkfs_status -eq 0 ]; then
+               xfs_admin -U generate $SCRATCH_DEV > /dev/null
+       fi

triggers all sorts of errors in xfstests. xfs/104 is an example,
where growfs fails with a UUID mismatch corruption detected by
xfs_agf_write_verify() when trying to write the first new AG
headers.

Fix this problem by making sure we copy the sb_meta_uuid into new
metadata written by growfs.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_fsops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 9b3438a..ee3aaa0a 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -250,7 +250,7 @@ xfs_growfs_data_private(
 		agf->agf_freeblks = cpu_to_be32(tmpsize);
 		agf->agf_longest = cpu_to_be32(tmpsize);
 		if (xfs_sb_version_hascrc(&mp->m_sb))
-			uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_uuid);
+			uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid);
 
 		error = xfs_bwrite(bp);
 		xfs_buf_relse(bp);
@@ -273,7 +273,7 @@ xfs_growfs_data_private(
 		if (xfs_sb_version_hascrc(&mp->m_sb)) {
 			agfl->agfl_magicnum = cpu_to_be32(XFS_AGFL_MAGIC);
 			agfl->agfl_seqno = cpu_to_be32(agno);
-			uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_uuid);
+			uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid);
 		}
 
 		agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, bp);
@@ -309,7 +309,7 @@ xfs_growfs_data_private(
 		agi->agi_newino = cpu_to_be32(NULLAGINO);
 		agi->agi_dirino = cpu_to_be32(NULLAGINO);
 		if (xfs_sb_version_hascrc(&mp->m_sb))
-			uuid_copy(&agi->agi_uuid, &mp->m_sb.sb_uuid);
+			uuid_copy(&agi->agi_uuid, &mp->m_sb.sb_meta_uuid);
 		if (xfs_sb_version_hasfinobt(&mp->m_sb)) {
 			agi->agi_free_root = cpu_to_be32(XFS_FIBT_BLOCK(mp));
 			agi->agi_free_level = cpu_to_be32(1);
-- 
2.1.4

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

[PATCH 3/4] xfs: log recovery needs to validate against sb_meta_uuid

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Now that sb_uuid can be changed by the user, we cannot use this to
validate the metadata blocks being recovered belong to this
filesystem. We must check against the sb_meta_uuid as that will
remain unchanged.

There is a complication in this code - the superblock itself. We can
not check the sb_meta_uuid unconditionally, as that may not be set
on disk. Hence we must verify the superblock sb_uuid matches between
the log record and the in-core superblock.

Found by inspection after the previous two problems were found.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index c674b40..6f83d12 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1895,15 +1895,25 @@ xlog_recover_get_buf_lsn(
 		 */
 		goto recover_immediately;
 	case XFS_SB_MAGIC:
+		/*
+		 * superblock uuids are magic. We may or may not have a
+		 * sb_meta_uuid on disk, but it will be set in the in-core
+		 * superblock. We set the uuid pointer for verification
+		 * according to the superblock feature mask to ensure we check
+		 * the relevant UUID in the superblock.
+		 */
 		lsn = be64_to_cpu(((struct xfs_dsb *)blk)->sb_lsn);
-		uuid = &((struct xfs_dsb *)blk)->sb_uuid;
+		if (xfs_sb_version_hasmetauuid(&mp->m_sb))
+			uuid = &((struct xfs_dsb *)blk)->sb_meta_uuid;
+		else
+			uuid = &((struct xfs_dsb *)blk)->sb_uuid;
 		break;
 	default:
 		break;
 	}
 
 	if (lsn != (xfs_lsn_t)-1) {
-		if (!uuid_equal(&mp->m_sb.sb_uuid, uuid))
+		if (!uuid_equal(&mp->m_sb.sb_meta_uuid, uuid))
 			goto recover_immediately;
 		return lsn;
 	}
-- 
2.1.4

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

[PATCH 4/4] xfs: dquots should be stamped with sb_meta_uuid

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Once the sb_uuid is changed, the wrong uuid is stamped into new
dquots on disk. Found by inspection, verified by generic/219.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_dquot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 4143dc7..b1b26b6 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -251,7 +251,7 @@ xfs_qm_init_dquot_blk(
 		d->dd_diskdq.d_id = cpu_to_be32(curid);
 		d->dd_diskdq.d_flags = type;
 		if (xfs_sb_version_hascrc(&mp->m_sb)) {
-			uuid_copy(&d->dd_uuid, &mp->m_sb.sb_uuid);
+			uuid_copy(&d->dd_uuid, &mp->m_sb.sb_meta_uuid);
 			xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
 					 XFS_DQUOT_CRC_OFF);
 		}
-- 
2.1.4

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

Re: [PATCH 1/4] xfs: fix sb_meta_uuid usage

[Eric Sandeen]

On 8/3/15 12:40 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> After changing the UUID on a v5 filesystem, xfstests fails
> immediately on a debug kernel with:
> 
> XFS: Assertion failed: uuid_equal(&ip->i_d.di_uuid, &mp->m_sb.sb_uuid), file: fs/xfs/xfs_inode.c, line: 799
> 
> This needs to check against the sb_meta_uuid, not the user visible
> UUID that was changed.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

first of a few brown paper bags for me :(  I guess I didn't test on debug after all.  :(

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index fb52ff0..4156e37 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -796,7 +796,7 @@ xfs_ialloc(
>  
>  	if (ip->i_d.di_version == 3) {
>  		ASSERT(ip->i_d.di_ino == ino);
> -		ASSERT(uuid_equal(&ip->i_d.di_uuid, &mp->m_sb.sb_uuid));
> +		ASSERT(uuid_equal(&ip->i_d.di_uuid, &mp->m_sb.sb_meta_uuid));
>  		ip->i_d.di_crc = 0;
>  		ip->i_d.di_changecount = 1;
>  		ip->i_d.di_lsn = 0;
> 

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

Re: [PATCH 2/4] xfs: growfs not aware of sb_meta_uuid

[Eric Sandeen]

On 8/3/15 12:40 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Adding this simple change to xfstests:common/rc::_scratch_mkfs_xfs:
> 
> +       if [ $mkfs_status -eq 0 ]; then
> +               xfs_admin -U generate $SCRATCH_DEV > /dev/null
> +       fi
> 
> triggers all sorts of errors in xfstests. xfs/104 is an example,
> where growfs fails with a UUID mismatch corruption detected by
> xfs_agf_write_verify() when trying to write the first new AG
> headers.
> 
> Fix this problem by making sure we copy the sb_meta_uuid into new
> metadata written by growfs.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Ok, at least this one has a plausible story; I did have this
in my tree, and must have somehow (re?)sent outdated patches. :(
Yeah, it was in all the series I sent up until the last re-send,
damn.

I'll re-check those prior patches against what we end up with,
to make sure everything is in sync.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_fsops.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 9b3438a..ee3aaa0a 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -250,7 +250,7 @@ xfs_growfs_data_private(
>  		agf->agf_freeblks = cpu_to_be32(tmpsize);
>  		agf->agf_longest = cpu_to_be32(tmpsize);
>  		if (xfs_sb_version_hascrc(&mp->m_sb))
> -			uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_uuid);
> +			uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid);
>  
>  		error = xfs_bwrite(bp);
>  		xfs_buf_relse(bp);
> @@ -273,7 +273,7 @@ xfs_growfs_data_private(
>  		if (xfs_sb_version_hascrc(&mp->m_sb)) {
>  			agfl->agfl_magicnum = cpu_to_be32(XFS_AGFL_MAGIC);
>  			agfl->agfl_seqno = cpu_to_be32(agno);
> -			uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_uuid);
> +			uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid);
>  		}
>  
>  		agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, bp);
> @@ -309,7 +309,7 @@ xfs_growfs_data_private(
>  		agi->agi_newino = cpu_to_be32(NULLAGINO);
>  		agi->agi_dirino = cpu_to_be32(NULLAGINO);
>  		if (xfs_sb_version_hascrc(&mp->m_sb))
> -			uuid_copy(&agi->agi_uuid, &mp->m_sb.sb_uuid);
> +			uuid_copy(&agi->agi_uuid, &mp->m_sb.sb_meta_uuid);
>  		if (xfs_sb_version_hasfinobt(&mp->m_sb)) {
>  			agi->agi_free_root = cpu_to_be32(XFS_FIBT_BLOCK(mp));
>  			agi->agi_free_level = cpu_to_be32(1);
> 

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

Re: [PATCH 4/4] xfs: dquots should be stamped with sb_meta_uuid

[Eric Sandeen]

On 8/3/15 12:40 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Once the sb_uuid is changed, the wrong uuid is stamped into new
> dquots on disk. Found by inspection, verified by generic/219.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_dquot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 4143dc7..b1b26b6 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -251,7 +251,7 @@ xfs_qm_init_dquot_blk(
>  		d->dd_diskdq.d_id = cpu_to_be32(curid);
>  		d->dd_diskdq.d_flags = type;
>  		if (xfs_sb_version_hascrc(&mp->m_sb)) {
> -			uuid_copy(&d->dd_uuid, &mp->m_sb.sb_uuid);
> +			uuid_copy(&d->dd_uuid, &mp->m_sb.sb_meta_uuid);
>  			xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
>  					 XFS_DQUOT_CRC_OFF);
>  		}
> 

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

Re: [PATCH 4/4] xfs: dquots should be stamped with sb_meta_uuid

[Eric Sandeen]

On 8/3/15 9:23 AM, Eric Sandeen wrote:
> On 8/3/15 12:40 AM, Dave Chinner wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> Once the sb_uuid is changed, the wrong uuid is stamped into new
>> dquots on disk. Found by inspection, verified by generic/219.
>>
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

And yeah, sigh, this was in a prior version of my patch as well.

-Eric

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

Re: [PATCH 1/4] xfs: fix sb_meta_uuid usage

[Eric Sandeen]

On 8/3/15 9:12 AM, Eric Sandeen wrote:
> On 8/3/15 12:40 AM, Dave Chinner wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> After changing the UUID on a v5 filesystem, xfstests fails
>> immediately on a debug kernel with:
>>
>> XFS: Assertion failed: uuid_equal(&ip->i_d.di_uuid, &mp->m_sb.sb_uuid), file: fs/xfs/xfs_inode.c, line: 799
>>
>> This needs to check against the sb_meta_uuid, not the user visible
>> UUID that was changed.
>>
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> first of a few brown paper bags for me :(  I guess I didn't test on debug after all.  :(

Actually I did, this was also in my prior patches.  Le sigh.

-Eric

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

Re: [PATCH 3/4] xfs: log recovery needs to validate against sb_meta_uuid

[Eric Sandeen]

On 8/3/15 12:40 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Now that sb_uuid can be changed by the user, we cannot use this to
> validate the metadata blocks being recovered belong to this
> filesystem. We must check against the sb_meta_uuid as that will
> remain unchanged.
> 
> There is a complication in this code - the superblock itself. We can
> not check the sb_meta_uuid unconditionally, as that may not be set
> on disk. Hence we must verify the superblock sb_uuid matches between
> the log record and the in-core superblock.
> 
> Found by inspection after the previous two problems were found.

So, I also had this in my older patchset, I think it's needed for
proper log recovery as well.  I'm not sure why I didn't hit the
xlog_recover_get_buf_lsn problem, though:

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 4f5784f..4da291c 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -325,7 +325,7 @@ xlog_header_check_recover(
 		XFS_ERROR_REPORT("xlog_header_check_recover(1)",
 				 XFS_ERRLEVEL_HIGH, mp);
 		return -EFSCORRUPTED;
-	} else if (unlikely(!uuid_equal(&mp->m_sb.sb_uuid, &head->h_fs_uuid))) {
+	} else if (!uuid_equal(&mp->m_sb.sb_uuid, &head->h_fs_uuid)) {
 		xfs_warn(mp,
 	"dirty log entry has mismatched uuid - can't recover");
 		xlog_header_check_dump(mp, head);
@@ -353,7 +353,7 @@ xlog_header_check_mount(
 		 * by IRIX and continue.
 		 */
 		xfs_warn(mp, "nil uuid in log - IRIX style log");
-	} else if (unlikely(!uuid_equal(&mp->m_sb.sb_uuid, &head->h_fs_uuid))) {
+	} else if (!uuid_equal(&mp->m_sb.sb_uuid, &head->h_fs_uuid)) {
 		xfs_warn(mp, "log has mismatched uuid - can't recover");
 		xlog_header_check_dump(mp, head);
 		XFS_ERROR_REPORT("xlog_header_check_mount",


as for your patch ... looks fine.  Not sure why I didn't hit that.

Reviewed-by: Eric Sandeen <sandeen@redhat.com> unless you want
to roll the above changes in as well...

-Eric

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_recover.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index c674b40..6f83d12 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1895,15 +1895,25 @@ xlog_recover_get_buf_lsn(
>  		 */
>  		goto recover_immediately;
>  	case XFS_SB_MAGIC:
> +		/*
> +		 * superblock uuids are magic. We may or may not have a
> +		 * sb_meta_uuid on disk, but it will be set in the in-core
> +		 * superblock. We set the uuid pointer for verification
> +		 * according to the superblock feature mask to ensure we check
> +		 * the relevant UUID in the superblock.
> +		 */
>  		lsn = be64_to_cpu(((struct xfs_dsb *)blk)->sb_lsn);
> -		uuid = &((struct xfs_dsb *)blk)->sb_uuid;
> +		if (xfs_sb_version_hasmetauuid(&mp->m_sb))
> +			uuid = &((struct xfs_dsb *)blk)->sb_meta_uuid;
> +		else
> +			uuid = &((struct xfs_dsb *)blk)->sb_uuid;
>  		break;
>  	default:
>  		break;
>  	}
>  
>  	if (lsn != (xfs_lsn_t)-1) {
> -		if (!uuid_equal(&mp->m_sb.sb_uuid, uuid))
> +		if (!uuid_equal(&mp->m_sb.sb_meta_uuid, uuid))
>  			goto recover_immediately;
>  		return lsn;
>  	}
> 

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

Re: [PATCH 3/4] xfs: log recovery needs to validate against sb_meta_uuid

[Eric Sandeen]

On 8/3/15 9:41 AM, Eric Sandeen wrote:
> On 8/3/15 12:40 AM, Dave Chinner wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> Now that sb_uuid can be changed by the user, we cannot use this to
>> validate the metadata blocks being recovered belong to this
>> filesystem. We must check against the sb_meta_uuid as that will
>> remain unchanged.
>>
>> There is a complication in this code - the superblock itself. We can
>> not check the sb_meta_uuid unconditionally, as that may not be set
>> on disk. Hence we must verify the superblock sb_uuid matches between
>> the log record and the in-core superblock.
>>
>> Found by inspection after the previous two problems were found.
> 
> So, I also had this in my older patchset, I think it's needed for
> proper log recovery as well.  I'm not sure why I didn't hit the
> xlog_recover_get_buf_lsn problem, though:

Oh god, all this is is a difference in unlikely's upstream.  Kill me now!

-Eric

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