[PATCH] btrfs: fix unmountable seed device after fstrim

[PATCH] btrfs: fix unmountable seed device after fstrim

[Anand Jain]

The following test case reproduces an issue of wrongly freeing the in-use
block on the readonly seed device when fstrim is called on the rw sprout
device. As shown below.

create a seed device and add a sprout device to it
	mkfs.btrfs -fq -dsingle -msingle /dev/loop0
	btrfstune -S 1 /dev/loop0
	mount /dev/loop0 /btrfs
	btrfs dev add -f /dev/loop1 /btrfs

  BTRFS info (device loop0): relocating block group 290455552 flags system
  BTRFS info (device loop0): relocating block group 1048576 flags system
  BTRFS info (device loop0): disk added /dev/loop1

	umount /btrfs
mount the sprout device and run fstrim
	mount /dev/loop1 /btrfs
	fstrim /btrfs
	umount /btrfs

now try to mount the seed device, and it fails...
	mount /dev/loop0 /btrfs

  mount: /btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.

  BTRFS error (device loop0): bad tree block start, want 5292032 have 0
  BTRFS warning (device loop0): couldn't read-tree root
  BTRFS error (device loop0): open_ctree failed

Block 5292032 is missing on the readonly seed device.

From the dump-tree of the seed device taken before the fstrim. Block
5292032 belonged to the block group starting at 5242880

<snip>
 item 3 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16169 itemsize 24
	block group used 114688 chunk_objectid 256 flags METADATA
<snip>

From the dump-tree of the sprout device (taken before the fstrim).
fstrim used the block-group 5242880 to find the free space to free.

<snip>

 item 1 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16226 itemsize 24
	block group used 32768 chunk_objectid 256 flags METADATA
<snip>


bpf tracing the fstrim command finds the missing block 5292032 within the
range of the discarded blocks...

  kprobe:btrfs_discard_extent {
    printf("free start %llu end %llu num_bytes %llu\n", arg1, arg1+arg2, arg2);
  }

btrfs_discard_extent(..., start, num_bytes, ...):

 free start 5259264 end 5406720 num_bytes 147456
<snip>

Fix this by avoiding the discard command to the readonly seed device.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reported-by: Chris Murphy <lists@colorremedies.com>
---

