Btrfs devel - Sep 2011 - [PATCH 1/2] Btrfs: don't make a file partly checksummed through file clone

To reproduce the bug:

  # mount /dev/sda7 /mnt
  # dd if=/dev/zero of=/mnt/src bs=4K count=1
  # umount /mnt

  # mount -o nodatasum /dev/sda7 /mnt
  # dd if=/dev/zero of=/mnt/dst bs=4K count=1
  # clone_range -s 4K -l 4K /mnt/src /mnt/dst

  # echo 3 > /proc/sys/vm/drop_caches
  # cat /mnt/dst
  # dmesg
  ...
  btrfs no csum found for inode 258 start 0
  btrfs csum failed ino 258 off 0 csum 2566472073 private 0

It''s because part of the file is checksummed and the other part is not,
and then btrfs will complain checksum is not found when we read the file.

Disallow file clone if src and dst file have different checksum flag,
so we ensure a file is completely checksummed or unchecksummed.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 970977a..dc82bbb 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2177,6 +2177,11 @@ static noinline long btrfs_ioctl_clone(struct file *file,
unsigned long srcfd,
 	if (!(src_file->f_mode & FMODE_READ))
 		goto out_fput;
 
+	/* don''t make the dst file partly checksummed */
+	if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) !+	   
(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM))
+		goto out_fput;
+
 	ret = -EISDIR;
 	if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode))
 		goto out_fput;
-- 1.7.3.1 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The dst file will have the same inode flags with dst file after
file clone, and I think it''s unexpected.

For example, the dst file will suddenly become immutable after
getting some share of data with src file, if the src is immutable.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index dc82bbb..a401514 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2434,7 +2434,6 @@ static noinline long btrfs_ioctl_clone(struct file *file,
unsigned long srcfd,
 			if (endoff > inode->i_size)
 				btrfs_i_size_write(inode, endoff);
 
-			BTRFS_I(inode)->flags = BTRFS_I(src)->flags;
 			ret = btrfs_update_inode(trans, root, inode);
 			BUG_ON(ret);
 			btrfs_end_transaction(trans, root);
-- 1.7.3.1 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 14 Sep 2011, Li Zefan wrote:
> To reproduce the bug:
> 
>   # mount /dev/sda7 /mnt
>   # dd if=/dev/zero of=/mnt/src bs=4K count=1
>   # umount /mnt
> 
>   # mount -o nodatasum /dev/sda7 /mnt
>   # dd if=/dev/zero of=/mnt/dst bs=4K count=1
>   # clone_range -s 4K -l 4K /mnt/src /mnt/dst
> 
>   # echo 3 > /proc/sys/vm/drop_caches
>   # cat /mnt/dst
>   # dmesg
>   ...
>   btrfs no csum found for inode 258 start 0
>   btrfs csum failed ino 258 off 0 csum 2566472073 private 0
> 
> It''s because part of the file is checksummed and the other part is
not,
> and then btrfs will complain checksum is not found when we read the file.
> 
> Disallow file clone if src and dst file have different checksum flag,
> so we ensure a file is completely checksummed or unchecksummed.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Looks good to me.

Reviewed-by: Sage Weil <sage@newdream.net>
> ---
>  fs/btrfs/ioctl.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 970977a..dc82bbb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2177,6 +2177,11 @@ static noinline long btrfs_ioctl_clone(struct file
*file, unsigned long srcfd,
>  	if (!(src_file->f_mode & FMODE_READ))
>  		goto out_fput;
>  
> +	/* don''t make the dst file partly checksummed */
> +	if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) !> +	   
(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM))
> +		goto out_fput;
> +
>  	ret = -EISDIR;
>  	if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode))
>  		goto out_fput;
> -- 1.7.3.1 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Sep 14, 2011 at 01:25:21PM +0800, Li Zefan
wrote:
> It''s because part of the file is checksummed and the other part is
not,
> and then btrfs will complain checksum is not found when we read the file.
> 
> Disallow file clone if src and dst file have different checksum flag,
> so we ensure a file is completely checksummed or unchecksummed.
Your fix prevents the bug, but I don''t think it''s good to let
file clone
fail without any other message. ret is set to -EINVAL at the time of
''goto out_fput'', which is fine, but the user has no clue what
happened
or how to fix it.

The nodatasum status is recorded in inode flags and remains like that
regardless of the ''mount -o nodatasum'', persistent and de
facto
unchangable (unless the file is created again with the opposite nodatasum
mount). Even more, the user has no way to find out nodatasum flag of
any inode/file (the corresponding FS_NODATASUM_FL is not there).

My suggestion how to fix this:
1. add FS_NODATASUM_FL file flag and code to set/get via setflags ioctl
2. [this patch to skip cloning in case of nodatasum flag mismatch]
3. ... add a printk why it failed

