Re: [PATCH] Btrfs-progs: convert: do not clear header rev

[Lu Fengqi]

On Fri, Jun 30, 2017 at 04:52:54PM +0200, David Sterba wrote:
>On Tue, Jun 27, 2017 at 12:07:13PM -0700, Liu Bo wrote:
>> On Mon, Jun 26, 2017 at 05:55:02PM -0600, Liu Bo wrote:
>> > So btrfs_set_header_flags() vs btrfs_set_header_flag, the difference is 
>> > sort of
>> > similar to "=" vs "|=", when creating and initialising a new extent buffer,
>> > convert uses the former one which clears header_rev by accident.
>> >
>> 
>> Here is the regression test script,
>> 
>> mkfs.ext4 -F /disk
>> btrfs-convert -d /disk
>> mount /disk /mnt
>> xfs_io -f -c "pwrite 0 4K" /mnt/foobar
>> umount /disk
>> 
>> ---
>> It'd end up with 'unable to find ref blabla" shown in dmesg log.
>
>I can't reproduce it, is there something else needed? Tested kernel is
>4.11.6. Check does not complain either.

Excuse me, is your btrfs-progs already applied this patch?

I'm sure that I can reproduce it in my test environment.
1. kernel 4.11.6
2. btrfs-progs kdave/devel(drop this patch)

-- 
Thanks,
Lu


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs-progs: convert: do not clear header rev

[David Sterba]

On Tue, Jun 27, 2017 at 12:07:13PM -0700, Liu Bo wrote:
> On Mon, Jun 26, 2017 at 05:55:02PM -0600, Liu Bo wrote:
> > So btrfs_set_header_flags() vs btrfs_set_header_flag, the difference is 
> > sort of
> > similar to "=" vs "|=", when creating and initialising a new extent buffer,
> > convert uses the former one which clears header_rev by accident.
> >
> 
> Here is the regression test script,
> 
> mkfs.ext4 -F /disk
> btrfs-convert -d /disk
> mount /disk /mnt
> xfs_io -f -c "pwrite 0 4K" /mnt/foobar
> umount /disk
> 
> ---
> It'd end up with 'unable to find ref blabla" shown in dmesg log.

I can't reproduce it, is there something else needed? Tested kernel is
4.11.6. Check does not complain either.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs-progs: convert: do not clear header rev

[Liu Bo]

On Mon, Jun 26, 2017 at 05:55:02PM -0600, Liu Bo wrote:
> So btrfs_set_header_flags() vs btrfs_set_header_flag, the difference is sort 
> of
> similar to "=" vs "|=", when creating and initialising a new extent buffer,
> convert uses the former one which clears header_rev by accident.
>

Here is the regression test script,

mkfs.ext4 -F /disk
btrfs-convert -d /disk
mount /disk /mnt
xfs_io -f -c "pwrite 0 4K" /mnt/foobar
umount /disk

---
It'd end up with 'unable to find ref blabla" shown in dmesg log.

thanks,
-liubo

> Signed-off-by: Liu Bo 
> ---
>  convert/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/convert/common.c b/convert/common.c
> index 40bf32c..f0dd2cf 100644
> --- a/convert/common.c
> +++ b/convert/common.c
> @@ -167,7 +167,7 @@ static int setup_temp_extent_buffer(struct extent_buffer 
> *buf,
>   btrfs_set_header_generation(buf, 1);
>   btrfs_set_header_backref_rev(buf, BTRFS_MIXED_BACKREF_REV);
>   btrfs_set_header_owner(buf, owner);
> - btrfs_set_header_flags(buf, BTRFS_HEADER_FLAG_WRITTEN);
> + btrfs_set_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN);
>   write_extent_buffer(buf, chunk_uuid, btrfs_header_chunk_tree_uuid(buf),
>   BTRFS_UUID_SIZE);
>   write_extent_buffer(buf, fsid, btrfs_header_fsid(), BTRFS_FSID_SIZE);
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs-progs: convert: do not clear header rev

[Qu Wenruo]

At 06/27/2017 07:55 AM, Liu Bo wrote:

So btrfs_set_header_flags() vs btrfs_set_header_flag, the difference is sort of
similar to "=" vs "|=", when creating and initialising a new extent buffer,
convert uses the former one which clears header_rev by accident.


Thanks for catching this one.

Reviewed-by: Qu Wenruo 

Thanks,
Qu


Signed-off-by: Liu Bo 
---
  convert/common.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/convert/common.c b/convert/common.c
index 40bf32c..f0dd2cf 100644
--- a/convert/common.c
+++ b/convert/common.c
@@ -167,7 +167,7 @@ static int setup_temp_extent_buffer(struct extent_buffer 
*buf,
btrfs_set_header_generation(buf, 1);
btrfs_set_header_backref_rev(buf, BTRFS_MIXED_BACKREF_REV);
btrfs_set_header_owner(buf, owner);
-   btrfs_set_header_flags(buf, BTRFS_HEADER_FLAG_WRITTEN);
+   btrfs_set_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN);
write_extent_buffer(buf, chunk_uuid, btrfs_header_chunk_tree_uuid(buf),
BTRFS_UUID_SIZE);
write_extent_buffer(buf, fsid, btrfs_header_fsid(), BTRFS_FSID_SIZE);




--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html