A xfstests case to follow.

 fs/btrfs/extent-tree.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 7a28314189b4..0d19bd213715 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1340,12 +1340,19 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
 		stripe = bbio->stripes;
 		for (i = 0; i < bbio->num_stripes; i++, stripe++) {
 			u64 bytes;
+			struct btrfs_device *device = stripe->dev;
 
-			if (!stripe->dev->bdev) {
+			if (!device->bdev) {
 				ASSERT(btrfs_test_opt(fs_info, DEGRADED));
 				continue;
 			}
 
+			/*
+			 * Skip sending discard command to a non-writeable device.
+			 */
+			if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state))
+				continue;
+
 			ret = do_discard_extent(stripe, &bytes);
 			if (!ret) {
 				discarded_bytes += bytes;
-- 
2.29.2

Re: [PATCH] btrfs: fix unmountable seed device after fstrim

[Filipe Manana]

On Fri, Apr 30, 2021 at 10:14 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> The following test case reproduces an issue of wrongly freeing the in-use
> block on the readonly seed device when fstrim is called on the rw sprout
> device. As shown below.
>
> create a seed device and add a sprout device to it
>         mkfs.btrfs -fq -dsingle -msingle /dev/loop0
>         btrfstune -S 1 /dev/loop0
>         mount /dev/loop0 /btrfs
>         btrfs dev add -f /dev/loop1 /btrfs
>
>   BTRFS info (device loop0): relocating block group 290455552 flags system
>   BTRFS info (device loop0): relocating block group 1048576 flags system
>   BTRFS info (device loop0): disk added /dev/loop1
>
>         umount /btrfs
> mount the sprout device and run fstrim
>         mount /dev/loop1 /btrfs
>         fstrim /btrfs
>         umount /btrfs
>
> now try to mount the seed device, and it fails...
>         mount /dev/loop0 /btrfs
>
>   mount: /btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.
>
>   BTRFS error (device loop0): bad tree block start, want 5292032 have 0
>   BTRFS warning (device loop0): couldn't read-tree root
>   BTRFS error (device loop0): open_ctree failed
>
> Block 5292032 is missing on the readonly seed device.
>
> From the dump-tree of the seed device taken before the fstrim. Block
> 5292032 belonged to the block group starting at 5242880
>
> <snip>
>  item 3 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16169 itemsize 24
>         block group used 114688 chunk_objectid 256 flags METADATA
> <snip>
>
> From the dump-tree of the sprout device (taken before the fstrim).
> fstrim used the block-group 5242880 to find the free space to free.
>
> <snip>
>
>  item 1 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16226 itemsize 24
>         block group used 32768 chunk_objectid 256 flags METADATA
> <snip>
>
>
> bpf tracing the fstrim command finds the missing block 5292032 within the
> range of the discarded blocks...
>
>   kprobe:btrfs_discard_extent {
>     printf("free start %llu end %llu num_bytes %llu\n", arg1, arg1+arg2, arg2);
>   }
>
> btrfs_discard_extent(..., start, num_bytes, ...):
>
>  free start 5259264 end 5406720 num_bytes 147456
> <snip>
>
> Fix this by avoiding the discard command to the readonly seed device.

Quite hard to read the changelog.

It could be better organized, indentation is screwed up in many
places, shell command lines should have some prefix like $ or # to
make it clear what is a command and what is the output of a command,
missing full collons at the end of sentences, sentences not starting
with a capital letter, etc.

>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reported-by: Chris Murphy <lists@colorremedies.com>
> ---
>
> A xfstests case to follow.
>
>  fs/btrfs/extent-tree.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 7a28314189b4..0d19bd213715 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1340,12 +1340,19 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>                 stripe = bbio->stripes;
>                 for (i = 0; i < bbio->num_stripes; i++, stripe++) {
>                         u64 bytes;
> +                       struct btrfs_device *device = stripe->dev;
>
> -                       if (!stripe->dev->bdev) {
> +                       if (!device->bdev) {
>                                 ASSERT(btrfs_test_opt(fs_info, DEGRADED));
>                                 continue;
>                         }
>
> +                       /*
> +                        * Skip sending discard command to a non-writeable device.
> +                        */
> +                       if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state))
> +                               continue;

With such an obvious and easy to read conditional, the comment is
completely unnecessary, it doesn't add any value.

Other than that, the fix itself looks good.

Thanks.

> +
>                         ret = do_discard_extent(stripe, &bytes);
>                         if (!ret) {
>                                 discarded_bytes += bytes;
> --
> 2.29.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] btrfs: fix unmountable seed device after fstrim

[Anand Jain]

On 30/4/21 6:11 pm, Filipe Manana wrote:
> On Fri, Apr 30, 2021 at 10:14 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> The following test case reproduces an issue of wrongly freeing the in-use
>> block on the readonly seed device when fstrim is called on the rw sprout
>> device. As shown below.
>>
>> create a seed device and add a sprout device to it
>>          mkfs.btrfs -fq -dsingle -msingle /dev/loop0
>>          btrfstune -S 1 /dev/loop0
>>          mount /dev/loop0 /btrfs
>>          btrfs dev add -f /dev/loop1 /btrfs
>>
>>    BTRFS info (device loop0): relocating block group 290455552 flags system
>>    BTRFS info (device loop0): relocating block group 1048576 flags system
>>    BTRFS info (device loop0): disk added /dev/loop1
>>
>>          umount /btrfs
>> mount the sprout device and run fstrim
>>          mount /dev/loop1 /btrfs
>>          fstrim /btrfs
>>          umount /btrfs
>>
>> now try to mount the seed device, and it fails...
>>          mount /dev/loop0 /btrfs
>>
>>    mount: /btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.
>>
>>    BTRFS error (device loop0): bad tree block start, want 5292032 have 0
>>    BTRFS warning (device loop0): couldn't read-tree root
>>    BTRFS error (device loop0): open_ctree failed
>>
>> Block 5292032 is missing on the readonly seed device.
>>
>>  From the dump-tree of the seed device taken before the fstrim. Block
>> 5292032 belonged to the block group starting at 5242880
>>
>> <snip>
>>   item 3 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16169 itemsize 24
>>          block group used 114688 chunk_objectid 256 flags METADATA
>> <snip>
>>
>>  From the dump-tree of the sprout device (taken before the fstrim).
>> fstrim used the block-group 5242880 to find the free space to free.
>>
>> <snip>
>>
>>   item 1 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16226 itemsize 24
>>          block group used 32768 chunk_objectid 256 flags METADATA
>> <snip>
>>
>>
>> bpf tracing the fstrim command finds the missing block 5292032 within the
>> range of the discarded blocks...
>>
>>    kprobe:btrfs_discard_extent {
>>      printf("free start %llu end %llu num_bytes %llu\n", arg1, arg1+arg2, arg2);
>>    }
>>
>> btrfs_discard_extent(..., start, num_bytes, ...):
>>
>>   free start 5259264 end 5406720 num_bytes 147456
>> <snip>
>>
>> Fix this by avoiding the discard command to the readonly seed device.
> 
> Quite hard to read the changelog.
> 
> It could be better organized, indentation is screwed up in many
> places, shell command lines should have some prefix like $ or # to
> make it clear what is a command and what is the output of a command,
> missing full collons at the end of sentences, sentences not starting
> with a capital letter, etc.

   OK. Let me try to fix it.

> 
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Reported-by: Chris Murphy <lists@colorremedies.com>
>> ---
>>
>> A xfstests case to follow.
>>
>>   fs/btrfs/extent-tree.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 7a28314189b4..0d19bd213715 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -1340,12 +1340,19 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>>                  stripe = bbio->stripes;
>>                  for (i = 0; i < bbio->num_stripes; i++, stripe++) {
>>                          u64 bytes;
>> +                       struct btrfs_device *device = stripe->dev;
>>
>> -                       if (!stripe->dev->bdev) {
>> +                       if (!device->bdev) {
>>                                  ASSERT(btrfs_test_opt(fs_info, DEGRADED));
>>                                  continue;
>>                          }
>>
>> +                       /*
>> +                        * Skip sending discard command to a non-writeable device.
>> +                        */
>> +                       if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state))
>> +                               continue;
> 
> With such an obvious and easy to read conditional, the comment is
> completely unnecessary, it doesn't add any value.
> 
  Will remove.

> Other than that, the fix itself looks good.
> 

Thanks

> Thanks.
> 
>> +
>>                          ret = do_discard_extent(stripe, &bytes);
>>                          if (!ret) {
>>                                  discarded_bytes += bytes;
>> --
>> 2.29.2
>>
> 
> 

[PATCH] btrfs: fix unmountable seed device after fstrim

[Anand Jain]

The following test case reproduces an issue of wrongly freeing in-use
blocks on the readonly seed device when fstrim is called on the rw sprout
device. As shown below.

Create a seed device and add a sprout device to it:
	$ mkfs.btrfs -fq -dsingle -msingle /dev/loop0
	$ btrfstune -S 1 /dev/loop0
	$ mount /dev/loop0 /btrfs
	$ btrfs dev add -f /dev/loop1 /btrfs
	BTRFS info (device loop0): relocating block group 290455552 flags system
	BTRFS info (device loop0): relocating block group 1048576 flags system
	BTRFS info (device loop0): disk added /dev/loop1
	$ umount /btrfs

Mount the sprout device and run fstrim:
	$ mount /dev/loop1 /btrfs
	$ fstrim /btrfs
	$ umount /btrfs

Now try to mount the seed device, and it fails:
	$ mount /dev/loop0 /btrfs
	mount: /btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.

Block 5292032 is missing on the readonly seed device.
	$ dmesg -kt | tail
	<snip>
	BTRFS error (device loop0): bad tree block start, want 5292032 have 0
	BTRFS warning (device loop0): couldn't read-tree root
	BTRFS error (device loop0): open_ctree failed

From the dump-tree of the seed device (taken before the fstrim). Block
5292032 belonged to the block group starting at 5242880
	$ btrfs inspect dump-tree -e /dev/loop0 | grep -A1 BLOCK_GROUP
	<snip>
	item 3 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16169 itemsize 24
		block group used 114688 chunk_objectid 256 flags METADATA
	<snip>

From the dump-tree of the sprout device (taken before the fstrim).
fstrim(8) used block-group 5242880 to find the related free space to free.
	$ btrfs inspect dump-tree -e /dev/loop1 | grep -A1 BLOCK_GROUP
	<snip>
	item 1 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16226 itemsize 24
		block group used 32768 chunk_objectid 256 flags METADATA
	<snip>

Bpf kernel tracing the fstrim(8) command finds the missing block 5292032
within the range of the discarded blocks as below.
	kprobe:btrfs_discard_extent {
		printf("freeing start %llu end %llu num_bytes %llu:\n",
			arg1, arg1+arg2, arg2);
	}

	freeing start 5259264 end 5406720 num_bytes 147456
	<snip>

Fix this by avoiding the discard command to the readonly seed device.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reported-by: Chris Murphy <lists@colorremedies.com>
---
 fs/btrfs/extent-tree.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 7a28314189b4..f1d15b68994a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1340,12 +1340,16 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
 		stripe = bbio->stripes;
 		for (i = 0; i < bbio->num_stripes; i++, stripe++) {
 			u64 bytes;
+			struct btrfs_device *device = stripe->dev;
 
-			if (!stripe->dev->bdev) {
+			if (!device->bdev) {
 				ASSERT(btrfs_test_opt(fs_info, DEGRADED));
 				continue;
 			}
 
+			if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state))
+				continue;
+
 			ret = do_discard_extent(stripe, &bytes);
 			if (!ret) {
 				discarded_bytes += bytes;
-- 
2.29.2

Re: [PATCH] btrfs: fix unmountable seed device after fstrim

[Filipe Manana]

On Fri, Apr 30, 2021 at 1:03 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> The following test case reproduces an issue of wrongly freeing in-use
> blocks on the readonly seed device when fstrim is called on the rw sprout
> device. As shown below.
>
> Create a seed device and add a sprout device to it:
>         $ mkfs.btrfs -fq -dsingle -msingle /dev/loop0

An example of making things easier to the eye here, is adding a blank
line before the mkfs line.
The same applies to all the other similar places below.

>         $ btrfstune -S 1 /dev/loop0
>         $ mount /dev/loop0 /btrfs
>         $ btrfs dev add -f /dev/loop1 /btrfs
>         BTRFS info (device loop0): relocating block group 290455552 flags system
>         BTRFS info (device loop0): relocating block group 1048576 flags system
>         BTRFS info (device loop0): disk added /dev/loop1
>         $ umount /btrfs
>
> Mount the sprout device and run fstrim:
>         $ mount /dev/loop1 /btrfs
>         $ fstrim /btrfs
>         $ umount /btrfs
>
> Now try to mount the seed device, and it fails:
>         $ mount /dev/loop0 /btrfs
>         mount: /btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.
>
> Block 5292032 is missing on the readonly seed device.

Colon ":" instead of ".", plus blank line.

>         $ dmesg -kt | tail
>         <snip>
>         BTRFS error (device loop0): bad tree block start, want 5292032 have 0
>         BTRFS warning (device loop0): couldn't read-tree root
>         BTRFS error (device loop0): open_ctree failed
>
> From the dump-tree of the seed device (taken before the fstrim). Block
> 5292032 belonged to the block group starting at 5242880

Missing colon and blank line too.

>         $ btrfs inspect dump-tree -e /dev/loop0 | grep -A1 BLOCK_GROUP
>         <snip>
>         item 3 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16169 itemsize 24
>                 block group used 114688 chunk_objectid 256 flags METADATA
>         <snip>
>
> From the dump-tree of the sprout device (taken before the fstrim).
> fstrim(8) used block-group 5242880 to find the related free space to free.

Colon ":" and not ".", plus blank line.

>         $ btrfs inspect dump-tree -e /dev/loop1 | grep -A1 BLOCK_GROUP
>         <snip>
>         item 1 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16226 itemsize 24
>                 block group used 32768 chunk_objectid 256 flags METADATA
>         <snip>
>
> Bpf kernel tracing the fstrim(8) command finds the missing block 5292032
> within the range of the discarded blocks as below.

Same as before.

>         kprobe:btrfs_discard_extent {
>                 printf("freeing start %llu end %llu num_bytes %llu:\n",
>                         arg1, arg1+arg2, arg2);
>         }
>
>         freeing start 5259264 end 5406720 num_bytes 147456
>         <snip>
>
> Fix this by avoiding the discard command to the readonly seed device.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reported-by: Chris Murphy <lists@colorremedies.com>

The fix looks good. Don't feel forced to address the style comments
above, consider them more a recommendation for the future.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> ---
>  fs/btrfs/extent-tree.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 7a28314189b4..f1d15b68994a 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1340,12 +1340,16 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>                 stripe = bbio->stripes;
>                 for (i = 0; i < bbio->num_stripes; i++, stripe++) {
>                         u64 bytes;
> +                       struct btrfs_device *device = stripe->dev;
>
> -                       if (!stripe->dev->bdev) {
> +                       if (!device->bdev) {
>                                 ASSERT(btrfs_test_opt(fs_info, DEGRADED));
>                                 continue;
>                         }
>
> +                       if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state))
> +                               continue;
> +
>                         ret = do_discard_extent(stripe, &bytes);
>                         if (!ret) {
>                                 discarded_bytes += bytes;
> --
> 2.29.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] btrfs: fix unmountable seed device after fstrim

[Anand Jain]

On 30/4/21 8:14 pm, Filipe Manana wrote:
> On Fri, Apr 30, 2021 at 1:03 PM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> The following test case reproduces an issue of wrongly freeing in-use
>> blocks on the readonly seed device when fstrim is called on the rw sprout
>> device. As shown below.
>>
>> Create a seed device and add a sprout device to it:
>>          $ mkfs.btrfs -fq -dsingle -msingle /dev/loop0
> 
> An example of making things easier to the eye here, is adding a blank
> line before the mkfs line.
> The same applies to all the other similar places below.
> 
>>          $ btrfstune -S 1 /dev/loop0
>>          $ mount /dev/loop0 /btrfs
>>          $ btrfs dev add -f /dev/loop1 /btrfs
>>          BTRFS info (device loop0): relocating block group 290455552 flags system
>>          BTRFS info (device loop0): relocating block group 1048576 flags system
>>          BTRFS info (device loop0): disk added /dev/loop1
>>          $ umount /btrfs
>>
>> Mount the sprout device and run fstrim:
>>          $ mount /dev/loop1 /btrfs
>>          $ fstrim /btrfs
>>          $ umount /btrfs
>>
>> Now try to mount the seed device, and it fails:
>>          $ mount /dev/loop0 /btrfs
>>          mount: /btrfs: wrong fs type, bad option, bad superblock on /dev/loop0, missing codepage or helper program, or other error.
>>
>> Block 5292032 is missing on the readonly seed device.
> 
> Colon ":" instead of ".", plus blank line.
> 
>>          $ dmesg -kt | tail
>>          <snip>
>>          BTRFS error (device loop0): bad tree block start, want 5292032 have 0
>>          BTRFS warning (device loop0): couldn't read-tree root
>>          BTRFS error (device loop0): open_ctree failed
>>
>>  From the dump-tree of the seed device (taken before the fstrim). Block
>> 5292032 belonged to the block group starting at 5242880
> 
> Missing colon and blank line too.
> 
>>          $ btrfs inspect dump-tree -e /dev/loop0 | grep -A1 BLOCK_GROUP
>>          <snip>
>>          item 3 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16169 itemsize 24
>>                  block group used 114688 chunk_objectid 256 flags METADATA
>>          <snip>
>>
>>  From the dump-tree of the sprout device (taken before the fstrim).
>> fstrim(8) used block-group 5242880 to find the related free space to free.
> 
> Colon ":" and not ".", plus blank line.
> 
>>          $ btrfs inspect dump-tree -e /dev/loop1 | grep -A1 BLOCK_GROUP
>>          <snip>
>>          item 1 key (5242880 BLOCK_GROUP_ITEM 8388608) itemoff 16226 itemsize 24
>>                  block group used 32768 chunk_objectid 256 flags METADATA
>>          <snip>
>>
>> Bpf kernel tracing the fstrim(8) command finds the missing block 5292032
>> within the range of the discarded blocks as below.
> 
> Same as before.
> 
>>          kprobe:btrfs_discard_extent {
>>                  printf("freeing start %llu end %llu num_bytes %llu:\n",
>>                          arg1, arg1+arg2, arg2);
>>          }
>>
>>          freeing start 5259264 end 5406720 num_bytes 147456
>>          <snip>
>>
>> Fix this by avoiding the discard command to the readonly seed device.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Reported-by: Chris Murphy <lists@colorremedies.com>
> 
> The fix looks good. Don't feel forced to address the style comments
> above, consider them more a recommendation for the future.
> 

Yep. Thanks.
  Also, I have missed the re-roll count and its log here.
  I will just mention that here.
     v2:
        Fix commit changelog.
        Drop a code comment.
If David needs, I don't mind resending with these changes.

> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Thanks.
> 
>> ---
>>   fs/btrfs/extent-tree.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 7a28314189b4..f1d15b68994a 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -1340,12 +1340,16 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>>                  stripe = bbio->stripes;
>>                  for (i = 0; i < bbio->num_stripes; i++, stripe++) {
>>                          u64 bytes;
>> +                       struct btrfs_device *device = stripe->dev;
>>
>> -                       if (!stripe->dev->bdev) {
>> +                       if (!device->bdev) {
>>                                  ASSERT(btrfs_test_opt(fs_info, DEGRADED));
>>                                  continue;
>>                          }
>>
>> +                       if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state))
>> +                               continue;
>> +
>>                          ret = do_discard_extent(stripe, &bytes);
>>                          if (!ret) {
>>                                  discarded_bytes += bytes;
>> --
>> 2.29.2
>>
> 
> 

[PATCH] btrfs: add fstrim test case on the sprout device

[Anand Jain]

Add fstrim test case on the sprout device, verify seed device
integrity.

Needs kernel patch [1] to pass the test case.
[1]
  btrfs: fix unmountable seed device after fstrim

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/btrfs/236     | 72 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/236.out |  5 ++++
 tests/btrfs/group   |  1 +
 3 files changed, 78 insertions(+)
 create mode 100755 tests/btrfs/236
 create mode 100644 tests/btrfs/236.out

diff --git a/tests/btrfs/236 b/tests/btrfs/236
new file mode 100755
index 000000000000..599892bebf10
--- /dev/null
+++ b/tests/btrfs/236
@@ -0,0 +1,72 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 Oracle. All Rights Reserved.
+#
+# FS QA Test 236
+#
+# Check seed device integrity after fstrim on the sprout device.
+#
+#  Kernel bug is fixed by the commit:
+#    btrfs: fix unmountable seed device after fstrim
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_command "$BTRFS_TUNE_PROG" btrfstune
+_require_scratch_dev_pool 2
+_scratch_dev_pool_get 2
+
+seed=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $1}')
+sprout=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $2}')
+
+_mkfs_dev $seed
+_mount $seed $SCRATCH_MNT
+
+$XFS_IO_PROG -f -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
+_scratch_unmount
+$BTRFS_TUNE_PROG -S 1 $seed
+
+# Mount the seed device and add the rw device
+_mount $seed $SCRATCH_MNT 2>&1 | _filter_scratch
+md5sum $SCRATCH_MNT/foo | _filter_scratch
+
+$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
+_scratch_unmount
+
+# Now remount
+_mount $sprout $SCRATCH_MNT
+$XFS_IO_PROG -f -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null
+
+fstrim $SCRATCH_MNT
+
+_scratch_unmount
+_mount $seed $SCRATCH_MNT 2>&1 | _filter_scratch
+md5sum $SCRATCH_MNT/foo | _filter_scratch
+_scratch_unmount
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/236.out b/tests/btrfs/236.out
new file mode 100644
index 000000000000..2929d39395a8
--- /dev/null
+++ b/tests/btrfs/236.out
@@ -0,0 +1,5 @@
+QA output created by 236
+mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
+096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
+mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
+096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
diff --git a/tests/btrfs/group b/tests/btrfs/group
index 331dd432fac3..5032259244e0 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -238,3 +238,4 @@
 233 auto quick subvolume
 234 auto quick compress rw
 235 auto quick send
+236 auto quick seed trim
-- 
2.27.0

Re: [PATCH] btrfs: add fstrim test case on the sprout device

[Filipe Manana]

On Fri, Apr 30, 2021 at 3:40 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> Add fstrim test case on the sprout device, verify seed device
> integrity.
>
> Needs kernel patch [1] to pass the test case.
> [1]
>   btrfs: fix unmountable seed device after fstrim
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  tests/btrfs/236     | 72 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/236.out |  5 ++++
>  tests/btrfs/group   |  1 +
>  3 files changed, 78 insertions(+)
>  create mode 100755 tests/btrfs/236
>  create mode 100644 tests/btrfs/236.out
>
> diff --git a/tests/btrfs/236 b/tests/btrfs/236
> new file mode 100755
> index 000000000000..599892bebf10
> --- /dev/null
> +++ b/tests/btrfs/236
> @@ -0,0 +1,72 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 Oracle. All Rights Reserved.
> +#
> +# FS QA Test 236
> +#
> +# Check seed device integrity after fstrim on the sprout device.
> +#
> +#  Kernel bug is fixed by the commit:
> +#    btrfs: fix unmountable seed device after fstrim
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_command "$BTRFS_TUNE_PROG" btrfstune
> +_require_scratch_dev_pool 2
> +_scratch_dev_pool_get 2
> +
> +seed=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $1}')
> +sprout=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $2}')
> +
> +_mkfs_dev $seed
> +_mount $seed $SCRATCH_MNT

