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

Non-resident COW files #273

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dakotarwilliams
Copy link
Contributor

Cow files can now exist off of the disk it is cow-ing. This comes with a
few limitations:

  1. You cannot unmount a device with an active cow file on it. You would
    have to destroy or unmount the device being snapshotted by that cow
    file. DattoBD will hold the file handle to the cow file and will
    prevent the unmount.
  2. Don't create loops with the snapshotted devices. If device A's cow
    file exists on device B, and device B's cow file is on device A, then
    you wouldn't be able to unmount either device or shutdown the
    machine. You'd have to destroy either snapshot to be able to unmount.

This commit also adds some fields to the /proc/datto-info device. The
resident field is a 0/1 false/true value that answers the question of
whether the cow file is on the device it is tracking. The
full_cow_path field represents the full path to the cow device, rather
than cow_file's relative-to-mountpoint path. You are encouraged to use
the new full_cow_path field; cow_file will be kept, but deprecated,
for backwards compatibility.

Both of these limitations intend to be addressed in the future.

Cow files can now exist off of the disk it is cow-ing. This comes with a
few limitations:
1. You cannot unmount a device with an active cow file on it. You would
   have to destroy or unmount the device being snapshotted by that cow
   file. DattoBD will hold the file handle to the cow file and will
   prevent the unmount.
3. Don't create loops with the snapshotted devices. If device A's cow
   file exists on device B, and device B's cow file is on device A, then
   you wouldn't be able to unmount either device or shutdown the
   machine. You'd have to destroy either snapshot to be able to unmount.

This commit also adds some fields to the /proc/datto-info device. The
`resident` field is a 0/1 false/true value that answers the question of
whether the cow file is on the device it is tracking. The
`full_cow_path` field represents the full path to the cow device, rather
than `cow_file`'s relative-to-mountpoint path. You are encouraged to use
the new `full_cow_path` field; `cow_file` will be kept, but deprecated,
for backwards compatibility.
FUSE mounts cannot have resident cow files. FUSE mounts the device
first, then sets up its ioctl callbacks (open, close, read, write).
This becomes a problem on a remount, since we'd try to reopen the cow
file during the mount syscall, which will hang since open hasn't been
set by FUSE yet.
Copy link
Contributor

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@@ -31,7 +35,8 @@ struct snap_device {
struct block_device *sd_base_dev; // device being snapshot
char *sd_bdev_path; // base device file path
struct cow_manager *sd_cow; // cow manager
char *sd_cow_path; // cow file path
char *sd_cow_path; // cow file path (for resident cow files)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be marked as deprecated in a comment?

src/tracer.c Outdated
@@ -1608,13 +1624,13 @@ void __tracer_dormant_to_active(struct snap_device *dev,
set_bit(ACTIVE, &dev->sd_state);
clear_bit(UNVERIFIED, &dev->sd_state);

kfree(cow_path);
kfree(resident_cow_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be freed immediately after __tracer_setup_cow_reopen() and then the extra test/free in the error: case could be removed?

if (0 == strncmp(bdev->bd_super->s_type->name, fuseblk_name,
FUSEBLK_MNT_LEN)) {
ret = -EDEADLOCK;
LOG_ERROR(ret, "Cow file cannot be on same volume for"
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to ignore but as a user of this driver would it be helpful to know slightly more about why the cow file cannot be on the same block device without having to consult the code? The comment above is great. I'm just thinking about having a bit more information so that I could at least google an answer to why dattobd is telling me I cannot do what I want to do. We should probably consider adding enough information in our log messages so that steps to correct are at least Googleable. As I said, feel free to ignore, I'm just thinking about the experience of trying to figure out why something, that seems on the surface to be sane, does not work.

@nickchen-cpu
Copy link

nickchen-cpu commented May 23, 2022

Hi,
Thanks for implementing this feature.
After adding this feature, we could implement the COW file dynamic extending feature.
Because there's no need to be worried that COW file's extending data would overlay the deleted data area without protection.

[Discuss]
#245

@dakotarwilliams
Copy link
Contributor Author

@nickchen-cpu You're correct in that we wouldn't need to worry about the deleted data on a separate disk, but we'd run into a similar problem as my second limitation in the first comment. The issue would show up if the device you put the cow file on was snapshotted afterwards. It depends if we were to disallow this behavior entirely and tbh I don't think we would, so we're back the same unused block problem as before.

I've found some problems in this MRs implementation at the moment and am still working to resolve them. Going forward, I will keep your feature in mind.

@koolkhel
Copy link

Does seem work for me. Would love to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants