On Thu, Feb 02, 2017 at 07:42:43PM +0100, David Sterba wrote:

On Mon, Jan 30, 2017 at 12:26:30PM -0800, Liu Bo wrote:

quoted

This creates a helper to manipulate page bits to avoid duplicate uses.

This seems too short for what the patch does. It adds a new page ops bit
that would deserve a separate patch. Please try to split it to smaller
parts.

Hmm..the newly added 'PAGE_LOCK' is only used in lock_delalloc_pages in
order to make the helper lock pages.

I could firstly introduce the helper from extent_clear_unlock_delalloc()
, then introduce the new 'PAGE_LOCK' and make lock_delalloc_pages and
__unlock_for_delalloc to use the helper, does it sound better to review
and bisect?

quoted

+static noinline void __unlock_for_delalloc(struct inode *inode,
+					   struct page *locked_page,
+					   u64 start, u64 end)
+{
+	unsigned long page_ops = PAGE_UNLOCK;

Used only once, not necessary IMO.

OK.

quoted

+
+	ASSERT(locked_page);
+	__process_pages_contig(inode->i_mapping, locked_page,
+			       start >> PAGE_SHIFT, end >> PAGE_SHIFT,
+			       page_ops, NULL);
+}
+
+static noinline int lock_delalloc_pages(struct inode *inode,
+					struct page *locked_page,
+					u64 delalloc_start,
+					u64 delalloc_end)
+{
+	pgoff_t index = delalloc_start >> PAGE_SHIFT;
+	pgoff_t start_index = index;
+	pgoff_t end_index = delalloc_end >> PAGE_SHIFT;
+	unsigned long page_ops = PAGE_LOCK;

Same here.

OK.

Thanks,

-liubo

quoted

+	int ret = 0;
+
+	ASSERT(locked_page);
+
+	ret = __process_pages_contig(inode->i_mapping, locked_page, start_index,
+			       end_index, page_ops, &index);
+	if (ret == -EAGAIN) {
+		__unlock_for_delalloc(inode, locked_page, delalloc_start,
+				      ((u64)index) << PAGE_SHIFT);
 	}
+
 	return ret;
 }