Missing the check for discard/trim support:

_require_batched_discard $SCRATCH_MNT

> +
> +$XFS_IO_PROG -f -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
> +_scratch_unmount
> +$BTRFS_TUNE_PROG -S 1 $seed
> +
> +# Mount the seed device and add the rw device
> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_scratch
> +md5sum $SCRATCH_MNT/foo | _filter_scratch
> +
> +$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
> +_scratch_unmount
> +
> +# Now remount
> +_mount $sprout $SCRATCH_MNT
> +$XFS_IO_PROG -f -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null
> +
> +fstrim $SCRATCH_MNT

Should use $FSTRIM_PROG instead of fstrim.

> +
> +_scratch_unmount
> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_scratch
> +md5sum $SCRATCH_MNT/foo | _filter_scratch
> +_scratch_unmount

Missing a call to _scratch_dev_pool_put

> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/236.out b/tests/btrfs/236.out
> new file mode 100644
> index 000000000000..2929d39395a8
> --- /dev/null
> +++ b/tests/btrfs/236.out
> @@ -0,0 +1,5 @@
> +QA output created by 236
> +mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
> +096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
> +mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
> +096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 331dd432fac3..5032259244e0 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -238,3 +238,4 @@
>  233 auto quick subvolume
>  234 auto quick compress rw
>  235 auto quick send
> +236 auto quick seed trim

