diff mbox

[1/6] Btrfs: fix a bug of per-file nocow

Message ID 1346391419-4278-2-git-send-email-bo.li.liu@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Liu Bo Aug. 31, 2012, 5:36 a.m. UTC
Users report a bug, the reproducer is:
$ mkfs.btrfs /dev/loop0
$ mount /dev/loop0 /mnt/btrfs/
$ mkdir /mnt/btrfs/dir
$ chattr +C /mnt/btrfs/dir/
$ dd if=/dev/zero of=/mnt/btrfs/dir/foo bs=4K count=10;
$ lsattr /mnt/btrfs/dir/foo
---------------C- /mnt/btrfs/dir/foo
$ filefrag /mnt/btrfs/dir/foo
/mnt/btrfs/dir/foo: 1 extent found    ---> an extent
$ dd if=/dev/zero of=/mnt/btrfs/dir/foo bs=4K count=1 seek=5 conv=notrunc,nocreat; sync
$ filefrag /mnt/btrfs/dir/foo
/mnt/btrfs/dir/foo: 3 extents found   ---> with nocow, btrfs breaks the extent into three parts

The new created file should not only inherit the NODATACOW flag, but also
honor NODATASUM flag, because we must do COW on a file extent with checksum.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/inode.c |    3 +--
 fs/btrfs/ioctl.c |    3 ++-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

David Sterba Sept. 6, 2012, 4:26 p.m. UTC | #1
On Fri, Aug 31, 2012 at 01:36:54PM +0800, Liu Bo wrote:
> The new created file should not only inherit the NODATACOW flag, but also
> honor NODATASUM flag, because we must do COW on a file extent with checksum.

I was not entierly correct when suggesting to move NODATASUM to
inherit_iflags. Se below.

> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/inode.c |    3 +--
>  fs/btrfs/ioctl.c |    3 ++-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b2c3514..9bce6e9 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4720,8 +4720,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
>  	if (S_ISREG(mode)) {

Originaly it applied to regular files only.

>  		if (btrfs_test_opt(root, NODATASUM))
>  			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
> -		if (btrfs_test_opt(root, NODATACOW) ||
> -		    (BTRFS_I(dir)->flags & BTRFS_INODE_NODATACOW))
> +		if (btrfs_test_opt(root, NODATACOW))
>  			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
>  	}
>  
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 00ddf22..7390b25 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -141,7 +141,8 @@ void btrfs_inherit_iflags(struct inode *inode, struct inode *dir)
>  	}
>  
>  	if (flags & BTRFS_INODE_NODATACOW)
> -		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
> +		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW |
> +					 BTRFS_INODE_NODATASUM;

But now this will happen to:

* nodes (mknod)
* directories -- NODATASUM for a dir does nothing, but does not feel
  right
* symlinks -- same
* subvolume roots -- same, and even more confusing what it does

While the fix you suggested in

http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg18292.html

is simple, I'm still inclined to keep all flag inheritance at one place,
so the fix I'm proposing is to do

>  	if (flags & BTRFS_INODE_NODATACOW)
>  		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;

		if (S_ISREG(inode->i_mode))
			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
	}

>  
>  	btrfs_update_iflags(inode);
>  }
--
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
diff mbox

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b2c3514..9bce6e9 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4720,8 +4720,7 @@  static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	if (S_ISREG(mode)) {
 		if (btrfs_test_opt(root, NODATASUM))
 			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
-		if (btrfs_test_opt(root, NODATACOW) ||
-		    (BTRFS_I(dir)->flags & BTRFS_INODE_NODATACOW))
+		if (btrfs_test_opt(root, NODATACOW))
 			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
 	}
 
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 00ddf22..7390b25 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -141,7 +141,8 @@  void btrfs_inherit_iflags(struct inode *inode, struct inode *dir)
 	}
 
 	if (flags & BTRFS_INODE_NODATACOW)
-		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW;
+		BTRFS_I(inode)->flags |= BTRFS_INODE_NODATACOW |
+					 BTRFS_INODE_NODATASUM;
 
 	btrfs_update_iflags(inode);
 }