[PATCH v3 0/3] btrfs: allow btrfs_truncate_block() to fallback to nocow for data space reservation

[PATCH v3 0/3] btrfs: allow btrfs_truncate_block() to fallback to nocow for data space reservation

[Qu Wenruo]

Before this patch, btrfs_truncate_block() never checks the NODATACOW
bit, thus when we run out of data space, we can return ENOSPC for
truncate for NODATACOW inode.

This patchset will address this problem by doing the same behavior as
buffered write to address it.

Changelog:
v2:
- Rebased to misc-next
  Only one minor conflict in ctree.h

v3:
- Added two new patches
- Refactor check_can_nocow()
  Since the introduction of nowait, check_can_nocow() are in fact split
  into two usage patterns: check_can_nocow(nowait = false) with
  btrfs_drew_write_unlock(), and single check_can_nocow(nowait = true).
  Refactor them into two functions: start_nocow_check() paired with
  end_nocow_check(), and single try_nocow_check(). With comment added.

- Rebased to latest misc-next

- Added btrfs_assert_drew_write_locked() for btrfs_end_nocow_check()
  This is a little concerning one, as it's in the hot path of buffered
  write.
  It has percpu_counter_sum() called in that hot path, causing
  obvious performance drop for CONFIG_BTRFS_DEBUG build.
  Not sure if the assert is worthy since there aren't any other users.

Qu Wenruo (3):
  btrfs: add comments for check_can_nocow() and can_nocow_extent()
  btrfs: refactor check_can_nocow() into two variants
  btrfs: allow btrfs_truncate_block() to fallback to nocow for data
    space reservation

 fs/btrfs/ctree.h   |  3 +++
 fs/btrfs/file.c    | 52 +++++++++++++++++++++++++++++++++------
 fs/btrfs/inode.c   | 61 ++++++++++++++++++++++++++++++++++++++++------
 fs/btrfs/locking.h | 13 ++++++++++
 4 files changed, 113 insertions(+), 16 deletions(-)

-- 
2.27.0

[PATCH v3 1/3] btrfs: add comments for check_can_nocow() and can_nocow_extent()

[Qu Wenruo]

These two functions have extra conditions that their callers need to
meet, and some not-that-common parameters used for return value.

So adding some comments may save reviewers some time.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file.c  | 19 +++++++++++++++++++
 fs/btrfs/inode.c | 19 +++++++++++++++++--
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index fccf5862cd3e..0e4f57fb2737 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1533,6 +1533,25 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages,
 	return ret;
 }
 
+/*
+ * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
+ *
+ * This function will flush ordered extents in the range to ensure proper
+ * nocow checks for (nowait == false) case.
+ *
+ * Return >0 and update @write_bytes if we can do nocow write into the range.
+ * Return 0 if we can't do nocow write.
+ * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case,
+ * there are ordered extents need to be flushed.
+ * Return <0 for if other error happened.
+ *
+ * NOTE: For wait (nowait==false) calls, callers need to release the drew write
+ * 	 lock of inode->root->snapshot_lock if return value > 0.
+ *
+ * @pos:	 File offset of the range
+ * @write_bytes: The length of the range to check, also contains the nocow
+ * 		 writable length if we can do nocow write
+ */
 static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
 				    size_t *write_bytes, bool nowait)
 {
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 86f7aa377da9..48e16eae7278 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6922,8 +6922,23 @@ static struct extent_map *btrfs_new_extent_direct(struct inode *inode,
 }
 
 /*
- * returns 1 when the nocow is safe, < 1 on error, 0 if the
- * block must be cow'd
+ * Check if we can write into [@offset, @offset + @len) of @inode.
+ *
+ * Return >0 and update @len if we can do nocow write into [@offset, @offset +
+ * @len).
+ * Return 0 if we can't do nocow write.
+ * Return <0 if error happened.
+ *
+ * NOTE: This only checks the file extents, caller is responsible to wait for
+ *	 any ordered extents.
+ *
+ * @offset:	File offset
+ * @len:	The length to write, will be updated to the nocow writable
+ * 		range
+ *
+ * @orig_start:	(Optional) Return the original file offset of the file extent
+ * @orig_len:	(Optional) Return the original on-disk length of the file extent
+ * @ram_bytes:	(Optional) Return the ram_bytes of the file extent
  */
 noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
 			      u64 *orig_start, u64 *orig_block_len,
-- 
2.27.0

[PATCH v3 2/3] btrfs: refactor check_can_nocow() into two variants

[Qu Wenruo]

The function check_can_nocow() now has two completely different call
patterns.
For nowait variant, callers don't need to do any cleanup.
While for wait variant, callers need to release the lock if they can do
nocow write.

This is somehow confusing, and will be a problem if check_can_nocow()
get exported.

So this patch will separate the different patterns into different
functions.
For nowait variant, the function will be called try_nocow_check().
For wait variant, the function pair will be start_nocow_check() and
end_nocow_check().

Also, adds btrfs_assert_drew_write_locked() for end_nocow_check() to
detected unpaired calls.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file.c    | 71 ++++++++++++++++++++++++++++------------------
 fs/btrfs/locking.h | 13 +++++++++
 2 files changed, 57 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 0e4f57fb2737..7c904e41c5b6 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1533,27 +1533,8 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages,
 	return ret;
 }
 
-/*
- * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
- *
- * This function will flush ordered extents in the range to ensure proper
- * nocow checks for (nowait == false) case.
- *
- * Return >0 and update @write_bytes if we can do nocow write into the range.
- * Return 0 if we can't do nocow write.
- * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case,
- * there are ordered extents need to be flushed.
- * Return <0 for if other error happened.
- *
- * NOTE: For wait (nowait==false) calls, callers need to release the drew write
- * 	 lock of inode->root->snapshot_lock if return value > 0.
- *
- * @pos:	 File offset of the range
- * @write_bytes: The length of the range to check, also contains the nocow
- * 		 writable length if we can do nocow write
- */
-static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
-				    size_t *write_bytes, bool nowait)
+static noinline int __check_can_nocow(struct btrfs_inode *inode, loff_t pos,
+				      size_t *write_bytes, bool nowait)
 {
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 	struct btrfs_root *root = inode->root;
@@ -1603,6 +1584,43 @@ static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
 	return ret;
 }
 
