Btrfs: a few more cleanups and fixes for clone/deduplication

From: Filipe Manana <[email protected]>

Move the check that verifies if both inodes have checksums disabled or both have them enabled, from the clone and deduplication functions into the new common helper btrfs_remap_file_range_prep().

Signed-off-by Filipe Manana <[email protected]>

From: Filipe Manana <[email protected]>

There is no reason why this check was performed for clone operations but not for deduplication operations, after all deduplication is a special case of cloning. So make the check happen for deduplication as well.

This check used to be done and got removed by accident in commit 2b3909f8a7fe9 ("btrfs: use new dedupe data function pointer").

Fixes 2b3909f8a7fe9 ("btrfs: use new dedupe data function pointer").

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/ioctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

fs/btrfs/ioctl.c +0 -0

index 1e90d10b5638..ffe940ceb80a 100644
@@ -3912,12 +3912,12 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 
 		if (btrfs_root_readonly(root_out))

Quoted 22 lines · On Wed, Dec 12, 2018 at 06:05:57PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

There is no reason why this check was performed for clone operations but not for deduplication operations, after all deduplication is a special case of cloning. So make the check happen for deduplication as well.

This check used to be done and got removed by accident in commit 2b3909f8a7fe9 ("btrfs: use new dedupe data function pointer").

Fixes 2b3909f8a7fe9 ("btrfs: use new dedupe data function pointer").

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/ioctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

fs/btrfs/ioctl.c +0 -0

index 1e90d10b5638..ffe940ceb80a 100644
@@ -3912,12 +3912,12 @@ static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 
 		if (btrfs_root_readonly(root_out))

fs/btrfs/ioctl.c static int btrfs_remap_file_range_prep(struct file *file_in, loff_t pos_in, · line 3919

Quoted 10 lines

fs/btrfs/ioctl.c +3 -4

 			return -EROFS;
-
-		if (file_in->f_path.mnt != file_out->f_path.mnt ||
-		    inode_in->i_sb != inode_out->i_sb)
-			return -EXDEV;
 	}
 
+	if (file_in->f_path.mnt != file_out->f_path.mnt ||
+	    inode_in->i_sb != inode_out->i_sb)
+		return -EXDEV;

I'm checking if this is or is not a change in the dedupe behaviour regarding dedupe on different mont points.

I can't see the f_path.mnt check anywhere in the VFS calls so after that patch deduping stops to work in some cases.

From: Filipe Manana <[email protected]>

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Quoted 25 lines · Filipe Manana

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

Quoted 6 lines

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Only if you want your incremental send chain to break on the way
past...

I think it's fairly clear by now (particularly from the last thread
on this a couple of weeks ago) that making RO subvols RW and then back again is a fast way to broken incremental receives.

Quoted 33 lines · Hugo Mills

Quoted 25 lines · Filipe Manana

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

Quoted 6 lines

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Quoted 6 lines

Only if you want your incremental send chain to break on the way
past...

I think it's fairly clear by now (particularly from the last thread
on this a couple of weeks ago) that making RO subvols RW and then back again is a fast way to broken incremental receives.

So, I think the way it should be fixed is to keep deduplication off the read-only subvolumes. The main reason I see is to avoid the random problems that arise from send + dedupe interaction. The cost is lower deduplication ratio.

The main usecase being a primary subvolume with RO snapshots over time, with optional incremental send. That I know is common and widely used.

A problematic usecase that would utilize deduplication over RO snapshots does could be something like a set of subvolumes that have very similar data, get snapshotted or set RO, followed by a dedupe pass.

To support the latter I'd go only via the RO->RW, dedupe, RW->RO way, as this records that there was a change and does not collide with send assumptions. That this leads to loss of incremental send must be communicated to the user with possible override, eg. --I-know-what-I-am-doing option.

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Thanks.

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Thanks.

Quoted 14 lines · Filipe Manana

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Quoted 6 lines

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem bees ran into. There is a workaround in bees (called --workaround-btrfs-send) that avoids RO subvols as dedupe targets. As the name of the option implies, it works around problems in btrfs send.

This kernel change makes the workaround mandatory now, as the default case (without workaround) will fail on every RO subvol even if that behavior is desired by the user. That breaks an important use case on the receiving side of sends--to dedupe the received subvols together while also protecting them against modification on the target system with the RO flag--and preserving that use case is why the send workaround was optional (and not default) in bees.

