Re: [PATCH] btrfs: detect nocow for swap after snapshot delete

[David Sterba <dsterba@suse.cz>]

On Tue, Aug 18, 2020 at 11:00:05AM -0700, Boris Burkov wrote:
> can_nocow_extent and btrfs_cross_ref_exist both rely on a heuristic for
> detecting a must cow condition which is not exactly accurate, but saves
> unnecessary tree traversal. The incorrect assumption is that if the
> extent was created in a generation smaller than the last snapshot
> generation, it must be referenced by that snapshot. That is true, except
> the snapshot could have since been deleted, without affecting the last
> snapshot generation.
> 
> The original patch claimed a performance win from this check, but it
> also leads to a bug where you are unable to use a swapfile if you ever
> snapshotted the subvolume it's in.

Do you mean snapshotted and deleted? As I understand it:

- create fs
- create swapfile in a subvolume
- activate, use, deactivate it
- create snapshot, maybe changing the swapfile
- delete the snapshot, wait until it's gone
- swapfile activation fails, although there are no shared extents and
  the state is effectively the same as if it were created from scratch

> Make the check slower and more strict
> for the swapon case, without modifying the general cow checks as a
> compromise. Turning swap on does not seem to be a particularly
> performance sensitive operation, so incurring a possibly unnecessary
> btrfs_search_slot seems worthwhile for the added usability.

Yeah slowdown should be acceptable here, it's a one time action going
over some metadata.

> Note: Until the snapshot delete transaction is committed,
> check_committed_refs will still cause the logic to think that cow is
> necessary, so the user must sync before swapon.

How often could the snapshot deletion and swapfile activation happen at
the same time? Snapshotting subvolume with the swapfile requires
deactivation, snapshot/send/whatever and then activation. This sounds
like a realistic usecase.