+/*
+ * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
+ *
+ * The start_nocow_check() version will flush ordered extents before checking,
+ * and needs end_nocow_check() calls if we can do nocow writes.
+ *
+ * While try_nocow_check() version won't do any sleep or hold any lock, thus
+ * no need to call end_nocow_check().
+ *
+ * Return >0 and update @write_bytes if we can do nocow write into the range.
+ * Return 0 if we can't do nocow write.
+ * Return -EAGAIN if we can't get the needed lock, or there are ordered extents
+ * needs to be flushed.
+ * Return <0 for if other error happened.
+ *
+ * @pos:	 File offset of the range
+ * @write_bytes: The length of the range to check, also contains the nocow
+ * 		 writable length if we can do nocow write
+ */
+static int start_nocow_check(struct btrfs_inode *inode, loff_t pos,
+			     size_t *write_bytes)
+{
+	return __check_can_nocow(inode, pos, write_bytes, false);
+}
+
+static int try_nocow_check(struct btrfs_inode *inode, loff_t pos,
+			   size_t *write_bytes)
+{
+	return __check_can_nocow(inode, pos, write_bytes, true);
+}
+
+static void end_nocow_check(struct btrfs_inode *inode)
+{
+	btrfs_assert_drew_write_locked(&inode->root->snapshot_lock);
+	btrfs_drew_write_unlock(&inode->root->snapshot_lock);
+}
+
 static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 					       struct iov_iter *i)
 {
@@ -1668,8 +1686,8 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 		if (ret < 0) {
 			if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
 						      BTRFS_INODE_PREALLOC)) &&
-			    check_can_nocow(BTRFS_I(inode), pos,
-					    &write_bytes, false) > 0) {
+			    start_nocow_check(BTRFS_I(inode), pos,
+				    	      &write_bytes) > 0) {
 				/*
 				 * For nodata cow case, no need to reserve
 				 * data space.
@@ -1802,7 +1820,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 
 		release_bytes = 0;
 		if (only_release_metadata)
-			btrfs_drew_write_unlock(&root->snapshot_lock);
+			end_nocow_check(BTRFS_I(inode));
 
 		if (only_release_metadata && copied > 0) {
 			lockstart = round_down(pos,
@@ -1829,7 +1847,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 
 	if (release_bytes) {
 		if (only_release_metadata) {
-			btrfs_drew_write_unlock(&root->snapshot_lock);
+			end_nocow_check(BTRFS_I(inode));
 			btrfs_delalloc_release_metadata(BTRFS_I(inode),
 					release_bytes, true);
 		} else {
@@ -1946,8 +1964,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 		 */
 		if (!(BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
 					      BTRFS_INODE_PREALLOC)) ||
-		    check_can_nocow(BTRFS_I(inode), pos, &nocow_bytes,
-				    true) <= 0) {
+		    try_nocow_check(BTRFS_I(inode), pos, &nocow_bytes) <= 0) {
 			inode_unlock(inode);
 			return -EAGAIN;
 		}
diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
index d715846c10b8..28995fccafde 100644
--- a/fs/btrfs/locking.h
+++ b/fs/btrfs/locking.h
@@ -68,4 +68,17 @@ void btrfs_drew_write_unlock(struct btrfs_drew_lock *lock);
 void btrfs_drew_read_lock(struct btrfs_drew_lock *lock);
 void btrfs_drew_read_unlock(struct btrfs_drew_lock *lock);
 
+#ifdef CONFIG_BTRFS_DEBUG
+static inline void btrfs_assert_drew_write_locked(struct btrfs_drew_lock *lock)
+{
+	/* If there are readers, we're definitely not write locked */
+	BUG_ON(atomic_read(&lock->readers));
+	/* We should hold one percpu count, thus the value shouldn't be zero */
+	BUG_ON(percpu_counter_sum(&lock->writers) <= 0);
+}
+#else
+static inline void btrfs_assert_drew_write_locked(struct btrfs_drew_lock *lock)
+{
+}
+#endif
 #endif
-- 
2.27.0

[PATCH v3 3/3] btrfs: allow btrfs_truncate_block() to fallback to nocow for data space reservation

[Qu Wenruo]

[BUG]
When the data space is exhausted, even the inode has NOCOW attribute,
btrfs will still refuse to truncate unaligned range due to ENOSPC.

The following script can reproduce it pretty easily:
  #!/bin/bash

  dev=/dev/test/test
  mnt=/mnt/btrfs

  umount $dev &> /dev/null
  umount $mnt&> /dev/null

  mkfs.btrfs -f $dev -b 1G
  mount -o nospace_cache $dev $mnt
  touch $mnt/foobar
  chattr +C $mnt/foobar

  xfs_io -f -c "pwrite -b 4k 0 4k" $mnt/foobar > /dev/null
  xfs_io -f -c "pwrite -b 4k 0 1G" $mnt/padding &> /dev/null
  sync

  xfs_io -c "fpunch 0 2k" $mnt/foobar
  umount $mnt

Current btrfs will fail at the fpunch part.

[CAUSE]
Because btrfs_truncate_block() always reserve space without checking the
NOCOW attribute.

Since the writeback path follows NOCOW bit, we only need to bother the
space reservation code in btrfs_truncate_block().

[FIX]
Make btrfs_truncate_block() to follow btrfs_buffered_write() to try to
reserve data space first, and falls back to NOCOW check only when we
don't have enough space.

Such always-try-reserve is an optimization introduced in
btrfs_buffered_write(), to avoid expensive btrfs_check_can_nocow() call.

This patch will use btrfs_start_nocow_check() to do the same check in
btrfs_buffered_write() to fix the problem.