Ok, it fails as expected without the btrfs fix.
But with the fix applied, it fails differently for me. It looks like
different mount versions output different strings maybe:

root 16:18:40 /home/fdmanana/git/hub/xfstests (master)> ./check btrfs/237
FSTYP         -- btrfs
PLATFORM      -- Linux/x86_64 debian8 5.12.0-rc8-btrfs-next-86 #1 SMP
PREEMPT Fri Apr 23 17:35:49 WEST 2021
MKFS_OPTIONS  -- /dev/sdc
MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1

btrfs/237 - output mismatch (see
/home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad)
    --- tests/btrfs/237.out 2021-04-30 16:09:33.103380077 +0100
    +++ /home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad
2021-04-30 16:19:31.577988213 +0100
    @@ -1,5 +1,5 @@
     QA output created by 237
    -mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
    +mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
     096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
    -mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
    +mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
     096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
    ...
    (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/237.out
/home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad'  to see
the entire diff)
Ran: btrfs/237
Failures: btrfs/237
Failed 1 of 1 tests

root 16:19:31 /home/fdmanana/git/hub/xfstests (master)> diff -u
/home/fdmanana/git/hub/xfstests/tests/btrfs/237.out
/home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad
--- /home/fdmanana/git/hub/xfstests/tests/btrfs/237.out 2021-04-30
16:09:33.103380077 +0100
+++ /home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad
2021-04-30 16:19:31.577988213 +0100
@@ -1,5 +1,5 @@
 QA output created by 237
-mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
+mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
 096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
-mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
+mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
 096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
root 16:20:06 /home/fdmanana/git/hub/xfstests (master)>

Thanks.

> --
> 2.27.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] btrfs: add fstrim test case on the sprout device

[Anand Jain]

>> +
>> +_mkfs_dev $seed
>> +_mount $seed $SCRATCH_MNT
> 
> Missing the check for discard/trim support:
> 
> _require_batched_discard $SCRATCH_MNT
> 
  Oh. I will add.


>> +fstrim $SCRATCH_MNT
> 
> Should use $FSTRIM_PROG instead of fstrim.
> 

  Right. Ok.


>> +
>> +_scratch_unmount
>> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_scratch
>> +md5sum $SCRATCH_MNT/foo | _filter_scratch
>> +_scratch_unmount
> 
> Missing a call to _scratch_dev_pool_put
> 

  Yep. I will add.


> Ok, it fails as expected without the btrfs fix.
> But with the fix applied, it fails differently for me. It looks like
> different mount versions output different strings maybe:
> > root 16:18:40 /home/fdmanana/git/hub/xfstests (master)> ./check btrfs/237
> FSTYP         -- btrfs
> PLATFORM      -- Linux/x86_64 debian8 5.12.0-rc8-btrfs-next-86 #1 SMP
> PREEMPT Fri Apr 23 17:35:49 WEST 2021
> MKFS_OPTIONS  -- /dev/sdc
> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
> 
> btrfs/237 - output mismatch (see
> /home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad)
>      --- tests/btrfs/237.out 2021-04-30 16:09:33.103380077 +0100
>      +++ /home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad
> 2021-04-30 16:19:31.577988213 +0100
>      @@ -1,5 +1,5 @@
>       QA output created by 237
>      -mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
>      +mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
>       096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
>      -mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
>      +mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
>       096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
>      ...
>      (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/237.out
> /home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad'  to see
> the entire diff)
> Ran: btrfs/237
> Failures: btrfs/237
> Failed 1 of 1 tests
> 
> root 16:19:31 /home/fdmanana/git/hub/xfstests (master)> diff -u
> /home/fdmanana/git/hub/xfstests/tests/btrfs/237.out
> /home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad
> --- /home/fdmanana/git/hub/xfstests/tests/btrfs/237.out 2021-04-30
> 16:09:33.103380077 +0100
> +++ /home/fdmanana/git/hub/xfstests/results//btrfs/237.out.bad
> 2021-04-30 16:19:31.577988213 +0100
> @@ -1,5 +1,5 @@
>   QA output created by 237
> -mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
> +mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
>   096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
> -mount: SCRATCH_MNT: WARNING: device write-protected, mounted read-only.
> +mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
>   096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
> root 16:20:06 /home/fdmanana/git/hub/xfstests (master)>
> 

  Just found there is _filter_ro_mount, which comes in handy here.
  I will use it.

