[PATCH 1/3] btrfs: Continue write in case of can_not_nocow

[PATCH 1/3] btrfs: Continue write in case of can_not_nocow

[Zhao Lei]

btrfs failed in xfstests btrfs/080 with -o nodatacow.

Can be reproduced by following script:
  DEV=/dev/vdg
  MNT=/mnt/tmp

  umount $DEV &>/dev/null
  mkfs.btrfs -f $DEV
  mount -o nodatacow $DEV $MNT

  dd if=/dev/zero of=$MNT/test bs=1 count=2048 &
  btrfs subvolume snapshot -r $MNT $MNT/test_snap &
  wait
  --
  We can see dd failed on NO_SPACE.

Reason:
  __btrfs_buffered_write should run cow write when no_cow impossible,
  and current code is designed with above logic.
  But check_can_nocow() have 2 type of return value(0 and <0) on
  can_not_no_cow, and current code only continue write on first case,
  the second case happened in doing subvolume.

Fix:
  Continue write when check_can_nocow() return 0 and <0.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 fs/btrfs/file.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 977e715..11fd981 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1516,27 +1516,24 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
 
 		reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
 
-		if (BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
-					     BTRFS_INODE_PREALLOC)) {
-			ret = check_can_nocow(inode, pos, &write_bytes);
-			if (ret < 0)
-				break;
-			if (ret > 0) {
-				/*
-				 * For nodata cow case, no need to reserve
-				 * data space.
-				 */
-				only_release_metadata = true;
-				/*
-				 * our prealloc extent may be smaller than
-				 * write_bytes, so scale down.
-				 */
-				num_pages = DIV_ROUND_UP(write_bytes + offset,
-							 PAGE_CACHE_SIZE);
-				reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
-				goto reserve_metadata;
-			}
+		if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
+					      BTRFS_INODE_PREALLOC)) &&
+		    check_can_nocow(inode, pos, &write_bytes) > 0) {
+			/*
+			 * For nodata cow case, no need to reserve
+			 * data space.
+			 */
+			only_release_metadata = true;
+			/*
+			 * our prealloc extent may be smaller than
+			 * write_bytes, so scale down.
+			 */
+			num_pages = DIV_ROUND_UP(write_bytes + offset,
+						 PAGE_CACHE_SIZE);
+			reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
+			goto reserve_metadata;
 		}
+
 		ret = btrfs_check_data_free_space(inode, pos, write_bytes);
 		if (ret < 0)
 			break;
-- 
1.8.5.1

[PATCH 2/3] btrfs: delete no_used argument in btrfs_copy_from_user

[Zhao Lei]

size_t write_bytes is not necessary for btrfs_copy_from_user(),
delete it.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 fs/btrfs/file.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 11fd981..e4f287c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -406,8 +406,7 @@ int btrfs_run_defrag_inodes(struct btrfs_fs_info *fs_info)
 /* simple helper to fault in pages and copy.  This should go away
  * and be replaced with calls into generic code.
  */