Reported-by: Martin Doucha <martin.doucha@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h |  3 +++
 fs/btrfs/file.c  | 14 +++++++-------
 fs/btrfs/inode.c | 42 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index d8301bf240e0..61ca99423b51 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3035,6 +3035,9 @@ int btrfs_dirty_pages(struct inode *inode, struct page **pages,
 		      size_t num_pages, loff_t pos, size_t write_bytes,
 		      struct extent_state **cached);
 int btrfs_fdatawrite_range(struct inode *inode, loff_t start, loff_t end);
+int btrfs_start_nocow_check(struct btrfs_inode *inode, loff_t pos,
+			    size_t *write_bytes);
+void btrfs_end_nocow_check(struct btrfs_inode *inode);
 
 /* tree-defrag.c */
 int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 7c904e41c5b6..2a9b1a2860e9 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1603,8 +1603,8 @@ static noinline int __check_can_nocow(struct btrfs_inode *inode, loff_t pos,
  * @write_bytes: The length of the range to check, also contains the nocow
  * 		 writable length if we can do nocow write
  */
-static int start_nocow_check(struct btrfs_inode *inode, loff_t pos,
-			     size_t *write_bytes)
+int btrfs_start_nocow_check(struct btrfs_inode *inode, loff_t pos,
+			    size_t *write_bytes)
 {
 	return __check_can_nocow(inode, pos, write_bytes, false);
 }
@@ -1615,7 +1615,7 @@ static int try_nocow_check(struct btrfs_inode *inode, loff_t pos,
 	return __check_can_nocow(inode, pos, write_bytes, true);
 }
 
-static void end_nocow_check(struct btrfs_inode *inode)
+void btrfs_end_nocow_check(struct btrfs_inode *inode)
 {
 	btrfs_assert_drew_write_locked(&inode->root->snapshot_lock);
 	btrfs_drew_write_unlock(&inode->root->snapshot_lock);
@@ -1686,8 +1686,8 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 		if (ret < 0) {
 			if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
 						      BTRFS_INODE_PREALLOC)) &&
-			    start_nocow_check(BTRFS_I(inode), pos,
-				    	      &write_bytes) > 0) {
+			    btrfs_start_nocow_check(BTRFS_I(inode), pos,
+						    &write_bytes) > 0) {
 				/*
 				 * For nodata cow case, no need to reserve
 				 * data space.
@@ -1820,7 +1820,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 
 		release_bytes = 0;
 		if (only_release_metadata)
-			end_nocow_check(BTRFS_I(inode));
+			btrfs_end_nocow_check(BTRFS_I(inode));
 
 		if (only_release_metadata && copied > 0) {
 			lockstart = round_down(pos,
@@ -1847,7 +1847,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 
 	if (release_bytes) {
 		if (only_release_metadata) {
-			end_nocow_check(BTRFS_I(inode));
+			btrfs_end_nocow_check(BTRFS_I(inode));
 			btrfs_delalloc_release_metadata(BTRFS_I(inode),
 					release_bytes, true);
 		} else {
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 48e16eae7278..ef636b193227 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4519,11 +4519,13 @@ int btrfs_truncate_block(struct inode *inode, loff_t from, loff_t len,
 	struct extent_state *cached_state = NULL;
 	struct extent_changeset *data_reserved = NULL;
 	char *kaddr;
+	bool only_release_metadata = false;
 	u32 blocksize = fs_info->sectorsize;
 	pgoff_t index = from >> PAGE_SHIFT;
 	unsigned offset = from & (blocksize - 1);
 	struct page *page;
 	gfp_t mask = btrfs_alloc_write_mask(mapping);
+	size_t write_bytes = blocksize;
 	int ret = 0;
 	u64 block_start;
 	u64 block_end;
@@ -4535,10 +4537,26 @@ int btrfs_truncate_block(struct inode *inode, loff_t from, loff_t len,
 	block_start = round_down(from, blocksize);
 	block_end = block_start + blocksize - 1;
 
-	ret = btrfs_delalloc_reserve_space(inode, &data_reserved,
-					   block_start, blocksize);
-	if (ret)
+	ret = btrfs_check_data_free_space(inode, &data_reserved, block_start,
+					  blocksize);
+	if (ret < 0) {
+		if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
+					      BTRFS_INODE_PREALLOC)) &&
+		    btrfs_start_nocow_check(BTRFS_I(inode), block_start,
+					    &write_bytes) > 0) {
+			/* For nocow case, no need to reserve data space. */
+			only_release_metadata = true;
+		} else {
+			goto out;
+		}
+	}
+	ret = btrfs_delalloc_reserve_metadata(BTRFS_I(inode), blocksize);
+	if (ret < 0) {
+		if (!only_release_metadata)
+			btrfs_free_reserved_data_space(inode, data_reserved,
+					block_start, blocksize);
 		goto out;
+	}
 
 again:
 	page = find_or_create_page(mapping, index, mask);
@@ -4608,14 +4626,26 @@ int btrfs_truncate_block(struct inode *inode, loff_t from, loff_t len,
 	set_page_dirty(page);
 	unlock_extent_cached(io_tree, block_start, block_end, &cached_state);
 
+	if (only_release_metadata)
+		set_extent_bit(&BTRFS_I(inode)->io_tree, block_start,
+				block_end, EXTENT_NORESERVE, NULL, NULL,
+				GFP_NOFS);
+
 out_unlock:
-	if (ret)
-		btrfs_delalloc_release_space(inode, data_reserved, block_start,
-					     blocksize, true);
+	if (ret) {
+		if (!only_release_metadata)
+			btrfs_delalloc_release_space(inode, data_reserved,
+					block_start, blocksize, true);
+		else
+			btrfs_delalloc_release_metadata(BTRFS_I(inode),
+					blocksize, true);
+	}
 	btrfs_delalloc_release_extents(BTRFS_I(inode), blocksize);
 	unlock_page(page);
 	put_page(page);
 out:
+	if (only_release_metadata)
+		btrfs_end_nocow_check(BTRFS_I(inode));
 	extent_changeset_free(data_reserved);
 	return ret;
 }
-- 
2.27.0