bees also won't handle the RO/RW/RO transition correctly, as it didn't seem like a sane thing to support at the time. That is arguably something to be fixed in bees.

Quoted 3 lines

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

Is there a reason why incremental send can't simply be fixed?

As far
as I can tell, send is failing because of a runtime check that seems to be too strict; however, I haven't tried removing that check to see if it fixes the problem in send, or just hides the next problem.

More details at:

        https://github.com/Zygo/bees/issues/79#issuecomment-429039036

Quoted 8 lines

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Thanks.

Quoted 39 lines · Zygo Blaxell

Quoted 14 lines · Filipe Manana

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Quoted 6 lines

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem bees ran into. There is a workaround in bees (called --workaround-btrfs-send) that avoids RO subvols as dedupe targets. As the name of the option implies, it works around problems in btrfs send.

This kernel change makes the workaround mandatory now, as the default case (without workaround) will fail on every RO subvol even if that behavior is desired by the user. That breaks an important use case on the receiving side of sends--to dedupe the received subvols together while also protecting them against modification on the target system with the RO flag--and preserving that use case is why the send workaround was optional (and not default) in bees.

bees also won't handle the RO/RW/RO transition correctly, as it didn't seem like a sane thing to support at the time. That is arguably something to be fixed in bees.

Quoted 5 lines

Quoted 3 lines

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

Is there a reason why incremental send can't simply be fixed?

This is a problem that affects both incremental and non-incremental (full) send.

Quoted 4 lines

As far
as I can tell, send is failing because of a runtime check that seems to be too strict; however, I haven't tried removing that check to see if it fixes the problem in send, or just hides the next problem.

The problem is send was designed with the idea that read-only roots don't ever change.

The failures that can happen are many and unpredictable, from occasionally failing with some error, to invalid memory accesses, use after free problems, etc.
Essentially all caused by races when the nodes/leafs from the read-only tree change while send is running.

I don't know what runtime check you are mentioning that is too strict.

You can definitely do dedupe on a read-only root and after it finishes do a send (either full or incremental), and it will work.

Quoted 12 lines

More details at:

        https://github.com/Zygo/bees/issues/79#issuecomment-429039036

Quoted 8 lines

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Thanks.

Quoted 54 lines · Filipe Manana

Quoted 39 lines · Zygo Blaxell

Quoted 14 lines · Filipe Manana

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Quoted 6 lines

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem bees ran into. There is a workaround in bees (called --workaround-btrfs-send) that avoids RO subvols as dedupe targets. As the name of the option implies, it works around problems in btrfs send.

This kernel change makes the workaround mandatory now, as the default case (without workaround) will fail on every RO subvol even if that behavior is desired by the user. That breaks an important use case on the receiving side of sends--to dedupe the received subvols together while also protecting them against modification on the target system with the RO flag--and preserving that use case is why the send workaround was optional (and not default) in bees.

bees also won't handle the RO/RW/RO transition correctly, as it didn't seem like a sane thing to support at the time. That is arguably something to be fixed in bees.

Quoted 5 lines

Quoted 3 lines

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

Is there a reason why incremental send can't simply be fixed?

This is a problem that affects both incremental and non-incremental (full) send.

Quoted 4 lines

As far
as I can tell, send is failing because of a runtime check that seems to be too strict; however, I haven't tried removing that check to see if it fixes the problem in send, or just hides the next problem.

Quoted 2 lines

The problem is send was designed with the idea that read-only roots don't ever change.

...except when they're snapshotted, balanced, or deduped (to list places where that assumption hasn't held so far).

Quoted 6 lines

The failures that can happen are many and unpredictable, from occasionally failing with some error, to invalid memory accesses, use after free problems, etc.
Essentially all caused by races when the nodes/leafs from the read-only tree change while send is running.

Maybe you can explain this further? As far as I can tell, all of those are send bugs that should just be (and over the years have been) fixed.

Quoted 2 lines

I don't know what runtime check you are mentioning that is too strict.