-static noinline int btrfs_copy_from_user(loff_t pos, int num_pages,
-					 size_t write_bytes,
+static noinline int btrfs_copy_from_user(loff_t pos, size_t write_bytes,
 					 struct page **prepared_pages,
 					 struct iov_iter *i)
 {
@@ -1575,8 +1574,7 @@ again:
 			ret = 0;
 		}
 
-		copied = btrfs_copy_from_user(pos, num_pages,
-					   write_bytes, pages, i);
+		copied = btrfs_copy_from_user(pos, write_bytes, pages, i);
 
 		/*
 		 * if we have trouble faulting in the pages, fall
-- 
1.8.5.1

[PATCH 3/3] btrfs: merge functions for wait snapshot creation

[Zhao Lei]

wait_for_snapshot_creation() is in same group with oher two:
 btrfs_start_write_no_snapshoting()
 btrfs_end_write_no_snapshoting()

Rename wait_for_snapshot_creation() and move it into same place
with other two.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 fs/btrfs/ctree.h       |  1 +
 fs/btrfs/extent-tree.c | 20 ++++++++++++++++++++
 fs/btrfs/inode.c       | 22 +---------------------
 3 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0912f89..5d7a2c4d 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3566,6 +3566,7 @@ int btrfs_delayed_refs_qgroup_accounting(struct btrfs_trans_handle *trans,
 int __get_raid_index(u64 flags);
 int btrfs_start_write_no_snapshoting(struct btrfs_root *root);
 void btrfs_end_write_no_snapshoting(struct btrfs_root *root);
+void btrfs_wait_for_snapshot_creation(struct btrfs_root *root);
 void check_system_chunk(struct btrfs_trans_handle *trans,
 			struct btrfs_root *root,
 			const u64 type);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index dc3763a..e83ffa8 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -10704,3 +10704,23 @@ int btrfs_start_write_no_snapshoting(struct btrfs_root *root)
 	}
 	return 1;
 }
+
+static int wait_snapshoting_atomic_t(atomic_t *a)
+{
+	schedule();
+	return 0;
+}
+
+void btrfs_wait_for_snapshot_creation(struct btrfs_root *root)
+{
+	while (true) {
+		int ret;
+
+		ret = btrfs_start_write_no_snapshoting(root);
+		if (ret)
+			break;
+		wait_on_atomic_t(&root->will_be_snapshoted,
+				 wait_snapshoting_atomic_t,
+				 TASK_UNINTERRUPTIBLE);
+	}
+}
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 994490d..21d7cd1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4897,26 +4897,6 @@ next:
 	return err;
 }
 
-static int wait_snapshoting_atomic_t(atomic_t *a)
-{
-	schedule();
-	return 0;
-}
-
-static void wait_for_snapshot_creation(struct btrfs_root *root)
-{
-	while (true) {
-		int ret;
-
-		ret = btrfs_start_write_no_snapshoting(root);
-		if (ret)
-			break;
-		wait_on_atomic_t(&root->will_be_snapshoted,
-				 wait_snapshoting_atomic_t,
-				 TASK_UNINTERRUPTIBLE);
-	}
-}
-
 static int btrfs_setsize(struct inode *inode, struct iattr *attr)
 {
 	struct btrfs_root *root = BTRFS_I(inode)->root;
@@ -4948,7 +4928,7 @@ static int btrfs_setsize(struct inode *inode, struct iattr *attr)
 		 * truncation, it must capture all writes that happened before
 		 * this truncation.
 		 */
-		wait_for_snapshot_creation(root);
+		btrfs_wait_for_snapshot_creation(root);
 		ret = btrfs_cont_expand(inode, oldsize, newsize);
 		if (ret) {
 			btrfs_end_write_no_snapshoting(root);
-- 
1.8.5.1

Re: [PATCH 1/3] btrfs: Continue write in case of can_not_nocow

[Filipe Manana]

On Wed, Jan 6, 2016 at 11:00 AM, Zhao Lei <zhaolei@cn.fujitsu.com> wrote:
> btrfs failed in xfstests btrfs/080 with -o nodatacow.
>
> Can be reproduced by following script:
>   DEV=/dev/vdg
>   MNT=/mnt/tmp
>
>   umount $DEV &>/dev/null
>   mkfs.btrfs -f $DEV
>   mount -o nodatacow $DEV $MNT
>
>   dd if=/dev/zero of=$MNT/test bs=1 count=2048 &
>   btrfs subvolume snapshot -r $MNT $MNT/test_snap &
>   wait
>   --
>   We can see dd failed on NO_SPACE.
>
> Reason:
>   __btrfs_buffered_write should run cow write when no_cow impossible,
>   and current code is designed with above logic.
>   But check_can_nocow() have 2 type of return value(0 and <0) on
>   can_not_no_cow, and current code only continue write on first case,
>   the second case happened in doing subvolume.
>
> Fix:
>   Continue write when check_can_nocow() return 0 and <0.
>
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>

Could you please submit a test case for xfstests as well?
Thanks.

> ---
>  fs/btrfs/file.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 977e715..11fd981 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1516,27 +1516,24 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
>
>                 reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
>
> -               if (BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> -                                            BTRFS_INODE_PREALLOC)) {
> -                       ret = check_can_nocow(inode, pos, &write_bytes);
> -                       if (ret < 0)
> -                               break;
> -                       if (ret > 0) {
> -                               /*
> -                                * For nodata cow case, no need to reserve
> -                                * data space.
> -                                */
> -                               only_release_metadata = true;
> -                               /*
> -                                * our prealloc extent may be smaller than
> -                                * write_bytes, so scale down.
> -                                */
> -                               num_pages = DIV_ROUND_UP(write_bytes + offset,
> -                                                        PAGE_CACHE_SIZE);
> -                               reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
> -                               goto reserve_metadata;
> -                       }
> +               if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> +                                             BTRFS_INODE_PREALLOC)) &&
> +                   check_can_nocow(inode, pos, &write_bytes) > 0) {
> +                       /*
> +                        * For nodata cow case, no need to reserve
> +                        * data space.
> +                        */
> +                       only_release_metadata = true;
> +                       /*
> +                        * our prealloc extent may be smaller than
> +                        * write_bytes, so scale down.
> +                        */
> +                       num_pages = DIV_ROUND_UP(write_bytes + offset,
> +                                                PAGE_CACHE_SIZE);
> +                       reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
> +                       goto reserve_metadata;
>                 }
> +
>                 ret = btrfs_check_data_free_space(inode, pos, write_bytes);
>                 if (ret < 0)
>                         break;
> --
> 1.8.5.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



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

