linux-next: manual merge of the writeback tree with the vfs tree

linux-next: manual merge of the writeback tree with the vfs tree

[Stephen Rothwell]

Hi Wu,

Today's linux-next merge of the writeback tree got a conflict in
mm/filemap.c between commit ba96a361e21b ("fs: kill i_alloc_sem") from
the vfs tree and commit f758eeabeb96 ("writeback: split
inode_wb_list_lock into bdi_writeback.list_lock") from the writeback tree.

I fixed it up (see below) and can carry the fix as necessary.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc mm/filemap.c
index f820e60,1e492c3..0000000
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@@ -78,7 -78,10 +78,7 @@@
   *  ->i_mutex			(generic_file_buffered_write)
   *    ->mmap_sem		(fault_in_pages_readable->do_page_fault)
   *
-  *  inode_wb_list_lock
 - *  ->i_mutex
 - *    ->i_alloc_sem             (various)
 - *
+  *  bdi->wb.list_lock
   *    sb_lock			(fs/fs-writeback.c)
   *    ->mapping->tree_lock	(__sync_single_inode)
   *

Re: linux-next: manual merge of the writeback tree with the vfs tree

[Wu Fengguang]

Hi Stephen,

On Mon, Jul 18, 2011 at 03:29:44PM +0800, Stephen Rothwell wrote:
> Hi Wu,
> 
> Today's linux-next merge of the writeback tree got a conflict in
> mm/filemap.c between commit ba96a361e21b ("fs: kill i_alloc_sem") from
> the vfs tree and commit f758eeabeb96 ("writeback: split
> inode_wb_list_lock into bdi_writeback.list_lock") from the writeback tree.
> 
> I fixed it up (see below) and can carry the fix as necessary.

OK, thank you very much!

Cheers,
Fengguang

> diff --cc mm/filemap.c
> index f820e60,1e492c3..0000000
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@@ -78,7 -78,10 +78,7 @@@
>    *  ->i_mutex			(generic_file_buffered_write)
>    *    ->mmap_sem		(fault_in_pages_readable->do_page_fault)
>    *
> -  *  inode_wb_list_lock
>  - *  ->i_mutex
>  - *    ->i_alloc_sem             (various)
>  - *
> +  *  bdi->wb.list_lock
>    *    sb_lock			(fs/fs-writeback.c)
>    *    ->mapping->tree_lock	(__sync_single_inode)
>    *

linux-next: manual merge of the writeback tree with the vfs tree

[Stephen Rothwell]

Hi Wu,

Today's linux-next merge of the writeback tree got a conflict in
fs/fs-writeback.c between commit dffe5a6c5214 ("superblock: move
pin_sb_for_writeback() to fs/super.c") from the vfs tree and commits
f758eeabeb96 ("writeback: split inode_wb_list_lock into
bdi_writeback.list_lock") and d46db3d58233 ("writeback: make
writeback_control.nr_to_write straight") from the writeback tree.

I fixed it up (I think - see below) and can carry the fix as necessary.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc fs/fs-writeback.c
index b8c507c,6d49439..0000000
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@@ -460,6 -480,63 +480,37 @@@ writeback_single_inode(struct inode *in
  	return ret;
  }
  
 -/*
 - * For background writeback the caller does not have the sb pinned
 - * before calling writeback. So make sure that we do pin it, so it doesn't
 - * go away while we are writing inodes from it.
 - */
 -static bool pin_sb_for_writeback(struct super_block *sb)
 -{
 -	spin_lock(&sb_lock);
 -	if (list_empty(&sb->s_instances)) {
 -		spin_unlock(&sb_lock);
 -		return false;
 -	}
 -
 -	sb->s_count++;
 -	spin_unlock(&sb_lock);
 -
 -	if (down_read_trylock(&sb->s_umount)) {
 -		if (sb->s_root)
 -			return true;
 -		up_read(&sb->s_umount);
 -	}
 -
 -	put_super(sb);
 -	return false;
 -}
 -
+ static long writeback_chunk_size(struct backing_dev_info *bdi,
+ 				 struct wb_writeback_work *work)
+ {
+ 	long pages;
+ 
+ 	/*
+ 	 * WB_SYNC_ALL mode does livelock avoidance by syncing dirty
+ 	 * inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX
+ 	 * here avoids calling into writeback_inodes_wb() more than once.
+ 	 *
+ 	 * The intended call sequence for WB_SYNC_ALL writeback is:
+ 	 *
+ 	 *      wb_writeback()
+ 	 *          writeback_sb_inodes()       <== called only once
+ 	 *              write_cache_pages()     <== called once for each inode
+ 	 *                   (quickly) tag currently dirty pages
+ 	 *                   (maybe slowly) sync all tagged pages
+ 	 */
+ 	if (work->sync_mode == WB_SYNC_ALL || work->tagged_writepages)
+ 		pages = LONG_MAX;
+ 	else {
+ 		pages = min(bdi->avg_write_bandwidth / 2,
+ 			    global_dirty_limit / DIRTY_SCOPE);
+ 		pages = min(pages, work->nr_pages);
+ 		pages = round_down(pages + MIN_WRITEBACK_PAGES,
+ 				   MIN_WRITEBACK_PAGES);
+ 	}
+ 
+ 	return pages;
+ }
+ 
  /*
   * Write a portion of b_io inodes which belong to @sb.
   *
@@@ -559,40 -643,41 +617,41 @@@ static long __writeback_inodes_wb(struc
  		struct inode *inode = wb_inode(wb->b_io.prev);
  		struct super_block *sb = inode->i_sb;
  
 -		if (!pin_sb_for_writeback(sb)) {
 +		if (!grab_super_passive(sb)) {
- 			requeue_io(inode);
+ 			requeue_io(inode, wb);
  			continue;
  		}
- 		ret = writeback_sb_inodes(sb, wb, wbc, false);
+ 		wrote += writeback_sb_inodes(sb, wb, work);
  		drop_super(sb);
  
- 		if (ret)
- 			break;
+ 		/* refer to the same tests at the end of writeback_sb_inodes */
+ 		if (wrote) {
+ 			if (time_is_before_jiffies(start_time + HZ / 10UL))
+ 				break;
+ 			if (work->nr_pages <= 0)
+ 				break;
+ 		}
  	}
- 	spin_unlock(&inode_wb_list_lock);
  	/* Leave any unwritten inodes on b_io */
+ 	return wrote;
  }
  
- static void __writeback_inodes_sb(struct super_block *sb,
- 		struct bdi_writeback *wb, struct writeback_control *wbc)
+ long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages)
  {
- 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
+ 	struct wb_writeback_work work = {
+ 		.nr_pages	= nr_pages,
+ 		.sync_mode	= WB_SYNC_NONE,
+ 		.range_cyclic	= 1,
+ 	};
  
- 	spin_lock(&inode_wb_list_lock);
- 	if (!wbc->for_kupdate || list_empty(&wb->b_io))
- 		queue_io(wb, wbc->older_than_this);
- 	writeback_sb_inodes(sb, wb, wbc, true);
- 	spin_unlock(&inode_wb_list_lock);
- }
+ 	spin_lock(&wb->list_lock);
+ 	if (list_empty(&wb->b_io))
+ 		queue_io(wb, NULL);
+ 	__writeback_inodes_wb(wb, &work);
+ 	spin_unlock(&wb->list_lock);
  
- /*
-  * The maximum number of pages to writeout in a single bdi flush/kupdate
-  * operation.  We do this so we don't hold I_SYNC against an inode for
-  * enormous amounts of time, which would block a userspace task which has
-  * been forced to throttle against that inode.  Also, the code reevaluates
-  * the dirty each time it has written this many pages.
-  */
- #define MAX_WRITEBACK_PAGES     1024
+ 	return nr_pages - work.nr_pages;
+ }
  
  static inline bool over_bground_thresh(void)
  {

Re: linux-next: manual merge of the writeback tree with the vfs tree

[Wu Fengguang]

Hi Stephen,

On Mon, Jul 18, 2011 at 03:34:09PM +0800, Stephen Rothwell wrote:
> Hi Wu,
> 
> Today's linux-next merge of the writeback tree got a conflict in
> fs/fs-writeback.c between commit dffe5a6c5214 ("superblock: move
> pin_sb_for_writeback() to fs/super.c") from the vfs tree and commits
> f758eeabeb96 ("writeback: split inode_wb_list_lock into
> bdi_writeback.list_lock") and d46db3d58233 ("writeback: make
> writeback_control.nr_to_write straight") from the writeback tree.
> 
> I fixed it up (I think - see below) and can carry the fix as necessary.

OK, please. Thank you very much!

Cheers,
Fengguang

> diff --cc fs/fs-writeback.c
> index b8c507c,6d49439..0000000
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@@ -460,6 -480,63 +480,37 @@@ writeback_single_inode(struct inode *in
>   	return ret;
>   }
>   
>  -/*
>  - * For background writeback the caller does not have the sb pinned
>  - * before calling writeback. So make sure that we do pin it, so it doesn't
>  - * go away while we are writing inodes from it.
>  - */
>  -static bool pin_sb_for_writeback(struct super_block *sb)
>  -{
>  -	spin_lock(&sb_lock);
>  -	if (list_empty(&sb->s_instances)) {
>  -		spin_unlock(&sb_lock);
>  -		return false;
>  -	}
>  -
>  -	sb->s_count++;
>  -	spin_unlock(&sb_lock);
>  -
>  -	if (down_read_trylock(&sb->s_umount)) {
>  -		if (sb->s_root)
>  -			return true;
>  -		up_read(&sb->s_umount);
>  -	}
>  -
>  -	put_super(sb);
>  -	return false;
>  -}
>  -
> + static long writeback_chunk_size(struct backing_dev_info *bdi,
> + 				 struct wb_writeback_work *work)
> + {
> + 	long pages;
> + 
> + 	/*
> + 	 * WB_SYNC_ALL mode does livelock avoidance by syncing dirty
> + 	 * inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX
> + 	 * here avoids calling into writeback_inodes_wb() more than once.
> + 	 *
> + 	 * The intended call sequence for WB_SYNC_ALL writeback is:
> + 	 *
> + 	 *      wb_writeback()
> + 	 *          writeback_sb_inodes()       <== called only once
> + 	 *              write_cache_pages()     <== called once for each inode
> + 	 *                   (quickly) tag currently dirty pages
> + 	 *                   (maybe slowly) sync all tagged pages
> + 	 */
> + 	if (work->sync_mode == WB_SYNC_ALL || work->tagged_writepages)
> + 		pages = LONG_MAX;
> + 	else {
> + 		pages = min(bdi->avg_write_bandwidth / 2,
> + 			    global_dirty_limit / DIRTY_SCOPE);
> + 		pages = min(pages, work->nr_pages);
> + 		pages = round_down(pages + MIN_WRITEBACK_PAGES,
> + 				   MIN_WRITEBACK_PAGES);
> + 	}
> + 
> + 	return pages;
> + }
> + 
>   /*
>    * Write a portion of b_io inodes which belong to @sb.
>    *
> @@@ -559,40 -643,41 +617,41 @@@ static long __writeback_inodes_wb(struc
>   		struct inode *inode = wb_inode(wb->b_io.prev);
>   		struct super_block *sb = inode->i_sb;
>   
>  -		if (!pin_sb_for_writeback(sb)) {
>  +		if (!grab_super_passive(sb)) {
> - 			requeue_io(inode);
> + 			requeue_io(inode, wb);
>   			continue;
>   		}
> - 		ret = writeback_sb_inodes(sb, wb, wbc, false);
> + 		wrote += writeback_sb_inodes(sb, wb, work);
>   		drop_super(sb);
>   
> - 		if (ret)
> - 			break;
> + 		/* refer to the same tests at the end of writeback_sb_inodes */
> + 		if (wrote) {
> + 			if (time_is_before_jiffies(start_time + HZ / 10UL))
> + 				break;
> + 			if (work->nr_pages <= 0)
> + 				break;
> + 		}
>   	}
> - 	spin_unlock(&inode_wb_list_lock);
>   	/* Leave any unwritten inodes on b_io */
> + 	return wrote;
>   }
>   
> - static void __writeback_inodes_sb(struct super_block *sb,
> - 		struct bdi_writeback *wb, struct writeback_control *wbc)
> + long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages)
>   {
> - 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
> + 	struct wb_writeback_work work = {
> + 		.nr_pages	= nr_pages,
> + 		.sync_mode	= WB_SYNC_NONE,
> + 		.range_cyclic	= 1,
> + 	};
>   
> - 	spin_lock(&inode_wb_list_lock);
> - 	if (!wbc->for_kupdate || list_empty(&wb->b_io))
> - 		queue_io(wb, wbc->older_than_this);
> - 	writeback_sb_inodes(sb, wb, wbc, true);
> - 	spin_unlock(&inode_wb_list_lock);
> - }
> + 	spin_lock(&wb->list_lock);
> + 	if (list_empty(&wb->b_io))
> + 		queue_io(wb, NULL);
> + 	__writeback_inodes_wb(wb, &work);
> + 	spin_unlock(&wb->list_lock);
>   
> - /*
> -  * The maximum number of pages to writeout in a single bdi flush/kupdate
> -  * operation.  We do this so we don't hold I_SYNC against an inode for
> -  * enormous amounts of time, which would block a userspace task which has
> -  * been forced to throttle against that inode.  Also, the code reevaluates
> -  * the dirty each time it has written this many pages.
> -  */
> - #define MAX_WRITEBACK_PAGES     1024
> + 	return nr_pages - work.nr_pages;
> + }
>   
>   static inline bool over_bground_thresh(void)
>   {

Re: linux-next: manual merge of the writeback tree with the vfs tree

[Wu Fengguang]

Hi Stephen,

What should I do with the 2 conflicts for upstream merge?

I noticed that Linus just merged the vfs tree. So the obvious thing I
could do is to rebase writeback.git to Linus' tree and fix the
conflicts, and then ask Linus to pull the rebased writeback tree.

Or are there better practices?

Thanks,
Fengguang

On Mon, Jul 18, 2011 at 03:34:09PM +0800, Stephen Rothwell wrote:
> Hi Wu,
> 
> Today's linux-next merge of the writeback tree got a conflict in
> fs/fs-writeback.c between commit dffe5a6c5214 ("superblock: move
> pin_sb_for_writeback() to fs/super.c") from the vfs tree and commits
> f758eeabeb96 ("writeback: split inode_wb_list_lock into
> bdi_writeback.list_lock") and d46db3d58233 ("writeback: make
> writeback_control.nr_to_write straight") from the writeback tree.
> 
> I fixed it up (I think - see below) and can carry the fix as necessary.
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
> 
> diff --cc fs/fs-writeback.c
> index b8c507c,6d49439..0000000
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@@ -460,6 -480,63 +480,37 @@@ writeback_single_inode(struct inode *in
>   	return ret;
>   }
>   
>  -/*
>  - * For background writeback the caller does not have the sb pinned
>  - * before calling writeback. So make sure that we do pin it, so it doesn't
>  - * go away while we are writing inodes from it.
>  - */
>  -static bool pin_sb_for_writeback(struct super_block *sb)
>  -{
>  -	spin_lock(&sb_lock);
>  -	if (list_empty(&sb->s_instances)) {
>  -		spin_unlock(&sb_lock);
>  -		return false;
>  -	}
>  -
>  -	sb->s_count++;
>  -	spin_unlock(&sb_lock);
>  -
>  -	if (down_read_trylock(&sb->s_umount)) {
>  -		if (sb->s_root)
>  -			return true;
>  -		up_read(&sb->s_umount);
>  -	}
>  -
>  -	put_super(sb);
>  -	return false;
>  -}
>  -
> + static long writeback_chunk_size(struct backing_dev_info *bdi,
> + 				 struct wb_writeback_work *work)
> + {
> + 	long pages;
> + 
> + 	/*
> + 	 * WB_SYNC_ALL mode does livelock avoidance by syncing dirty
> + 	 * inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX
> + 	 * here avoids calling into writeback_inodes_wb() more than once.
> + 	 *
> + 	 * The intended call sequence for WB_SYNC_ALL writeback is:
> + 	 *
> + 	 *      wb_writeback()
> + 	 *          writeback_sb_inodes()       <== called only once
> + 	 *              write_cache_pages()     <== called once for each inode
> + 	 *                   (quickly) tag currently dirty pages
> + 	 *                   (maybe slowly) sync all tagged pages
> + 	 */
> + 	if (work->sync_mode == WB_SYNC_ALL || work->tagged_writepages)
> + 		pages = LONG_MAX;
> + 	else {
> + 		pages = min(bdi->avg_write_bandwidth / 2,
> + 			    global_dirty_limit / DIRTY_SCOPE);
> + 		pages = min(pages, work->nr_pages);
> + 		pages = round_down(pages + MIN_WRITEBACK_PAGES,
> + 				   MIN_WRITEBACK_PAGES);
> + 	}
> + 
> + 	return pages;
> + }
> + 
>   /*
>    * Write a portion of b_io inodes which belong to @sb.
>    *
> @@@ -559,40 -643,41 +617,41 @@@ static long __writeback_inodes_wb(struc
>   		struct inode *inode = wb_inode(wb->b_io.prev);
>   		struct super_block *sb = inode->i_sb;
>   
>  -		if (!pin_sb_for_writeback(sb)) {
>  +		if (!grab_super_passive(sb)) {
> - 			requeue_io(inode);
> + 			requeue_io(inode, wb);
>   			continue;
>   		}
> - 		ret = writeback_sb_inodes(sb, wb, wbc, false);
> + 		wrote += writeback_sb_inodes(sb, wb, work);
>   		drop_super(sb);
>   
> - 		if (ret)
> - 			break;
> + 		/* refer to the same tests at the end of writeback_sb_inodes */
> + 		if (wrote) {
> + 			if (time_is_before_jiffies(start_time + HZ / 10UL))
> + 				break;
> + 			if (work->nr_pages <= 0)
> + 				break;
> + 		}
>   	}
> - 	spin_unlock(&inode_wb_list_lock);
>   	/* Leave any unwritten inodes on b_io */
> + 	return wrote;
>   }
>   
> - static void __writeback_inodes_sb(struct super_block *sb,
> - 		struct bdi_writeback *wb, struct writeback_control *wbc)
> + long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages)
>   {
> - 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
> + 	struct wb_writeback_work work = {
> + 		.nr_pages	= nr_pages,
> + 		.sync_mode	= WB_SYNC_NONE,
> + 		.range_cyclic	= 1,
> + 	};
>   
> - 	spin_lock(&inode_wb_list_lock);
> - 	if (!wbc->for_kupdate || list_empty(&wb->b_io))
> - 		queue_io(wb, wbc->older_than_this);
> - 	writeback_sb_inodes(sb, wb, wbc, true);
> - 	spin_unlock(&inode_wb_list_lock);
> - }
> + 	spin_lock(&wb->list_lock);
> + 	if (list_empty(&wb->b_io))
> + 		queue_io(wb, NULL);
> + 	__writeback_inodes_wb(wb, &work);
> + 	spin_unlock(&wb->list_lock);
>   
> - /*
> -  * The maximum number of pages to writeout in a single bdi flush/kupdate
> -  * operation.  We do this so we don't hold I_SYNC against an inode for
> -  * enormous amounts of time, which would block a userspace task which has
> -  * been forced to throttle against that inode.  Also, the code reevaluates
> -  * the dirty each time it has written this many pages.
> -  */
> - #define MAX_WRITEBACK_PAGES     1024
> + 	return nr_pages - work.nr_pages;
> + }
>   
>   static inline bool over_bground_thresh(void)
>   {

Re: linux-next: manual merge of the writeback tree with the vfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 881 bytes --]