It's this one, in send.c:

                        /*
                         * We may have found an extent item that has changed
                         * only its disk_bytenr field and the corresponding
                         * inode item was not updated. This case happens due to
                         * very specific timings during relocation when a leaf
                         * that contains file extent items is COWed while
                         * relocation is ongoing and its in the stage where it
                         * updates data pointers. So when this happens we can
                         * safely ignore it since we know it's the same extent,
                         * but just at different logical and physical locations
                         * (when an extent is fully replaced with a new one, we
                         * know the generation number must have changed too,
                         * since snapshot creation implies committing the current
                         * transaction, and the inode item must have been updated
                         * as well).
                         * This replacement of the disk_bytenr happens at
                         * relocation.c:replace_file_extents() through
                         * relocation.c:btrfs_reloc_cow_block().
                         */
                        if (btrfs_file_extent_generation(leaf_l, ei_l) ==
                            btrfs_file_extent_generation(leaf_r, ei_r) &&
                            btrfs_file_extent_ram_bytes(leaf_l, ei_l) ==
                            btrfs_file_extent_ram_bytes(leaf_r, ei_r) &&
                            btrfs_file_extent_compression(leaf_l, ei_l) ==
                            btrfs_file_extent_compression(leaf_r, ei_r) &&
                            btrfs_file_extent_encryption(leaf_l, ei_l) ==
                            btrfs_file_extent_encryption(leaf_r, ei_r) &&
                            btrfs_file_extent_other_encoding(leaf_l, ei_l) ==
                            btrfs_file_extent_other_encoding(leaf_r, ei_r) &&
                            btrfs_file_extent_type(leaf_l, ei_l) ==
                            btrfs_file_extent_type(leaf_r, ei_r) &&
                            btrfs_file_extent_disk_bytenr(leaf_l, ei_l) !=
                            btrfs_file_extent_disk_bytenr(leaf_r, ei_r) &&
                            btrfs_file_extent_disk_num_bytes(leaf_l, ei_l) ==
                            btrfs_file_extent_disk_num_bytes(leaf_r, ei_r) &&
                            btrfs_file_extent_offset(leaf_l, ei_l) ==
                            btrfs_file_extent_offset(leaf_r, ei_r) &&
                            btrfs_file_extent_num_bytes(leaf_l, ei_l) ==
                            btrfs_file_extent_num_bytes(leaf_r, ei_r))
                                return 0;
                }

                inconsistent_snapshot_error(sctx, result, "extent");
                return -EIO;

This is the point where bees users report send failures.

I don't really understand what that code is trying to achieve, though. If we are diffing two subvol trees then we should just send anything that's different--even if we occasionally send a few redundant extents because we happen to be sending during a balance, because that's better than bombing out completely.

Or is the problem more like we lose our ei pointer in one of the subvols when the extent tree changes under it? That would be harder to solve, it would have to keep releasing and reacquiring everything.

Quoted 2 lines

You can definitely do dedupe on a read-only root and after it finishes do a send (either full or incremental), and it will work.

If there's a way to make a subvol RW temporarily _without breaking incremental send from that subvol_ (i.e. without clearing the parent UUIDs, maybe also allowing only dedupe) then I have no objection.

Quoted 12 lines

Quoted 12 lines

More details at:

        https://github.com/Zygo/bees/issues/79#issuecomment-429039036

Quoted 8 lines

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Thanks.

So not doing nothing for now, that is, not applying this patch to disable dedupe on RO roots and wait for a better solution (best case, for 5.2 merge window), is reasonable
and not something I'm against.
David, would you consider at least excluding it from 5.1 to allow for a different solution to pop up for another merge window?

Quoted 39 lines · Zygo Blaxell

Quoted 14 lines · Filipe Manana

Quoted 4 lines · David Sterba

Quoted 2 lines · On Wed, Dec 12, 2018 at 06:05:58PM +0000, [email protected] wrote:

From: Filipe Manana <[email protected]>

Quoted 8 lines

Quoted 5 lines

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

And this is also change in user-visible behaviour of dedupe, so this needs to be verified if it's not breaking existing tools.

Quoted 6 lines

Have you had the chance to do such verification?

This actually conflicts with send. Send does not expect a root/tree to change, and with dedupe on read-only roots happening
in parallel with send is going to cause all sorts of unexpected and undesired problems...

This is a problem bees ran into. There is a workaround in bees (called --workaround-btrfs-send) that avoids RO subvols as dedupe targets. As the name of the option implies, it works around problems in btrfs send.