RE: [PATCH 1/3] btrfs: Continue write in case of can_not_nocow

[Zhao Lei]

Hi, Filipe Manana

> -----Original Message-----
> From: Filipe Manana [mailto:fdmanana@gmail.com]
> Sent: Thursday, January 21, 2016 4:49 AM
> To: Zhao Lei <zhaolei@cn.fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH 1/3] btrfs: Continue write in case of can_not_nocow
> 
> On Wed, Jan 6, 2016 at 11:00 AM, Zhao Lei <zhaolei@cn.fujitsu.com> wrote:
> > btrfs failed in xfstests btrfs/080 with -o nodatacow.
> >
> > Can be reproduced by following script:
> >   DEV=/dev/vdg
> >   MNT=/mnt/tmp
> >
> >   umount $DEV &>/dev/null
> >   mkfs.btrfs -f $DEV
> >   mount -o nodatacow $DEV $MNT
> >
> >   dd if=/dev/zero of=$MNT/test bs=1 count=2048 &
> >   btrfs subvolume snapshot -r $MNT $MNT/test_snap &
> >   wait
> >   --
> >   We can see dd failed on NO_SPACE.
> >
> > Reason:
> >   __btrfs_buffered_write should run cow write when no_cow impossible,
> >   and current code is designed with above logic.
> >   But check_can_nocow() have 2 type of return value(0 and <0) on
> >   can_not_no_cow, and current code only continue write on first case,
> >   the second case happened in doing subvolume.
> >
> > Fix:
> >   Continue write when check_can_nocow() return 0 and <0.
> >
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
Thanks for review.

> Could you please submit a test case for xfstests as well?

btrfs/080 with -o nodatacow already can trigger this bug.

Thanks
Zhaolei

> Thanks.
> 

> > ---
> >  fs/btrfs/file.c | 37 +++++++++++++++++--------------------
> >  1 file changed, 17 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 977e715..11fd981
> > 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1516,27 +1516,24 @@ static noinline ssize_t
> > __btrfs_buffered_write(struct file *file,
> >
> >                 reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
> >
> > -               if (BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> > -
> BTRFS_INODE_PREALLOC)) {
> > -                       ret = check_can_nocow(inode, pos,
> &write_bytes);
> > -                       if (ret < 0)
> > -                               break;
> > -                       if (ret > 0) {
> > -                               /*
> > -                                * For nodata cow case, no need to
> reserve
> > -                                * data space.
> > -                                */
> > -                               only_release_metadata = true;
> > -                               /*
> > -                                * our prealloc extent may be smaller
> than
> > -                                * write_bytes, so scale down.
> > -                                */
> > -                               num_pages =
> DIV_ROUND_UP(write_bytes + offset,
> > -
> PAGE_CACHE_SIZE);
> > -                               reserve_bytes = num_pages <<
> PAGE_CACHE_SHIFT;
> > -                               goto reserve_metadata;
> > -                       }
> > +               if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW
> |
> > +
> BTRFS_INODE_PREALLOC)) &&
> > +                   check_can_nocow(inode, pos, &write_bytes) > 0) {
> > +                       /*
> > +                        * For nodata cow case, no need to reserve
> > +                        * data space.
> > +                        */
> > +                       only_release_metadata = true;
> > +                       /*
> > +                        * our prealloc extent may be smaller than
> > +                        * write_bytes, so scale down.
> > +                        */
> > +                       num_pages = DIV_ROUND_UP(write_bytes +
> offset,
> > +
> PAGE_CACHE_SIZE);
> > +                       reserve_bytes = num_pages <<
> PAGE_CACHE_SHIFT;
> > +                       goto reserve_metadata;
> >                 }
> > +
> >                 ret = btrfs_check_data_free_space(inode, pos,
> write_bytes);
> >                 if (ret < 0)
> >                         break;
> > --
> > 1.8.5.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
> 
> 
> 
> --
> Filipe David Manana,
> 
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."

Re: [PATCH 1/3] btrfs: Continue write in case of can_not_nocow

[Roman Mamedov]

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

On Wed, 6 Jan 2016 19:00:17 +0800
Zhao Lei <zhaolei@cn.fujitsu.com> wrote:

