[PATCH] btrfs-progs: have restore set atime/mtime

[PATCH] btrfs-progs: have restore set atime/mtime

[Dan Merillat]

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);
+		times[1].tv_sec=bts.sec;
+		times[1].tv_nsec=bts.nsec;
+		times_ok=1;
 	}
 	btrfs_release_path(path);

@@ -680,6 +690,8 @@ set_size:
 		if (ret)
 			return ret;
 	}
+	if (times_ok)
+		futimens(fd, times);
 	return 0;
 }

-- 
2.1.4

Re: [PATCH] btrfs-progs: have restore set atime/mtime

[Dan Merillat]

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.

Re: [PATCH] btrfs-progs: have restore set atime/mtime

[Duncan]

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

Re: [PATCH] btrfs-progs: have restore set atime/mtime

[Dan Merillat]

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

Re: [PATCH] btrfs-progs: have restore set atime/mtime

[Duncan]

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. =:^)

-- 
Duncan - No HTML messages please; they are filtered as spam.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [PATCH] btrfs-progs: have restore set atime/mtime

[Dan Merillat]

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.