[PATCH 0/3] xfsdump: more projid32bit fixes

[PATCH 0/3] xfsdump: more projid32bit fixes

[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.

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

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

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

[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 <sandeen@redhat.com>
---

diff --git a/common/fs.c b/common/fs.c
index 6f4cb6c..bf2f164 100644
--- a/common/fs.c
+++ b/common/fs.c
@@ -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;
 }
diff --git a/common/fs.h b/common/fs.h
index 9ad1636..878385b 100644
--- a/common/fs.h
+++ b/common/fs.h
@@ -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
  */
diff --git a/dump/content.c b/dump/content.c
index ec5e954..9ed8459 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -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,
diff --git a/dump/var.c b/dump/var.c
index 8370fa4..7adcc7d 100644
--- a/dump/var.c
+++ b/dump/var.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[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 <sandeen@redhat.com>
---

diff --git a/common/fs.h b/common/fs.h
index 878385b..7e63f8e 100644
--- a/common/fs.h
+++ b/common/fs.h
@@ -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.
  *
diff --git a/common/global.h b/common/global.h
index 5138ed8..a847c5d 100644
--- a/common/global.h
+++ b/common/global.h
@@ -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
diff --git a/dump/content.c b/dump/content.c
index 9ed8459..94ca787 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[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 <sandeen@redhat.com>
---

p.s. i'd rather go by whether the contents of the dump have any
> 16 bit project ids than by the feature flag, but I don't see
a way to do that.

diff --git a/restore/content.c b/restore/content.c
index edd00ed..fb15779 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/3] xfsdump: more projid32bit fixes

[Eric Sandeen]

On 10/12/12 4:32 PM, Eric Sandeen wrote:
> 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?)  ;)

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:

>> 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:

> 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


> _______________________________________________
> 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 0/3] xfsdump: more projid32bit fixes

[Ben Myers]

Hey Eric,

On Mon, Oct 22, 2012 at 09:36:14AM -0500, Eric Sandeen wrote:
> On 10/12/12 4:32 PM, Eric Sandeen wrote:
> > 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?)  ;)
> 
> 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.

> Here's what I'd said to Bill:
> 
> >> 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:
> 
> > 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/3] xfsdump: more projid32bit fixes

[Eric Sandeen]

On 10/22/12 10:56 AM, Ben Myers wrote:
> Hey Eric,
> 
> On Mon, Oct 22, 2012 at 09:36:14AM -0500, Eric Sandeen wrote:
>> On 10/12/12 4:32 PM, Eric Sandeen wrote:
>>> 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?)  ;)
>>
>> 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.
> 
>> Here's what I'd said to Bill:
>>
>>>> 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:
>>
>>> 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?

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



> Regards,
> 	Ben
> 

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

Re: [PATCH 0/3] xfsdump: more projid32bit fixes

[Eric Sandeen]

On 10/12/12 4:32 PM, Eric Sandeen wrote:
> 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 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

> 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
> 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 0/3] xfsdump: more projid32bit fixes

[Ben Myers]

Hey Eric,

On Mon, Oct 22, 2012 at 11:22:44AM -0500, Eric Sandeen wrote:
> On 10/22/12 10:56 AM, Ben Myers wrote:
> > On Mon, Oct 22, 2012 at 09:36:14AM -0500, Eric Sandeen wrote:
> >> On 10/12/12 4:32 PM, Eric Sandeen wrote:
> >>> 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?)  ;)
> >>
> >> 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.
> > 
> >> Here's what I'd said to Bill:
> >>
> >>>> 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:
> >>
> >>> 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?
> 
> 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.

> * 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.

> * 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?

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

This would allow the newer xfsdump to create a v3 dump level.
 
> 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.

> * 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/3] xfsdump: more projid32bit fixes

[Eric Sandeen]

On 10/22/12 3:24 PM, Ben Myers wrote:
> Hey Eric,
> 
> On Mon, Oct 22, 2012 at 11:22:44AM -0500, Eric Sandeen wrote:
>> On 10/22/12 10:56 AM, Ben Myers wrote:
>>> On Mon, Oct 22, 2012 at 09:36:14AM -0500, Eric Sandeen wrote:
>>>> On 10/12/12 4:32 PM, Eric Sandeen wrote:
>>>>> 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?)  ;)
>>>>
>>>> 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.
>>>
>>>> Here's what I'd said to Bill:
>>>>
>>>>>> 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:
>>>>
>>>>> 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?
>>
>> 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"

>> * 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

>> * 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.

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

yeah.

>> 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.

fair enough.

>> * 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 .. ?

> 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

> Regards,
> 	Ben
> 

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

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

[Rich Johnston]

On 10/12/2012 04:35 PM, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---
>

Looks good Eric.
Reviewed-by: Rich Johnston <rjohnston@sgi.com>

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

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

[Rich Johnston]

On 10/12/2012 04:37 PM, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---
>
> diff --git a/common/fs.h b/common/fs.h
> index 878385b..7e63f8e 100644
> --- a/common/fs.h
> +++ b/common/fs.h
> @@ -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.
>    *
> diff --git a/common/global.h b/common/global.h
> index 5138ed8..a847c5d 100644
> --- a/common/global.h
> +++ b/common/global.h
> @@ -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.
> -	/* 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
===================================================================
--- a/common/global.h
+++ 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 <rjohnston@sgi.com>

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

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

[Rich Johnston]

On 10/12/2012 04:42 PM, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---
>
> p.s. i'd rather go by whether the contents of the dump have any
>> 16 bit project ids than by the feature flag, but I don't see
> a way to do that.
>
> diff --git a/restore/content.c b/restore/content.c

Looks good Eric.
Reviewed-by: Rich Johnston <rjohnston@sgi.com>

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

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

[Eric Sandeen]

On 11/9/12 12:31 PM, Rich Johnston wrote:
> On 10/12/2012 04:37 PM, Eric Sandeen wrote:
>> 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 <sandeen@redhat.com>
>> ---
>>
>> diff --git a/common/fs.h b/common/fs.h
>> index 878385b..7e63f8e 100644
>> --- a/common/fs.h
>> +++ b/common/fs.h
>> @@ -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.
>>    *
>> diff --git a/common/global.h b/common/global.h
>> index 5138ed8..a847c5d 100644
>> --- a/common/global.h
>> +++ b/common/global.h
>> @@ -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.
>> -    /* 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
> ===================================================================
> --- a/common/global.h
> +++ 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:

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

> Reviewed-by Rich Johnston <rjohnston@sgi.com>
> 

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