Hi,

On Tue, 26 Jul 2011 12:58:12 +0800 Wu Fengguang <fengguang.wu@intel.com> wrote:
>
> What should I do with the 2 conflicts for upstream merge?
> 
> I noticed that Linus just merged the vfs tree. So the obvious thing I
> could do is to rebase writeback.git to Linus' tree and fix the
> conflicts, and then ask Linus to pull the rebased writeback tree.
> 
> Or are there better practices?

Normally you just ask Linus to pull your tree and let him know that there
are some conflicts and also ask him to let you know if he wants you to
fix them up before merging.  He is a smart fellow, and if I worked it out,
I am sure he can as well.  :-)

Some people pull Linus' tree into theirs and resolve the conflicts, but
he prefers to do them himself usually.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: linux-next: manual merge of the writeback tree with the vfs tree

[Wu Fengguang]

Stephen,

On Tue, Jul 26, 2011 at 01:14:51PM +0800, Stephen Rothwell wrote:
> Hi,
> 
> On Tue, 26 Jul 2011 12:58:12 +0800 Wu Fengguang <fengguang.wu@intel.com> wrote:
> >
> > What should I do with the 2 conflicts for upstream merge?
> > 
> > I noticed that Linus just merged the vfs tree. So the obvious thing I
> > could do is to rebase writeback.git to Linus' tree and fix the
> > conflicts, and then ask Linus to pull the rebased writeback tree.
> > 
> > Or are there better practices?
> 
> Normally you just ask Linus to pull your tree and let him know that there
> are some conflicts and also ask him to let you know if he wants you to
> fix them up before merging.  He is a smart fellow, and if I worked it out,
> I am sure he can as well.  :-)
> 
> Some people pull Linus' tree into theirs and resolve the conflicts, but
> he prefers to do them himself usually.

Good point. Thank you for the nice guide!

Thanks,
Fengguang