Re: [PATCH v3 1/3] btrfs: add comments for check_can_nocow() and can_nocow_extent()

[Johannes Thumshirn]

On 18/06/2020 09:50, Qu Wenruo wrote:
> These two functions have extra conditions that their callers need to
> meet, and some not-that-common parameters used for return value.
> 
> So adding some comments may save reviewers some time.

These comments have a style/formatting similar to kerneldoc, can you make
them real kerneldoc?

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file.c  | 19 +++++++++++++++++++
>  fs/btrfs/inode.c | 19 +++++++++++++++++--
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index fccf5862cd3e..0e4f57fb2737 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1533,6 +1533,25 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages,
>  	return ret;
>  }
>  
> +/*
> + * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
> + *
> + * This function will flush ordered extents in the range to ensure proper
> + * nocow checks for (nowait == false) case.
> + *
> + * Return >0 and update @write_bytes if we can do nocow write into the range.
> + * Return 0 if we can't do nocow write.
> + * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case,
> + * there are ordered extents need to be flushed.
> + * Return <0 for if other error happened.
> + *
> + * NOTE: For wait (nowait==false) calls, callers need to release the drew write
> + * 	 lock of inode->root->snapshot_lock if return value > 0.
> + *
> + * @pos:	 File offset of the range
> + * @write_bytes: The length of the range to check, also contains the nocow
> + * 		 writable length if we can do nocow write
> + */
>  static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
>  				    size_t *write_bytes, bool nowait)
>  {
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 86f7aa377da9..48e16eae7278 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6922,8 +6922,23 @@ static struct extent_map *btrfs_new_extent_direct(struct inode *inode,
>  }
>  
>  /*
> - * returns 1 when the nocow is safe, < 1 on error, 0 if the
> - * block must be cow'd
> + * Check if we can write into [@offset, @offset + @len) of @inode.
> + *
> + * Return >0 and update @len if we can do nocow write into [@offset, @offset +
> + * @len).
> + * Return 0 if we can't do nocow write.
> + * Return <0 if error happened.
> + *
> + * NOTE: This only checks the file extents, caller is responsible to wait for
> + *	 any ordered extents.
> + *
> + * @offset:	File offset
> + * @len:	The length to write, will be updated to the nocow writable
> + * 		range
> + *
> + * @orig_start:	(Optional) Return the original file offset of the file extent
> + * @orig_len:	(Optional) Return the original on-disk length of the file extent
> + * @ram_bytes:	(Optional) Return the ram_bytes of the file extent
>   */
>  noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
>  			      u64 *orig_start, u64 *orig_block_len,
> 

Re: [PATCH v3 1/3] btrfs: add comments for check_can_nocow() and can_nocow_extent()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3302 bytes --]



On 2020/6/18 下午7:57, Johannes Thumshirn wrote:
> On 18/06/2020 09:50, Qu Wenruo wrote:
>> These two functions have extra conditions that their callers need to
>> meet, and some not-that-common parameters used for return value.
>>
>> So adding some comments may save reviewers some time.
> 
> These comments have a style/formatting similar to kerneldoc, can you make
> them real kerneldoc?

Mind to give an example? It must be a coincident to match some existing
format.

Thanks,
Qu

> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/file.c  | 19 +++++++++++++++++++
>>  fs/btrfs/inode.c | 19 +++++++++++++++++--
>>  2 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index fccf5862cd3e..0e4f57fb2737 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -1533,6 +1533,25 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages,
>>  	return ret;
>>  }
>>  
>> +/*
>> + * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
>> + *
>> + * This function will flush ordered extents in the range to ensure proper
>> + * nocow checks for (nowait == false) case.
>> + *
>> + * Return >0 and update @write_bytes if we can do nocow write into the range.
>> + * Return 0 if we can't do nocow write.
>> + * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case,
>> + * there are ordered extents need to be flushed.
>> + * Return <0 for if other error happened.
>> + *
>> + * NOTE: For wait (nowait==false) calls, callers need to release the drew write
>> + * 	 lock of inode->root->snapshot_lock if return value > 0.
>> + *
>> + * @pos:	 File offset of the range
>> + * @write_bytes: The length of the range to check, also contains the nocow
>> + * 		 writable length if we can do nocow write
>> + */
>>  static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
>>  				    size_t *write_bytes, bool nowait)
>>  {
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 86f7aa377da9..48e16eae7278 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -6922,8 +6922,23 @@ static struct extent_map *btrfs_new_extent_direct(struct inode *inode,
>>  }
>>  
>>  /*
>> - * returns 1 when the nocow is safe, < 1 on error, 0 if the
>> - * block must be cow'd
>> + * Check if we can write into [@offset, @offset + @len) of @inode.
>> + *
>> + * Return >0 and update @len if we can do nocow write into [@offset, @offset +
>> + * @len).
>> + * Return 0 if we can't do nocow write.
>> + * Return <0 if error happened.
>> + *
>> + * NOTE: This only checks the file extents, caller is responsible to wait for
>> + *	 any ordered extents.
>> + *
>> + * @offset:	File offset
>> + * @len:	The length to write, will be updated to the nocow writable
>> + * 		range
>> + *
>> + * @orig_start:	(Optional) Return the original file offset of the file extent
>> + * @orig_len:	(Optional) Return the original on-disk length of the file extent
>> + * @ram_bytes:	(Optional) Return the ram_bytes of the file extent
>>   */
>>  noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
>>  			      u64 *orig_start, u64 *orig_block_len,
>>
> 


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

Re: [PATCH v3 2/3] btrfs: refactor check_can_nocow() into two variants

[Johannes Thumshirn]

On 18/06/2020 09:50, Qu Wenruo wrote:
> The function check_can_nocow() now has two completely different call
> patterns.
> For nowait variant, callers don't need to do any cleanup.
> While for wait variant, callers need to release the lock if they can do
> nocow write.
> 
> This is somehow confusing, and will be a problem if check_can_nocow()
> get exported.
> 
> So this patch will separate the different patterns into different
> functions.
> For nowait variant, the function will be called try_nocow_check().
> For wait variant, the function pair will be start_nocow_check() and
> end_nocow_check().

I find that try_ naming uneasy. If you take the example from locking 
for instance, after a successful mutex_trylock() you still need to call
mutex_unlock().

Maybe star_nowcow_check_unlocked() or start_nowcow_check_nowait() [though
the latter could imply it's putting things on a waitqueue] 

> 
> Also, adds btrfs_assert_drew_write_locked() for end_nocow_check() to
> detected unpaired calls.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file.c    | 71 ++++++++++++++++++++++++++++------------------
>  fs/btrfs/locking.h | 13 +++++++++
>  2 files changed, 57 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 0e4f57fb2737..7c904e41c5b6 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1533,27 +1533,8 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages,
>  	return ret;
>  }
>  
> -/*
> - * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
> - *
> - * This function will flush ordered extents in the range to ensure proper
> - * nocow checks for (nowait == false) case.
> - *
> - * Return >0 and update @write_bytes if we can do nocow write into the range.
> - * Return 0 if we can't do nocow write.
> - * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case,
> - * there are ordered extents need to be flushed.
> - * Return <0 for if other error happened.
> - *
> - * NOTE: For wait (nowait==false) calls, callers need to release the drew write
> - * 	 lock of inode->root->snapshot_lock if return value > 0.
> - *
> - * @pos:	 File offset of the range
> - * @write_bytes: The length of the range to check, also contains the nocow
> - * 		 writable length if we can do nocow write
> - */
> -static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
> -				    size_t *write_bytes, bool nowait)
> +static noinline int __check_can_nocow(struct btrfs_inode *inode, loff_t pos,
> +				      size_t *write_bytes, bool nowait)
>  {
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>  	struct btrfs_root *root = inode->root;
> @@ -1603,6 +1584,43 @@ static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
>  	return ret;
>  }
>  
> +/*
> + * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
> + *
> + * The start_nocow_check() version will flush ordered extents before checking,
> + * and needs end_nocow_check() calls if we can do nocow writes.
> + *
> + * While try_nocow_check() version won't do any sleep or hold any lock, thus
> + * no need to call end_nocow_check().
> + *
> + * Return >0 and update @write_bytes if we can do nocow write into the range.
> + * Return 0 if we can't do nocow write.
> + * Return -EAGAIN if we can't get the needed lock, or there are ordered extents
> + * needs to be flushed.
> + * Return <0 for if other error happened.
> + *
> + * @pos:	 File offset of the range
> + * @write_bytes: The length of the range to check, also contains the nocow
> + * 		 writable length if we can do nocow write
> + */
> +static int start_nocow_check(struct btrfs_inode *inode, loff_t pos,
> +			     size_t *write_bytes)
> +{
> +	return __check_can_nocow(inode, pos, write_bytes, false);
> +}
> +
> +static int try_nocow_check(struct btrfs_inode *inode, loff_t pos,
> +			   size_t *write_bytes)
> +{
> +	return __check_can_nocow(inode, pos, write_bytes, true);
> +}
> +
> +static void end_nocow_check(struct btrfs_inode *inode)
> +{
> +	btrfs_assert_drew_write_locked(&inode->root->snapshot_lock);
> +	btrfs_drew_write_unlock(&inode->root->snapshot_lock);
> +}
> +
>  static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>  					       struct iov_iter *i)
>  {
> @@ -1668,8 +1686,8 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>  		if (ret < 0) {
>  			if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
>  						      BTRFS_INODE_PREALLOC)) &&
> -			    check_can_nocow(BTRFS_I(inode), pos,
> -					    &write_bytes, false) > 0) {
> +			    start_nocow_check(BTRFS_I(inode), pos,
> +				    	      &write_bytes) > 0) {
>  				/*
>  				 * For nodata cow case, no need to reserve
>  				 * data space.
> @@ -1802,7 +1820,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>  
>  		release_bytes = 0;
>  		if (only_release_metadata)
> -			btrfs_drew_write_unlock(&root->snapshot_lock);
> +			end_nocow_check(BTRFS_I(inode));
>  
>  		if (only_release_metadata && copied > 0) {
>  			lockstart = round_down(pos,
> @@ -1829,7 +1847,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>  
>  	if (release_bytes) {
>  		if (only_release_metadata) {
> -			btrfs_drew_write_unlock(&root->snapshot_lock);
> +			end_nocow_check(BTRFS_I(inode));
>  			btrfs_delalloc_release_metadata(BTRFS_I(inode),
>  					release_bytes, true);
>  		} else {
> @@ -1946,8 +1964,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
>  		 */
>  		if (!(BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
>  					      BTRFS_INODE_PREALLOC)) ||
> -		    check_can_nocow(BTRFS_I(inode), pos, &nocow_bytes,
> -				    true) <= 0) {
> +		    try_nocow_check(BTRFS_I(inode), pos, &nocow_bytes) <= 0) {
>  			inode_unlock(inode);
>  			return -EAGAIN;
>  		}
> diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
> index d715846c10b8..28995fccafde 100644
> --- a/fs/btrfs/locking.h
> +++ b/fs/btrfs/locking.h
> @@ -68,4 +68,17 @@ void btrfs_drew_write_unlock(struct btrfs_drew_lock *lock);
>  void btrfs_drew_read_lock(struct btrfs_drew_lock *lock);
>  void btrfs_drew_read_unlock(struct btrfs_drew_lock *lock);
>  
> +#ifdef CONFIG_BTRFS_DEBUG
> +static inline void btrfs_assert_drew_write_locked(struct btrfs_drew_lock *lock)
> +{
> +	/* If there are readers, we're definitely not write locked */
> +	BUG_ON(atomic_read(&lock->readers));
> +	/* We should hold one percpu count, thus the value shouldn't be zero */
> +	BUG_ON(percpu_counter_sum(&lock->writers) <= 0);
> +}
> +#else
> +static inline void btrfs_assert_drew_write_locked(struct btrfs_drew_lock *lock)
> +{
> +}
> +#endif
>  #endif
> 

