xfsdump: more projid32bit fixes

PATCH In review Eric Sandeen 4 participants 14 messages Oct 12, 2012 – Nov 9, 2012

Share & tools

Eric Sandeen

author

2012-10-12 21:30 UTC #0 lore raw

cover xfsdump: more projid32bit fixes

I recently sent a patch for 32-bit project IDs for xfsdump, to properly restore the top 16 bits, which otherwise get lost. This forced a new dump format version 4 (we were currently at 3).

One thing missing is that we should not restore a dump with 32-bit project IDs onto a filesystem w/o that format; the restore will fail to restore the top 16 bits (but otherwise it returns success; attribute setting failures are not fatal (!?))

Also, 32-bit project ID is a bit uncommon; bumping the format (and making older restore incompatible) is a bit draconian.

3 patches here:

1/3: extend fs info call to get fs flags as well
2/3: default back to V3 and go to V4 only if the projid32 flag is set 3/3: fail restore if the target XFS fs doesn't have projid32 set

I have to say, I'm not super happy with this. I have nagging fear of feature-flag-itis, and I'm not sure how extensible this is as newer versions may appear. But anyway, here's a place to start.

(p.s. anybody have wkendall's new email?) ;)

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

Eric Sandeen

2012-10-12 21:33 UTC #1 lore raw

PATCH 1/3 xfsdump: extend fs_info to gather fs feature flags

extend fs_info and fs_getid to return fs flags (features) from GEOM call as well. Not used yet in this patch. Will be used in subsequent patch to detect 32-bit project ID filesystems.

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

---

common/fs.c +12 -9

index 6f4cb6c..bf2f164 100644
@@ -111,6 +111,7 @@ fs_info( char *typb,		/* out */
 	 char *mntb,		/* out */
 	 intgen_t mntbz,
 	 uuid_t *idb,		/* out */
+	 __u32 *flgb,		/* out */
 	 char *usrs )		/* in */
 {
 	struct stat64 statb;

@@ -164,10 +165,10 @@ fs_info( char *typb,		/* out */
 	ASSERT( ok != BOOL_UNKNOWN );
 
 	if ( ok == BOOL_TRUE ) {
-		intgen_t rval = fs_getid( mntb, idb );
+		intgen_t rval = fs_getinfo( mntb, idb, flgb );
 		if ( rval ) {
 			mlog( MLOG_NORMAL,
-			      _("unable to determine uuid of fs mounted at %s: "
+			      _("unable to determine info for fs mounted at %s: "
 			      "%s\n"),
 			      mntb,
 			      strerror( errno ));

@@ -176,9 +177,9 @@ fs_info( char *typb,		/* out */
 			char string_uuid[37];
 			uuid_unparse( *idb, string_uuid );
 			mlog( MLOG_DEBUG,
-			      "fs %s uuid [%s]\n",
+			      "fs %s uuid [%s] flags [0x%x]\n",
 			      mntb,
-			      string_uuid );
+			      string_uuid, *flgb );
 		}
 	}

@@ -197,23 +198,25 @@ fs_mounted( char *typs, char *chrs, char *mnts, uuid_t *idp )
 }
 
 intgen_t
-fs_getid( char *mnts, uuid_t *idb )
+fs_getinfo( char *mnts, uuid_t *idb, __u32 *flgb )
 {
 	xfs_fsop_geom_v1_t geo;
 	int fd;
 
+	uuid_clear( *idb );
+	*flgb = 0;
+
 	fd = open( mnts, O_RDONLY );
-	if ( fd < 0 ) {
-		uuid_clear( *idb );
+	if ( fd < 0 )
 		return -1;
-	}
+
 	if ( ioctl(fd, XFS_IOC_FSGEOMETRY_V1, &geo ) ) {
-		uuid_clear( *idb );
 		close(fd);
 		return -1;
 	}
 	close(fd);
 	uuid_copy( *idb, geo.uuid );
+	*flgb = geo.flags;
 
 	return 0;
 }

common/fs.h +4 -3

index 9ad1636..878385b 100644
@@ -39,6 +39,7 @@ extern bool_t fs_info( char *fstype,		/* out: fs type (fsid.h) */
 		       char *mntpt,		/* out: where fs mounted */
 		       intgen_t mntptsz,	/* in: buffer size */
 		       uuid_t *fsid,		/* out: fs uuid */
+		       __u32 *fsflags,		/* out: fs flags */
 		       char *srcname );		/* in: how user named the fs */
 
 /* fs_mounted - checks if a file system is mounted at its mount point

@@ -48,10 +49,10 @@ extern bool_t fs_mounted( char *fstype,
 		          char *mntpt,
 		          uuid_t *fsid );
 
-/* fs_getid - retrieves the uuid of the file system containing the named
- * file. returns -1 with errno set on error.
+/* fs_getinfo - retrieves the uuid & flags of the file system containing the
+ * named file. returns -1 with errno set on error.
  */
-extern intgen_t fs_getid( char *fullpathname, uuid_t *fsidp );
+extern intgen_t fs_getinfo( char *fullpathname, uuid_t *fsidp, __u32 *fsflgp );
 
 /* tells how many inos in use
  */

dump/content.c +2 -1

index ec5e954..9ed8459 100644
@@ -526,6 +526,7 @@ content_init( intgen_t argc,
 	char fstype[ CONTENT_HDR_FSTYPE_SZ ];
 	bool_t skip_unchanged_dirs = BOOL_FALSE;
 	uuid_t fsid;
+	__u32 fsflags;
 	bool_t underfoundpr;
 	ix_t underlevel = ( ix_t )( -1 );
 	time32_t undertime = 0;

@@ -770,6 +771,7 @@ content_init( intgen_t argc,
 			mntpnt,
 			sizeof( mntpnt ),
 			&fsid,
+			&fsflags,
 			srcname )) {
 
 		mlog( MLOG_NORMAL | MLOG_ERROR, _(

@@ -789,7 +791,6 @@ content_init( intgen_t argc,
 		      srcname );
 		return BOOL_FALSE;
 	}
-
 	/* place the fs info in the write hdr template
 	 */
 	( void )strncpyterm( cwhdrtemplatep->ch_mntpnt,

dump/var.c +2 -1

index 8370fa4..7adcc7d 100644
@@ -79,11 +79,12 @@ void
 var_skip( uuid_t *dumped_fsidp, void ( *cb )( xfs_ino_t ino ))
 {
 	uuid_t fsid;
+	__u32 fsflags;
 	intgen_t rval;
 
 	/* see if the fs uuid's match
 	 */
-	rval = fs_getid( XFSDUMP_DIRPATH, &fsid );
+	rval = fs_getinfo( XFSDUMP_DIRPATH, &fsid, &fsflags);
 	if ( rval ) {
 #ifdef HIDDEN
                 /* NOTE: this will happen for non-XFS file systems */

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

Eric Sandeen

2012-10-12 21:35 UTC #3 lore raw

PATCH 2/3 xfsdump: default to V3, use V4 if projid32bit is set

In reply to Eric Sandeen

Because 32-bit project ID is not default and semi-rare, default to version 3 dumps unless dumping a filesystem with the 32-bit project ID feature flag set.

XFS_FSOP_GEOM_FLAGS_PROJID32 is a newish flag so I've redefined it here just in caes as well.

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

---

common/fs.h +4 -0

index 878385b..7e63f8e 100644
@@ -26,6 +26,10 @@
 #define FS_MAXNAMELEN_DEFAULT	256
 #define FS_MAXPATHLEN_DEFAULT	1024
 
+#ifndef XFS_FSOP_GEOM_FLAGS_PROJID32
+#define XFS_FSOP_GEOM_FLAGS_PROJID32 0x0800
+#endif
+
 /* fs_info - decides if a source name describes a file system, and if
  * so returns useful information about that file system.
  *

common/global.h +2 -2

index 5138ed8..a847c5d 100644
@@ -29,14 +29,14 @@
 #define GLOBAL_HDR_VERSION_2	2
 #define GLOBAL_HDR_VERSION_3	3
 #define GLOBAL_HDR_VERSION_4	4
-	/* version 4 adds 32-bit projid (projid_hi)
+	/* version 4 adds 32-bit projid (projid_hi) only used if projid32 in place.
 	 * version 3 uses the full 32-bit inode generation number in direnthdr_t.
 	 * version 2 adds encoding of holes and a change to on-tape inventory format.
 	 * version 1 adds extended file attribute dumping.
 	 * version 0 xfsrestore can't handle media produced
 	 * by version 1 xfsdump. 
 	 */
-#define GLOBAL_HDR_VERSION	GLOBAL_HDR_VERSION_4
+#define GLOBAL_HDR_VERSION	GLOBAL_HDR_VERSION_3
 
 #define GLOBAL_HDR_STRING_SZ	0x100
 #define GLOBAL_HDR_TIME_SZ	4

dump/content.c +9 -0

index 9ed8459..94ca787 100644
@@ -791,6 +791,15 @@ content_init( intgen_t argc,
 		      srcname );
 		return BOOL_FALSE;
 	}
+
+	/* If 32 bit project IDs are in use, bump the header version */
+	if ((fsflags & XFS_FSOP_GEOM_FLAGS_PROJID32) &&
+	    gwhdrtemplatep->gh_version < GLOBAL_HDR_VERSION_4) {
+		mlog( MLOG_NORMAL | MLOG_NOTE, _(
+			"32bit project ids in use, bumping to dump version 4\n"));
+		gwhdrtemplatep->gh_version = GLOBAL_HDR_VERSION_4;
+	}
+
 	/* place the fs info in the write hdr template
 	 */
 	( void )strncpyterm( cwhdrtemplatep->ch_mntpnt,

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

Eric Sandeen

2012-10-12 21:40 UTC #6 lore raw

PATCH 3/3 xfsdump: refuse restore of V4 format unless FS has projid32bit set

In reply to Eric Sandeen

Restoring a dump w/ the top 16 bits of a 32-bit project ID set will fail to restore the full ID unless the projid32 feature flag is set on the filesystem.

So if the target fs is xfs, fail the restore if we have a version 4 dump (only used currently if dumped from a projid32 fs) and the target xfs filesystem does not have that feature set.

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

---

p.s. i'd rather go by whether the contents of the dump have any

Quoted 1 line

16 bit project ids than by the feature flag, but I don't see

a way to do that.

restore/content.c +26 -0

index edd00ed..fb15779 100644
@@ -36,6 +36,7 @@
 #include <pthread.h>
 
 #include "types.h"
+#include "fs.h"
 #include "timeutil.h"
 #include "util.h"
 #include "cldmgr.h"

@@ -2021,6 +2022,8 @@ content_stream_restore( ix_t thrdix )
 		bool_t resumepr;
 		ix_t level;
 		uuid_t *baseidp;
+		uuid_t fsuuid;
+		__u32 fsflags = 0;
 
 		rv = Media_mfile_next( Mediap,
 				       PURP_SEARCH,

@@ -2160,6 +2163,29 @@ content_stream_restore( ix_t thrdix )
 			Media_end( Mediap );
 			return mlog_exit(EXIT_ERROR, RV_COMPAT);
 		}
+
+		/* if dump format supports 32 bit project IDs and target fs
+		 * is XFS, make sure the target fs supports it as well
+		 */
+		if ( persp->a.dstdirisxfspr &&
+		     grhdrp->gh_version >= GLOBAL_HDR_VERSION_4) {
+			rval = fs_getinfo(persp->a.dstdir, &fsuuid, &fsflags);
+			if ( rval ) {
+				mlog( MLOG_NORMAL | MLOG_WARNING, _(
+					"could not query filesystem features "
+					"for %s\n"),
+					persp->a.dstdir);
+			}
+			if (!(fsflags & XFS_FSOP_GEOM_FLAGS_PROJID32)) {
+				mlog( MLOG_NORMAL | MLOG_ERROR, _(
+					"dump version >= %d requires projid32 "
+					"feature on target filesystem\n"),
+					GLOBAL_HDR_VERSION_4);
+				Media_end( Mediap );
+				return mlog_exit(EXIT_ERROR, RV_COMPAT);
+			}
+		}
+
 		strncpyterm( persp->s.dumplab,
 			     grhdrp->gh_dumplabel,
 			     sizeof( persp->s.dumplab ));

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

Eric Sandeen

2012-10-22 14:34 UTC #8 lore raw

In reply to Eric Sandeen

Quoted 18 lines · Eric Sandeen

I recently sent a patch for 32-bit project IDs for xfsdump, to properly restore the top 16 bits, which otherwise get lost. This forced a new dump format version 4 (we were currently at 3).
One thing missing is that we should not restore a dump with 32-bit project IDs onto a filesystem w/o that format; the restore will fail to restore the top 16 bits (but otherwise it returns success; attribute setting failures are not fatal (!?))
Also, 32-bit project ID is a bit uncommon; bumping the format (and making older restore incompatible) is a bit draconian.
3 patches here:
1/3: extend fs info call to get fs flags as well
2/3: default back to V3 and go to V4 only if the projid32 flag is set 3/3: fail restore if the target XFS fs doesn't have projid32 set

Quoted 5 lines

I have to say, I'm not super happy with this. I have nagging fear of feature-flag-itis, and I'm not sure how extensible this is as newer versions may appear. But anyway, here's a place to start.
(p.s. anybody have wkendall's new email?) ;)

I spoke with Bill, and he actually didn't feel that a new version was needed for the projid32 fix. I'd like to get some discussion here, and reach an agreement. *NOT* bumping the version simplifies a whole lot of things.

Here's what I'd said to Bill:

Quoted 10 lines

Quoted 10 lines
If we restore old dumps w/ new xfsdump, nothing special is needed; 0 gets restored for the top 16 bits (vs. garbage, which WOULD be bad).
So bumping the version really only prevents old restore from restoring newer dumps.
If I *didn't* bump the version, then old restore would work, and would simply not restore the top 16 bits - just like an old dump+restore option did.

And Bill replied:

Quoted 3 lines

Had a look at xfsdump, and I agree, there's no need to bump the format version. Nice of someone to leave some zeroed pad bytes next to the project id.

so what are people's thoughts? Moving to a new version has complexity & compatibility consequences...

Thanks,
-Eric

Quoted 5 lines

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

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

Ben Myers

2012-10-22 15:54 UTC #9 lore raw

Hey Eric,

Quoted 23 lines · Eric Sandeen

Quoted 22 lines · Eric Sandeen
I recently sent a patch for 32-bit project IDs for xfsdump, to properly restore the top 16 bits, which otherwise get lost. This forced a new dump format version 4 (we were currently at 3).
One thing missing is that we should not restore a dump with 32-bit project IDs onto a filesystem w/o that format; the restore will fail to restore the top 16 bits (but otherwise it returns success; attribute setting failures are not fatal (!?))
Also, 32-bit project ID is a bit uncommon; bumping the format (and making older restore incompatible) is a bit draconian.
3 patches here:
1/3: extend fs info call to get fs flags as well
2/3: default back to V3 and go to V4 only if the projid32 flag is set 3/3: fail restore if the target XFS fs doesn't have projid32 set
I have to say, I'm not super happy with this. I have nagging fear of feature-flag-itis, and I'm not sure how extensible this is as newer versions may appear. But anyway, here's a place to start.

Quoted 6 lines

Quoted 1 line
(p.s. anybody have wkendall's new email?) ;)
I spoke with Bill, and he actually didn't feel that a new version was needed for the projid32 fix. I'd like to get some discussion here, and reach an agreement. *NOT* bumping the version simplifies a whole lot of things.

Can you post a list of things that it simplifies? That would help with the discussion.

Quoted 13 lines

Here's what I'd said to Bill:
Quoted 10 lines
Quoted 10 lines
If we restore old dumps w/ new xfsdump, nothing special is needed; 0 gets restored for the top 16 bits (vs. garbage, which WOULD be bad).
So bumping the version really only prevents old restore from restoring newer dumps.
If I *didn't* bump the version, then old restore would work, and would simply not restore the top 16 bits - just like an old dump+restore option did.

Quoted 8 lines

And Bill replied:
Quoted 3 lines
Had a look at xfsdump, and I agree, there's no need to bump the format version. Nice of someone to leave some zeroed pad bytes next to the project id.
so what are people's thoughts? Moving to a new version has complexity & compatibility consequences...

Initially I think that moving to the new version was the right thing to do. It's good for users to have some warning when there are consequences of upgrading to a new release of xfsdump.

A new dump format version will make old xfs_restore fail with an error if a v4 dump is encountered, rather than do the restore and chop off the top 16 bits of the project id silently. I think that for a user it is reasonable to expect to be notified if project ids are going to be lost. Looks like there is no way for an older xfsrestore to force an override, so if there is to be some level of compatability between versions, newer xfsdump would have to still be able to create v3 dumps.

What other complexity/compat issues are there?

Regards,
Ben

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

Eric Sandeen

2012-10-22 16:20 UTC #10 lore raw

Quoted 70 lines · Ben Myers

Hey Eric,
Quoted 23 lines · Eric Sandeen
Quoted 22 lines · Eric Sandeen
I recently sent a patch for 32-bit project IDs for xfsdump, to properly restore the top 16 bits, which otherwise get lost. This forced a new dump format version 4 (we were currently at 3).
One thing missing is that we should not restore a dump with 32-bit project IDs onto a filesystem w/o that format; the restore will fail to restore the top 16 bits (but otherwise it returns success; attribute setting failures are not fatal (!?))
Also, 32-bit project ID is a bit uncommon; bumping the format (and making older restore incompatible) is a bit draconian.
3 patches here:
1/3: extend fs info call to get fs flags as well
2/3: default back to V3 and go to V4 only if the projid32 flag is set 3/3: fail restore if the target XFS fs doesn't have projid32 set
I have to say, I'm not super happy with this. I have nagging fear of feature-flag-itis, and I'm not sure how extensible this is as newer versions may appear. But anyway, here's a place to start.
Quoted 6 lines
Quoted 1 line
(p.s. anybody have wkendall's new email?) ;)
I spoke with Bill, and he actually didn't feel that a new version was needed for the projid32 fix. I'd like to get some discussion here, and reach an agreement. *NOT* bumping the version simplifies a whole lot of things.
Can you post a list of things that it simplifies? That would help with the discussion.
Quoted 13 lines
Here's what I'd said to Bill:
Quoted 10 lines
Quoted 10 lines
If we restore old dumps w/ new xfsdump, nothing special is needed; 0 gets restored for the top 16 bits (vs. garbage, which WOULD be bad).
So bumping the version really only prevents old restore from restoring newer dumps.
If I *didn't* bump the version, then old restore would work, and would simply not restore the top 16 bits - just like an old dump+restore option did.
Quoted 8 lines
And Bill replied:
Quoted 3 lines
Had a look at xfsdump, and I agree, there's no need to bump the format version. Nice of someone to leave some zeroed pad bytes next to the project id.
so what are people's thoughts? Moving to a new version has complexity & compatibility consequences...
Initially I think that moving to the new version was the right thing to do. It's good for users to have some warning when there are consequences of upgrading to a new release of xfsdump.
A new dump format version will make old xfs_restore fail with an error if a v4 dump is encountered, rather than do the restore and chop off the top 16 bits of the project id silently. I think that for a user it is reasonable to expect to be notified if project ids are going to be lost. Looks like there is no way for an older xfsrestore to force an override, so if there is to be some level of compatability between versions, newer xfsdump would have to still be able to create v3 dumps.

Quoted 1 line

What other complexity/compat issues are there?

Well, I started putting in some feature flag detection. So in a super-perfect world we could:

* Dump to to V4 only if the PROJID32 flag is set on the fs being dumped, else V3
* Refuse / warn if restoring a < V4 dump onto a fs w/ the PROJID32 set
* Refuse / warn if restoring a >= V4 dump onto a non-PROJID32 fs
* Extend the -K option to allow specifying/forcing arbitrary dump levels

But:

* No released kernel yet reports the feature flag, so we don't have guarantees about feature presence
- So going to V4 only if flag found will silently fail prior to kernel 3.7, since no feature flag is available there.
- so then we should default to V4 regardless of feature flag test ~ so then we can't make restore of V4 require the feature flag on the fs
* Older dump/restore already has the silent failure problem
* By the time you are restoring a <V4 dump onto a PROJID32 fs, it's unlikely that you have further options anyway (you're possibly in recovery, can't go back & make V4 dumps to restore from)
* Warning on >= V4 dump to non-PROJID32 fs only makes sense if V4 is contingent on feature set/recognized at dump time but we cant' reliably detect that.
* projid32bit is a non-default option today, so it's an infrequent case...

-Eric

Quoted 3 lines

Regards,
Ben

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

Eric Sandeen

2012-10-22 20:09 UTC #13 lore raw

In reply to Eric Sandeen

Quoted 9 lines · Eric Sandeen

I recently sent a patch for 32-bit project IDs for xfsdump, to properly restore the top 16 bits, which otherwise get lost. This forced a new dump format version 4 (we were currently at 3).
One thing missing is that we should not restore a dump with 32-bit project IDs onto a filesystem w/o that format; the restore will fail to restore the top 16 bits (but otherwise it returns success; attribute setting failures are not fatal (!?))

Quoted 8 lines

Also, 32-bit project ID is a bit uncommon; bumping the format (and making older restore incompatible) is a bit draconian.
3 patches here:
1/3: extend fs info call to get fs flags as well
2/3: default back to V3 and go to V4 only if the projid32 flag is set 3/3: fail restore if the target XFS fs doesn't have projid32 set

I think I should self-NAK this patchset, after a bit more thought.

Because the PROJID32BIT feature flag isn't available via the GEOM ioctl for almost every kernel in existence (it'll be in what, 3.8?),

2/3) we'll fall back to V3 even if projid32bit is actually in use, and/or 3/3) we'll refuse to restore perfectly valid V4 dumps with 32-bit
projids to older kernels just because they don't report the feature.

I think this'd cause more confusion/trouble than it's worth, since we've been lacking the GEOM flag for so long.

-Eric

Quoted 11 lines

I have to say, I'm not super happy with this. I have nagging fear of feature-flag-itis, and I'm not sure how extensible this is as newer versions may appear. But anyway, here's a place to start.
(p.s. anybody have wkendall's new email?) ;)
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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

Ben Myers

2012-10-22 20:22 UTC #11 lore raw

In reply to Eric Sandeen

Hey Eric,

Quoted 69 lines · Eric Sandeen

Quoted 68 lines · Ben Myers
Quoted 23 lines · Eric Sandeen
Quoted 22 lines · Eric Sandeen
I recently sent a patch for 32-bit project IDs for xfsdump, to properly restore the top 16 bits, which otherwise get lost. This forced a new dump format version 4 (we were currently at 3).
One thing missing is that we should not restore a dump with 32-bit project IDs onto a filesystem w/o that format; the restore will fail to restore the top 16 bits (but otherwise it returns success; attribute setting failures are not fatal (!?))
Also, 32-bit project ID is a bit uncommon; bumping the format (and making older restore incompatible) is a bit draconian.
3 patches here:
1/3: extend fs info call to get fs flags as well
2/3: default back to V3 and go to V4 only if the projid32 flag is set 3/3: fail restore if the target XFS fs doesn't have projid32 set
I have to say, I'm not super happy with this. I have nagging fear of feature-flag-itis, and I'm not sure how extensible this is as newer versions may appear. But anyway, here's a place to start.
Quoted 6 lines
Quoted 1 line
(p.s. anybody have wkendall's new email?) ;)
I spoke with Bill, and he actually didn't feel that a new version was needed for the projid32 fix. I'd like to get some discussion here, and reach an agreement. *NOT* bumping the version simplifies a whole lot of things.
Can you post a list of things that it simplifies? That would help with the discussion.
Quoted 13 lines
Here's what I'd said to Bill:
Quoted 10 lines
Quoted 10 lines
If we restore old dumps w/ new xfsdump, nothing special is needed; 0 gets restored for the top 16 bits (vs. garbage, which WOULD be bad).
So bumping the version really only prevents old restore from restoring newer dumps.
If I *didn't* bump the version, then old restore would work, and would simply not restore the top 16 bits - just like an old dump+restore option did.
Quoted 8 lines
And Bill replied:
Quoted 3 lines
Had a look at xfsdump, and I agree, there's no need to bump the format version. Nice of someone to leave some zeroed pad bytes next to the project id.
so what are people's thoughts? Moving to a new version has complexity & compatibility consequences...
Initially I think that moving to the new version was the right thing to do. It's good for users to have some warning when there are consequences of upgrading to a new release of xfsdump.
A new dump format version will make old xfs_restore fail with an error if a v4 dump is encountered, rather than do the restore and chop off the top 16 bits of the project id silently. I think that for a user it is reasonable to expect to be notified if project ids are going to be lost. Looks like there is no way for an older xfsrestore to force an override, so if there is to be some level of compatability between versions, newer xfsdump would have to still be able to create v3 dumps.

Quoted 6 lines

Quoted 1 line
What other complexity/compat issues are there?
Well, I started putting in some feature flag detection. So in a super-perfect world we could:
* Dump to to V4 only if the PROJID32 flag is set on the fs being dumped, else V3

It might be better to just do v4 always (except if -K is specified). I guess I don't understand what the additional level of complexity buys us.

Quoted 1 line

* Refuse / warn if restoring a < V4 dump onto a fs w/ the PROJID32 set

Lets see if I understand...

In this case we're warning the user because we are concerned that they may have been running with projid32 and dumped the filesystem with an older xfsdump which didn't capture the top 16 bits of the projid. The restore of the filesystem would result in the loss of those top 16 bits.

Quoted 1 line

* Refuse / warn if restoring a >= V4 dump onto a non-PROJID32 fs

In this case we're warning the user because... the filesystem would drop the top 16 bits of the valid project ids in the dump?

Quoted 1 line

* Extend the -K option to allow specifying/forcing arbitrary dump levels

This would allow the newer xfsdump to create a v3 dump level.

Quoted 12 lines

But:
* No released kernel yet reports the feature flag, so we don't have guarantees about feature presence
- So going to V4 only if flag found will silently fail prior to kernel 3.7, since no feature flag is available there.
- so then we should default to V4 regardless of feature flag test ~ so then we can't make restore of V4 require the feature flag on the fs
* Older dump/restore already has the silent failure problem
* By the time you are restoring a <V4 dump onto a PROJID32 fs, it's unlikely that you have further options anyway (you're possibly in recovery, can't go back & make V4 dumps to restore from)

xfsdump/restore aren't just for those recovery situations: some people might dump on one machine and pipe it to another host during an upgrade.

Quoted 3 lines

* Warning on >= V4 dump to non-PROJID32 fs only makes sense if V4 is contingent on feature set/recognized at dump time but we cant' reliably detect that.
* projid32bit is a non-default option today, so it's an infrequent case...

It sounds like the super-perfect world needs to have kernel support for reporting whether a filesystem has the projid32 feature flag set. Are there other options for getting that data? xfs_db?

In this somewhat-less-than-perfect world, I think it might be ok to extend the -K option to have some backward compatability in the near term, and then work toward improved warnings once your fsgeom series hits upstream. Maybe I'm missing something.

Regards,
Ben

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

Eric Sandeen

2012-10-22 20:28 UTC #12 lore raw

Quoted 74 lines · Ben Myers

Hey Eric,
Quoted 71 lines · Eric Sandeen
Quoted 69 lines · Ben Myers
Quoted 23 lines · Eric Sandeen
Quoted 22 lines · Eric Sandeen
I recently sent a patch for 32-bit project IDs for xfsdump, to properly restore the top 16 bits, which otherwise get lost. This forced a new dump format version 4 (we were currently at 3).
One thing missing is that we should not restore a dump with 32-bit project IDs onto a filesystem w/o that format; the restore will fail to restore the top 16 bits (but otherwise it returns success; attribute setting failures are not fatal (!?))
Also, 32-bit project ID is a bit uncommon; bumping the format (and making older restore incompatible) is a bit draconian.
3 patches here:
1/3: extend fs info call to get fs flags as well
2/3: default back to V3 and go to V4 only if the projid32 flag is set 3/3: fail restore if the target XFS fs doesn't have projid32 set
I have to say, I'm not super happy with this. I have nagging fear of feature-flag-itis, and I'm not sure how extensible this is as newer versions may appear. But anyway, here's a place to start.
Quoted 6 lines
Quoted 1 line
(p.s. anybody have wkendall's new email?) ;)
I spoke with Bill, and he actually didn't feel that a new version was needed for the projid32 fix. I'd like to get some discussion here, and reach an agreement. *NOT* bumping the version simplifies a whole lot of things.
Can you post a list of things that it simplifies? That would help with the discussion.
Quoted 13 lines
Here's what I'd said to Bill:
Quoted 10 lines
Quoted 10 lines
If we restore old dumps w/ new xfsdump, nothing special is needed; 0 gets restored for the top 16 bits (vs. garbage, which WOULD be bad).
So bumping the version really only prevents old restore from restoring newer dumps.
If I *didn't* bump the version, then old restore would work, and would simply not restore the top 16 bits - just like an old dump+restore option did.
Quoted 8 lines
And Bill replied:
Quoted 3 lines
Had a look at xfsdump, and I agree, there's no need to bump the format version. Nice of someone to leave some zeroed pad bytes next to the project id.
so what are people's thoughts? Moving to a new version has complexity & compatibility consequences...
Initially I think that moving to the new version was the right thing to do. It's good for users to have some warning when there are consequences of upgrading to a new release of xfsdump.
A new dump format version will make old xfs_restore fail with an error if a v4 dump is encountered, rather than do the restore and chop off the top 16 bits of the project id silently. I think that for a user it is reasonable to expect to be notified if project ids are going to be lost. Looks like there is no way for an older xfsrestore to force an override, so if there is to be some level of compatability between versions, newer xfsdump would have to still be able to create v3 dumps.
What other complexity/compat issues are there?

Quoted 7 lines

Quoted 4 lines
Well, I started putting in some feature flag detection. So in a super-perfect world we could:
* Dump to to V4 only if the PROJID32 flag is set on the fs being dumped, else V3
It might be better to just do v4 always (except if -K is specified). I guess I don't understand what the additional level of complexity buys us.

Because then "V4" signifies "top 16 bits are set in projid"

Quoted 8 lines

Quoted 1 line
* Refuse / warn if restoring a < V4 dump onto a fs w/ the PROJID32 set
Lets see if I understand...
In this case we're warning the user because we are concerned that they may have been running with projid32 and dumped the filesystem with an older xfsdump which didn't capture the top 16 bits of the projid. The restore of the filesystem would result in the loss of those top 16 bits.

right

Quoted 4 lines

Quoted 1 line
* Refuse / warn if restoring a >= V4 dump onto a non-PROJID32 fs
In this case we're warning the user because... the filesystem would drop the top 16 bits of the valid project ids in the dump?

well, I think dump would continue but would issue failures/warnings when it tries to set the high bits of projid. Failing to set attrs isn't fatal to the restore process, IIRC.

Quoted 3 lines

Quoted 1 line
* Extend the -K option to allow specifying/forcing arbitrary dump levels
This would allow the newer xfsdump to create a v3 dump level.

yeah.

Quoted 13 lines

Quoted 12 lines
But:
* No released kernel yet reports the feature flag, so we don't have guarantees about feature presence
- So going to V4 only if flag found will silently fail prior to kernel 3.7, since no feature flag is available there.
- so then we should default to V4 regardless of feature flag test ~ so then we can't make restore of V4 require the feature flag on the fs
* Older dump/restore already has the silent failure problem
* By the time you are restoring a <V4 dump onto a PROJID32 fs, it's unlikely that you have further options anyway (you're possibly in recovery, can't go back & make V4 dumps to restore from)

Quoted 2 lines

xfsdump/restore aren't just for those recovery situations: some people might dump on one machine and pipe it to another host during an upgrade.

fair enough.

Quoted 7 lines

Quoted 3 lines
* Warning on >= V4 dump to non-PROJID32 fs only makes sense if V4 is contingent on feature set/recognized at dump time but we cant' reliably detect that.
* projid32bit is a non-default option today, so it's an infrequent case...
It sounds like the super-perfect world needs to have kernel support for reporting whether a filesystem has the projid32 feature flag set. Are there other options for getting that data? xfs_db?

I suppose, but do you really want xfsdump to shell out to xfs_db, or .. ?

Quoted 4 lines

In this somewhat-less-than-perfect world, I think it might be ok to extend the -K option to have some backward compatability in the near term, and then work toward improved warnings once your fsgeom series hits upstream. Maybe I'm missing something.

I think it's too many knobs with too many caveats, and too much work, for very little gain. Bear in mind this dump/restore bug has never even been reported in the wild.

-Eric

Quoted 3 lines

Regards,
Ben

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

Rich Johnston

Reviewed-by on v1 1/3

2012-11-09 18:29 UTC #2 lore raw

Re: [PATCH 1/3] xfsdump: extend fs_info to gather fs feature flags

In reply to Eric Sandeen

Quoted 8 lines · Eric Sandeen

extend fs_info and fs_getid to return fs flags (features) from GEOM call as well. Not used yet in this patch. Will be used in subsequent patch to detect 32-bit project ID filesystems.
Signed-off-by Eric Sandeen <[email protected]>
---

Looks good Eric.

Reviewed-by Rich Johnston <[email protected]>

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

Rich Johnston

2012-11-09 18:30 UTC #4 lore raw

Re: [PATCH 2/3] xfsdump: default to V3, use V4 if projid32bit is set

In reply to Eric Sandeen

Quoted 30 lines · Eric Sandeen

Because 32-bit project ID is not default and semi-rare, default to version 3 dumps unless dumping a filesystem with the 32-bit project ID feature flag set.
XFS_FSOP_GEOM_FLAGS_PROJID32 is a newish flag so I've redefined it here just in caes as well.
Signed-off-by Eric Sandeen <[email protected]>
---
common/fs.h +4 -0
index 878385b..7e63f8e 100644
@@ -26,6 +26,10 @@
  #define FS_MAXNAMELEN_DEFAULT	256
  #define FS_MAXPATHLEN_DEFAULT	1024
 
+#ifndef XFS_FSOP_GEOM_FLAGS_PROJID32
+#define XFS_FSOP_GEOM_FLAGS_PROJID32 0x0800
+#endif
+
  /* fs_info - decides if a source name describes a file system, and if
   * so returns useful information about that file system.
   *
common/global.h +0 -0
index 5138ed8..a847c5d 100644

common/global.h line 31

Quoted 3 lines

common/global.h +0 -0

@@ -29,14 +29,14 @@
  #define GLOBAL_HDR_VERSION_2	2
  #define GLOBAL_HDR_VERSION_3	3
  #define GLOBAL_HDR_VERSION_4	4

Hmm looks a a patch from your previous version.

common/global.h line 42

Quoted 13 lines

common/global.h +2 -2

-	/* version 4 adds 32-bit projid (projid_hi)
+	/* version 4 adds 32-bit projid (projid_hi) only used if projid32 in place.
  	 * version 3 uses the full 32-bit inode generation number in direnthdr_t.
  	 * version 2 adds encoding of holes and a change to on-tape inventory format.
  	 * version 1 adds extended file attribute dumping.
  	 * version 0 xfsrestore can't handle media produced
  	 * by version 1 xfsdump.
  	 */
-#define GLOBAL_HDR_VERSION	GLOBAL_HDR_VERSION_4
+#define GLOBAL_HDR_VERSION	GLOBAL_HDR_VERSION_3
 
  #define GLOBAL_HDR_STRING_SZ	0x100
  #define GLOBAL_HDR_TIME_SZ	4

I used the following for testing purposes.

common/global.h +3 -1

Index: b/common/global.h
===================================================================
@@ -28,7 +28,9 @@
 #define GLOBAL_HDR_VERSION_1	1
 #define GLOBAL_HDR_VERSION_2	2
 #define GLOBAL_HDR_VERSION_3	3
-	/* version 3 uses the full 32-bit inode generation number in direnthdr_t.
+#define GLOBAL_HDR_VERSION_4	4
+	/* version 4 adds 32-bit projid (projid_hi) only used if projid32 in place.
+	 * version 3 uses the full 32-bit inode generation number in direnthdr_t.
 	 * version 2 adds encoding of holes and a change to on-tape inventory

format.
* version 1 adds extended file attribute dumping.
* version 0 xfsrestore can't handle media produced

Rest of the patch looks good. Correct the common/global.h section and you can add a:

Reviewed-by Rich Johnston <[email protected]>

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

Rich Johnston

Reviewed-by on v1 3/3

2012-11-09 18:30 UTC #7 lore raw

Re: [PATCH 3/3] xfsdump: refuse restore of V4 format unless FS has projid32bit set

In reply to Eric Sandeen

restore/content.c

Quoted 16 lines · Eric Sandeen

Restoring a dump w/ the top 16 bits of a 32-bit project ID set will fail to restore the full ID unless the projid32 feature flag is set on the filesystem.
So if the target fs is xfs, fail the restore if we have a version 4 dump (only used currently if dumped from a projid32 fs) and the target xfs filesystem does not have that feature set.
Signed-off-by Eric Sandeen <[email protected]>
---
p.s. i'd rather go by whether the contents of the dump have any
Quoted 1 line
16 bit project ids than by the feature flag, but I don't see
a way to do that.
restore/content.c +0 -0

Looks good Eric.

Reviewed-by Rich Johnston <[email protected]>

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

Eric Sandeen

2012-11-09 18:30 UTC #5 lore raw

Re: [PATCH 2/3] xfsdump: default to V3, use V4 if projid32bit is set

In reply to Rich Johnston

Quoted 68 lines · Rich Johnston

Quoted 30 lines · Eric Sandeen

Because 32-bit project ID is not default and semi-rare, default to version 3 dumps unless dumping a filesystem with the 32-bit project ID feature flag set.
XFS_FSOP_GEOM_FLAGS_PROJID32 is a newish flag so I've redefined it here just in caes as well.
Signed-off-by Eric Sandeen <[email protected]>
---
common/fs.h +4 -0
index 878385b..7e63f8e 100644
@@ -26,6 +26,10 @@
  #define FS_MAXNAMELEN_DEFAULT    256
  #define FS_MAXPATHLEN_DEFAULT    1024
 
+#ifndef XFS_FSOP_GEOM_FLAGS_PROJID32
+#define XFS_FSOP_GEOM_FLAGS_PROJID32 0x0800
+#endif
+
  /* fs_info - decides if a source name describes a file system, and if
   * so returns useful information about that file system.
   *
common/global.h +0 -0
index 5138ed8..a847c5d 100644

common/global.h line 31

Quoted 3 lines

common/global.h +0 -0

@@ -29,14 +29,14 @@
  #define GLOBAL_HDR_VERSION_2    2
  #define GLOBAL_HDR_VERSION_3    3
  #define GLOBAL_HDR_VERSION_4    4

Hmm looks a a patch from your previous version.

common/global.h line 42

Quoted 13 lines

common/global.h +2 -2

-    /* version 4 adds 32-bit projid (projid_hi)
+    /* version 4 adds 32-bit projid (projid_hi) only used if projid32 in place.
       * version 3 uses the full 32-bit inode generation number in direnthdr_t.
       * version 2 adds encoding of holes and a change to on-tape inventory format.
       * version 1 adds extended file attribute dumping.
       * version 0 xfsrestore can't handle media produced
       * by version 1 xfsdump.
       */
-#define GLOBAL_HDR_VERSION    GLOBAL_HDR_VERSION_4
+#define GLOBAL_HDR_VERSION    GLOBAL_HDR_VERSION_3
 
  #define GLOBAL_HDR_STRING_SZ    0x100
  #define GLOBAL_HDR_TIME_SZ    4

I used the following for testing purposes.

Index: b/common/global.h
===================================================================

common/global.h +3 -1

@@ -28,7 +28,9 @@
 #define GLOBAL_HDR_VERSION_1    1
 #define GLOBAL_HDR_VERSION_2    2
 #define GLOBAL_HDR_VERSION_3    3
-    /* version 3 uses the full 32-bit inode generation number in direnthdr_t.
+#define GLOBAL_HDR_VERSION_4    4
+    /* version 4 adds 32-bit projid (projid_hi) only used if projid32 in place.
+     * version 3 uses the full 32-bit inode generation number in direnthdr_t.
      * version 2 adds encoding of holes and a change to on-tape inventory format.
      * version 1 adds extended file attribute dumping.
      * version 0 xfsrestore can't handle media produced

Quoted 1 line

Rest of the patch looks good. Correct the common/global.h section and you can add a:

Thanks, but I think we NAK'd this one (and the series) for now, and decided not to bump the version just for this.

-Eric

Quoted 2 lines

Reviewed-by Rich Johnston <[email protected]>

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