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;
> > }
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