Re: [PATCH v3 2/3] btrfs: refactor check_can_nocow() into two variants

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 7260 bytes --]



On 2020/6/18 下午8:05, Johannes Thumshirn wrote:
> On 18/06/2020 09:50, Qu Wenruo wrote:
>> The function check_can_nocow() now has two completely different call
>> patterns.
>> For nowait variant, callers don't need to do any cleanup.
>> While for wait variant, callers need to release the lock if they can do
>> nocow write.
>>
>> This is somehow confusing, and will be a problem if check_can_nocow()
>> get exported.
>>
>> So this patch will separate the different patterns into different
>> functions.
>> For nowait variant, the function will be called try_nocow_check().
>> For wait variant, the function pair will be start_nocow_check() and
>> end_nocow_check().
> 
> I find that try_ naming uneasy. If you take the example from locking 
> for instance, after a successful mutex_trylock() you still need to call
> mutex_unlock().

Yep, I have the same concern, although no good alternative naming.
> 
> Maybe star_nowcow_check_unlocked() or start_nowcow_check_nowait() [though
> the latter could imply it's putting things on a waitqueue] 

unlocked() looks better.

Will wait for some other suggestions.

Thanks,
Qu

> 
>>
>> Also, adds btrfs_assert_drew_write_locked() for end_nocow_check() to
>> detected unpaired calls.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/file.c    | 71 ++++++++++++++++++++++++++++------------------
>>  fs/btrfs/locking.h | 13 +++++++++
>>  2 files changed, 57 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index 0e4f57fb2737..7c904e41c5b6 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -1533,27 +1533,8 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages,
>>  	return ret;
>>  }
>>  
>> -/*
>> - * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
>> - *
>> - * This function will flush ordered extents in the range to ensure proper
>> - * nocow checks for (nowait == false) case.
>> - *
>> - * Return >0 and update @write_bytes if we can do nocow write into the range.
>> - * Return 0 if we can't do nocow write.
>> - * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case,
>> - * there are ordered extents need to be flushed.
>> - * Return <0 for if other error happened.
>> - *
>> - * NOTE: For wait (nowait==false) calls, callers need to release the drew write
>> - * 	 lock of inode->root->snapshot_lock if return value > 0.
>> - *
>> - * @pos:	 File offset of the range
>> - * @write_bytes: The length of the range to check, also contains the nocow
>> - * 		 writable length if we can do nocow write
>> - */
>> -static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
>> -				    size_t *write_bytes, bool nowait)
>> +static noinline int __check_can_nocow(struct btrfs_inode *inode, loff_t pos,
>> +				      size_t *write_bytes, bool nowait)
>>  {
>>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>>  	struct btrfs_root *root = inode->root;
>> @@ -1603,6 +1584,43 @@ static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
>>  	return ret;
>>  }
>>  
>> +/*
>> + * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
>> + *
>> + * The start_nocow_check() version will flush ordered extents before checking,
>> + * and needs end_nocow_check() calls if we can do nocow writes.
>> + *
>> + * While try_nocow_check() version won't do any sleep or hold any lock, thus
>> + * no need to call end_nocow_check().
>> + *
>> + * Return >0 and update @write_bytes if we can do nocow write into the range.
>> + * Return 0 if we can't do nocow write.
>> + * Return -EAGAIN if we can't get the needed lock, or there are ordered extents
>> + * needs to be flushed.
>> + * Return <0 for if other error happened.
>> + *
>> + * @pos:	 File offset of the range
>> + * @write_bytes: The length of the range to check, also contains the nocow
>> + * 		 writable length if we can do nocow write
>> + */
>> +static int start_nocow_check(struct btrfs_inode *inode, loff_t pos,
>> +			     size_t *write_bytes)
>> +{
>> +	return __check_can_nocow(inode, pos, write_bytes, false);
>> +}
>> +
>> +static int try_nocow_check(struct btrfs_inode *inode, loff_t pos,
>> +			   size_t *write_bytes)
>> +{
>> +	return __check_can_nocow(inode, pos, write_bytes, true);
>> +}
>> +
>> +static void end_nocow_check(struct btrfs_inode *inode)
>> +{
>> +	btrfs_assert_drew_write_locked(&inode->root->snapshot_lock);
>> +	btrfs_drew_write_unlock(&inode->root->snapshot_lock);
>> +}
>> +
>>  static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>>  					       struct iov_iter *i)
>>  {
>> @@ -1668,8 +1686,8 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>>  		if (ret < 0) {
>>  			if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
>>  						      BTRFS_INODE_PREALLOC)) &&
>> -			    check_can_nocow(BTRFS_I(inode), pos,
>> -					    &write_bytes, false) > 0) {
>> +			    start_nocow_check(BTRFS_I(inode), pos,
>> +				    	      &write_bytes) > 0) {
>>  				/*
>>  				 * For nodata cow case, no need to reserve
>>  				 * data space.
>> @@ -1802,7 +1820,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>>  
>>  		release_bytes = 0;
>>  		if (only_release_metadata)
>> -			btrfs_drew_write_unlock(&root->snapshot_lock);
>> +			end_nocow_check(BTRFS_I(inode));
>>  
>>  		if (only_release_metadata && copied > 0) {
>>  			lockstart = round_down(pos,
>> @@ -1829,7 +1847,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>>  
>>  	if (release_bytes) {
>>  		if (only_release_metadata) {
>> -			btrfs_drew_write_unlock(&root->snapshot_lock);
>> +			end_nocow_check(BTRFS_I(inode));
>>  			btrfs_delalloc_release_metadata(BTRFS_I(inode),
>>  					release_bytes, true);
>>  		} else {
>> @@ -1946,8 +1964,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
>>  		 */
>>  		if (!(BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
>>  					      BTRFS_INODE_PREALLOC)) ||
>> -		    check_can_nocow(BTRFS_I(inode), pos, &nocow_bytes,
>> -				    true) <= 0) {
>> +		    try_nocow_check(BTRFS_I(inode), pos, &nocow_bytes) <= 0) {
>>  			inode_unlock(inode);
>>  			return -EAGAIN;
>>  		}
>> diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
>> index d715846c10b8..28995fccafde 100644
>> --- a/fs/btrfs/locking.h
>> +++ b/fs/btrfs/locking.h
>> @@ -68,4 +68,17 @@ void btrfs_drew_write_unlock(struct btrfs_drew_lock *lock);
>>  void btrfs_drew_read_lock(struct btrfs_drew_lock *lock);
>>  void btrfs_drew_read_unlock(struct btrfs_drew_lock *lock);
>>  
>> +#ifdef CONFIG_BTRFS_DEBUG
>> +static inline void btrfs_assert_drew_write_locked(struct btrfs_drew_lock *lock)
>> +{
>> +	/* If there are readers, we're definitely not write locked */
>> +	BUG_ON(atomic_read(&lock->readers));
>> +	/* We should hold one percpu count, thus the value shouldn't be zero */
>> +	BUG_ON(percpu_counter_sum(&lock->writers) <= 0);
>> +}
>> +#else
>> +static inline void btrfs_assert_drew_write_locked(struct btrfs_drew_lock *lock)
>> +{
>> +}
>> +#endif
>>  #endif
>>
> 


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

