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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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)) {