Thanks, Anand


> Thanks.
> 
>> --
>> 2.27.0
>>
> 
> 

[PATCH v2] btrfs: add fstrim test case on the sprout device

[Anand Jain]

Add fstrim test case on the sprout device, verify seed device
integrity.

 btrfs: fix unmountable seed device after fstrim

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2:
  Add _require_fstrim and _require_batched_discard.
  Use FSTRIM_PROG.
  Use _filter_ro_mount to handle the difference in output in different
     mount(8) version.
  Call _scratch_dev_pool_put.
  Add _check_btrfs_filesystem $seed to check the whole seed fs.
  Update in-code comments.

 tests/btrfs/236     | 81 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/236.out |  5 +++
 tests/btrfs/group   |  1 +
 3 files changed, 87 insertions(+)
 create mode 100755 tests/btrfs/236
 create mode 100644 tests/btrfs/236.out

diff --git a/tests/btrfs/236 b/tests/btrfs/236
new file mode 100755
index 000000000000..aac27fac06dd
--- /dev/null
+++ b/tests/btrfs/236
@@ -0,0 +1,81 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 Oracle. All Rights Reserved.
+#
+# FS QA Test 236
+#
+# Check seed device integrity after fstrim on the sprout device.
+#
+#  Kernel bug is fixed by the commit:
+#    btrfs: fix unmountable seed device after fstrim
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_command "$BTRFS_TUNE_PROG" btrfstune
+_require_fstrim
+_require_scratch_dev_pool 2
+_scratch_dev_pool_get 2
+
+seed=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $1}')
+sprout=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $2}')
+
+_mkfs_dev $seed
+_mount $seed $SCRATCH_MNT
+
+$XFS_IO_PROG -f -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
+_scratch_unmount
+$BTRFS_TUNE_PROG -S 1 $seed
+
+# Mount the seed device and add the rw device
+_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
+md5sum $SCRATCH_MNT/foo | _filter_scratch
+
+$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
+_scratch_unmount
+
+# Now remount writeable sprout device, create some data and run fstrim
+_mount $sprout $SCRATCH_MNT
+_require_batched_discard $SCRATCH_MNT
+
+$XFS_IO_PROG -f -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null
+
+$FSTRIM_PROG $SCRATCH_MNT
+
+_scratch_unmount
+
+# Verify seed device is all ok
+_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
+md5sum $SCRATCH_MNT/foo | _filter_scratch
+_scratch_unmount
+
+_check_btrfs_filesystem $seed
+
+_scratch_dev_pool_put
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/236.out b/tests/btrfs/236.out
new file mode 100644
index 000000000000..01699b8fc291
--- /dev/null
+++ b/tests/btrfs/236.out
@@ -0,0 +1,5 @@
+QA output created by 236
+mount: device write-protected, mounting read-only
+096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
+mount: device write-protected, mounting read-only
+096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
diff --git a/tests/btrfs/group b/tests/btrfs/group
index 331dd432fac3..5032259244e0 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -238,3 +238,4 @@
 233 auto quick subvolume
 234 auto quick compress rw
 235 auto quick send
+236 auto quick seed trim
-- 
2.27.0

Re: [PATCH v2] btrfs: add fstrim test case on the sprout device

[Filipe Manana]

On Sat, May 1, 2021 at 6:24 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> Add fstrim test case on the sprout device, verify seed device
> integrity.
>
>  btrfs: fix unmountable seed device after fstrim
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2:
>   Add _require_fstrim and _require_batched_discard.
>   Use FSTRIM_PROG.
>   Use _filter_ro_mount to handle the difference in output in different
>      mount(8) version.
>   Call _scratch_dev_pool_put.
>   Add _check_btrfs_filesystem $seed to check the whole seed fs.
>   Update in-code comments.
>
>  tests/btrfs/236     | 81 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/236.out |  5 +++
>  tests/btrfs/group   |  1 +
>  3 files changed, 87 insertions(+)
>  create mode 100755 tests/btrfs/236
>  create mode 100644 tests/btrfs/236.out
>
> diff --git a/tests/btrfs/236 b/tests/btrfs/236
> new file mode 100755
> index 000000000000..aac27fac06dd
> --- /dev/null
> +++ b/tests/btrfs/236
> @@ -0,0 +1,81 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 Oracle. All Rights Reserved.
> +#
> +# FS QA Test 236
> +#
> +# Check seed device integrity after fstrim on the sprout device.
> +#
> +#  Kernel bug is fixed by the commit:
> +#    btrfs: fix unmountable seed device after fstrim
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_command "$BTRFS_TUNE_PROG" btrfstune
> +_require_fstrim
> +_require_scratch_dev_pool 2
> +_scratch_dev_pool_get 2
> +
> +seed=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $1}')
> +sprout=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $2}')
> +
> +_mkfs_dev $seed
> +_mount $seed $SCRATCH_MNT
> +
> +$XFS_IO_PROG -f -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
> +_scratch_unmount
> +$BTRFS_TUNE_PROG -S 1 $seed
> +
> +# Mount the seed device and add the rw device
> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
> +md5sum $SCRATCH_MNT/foo | _filter_scratch
> +
> +$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
> +_scratch_unmount
> +
> +# Now remount writeable sprout device, create some data and run fstrim
> +_mount $sprout $SCRATCH_MNT
> +_require_batched_discard $SCRATCH_MNT
> +
> +$XFS_IO_PROG -f -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null