This kernel change makes the workaround mandatory now, as the default case (without workaround) will fail on every RO subvol even if that behavior is desired by the user. That breaks an important use case on the receiving side of sends--to dedupe the received subvols together while also protecting them against modification on the target system with the RO flag--and preserving that use case is why the send workaround was optional (and not default) in bees.

bees also won't handle the RO/RW/RO transition correctly, as it didn't seem like a sane thing to support at the time. That is arguably something to be fixed in bees.

Quoted 5 lines

Quoted 3 lines

This is a problem introduced by dedupe ioctl when it landed, since send existed for a longer time (when nothing else was allowed to change read-only roots, including defrag).

Is there a reason why incremental send can't simply be fixed?

This is a problem that affects both incremental and non-incremental (full) send.

Quoted 4 lines

As far
as I can tell, send is failing because of a runtime check that seems to be too strict; however, I haven't tried removing that check to see if it fixes the problem in send, or just hides the next problem.

The problem is send was designed with the idea that read-only roots don't ever change.

The failures that can happen are many and unpredictable, from occasionally failing with some error, to invalid memory accesses, use after free problems, etc.
Essentially all caused by races when the nodes/leafs from the read-only tree change while send is running.

I don't know what runtime check you are mentioning that is too strict. You can definitely do dedupe on a read-only root and after it finishes do a send (either full or incremental), and it will work.

Quoted 13 lines

More details at:

        https://github.com/Zygo/bees/issues/79#issuecomment-429039036

Quoted 8 lines

I understand it can break some applications, but adding other solution such as preventing send and dedupe from running in parallel (erroring out or block and wait for each other, etc) is going to be really ugly. There's always the workaround for apps to set the subvolume
to RW mode, do the dedupe, then switch it back to RO mode.

Thanks.

From: Filipe Manana <[email protected]>

Checking if the destination root is read-only was being performed only for clone operations. Make deduplication check it as well, as it does not make sense to not do it, even if it is an operation that does not change the file contents (such as defrag for example, which checks first if the root is read-only).

Signed-off-by Filipe Manana <[email protected]>

From: Filipe Manana <[email protected]>

Comparing the content of the pages in the range to deduplicate is now done by the generic helper generic_remap_file_range_prep(), which takes care of

ensuring we do not compare/deduplicate undefined data beyond a file's eof (range from eof to the next block boundary). So remove these checks which are now redundant.

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/ioctl.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

fs/btrfs/ioctl.c +2 -17

index 4e9efc93340e..3a27efa2b955 100644
@@ -3206,31 +3206,16 @@ static void btrfs_double_inode_lock(struct inode *inode1, struct inode *inode2)
 	inode_lock_nested(inode2, I_MUTEX_CHILD);
 }
 
-static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 olen,
+static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len,
 				   struct inode *dst, u64 dst_loff)
 {
-	u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
 	int ret;
-	u64 len = olen;
-
-	if (loff + len == src->i_size)
-		len = ALIGN(src->i_size, bs) - loff;
-	/*
-	 * For same inode case we don't want our length pushed out past i_size
-	 * as comparing that data range makes no sense.
-	 *
-	 * This effectively means we require aligned extents for the single
-	 * inode case, whereas the other cases allow an unaligned length so long
-	 * as it ends at i_size.
-	 */
-	if (dst == src && len != olen)
-		return -EINVAL;
 
 	/*
 	 * Lock destination range to serialize with concurrent readpages().
 	 */
 	lock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1);
-	ret = btrfs_clone(src, dst, loff, olen, len, dst_loff, 1);
+	ret = btrfs_clone(src, dst, loff, len, len, dst_loff, 1);
 	unlock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1);
 
 	return ret;

From: Filipe Manana <[email protected]>

Comparing the content of the pages in the range to deduplicate is now done by the generic helper generic_remap_file_range_prep(), which takes care of ensuring we do not compare/deduplicate undefined data beyond a file's eof (range from eof to the next block boundary). So remove these checks which are now redundant.

Signed-off-by Filipe Manana <[email protected]>

---
 fs/btrfs/ioctl.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

fs/btrfs/ioctl.c +2 -18

index 4e9efc93340e..3a27efa2b955 100644
@@ -3206,31 +3206,16 @@ static void btrfs_double_inode_lock(struct inode *inode1, struct inode *inode2)
        inode_lock_nested(inode2, I_MUTEX_CHILD);
 }
 
