xfs_fsr: extra debugging info

In some cases the target file may have a non-local attribute, but the temp file gets assigned a local attribute on creation, due to selinux, for example.

In this case, the large-ish selinux attr will create a forkoff in the temp file smaller than the forkoff in the target file, because the FMT_EXTENTS attr takes up less space. There is no mechanism to grow the forkoff to match, so we can end up failing to swap these 2 inodes if the result is not enough data space in the temp inode as a result.

After testing the target file for a non-local extent, and

checking to see if the forkoff needs to be grown on the first pass, we can add a large attr to knock all attributes of the temp file out of local format, and grow the fork offset.

This passes xfstest 227, and also resolves issues seen on a metadata image provided by Gabriel.

Reported-by Gabriel VLASIU <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fsr/xfs_fsr.c +23 -1

index d83bdc9..e8ab028 100644
@@ -1022,6 +1022,8 @@ fsr_setup_attr_fork(
 	struct stat64	tstatbuf;
 	int		i;
 	int		diff = 0;
+	struct fsxattr	fsx;
+	int		target_attr_local = 0;
 	int		last_forkoff = 0;
 	int		no_change_cnt = 0;
 	int		ret;

@@ -1052,6 +1054,11 @@ fsr_setup_attr_fork(
 		return -1;
 	}
 
+	memset(&fsx, 0, sizeof(fsx));
+	ioctl(fd, XFS_IOC_FSGETXATTR, &fsx);
+	if (fsx.fsx_nextents == 0)
+		target_attr_local = 1;
+
 	i = 0;
 	do {
 		xfs_bstat_t	tbstat;

@@ -1072,6 +1079,7 @@ fsr_setup_attr_fork(
 		if (dflag)
 			fsrprintf(_("orig forkoff %d, temp forkoff %d\n"),
 					bstatp->bs_forkoff, tbstat.bs_forkoff);
+		diff = tbstat.bs_forkoff - bstatp->bs_forkoff;
 
 		snprintf(name, sizeof(name), "user.%d", i);
 

@@ -1086,6 +1094,21 @@ fsr_setup_attr_fork(
 				return -1;
 			}
 			continue;
+		} else if (i == 0 && diff < 0 && target_attr_local == 0) {
+			/*
+			 * A small attr may exist from eg selinux but, if the
+			 * target is not local, write a big attr to tempfile
+			 * to knock it out of local format to match target.
+			 * This should actually increase the temp forkoffset.
+			 */
+			char val[2048];
+			memset(val, 'X', 2048);
+			ret = fsetxattr(tfd, name, val, 2048, XATTR_CREATE);
+			if (ret) {
+				fsrprintf(_("could not set large ATTR\n"));
+				return -1;
+			}
+			continue;
 		}
 
 		/*

@@ -1100,7 +1123,6 @@ fsr_setup_attr_fork(
 		last_forkoff = tbstat.bs_forkoff;
 
 		/* work out which way to grow the fork */
-		diff = tbstat.bs_forkoff - bstatp->bs_forkoff;
 		if (abs(diff) > fsgeom.inodesize - sizeof(struct xfs_dinode)) {
 			fsrprintf(_("forkoff diff %d too large!\n"), diff);
 			return -1;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

In some cases the target file may have a non-local attribute, but the temp file gets assigned a local attribute on creation, due to selinux, for example.

In this case, the large-ish selinux attr will create a forkoff in the temp file smaller than the forkoff in the target file, because the FMT_EXTENTS attr takes up less space. There is no mechanism to grow the forkoff to match, so we can end up failing to swap these 2 inodes if the result is not enough data space in the temp inode as a result.

After testing the target file for a non-local extent, and checking to see if the forkoff needs to be grown on the first pass, we can add a large attr to knock all attributes of the temp file out of local format, and grow the fork offset.

This passes xfstest 227, and also resolves issues seen on a metadata image provided by Gabriel.

Reported-by Gabriel VLASIU <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fsr/xfs_fsr.c +23 -1

index d83bdc9..e8ab028 100644
@@ -1022,6 +1022,8 @@ fsr_setup_attr_fork(
 	struct stat64	tstatbuf;
 	int		i;
 	int		diff = 0;
+	struct fsxattr	fsx;
+	int		target_attr_local = 0;
 	int		last_forkoff = 0;
 	int		no_change_cnt = 0;
 	int		ret;

@@ -1052,6 +1054,11 @@ fsr_setup_attr_fork(
 		return -1;
 	}
 
+	memset(&fsx, 0, sizeof(fsx));
+	ioctl(fd, XFS_IOC_FSGETXATTR, &fsx);
+	if (fsx.fsx_nextents == 0)
+		target_attr_local = 1;
+
 	i = 0;
 	do {
 		xfs_bstat_t	tbstat;

@@ -1072,6 +1079,7 @@ fsr_setup_attr_fork(
 		if (dflag)
 			fsrprintf(_("orig forkoff %d, temp forkoff %d\n"),
 					bstatp->bs_forkoff, tbstat.bs_forkoff);
+		diff = tbstat.bs_forkoff - bstatp->bs_forkoff;
 
 		snprintf(name, sizeof(name), "user.%d", i);
 

@@ -1086,6 +1094,21 @@ fsr_setup_attr_fork(
 				return -1;
 			}
 			continue;
+		} else if (i == 0 && diff < 0 && target_attr_local == 0) {
+			/*
+			 * A small attr may exist from eg selinux but, if the
+			 * target is not local, write a big attr to tempfile
+			 * to knock it out of local format to match target.
+			 * This should actually increase the temp forkoffset.
+			 */
+			char val[2048];
+			memset(val, 'X', 2048);
+			ret = fsetxattr(tfd, name, val, 2048, XATTR_CREATE);
+			if (ret) {
+				fsrprintf(_("could not set large ATTR\n"));
+				return -1;
+			}
+			continue;
 		}
 
 		/*

@@ -1100,7 +1123,6 @@ fsr_setup_attr_fork(
 		last_forkoff = tbstat.bs_forkoff;
 
 		/* work out which way to grow the fork */
-		diff = tbstat.bs_forkoff - bstatp->bs_forkoff;
 		if (abs(diff) > fsgeom.inodesize - sizeof(struct xfs_dinode)) {
 			fsrprintf(_("forkoff diff %d too large!\n"), diff);
 			return -1;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

In some cases the target file may have a non-local attribute, but the temp file gets assigned a local attribute on creation, due to selinux, for example.

In this case, the large-ish selinux attr will create a forkoff in the temp file smaller than the forkoff in the target file, because the FMT_EXTENTS attr takes up less space. There is no mechanism to grow the forkoff to match, so we can end up failing to swap these 2 inodes if the result is not enough data space in the temp inode as a result.

After testing the target file for a non-local extent, and checking to see if the forkoff needs to be grown on the first pass, we can add a large attr to knock all attributes of the temp file out of local format, and grow the fork offset.

This passes xfstest 227, and also resolves issues seen on a metadata image provided by Gabriel.

Reported-by Gabriel VLASIU <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fsr/xfs_fsr.c +23 -1

index d83bdc9..e8ab028 100644
@@ -1022,6 +1022,8 @@ fsr_setup_attr_fork(
 	struct stat64	tstatbuf;
 	int		i;
 	int		diff = 0;
+	struct fsxattr	fsx;
+	int		target_attr_local = 0;
 	int		last_forkoff = 0;
 	int		no_change_cnt = 0;
 	int		ret;

@@ -1052,6 +1054,11 @@ fsr_setup_attr_fork(
 		return -1;
 	}
 
+	memset(&fsx, 0, sizeof(fsx));
+	ioctl(fd, XFS_IOC_FSGETXATTR, &fsx);
+	if (fsx.fsx_nextents == 0)
+		target_attr_local = 1;
+
 	i = 0;
 	do {
 		xfs_bstat_t	tbstat;

@@ -1072,6 +1079,7 @@ fsr_setup_attr_fork(
 		if (dflag)
 			fsrprintf(_("orig forkoff %d, temp forkoff %d\n"),
 					bstatp->bs_forkoff, tbstat.bs_forkoff);
+		diff = tbstat.bs_forkoff - bstatp->bs_forkoff;
 
 		snprintf(name, sizeof(name), "user.%d", i);
 

@@ -1086,6 +1094,21 @@ fsr_setup_attr_fork(
 				return -1;
 			}
 			continue;
+		} else if (i == 0 && diff < 0 && target_attr_local == 0) {
+			/*
+			 * A small attr may exist from eg selinux but, if the
+			 * target is not local, write a big attr to tempfile
+			 * to knock it out of local format to match target.
+			 * This should actually increase the temp forkoffset.
+			 */
+			char val[2048];
+			memset(val, 'X', 2048);
+			ret = fsetxattr(tfd, name, val, 2048, XATTR_CREATE);
+			if (ret) {
+				fsrprintf(_("could not set large ATTR\n"));
+				return -1;
+			}
+			continue;
 		}
 
 		/*

@@ -1100,7 +1123,6 @@ fsr_setup_attr_fork(
 		last_forkoff = tbstat.bs_forkoff;
 
 		/* work out which way to grow the fork */
-		diff = tbstat.bs_forkoff - bstatp->bs_forkoff;
 		if (abs(diff) > fsgeom.inodesize - sizeof(struct xfs_dinode)) {
 			fsrprintf(_("forkoff diff %d too large!\n"), diff);
 			return -1;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 19 lines · Eric Sandeen

In some cases the target file may have a non-local attribute, but the temp file gets assigned a local attribute on creation, due to selinux, for example.

In this case, the large-ish selinux attr will create a forkoff in the temp file smaller than the forkoff in the target file, because the FMT_EXTENTS attr takes up less space. There is no mechanism to grow the forkoff to match, so we can end up failing to swap these 2 inodes if the result is not enough data space in the temp inode as a result.

After testing the target file for a non-local extent, and checking to see if the forkoff needs to be grown on the first pass, we can add a large attr to knock all attributes of the temp file out of local format, and grow the fork offset.

This passes xfstest 227, and also resolves issues seen on a metadata image provided by Gabriel.

Quoted 3 lines

Reported-by Gabriel VLASIU <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

Ping #2? This was a real bug once, IIRC. Probably still is...

Patch 1/2 helped identify the problem, so ping on that too I guess.

Quoted 26 lines · Eric Sandeen

Quoted 23 lines · Eric Sandeen

In some cases the target file may have a non-local attribute, but the temp file gets assigned a local attribute on creation, due to selinux, for example.

In this case, the large-ish selinux attr will create a forkoff in the temp file smaller than the forkoff in the target file, because the FMT_EXTENTS attr takes up less space. There is no mechanism to grow the forkoff to match, so we can end up failing to swap these 2 inodes if the result is not enough data space in the temp inode as a result.

After testing the target file for a non-local extent, and checking to see if the forkoff needs to be grown on the first pass, we can add a large attr to knock all attributes of the temp file out of local format, and grow the fork offset.

This passes xfstest 227, and also resolves issues seen on a metadata image provided by Gabriel.

Reported-by Gabriel VLASIU <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

Quoted 3 lines

Ping #2? This was a real bug once, IIRC. Probably still is...

Patch 1/2 helped identify the problem, so ping on that too I guess.

Can you repost all these old patches as a new series to make it easy to apply and test them?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]