Re: [PATCH v3 1/3] btrfs: add comments for check_can_nocow() and can_nocow_extent()

[Johannes Thumshirn]

On 18/06/2020 14:00, Qu Wenruo wrote:
> 
> 
> On 2020/6/18 下午7:57, Johannes Thumshirn wrote:
>> On 18/06/2020 09:50, Qu Wenruo wrote:
>>> These two functions have extra conditions that their callers need to
>>> meet, and some not-that-common parameters used for return value.
>>>
>>> So adding some comments may save reviewers some time.
>>
>> These comments have a style/formatting similar to kerneldoc, can you make
>> them real kerneldoc?
> 
> Mind to give an example? It must be a coincident to match some existing
> format.
> 

Sure:

/**
 * check_can_nocow() - Check if we can write into [@offset, @offset + @len) of @inode.
 *
 * @offset:	File offset
 * @len:	The length to write, will be updated to the nocow writable
 * 		range
 * @orig_start:	Return the original file offset of the file extent (Optional)
 * @orig_len:	Return the original on-disk length of the file extent (Optional)
 * @ram_bytes:	Return the ram_bytes of the file extent (Optional)
 *
 * Return: >0 and update @len if we can do nocow write into [@offset, @offset + @len),
 * 0 if we can't do nocow write or <0 if error happened.
 *
 * NOTE: This only checks the file extents, caller is responsible to wait for
 *	 any ordered extents.
 *
 */

See also Documentation/doc-guide/kernel-doc.rst for a detailed description

Thanks,
	Johannes

Re: [PATCH v3 2/3] btrfs: refactor check_can_nocow() into two variants

[Johannes Thumshirn]

On 18/06/2020 14:09, Qu Wenruo wrote:
>> I find that try_ naming uneasy. If you take the example from locking 
>> for instance, after a successful mutex_trylock() you still need to call
>> mutex_unlock().
> 
> Yep, I have the same concern, although no good alternative naming.

Yeah, there's only two hard things in computer science, cache coherency, 
naming things and off-by-one errors.

Re: [PATCH v3 2/3] btrfs: refactor check_can_nocow() into two variants

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]



On 2020/6/18 下午8:16, Johannes Thumshirn wrote:
> On 18/06/2020 14:09, Qu Wenruo wrote:
>>> I find that try_ naming uneasy. If you take the example from locking 
>>> for instance, after a successful mutex_trylock() you still need to call
>>> mutex_unlock().
>>
>> Yep, I have the same concern, although no good alternative naming.
> 
> Yeah, there's only two hard things in computer science, cache coherency, 
> naming things and off-by-one errors.
> 
Best meme of the day!


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

Re: [PATCH v3 1/3] btrfs: add comments for check_can_nocow() and can_nocow_extent()

[David Sterba]

On Thu, Jun 18, 2020 at 12:14:00PM +0000, Johannes Thumshirn wrote:
> On 18/06/2020 14:00, Qu Wenruo wrote:
> > 
> > 
> > On 2020/6/18 下午7:57, Johannes Thumshirn wrote:
> >> On 18/06/2020 09:50, Qu Wenruo wrote:
> >>> These two functions have extra conditions that their callers need to
> >>> meet, and some not-that-common parameters used for return value.
> >>>
> >>> So adding some comments may save reviewers some time.
> >>
> >> These comments have a style/formatting similar to kerneldoc, can you make
> >> them real kerneldoc?
> > 
> > Mind to give an example? It must be a coincident to match some existing
> > format.
> > 
> 
> Sure:
> 
> /**
>  * check_can_nocow() - Check if we can write into [@offset, @offset + @len) of @inode.
>  *
>  * @offset:	File offset
>  * @len:	The length to write, will be updated to the nocow writable
>  * 		range
>  * @orig_start:	Return the original file offset of the file extent (Optional)
>  * @orig_len:	Return the original on-disk length of the file extent (Optional)
>  * @ram_bytes:	Return the ram_bytes of the file extent (Optional)
>  *
>  * Return: >0 and update @len if we can do nocow write into [@offset, @offset + @len),
>  * 0 if we can't do nocow write or <0 if error happened.
>  *
>  * NOTE: This only checks the file extents, caller is responsible to wait for
>  *	 any ordered extents.
>  *
>  */
> 
> See also Documentation/doc-guide/kernel-doc.rst for a detailed description

The kernel doc is relatively good, but for our internal helpers I see
much point using it and keep the function comment style more relaxed.

The short summary, argument description, long description and return
values. No strict formatting of the parameters but the "@name: text"
looks ok.

Re: [PATCH v3 2/3] btrfs: refactor check_can_nocow() into two variants

[David Sterba]

On Thu, Jun 18, 2020 at 03:49:49PM +0800, Qu Wenruo wrote:
> The function check_can_nocow() now has two completely different call
> patterns.
> For nowait variant, callers don't need to do any cleanup.
> While for wait variant, callers need to release the lock if they can do
> nocow write.
> 
> This is somehow confusing, and will be a problem if check_can_nocow()
> get exported.
> 
> So this patch will separate the different patterns into different
> functions.
> For nowait variant, the function will be called try_nocow_check().
> For wait variant, the function pair will be start_nocow_check() and
> end_nocow_check().
> 
> Also, adds btrfs_assert_drew_write_locked() for end_nocow_check() to
> detected unpaired calls.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file.c    | 71 ++++++++++++++++++++++++++++------------------
>  fs/btrfs/locking.h | 13 +++++++++
>  2 files changed, 57 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 0e4f57fb2737..7c904e41c5b6 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1533,27 +1533,8 @@ lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct page **pages,
>  	return ret;
>  }
>  
> -/*
> - * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
> - *
> - * This function will flush ordered extents in the range to ensure proper
> - * nocow checks for (nowait == false) case.
> - *
> - * Return >0 and update @write_bytes if we can do nocow write into the range.
> - * Return 0 if we can't do nocow write.
> - * Return -EAGAIN if we can't get the needed lock, or for (nowait == true) case,
> - * there are ordered extents need to be flushed.
> - * Return <0 for if other error happened.
> - *
> - * NOTE: For wait (nowait==false) calls, callers need to release the drew write
> - * 	 lock of inode->root->snapshot_lock if return value > 0.
> - *
> - * @pos:	 File offset of the range
> - * @write_bytes: The length of the range to check, also contains the nocow
> - * 		 writable length if we can do nocow write
> - */
> -static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
> -				    size_t *write_bytes, bool nowait)
> +static noinline int __check_can_nocow(struct btrfs_inode *inode, loff_t pos,
> +				      size_t *write_bytes, bool nowait)

This is another patch where you add a function with __, for no apparent
reason. We want to get rid of this naming, not introduce more. There's a
limited number of valid uses for the underscored functions.

>  {
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>  	struct btrfs_root *root = inode->root;
> @@ -1603,6 +1584,43 @@ static noinline int check_can_nocow(struct btrfs_inode *inode, loff_t pos,
>  	return ret;
>  }
>  
> +/*
> + * Check if we can do nocow write into the range [@pos, @pos + @write_bytes)
> + *
> + * The start_nocow_check() version will flush ordered extents before checking,
> + * and needs end_nocow_check() calls if we can do nocow writes.
> + *
> + * While try_nocow_check() version won't do any sleep or hold any lock, thus
> + * no need to call end_nocow_check().
> + *
> + * Return >0 and update @write_bytes if we can do nocow write into the range.
> + * Return 0 if we can't do nocow write.
> + * Return -EAGAIN if we can't get the needed lock, or there are ordered extents
> + * needs to be flushed.
> + * Return <0 for if other error happened.
> + *
> + * @pos:	 File offset of the range
> + * @write_bytes: The length of the range to check, also contains the nocow
> + * 		 writable length if we can do nocow write
> + */
> +static int start_nocow_check(struct btrfs_inode *inode, loff_t pos,
> +			     size_t *write_bytes)
> +{
> +	return __check_can_nocow(inode, pos, write_bytes, false);
> +}
> +
> +static int try_nocow_check(struct btrfs_inode *inode, loff_t pos,
> +			   size_t *write_bytes)
> +{
> +	return __check_can_nocow(inode, pos, write_bytes, true);
> +}
> +
> +static void end_nocow_check(struct btrfs_inode *inode)
> +{
> +	btrfs_assert_drew_write_locked(&inode->root->snapshot_lock);
> +	btrfs_drew_write_unlock(&inode->root->snapshot_lock);
> +}

The helpers that make the sections more visible are a good idea,
start/end, the locking is still not apparent from the name but at least
the pairing can be made more visible.

> +
>  static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>  					       struct iov_iter *i)
>  {
> @@ -1668,8 +1686,8 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
>  		if (ret < 0) {
>  			if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
>  						      BTRFS_INODE_PREALLOC)) &&
> -			    check_can_nocow(BTRFS_I(inode), pos,
> -					    &write_bytes, false) > 0) {
> +			    start_nocow_check(BTRFS_I(inode), pos,
> +				    	      &write_bytes) > 0) {

I was suggesting to push the BTRFS_INODE_NODATACOW and
BTRFS_INODE_PREALLOC checks down to the helper as all call sites do the
check. It's easier to reason about the condition.

Re: [PATCH v3 0/3] btrfs: allow btrfs_truncate_block() to fallback to nocow for data space reservation

[David Sterba]

On Thu, Jun 18, 2020 at 03:49:47PM +0800, Qu Wenruo wrote:
> v3:
> - Added two new patches
> - Refactor check_can_nocow()
>   Since the introduction of nowait, check_can_nocow() are in fact split
>   into two usage patterns: check_can_nocow(nowait = false) with
>   btrfs_drew_write_unlock(), and single check_can_nocow(nowait = true).
>   Refactor them into two functions: start_nocow_check() paired with
>   end_nocow_check(), and single try_nocow_check(). With comment added.
> 
> - Rebased to latest misc-next
> 
> - Added btrfs_assert_drew_write_locked() for btrfs_end_nocow_check()
>   This is a little concerning one, as it's in the hot path of buffered
>   write.
>   It has percpu_counter_sum() called in that hot path, causing
>   obvious performance drop for CONFIG_BTRFS_DEBUG build.
>   Not sure if the assert is worthy since there aren't any other users.
> 
> Qu Wenruo (3):
>   btrfs: add comments for check_can_nocow() and can_nocow_extent()
>   btrfs: refactor check_can_nocow() into two variants
>   btrfs: allow btrfs_truncate_block() to fallback to nocow for data
>     space reservation

As the patch is a stable backport candidate, please reorder the series
so the fix comes first and then the cleanups. The fixing patch does not
need to be perfect regarding naming. Thanks.