The user then has at least option to drop/add the nodatasum flag for one of
the. Unfortunatelly this makes file cloning less straightforward.


david

> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 970977a..dc82bbb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2177,6 +2177,11 @@ static noinline long btrfs_ioctl_clone(struct file
*file, unsigned long srcfd,
>  	if (!(src_file->f_mode & FMODE_READ))
>  		goto out_fput;
>  
> +	/* don''t make the dst file partly checksummed */
> +	if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) !> +	   
(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM))
> +		goto out_fput;
> +
>  	ret = -EISDIR;
>  	if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode))
>  		goto out_fput;
> -- 1.7.3.1 
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
> the body of a message to majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Sep 14, 2011 at 01:25:36PM +0800, Li Zefan
wrote:
> The dst file will have the same inode flags with dst file after
> file clone, and I think it''s unexpected.
> 
> For example, the dst file will suddenly become immutable after
> getting some share of data with src file, if the src is immutable.
Good catch! (I did not find any further direct assignment of two inode
flags.)
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Reviewed-by: David Sterba <dsterba@suse.cz>

IMNSHO should go to stable.


david
> ---
>  fs/btrfs/ioctl.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index dc82bbb..a401514 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2434,7 +2434,6 @@ static noinline long btrfs_ioctl_clone(struct file
*file, unsigned long srcfd,
>  			if (endoff > inode->i_size)
>  				btrfs_i_size_write(inode, endoff);
>  
> -			BTRFS_I(inode)->flags = BTRFS_I(src)->flags;
>  			ret = btrfs_update_inode(trans, root, inode);
>  			BUG_ON(ret);
>  			btrfs_end_transaction(trans, root);
> -- 1.7.3.1 
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
> the body of a message to majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba wrote:
> On Wed, Sep 14, 2011 at 01:25:21PM +0800, Li Zefan wrote:
>> It''s because part of the file is checksummed and the other
part is not,
>> and then btrfs will complain checksum is not found when we read the
file.
>>
>> Disallow file clone if src and dst file have different checksum flag,
>> so we ensure a file is completely checksummed or unchecksummed.
> 
> Your fix prevents the bug, but I don''t think it''s good to
let file clone
> fail without any other message. ret is set to -EINVAL at the time of
> ''goto out_fput'', which is fine, but the user has no clue
what happened
> or how to fix it.
While I agree with you on this comment..
> 
> The nodatasum status is recorded in inode flags and remains like that
> regardless of the ''mount -o nodatasum'', persistent and de
facto
> unchangable (unless the file is created again with the opposite nodatasum
> mount). Even more, the user has no way to find out nodatasum flag of
> any inode/file (the corresponding FS_NODATASUM_FL is not there).
> 
> My suggestion how to fix this:
> 1. add FS_NODATASUM_FL file flag and code to set/get via setflags ioctl
This means we can have a file partly checksummed, which is what we want
to avoid in this patch.
> 2. [this patch to skip cloning in case of nodatasum flag mismatch]
> 3. ... add a printk why it failed
I don''t think this is a good idea.
> 
> The user then has at least option to drop/add the nodatasum flag for one of
> the. Unfortunatelly this makes file cloning less straightforward.
> 
I don''t know if Chris has plan on finer-grained checksum (not per file
but per extent), if yes, we can eliminate this constraint in file cloning
in the future.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Add CC to Coreutils, cp --reflink performs btrfs clone operation.

Thanks,
-Jeff
On 09/15/2011 07:43 PM, David Sterba wrote:
> On Wed, Sep 14, 2011 at 01:25:36PM +0800, Li Zefan wrote:
>> The dst file will have the same inode flags with dst file after
>> file clone, and I think it''s unexpected.
>>
>> For example, the dst file will suddenly become immutable after
>> getting some share of data with src file, if the src is immutable.
> 
> Good catch! (I did not find any further direct assignment of two inode
> flags.)
> 
>>
>> Signed-off-by: Li Zefan
<lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Reviewed-by: David Sterba <dsterba-AlSwsSmVLrQ@public.gmane.org>
> 
> IMNSHO should go to stable.
> 
> 
> david
> 
>> ---
>>  fs/btrfs/ioctl.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index dc82bbb..a401514 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2434,7 +2434,6 @@ static noinline long btrfs_ioctl_clone(struct
file *file, unsigned long srcfd,
>>  			if (endoff > inode->i_size)
>>  				btrfs_i_size_write(inode, endoff);
>>  
>> -			BTRFS_I(inode)->flags = BTRFS_I(src)->flags;
>>  			ret = btrfs_update_inode(trans, root, inode);
>>  			BUG_ON(ret);
>>  			btrfs_end_transaction(trans, root);
>> -- 1.7.3.1 
>> --
>> To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
>> the body of a message to
majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html