| Message ID | 553046DA.7090809@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I think thunderbird ate that patch, sorry. I didn't make it conditional - there's really no reason to not restore the information. I was actually surprised that it didn't restore before this patch. If it looks good I'll resend without the word-wrapping. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dan Merillat posted on Thu, 16 Apr 2015 19:33:46 -0400 as excerpted:
> The inode is already found, use the data and make restore friendlier.
Unless things have changed recently, restore doesn't even restore user/
group ownership, let alone permissions. IOW, atime/mtime are the least
of the problem (particularly if people are running noatime as is
recommended, unless you really need it for some reason).
It simply creates the files it restores as the owner/group it is run as
(normally root), using standard umask rules, I believe.
So if you're going to have it start restoring metadata at all, might as
well have it do ownership/perms too, if it can. Otherwise atime/mtime
are hardly worth bothering with.
That's not a bad idea. In my case it was all owned by the same user (media storage) so the only thing of interest was the timestamps. I can whip up a patch to do that as well. On Thu, Apr 16, 2015 at 9:09 PM, Duncan <1i5t5.duncan@cox.net> wrote: > Dan Merillat posted on Thu, 16 Apr 2015 19:33:46 -0400 as excerpted: > >> The inode is already found, use the data and make restore friendlier. > > Unless things have changed recently, restore doesn't even restore user/ > group ownership, let alone permissions. IOW, atime/mtime are the least > of the problem (particularly if people are running noatime as is > recommended, unless you really need it for some reason). > > It simply creates the files it restores as the owner/group it is run as > (normally root), using standard umask rules, I believe. > > So if you're going to have it start restoring metadata at all, might as > well have it do ownership/perms too, if it can. Otherwise atime/mtime > are hardly worth bothering with. > > -- > Duncan - List replies preferred. No HTML msgs. > "Every nonfree program has a lord, a master -- > and if you use the program, he is your master." Richard Stallman > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 16 Apr 2015 22:19:46 -0400 Dan Merillat <dan.merillat@gmail.com> wrote: [Reordered to standard list quote/reply-in-context order.] > On Thu, Apr 16, 2015 at 9:09 PM, Duncan <1i5t5.duncan@cox.net> wrote: > > Dan Merillat posted on Thu, 16 Apr 2015 19:33:46 -0400 as excerpted: > > > >> The inode is already found, use the data and make restore > >> friendlier. > > > > Unless things have changed recently, restore doesn't even restore > > user/ group ownership, let alone permissions. [...] It simply > > creates the files it restores as the owner/group it is run as > > (normally root), using standard umask rules, I believe. > > > > So if you're going to have it start restoring metadata at all, > > might as well have it do ownership/perms too, if it can. Otherwise > > atime/mtime are hardly worth bothering with. > That's not a bad idea. In my case it was all owned by the same user > (media storage) so the only thing of interest was the timestamps. > > I can whip up a patch to do that as well. That would be much appreciated. =:^) Last time I had to do a restore I had a backup, but it was a bit older than I would have liked. As a result, once I realized ownership/perms metadata wasn't restored, it was simple enough to quick-hack a script to check each restored file against the backup, and where the file existed in both, do a chown and chmod using --reference=$bakfile. That left only the new-since-backup files, which were few enough and obvious enough I could fix them manually. If I'd have not had a backup (if a bit dated), I'd have obviously been even *more* thankful for restore, and probably would have done arbitrary chown/chmod and simply had more manual fixes to do, but I'd have surely been swearing rather more in the process. So patching restore to "just work", restoring all metadata possible (of course it won't always be possible, we /are/ assuming a likely heavily damaged btrfs if we're resorting to restore, so it's not always going to be possible), would be a huge improvement. BTW, symlinks too. At least back then (I think about a year ago, quite some changes since then), restore entirely skipped symlinks. I'm not a coder but I assumed that was because that was entirely metadata, and all it was restoring was data. So I had to manually recreate all my symlinks too. If that could be patched at the same time, I'm sure it'd make a lot of folks happy! =:^) Obviously, if you're resorting to restore, you take what you can get, and I /was/ happy with what I got, if a bit disappointed at the same time, but the more it's possible to get... IMO, getting all that in btrfs restore would surely make for a milestone btrfs-progs release. =:^)
On Fri, Apr 17, 2015 at 7:54 AM, Noah Massey <noah.massey@gmail.com> wrote: > On Thu, Apr 16, 2015 at 7:33 PM, Dan Merillat <dan.merillat@gmail.com> wrote: >> The inode is already found, use the data and make restore friendlier. >> >> Signed-off-by: Dan Merillat <dan.merillat@gmail.com> >> --- >> cmds-restore.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/cmds-restore.c b/cmds-restore.c >> index d2fc951..95ac487 100644 >> --- a/cmds-restore.c >> +++ b/cmds-restore.c >> @@ -567,12 +567,22 @@ static int copy_file(struct btrfs_root *root, int >> fd, struct btrfs_key *key, >> fprintf(stderr, "Ran out of memory\n"); >> return -ENOMEM; >> } >> + struct timespec times[2]; >> + int times_ok=0; >> >> ret = btrfs_lookup_inode(NULL, root, path, key, 0); >> if (ret == 0) { >> inode_item = btrfs_item_ptr(path->nodes[0], path->slots[0], >> struct btrfs_inode_item); >> found_size = btrfs_inode_size(path->nodes[0], inode_item); >> + struct btrfs_timespec bts; >> + read_eb_member(path->nodes[0], inode_item, struct btrfs_inode_item, atime, &bts); >> + times[0].tv_sec=bts.sec; >> + times[0].tv_nsec=bts.nsec; >> + read_eb_member(path->nodes[0], inode_item, struct btrfs_inode_item, atime, &bts); > > I think you mean 'mtime' here I absolutely do, whoops. This is probably a good time to mention how much I dislike the fake pointers being used everywhere, coupled with the partially-implemented macro magic to get fields out of them. Is there a good reason why btrfs_item_ptr isn't just a type-pun, with the understanding that you'll need to copy it to keep it? >> + if (times_ok) >> + futimens(fd, times); > > return value isn't checked here. What could we do with the error if it occurred? Restoring times is a nice bonus if it works, but if it gets lost while the data was restored successfully, that shouldn't be an error condition. I can add a comment to that effect to make it clearer why it's being ignored though, or perhaps something like a warn_once if the filesystem being restored to doesn't support changing the times. On the subject of errors - is it possible for read_eb_member to fail the way I'm using it? It's defined, but never used anywhere else, so I have nothing to compare it to. My feeling is that if btrfs_item_ptr works the data in the structure returned is going to be there, but I'm not sure. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/cmds-restore.c b/cmds-restore.c index d2fc951..95ac487 100644 --- a/cmds-restore.c +++ b/cmds-restore.c @@ -567,12 +567,22 @@ static int copy_file(struct btrfs_root *root, int fd, struct btrfs_key *key, fprintf(stderr, "Ran out of memory\n"); return -ENOMEM; } + struct timespec times[2]; + int times_ok=0; ret = btrfs_lookup_inode(NULL, root, path, key, 0); if (ret == 0) { inode_item = btrfs_item_ptr(path->nodes[0], path->slots[0], struct btrfs_inode_item); found_size = btrfs_inode_size(path->nodes[0], inode_item); + struct btrfs_timespec bts; + read_eb_member(path->nodes[0], inode_item, struct btrfs_inode_item, atime, &bts); + times[0].tv_sec=bts.sec; + times[0].tv_nsec=bts.nsec; + read_eb_member(path->nodes[0], inode_item, struct btrfs_inode_item, atime, &bts); + times[1].tv_sec=bts.sec; + times[1].tv_nsec=bts.nsec; + times_ok=1;
The inode is already found, use the data and make restore friendlier. Signed-off-by: Dan Merillat <dan.merillat@gmail.com> --- cmds-restore.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) } btrfs_release_path(path); @@ -680,6 +690,8 @@ set_size: if (ret) return ret; } + if (times_ok) + futimens(fd, times); return 0; }