-static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 olen,
+static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len,
                                   struct inode *dst, u64 dst_loff)
 {
-       u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
        int ret;
-       u64 len = olen;
-
-       if (loff + len == src->i_size)
-               len = ALIGN(src->i_size, bs) - loff;
-       /*
-        * For same inode case we don't want our length pushed out past i_size
-        * as comparing that data range makes no sense.
-        *
-        * This effectively means we require aligned extents for the single
-        * inode case, whereas the other cases allow an unaligned length so long
-        * as it ends at i_size.
-        */
-       if (dst == src && len != olen)
-               return -EINVAL;
 
        /*
         * Lock destination range to serialize with concurrent readpages().
         */
        lock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1);
-       ret = btrfs_clone(src, dst, loff, olen, len, dst_loff, 1);
+       ret = btrfs_clone(src, dst, loff, len, len, dst_loff, 1);
        unlock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1);
 
        return ret;
--

2.11.0

Quoted 9 lines · On Wed, Dec 12, 2018 at 6:07 PM <[email protected]> wrote:

From: Filipe Manana <[email protected]>

Comparing the content of the pages in the range to deduplicate is now done by the generic helper generic_remap_file_range_prep(), which takes care of ensuring we do not compare/deduplicate undefined data beyond a file's eof (range from eof to the next block boundary). So remove these checks which are now redundant.

Quoted 1 line

Signed-off-by Filipe Manana <[email protected]>

Any reason why this was not yet picked?
Thanks.

Quoted 46 lines

---
 fs/btrfs/ioctl.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

fs/btrfs/ioctl.c +2 -18

index 4e9efc93340e..3a27efa2b955 100644
@@ -3206,31 +3206,16 @@ static void btrfs_double_inode_lock(struct inode *inode1, struct inode *inode2)
        inode_lock_nested(inode2, I_MUTEX_CHILD);
 }
 
-static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 olen,
+static int btrfs_extent_same_range(struct inode *src, u64 loff, u64 len,
                                   struct inode *dst, u64 dst_loff)
 {
-       u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
        int ret;
-       u64 len = olen;
-
-       if (loff + len == src->i_size)
-               len = ALIGN(src->i_size, bs) - loff;
-       /*
-        * For same inode case we don't want our length pushed out past i_size
-        * as comparing that data range makes no sense.
-        *
-        * This effectively means we require aligned extents for the single
-        * inode case, whereas the other cases allow an unaligned length so long
-        * as it ends at i_size.
-        */
-       if (dst == src && len != olen)
-               return -EINVAL;
 
        /*
         * Lock destination range to serialize with concurrent readpages().
         */
        lock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1);
-       ret = btrfs_clone(src, dst, loff, olen, len, dst_loff, 1);
+       ret = btrfs_clone(src, dst, loff, len, len, dst_loff, 1);
        unlock_extent(&BTRFS_I(dst)->io_tree, dst_loff, dst_loff + len - 1);
 
        return ret;
--

2.11.0

Quoted 9 lines · On Wed, Dec 12, 2018 at 6:07 PM <[email protected]> wrote:

From: Filipe Manana <[email protected]>

Comparing the content of the pages in the range to deduplicate is now done by the generic helper generic_remap_file_range_prep(), which takes care of ensuring we do not compare/deduplicate undefined data beyond a file's eof (range from eof to the next block boundary). So remove these checks which are now redundant.

Quoted 1 line

Signed-off-by Filipe Manana <[email protected]>

Any reason why this was not yet picked?

From: Filipe Manana <[email protected]>

This is a follow up to the recent change that makes btrfs use the generic helper for cloning and deduplication [1]. It contains a mix of cleanups and fixes, which existed before that change.

[1] Btrfs: use generic_remap_file_range_prep() for cloning and deduplication

Filipe Manana (4):
Btrfs: move duplicated nodatasum check into common reflink/dedupe
helper
Btrfs: use cross mount point check for cloning and deduplication
Btrfs: check if destination root is read-only for deduplication
Btrfs: remove no longer needed range length checks for deduplication

 fs/btrfs/ioctl.c | 50 ++++++++++++++------------------------------------
 1 file changed, 14 insertions(+), 36 deletions(-)