We aren't doing anything with this file ("bar"). Just remove it.

Otherwise it passes now with the filter.

Thanks.

> +
> +$FSTRIM_PROG $SCRATCH_MNT
> +
> +_scratch_unmount
> +
> +# Verify seed device is all ok
> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
> +md5sum $SCRATCH_MNT/foo | _filter_scratch
> +_scratch_unmount
> +
> +_check_btrfs_filesystem $seed
> +
> +_scratch_dev_pool_put
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/236.out b/tests/btrfs/236.out
> new file mode 100644
> index 000000000000..01699b8fc291
> --- /dev/null
> +++ b/tests/btrfs/236.out
> @@ -0,0 +1,5 @@
> +QA output created by 236
> +mount: device write-protected, mounting read-only
> +096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
> +mount: device write-protected, mounting read-only
> +096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 331dd432fac3..5032259244e0 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -238,3 +238,4 @@
>  233 auto quick subvolume
>  234 auto quick compress rw
>  235 auto quick send
> +236 auto quick seed trim
> --
> 2.27.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] btrfs: add fstrim test case on the sprout device

[Anand Jain]

On 03/05/2021 17:54, Filipe Manana wrote:
> On Sat, May 1, 2021 at 6:24 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> Add fstrim test case on the sprout device, verify seed device
>> integrity.
>>
>>   btrfs: fix unmountable seed device after fstrim
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2:
>>    Add _require_fstrim and _require_batched_discard.
>>    Use FSTRIM_PROG.
>>    Use _filter_ro_mount to handle the difference in output in different
>>       mount(8) version.
>>    Call _scratch_dev_pool_put.
>>    Add _check_btrfs_filesystem $seed to check the whole seed fs.
>>    Update in-code comments.
>>
>>   tests/btrfs/236     | 81 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/236.out |  5 +++
>>   tests/btrfs/group   |  1 +
>>   3 files changed, 87 insertions(+)
>>   create mode 100755 tests/btrfs/236
>>   create mode 100644 tests/btrfs/236.out
>>
>> diff --git a/tests/btrfs/236 b/tests/btrfs/236
>> new file mode 100755
>> index 000000000000..aac27fac06dd
>> --- /dev/null
>> +++ b/tests/btrfs/236
>> @@ -0,0 +1,81 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2021 Oracle. All Rights Reserved.
>> +#
>> +# FS QA Test 236
>> +#
>> +# Check seed device integrity after fstrim on the sprout device.
>> +#
>> +#  Kernel bug is fixed by the commit:
>> +#    btrfs: fix unmountable seed device after fstrim
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1       # failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +       cd /
>> +       rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs btrfs
>> +_require_command "$BTRFS_TUNE_PROG" btrfstune
>> +_require_fstrim
>> +_require_scratch_dev_pool 2
>> +_scratch_dev_pool_get 2
>> +
>> +seed=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $1}')
>> +sprout=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $2}')
>> +
>> +_mkfs_dev $seed
>> +_mount $seed $SCRATCH_MNT
>> +
>> +$XFS_IO_PROG -f -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
>> +_scratch_unmount
>> +$BTRFS_TUNE_PROG -S 1 $seed
>> +
>> +# Mount the seed device and add the rw device
>> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
>> +md5sum $SCRATCH_MNT/foo | _filter_scratch
>> +
>> +$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
>> +_scratch_unmount
>> +
>> +# Now remount writeable sprout device, create some data and run fstrim
>> +_mount $sprout $SCRATCH_MNT
>> +_require_batched_discard $SCRATCH_MNT
>> +
>> +$XFS_IO_PROG -f -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null
> 
> We aren't doing anything with this file ("bar"). Just remove it.
> 

  Right.  It can go.  I will fix it in v3.
Thanks.


> Otherwise it passes now with the filter.
> 
> Thanks.
> 
>> +
>> +$FSTRIM_PROG $SCRATCH_MNT
>> +
>> +_scratch_unmount
>> +
>> +# Verify seed device is all ok
>> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
>> +md5sum $SCRATCH_MNT/foo | _filter_scratch
>> +_scratch_unmount
>> +
>> +_check_btrfs_filesystem $seed
>> +
>> +_scratch_dev_pool_put
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/236.out b/tests/btrfs/236.out
>> new file mode 100644
>> index 000000000000..01699b8fc291
>> --- /dev/null
>> +++ b/tests/btrfs/236.out
>> @@ -0,0 +1,5 @@
>> +QA output created by 236
>> +mount: device write-protected, mounting read-only
>> +096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
>> +mount: device write-protected, mounting read-only
>> +096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index 331dd432fac3..5032259244e0 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -238,3 +238,4 @@
>>   233 auto quick subvolume
>>   234 auto quick compress rw
>>   235 auto quick send
>> +236 auto quick seed trim
>> --
>> 2.27.0
>>
> 
> 

[PATCH v3] btrfs: add fstrim test case on the sprout device

[Anand Jain]

Add fstrim test case on the sprout device, verify seed device
integrity.

 btrfs: fix unmountable seed device after fstrim

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3:
  Drop useless write into the sprout device as we are testing the effect of
     fstrim run in the sprout on the seed device.
v2:
  Add _require_fstrim and _require_batched_discard.
  Use FSTRIM_PROG.
  Use _filter_ro_mount to handle the difference in output in different
     mount(8) version.
  Call _scratch_dev_pool_put.
  Add _check_btrfs_filesystem $seed to check the whole seed fs.
  Update in-code comments.

 tests/btrfs/236     | 79 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/236.out |  5 +++
 tests/btrfs/group   |  1 +
 3 files changed, 85 insertions(+)
 create mode 100755 tests/btrfs/236
 create mode 100644 tests/btrfs/236.out

