Add overlay driver reflink support detection

[ngopalak-redhat]

We are working with a customer who requires dedup but not sure if the filesystem that's used supports it. In the tests we observed that calling dedup will first process the images directory and then on first attempt if it fails will throw an error. This is an unnecessary delay each time we start cri-o/containerd.

The alternative here is the do this check in the caller, but that required understanding of the filesystem. The fileutils available here in the storage layer needs to be reused is the alternative.

The best solution is to ask this storage layer for ref-links support. If available we can tell the customer if the dedup will be supported.

If we plan to include deduping during system start-up, this function helps determine if it will work.

Note on AI usage:

• I wrote the source code for storage/drivers/overlay/reflink_check_linux.go
• I have used AI for unit tests.
• Manually tested with unit tests run on a linux system.

Implementation:
Add checkSupportsReflinks() function to detect if the filesystem supports reflink/CoW file cloning via FICLONE ioctl. This allows storage consumers (CRI-O, containerd) to inform users whether deduplication will work on their filesystem.

The detection result is cached and exposed via the Status() output as "Supports reflinks: true/false". Reflinks are supported on Btrfs, XFS (with reflink=1), and OCFS2.

cc: @haircommander @asahay19

[Luap99]

We are working with a customer who requires dedup but not sure if the filesystem that's used supports it. In the tests we observed that calling dedup will first process the images directory and then on first attempt if it fails will throw an error. This is an unnecessary delay each time we start cri-o/containerd.

The alternative here is the do this check in the caller, but that required understanding of the filesystem. The fileutils available here in the storage layer needs to be reused is the alternative.

The approach taken feels backwards, why should the caller know this and must call and parse a type unsafe field from status?

If the goal is to stop the directory walk move the supports reflink check into Dedup() as first thing it does which needs no extra awareness of callers.

Also overall this is not limited to overlayfs, if we would add such a check it needs to be added to all drivers that support this debug which also includes vfs

cc @giuseppe @mtrmac

[Luap99]

unless I am missing something this is being called in Status without the store lock held which means these field assignments are not concurrent safe and can cause a panic if status is being called concurrently for the first time.

[ngopalak-redhat]

We are working with a customer who requires dedup but not sure if the filesystem that's used supports it. In the tests we observed that calling dedup will first process the images directory and then on first attempt if it fails will throw an error. This is an unnecessary delay each time we start cri-o/containerd.

The alternative here is the do this check in the caller, but that required understanding of the filesystem. The fileutils available here in the storage layer needs to be reused is the alternative.

The approach taken feels backwards, why should the caller know this and must call and parse a type unsafe field from status?

If the goal is to stop the directory walk move the supports reflink check into Dedup() as first thing it does which needs no extra awareness of callers.

Also overall this is not limited to overlayfs, if we would add such a check it needs to be added to all drivers that support this debug which also includes vfs

cc @giuseppe @mtrmac

@Luap99 Thanks for the review, At layer above container-d/cri-o we'd like to know if the dedupping is possible. If yes, we'd like the customer to choose if they need it or not. Moving the check into the Dedup() itself will mean that we attempted the Dedup without a user consent.
Without this functionality, the user might choose to dedup without the filesystem supporting it. Happy to fix the other comment you have given. But do you agree that we need this functionality. Happy to explore other alternatives too.

[giuseppe]

why can't you just call Dedup() and ignore the error when the operation is not supported?

The caller can cache that result.

If we don't report a proper error in this case, then we need to fix it

[ngopalak-redhat]

why can't you just call Dedup() and ignore the error when the operation is not supported?

The caller can cache that result.

If we don't report a proper error in this case, then we need to fix it

Thanks @giuseppe , I'm actually caught between two ends :)

• Customer needs to know if they can dedup or not prior to choosing that option
• In container storage layer there's no way to tell until the real dedup is attempted

The only blocker I see is the filesystem support for Dedupping. It better to tell the customer if its possible to dedup by checking the ref links support. Hence this PR :)

[giuseppe]

• Customer needs to know if they can dedup or not prior to choosing that option

why? Why wouldn't they enable it in any case and make sure we ignore the error when it is not supported by the underlying filesystem?

They know where the overlay file system is located, so that can be checked directly on the file system with a simple cp --reflink=always test

[ngopalak-redhat]

• Customer needs to know if they can dedup or not prior to choosing that option

why? Why wouldn't they enable it in any case and make sure we ignore the error when it is not supported by the underlying filesystem?

They know where the overlay file system is located, so that can be checked directly on the file system with a simple cp --reflink=always test

The Dedup operation takes several minutes for 100 GB of images. So the user needs to plan it. The cp --reflink=always approach would require exposing the directory names all the way up and making sure that the permissions for write and correctly handled. The test file should also be cleaned up. Eaiser to have one layer do it, that is the container storage.

