* [RFC][PATCH] xfs_restore: detect rtinherit on destination @ 2019-06-05 21:16 Sheena Artrip 2019-06-06 14:11 ` Eric Sandeen 0 siblings, 1 reply; 14+ messages in thread From: Sheena Artrip @ 2019-06-05 21:16 UTC (permalink / raw) To: linux-xfs When running xfs_restore with a non-rtdev dump, it will ignore any rtinherit flags on the destination and send I/O to the metadata region. Instead, detect rtinherit on the destination XFS fileystem root inode and use that to override the incoming inode flags. Original version of this patch missed some branches so multiple invocations of xfsrestore onto the same fs caused the rtinherit bit to get re-removed. There could be some additional edge cases in non-realtime to realtime workflows so the outstanding question would be: is it worth supporting? Signed-off-by: Sheena Artrip <sheena.artrip@gmail.com> --- restore/content.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/restore/content.c b/restore/content.c index 6b22965..96dd698 100644 --- a/restore/content.c +++ b/restore/content.c @@ -670,6 +670,9 @@ struct tran { /* to establish critical regions while updating pers * inventory */ + bool_t t_dstisrealtime; + /* to force the realtime flag on incoming inodes + */ }; typedef struct tran tran_t; @@ -1803,6 +1806,51 @@ content_init(int argc, char *argv[], size64_t vmsz) free_handle(fshanp, fshlen); } + /* determine if destination root inode has rtinherit. + * If so, we should force XFS_REALTIME on the incoming inodes. + */ + if (persp->a.dstdirisxfspr) { + stat64_t rootstat; + xfs_fsop_bulkreq_t bulkreq; + int ocount = 0; + xfs_bstat_t *sc_rootxfsstatp; + + int rootfd = open(persp->a.dstdir, O_RDONLY); + + sc_rootxfsstatp = + (xfs_bstat_t *)calloc(1, sizeof(xfs_bstat_t)); + assert(sc_rootxfsstatp); + + /* Get the inode of the destination folder */ + int rval = fstat64(rootfd, &rootstat); + if (rval) { + (void)close(rootfd); + mlog(MLOG_NORMAL, _( + "could not stat %s\n"), + persp->a.dstdir); + return BOOL_FALSE; + } + + /* Get the first valid (i.e. root) inode in this fs */ + bulkreq.lastip = (__u64 *)&rootstat.st_ino; + bulkreq.icount = 1; + bulkreq.ubuffer = sc_rootxfsstatp; + bulkreq.ocount = &ocount; + if (ioctl(rootfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) { + (void)close(rootfd); + mlog(MLOG_ERROR, + _("failed to get bulkstat information for root inode\n")); + return BOOL_FALSE; + } + + (void)close(rootfd); + + /* test against rtinherit */ + if((sc_rootxfsstatp->bs_xflags & XFS_XFLAG_RTINHERIT) != 0) { + tranp->t_dstisrealtime = true; + } + } + /* map in pers. inv. descriptors, if any. NOTE: this ptr is to be * referenced ONLY via the macros provided; the descriptors will be * occasionally remapped, causing the ptr to change. @@ -7270,6 +7318,10 @@ restore_file_cb(void *cp, bool_t linkpr, char *path1, char *path2) bool_t ahcs = contextp->cb_ahcs; stream_context_t *strctxp = (stream_context_t *)drivep->d_strmcontextp; + if (tranp->t_dstisrealtime) { + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; + } + int rval; bool_t ok; @@ -7480,6 +7532,10 @@ restore_reg(drive_t *drivep, if (tranp->t_toconlypr) return BOOL_TRUE; + if (tranp->t_dstisrealtime) { + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; + } + oflags = O_CREAT | O_RDWR; if (persp->a.dstdirisxfspr && bstatp->bs_xflags & XFS_XFLAG_REALTIME) oflags |= O_DIRECT; @@ -8470,6 +8526,11 @@ restore_extent(filehdr_t *fhdrp, } assert(new_off == off); } + + if (tranp->t_dstisrealtime) { + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; + } + if ((fd != -1) && (bstatp->bs_xflags & XFS_XFLAG_REALTIME)) { if ((ioctl(fd, XFS_IOC_DIOINFO, &da) < 0)) { mlog(MLOG_NORMAL | MLOG_WARNING, _( @@ -8729,6 +8790,10 @@ restore_extattr(drive_t *drivep, assert(extattrbufp); + if (tranp->t_dstisrealtime) { + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; + } + if (!isdirpr) isfilerestored = partial_check(bstatp->bs_ino, bstatp->bs_size); -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC][PATCH] xfs_restore: detect rtinherit on destination 2019-06-05 21:16 [RFC][PATCH] xfs_restore: detect rtinherit on destination Sheena Artrip @ 2019-06-06 14:11 ` Eric Sandeen 2019-06-06 18:12 ` Sheena Artrip 0 siblings, 1 reply; 14+ messages in thread From: Eric Sandeen @ 2019-06-06 14:11 UTC (permalink / raw) To: Sheena Artrip, linux-xfs On 6/5/19 4:16 PM, Sheena Artrip wrote: > When running xfs_restore with a non-rtdev dump, > it will ignore any rtinherit flags on the destination > and send I/O to the metadata region. > > Instead, detect rtinherit on the destination XFS fileystem root inode > and use that to override the incoming inode flags. > > Original version of this patch missed some branches so multiple > invocations of xfsrestore onto the same fs caused > the rtinherit bit to get re-removed. There could be some > additional edge cases in non-realtime to realtime workflows so > the outstanding question would be: is it worth supporting? Hm, interesting. So this is a mechanism to allow dump/restore to migrate everything to the realtime subvol? I can't decide if I like this - normally I'd think of an xfsdump/xfsrestore session as more or less replicating the filesystem that was dumped, and not something that will fundamentally change what was dumped. OTOH, we can restore onto any dir we want, and I could see the argument that we should respect things like the rtinherit flag if that's what the destination dir says. One thing about the patch - the mechanism you've copied to get the root inode number via bulkstat turns out to be broken ... it's possible to have a non-root inode with the lowest number on the fs, unfortunately. But, wouldn't you want to test the rtinherit flag on the target dir anyway, not necessarily the root dir? -Eric > Signed-off-by: Sheena Artrip <sheena.artrip@gmail.com> > --- > restore/content.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 65 insertions(+) > > diff --git a/restore/content.c b/restore/content.c > index 6b22965..96dd698 100644 > --- a/restore/content.c > +++ b/restore/content.c > @@ -670,6 +670,9 @@ struct tran { > /* to establish critical regions while updating pers > * inventory > */ > + bool_t t_dstisrealtime; > + /* to force the realtime flag on incoming inodes > + */ > }; > > typedef struct tran tran_t; > @@ -1803,6 +1806,51 @@ content_init(int argc, char *argv[], size64_t vmsz) > free_handle(fshanp, fshlen); > } > > + /* determine if destination root inode has rtinherit. > + * If so, we should force XFS_REALTIME on the incoming inodes. > + */ > + if (persp->a.dstdirisxfspr) { > + stat64_t rootstat; > + xfs_fsop_bulkreq_t bulkreq; > + int ocount = 0; > + xfs_bstat_t *sc_rootxfsstatp; > + > + int rootfd = open(persp->a.dstdir, O_RDONLY); > + > + sc_rootxfsstatp = > + (xfs_bstat_t *)calloc(1, sizeof(xfs_bstat_t)); > + assert(sc_rootxfsstatp); > + > + /* Get the inode of the destination folder */ > + int rval = fstat64(rootfd, &rootstat); > + if (rval) { > + (void)close(rootfd); > + mlog(MLOG_NORMAL, _( > + "could not stat %s\n"), > + persp->a.dstdir); > + return BOOL_FALSE; > + } > + > + /* Get the first valid (i.e. root) inode in this fs */ > + bulkreq.lastip = (__u64 *)&rootstat.st_ino; > + bulkreq.icount = 1; > + bulkreq.ubuffer = sc_rootxfsstatp; > + bulkreq.ocount = &ocount; > + if (ioctl(rootfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) { > + (void)close(rootfd); > + mlog(MLOG_ERROR, > + _("failed to get bulkstat information > for root inode\n")); > + return BOOL_FALSE; > + } > + > + (void)close(rootfd); > + > + /* test against rtinherit */ > + if((sc_rootxfsstatp->bs_xflags & XFS_XFLAG_RTINHERIT) != 0) { > + tranp->t_dstisrealtime = true; > + } > + } > + > /* map in pers. inv. descriptors, if any. NOTE: this ptr is to be > * referenced ONLY via the macros provided; the descriptors will be > * occasionally remapped, causing the ptr to change. > @@ -7270,6 +7318,10 @@ restore_file_cb(void *cp, bool_t linkpr, char > *path1, char *path2) > bool_t ahcs = contextp->cb_ahcs; > stream_context_t *strctxp = (stream_context_t *)drivep->d_strmcontextp; > > + if (tranp->t_dstisrealtime) { > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > + } > + > int rval; > bool_t ok; > > @@ -7480,6 +7532,10 @@ restore_reg(drive_t *drivep, > if (tranp->t_toconlypr) > return BOOL_TRUE; > > + if (tranp->t_dstisrealtime) { > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > + } > + > oflags = O_CREAT | O_RDWR; > if (persp->a.dstdirisxfspr && bstatp->bs_xflags & XFS_XFLAG_REALTIME) > oflags |= O_DIRECT; > @@ -8470,6 +8526,11 @@ restore_extent(filehdr_t *fhdrp, > } > assert(new_off == off); > } > + > + if (tranp->t_dstisrealtime) { > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > + } > + > if ((fd != -1) && (bstatp->bs_xflags & XFS_XFLAG_REALTIME)) { > if ((ioctl(fd, XFS_IOC_DIOINFO, &da) < 0)) { > mlog(MLOG_NORMAL | MLOG_WARNING, _( > @@ -8729,6 +8790,10 @@ restore_extattr(drive_t *drivep, > > assert(extattrbufp); > > + if (tranp->t_dstisrealtime) { > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > + } > + > if (!isdirpr) > isfilerestored = partial_check(bstatp->bs_ino, > bstatp->bs_size); > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC][PATCH] xfs_restore: detect rtinherit on destination 2019-06-06 14:11 ` Eric Sandeen @ 2019-06-06 18:12 ` Sheena Artrip 2019-06-06 18:39 ` Eric Sandeen 0 siblings, 1 reply; 14+ messages in thread From: Sheena Artrip @ 2019-06-06 18:12 UTC (permalink / raw) To: Eric Sandeen, linux-xfs; +Cc: sheenobu On Thu, Jun 6, 2019 at 7:11 AM Eric Sandeen <sandeen@sandeen.net> wrote: > > On 6/5/19 4:16 PM, Sheena Artrip wrote: > > When running xfs_restore with a non-rtdev dump, > > it will ignore any rtinherit flags on the destination > > and send I/O to the metadata region. > > > > Instead, detect rtinherit on the destination XFS fileystem root inode > > and use that to override the incoming inode flags. > > > > Original version of this patch missed some branches so multiple > > invocations of xfsrestore onto the same fs caused > > the rtinherit bit to get re-removed. There could be some > > additional edge cases in non-realtime to realtime workflows so > > the outstanding question would be: is it worth supporting? > > Hm, interesting. > > So this is a mechanism to allow dump/restore to migrate everything > to the realtime subvol? I can't decide if I like this - normally I'd > think of an xfsdump/xfsrestore session as more or less replicating the > filesystem that was dumped, and not something that will fundamentally > change what was dumped. > > OTOH, we can restore onto any dir we want, and I could see the argument > that we should respect things like the rtinherit flag if that's what > the destination dir says. Yes. What is strange is that an xfsrestore onto a rtdev system will silently "fill" the metadata partition until the available inode count goes to zero and we get an ENOSPC. Not yet sure if the file data goes straight to the metadata partition or if it's simply accounting for it in the metadata partition. I'm guessing xfsrestore should either fail-fast or allow this via rtinherit detection. I don't mind putting it behind a flag either. > One thing about the patch - the mechanism you've copied to get the root > inode number via bulkstat turns out to be broken ... it's possible > to have a non-root inode with the lowest number on the fs, unfortunately. I think i saw that on the list but this code is also a near-identical copy of what is in xfsdump/content.c. > But, wouldn't you want to test the rtinherit flag on the target dir anyway, > not necessarily the root dir? Makes sense. How would I get the rtinherit flag on the target dir? Is there a xfs-specific stat function that will give us a xfs_bstat_t for the dstdir inode I've opened or is it already part of stat64_t? > > Signed-off-by: Sheena Artrip <sheena.artrip@gmail.com> > > --- > > restore/content.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 65 insertions(+) > > > > diff --git a/restore/content.c b/restore/content.c > > index 6b22965..96dd698 100644 > > --- a/restore/content.c > > +++ b/restore/content.c > > @@ -670,6 +670,9 @@ struct tran { > > /* to establish critical regions while updating pers > > * inventory > > */ > > + bool_t t_dstisrealtime; > > + /* to force the realtime flag on incoming inodes > > + */ > > }; > > > > typedef struct tran tran_t; > > @@ -1803,6 +1806,51 @@ content_init(int argc, char *argv[], size64_t vmsz) > > free_handle(fshanp, fshlen); > > } > > > > + /* determine if destination root inode has rtinherit. > > + * If so, we should force XFS_REALTIME on the incoming inodes. > > + */ > > + if (persp->a.dstdirisxfspr) { > > + stat64_t rootstat; > > + xfs_fsop_bulkreq_t bulkreq; > > + int ocount = 0; > > + xfs_bstat_t *sc_rootxfsstatp; > > + > > + int rootfd = open(persp->a.dstdir, O_RDONLY); > > + > > + sc_rootxfsstatp = > > + (xfs_bstat_t *)calloc(1, sizeof(xfs_bstat_t)); > > + assert(sc_rootxfsstatp); > > + > > + /* Get the inode of the destination folder */ > > + int rval = fstat64(rootfd, &rootstat); > > + if (rval) { > > + (void)close(rootfd); > > + mlog(MLOG_NORMAL, _( > > + "could not stat %s\n"), > > + persp->a.dstdir); > > + return BOOL_FALSE; > > + } > > + > > + /* Get the first valid (i.e. root) inode in this fs */ > > + bulkreq.lastip = (__u64 *)&rootstat.st_ino; > > + bulkreq.icount = 1; > > + bulkreq.ubuffer = sc_rootxfsstatp; > > + bulkreq.ocount = &ocount; > > + if (ioctl(rootfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) { > > + (void)close(rootfd); > > + mlog(MLOG_ERROR, > > + _("failed to get bulkstat information > > for root inode\n")); > > + return BOOL_FALSE; > > + } > > + > > + (void)close(rootfd); > > + > > + /* test against rtinherit */ > > + if((sc_rootxfsstatp->bs_xflags & XFS_XFLAG_RTINHERIT) != 0) { > > + tranp->t_dstisrealtime = true; > > + } > > + } > > + > > /* map in pers. inv. descriptors, if any. NOTE: this ptr is to be > > * referenced ONLY via the macros provided; the descriptors will be > > * occasionally remapped, causing the ptr to change. > > @@ -7270,6 +7318,10 @@ restore_file_cb(void *cp, bool_t linkpr, char > > *path1, char *path2) > > bool_t ahcs = contextp->cb_ahcs; > > stream_context_t *strctxp = (stream_context_t *)drivep->d_strmcontextp; > > > > + if (tranp->t_dstisrealtime) { > > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > > + } > > + > > int rval; > > bool_t ok; > > > > @@ -7480,6 +7532,10 @@ restore_reg(drive_t *drivep, > > if (tranp->t_toconlypr) > > return BOOL_TRUE; > > > > + if (tranp->t_dstisrealtime) { > > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > > + } > > + > > oflags = O_CREAT | O_RDWR; > > if (persp->a.dstdirisxfspr && bstatp->bs_xflags & XFS_XFLAG_REALTIME) > > oflags |= O_DIRECT; > > @@ -8470,6 +8526,11 @@ restore_extent(filehdr_t *fhdrp, > > } > > assert(new_off == off); > > } > > + > > + if (tranp->t_dstisrealtime) { > > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > > + } > > + > > if ((fd != -1) && (bstatp->bs_xflags & XFS_XFLAG_REALTIME)) { > > if ((ioctl(fd, XFS_IOC_DIOINFO, &da) < 0)) { > > mlog(MLOG_NORMAL | MLOG_WARNING, _( > > @@ -8729,6 +8790,10 @@ restore_extattr(drive_t *drivep, > > > > assert(extattrbufp); > > > > + if (tranp->t_dstisrealtime) { > > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > > + } > > + > > if (!isdirpr) > > isfilerestored = partial_check(bstatp->bs_ino, > > bstatp->bs_size); > > > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC][PATCH] xfs_restore: detect rtinherit on destination 2019-06-06 18:12 ` Sheena Artrip @ 2019-06-06 18:39 ` Eric Sandeen 2019-06-06 19:50 ` Sheena Artrip 2019-06-06 19:57 ` [PATCH v2] " Sheena Artrip 0 siblings, 2 replies; 14+ messages in thread From: Eric Sandeen @ 2019-06-06 18:39 UTC (permalink / raw) To: Sheena Artrip, linux-xfs; +Cc: sheenobu On 6/6/19 1:12 PM, Sheena Artrip wrote: > On Thu, Jun 6, 2019 at 7:11 AM Eric Sandeen <sandeen@sandeen.net> wrote: >> >> On 6/5/19 4:16 PM, Sheena Artrip wrote: >>> When running xfs_restore with a non-rtdev dump, >>> it will ignore any rtinherit flags on the destination >>> and send I/O to the metadata region. >>> >>> Instead, detect rtinherit on the destination XFS fileystem root inode >>> and use that to override the incoming inode flags. >>> >>> Original version of this patch missed some branches so multiple >>> invocations of xfsrestore onto the same fs caused >>> the rtinherit bit to get re-removed. There could be some >>> additional edge cases in non-realtime to realtime workflows so >>> the outstanding question would be: is it worth supporting? >> >> Hm, interesting. >> >> So this is a mechanism to allow dump/restore to migrate everything >> to the realtime subvol? I can't decide if I like this - normally I'd >> think of an xfsdump/xfsrestore session as more or less replicating the >> filesystem that was dumped, and not something that will fundamentally >> change what was dumped. >> >> OTOH, we can restore onto any dir we want, and I could see the argument >> that we should respect things like the rtinherit flag if that's what >> the destination dir says. > > Yes. What is strange is that an xfsrestore onto a rtdev system will > silently "fill" from a filesystem that previously did not have rt files, I guess? > the metadata partition until the available inode count goes to zero and we get > an ENOSPC. Not yet sure if the file data goes straight to the metadata partition > or if it's simply accounting for it in the metadata partition. > > I'm guessing xfsrestore should either fail-fast or allow this via > rtinherit detection. I don't mind putting it behind a flag either. Hm, can you more completely describe the usecase/testcase that leads to the problem? I just want to make sure I have the whole picture. >> One thing about the patch - the mechanism you've copied to get the root >> inode number via bulkstat turns out to be broken ... it's possible >> to have a non-root inode with the lowest number on the fs, unfortunately. > > I think i saw that on the list but this code is also a near-identical > copy of what is in xfsdump/content.c. yeah that's my mistake to fix now ;) >> But, wouldn't you want to test the rtinherit flag on the target dir anyway, >> not necessarily the root dir? > > Makes sense. How would I get the rtinherit flag on the target dir? Is there > a xfs-specific stat function that will give us a xfs_bstat_t for the > dstdir inode > I've opened or is it already part of stat64_t? it's available via the FS_IOC_FSGETXATTR ioctl: # xfs_io -c "chattr +t" mnt/rtdir # strace -e ioctl xfs_io -c lsattr mnt/rtdir ioctl(3, _IOC(_IOC_READ, 0x58, 0x7c, 0x70), 0x7ffd4cab44c0) = 0 ioctl(3, FS_IOC_FSGETXATTR, 0x7ffd4cab45a0) = 0 -------t-------- mnt/rtdir -Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC][PATCH] xfs_restore: detect rtinherit on destination 2019-06-06 18:39 ` Eric Sandeen @ 2019-06-06 19:50 ` Sheena Artrip 2019-06-06 19:57 ` [PATCH v2] " Sheena Artrip 1 sibling, 0 replies; 14+ messages in thread From: Sheena Artrip @ 2019-06-06 19:50 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs, sheenobu On Thu, Jun 6, 2019 at 11:39 AM Eric Sandeen <sandeen@sandeen.net> wrote: > > On 6/6/19 1:12 PM, Sheena Artrip wrote: > > On Thu, Jun 6, 2019 at 7:11 AM Eric Sandeen <sandeen@sandeen.net> wrote: > >> > >> On 6/5/19 4:16 PM, Sheena Artrip wrote: > >>> When running xfs_restore with a non-rtdev dump, > >>> it will ignore any rtinherit flags on the destination > >>> and send I/O to the metadata region. > >>> > >>> Instead, detect rtinherit on the destination XFS fileystem root inode > >>> and use that to override the incoming inode flags. > >>> > >>> Original version of this patch missed some branches so multiple > >>> invocations of xfsrestore onto the same fs caused > >>> the rtinherit bit to get re-removed. There could be some > >>> additional edge cases in non-realtime to realtime workflows so > >>> the outstanding question would be: is it worth supporting? > >> > >> Hm, interesting. > >> > >> So this is a mechanism to allow dump/restore to migrate everything > >> to the realtime subvol? I can't decide if I like this - normally I'd > >> think of an xfsdump/xfsrestore session as more or less replicating the > >> filesystem that was dumped, and not something that will fundamentally > >> change what was dumped. > >> > >> OTOH, we can restore onto any dir we want, and I could see the argument > >> that we should respect things like the rtinherit flag if that's what > >> the destination dir says. > > > > Yes. What is strange is that an xfsrestore onto a rtdev system will > > silently "fill" > > from a filesystem that previously did not have rt files, I guess? > Exactly, yes. > > the metadata partition until the available inode count goes to zero and we get > > an ENOSPC. Not yet sure if the file data goes straight to the metadata partition > > or if it's simply accounting for it in the metadata partition. > > > > I'm guessing xfsrestore should either fail-fast or allow this via > > rtinherit detection. I don't mind putting it behind a flag either. > > Hm, can you more completely describe the usecase/testcase that leads to > the problem? I just want to make sure I have the whole picture. > The use case is data migration from system A to system B. Traditionally, they've all been non-realtime to non-realtime so xfsdump/xfsrestore worked OK. Trying the same process for non-realtime to realtime brought up these surprising issues. > > >> One thing about the patch - the mechanism you've copied to get the root > >> inode number via bulkstat turns out to be broken ... it's possible > >> to have a non-root inode with the lowest number on the fs, unfortunately. > > > > I think i saw that on the list but this code is also a near-identical > > copy of what is in xfsdump/content.c. > > yeah that's my mistake to fix now ;) > > >> But, wouldn't you want to test the rtinherit flag on the target dir anyway, > >> not necessarily the root dir? > > > > Makes sense. How would I get the rtinherit flag on the target dir? Is there > > a xfs-specific stat function that will give us a xfs_bstat_t for the > > dstdir inode > > I've opened or is it already part of stat64_t? > > it's available via the FS_IOC_FSGETXATTR ioctl: > > # xfs_io -c "chattr +t" mnt/rtdir > # strace -e ioctl xfs_io -c lsattr mnt/rtdir > ioctl(3, _IOC(_IOC_READ, 0x58, 0x7c, 0x70), 0x7ffd4cab44c0) = 0 > ioctl(3, FS_IOC_FSGETXATTR, 0x7ffd4cab45a0) = 0 > -------t-------- mnt/rtdir Thanks! ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-06 18:39 ` Eric Sandeen 2019-06-06 19:50 ` Sheena Artrip @ 2019-06-06 19:57 ` Sheena Artrip 2019-06-06 21:23 ` Eric Sandeen 1 sibling, 1 reply; 14+ messages in thread From: Sheena Artrip @ 2019-06-06 19:57 UTC (permalink / raw) To: Eric Sandeen; +Cc: sheena.artrip, linux-xfs, Sheena Artrip When running xfs_restore with a non-rtdev dump, it will ignore any rtinherit flags on the destination and send I/O to the metadata region. Instead, detect rtinherit on the destination XFS fileystem root inode and use that to override the incoming inode flags. Original version of this patch missed some branches so multiple invocations of xfsrestore onto the same fs caused the rtinherit bit to get re-removed. There could be some additional edge cases in non-realtime to realtime workflows so the outstanding question would be: is it worth supporting? Changes in v2: * Changed root inode bulkstat to just ioctl to the destdir inode Signed-off-by: Sheena Artrip <sheenobu@fb.com> --- restore/content.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/restore/content.c b/restore/content.c index 6b22965..4822d1c 100644 --- a/restore/content.c +++ b/restore/content.c @@ -670,6 +670,9 @@ struct tran { /* to establish critical regions while updating pers * inventory */ + bool_t t_dstisrealtime; + /* to force the realtime flag on incoming inodes + */ }; typedef struct tran tran_t; @@ -1803,6 +1806,37 @@ content_init(int argc, char *argv[], size64_t vmsz) free_handle(fshanp, fshlen); } + /* determine if destination root inode has rtinherit. + * If so, we should force XFS_REALTIME on the incoming inodes. + */ + if (persp->a.dstdirisxfspr) { + struct fsxattr dstxattr; + + int dstfd = open(persp->a.dstdir, O_RDONLY); + if (dstfd < 0) { + mlog(MLOG_NORMAL | MLOG_WARNING, + _("open of %s failed: %s\n"), + persp->a.dstdir, + strerror(errno)); + return BOOL_FALSE; + } + + /* Get the xattr details for the destination folder */ + if (ioctl(dstfd, XFS_IOC_FSGETXATTR, &dstxattr) < 0) { + (void)close(dstfd); + mlog(MLOG_ERROR, + _("failed to get xattr information for dst inode\n")); + return BOOL_FALSE; + } + + (void)close(dstfd); + + /* test against rtinherit */ + if((dstxattr.fsx_xflags & XFS_XFLAG_RTINHERIT) != 0) { + tranp->t_dstisrealtime = true; + } + } + /* map in pers. inv. descriptors, if any. NOTE: this ptr is to be * referenced ONLY via the macros provided; the descriptors will be * occasionally remapped, causing the ptr to change. @@ -7270,6 +7304,10 @@ restore_file_cb(void *cp, bool_t linkpr, char *path1, char *path2) bool_t ahcs = contextp->cb_ahcs; stream_context_t *strctxp = (stream_context_t *)drivep->d_strmcontextp; + if (tranp->t_dstisrealtime) { + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; + } + int rval; bool_t ok; @@ -7480,6 +7518,10 @@ restore_reg(drive_t *drivep, if (tranp->t_toconlypr) return BOOL_TRUE; + if (tranp->t_dstisrealtime) { + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; + } + oflags = O_CREAT | O_RDWR; if (persp->a.dstdirisxfspr && bstatp->bs_xflags & XFS_XFLAG_REALTIME) oflags |= O_DIRECT; @@ -8470,6 +8512,11 @@ restore_extent(filehdr_t *fhdrp, } assert(new_off == off); } + + if (tranp->t_dstisrealtime) { + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; + } + if ((fd != -1) && (bstatp->bs_xflags & XFS_XFLAG_REALTIME)) { if ((ioctl(fd, XFS_IOC_DIOINFO, &da) < 0)) { mlog(MLOG_NORMAL | MLOG_WARNING, _( @@ -8729,6 +8776,10 @@ restore_extattr(drive_t *drivep, assert(extattrbufp); + if (tranp->t_dstisrealtime) { + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; + } + if (!isdirpr) isfilerestored = partial_check(bstatp->bs_ino, bstatp->bs_size); -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-06 19:57 ` [PATCH v2] " Sheena Artrip @ 2019-06-06 21:23 ` Eric Sandeen 2019-06-06 21:50 ` Dave Chinner 0 siblings, 1 reply; 14+ messages in thread From: Eric Sandeen @ 2019-06-06 21:23 UTC (permalink / raw) To: Sheena Artrip; +Cc: sheena.artrip, linux-xfs On 6/6/19 2:57 PM, Sheena Artrip wrote: > When running xfs_restore with a non-rtdev dump, > it will ignore any rtinherit flags on the destination > and send I/O to the metadata region. > > Instead, detect rtinherit on the destination XFS fileystem root inode > and use that to override the incoming inode flags. > > Original version of this patch missed some branches so multiple > invocations of xfsrestore onto the same fs caused > the rtinherit bit to get re-removed. There could be some > additional edge cases in non-realtime to realtime workflows so > the outstanding question would be: is it worth supporting? > > Changes in v2: > * Changed root inode bulkstat to just ioctl to the destdir inode Thanks for that fixup (though comment still says "root" FWIW) Thinking about this some more, I'm really kind of wondering how this should all be expected to work. There are several scenarios here, and "is this file rt?" is prescribed in different ways - either in the dump itself, or on the target fs via inheritance flags... (NB: rt is not the only inheritable flag, so would we need to handle the others?) non-rt fs dump, restored onto non-rt fs - obviously this is fine rt fs dump, restored onto rt fs - obviously this is fine as well rt fs dump, restored onto non-rt fs - this works, with errors - all rt files become non-rt - nothing else to do here other than fail outright non-rt fs dump, restored into rt fs dir/fs with "rtinherit" set - this one is your case - today it's ignored, files stay non-rt - you're suggesting it be honored and files turned into rt the one case that's not handled here is "what if I want to have my realtime dump with realtime files restored onto an rt-capable fs, but turned into regular files?" So your patch gives us one mechanism (restore non-rt files as rt files) but not the converse (restore rt files as non-rt files) - I'm not sure if that matters, but the symmetry bugs me a little. I'm trying to decide if dump/restore is truly the right way to migrate files from non-rt to rt or vice versa, TBH. Maybe dchinner or djwong will have thoughts as well... I'll worry more about the details of the patch after we decide if this is the right behavior in the first place... -Eric > Signed-off-by: Sheena Artrip <sheenobu@fb.com> > --- > restore/content.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/restore/content.c b/restore/content.c > index 6b22965..4822d1c 100644 > --- a/restore/content.c > +++ b/restore/content.c > @@ -670,6 +670,9 @@ struct tran { > /* to establish critical regions while updating pers > * inventory > */ > + bool_t t_dstisrealtime; > + /* to force the realtime flag on incoming inodes > + */ > }; > > typedef struct tran tran_t; > @@ -1803,6 +1806,37 @@ content_init(int argc, char *argv[], size64_t vmsz) > free_handle(fshanp, fshlen); > } > > + /* determine if destination root inode has rtinherit. > + * If so, we should force XFS_REALTIME on the incoming inodes. > + */ > + if (persp->a.dstdirisxfspr) { > + struct fsxattr dstxattr; > + > + int dstfd = open(persp->a.dstdir, O_RDONLY); > + if (dstfd < 0) { > + mlog(MLOG_NORMAL | MLOG_WARNING, > + _("open of %s failed: %s\n"), > + persp->a.dstdir, > + strerror(errno)); > + return BOOL_FALSE; > + } > + > + /* Get the xattr details for the destination folder */ > + if (ioctl(dstfd, XFS_IOC_FSGETXATTR, &dstxattr) < 0) { > + (void)close(dstfd); > + mlog(MLOG_ERROR, > + _("failed to get xattr information for dst inode\n")); > + return BOOL_FALSE; > + } > + > + (void)close(dstfd); > + > + /* test against rtinherit */ > + if((dstxattr.fsx_xflags & XFS_XFLAG_RTINHERIT) != 0) { > + tranp->t_dstisrealtime = true; > + } > + } > + > /* map in pers. inv. descriptors, if any. NOTE: this ptr is to be > * referenced ONLY via the macros provided; the descriptors will be > * occasionally remapped, causing the ptr to change. > @@ -7270,6 +7304,10 @@ restore_file_cb(void *cp, bool_t linkpr, char *path1, char *path2) > bool_t ahcs = contextp->cb_ahcs; > stream_context_t *strctxp = (stream_context_t *)drivep->d_strmcontextp; > > + if (tranp->t_dstisrealtime) { > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > + } > + > int rval; > bool_t ok; > > @@ -7480,6 +7518,10 @@ restore_reg(drive_t *drivep, > if (tranp->t_toconlypr) > return BOOL_TRUE; > > + if (tranp->t_dstisrealtime) { > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > + } > + > oflags = O_CREAT | O_RDWR; > if (persp->a.dstdirisxfspr && bstatp->bs_xflags & XFS_XFLAG_REALTIME) > oflags |= O_DIRECT; > @@ -8470,6 +8512,11 @@ restore_extent(filehdr_t *fhdrp, > } > assert(new_off == off); > } > + > + if (tranp->t_dstisrealtime) { > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > + } > + > if ((fd != -1) && (bstatp->bs_xflags & XFS_XFLAG_REALTIME)) { > if ((ioctl(fd, XFS_IOC_DIOINFO, &da) < 0)) { > mlog(MLOG_NORMAL | MLOG_WARNING, _( > @@ -8729,6 +8776,10 @@ restore_extattr(drive_t *drivep, > > assert(extattrbufp); > > + if (tranp->t_dstisrealtime) { > + bstatp->bs_xflags |= XFS_XFLAG_REALTIME; > + } > + > if (!isdirpr) > isfilerestored = partial_check(bstatp->bs_ino, bstatp->bs_size); > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-06 21:23 ` Eric Sandeen @ 2019-06-06 21:50 ` Dave Chinner 2019-06-06 22:08 ` Eric Sandeen 0 siblings, 1 reply; 14+ messages in thread From: Dave Chinner @ 2019-06-06 21:50 UTC (permalink / raw) To: Eric Sandeen; +Cc: Sheena Artrip, sheena.artrip, linux-xfs On Thu, Jun 06, 2019 at 04:23:51PM -0500, Eric Sandeen wrote: > On 6/6/19 2:57 PM, Sheena Artrip wrote: > > When running xfs_restore with a non-rtdev dump, > > it will ignore any rtinherit flags on the destination > > and send I/O to the metadata region. > > > > Instead, detect rtinherit on the destination XFS fileystem root inode > > and use that to override the incoming inode flags. > > > > Original version of this patch missed some branches so multiple > > invocations of xfsrestore onto the same fs caused > > the rtinherit bit to get re-removed. There could be some > > additional edge cases in non-realtime to realtime workflows so > > the outstanding question would be: is it worth supporting? > > > > Changes in v2: > > * Changed root inode bulkstat to just ioctl to the destdir inode > > Thanks for that fixup (though comment still says "root" FWIW) > > Thinking about this some more, I'm really kind of wondering how this > should all be expected to work. There are several scenarios here, > and "is this file rt?" is prescribed in different ways - either in > the dump itself, or on the target fs via inheritance flags... > > (NB: rt is not the only inheritable flag, so would we need to handle > the others?) > > non-rt fs dump, restored onto non-rt fs > - obviously this is fine > > rt fs dump, restored onto rt fs > - obviously this is fine as well > > rt fs dump, restored onto non-rt fs > - this works, with errors - all rt files become non-rt > - nothing else to do here other than fail outright This should just work, without errors or warnings. > non-rt fs dump, restored into rt fs dir/fs with "rtinherit" set > - this one is your case > - today it's ignored, files stay non-rt > - you're suggesting it be honored and files turned into rt Current filesystem policy should override the policy in dump image as the dump image may contain an invalid policy.... > the one case that's not handled here is "what if I want to have my > realtime dump with realtime files restored onto an rt-capable fs, but > turned into regular files?" Which is where having the kernel policy override the dump file is necesary... > So your patch gives us one mechanism (restore non-rt files as > rt files) but not the converse (restore rt files as non-rt files) - > I'm not sure if that matters, but the symmetry bugs me a little. > > I'm trying to decide if dump/restore is truly the right way to > migrate files from non-rt to rt or vice versa, TBH. Maybe dchinner > or djwong will have thoughts as well... *nod* My take on this is that we need to decide which allocation policy to use - the kernel policy or the dump file policy - in the different situations. It's a simple, easy to document and understand solution. At minimum, if there's a mismatch between rtdev/non-rtdev between dump and restore, then restore should not try to restore or clear rt flags at all. i.e the rt flags in the dump image should be considered invalid in this situation and masked out in the restore process. This prevents errors from being reported during restore, and it does "the right thing" according to how the user has configured the destination directory. i.e. if the destdir has the rtinherit bit set and there's a rtdev present, the kernel policy will cause all file data that is restored to be allocated on the rtdev. Otherwise the kernel will place it (correctly) on the data dev. In the case where both have rtdevs, but you want to restore to ignore the dump file rtdev policy, we really only need to add a CLI option to say "ignore rt flags" and that then allows the kernel policy to dictate how the restored files are placed in the same way that having a rtdev mismatch does. This is simple, consistent, fulfils the requirements and should have no hidden surprises for users.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-06 21:50 ` Dave Chinner @ 2019-06-06 22:08 ` Eric Sandeen 2019-06-06 22:36 ` Dave Chinner 0 siblings, 1 reply; 14+ messages in thread From: Eric Sandeen @ 2019-06-06 22:08 UTC (permalink / raw) To: Dave Chinner; +Cc: Sheena Artrip, sheena.artrip, linux-xfs On 6/6/19 4:50 PM, Dave Chinner wrote: > On Thu, Jun 06, 2019 at 04:23:51PM -0500, Eric Sandeen wrote: >> On 6/6/19 2:57 PM, Sheena Artrip wrote: >>> When running xfs_restore with a non-rtdev dump, >>> it will ignore any rtinherit flags on the destination >>> and send I/O to the metadata region. >>> >>> Instead, detect rtinherit on the destination XFS fileystem root inode >>> and use that to override the incoming inode flags. >>> >>> Original version of this patch missed some branches so multiple >>> invocations of xfsrestore onto the same fs caused >>> the rtinherit bit to get re-removed. There could be some >>> additional edge cases in non-realtime to realtime workflows so >>> the outstanding question would be: is it worth supporting? >>> >>> Changes in v2: >>> * Changed root inode bulkstat to just ioctl to the destdir inode >> >> Thanks for that fixup (though comment still says "root" FWIW) >> >> Thinking about this some more, I'm really kind of wondering how this >> should all be expected to work. There are several scenarios here, >> and "is this file rt?" is prescribed in different ways - either in >> the dump itself, or on the target fs via inheritance flags... >> >> (NB: rt is not the only inheritable flag, so would we need to handle >> the others?) >> >> non-rt fs dump, restored onto non-rt fs >> - obviously this is fine >> >> rt fs dump, restored onto rt fs >> - obviously this is fine as well >> >> rt fs dump, restored onto non-rt fs >> - this works, with errors - all rt files become non-rt >> - nothing else to do here other than fail outright > > This should just work, without errors or warnings. I said errors but I meant warnings: xfsrestore: restoring non-directory files xfsrestore: WARNING: attempt to set extended attributes (xflags 0x80000001, extsize = 0x0, projid = 0x0) of rtdir/bar failed: Invalid argument xfsrestore: WARNING: attempt to set extended attributes (xflags 0x80000001, extsize = 0x0, projid = 0x0) of rtdir/baz failed: Invalid argument xfsrestore: restore complete: 0 seconds elapsed and yeah, probably should not be noisy there. >> non-rt fs dump, restored into rt fs dir/fs with "rtinherit" set >> - this one is your case >> - today it's ignored, files stay non-rt >> - you're suggesting it be honored and files turned into rt > > Current filesystem policy should override the policy in dump image > as the dump image may contain an invalid policy.... > >> the one case that's not handled here is "what if I want to have my >> realtime dump with realtime files restored onto an rt-capable fs, but >> turned into regular files?" > > Which is where having the kernel policy override the dump file is > necesary... The trick is that we don't have a "no-rt" flag we can set on a dir, so there is no "files in this dir ar not rt" policy to follow. >> So your patch gives us one mechanism (restore non-rt files as >> rt files) but not the converse (restore rt files as non-rt files) - >> I'm not sure if that matters, but the symmetry bugs me a little. >> >> I'm trying to decide if dump/restore is truly the right way to >> migrate files from non-rt to rt or vice versa, TBH. Maybe dchinner >> or djwong will have thoughts as well... > > *nod* > > My take on this is that we need to decide which allocation policy to > use - the kernel policy or the dump file policy - in the different > situations. It's a simple, easy to document and understand solution. > > At minimum, if there's a mismatch between rtdev/non-rtdev between > dump and restore, then restore should not try to restore or clear rt > flags at all. i.e the rt flags in the dump image should be > considered invalid in this situation and masked out in the restore > process. This prevents errors from being reported during restore, > and it does "the right thing" according to how the user has > configured the destination directory. i.e. if the destdir has the > rtinherit bit set and there's a rtdev present, the kernel policy > will cause all file data that is restored to be allocated on the > rtdev. Otherwise the kernel will place it (correctly) on the data > dev. > > In the case where both have rtdevs, but you want to restore to > ignore the dump file rtdev policy, we really only need to add a CLI > option to say "ignore rt flags" and that then allows the kernel > policy to dictate how the restored files are placed in the same way > that having a rtdev mismatch does. > > This is simple, consistent, fulfils the requirements and should have > no hidden surprises for users.... Sounds reasonable. So the CLI flag would say "ignore RT info in the dump, and write files according to the destination fs policy?" I think that makes sense. Now: do we need to do the same for all inheritable flags? projid, extsize, etc? I think we probably do. -Eric ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-06 22:08 ` Eric Sandeen @ 2019-06-06 22:36 ` Dave Chinner 2019-06-17 22:09 ` Sheena Artrip 0 siblings, 1 reply; 14+ messages in thread From: Dave Chinner @ 2019-06-06 22:36 UTC (permalink / raw) To: Eric Sandeen; +Cc: Sheena Artrip, sheena.artrip, linux-xfs On Thu, Jun 06, 2019 at 05:08:12PM -0500, Eric Sandeen wrote: > On 6/6/19 4:50 PM, Dave Chinner wrote: > > My take on this is that we need to decide which allocation policy to > > use - the kernel policy or the dump file policy - in the different > > situations. It's a simple, easy to document and understand solution. > > > > At minimum, if there's a mismatch between rtdev/non-rtdev between > > dump and restore, then restore should not try to restore or clear rt > > flags at all. i.e the rt flags in the dump image should be > > considered invalid in this situation and masked out in the restore > > process. This prevents errors from being reported during restore, > > and it does "the right thing" according to how the user has > > configured the destination directory. i.e. if the destdir has the > > rtinherit bit set and there's a rtdev present, the kernel policy > > will cause all file data that is restored to be allocated on the > > rtdev. Otherwise the kernel will place it (correctly) on the data > > dev. > > > > In the case where both have rtdevs, but you want to restore to > > ignore the dump file rtdev policy, we really only need to add a CLI > > option to say "ignore rt flags" and that then allows the kernel > > policy to dictate how the restored files are placed in the same way > > that having a rtdev mismatch does. > > > > This is simple, consistent, fulfils the requirements and should have > > no hidden surprises for users.... > > Sounds reasonable. So the CLI flag would say "ignore RT info in the > dump, and write files according to the destination fs policy?" > I think that makes sense. *nod* > Now: do we need to do the same for all inheritable flags? projid, > extsize, etc? I think we probably do. I disagree. These things are all supported on all destination filesystems, unlike the rtdev. They are also things that can be changed after the fact, unlike rtdev allocation policy. i.e. rtdev has to be set /before/ restore, just about everything else can be set or reset after the fact.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-06 22:36 ` Dave Chinner @ 2019-06-17 22:09 ` Sheena Artrip 2019-06-17 22:55 ` Darrick J. Wong 0 siblings, 1 reply; 14+ messages in thread From: Sheena Artrip @ 2019-06-17 22:09 UTC (permalink / raw) To: Dave Chinner; +Cc: Eric Sandeen, Sheena Artrip, linux-xfs On Thu, Jun 6, 2019 at 3:37 PM Dave Chinner <david@fromorbit.com> wrote: > > On Thu, Jun 06, 2019 at 05:08:12PM -0500, Eric Sandeen wrote: > > On 6/6/19 4:50 PM, Dave Chinner wrote: > > > My take on this is that we need to decide which allocation policy to > > > use - the kernel policy or the dump file policy - in the different > > > situations. It's a simple, easy to document and understand solution. > > > > > > At minimum, if there's a mismatch between rtdev/non-rtdev between > > > dump and restore, then restore should not try to restore or clear rt > > > flags at all. i.e the rt flags in the dump image should be > > > considered invalid in this situation and masked out in the restore > > > process. This prevents errors from being reported during restore, > > > and it does "the right thing" according to how the user has > > > configured the destination directory. i.e. if the destdir has the > > > rtinherit bit set and there's a rtdev present, the kernel policy > > > will cause all file data that is restored to be allocated on the > > > rtdev. Otherwise the kernel will place it (correctly) on the data > > > dev. > > > > > > In the case where both have rtdevs, but you want to restore to > > > ignore the dump file rtdev policy, we really only need to add a CLI > > > option to say "ignore rt flags" and that then allows the kernel > > > policy to dictate how the restored files are placed in the same way > > > that having a rtdev mismatch does. > > > > > > This is simple, consistent, fulfils the requirements and should have > > > no hidden surprises for users.... > > > > Sounds reasonable. So the CLI flag would say "ignore RT info in the > > dump, and write files according to the destination fs policy?" > > I think that makes sense. Any suggested flag name/prefix for this? Last i checked all the single letters were taken up? > *nod* > > > Now: do we need to do the same for all inheritable flags? projid, > > extsize, etc? I think we probably do. > > I disagree. These things are all supported on all destination > filesystems, unlike the rtdev. They are also things that can be > changed after the fact, unlike rtdev allocation policy. i.e. rtdev > has to be set /before/ restore, just about everything else can be > set or reset after the fact.... > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-17 22:09 ` Sheena Artrip @ 2019-06-17 22:55 ` Darrick J. Wong 2019-06-18 6:28 ` Sheena Artrip 0 siblings, 1 reply; 14+ messages in thread From: Darrick J. Wong @ 2019-06-17 22:55 UTC (permalink / raw) To: Sheena Artrip; +Cc: Dave Chinner, Eric Sandeen, Sheena Artrip, linux-xfs On Mon, Jun 17, 2019 at 03:09:15PM -0700, Sheena Artrip wrote: > On Thu, Jun 6, 2019 at 3:37 PM Dave Chinner <david@fromorbit.com> wrote: > > > > On Thu, Jun 06, 2019 at 05:08:12PM -0500, Eric Sandeen wrote: > > > On 6/6/19 4:50 PM, Dave Chinner wrote: > > > > My take on this is that we need to decide which allocation policy to > > > > use - the kernel policy or the dump file policy - in the different > > > > situations. It's a simple, easy to document and understand solution. > > > > > > > > At minimum, if there's a mismatch between rtdev/non-rtdev between > > > > dump and restore, then restore should not try to restore or clear rt > > > > flags at all. i.e the rt flags in the dump image should be > > > > considered invalid in this situation and masked out in the restore > > > > process. This prevents errors from being reported during restore, > > > > and it does "the right thing" according to how the user has > > > > configured the destination directory. i.e. if the destdir has the > > > > rtinherit bit set and there's a rtdev present, the kernel policy > > > > will cause all file data that is restored to be allocated on the > > > > rtdev. Otherwise the kernel will place it (correctly) on the data > > > > dev. > > > > > > > > In the case where both have rtdevs, but you want to restore to > > > > ignore the dump file rtdev policy, we really only need to add a CLI > > > > option to say "ignore rt flags" and that then allows the kernel > > > > policy to dictate how the restored files are placed in the same way > > > > that having a rtdev mismatch does. > > > > > > > > This is simple, consistent, fulfils the requirements and should have > > > > no hidden surprises for users.... > > > > > > Sounds reasonable. So the CLI flag would say "ignore RT info in the > > > dump, and write files according to the destination fs policy?" > > > I think that makes sense. > > Any suggested flag name/prefix for this? Last i checked all the single > letters were taken up? I suggest --preserve-xflags=<same letters as xfs_io lsattr command> --D > > *nod* > > > > > Now: do we need to do the same for all inheritable flags? projid, > > > extsize, etc? I think we probably do. > > > > I disagree. These things are all supported on all destination > > filesystems, unlike the rtdev. They are also things that can be > > changed after the fact, unlike rtdev allocation policy. i.e. rtdev > > has to be set /before/ restore, just about everything else can be > > set or reset after the fact.... > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@fromorbit.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-17 22:55 ` Darrick J. Wong @ 2019-06-18 6:28 ` Sheena Artrip 2019-06-18 15:09 ` Darrick J. Wong 0 siblings, 1 reply; 14+ messages in thread From: Sheena Artrip @ 2019-06-18 6:28 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Dave Chinner, Eric Sandeen, Sheena Artrip, linux-xfs On Mon, Jun 17, 2019 at 3:56 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Mon, Jun 17, 2019 at 03:09:15PM -0700, Sheena Artrip wrote: > > On Thu, Jun 6, 2019 at 3:37 PM Dave Chinner <david@fromorbit.com> wrote: > > > > > > On Thu, Jun 06, 2019 at 05:08:12PM -0500, Eric Sandeen wrote: > > > > On 6/6/19 4:50 PM, Dave Chinner wrote: > > > > > My take on this is that we need to decide which allocation policy to > > > > > use - the kernel policy or the dump file policy - in the different > > > > > situations. It's a simple, easy to document and understand solution. > > > > > > > > > > At minimum, if there's a mismatch between rtdev/non-rtdev between > > > > > dump and restore, then restore should not try to restore or clear rt > > > > > flags at all. i.e the rt flags in the dump image should be > > > > > considered invalid in this situation and masked out in the restore > > > > > process. This prevents errors from being reported during restore, > > > > > and it does "the right thing" according to how the user has > > > > > configured the destination directory. i.e. if the destdir has the > > > > > rtinherit bit set and there's a rtdev present, the kernel policy > > > > > will cause all file data that is restored to be allocated on the > > > > > rtdev. Otherwise the kernel will place it (correctly) on the data > > > > > dev. > > > > > > > > > > In the case where both have rtdevs, but you want to restore to > > > > > ignore the dump file rtdev policy, we really only need to add a CLI > > > > > option to say "ignore rt flags" and that then allows the kernel > > > > > policy to dictate how the restored files are placed in the same way > > > > > that having a rtdev mismatch does. > > > > > > > > > > This is simple, consistent, fulfils the requirements and should have > > > > > no hidden surprises for users.... > > > > > > > > Sounds reasonable. So the CLI flag would say "ignore RT info in the > > > > dump, and write files according to the destination fs policy?" > > > > I think that makes sense. > > > > Any suggested flag name/prefix for this? Last i checked all the single > > letters were taken up? > > I suggest --preserve-xflags=<same letters as xfs_io lsattr command> What's the implication? That we do not copy any xflags bits unless you include them in --preserve-xflags? The defaults of this would be all the available fields. That still leaves the destination needing a xflag bit like realtime and the source not having it...Maybe xfsdump needs --preserve-xflags and xfsrestore needs --apply-xflags ? That will catch all the cases and the solution is just an and/xor on the outgoing/incoming bsp_xflags field: * realtime->realtime (--preserve-xflags=all --apply-xflags=none) * non-realtime->realtime (--preserve-xflags=all --apply-xflags=t) * non-realtime->non-realtime (--preserve-xflags=all --apply-xflags=none) * realtime->non-realtime (--preserve-xflags=all-but-t) Thanks! > --D > > > > *nod* > > > > > > > Now: do we need to do the same for all inheritable flags? projid, > > > > extsize, etc? I think we probably do. > > > > > > I disagree. These things are all supported on all destination > > > filesystems, unlike the rtdev. They are also things that can be > > > changed after the fact, unlike rtdev allocation policy. i.e. rtdev > > > has to be set /before/ restore, just about everything else can be > > > set or reset after the fact.... > > > Cheers, > > > > > > Dave. > > > -- > > > Dave Chinner > > > david@fromorbit.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] xfs_restore: detect rtinherit on destination 2019-06-18 6:28 ` Sheena Artrip @ 2019-06-18 15:09 ` Darrick J. Wong 0 siblings, 0 replies; 14+ messages in thread From: Darrick J. Wong @ 2019-06-18 15:09 UTC (permalink / raw) To: Sheena Artrip; +Cc: Dave Chinner, Eric Sandeen, Sheena Artrip, linux-xfs On Mon, Jun 17, 2019 at 11:28:03PM -0700, Sheena Artrip wrote: > On Mon, Jun 17, 2019 at 3:56 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > On Mon, Jun 17, 2019 at 03:09:15PM -0700, Sheena Artrip wrote: > > > On Thu, Jun 6, 2019 at 3:37 PM Dave Chinner <david@fromorbit.com> wrote: > > > > > > > > On Thu, Jun 06, 2019 at 05:08:12PM -0500, Eric Sandeen wrote: > > > > > On 6/6/19 4:50 PM, Dave Chinner wrote: > > > > > > My take on this is that we need to decide which allocation policy to > > > > > > use - the kernel policy or the dump file policy - in the different > > > > > > situations. It's a simple, easy to document and understand solution. > > > > > > > > > > > > At minimum, if there's a mismatch between rtdev/non-rtdev between > > > > > > dump and restore, then restore should not try to restore or clear rt > > > > > > flags at all. i.e the rt flags in the dump image should be > > > > > > considered invalid in this situation and masked out in the restore > > > > > > process. This prevents errors from being reported during restore, > > > > > > and it does "the right thing" according to how the user has > > > > > > configured the destination directory. i.e. if the destdir has the > > > > > > rtinherit bit set and there's a rtdev present, the kernel policy > > > > > > will cause all file data that is restored to be allocated on the > > > > > > rtdev. Otherwise the kernel will place it (correctly) on the data > > > > > > dev. > > > > > > > > > > > > In the case where both have rtdevs, but you want to restore to > > > > > > ignore the dump file rtdev policy, we really only need to add a CLI > > > > > > option to say "ignore rt flags" and that then allows the kernel > > > > > > policy to dictate how the restored files are placed in the same way > > > > > > that having a rtdev mismatch does. > > > > > > > > > > > > This is simple, consistent, fulfils the requirements and should have > > > > > > no hidden surprises for users.... > > > > > > > > > > Sounds reasonable. So the CLI flag would say "ignore RT info in the > > > > > dump, and write files according to the destination fs policy?" > > > > > I think that makes sense. > > > > > > Any suggested flag name/prefix for this? Last i checked all the single > > > letters were taken up? > > > > I suggest --preserve-xflags=<same letters as xfs_io lsattr command> > > What's the implication? That we do not copy any xflags bits unless you > include them in --preserve-xflags? > The defaults of this would be all the available fields. > > That still leaves the destination needing a xflag bit like realtime > and the source not having it...Maybe > xfsdump needs --preserve-xflags and xfsrestore needs --apply-xflags ? > That will catch > all the cases and the solution is just an and/xor on the > outgoing/incoming bsp_xflags field: > > * realtime->realtime (--preserve-xflags=all --apply-xflags=none) > * non-realtime->realtime (--preserve-xflags=all --apply-xflags=t) > * non-realtime->non-realtime (--preserve-xflags=all --apply-xflags=none) > * realtime->non-realtime (--preserve-xflags=all-but-t) I was clearly confusing, my apologies. :/ What I should've said more explicitly was that xfsdump continues to save all the xflags as it does now, and that we should consider adding a new cli option to xfsrestore to preserve certain xflags from the underlying fs with a new --preserve-xflags= option. Maybe it should be --preserve-dest-xflags to make it clear that the xflags of the destination are overriding the dump. For your "create dump on non-rt fs and restore to rt dev on rt fs" use case, you'd do.... # xfsdump -f /dumpfile /oldfs # mkfs.xfs -r rtdev=/dev/sdX -d rtinherit=1 /dev/sdY # mount /dev/sdY /newfs # xfsrestore -f /dumpfile --preserve-dest-xflags=t /newfs --D > Thanks! > > > --D > > > > > > *nod* > > > > > > > > > Now: do we need to do the same for all inheritable flags? projid, > > > > > extsize, etc? I think we probably do. > > > > > > > > I disagree. These things are all supported on all destination > > > > filesystems, unlike the rtdev. They are also things that can be > > > > changed after the fact, unlike rtdev allocation policy. i.e. rtdev > > > > has to be set /before/ restore, just about everything else can be > > > > set or reset after the fact.... > > > > Cheers, > > > > > > > > Dave. > > > > -- > > > > Dave Chinner > > > > david@fromorbit.com ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-06-18 15:10 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-05 21:16 [RFC][PATCH] xfs_restore: detect rtinherit on destination Sheena Artrip 2019-06-06 14:11 ` Eric Sandeen 2019-06-06 18:12 ` Sheena Artrip 2019-06-06 18:39 ` Eric Sandeen 2019-06-06 19:50 ` Sheena Artrip 2019-06-06 19:57 ` [PATCH v2] " Sheena Artrip 2019-06-06 21:23 ` Eric Sandeen 2019-06-06 21:50 ` Dave Chinner 2019-06-06 22:08 ` Eric Sandeen 2019-06-06 22:36 ` Dave Chinner 2019-06-17 22:09 ` Sheena Artrip 2019-06-17 22:55 ` Darrick J. Wong 2019-06-18 6:28 ` Sheena Artrip 2019-06-18 15:09 ` Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.