Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1346149 > unrolled thread

[PATCH block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH block/for-linus] writeback: flush inode cgroup wb switches  instead of pinning super_block Tejun Heo <tj@kernel.org> - 2016-02-29 21:50 +0100
    Re: [PATCH block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 22:00 +0100
      Re: [PATCH block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Tejun Heo <tj@kernel.org> - 2016-02-29 22:00 +0100
        Re: [PATCH block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Tejun Heo <tj@kernel.org> - 2016-02-29 22:10 +0100
          Re: [PATCH block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Jan Kara <jack@suse.cz> - 2016-02-29 22:30 +0100
        Re: [PATCH block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-29 22:10 +0100
    [PATCH v2 block/for-linus] writeback: flush inode cgroup wb switches  instead of pinning super_block Tejun Heo <tj@kernel.org> - 2016-03-01 00:30 +0100
      Re: [PATCH v2 block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Jan Kara <jack@suse.cz> - 2016-03-01 10:30 +0100
      Re: [PATCH v2 block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Jens Axboe <axboe@kernel.dk> - 2016-03-01 18:50 +0100
        Re: [PATCH v2 block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Tejun Heo <tj@kernel.org> - 2016-03-01 19:00 +0100
          Re: [PATCH v2 block/for-linus] writeback: flush inode cgroup wb  switches instead of pinning super_block Jan Kara <jack@suse.cz> - 2016-03-02 11:30 +0100
    Re: [PATCH block/for-linus] writeback: flush inode cgroup wb switches  instead of pinning super_block Tahsin Erdogan <tahsin@google.com> - 2016-03-01 14:40 +0100

#1346149 — [PATCH block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

If cgroup writeback is in use, inodes can be scheduled for
asynchronous wb switching.  Before 5ff8eaac1636 ("writeback: keep
superblock pinned during cgroup writeback association switches"), this
could race with umount leading to super_block being destroyed while
inodes are pinned for wb switching.  5ff8eaac1636 fixed it by bumping
s_active while wb switches are in flight; however, this allowed
in-flight wb switches to make umounts asynchronous when the userland
expected synchronosity - e.g. fsck immediately following umount may
fail because the device is still busy.

This patch removes the problematic super_block pinning and instead
makes generic_shutdown_super() flush in-flight wb switches.  wb
switches are now executed on a dedicated isw_wq so that they can be
flushed and isw_nr_in_flight keeps track of the number of in-flight wb
switches so that flushing can be avoided in most cases.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Tahsin Erdogan <tahsin@google.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Link: http://lkml.kernel.org/g/CAAeU0aNCq7LGODvVGRU-oU_o-6enii5ey0p1c26D1ZzYwkDc5A@mail.gmail.com
Fixes: 5ff8eaac1636 ("writeback: keep superblock pinned during cgroup writeback association switches")
Cc: stable@vger.kernel.org #v4.5
---
Hello,

Tahsin, can you please verify that this removes the asynchronous
behavior while still avoiding the original issue?

Thanks.

 fs/fs-writeback.c         |   51 +++++++++++++++++++++++++++++++++++-----------
 fs/super.c                |    1 
 include/linux/writeback.h |    5 ++++
 3 files changed, 45 insertions(+), 12 deletions(-)

--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -223,6 +223,9 @@ static void wb_wait_for_completion(struc
 #define WB_FRN_HIST_MAX_SLOTS	(WB_FRN_HIST_THR_SLOTS / 2 + 1)
 					/* one round can affect upto 5 slots */
 
+static atomic_t isw_nr_in_flight = ATOMIC_INIT(0);
+static struct workqueue_struct *isw_wq;
+
 void __inode_attach_wb(struct inode *inode, struct page *page)
 {
 	struct backing_dev_info *bdi = inode_to_bdi(inode);
@@ -317,7 +320,6 @@ static void inode_switch_wbs_work_fn(str
 	struct inode_switch_wbs_context *isw =
 		container_of(work, struct inode_switch_wbs_context, work);
 	struct inode *inode = isw->inode;
-	struct super_block *sb = inode->i_sb;
 	struct address_space *mapping = inode->i_mapping;
 	struct bdi_writeback *old_wb = inode->i_wb;
 	struct bdi_writeback *new_wb = isw->new_wb;
@@ -424,8 +426,9 @@ skip_switch:
 	wb_put(new_wb);
 
 	iput(inode);
-	deactivate_super(sb);
 	kfree(isw);
+
+	atomic_dec(&isw_nr_in_flight);
 }
 
 static void inode_switch_wbs_rcu_fn(struct rcu_head *rcu_head)
@@ -435,7 +438,7 @@ static void inode_switch_wbs_rcu_fn(stru
 
 	/* needs to grab bh-unsafe locks, bounce to work item */
 	INIT_WORK(&isw->work, inode_switch_wbs_work_fn);
-	schedule_work(&isw->work);
+	queue_work(isw_wq, &isw->work);
 }
 
 /**
@@ -471,20 +474,19 @@ static void inode_switch_wbs(struct inod
 
 	/* while holding I_WB_SWITCH, no one else can update the association */
 	spin_lock(&inode->i_lock);
-
 	if (inode->i_state & (I_WB_SWITCH | I_FREEING) ||
-	    inode_to_wb(inode) == isw->new_wb)
-		goto out_unlock;
-
-	if (!atomic_inc_not_zero(&inode->i_sb->s_active))
-		goto out_unlock;
-
+	    inode_to_wb(inode) == isw->new_wb) {
+		spin_unlock(&inode->i_lock);
+		goto out_free;
+	}
 	inode->i_state |= I_WB_SWITCH;
 	spin_unlock(&inode->i_lock);
 
 	ihold(inode);
 	isw->inode = inode;
 
+	atomic_inc(&isw_nr_in_flight);
+
 	/*
 	 * In addition to synchronizing among switchers, I_WB_SWITCH tells
 	 * the RCU protected stat update paths to grab the mapping's
@@ -494,8 +496,6 @@ static void inode_switch_wbs(struct inod
 	call_rcu(&isw->rcu_head, inode_switch_wbs_rcu_fn);
 	return;
 
-out_unlock:
-	spin_unlock(&inode->i_lock);
 out_free:
 	if (isw->new_wb)
 		wb_put(isw->new_wb);
@@ -847,6 +847,33 @@ restart:
 		wb_put(last_wb);
 }
 
+/**
+ * cgroup_writeback_umount - flush inode wb switches for umount
+ *
+ * This function is called when a super_block is about to be destroyed and
+ * flushes in-flight inode wb switches.  An inode wb switch goes through
+ * RCU and then workqueue, so the two need to be flushed in order to ensure
+ * that all previously scheduled switches are finished.  As wb switches are
+ * rare occurrences and synchronize_rcu() can take a while, perform
+ * flushing iff wb switches are in flight.
+ */
+void cgroup_writeback_umount(void)
+{
+	if (atomic_read(&isw_nr_in_flight)) {
+		synchronize_rcu();
+		flush_workqueue(isw_wq);
+	}
+}
+
+static int __init cgroup_writeback_init(void)
+{
+	isw_wq = alloc_workqueue("inode_switch_wbs", 0, 0);
+	if (!isw_wq)
+		return -ENOMEM;
+	return 0;
+}
+fs_initcall(cgroup_writeback_init);
+
 #else	/* CONFIG_CGROUP_WRITEBACK */
 
 static struct bdi_writeback *
--- a/fs/super.c
+++ b/fs/super.c
@@ -414,6 +414,7 @@ void generic_shutdown_super(struct super
 		sync_filesystem(sb);
 		sb->s_flags &= ~MS_ACTIVE;
 
+		cgroup_writeback_umount();
 		fsnotify_unmount_inodes(sb);
 
 		evict_inodes(sb);
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -198,6 +198,7 @@ void wbc_attach_and_unlock_inode(struct
 void wbc_detach_inode(struct writeback_control *wbc);
 void wbc_account_io(struct writeback_control *wbc, struct page *page,
 		    size_t bytes);
+void cgroup_writeback_umount(void);
 
 /**
  * inode_attach_wb - associate an inode with its wb
@@ -301,6 +302,10 @@ static inline void wbc_account_io(struct
 {
 }
 
+static inline void cgroup_writeback_umount(void)
+{
+}
+
 #endif	/* CONFIG_CGROUP_WRITEBACK */
 
 /*

[toc] | [next] | [standalone]

#1346161 — Re: [PATCH block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

On Mon, Feb 29, 2016 at 03:47:24PM -0500, Tejun Heo wrote:
> If cgroup writeback is in use, inodes can be scheduled for
> asynchronous wb switching.  Before 5ff8eaac1636 ("writeback: keep
> superblock pinned during cgroup writeback association switches"), this
> could race with umount leading to super_block being destroyed while
> inodes are pinned for wb switching.  5ff8eaac1636 fixed it by bumping
> s_active while wb switches are in flight; however, this allowed
> in-flight wb switches to make umounts asynchronous when the userland
> expected synchronosity - e.g. fsck immediately following umount may
> fail because the device is still busy.
> 
> This patch removes the problematic super_block pinning and instead
> makes generic_shutdown_super() flush in-flight wb switches.  wb
> switches are now executed on a dedicated isw_wq so that they can be
> flushed and isw_nr_in_flight keeps track of the number of in-flight wb
> switches so that flushing can be avoided in most cases.

Wait a bloody minute.  What's to prevent shrink_dcache_for_umount() from
dirtying more inodes, triggering more of the same?

> -	if (!atomic_inc_not_zero(&inode->i_sb->s_active))
> -		goto out_unlock;

This would've failed for inodes on superblock in the middle of shutdown;
what's to do the same for the new variant?

[toc] | [prev] | [next] | [standalone]

#1346165 — Re: [PATCH block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

On Mon, Feb 29, 2016 at 08:54:28PM +0000, Al Viro wrote:
> > This patch removes the problematic super_block pinning and instead
> > makes generic_shutdown_super() flush in-flight wb switches.  wb
> > switches are now executed on a dedicated isw_wq so that they can be
> > flushed and isw_nr_in_flight keeps track of the number of in-flight wb
> > switches so that flushing can be avoided in most cases.
> 
> Wait a bloody minute.  What's to prevent shrink_dcache_for_umount() from
> dirtying more inodes, triggering more of the same?

Hmmm?  The flushing is done after shrink_dcache_for_umount() and
sync_filesystems().  Aren't inodes supposed to stay clean after that?

> > -	if (!atomic_inc_not_zero(&inode->i_sb->s_active))
> > -		goto out_unlock;
> 
> This would've failed for inodes on superblock in the middle of shutdown;
> what's to do the same for the new variant?

I don't follow.  As long as no new writeback operations are initiated
after flushing, none can be in flight for the super_block.  Isn't that
enough?

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1346169 — Re: [PATCH block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

On Mon, Feb 29, 2016 at 09:06:15PM +0000, Al Viro wrote:
> > Hmmm?  The flushing is done after shrink_dcache_for_umount() and
> > sync_filesystems().  Aren't inodes supposed to stay clean after that?
> 
> s/shrink_dcache_for_umount/fsnotify_unmount_inodes/ - sorry.

Is that allowed to dirty indoes and initiate writebacks again, after
sync_filesystems() is done?  That sounds weird but it's trivial to
move cgroup_writeback_umount() below that if so.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1346178 — Re: [PATCH block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

On Mon 29-02-16 16:08:00, Tejun Heo wrote:
> On Mon, Feb 29, 2016 at 09:06:15PM +0000, Al Viro wrote:
> > > Hmmm?  The flushing is done after shrink_dcache_for_umount() and
> > > sync_filesystems().  Aren't inodes supposed to stay clean after that?
> > 
> > s/shrink_dcache_for_umount/fsnotify_unmount_inodes/ - sorry.
> 
> Is that allowed to dirty indoes and initiate writebacks again, after
> sync_filesystems() is done?  That sounds weird but it's trivial to
> move cgroup_writeback_umount() below that if so.

Hardly, but generally it is true that filesystem may still dirty something
(e.g. from outstanding workqueue work, but most likely some special "system
inodes" may still become dirty) until ->put_super() is finished.

Anyway, to make this foolproof, I'd just avoid queueing any new switching
work after S_ACTIVE is cleared on the superblock and flush the workqueue just
before evict_inodes() call.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1346170 — Re: [PATCH block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

On Mon, Feb 29, 2016 at 03:58:37PM -0500, Tejun Heo wrote:
> On Mon, Feb 29, 2016 at 08:54:28PM +0000, Al Viro wrote:
> > > This patch removes the problematic super_block pinning and instead
> > > makes generic_shutdown_super() flush in-flight wb switches.  wb
> > > switches are now executed on a dedicated isw_wq so that they can be
> > > flushed and isw_nr_in_flight keeps track of the number of in-flight wb
> > > switches so that flushing can be avoided in most cases.
> > 
> > Wait a bloody minute.  What's to prevent shrink_dcache_for_umount() from
> > dirtying more inodes, triggering more of the same?
> 
> Hmmm?  The flushing is done after shrink_dcache_for_umount() and
> sync_filesystems().  Aren't inodes supposed to stay clean after that?

s/shrink_dcache_for_umount/fsnotify_unmount_inodes/ - sorry.

[toc] | [prev] | [next] | [standalone]

#1346250 — [PATCH v2 block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

If cgroup writeback is in use, inodes can be scheduled for
asynchronous wb switching.  Before 5ff8eaac1636 ("writeback: keep
superblock pinned during cgroup writeback association switches"), this
could race with umount leading to super_block being destroyed while
inodes are pinned for wb switching.  5ff8eaac1636 fixed it by bumping
s_active while wb switches are in flight; however, this allowed
in-flight wb switches to make umounts asynchronous when the userland
expected synchronosity - e.g. fsck immediately following umount may
fail because the device is still busy.

This patch removes the problematic super_block pinning and instead
makes generic_shutdown_super() flush in-flight wb switches.  wb
switches are now executed on a dedicated isw_wq so that they can be
flushed and isw_nr_in_flight keeps track of the number of in-flight wb
switches so that flushing can be avoided in most cases.

v2: Move cgroup_writeback_umount() further below and add MS_ACTIVE
    check in inode_switch_wbs() as Jan an Al suggested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Tahsin Erdogan <tahsin@google.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Link: http://lkml.kernel.org/g/CAAeU0aNCq7LGODvVGRU-oU_o-6enii5ey0p1c26D1ZzYwkDc5A@mail.gmail.com
Fixes: 5ff8eaac1636 ("writeback: keep superblock pinned during cgroup writeback association switches")
Cc: stable@vger.kernel.org #v4.5
---
 fs/fs-writeback.c         |   54 ++++++++++++++++++++++++++++++++++------------
 fs/super.c                |    1 
 include/linux/writeback.h |    5 ++++
 3 files changed, 47 insertions(+), 13 deletions(-)

--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -223,6 +223,9 @@ static void wb_wait_for_completion(struc
 #define WB_FRN_HIST_MAX_SLOTS	(WB_FRN_HIST_THR_SLOTS / 2 + 1)
 					/* one round can affect upto 5 slots */
 
+static atomic_t isw_nr_in_flight = ATOMIC_INIT(0);
+static struct workqueue_struct *isw_wq;
+
 void __inode_attach_wb(struct inode *inode, struct page *page)
 {
 	struct backing_dev_info *bdi = inode_to_bdi(inode);
@@ -317,7 +320,6 @@ static void inode_switch_wbs_work_fn(str
 	struct inode_switch_wbs_context *isw =
 		container_of(work, struct inode_switch_wbs_context, work);
 	struct inode *inode = isw->inode;
-	struct super_block *sb = inode->i_sb;
 	struct address_space *mapping = inode->i_mapping;
 	struct bdi_writeback *old_wb = inode->i_wb;
 	struct bdi_writeback *new_wb = isw->new_wb;
@@ -424,8 +426,9 @@ skip_switch:
 	wb_put(new_wb);
 
 	iput(inode);
-	deactivate_super(sb);
 	kfree(isw);
+
+	atomic_dec(&isw_nr_in_flight);
 }
 
 static void inode_switch_wbs_rcu_fn(struct rcu_head *rcu_head)
@@ -435,7 +438,7 @@ static void inode_switch_wbs_rcu_fn(stru
 
 	/* needs to grab bh-unsafe locks, bounce to work item */
 	INIT_WORK(&isw->work, inode_switch_wbs_work_fn);
-	schedule_work(&isw->work);
+	queue_work(isw_wq, &isw->work);
 }
 
 /**
@@ -471,20 +474,20 @@ static void inode_switch_wbs(struct inod
 
 	/* while holding I_WB_SWITCH, no one else can update the association */
 	spin_lock(&inode->i_lock);
-
-	if (inode->i_state & (I_WB_SWITCH | I_FREEING) ||
-	    inode_to_wb(inode) == isw->new_wb)
-		goto out_unlock;
-
-	if (!atomic_inc_not_zero(&inode->i_sb->s_active))
-		goto out_unlock;
-
+	if (!(inode->i_sb->s_flags & MS_ACTIVE) ||
+	    inode->i_state & (I_WB_SWITCH | I_FREEING) ||
+	    inode_to_wb(inode) == isw->new_wb) {
+		spin_unlock(&inode->i_lock);
+		goto out_free;
+	}
 	inode->i_state |= I_WB_SWITCH;
 	spin_unlock(&inode->i_lock);
 
 	ihold(inode);
 	isw->inode = inode;
 
+	atomic_inc(&isw_nr_in_flight);
+
 	/*
 	 * In addition to synchronizing among switchers, I_WB_SWITCH tells
 	 * the RCU protected stat update paths to grab the mapping's
@@ -494,8 +497,6 @@ static void inode_switch_wbs(struct inod
 	call_rcu(&isw->rcu_head, inode_switch_wbs_rcu_fn);
 	return;
 
-out_unlock:
-	spin_unlock(&inode->i_lock);
 out_free:
 	if (isw->new_wb)
 		wb_put(isw->new_wb);
@@ -847,6 +848,33 @@ restart:
 		wb_put(last_wb);
 }
 
+/**
+ * cgroup_writeback_umount - flush inode wb switches for umount
+ *
+ * This function is called when a super_block is about to be destroyed and
+ * flushes in-flight inode wb switches.  An inode wb switch goes through
+ * RCU and then workqueue, so the two need to be flushed in order to ensure
+ * that all previously scheduled switches are finished.  As wb switches are
+ * rare occurrences and synchronize_rcu() can take a while, perform
+ * flushing iff wb switches are in flight.
+ */
+void cgroup_writeback_umount(void)
+{
+	if (atomic_read(&isw_nr_in_flight)) {
+		synchronize_rcu();
+		flush_workqueue(isw_wq);
+	}
+}
+
+static int __init cgroup_writeback_init(void)
+{
+	isw_wq = alloc_workqueue("inode_switch_wbs", 0, 0);
+	if (!isw_wq)
+		return -ENOMEM;
+	return 0;
+}
+fs_initcall(cgroup_writeback_init);
+
 #else	/* CONFIG_CGROUP_WRITEBACK */
 
 static struct bdi_writeback *
--- a/fs/super.c
+++ b/fs/super.c
@@ -415,6 +415,7 @@ void generic_shutdown_super(struct super
 		sb->s_flags &= ~MS_ACTIVE;
 
 		fsnotify_unmount_inodes(sb);
+		cgroup_writeback_umount();
 
 		evict_inodes(sb);
 
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -198,6 +198,7 @@ void wbc_attach_and_unlock_inode(struct
 void wbc_detach_inode(struct writeback_control *wbc);
 void wbc_account_io(struct writeback_control *wbc, struct page *page,
 		    size_t bytes);
+void cgroup_writeback_umount(void);
 
 /**
  * inode_attach_wb - associate an inode with its wb
@@ -301,6 +302,10 @@ static inline void wbc_account_io(struct
 {
 }
 
+static inline void cgroup_writeback_umount(void)
+{
+}
+
 #endif	/* CONFIG_CGROUP_WRITEBACK */
 
 /*

[toc] | [prev] | [next] | [standalone]

#1346475 — Re: [PATCH v2 block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

On Mon 29-02-16 18:28:53, Tejun Heo wrote:
> If cgroup writeback is in use, inodes can be scheduled for
> asynchronous wb switching.  Before 5ff8eaac1636 ("writeback: keep
> superblock pinned during cgroup writeback association switches"), this
> could race with umount leading to super_block being destroyed while
> inodes are pinned for wb switching.  5ff8eaac1636 fixed it by bumping
> s_active while wb switches are in flight; however, this allowed
> in-flight wb switches to make umounts asynchronous when the userland
> expected synchronosity - e.g. fsck immediately following umount may
> fail because the device is still busy.
> 
> This patch removes the problematic super_block pinning and instead
> makes generic_shutdown_super() flush in-flight wb switches.  wb
> switches are now executed on a dedicated isw_wq so that they can be
> flushed and isw_nr_in_flight keeps track of the number of in-flight wb
> switches so that flushing can be avoided in most cases.
> 
> v2: Move cgroup_writeback_umount() further below and add MS_ACTIVE
>     check in inode_switch_wbs() as Jan an Al suggested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Tahsin Erdogan <tahsin@google.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Link: http://lkml.kernel.org/g/CAAeU0aNCq7LGODvVGRU-oU_o-6enii5ey0p1c26D1ZzYwkDc5A@mail.gmail.com
> Fixes: 5ff8eaac1636 ("writeback: keep superblock pinned during cgroup writeback association switches")
> Cc: stable@vger.kernel.org #v4.5

The patch looks good to me now. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

									Honza

> ---
>  fs/fs-writeback.c         |   54 ++++++++++++++++++++++++++++++++++------------
>  fs/super.c                |    1 
>  include/linux/writeback.h |    5 ++++
>  3 files changed, 47 insertions(+), 13 deletions(-)
> 
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -223,6 +223,9 @@ static void wb_wait_for_completion(struc
>  #define WB_FRN_HIST_MAX_SLOTS	(WB_FRN_HIST_THR_SLOTS / 2 + 1)
>  					/* one round can affect upto 5 slots */
>  
> +static atomic_t isw_nr_in_flight = ATOMIC_INIT(0);
> +static struct workqueue_struct *isw_wq;
> +
>  void __inode_attach_wb(struct inode *inode, struct page *page)
>  {
>  	struct backing_dev_info *bdi = inode_to_bdi(inode);
> @@ -317,7 +320,6 @@ static void inode_switch_wbs_work_fn(str
>  	struct inode_switch_wbs_context *isw =
>  		container_of(work, struct inode_switch_wbs_context, work);
>  	struct inode *inode = isw->inode;
> -	struct super_block *sb = inode->i_sb;
>  	struct address_space *mapping = inode->i_mapping;
>  	struct bdi_writeback *old_wb = inode->i_wb;
>  	struct bdi_writeback *new_wb = isw->new_wb;
> @@ -424,8 +426,9 @@ skip_switch:
>  	wb_put(new_wb);
>  
>  	iput(inode);
> -	deactivate_super(sb);
>  	kfree(isw);
> +
> +	atomic_dec(&isw_nr_in_flight);
>  }
>  
>  static void inode_switch_wbs_rcu_fn(struct rcu_head *rcu_head)
> @@ -435,7 +438,7 @@ static void inode_switch_wbs_rcu_fn(stru
>  
>  	/* needs to grab bh-unsafe locks, bounce to work item */
>  	INIT_WORK(&isw->work, inode_switch_wbs_work_fn);
> -	schedule_work(&isw->work);
> +	queue_work(isw_wq, &isw->work);
>  }
>  
>  /**
> @@ -471,20 +474,20 @@ static void inode_switch_wbs(struct inod
>  
>  	/* while holding I_WB_SWITCH, no one else can update the association */
>  	spin_lock(&inode->i_lock);
> -
> -	if (inode->i_state & (I_WB_SWITCH | I_FREEING) ||
> -	    inode_to_wb(inode) == isw->new_wb)
> -		goto out_unlock;
> -
> -	if (!atomic_inc_not_zero(&inode->i_sb->s_active))
> -		goto out_unlock;
> -
> +	if (!(inode->i_sb->s_flags & MS_ACTIVE) ||
> +	    inode->i_state & (I_WB_SWITCH | I_FREEING) ||
> +	    inode_to_wb(inode) == isw->new_wb) {
> +		spin_unlock(&inode->i_lock);
> +		goto out_free;
> +	}
>  	inode->i_state |= I_WB_SWITCH;
>  	spin_unlock(&inode->i_lock);
>  
>  	ihold(inode);
>  	isw->inode = inode;
>  
> +	atomic_inc(&isw_nr_in_flight);
> +
>  	/*
>  	 * In addition to synchronizing among switchers, I_WB_SWITCH tells
>  	 * the RCU protected stat update paths to grab the mapping's
> @@ -494,8 +497,6 @@ static void inode_switch_wbs(struct inod
>  	call_rcu(&isw->rcu_head, inode_switch_wbs_rcu_fn);
>  	return;
>  
> -out_unlock:
> -	spin_unlock(&inode->i_lock);
>  out_free:
>  	if (isw->new_wb)
>  		wb_put(isw->new_wb);
> @@ -847,6 +848,33 @@ restart:
>  		wb_put(last_wb);
>  }
>  
> +/**
> + * cgroup_writeback_umount - flush inode wb switches for umount
> + *
> + * This function is called when a super_block is about to be destroyed and
> + * flushes in-flight inode wb switches.  An inode wb switch goes through
> + * RCU and then workqueue, so the two need to be flushed in order to ensure
> + * that all previously scheduled switches are finished.  As wb switches are
> + * rare occurrences and synchronize_rcu() can take a while, perform
> + * flushing iff wb switches are in flight.
> + */
> +void cgroup_writeback_umount(void)
> +{
> +	if (atomic_read(&isw_nr_in_flight)) {
> +		synchronize_rcu();
> +		flush_workqueue(isw_wq);
> +	}
> +}
> +
> +static int __init cgroup_writeback_init(void)
> +{
> +	isw_wq = alloc_workqueue("inode_switch_wbs", 0, 0);
> +	if (!isw_wq)
> +		return -ENOMEM;
> +	return 0;
> +}
> +fs_initcall(cgroup_writeback_init);
> +
>  #else	/* CONFIG_CGROUP_WRITEBACK */
>  
>  static struct bdi_writeback *
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -415,6 +415,7 @@ void generic_shutdown_super(struct super
>  		sb->s_flags &= ~MS_ACTIVE;
>  
>  		fsnotify_unmount_inodes(sb);
> +		cgroup_writeback_umount();
>  
>  		evict_inodes(sb);
>  
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -198,6 +198,7 @@ void wbc_attach_and_unlock_inode(struct
>  void wbc_detach_inode(struct writeback_control *wbc);
>  void wbc_account_io(struct writeback_control *wbc, struct page *page,
>  		    size_t bytes);
> +void cgroup_writeback_umount(void);
>  
>  /**
>   * inode_attach_wb - associate an inode with its wb
> @@ -301,6 +302,10 @@ static inline void wbc_account_io(struct
>  {
>  }
>  
> +static inline void cgroup_writeback_umount(void)
> +{
> +}
> +
>  #endif	/* CONFIG_CGROUP_WRITEBACK */
>  
>  /*
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1346807 — Re: [PATCH v2 block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

On 02/29/2016 04:28 PM, Tejun Heo wrote:
> If cgroup writeback is in use, inodes can be scheduled for
> asynchronous wb switching.  Before 5ff8eaac1636 ("writeback: keep
> superblock pinned during cgroup writeback association switches"), this
> could race with umount leading to super_block being destroyed while
> inodes are pinned for wb switching.  5ff8eaac1636 fixed it by bumping
> s_active while wb switches are in flight; however, this allowed
> in-flight wb switches to make umounts asynchronous when the userland
> expected synchronosity - e.g. fsck immediately following umount may
> fail because the device is still busy.
>
> This patch removes the problematic super_block pinning and instead
> makes generic_shutdown_super() flush in-flight wb switches.  wb
> switches are now executed on a dedicated isw_wq so that they can be
> flushed and isw_nr_in_flight keeps track of the number of in-flight wb
> switches so that flushing can be avoided in most cases.
>
> v2: Move cgroup_writeback_umount() further below and add MS_ACTIVE
>      check in inode_switch_wbs() as Jan an Al suggested.

I queued this up for 4.5, but I'm feeling a bit uneasy about it. But 
it's either that, or revert 5ff8eaac1636 and fix it for real in 4.6. 
Jan/Tejun, what do you think?

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1346821 — Re: [PATCH v2 block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

Hello, Jens.

On Tue, Mar 1, 2016 at 12:46 PM, Jens Axboe <axboe@kernel.dk> wrote:
> I queued this up for 4.5, but I'm feeling a bit uneasy about it. But it's
> either that, or revert 5ff8eaac1636 and fix it for real in 4.6. Jan/Tejun,
> what do you think?

Given that this only matters for cgroup writeback cases, this should
still be fairly low impact, so I don't think it'd matter too much
whether we fix this in this cycle or for 4.6. However, that also means
that we're not risking much by doing it in this cycle, so I'd vote for
doing it now.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1347927 — Re: [PATCH v2 block/for-linus] writeback: flush inode cgroup wb switches instead of pinning super_block

On Tue 01-03-16 12:50:19, Tejun Heo wrote:
> Hello, Jens.
> 
> On Tue, Mar 1, 2016 at 12:46 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > I queued this up for 4.5, but I'm feeling a bit uneasy about it. But it's
> > either that, or revert 5ff8eaac1636 and fix it for real in 4.6. Jan/Tejun,
> > what do you think?
> 
> Given that this only matters for cgroup writeback cases, this should
> still be fairly low impact, so I don't think it'd matter too much
> whether we fix this in this cycle or for 4.6. However, that also means
> that we're not risking much by doing it in this cycle, so I'd vote for
> doing it now.

Yeah, without CGROUP_WRITEBACK enabled this patch is NOP so I don't care
much when this gets merged.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1346622

On Mon, Feb 29, 2016 at 12:47 PM, Tejun Heo <tj@kernel.org> wrote:
> Tahsin, can you please verify that this removes the asynchronous
> behavior while still avoiding the original issue?
>

I verified that this fixes the original issue and also removes the
asynchronous umount behavior.

Thanks a lot for the fix.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web