Re: [PATCH] Btrfs: remove duplicated find_get_pages_contig

[Liu Bo <bo.li.liu@oracle.com>]

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:
> > 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?

> 
> > +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.

> 
> > +
> > +	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
> > +	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;
> >  }