[PATCH 1/2] xfs_fsr: extra debugging info

[PATCH 1/2] xfs_fsr: extra debugging info

[Eric Sandeen]

Provide 2 new pieces of information when -d is specified to
xfs_fsr:

* If we needed to grow the forkoffset and couldn't
* If we were unable to match the fork offset

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

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 2db2224..d83bdc9 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -1021,6 +1021,7 @@ fsr_setup_attr_fork(
 {
 	struct stat64	tstatbuf;
 	int		i;
+	int		diff = 0;
 	int		last_forkoff = 0;
 	int		no_change_cnt = 0;
 	int		ret;
@@ -1056,7 +1057,6 @@ fsr_setup_attr_fork(
 		xfs_bstat_t	tbstat;
 		xfs_ino_t	ino;
 		char		name[64];
-		int		diff;
 
 		/*
 		 * bulkstat the temp inode  to see what the forkoff is. Use
@@ -1122,6 +1122,8 @@ fsr_setup_attr_fork(
 			 * non-contiguous offsets.
 			 */
 			/* XXX: unimplemented! */
+			if (dflag)
+				printf(_("data fork growth unimplemented\n"));
 			goto out;
 		}
 
@@ -1137,6 +1139,10 @@ fsr_setup_attr_fork(
 out:
 	if (dflag)
 		fsrprintf(_("set temp attr\n"));
+	/* We failed to resolve the fork difference */
+	if (dflag && diff)
+		fsrprintf(_("failed to match fork offset\n"));;
+
 	return 0;
 }
 

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

[PATCH 2/2] xfs_fsr: create extent-based attr to grow forkoff

[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 <gabriel@vlasiu.net>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index d83bdc9..e8ab028 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_fsr: create extent-based attr to grow forkoff

[Eric Sandeen]

On 3/30/12 10:01 AM, Eric Sandeen wrote:
> 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

sorry that should read "non-local attribute."

> 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 <gabriel@vlasiu.net>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index d83bdc9..e8ab028 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

Re: [PATCH 2/2] xfs_fsr: create extent-based attr to grow forkoff

[Eric Sandeen]

On 3/30/12 10:01 AM, Eric Sandeen wrote:
> 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.

Ping on this one?  Would be nice to push it out.

-Eric

> Reported-by: Gabriel VLASIU <gabriel@vlasiu.net>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index d83bdc9..e8ab028 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

Re: [PATCH 2/2] xfs_fsr: create extent-based attr to grow forkoff

[Eric Sandeen]

On 3/30/12 10:01 AM, Eric Sandeen wrote:
> 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 <gabriel@vlasiu.net>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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.

-Eric

> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index d83bdc9..e8ab028 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -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
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

Re: [PATCH 2/2] xfs_fsr: create extent-based attr to grow forkoff

[Dave Chinner]

On Wed, Oct 09, 2013 at 10:11:29PM -0500, Eric Sandeen wrote:
> On 3/30/12 10:01 AM, Eric Sandeen wrote:
> > 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 <gabriel@vlasiu.net>
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> 
> 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.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/2] xfs_fsr: create extent-based attr to grow forkoff

[Eric Sandeen]

On Oct 9, 2013, at 10:21 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
>> On Wed, Oct 09, 2013 at 10:11:29PM -0500, Eric Sandeen wrote:
>>> On 3/30/12 10:01 AM, Eric Sandeen wrote:
>>> 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 <gabriel@vlasiu.net>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>> 
>> 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?
> 

Ok, good idea.

Eric

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

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