[PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[Qu Wenruo]

[BUG]
The following sequence operation can lead to a seed fs rejected by
kernel:

 # Generate a fs with dirty log
 mkfs.btrfs -f $file
 mount $dev $mnt
 xfs_io -f -c "pwrite 0 16k" -c fsync $mnt/file
 cp $file $file.backup
 umount $mnt
 mv $file.backup $file

 # now $file has dirty log, set seed flag on it
 btrfstune -S1 $file

 # mount will fail
 mount $file $mnt

The mount failure with the following dmesg:

[  980.363667] loop0: detected capacity change from 0 to 262144
[  980.371177] BTRFS info (device loop0): flagging fs with big metadata feature
[  980.372229] BTRFS info (device loop0): using free space tree
[  980.372639] BTRFS info (device loop0): has skinny extents
[  980.375075] BTRFS info (device loop0): start tree-log replay
[  980.375513] BTRFS warning (device loop0): log replay required on RO media
[  980.381652] BTRFS error (device loop0): open_ctree failed

[CAUSE]
Although btrfs will replay its dirty log even with RO mount, but kernel
will treat seed device as RO device, and dirty log can not be replayed
on RO device.

This rejection is already the better end, just imagine if we don't treat
seed device as RO, and replayed the dirty log.
The filesystem relying on the seed device will be completely screwed up.

[FIX]
Just add extra check on log tree in btrfstune to reject setting seed
flag on filesystems with dirty log.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 btrfstune.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/btrfstune.c b/btrfstune.c
index 33c83bf16291..7e4ad30a1cbd 100644
--- a/btrfstune.c
+++ b/btrfstune.c
@@ -59,6 +59,10 @@ static int update_seeding_flag(struct btrfs_root *root, int set_flag)
 						device);
 			return 1;
 		}
