Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid recreation of non-existing btrfs subvolumes (fix #1496) #1499

Merged

Conversation

OliverO2
Copy link
Contributor

This PR makes sure that a btrfs subvolume actually exists before trying to recreate it.

See #1496.

@jsmeix
Copy link
Member

jsmeix commented Sep 18, 2017

@OliverO2
from plain looking at the code everything looks perfect!

I only wonder about the reason behind
why you added a new lib/btrfs-functions.sh script
and did not add your btrfs_subvolume_exists function
to the existing lib/filesystems-functions.sh script?

@jsmeix jsmeix self-assigned this Sep 18, 2017
@jsmeix jsmeix added the enhancement Adaptions and new features label Sep 18, 2017
@jsmeix jsmeix added this to the ReaR v2.3 milestone Sep 18, 2017
@OliverO2
Copy link
Contributor Author

@jsmeix

Yes, I had considered filesystems-functions.sh but somehow decided against it. I agree that the content of btrfs-functions.sh could just go there. Would be simpler and more logical.

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks fine now.

@jsmeix
Copy link
Member

jsmeix commented Sep 18, 2017

If there are no objections I will merge it soon...

@OliverO2
Copy link
Contributor Author

@jsmeix
While we're dealing with btrfs code, would you mind stripping the #btrfssnapshotsubvol comments from disklayout.conf?

Over here, disklayout.conf contains 268 lines, 213 of them (86%) are #btrfssnapshotsubvol comments. That makes it difficult to review and edit the file, especially if done via a console with limited screen real estate.

What about just putting just a comment about the number of snapshot volumes encountered in disklayout.conf and put the snapshot paths in the log file only?

@jsmeix jsmeix merged commit a02ee7f into rear:master Sep 19, 2017
@jsmeix
Copy link
Member

jsmeix commented Sep 19, 2017

@OliverO2
for SUSE I have the btrfs snapshot subvolumes intentionally
listed in disklayout.conf but disabled from recreation.
At SUSE we can boot from a btrfs snapshot subvolume
when it was made by the SUSE tool 'snapper' - i.e. each
snapper btrfs snapshot subvolume could be used as
the subvolume that gets mounted at the '/' mountpoint.
Therefore I would like to have them listed in disklayout.conf.

In your case according to your valuable explanation in
#1496 (comment)
"We snapshot each volume every 5 minutes"
you get tons of btrfs snapshot subvolumes which fill up
the disklayout.conf file with mostly useless information.

I suggest to provide another config variable so that the
user can specify if he wants to have btrfs snapshot subvolumes
listed in disklayout.conf
or
your existing support for EXCLUDE_RECREATE for btrfs
could be somehow enhanced to be also used to get btrfs
snapshot subvolumes completely ignored
e.g. via something like

EXCLUDE_RECREATE=( "${EXCLUDE_RECREATE[@]}" "btrfssnapshotsubvol:all" )

where a special qualifier like 'all' could mean that all btrfs
snapshot subvolumes get completely ignored
or
whatever else you like.

I only like that by default btrfs snapshot subvolumes
are still listed (but disabled) in disklayout.conf.

@jsmeix
Copy link
Member

jsmeix commented Sep 19, 2017

I have tested that a ReaR recovery still works
with the SUSE SLES12-SP2 default btrfs structure.

@OliverO2 OliverO2 deleted the fix/btrfsmountsubvolume-bogus-docker-mount branch November 8, 2017 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants