diff mbox series

[v4,10/68] btrfs: extent_io: remove the forward declaration and rename __process_pages_contig

Message ID 20201021062554.68132-11-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: add basic rw support for subpage sector size | expand

Commit Message

Qu Wenruo Oct. 21, 2020, 6:24 a.m. UTC 
There is no need to do forward declaration for __process_pages_contig(),
so move it before it get first called.

Since we are here, also remove the "__" prefix since there is no special
meaning for it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 180 +++++++++++++++++++++++--------------------
 1 file changed, 95 insertions(+), 85 deletions(-)

Comments

David Sterba Oct. 27, 2020, 12:28 a.m. UTC | #1 
On Wed, Oct 21, 2020 at 02:24:56PM +0800, Qu Wenruo wrote:
> There is no need to do forward declaration for __process_pages_contig(),
> so move it before it get first called.

But without other good reason than prototype removal we don't want to
move the code.

> Since we are here, also remove the "__" prefix since there is no special
> meaning for it.

Renaming and adding the comment is fine on itself but does not justify
moving the chunk of code.
Qu Wenruo Oct. 27, 2020, 12:50 a.m. UTC | #2 
On 2020/10/27 上午8:28, David Sterba wrote:
> On Wed, Oct 21, 2020 at 02:24:56PM +0800, Qu Wenruo wrote:
>> There is no need to do forward declaration for __process_pages_contig(),
>> so move it before it get first called.
> 
> But without other good reason than prototype removal we don't want to
> move the code.
> 
>> Since we are here, also remove the "__" prefix since there is no special
>> meaning for it.
> 
> Renaming and adding the comment is fine on itself but does not justify
> moving the chunk of code.
> 
I thought the forward declaration should be something we clean up during
development.

But it looks like it's no longer the case or my memory is just blurry.

Anyway, I can definitely keep the forward declaration and just keep the
renaming and new comments.

Thanks,
Qu
David Sterba Oct. 27, 2020, 11:25 p.m. UTC | #3 
On Tue, Oct 27, 2020 at 08:50:15AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/10/27 上午8:28, David Sterba wrote:
> > On Wed, Oct 21, 2020 at 02:24:56PM +0800, Qu Wenruo wrote:
> >> There is no need to do forward declaration for __process_pages_contig(),
> >> so move it before it get first called.
> > 
> > But without other good reason than prototype removal we don't want to
> > move the code.
> > 
> >> Since we are here, also remove the "__" prefix since there is no special
> >> meaning for it.
> > 
> > Renaming and adding the comment is fine on itself but does not justify
> > moving the chunk of code.
> > 
> I thought the forward declaration should be something we clean up during
> development.

Eventually yes but commits that only move code pollute the git history
so there needs to be some other reason like splitting or refactoring.
Keeping the prototypes is not that bad, if it pops up during grep it's
quickly skipped, but when one is looking why some code changed it's very
annoying to land in some "move code" patch. I'm trying to keep such
changes to minimum but there are cases where we want that so it's not a
strict 'no never', rather case by case decision.
> 
> But it looks like it's no longer the case or my memory is just blurry.

The list of things to keep in mind is getting long
https://btrfs.wiki.kernel.org/index.php/Development_notes#Coding_style_preferences
diff mbox series

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 3f95c67f0c92..d5e03977c9c8 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1814,10 +1814,98 @@  bool btrfs_find_delalloc_range(struct extent_io_tree *tree, u64 *start,
 	return found;
 }
 
-static int __process_pages_contig(struct address_space *mapping,
-				  struct page *locked_page,
-				  pgoff_t start_index, pgoff_t end_index,
-				  unsigned long page_ops, pgoff_t *index_ret);
+/*
+ * A helper to update contiguous pages status according to @page_ops.
+ *
+ * @mapping:		The address space of the pages
+ * @locked_page:	The already locked page. Mostly for inline extent
+ * 			handling
+ * @start_index:	The start page index.
+ * @end_inde:		The last page index.
+ * @pages_opts:		The operations to be done
+ * @index_ret:		The last handled page index (for error case)
+ *
+ * Return 0 if every page is handled properly.
+ * Return <0 if something wrong happened, and update @index_ret.
+ */
+static int process_pages_contig(struct address_space *mapping,
+				struct page *locked_page,
+				pgoff_t start_index, pgoff_t end_index,
+				unsigned long page_ops, pgoff_t *index_ret)
+{
+	unsigned long nr_pages = end_index - start_index + 1;
+	unsigned long pages_locked = 0;
+	pgoff_t index = start_index;
+	struct page *pages[16];
+	unsigned ret;
+	int err = 0;
+	int i;
+
+	if (page_ops & PAGE_LOCK) {
+		ASSERT(page_ops == PAGE_LOCK);
+		ASSERT(index_ret && *index_ret == start_index);
+	}
+
+	if ((page_ops & PAGE_SET_ERROR) && nr_pages > 0)
+		mapping_set_error(mapping, -EIO);
+
+	while (nr_pages > 0) {
+		ret = find_get_pages_contig(mapping, index,
+				     min_t(unsigned long,
+				     nr_pages, ARRAY_SIZE(pages)), pages);
+		if (ret == 0) {
+			/*
+			 * Only if we're going to lock these pages,
+			 * can we find nothing at @index.
+			 */
+			ASSERT(page_ops & PAGE_LOCK);
+			err = -EAGAIN;
+			goto out;
+		}
+
+		for (i = 0; i < ret; i++) {
+			if (page_ops & PAGE_SET_PRIVATE2)
+				SetPagePrivate2(pages[i]);
+
+			if (locked_page && pages[i] == locked_page) {
+				put_page(pages[i]);
+				pages_locked++;
+				continue;
+			}
+			if (page_ops & PAGE_CLEAR_DIRTY)
+				clear_page_dirty_for_io(pages[i]);
+			if (page_ops & PAGE_SET_WRITEBACK)
+				set_page_writeback(pages[i]);
+			if (page_ops & PAGE_SET_ERROR)
+				SetPageError(pages[i]);
+			if (page_ops & PAGE_END_WRITEBACK)
+				end_page_writeback(pages[i]);
+			if (page_ops & PAGE_UNLOCK)
+				unlock_page(pages[i]);
+			if (page_ops & PAGE_LOCK) {
+				lock_page(pages[i]);
+				if (!PageDirty(pages[i]) ||
+				    pages[i]->mapping != mapping) {
+					unlock_page(pages[i]);
+					for (; i < ret; i++)
+						put_page(pages[i]);
+					err = -EAGAIN;
+					goto out;
+				}
+			}
+			put_page(pages[i]);
+			pages_locked++;
+		}
+		nr_pages -= ret;
+		index += ret;
+		cond_resched();
+	}
+out:
+	if (err && index_ret)
+		*index_ret = start_index + pages_locked - 1;
+	return err;
+}
+
 
 static noinline void __unlock_for_delalloc(struct inode *inode,
 					   struct page *locked_page,
@@ -1830,7 +1918,7 @@  static noinline void __unlock_for_delalloc(struct inode *inode,
 	if (index == locked_page->index && end_index == index)
 		return;
 
-	__process_pages_contig(inode->i_mapping, locked_page, index, end_index,
+	process_pages_contig(inode->i_mapping, locked_page, index, end_index,
 			       PAGE_UNLOCK, NULL);
 }
 
@@ -1848,7 +1936,7 @@  static noinline int lock_delalloc_pages(struct inode *inode,
 	if (index == locked_page->index && index == end_index)
 		return 0;
 
-	ret = __process_pages_contig(inode->i_mapping, locked_page, index,
+	ret = process_pages_contig(inode->i_mapping, locked_page, index,
 				     end_index, PAGE_LOCK, &index_ret);
 	if (ret == -EAGAIN)
 		__unlock_for_delalloc(inode, locked_page, delalloc_start,
@@ -1945,84 +2033,6 @@  noinline_for_stack bool find_lock_delalloc_range(struct inode *inode,
 	return found;
 }
 
-static int __process_pages_contig(struct address_space *mapping,
-				  struct page *locked_page,
-				  pgoff_t start_index, pgoff_t end_index,
-				  unsigned long page_ops, pgoff_t *index_ret)
-{
-	unsigned long nr_pages = end_index - start_index + 1;
-	unsigned long pages_locked = 0;
-	pgoff_t index = start_index;
-	struct page *pages[16];
-	unsigned ret;
-	int err = 0;
-	int i;
-
-	if (page_ops & PAGE_LOCK) {
-		ASSERT(page_ops == PAGE_LOCK);
-		ASSERT(index_ret && *index_ret == start_index);
-	}
-
-	if ((page_ops & PAGE_SET_ERROR) && nr_pages > 0)
-		mapping_set_error(mapping, -EIO);
-
-	while (nr_pages > 0) {
-		ret = find_get_pages_contig(mapping, index,
-				     min_t(unsigned long,
-				     nr_pages, ARRAY_SIZE(pages)), pages);
-		if (ret == 0) {
-			/*
-			 * Only if we're going to lock these pages,
-			 * can we find nothing at @index.
-			 */
-			ASSERT(page_ops & PAGE_LOCK);
-			err = -EAGAIN;
-			goto out;
-		}
-
-		for (i = 0; i < ret; i++) {
-			if (page_ops & PAGE_SET_PRIVATE2)
-				SetPagePrivate2(pages[i]);
-
-			if (locked_page && pages[i] == locked_page) {
-				put_page(pages[i]);
-				pages_locked++;
-				continue;
-			}
-			if (page_ops & PAGE_CLEAR_DIRTY)
-				clear_page_dirty_for_io(pages[i]);
-			if (page_ops & PAGE_SET_WRITEBACK)
-				set_page_writeback(pages[i]);
-			if (page_ops & PAGE_SET_ERROR)
-				SetPageError(pages[i]);
-			if (page_ops & PAGE_END_WRITEBACK)
-				end_page_writeback(pages[i]);
-			if (page_ops & PAGE_UNLOCK)
-				unlock_page(pages[i]);
-			if (page_ops & PAGE_LOCK) {
-				lock_page(pages[i]);
-				if (!PageDirty(pages[i]) ||
-				    pages[i]->mapping != mapping) {
-					unlock_page(pages[i]);
-					for (; i < ret; i++)
-						put_page(pages[i]);
-					err = -EAGAIN;
-					goto out;
-				}
-			}
-			put_page(pages[i]);
-			pages_locked++;
-		}
-		nr_pages -= ret;
-		index += ret;
-		cond_resched();
-	}
-out:
-	if (err && index_ret)
-		*index_ret = start_index + pages_locked - 1;
-	return err;
-}
-
 void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
 				  struct page *locked_page,
 				  unsigned clear_bits,
@@ -2030,7 +2040,7 @@  void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
 {
 	clear_extent_bit(&inode->io_tree, start, end, clear_bits, 1, 0, NULL);
 
-	__process_pages_contig(inode->vfs_inode.i_mapping, locked_page,
+	process_pages_contig(inode->vfs_inode.i_mapping, locked_page,
 			       start >> PAGE_SHIFT, end >> PAGE_SHIFT,
 			       page_ops, NULL);
 }