> btrfs failed in xfstests btrfs/080 with -o nodatacow.
> 
> Can be reproduced by following script:
>   DEV=/dev/vdg
>   MNT=/mnt/tmp
> 
>   umount $DEV &>/dev/null
>   mkfs.btrfs -f $DEV
>   mount -o nodatacow $DEV $MNT
> 
>   dd if=/dev/zero of=$MNT/test bs=1 count=2048 &
>   btrfs subvolume snapshot -r $MNT $MNT/test_snap &
>   wait
>   --
>   We can see dd failed on NO_SPACE.
> 
> Reason:
>   __btrfs_buffered_write should run cow write when no_cow impossible,
>   and current code is designed with above logic.
>   But check_can_nocow() have 2 type of return value(0 and <0) on
>   can_not_no_cow, and current code only continue write on first case,
>   the second case happened in doing subvolume.
> 
> Fix:
>   Continue write when check_can_nocow() return 0 and <0.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>

Guys please don't forget about this patch. It solves real problem for people,
http://www.spinics.net/lists/linux-btrfs/msg51276.html
http://www.spinics.net/lists/linux-btrfs/msg51819.html
but it's not in 4.4.1, not in 4.4.2... and now not in 4.4.3

> ---
>  fs/btrfs/file.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 977e715..11fd981 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1516,27 +1516,24 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
>  
>  		reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
>  
> -		if (BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> -					     BTRFS_INODE_PREALLOC)) {
> -			ret = check_can_nocow(inode, pos, &write_bytes);
> -			if (ret < 0)
> -				break;
> -			if (ret > 0) {
> -				/*
> -				 * For nodata cow case, no need to reserve
> -				 * data space.
> -				 */
> -				only_release_metadata = true;
> -				/*
> -				 * our prealloc extent may be smaller than
> -				 * write_bytes, so scale down.
> -				 */
> -				num_pages = DIV_ROUND_UP(write_bytes + offset,
> -							 PAGE_CACHE_SIZE);
> -				reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
> -				goto reserve_metadata;
> -			}
> +		if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> +					      BTRFS_INODE_PREALLOC)) &&
> +		    check_can_nocow(inode, pos, &write_bytes) > 0) {
> +			/*
> +			 * For nodata cow case, no need to reserve
> +			 * data space.
> +			 */
> +			only_release_metadata = true;
> +			/*
> +			 * our prealloc extent may be smaller than
> +			 * write_bytes, so scale down.
> +			 */
> +			num_pages = DIV_ROUND_UP(write_bytes + offset,
> +						 PAGE_CACHE_SIZE);
> +			reserve_bytes = num_pages << PAGE_CACHE_SHIFT;
> +			goto reserve_metadata;
>  		}
> +
>  		ret = btrfs_check_data_free_space(inode, pos, write_bytes);
>  		if (ret < 0)
>  			break;


-- 
With respect,
Roman

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/3] btrfs: Continue write in case of can_not_nocow

[Chris Mason]

On Fri, Feb 26, 2016 at 10:41:31AM +0500, Roman Mamedov wrote:
> On Wed, 6 Jan 2016 19:00:17 +0800
> Zhao Lei <zhaolei@cn.fujitsu.com> wrote:
> 
> > btrfs failed in xfstests btrfs/080 with -o nodatacow.
> > 
> > Can be reproduced by following script:
> >   DEV=/dev/vdg
> >   MNT=/mnt/tmp
> > 
> >   umount $DEV &>/dev/null
> >   mkfs.btrfs -f $DEV
> >   mount -o nodatacow $DEV $MNT
> > 
> >   dd if=/dev/zero of=$MNT/test bs=1 count=2048 &
> >   btrfs subvolume snapshot -r $MNT $MNT/test_snap &
> >   wait
> >   --
> >   We can see dd failed on NO_SPACE.
> > 
> > Reason:
> >   __btrfs_buffered_write should run cow write when no_cow impossible,
> >   and current code is designed with above logic.
> >   But check_can_nocow() have 2 type of return value(0 and <0) on
> >   can_not_no_cow, and current code only continue write on first case,
> >   the second case happened in doing subvolume.
> > 
> > Fix:
> >   Continue write when check_can_nocow() return 0 and <0.
> > 
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> 
> Guys please don't forget about this patch. It solves real problem for people,
> http://www.spinics.net/lists/linux-btrfs/msg51276.html
> http://www.spinics.net/lists/linux-btrfs/msg51819.html
> but it's not in 4.4.1, not in 4.4.2... and now not in 4.4.3

Dave already has it queued for the next merge window.  Thanks!

-chris