diff --git a/tests/btrfs/236 b/tests/btrfs/236
new file mode 100755
index 000000000000..1fcb3aab8c0c
--- /dev/null
+++ b/tests/btrfs/236
@@ -0,0 +1,79 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 Oracle. All Rights Reserved.
+#
+# FS QA Test 236
+#
+# Check seed device integrity after fstrim on the sprout device.
+#
+#  Kernel bug is fixed by the commit:
+#    btrfs: fix unmountable seed device after fstrim
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_command "$BTRFS_TUNE_PROG" btrfstune
+_require_fstrim
+_require_scratch_dev_pool 2
+_scratch_dev_pool_get 2
+
+seed=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $1}')
+sprout=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $2}')
+
+_mkfs_dev $seed
+_mount $seed $SCRATCH_MNT
+
+$XFS_IO_PROG -f -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
+_scratch_unmount
+$BTRFS_TUNE_PROG -S 1 $seed
+
+# Mount the seed device and add the rw device
+_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
+md5sum $SCRATCH_MNT/foo | _filter_scratch
+
+$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
+_scratch_unmount
+
+# Now remount writeable sprout device, create some data and run fstrim
+_mount $sprout $SCRATCH_MNT
+_require_batched_discard $SCRATCH_MNT
+
+$FSTRIM_PROG $SCRATCH_MNT
+
+_scratch_unmount
+
+# Verify seed device is all ok
+_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
+md5sum $SCRATCH_MNT/foo | _filter_scratch
+_scratch_unmount
+
+_check_btrfs_filesystem $seed
+
+_scratch_dev_pool_put
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/236.out b/tests/btrfs/236.out
new file mode 100644
index 000000000000..01699b8fc291
--- /dev/null
+++ b/tests/btrfs/236.out
@@ -0,0 +1,5 @@
+QA output created by 236
+mount: device write-protected, mounting read-only
+096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
+mount: device write-protected, mounting read-only
+096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
diff --git a/tests/btrfs/group b/tests/btrfs/group
index 331dd432fac3..5032259244e0 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -238,3 +238,4 @@
 233 auto quick subvolume
 234 auto quick compress rw
 235 auto quick send
+236 auto quick seed trim
-- 
2.27.0

Re: [PATCH v3] btrfs: add fstrim test case on the sprout device

[Filipe Manana]

On Mon, May 3, 2021 at 12:11 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> Add fstrim test case on the sprout device, verify seed device
> integrity.
>
>  btrfs: fix unmountable seed device after fstrim
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
> v3:
>   Drop useless write into the sprout device as we are testing the effect of
>      fstrim run in the sprout on the seed device.
> v2:
>   Add _require_fstrim and _require_batched_discard.
>   Use FSTRIM_PROG.
>   Use _filter_ro_mount to handle the difference in output in different
>      mount(8) version.
>   Call _scratch_dev_pool_put.
>   Add _check_btrfs_filesystem $seed to check the whole seed fs.
>   Update in-code comments.
>
>  tests/btrfs/236     | 79 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/236.out |  5 +++
>  tests/btrfs/group   |  1 +
>  3 files changed, 85 insertions(+)
>  create mode 100755 tests/btrfs/236
>  create mode 100644 tests/btrfs/236.out
>
> diff --git a/tests/btrfs/236 b/tests/btrfs/236
> new file mode 100755
> index 000000000000..1fcb3aab8c0c
> --- /dev/null
> +++ b/tests/btrfs/236
> @@ -0,0 +1,79 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 Oracle. All Rights Reserved.
> +#
> +# FS QA Test 236
> +#
> +# Check seed device integrity after fstrim on the sprout device.
> +#
> +#  Kernel bug is fixed by the commit:
> +#    btrfs: fix unmountable seed device after fstrim
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_command "$BTRFS_TUNE_PROG" btrfstune
> +_require_fstrim
> +_require_scratch_dev_pool 2
> +_scratch_dev_pool_get 2
> +
> +seed=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $1}')
> +sprout=$(echo $SCRATCH_DEV_POOL | $AWK_PROG '{print $2}')
> +
> +_mkfs_dev $seed
> +_mount $seed $SCRATCH_MNT
> +
> +$XFS_IO_PROG -f -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null
> +_scratch_unmount
> +$BTRFS_TUNE_PROG -S 1 $seed
> +
> +# Mount the seed device and add the rw device
> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
> +md5sum $SCRATCH_MNT/foo | _filter_scratch
> +
> +$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
> +_scratch_unmount
> +
> +# Now remount writeable sprout device, create some data and run fstrim
> +_mount $sprout $SCRATCH_MNT
> +_require_batched_discard $SCRATCH_MNT
> +
> +$FSTRIM_PROG $SCRATCH_MNT
> +
> +_scratch_unmount
> +
> +# Verify seed device is all ok
> +_mount $seed $SCRATCH_MNT 2>&1 | _filter_ro_mount | _filter_scratch
> +md5sum $SCRATCH_MNT/foo | _filter_scratch
> +_scratch_unmount
> +
> +_check_btrfs_filesystem $seed
> +
> +_scratch_dev_pool_put
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/236.out b/tests/btrfs/236.out
> new file mode 100644
> index 000000000000..01699b8fc291
> --- /dev/null
> +++ b/tests/btrfs/236.out
> @@ -0,0 +1,5 @@
> +QA output created by 236
> +mount: device write-protected, mounting read-only
> +096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
> +mount: device write-protected, mounting read-only
> +096003817ad2638000a6836e55866697  SCRATCH_MNT/foo
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 331dd432fac3..5032259244e0 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -238,3 +238,4 @@
>  233 auto quick subvolume
>  234 auto quick compress rw
>  235 auto quick send
> +236 auto quick seed trim
> --
> 2.27.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] btrfs: fix unmountable seed device after fstrim

[David Sterba]

On Fri, Apr 30, 2021 at 08:48:56PM +0800, Anand Jain wrote:
> On 30/4/21 8:14 pm, Filipe Manana wrote:
> > On Fri, Apr 30, 2021 at 1:03 PM Anand Jain <anand.jain@oracle.com> wrote:
> > 
> > The fix looks good. Don't feel forced to address the style comments
> > above, consider them more a recommendation for the future.
> > 
> 
> Yep. Thanks.
>   Also, I have missed the re-roll count and its log here.
>   I will just mention that here.
>      v2:
>         Fix commit changelog.
>         Drop a code comment.
> If David needs, I don't mind resending with these changes.

Not needed, I'm often adjusting changelog formatting so I'd do the
suggested fixups anyway. Patch added to misc-next, thanks.