Failing test case:

1) Write an MSDOS partition table to a disk and create two 256 MiB
partitions, aligned to MiB.  Layout:

# sfdisk -l -uS /dev/sdd

Disk /dev/sdd: 121601 cylinders, 255 heads, 63 sectors/track
Units = sectors of 512 bytes, counting from 0

   Device Boot    Start       End   #sectors  Id  System
/dev/sdd1          2048    526335     524288  83  Linux
/dev/sdd2        526336   1050623     524288  83  Linux


2) Ensure those partitions are empty.

# dd if=/dev/zero bs=1M of=/dev/sdd1
# dd if=/dev/zero bs=1M of=/dev/sdd2


3) Create file system to copy.

# mkfs.btrfs /dev/sdd1


4) Write marker to the last sector before the start of the first
partition.

# echo -n "hammer0" | dd bs=512 of=/dev/sdd seek=2047


5) Write a marker to the last two sectors of the first partition.

# echo -n "canary1" | dd bs=512 of=/dev/sdd1 seek=524286
# echo -n "canary2" | dd bs=512 of=/dev/sdd1 seek=524287

Disk contents:

# hexdump -C /dev/sdd | egrep 'hammer|canary|_BHRfS_M'
000ffe00  68 61 6d 6d 65 72 30 00  00 00 00 00 00 00 00 00  |hammer0.........|
00110040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
04100040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
100ffc00  63 61 6e 61 72 79 31 00  00 00 00 00 00 00 00 00  |canary1.........|
100ffe00  63 61 6e 61 72 79 32 00  00 00 00 00 00 00 00 00  |canary2.........|


6) Copy partition 1 to partition 2 using GParted.

Look at the disk contents again.  "hammer0" from 1 sector before first
partition has been written to 1 sector before the second partition
(over the last sector of the first partition).  Also didn't copy
"canary2", the last sector of the first partition.

# hexdump -C /dev/sdd | egrep 'hammer|canary|_BHRfS_M'
000ffe00  68 61 6d 6d 65 72 30 00  00 00 00 00 00 00 00 00  |hammer0.........|
00110040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
04100040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
100ffc00  63 61 6e 61 72 79 31 00  00 00 00 00 00 00 00 00  |canary1.........|
100ffe00  68 61 6d 6d 65 72 30 00  00 00 00 00 00 00 00 00  |hammer0.........|
10110040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
14100040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
200ffc00  63 61 6e 61 72 79 31 00  00 00 00 00 00 00 00 00  |canary1.........|

Expected disk layout.

# hexdump -C /dev/sdd | egrep 'hammer|canary|_BHRfS_M'
000ffe00  68 61 6d 6d 65 72 30 00  00 00 00 00 00 00 00 00  |hammer0.........|
00110040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
04100040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
100ffc00  63 61 6e 61 72 79 31 00  00 00 00 00 00 00 00 00  |canary1.........|
100ffe00  63 61 6e 61 72 79 32 00  00 00 00 00 00 00 00 00  |canary2.........|
10110040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
14100040  5f 42 48 52 66 53 5f 4d  04 00 00 00 00 00 00 00  |_BHRfS_M........|
200ffc00  63 61 6e 61 72 79 31 00  00 00 00 00 00 00 00 00  |canary1.........|
200ffe00  63 61 6e 61 72 79 32 00  00 00 00 00 00 00 00 00  |canary2.........|

Hi Phillip,

If you have understanding of how to fix this that would be appreciated.

Thanks,
Mike

Looks like a simple off by one somewhere; it's starting one sector too early and ending one sector too early.  I'll take a look.

I think I may have spotted it:

diff --git a/src/Copy_Blocks.cc b/src/Copy_Blocks.cc
index d040951..b9adcfc 100644
--- a/src/Copy_Blocks.cc
+++ b/src/Copy_Blocks.cc
@@ -105,9 +105,9 @@ void copy_blocks::copy_thread()
                {
                        blocksize -= 2*blocksize;
                        done -= 2*done;
-                       offset_src += ( (length / src_sector_size) - 1 );
+                       offset_src += ( length / src_sector_size );
                        /* Handle situation where src sector size is smaller than dst sector size and an addi
-                       offset_dst += ( ((length + (dst_sector_size - 1)) / dst_sector_size) - 1 );
+                       offset_dst += ((length + (dst_sector_size - 1)) / dst_sector_size);
                }
                success = true;
        } else success = false;

Created attachment 294544 [details] [review]
oops.diff

Bah... copy/paste mangled that... here's the patch.

I also noticed that the code for doing the move backwards to handle an in place move to the right is being used, even though it isn't needed since with the copy, the source and destinations do not overlap.  That should probably be fixed too.

Thank you Phillip for the quick fix on this.  I'll test in a day or so.


On the question of not needing to copy backwards if they are separate
partitions.  At the moment copy_blocks class doesn't know that they are
separate partitions and only decides the copy direction based on the
start sector of the source and destination.  Code fragment:

void copy_blocks::copy_thread()
...
	//Handle situation where we need to perform the copy beginning
	//  with the end of the partition and finishing with the start.
	if ( offset_src < offset_dst )
	...

Does it really matter if it's copying backwards?
Copying on the same drive will involve the same number of seeks in
either direction.  Copying backwards between drives will need 1 short
seek per buffer (typically 8 or 16 MiB) per drive, where as copying
forwards won't require any seeks.  This doesn't seem like an excessive
problem.  Nice to fix but probably won't notice any performance
difference.


Thanks,
Mike

Hi Phillip,

Created a git commit from your patch.  Successfully tested copying
partitions forwards and backwards.  As such the following commit has
been pushed upstream.

Fix off by one sector error in GParted internal block copy (#742920)
https://git.gnome.org/browse/gparted/commit/?id=8a952cd4a9fea6d395c05f46d075aaf368011653

Thanks,
Mike

Hi Curtis,

When you're back I think that we should do a GParted release to get this
fix in the hands of the users as soon as we can.  Given that we have
added other fixes too it should be called GParted 0.21.0.

Thanks,
Mike

Hi Mike,

I agree with your assessment and will start the release process with what we currently have committed in the git repository.  Since we have text string changes since the last release, we should allow time (usually 1 week) for the language translators to do their work,

Curtis

This enhancement was included in the GParted 0.21.0 release on January 26, 2015.