+		if (btrfs_super_log_root(disk_super)) {
+			error("this filesystem has dirty log, can not set seed flag");
+			return 1;
+		}
 		super_flags |= BTRFS_SUPER_FLAG_SEEDING;
 	} else {
 		if (!(super_flags & BTRFS_SUPER_FLAG_SEEDING)) {
-- 
2.35.1

Re: [PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[Nikolay Borisov]

On 15.04.22 г. 14:37 ч., Qu Wenruo wrote:
> [BUG]
> The following sequence operation can lead to a seed fs rejected by
> kernel:
> 
>   # Generate a fs with dirty log
>   mkfs.btrfs -f $file
>   mount $dev $mnt
>   xfs_io -f -c "pwrite 0 16k" -c fsync $mnt/file
>   cp $file $file.backup
>   umount $mnt
>   mv $file.backup $file
> 
>   # now $file has dirty log, set seed flag on it
>   btrfstune -S1 $file
> 
>   # mount will fail
>   mount $file $mnt
> 
> The mount failure with the following dmesg:
> 
> [  980.363667] loop0: detected capacity change from 0 to 262144
> [  980.371177] BTRFS info (device loop0): flagging fs with big metadata feature
> [  980.372229] BTRFS info (device loop0): using free space tree
> [  980.372639] BTRFS info (device loop0): has skinny extents
> [  980.375075] BTRFS info (device loop0): start tree-log replay
> [  980.375513] BTRFS warning (device loop0): log replay required on RO media
> [  980.381652] BTRFS error (device loop0): open_ctree failed
> 
> [CAUSE]
> Although btrfs will replay its dirty log even with RO mount, but kernel
> will treat seed device as RO device, and dirty log can not be replayed
> on RO device.
> 
> This rejection is already the better end, just imagine if we don't treat
> seed device as RO, and replayed the dirty log.
> The filesystem relying on the seed device will be completely screwed up.
> 
> [FIX]
> Just add extra check on log tree in btrfstune to reject setting seed
> flag on filesystems with dirty log.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

LGTM:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

One minor nit below but it can be rectified by David at merge time. Why 
don't you also add a btrfs-progs test for this functionality.

> ---
>   btrfstune.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/btrfstune.c b/btrfstune.c
> index 33c83bf16291..7e4ad30a1cbd 100644
> --- a/btrfstune.c
> +++ b/btrfstune.c
> @@ -59,6 +59,10 @@ static int update_seeding_flag(struct btrfs_root *root, int set_flag)
>   						device);
>   			return 1;
>   		}
> +		if (btrfs_super_log_root(disk_super)) {
> +			error("this filesystem has dirty log, can not set seed flag");


nit: I'd probably put something less colloquial such as:

"Filesystem with dirty log detected, not setting seed flag"

> +			return 1;
> +		}
>   		super_flags |= BTRFS_SUPER_FLAG_SEEDING;
>   	} else {
>   		if (!(super_flags & BTRFS_SUPER_FLAG_SEEDING)) {

Re: [PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[Qu Wenruo]

On 2022/4/18 14:41, Nikolay Borisov wrote:
>
>
> On 15.04.22 г. 14:37 ч., Qu Wenruo wrote:
>> [BUG]
>> The following sequence operation can lead to a seed fs rejected by
>> kernel:
>>
>>   # Generate a fs with dirty log
>>   mkfs.btrfs -f $file
>>   mount $dev $mnt
>>   xfs_io -f -c "pwrite 0 16k" -c fsync $mnt/file
>>   cp $file $file.backup
>>   umount $mnt
>>   mv $file.backup $file
>>
>>   # now $file has dirty log, set seed flag on it
>>   btrfstune -S1 $file
>>
>>   # mount will fail
>>   mount $file $mnt
>>
>> The mount failure with the following dmesg:
>>
>> [  980.363667] loop0: detected capacity change from 0 to 262144
>> [  980.371177] BTRFS info (device loop0): flagging fs with big
>> metadata feature
>> [  980.372229] BTRFS info (device loop0): using free space tree
>> [  980.372639] BTRFS info (device loop0): has skinny extents
>> [  980.375075] BTRFS info (device loop0): start tree-log replay
>> [  980.375513] BTRFS warning (device loop0): log replay required on RO
>> media
>> [  980.381652] BTRFS error (device loop0): open_ctree failed
>>
>> [CAUSE]
>> Although btrfs will replay its dirty log even with RO mount, but kernel
>> will treat seed device as RO device, and dirty log can not be replayed
>> on RO device.
>>
>> This rejection is already the better end, just imagine if we don't treat
>> seed device as RO, and replayed the dirty log.
>> The filesystem relying on the seed device will be completely screwed up.
>>
>> [FIX]
>> Just add extra check on log tree in btrfstune to reject setting seed
>> flag on filesystems with dirty log.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> LGTM:
>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>
> One minor nit below but it can be rectified by David at merge time. Why
> don't you also add a btrfs-progs test for this functionality.

The major concern is, I'm using a very ugly way to create a dirty journal.

But I'm not sure if btrfs-progs really wants a complicated dm based
solution to do the same thing.

Or maybe I can just use an raw image for that?

Thanks,
Qu

>
>> ---
>>   btrfstune.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/btrfstune.c b/btrfstune.c
>> index 33c83bf16291..7e4ad30a1cbd 100644
>> --- a/btrfstune.c
>> +++ b/btrfstune.c
>> @@ -59,6 +59,10 @@ static int update_seeding_flag(struct btrfs_root
>> *root, int set_flag)
>>                           device);
>>               return 1;
>>           }
>> +        if (btrfs_super_log_root(disk_super)) {
>> +            error("this filesystem has dirty log, can not set seed
>> flag");
>
>
> nit: I'd probably put something less colloquial such as:
>
> "Filesystem with dirty log detected, not setting seed flag" >
>> +            return 1;
>> +        }
>>           super_flags |= BTRFS_SUPER_FLAG_SEEDING;
>>       } else {
>>           if (!(super_flags & BTRFS_SUPER_FLAG_SEEDING)) {

Re: [PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[Nikolay Borisov]

On 18.04.22 г. 9:44 ч., Qu Wenruo wrote:
> y way to create a dirty journal.
> 
> But I'm not sure if btrfs-progs really wants a complicated dm based
> solution to do the same thing.
> 
> Or maybe I can just use an raw image for that?


It doesn't matter what way you use to create the journal, what's 
important is that you have a dirty journal and verify a particular 
functionality involving it. You can either encode the steps to create 
the dirty journal image in the test itself, or simply provide an image 
with a dirty journal.

Re: [PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[Josef Bacik]

On Fri, Apr 15, 2022 at 07:37:43PM +0800, Qu Wenruo wrote:
> [BUG]
> The following sequence operation can lead to a seed fs rejected by
> kernel:
> 
>  # Generate a fs with dirty log
>  mkfs.btrfs -f $file
>  mount $dev $mnt
>  xfs_io -f -c "pwrite 0 16k" -c fsync $mnt/file
>  cp $file $file.backup
>  umount $mnt
>  mv $file.backup $file
> 
>  # now $file has dirty log, set seed flag on it
>  btrfstune -S1 $file
> 
>  # mount will fail
>  mount $file $mnt
> 
> The mount failure with the following dmesg:
> 
> [  980.363667] loop0: detected capacity change from 0 to 262144
> [  980.371177] BTRFS info (device loop0): flagging fs with big metadata feature
> [  980.372229] BTRFS info (device loop0): using free space tree
> [  980.372639] BTRFS info (device loop0): has skinny extents
> [  980.375075] BTRFS info (device loop0): start tree-log replay
> [  980.375513] BTRFS warning (device loop0): log replay required on RO media
> [  980.381652] BTRFS error (device loop0): open_ctree failed
> 
> [CAUSE]
> Although btrfs will replay its dirty log even with RO mount, but kernel
> will treat seed device as RO device, and dirty log can not be replayed
> on RO device.
> 
> This rejection is already the better end, just imagine if we don't treat
> seed device as RO, and replayed the dirty log.
> The filesystem relying on the seed device will be completely screwed up.
> 
> [FIX]
> Just add extra check on log tree in btrfstune to reject setting seed
> flag on filesystems with dirty log.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Can we get a progs test for this as well?

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[Qu Wenruo]

On 2022/4/18 23:30, Josef Bacik wrote:
> On Fri, Apr 15, 2022 at 07:37:43PM +0800, Qu Wenruo wrote:
>> [BUG]
>> The following sequence operation can lead to a seed fs rejected by
>> kernel:
>>
>>   # Generate a fs with dirty log
>>   mkfs.btrfs -f $file
>>   mount $dev $mnt
>>   xfs_io -f -c "pwrite 0 16k" -c fsync $mnt/file
>>   cp $file $file.backup
>>   umount $mnt
>>   mv $file.backup $file
>>
>>   # now $file has dirty log, set seed flag on it
>>   btrfstune -S1 $file
>>
>>   # mount will fail
>>   mount $file $mnt
>>
>> The mount failure with the following dmesg:
>>
>> [  980.363667] loop0: detected capacity change from 0 to 262144
>> [  980.371177] BTRFS info (device loop0): flagging fs with big metadata feature
>> [  980.372229] BTRFS info (device loop0): using free space tree
>> [  980.372639] BTRFS info (device loop0): has skinny extents
>> [  980.375075] BTRFS info (device loop0): start tree-log replay
>> [  980.375513] BTRFS warning (device loop0): log replay required on RO media
>> [  980.381652] BTRFS error (device loop0): open_ctree failed
>>
>> [CAUSE]
>> Although btrfs will replay its dirty log even with RO mount, but kernel
>> will treat seed device as RO device, and dirty log can not be replayed
>> on RO device.
>>
>> This rejection is already the better end, just imagine if we don't treat
>> seed device as RO, and replayed the dirty log.
>> The filesystem relying on the seed device will be completely screwed up.
>>
>> [FIX]
>> Just add extra check on log tree in btrfstune to reject setting seed
>> flag on filesystems with dirty log.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Can we get a progs test for this as well?

Thanks to the suggestion from Nik, I finally remember we can use raw 
image for fs with dirty journal.

Test case will come soon.

Thanks,
Qu
> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> 
> Thanks,
> 
> Josef
> 

Re: [PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[Anand Jain]

On 4/15/22 19:37, Qu Wenruo wrote:
> [BUG]
> The following sequence operation can lead to a seed fs rejected by
> kernel:
> 
>   # Generate a fs with dirty log
>   mkfs.btrfs -f $file
>   mount $dev $mnt
>   xfs_io -f -c "pwrite 0 16k" -c fsync $mnt/file
>   cp $file $file.backup
>   umount $mnt
>   mv $file.backup $file
> 
>   # now $file has dirty log, set seed flag on it
>   btrfstune -S1 $file
> 
>   # mount will fail
>   mount $file $mnt
> 
> The mount failure with the following dmesg:
> 
> [  980.363667] loop0: detected capacity change from 0 to 262144
> [  980.371177] BTRFS info (device loop0): flagging fs with big metadata feature
> [  980.372229] BTRFS info (device loop0): using free space tree
> [  980.372639] BTRFS info (device loop0): has skinny extents
> [  980.375075] BTRFS info (device loop0): start tree-log replay
> [  980.375513] BTRFS warning (device loop0): log replay required on RO media
> [  980.381652] BTRFS error (device loop0): open_ctree failed
> 
> [CAUSE]
> Although btrfs will replay its dirty log even with RO mount, but kernel
> will treat seed device as RO device, and dirty log can not be replayed
> on RO device.
> 
> This rejection is already the better end, just imagine if we don't treat
> seed device as RO, and replayed the dirty log.
> The filesystem relying on the seed device will be completely screwed up.
> 
> [FIX]
> Just add extra check on log tree in btrfstune to reject setting seed
> flag on filesystems with dirty log.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

LGTM.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

a small nit below.

> ---
>   btrfstune.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/btrfstune.c b/btrfstune.c
> index 33c83bf16291..7e4ad30a1cbd 100644
> --- a/btrfstune.c
> +++ b/btrfstune.c
> @@ -59,6 +59,10 @@ static int update_seeding_flag(struct btrfs_root *root, int set_flag)
>   						device);
>   			return 1;
>   		}
> +		if (btrfs_super_log_root(disk_super)) {
> +			error("this filesystem has dirty log, can not set seed flag");

Also, add a note on how to overcome dirty log. Mount / unmount?

Thanks, Anand

> +			return 1;
> +		}
>   		super_flags |= BTRFS_SUPER_FLAG_SEEDING;
>   	} else {
>   		if (!(super_flags & BTRFS_SUPER_FLAG_SEEDING)) {

Re: [PATCH] btrfs-progs: do not allow setting seed flag on fs with dirty log

[Qu Wenruo]

On 2022/4/19 20:07, Anand Jain wrote:
> On 4/15/22 19:37, Qu Wenruo wrote:
>> [BUG]
>> The following sequence operation can lead to a seed fs rejected by
>> kernel:
>>
>>   # Generate a fs with dirty log
>>   mkfs.btrfs -f $file
>>   mount $dev $mnt
>>   xfs_io -f -c "pwrite 0 16k" -c fsync $mnt/file
>>   cp $file $file.backup
>>   umount $mnt
>>   mv $file.backup $file
>>
>>   # now $file has dirty log, set seed flag on it
>>   btrfstune -S1 $file
>>
>>   # mount will fail
>>   mount $file $mnt
>>
>> The mount failure with the following dmesg:
>>
>> [  980.363667] loop0: detected capacity change from 0 to 262144
>> [  980.371177] BTRFS info (device loop0): flagging fs with big
>> metadata feature
>> [  980.372229] BTRFS info (device loop0): using free space tree
>> [  980.372639] BTRFS info (device loop0): has skinny extents
>> [  980.375075] BTRFS info (device loop0): start tree-log replay
>> [  980.375513] BTRFS warning (device loop0): log replay required on RO
>> media
>> [  980.381652] BTRFS error (device loop0): open_ctree failed
>>
>> [CAUSE]
>> Although btrfs will replay its dirty log even with RO mount, but kernel
>> will treat seed device as RO device, and dirty log can not be replayed
>> on RO device.
>>
>> This rejection is already the better end, just imagine if we don't treat
>> seed device as RO, and replayed the dirty log.
>> The filesystem relying on the seed device will be completely screwed up.
>>
>> [FIX]
>> Just add extra check on log tree in btrfstune to reject setting seed
>> flag on filesystems with dirty log.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> LGTM.
>
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
>
> a small nit below.
>
>> ---
>>   btrfstune.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/btrfstune.c b/btrfstune.c
>> index 33c83bf16291..7e4ad30a1cbd 100644
>> --- a/btrfstune.c
>> +++ b/btrfstune.c
>> @@ -59,6 +59,10 @@ static int update_seeding_flag(struct btrfs_root
>> *root, int set_flag)
>>                           device);
>>               return 1;
>>           }
>> +        if (btrfs_super_log_root(disk_super)) {
>> +            error("this filesystem has dirty log, can not set seed
>> flag");
>
> Also, add a note on how to overcome dirty log. Mount / unmount?

Mount/unmount or zero log.

I guess if a user has a dirty log (by itself is not that rare) and still
want to set seed flag on it (if the user doesn't understand what she/he
is doing, then it's a big problem), it's worthy to ask in the mailing
list already.

Thanks,
Qu
>
> Thanks, Anand
>
>> +            return 1;
>> +        }
>>           super_flags |= BTRFS_SUPER_FLAG_SEEDING;
>>       } else {
>>           if (!(super_flags & BTRFS_SUPER_FLAG_SEEDING)) {
>