[giuseppe]

So the issue is that Dedup is slow when it can't deduplicate. Maybe we can add a check before doing any other work and give up if the file system has no reflinks

[ngopalak-redhat]

So the issue is that Dedup is slow when it can't deduplicate. Maybe we can add a check before doing any other work and give up if the file system has no reflinks

@giuseppe Not exactly the issue. Dedup does happen. It does take time and works. All that we want is to tell the user is to run it while in maintenance and not while workloads are running in parallel using those files. This PR is a check to make sure if the the user (or the directory that's involved) is eligible for dedupping or not.

[giuseppe]

that part is clear, we are trying to figure out if it is better to expose this information to the caller, or just call Dedup() that also checks if it is possible.

What is not clear is why you need to know this before you call Dedup()? If the intention is to call Dedup(), just call it; otherwise don't. What am I missing?

[Luap99]

There are two issues here discussed, first the PR description says the walking is expensive so the obvious fix is move that check at the start of dedup().

Second, they want to know if the fs supports reflink or not and expose it to the end user. Exposing this in status can be done for this but then it should be done consistently for all graph drivers.
With that crio or whatever UI you have can tell the user dedup is not supported on the backing fs.

IMO the simple thing is to do both.

Note however currently this feature check uses a different ioctl from the dedup code FIDEDUPERANGE vs FICLONE so if a fs supports one but the other this could cause incorrect results as well? Not sure if this is a concern in practise.

[mtrmac]

At layer above container-d/cri-o we'd like to know if the dedupping is possible. If yes, we'd like the customer to choose if they need it or not.

Intuitively, that looks like way too much manual control.

By its nature, in general, deduplication can never guarantee savings; in a general deployment, users must have storage capacity to fit almost everything without deduplication (e.g. the scheduler can happen to schedule jobs so that duplicates don’t land on the same node). And at that point, if the node does have enough storage… why bother with the cost of deduplication? It’s not unthinkable but the cost/benefit balance feels iffy.

OTOH, if the user strictly relies on deduplication to fit a carefully managed workload — well then, the user better have the system designed to make it possible, and the user should expect and want nonrecoverable error messages if deduplication fails.

[mtrmac]

The status field is targeted at human readers and is not an appropriate mechanism for API consumers. (E.g., purely hypothetically, it might be localized!)

(Also, well, the containers-storage tool is rarely given to users as a supported binary.)

Throughout the Go codebase, this needs to be an explicit API (DedupPossibleInPrinciple?) which returns a boolean. And then we can talk about whether / how it is consumable from shell scripts.

[ngopalak-redhat]

@mtrmac @giuseppe @Luap99 Thanks for the review. Here’s how I'm planning to refactor this based on your comments:

• Fail-fast on Dedup if reflinks are not supported. This will move the code in checkSupportsReflinks in the current PR into the Dedup function.
• Update graphdriver.DedupResult to include an Error field.

This ensures the caller will know if an error occurred. While testing with tmpfs today (where reflinks aren't supported), I realized that no error is currently thrown or logged to indicate a lack of support. Right now, if the space saved is zero, it could either mean deduplication wasn't useful or reflinks aren't supported. It's better to have this explicit distinction. I know this slightly changes external behavior, but it won't impact users who only rely on the Deduped field in DedupResult.

[mtrmac]

@mtrmac @giuseppe @Luap99 Thanks for the review. Here’s how I'm planning to refactor this based on your comments:

• Fail-fast on Dedup if reflinks are not supported. This will move the code in checkSupportsReflinks in the current PR into the Dedup function.

I’m at this point not sure whether this can be generic in the Dedup code or whether it should be a per-driver implementation. Ultimately there is probably a clear right answer to that design question.

• Update graphdriver.DedupResult to include an Error field.

.Dedup() can already return errors, I don’t see why that is necessary. (It’s possible I’m missing something.)

This ensures the caller will know if an error occurred. While testing with tmpfs today (where reflinks aren't supported), I realized that no error is currently thrown or logged to indicate a lack of support. Right now, if the space saved is zero, it could either mean deduplication wasn't useful or reflinks aren't supported. It's better to have this explicit distinction. I know this slightly changes external behavior, but it won't impact users who only rely on the Deduped field in DedupResult.

Callers can opt into the “fail if this can’t possibly save space” behavior; I think that’s somewhat preferable to returning errors in a previously-working situation.

---

(I still tend to think “Intuitively, that looks like way too much manual control.” and I’m not very convinced all of this is worth doing. That’s not a clear blocker at this point, but it is a discussion we might still have.)

[mtrmac]

Note #980, asking for deduplication to work … somehow … with separate container and image layer stores.

That could make the “does the filesystem support reflinks” question much more complicated.

I suppose we’d want two separate deduplication runs across the two separate directories / mounts?!