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

Groups > linux.kernel > #1335700 > unrolled thread

[PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

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-4.5-fixes] writeback: keep superblock pinned during  cgroup writeback association switches Tejun Heo <tj@kernel.org> - 2016-02-16 19:30 +0100
    Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Jens Axboe <axboe@kernel.dk> - 2016-02-16 19:40 +0100
    Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Jan Kara <jack@suse.cz> - 2016-02-17 22:00 +0100
      Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Tejun Heo <tj@kernel.org> - 2016-02-17 22:10 +0100
        Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Jan Kara <jack@suse.cz> - 2016-02-17 23:30 +0100
          Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Tahsin Erdogan <tahsin@google.com> - 2016-02-17 23:50 +0100
            Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Tejun Heo <tj@kernel.org> - 2016-02-18 00:10 +0100
              Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Jan Kara <jack@suse.cz> - 2016-02-18 11:00 +0100
                Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Tejun Heo <tj@kernel.org> - 2016-02-18 14:10 +0100
                  Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Jan Kara <jack@suse.cz> - 2016-02-18 14:30 +0100
                  Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-19 21:20 +0100
                    Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Tejun Heo <tj@kernel.org> - 2016-02-19 22:00 +0100
                      Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-19 23:00 +0100
                        Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Tejun Heo <tj@kernel.org> - 2016-02-19 23:20 +0100
                          Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Al Viro <viro@ZenIV.linux.org.uk> - 2016-02-19 23:30 +0100
    Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Nikolay Borisov <kernel@kyup.com> - 2016-02-18 11:20 +0100
      Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned  during cgroup writeback association switches Tejun Heo <tj@kernel.org> - 2016-02-18 14:00 +0100

#1335700 — [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

From 586afaa034bec88934bad4eb6ab38ba07031ec5a Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Tue, 16 Feb 2016 13:14:35 -0500

If cgroup writeback is in use, an inode is associated with a cgroup
for writeback.  If the inode's main dirtier changes to another cgroup,
the association gets updated asynchronously.  Nothing was pinning the
superblock while such switches are in progress and superblock could go
away while async switching is pending or in progress leading to
crashes like the following.

 kernel BUG at fs/jbd2/transaction.c:319!
 invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
 CPU: 1 PID: 29158 Comm: kworker/1:10 Not tainted 4.5.0-rc3 #51
 Hardware name: Google Google, BIOS Google 01/01/2011
 Workqueue: events inode_switch_wbs_work_fn
 task: ffff880213dbbd40 ti: ffff880209264000 task.ti: ffff880209264000
 RIP: 0010:[<ffffffff803e6922>]  [<ffffffff803e6922>] start_this_handle+0x382/0x3e0
 RSP: 0018:ffff880209267c30  EFLAGS: 00010202
 ...
 Call Trace:
  [<ffffffff803e6be4>] jbd2__journal_start+0xf4/0x190
  [<ffffffff803cfc7e>] __ext4_journal_start_sb+0x4e/0x70
  [<ffffffff803b31ec>] ext4_evict_inode+0x12c/0x3d0
  [<ffffffff8035338b>] evict+0xbb/0x190
  [<ffffffff80354190>] iput+0x130/0x190
  [<ffffffff80360223>] inode_switch_wbs_work_fn+0x343/0x4c0
  [<ffffffff80279819>] process_one_work+0x129/0x300
  [<ffffffff80279b16>] worker_thread+0x126/0x480
  [<ffffffff8027ed14>] kthread+0xc4/0xe0
  [<ffffffff809771df>] ret_from_fork+0x3f/0x70

Fix it by bumping s_active while cgroup association switching is in
flight.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-and-tested-by: Tahsin Erdogan <tahsin@google.com>
Link: http://lkml.kernel.org/g/CAAeU0aNCq7LGODvVGRU-oU_o-6enii5ey0p1c26D1ZzYwkDc5A@mail.gmail.com
Fixes: d10c80955265 ("writeback: implement foreign cgroup inode bdi_writeback switching")
Cc: stable@vger.kernel.org #v4.5+
---
 fs/fs-writeback.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 6915c95..1f76d89 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -317,6 +317,7 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
 	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;
@@ -423,6 +424,7 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
 	wb_put(new_wb);
 
 	iput(inode);
+	deactivate_super(sb);
 	kfree(isw);
 }
 
@@ -469,11 +471,14 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
 
 	/* 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) {
-		spin_unlock(&inode->i_lock);
-		goto out_free;
-	}
+	    inode_to_wb(inode) == isw->new_wb)
+		goto out_unlock;
+
+	if (!atomic_inc_not_zero(&inode->i_sb->s_active))
+		goto out_unlock;
+
 	inode->i_state |= I_WB_SWITCH;
 	spin_unlock(&inode->i_lock);
 
@@ -489,6 +494,8 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
 	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);
-- 
2.5.0

[toc] | [next] | [standalone]

#1335716 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On 02/16/2016 11:24 AM, Tejun Heo wrote:
>  From 586afaa034bec88934bad4eb6ab38ba07031ec5a Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Tue, 16 Feb 2016 13:14:35 -0500
>
> If cgroup writeback is in use, an inode is associated with a cgroup
> for writeback.  If the inode's main dirtier changes to another cgroup,
> the association gets updated asynchronously.  Nothing was pinning the
> superblock while such switches are in progress and superblock could go
> away while async switching is pending or in progress leading to
> crashes like the following.
>
>   kernel BUG at fs/jbd2/transaction.c:319!
>   invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
>   CPU: 1 PID: 29158 Comm: kworker/1:10 Not tainted 4.5.0-rc3 #51
>   Hardware name: Google Google, BIOS Google 01/01/2011
>   Workqueue: events inode_switch_wbs_work_fn
>   task: ffff880213dbbd40 ti: ffff880209264000 task.ti: ffff880209264000
>   RIP: 0010:[<ffffffff803e6922>]  [<ffffffff803e6922>] start_this_handle+0x382/0x3e0
>   RSP: 0018:ffff880209267c30  EFLAGS: 00010202
>   ...
>   Call Trace:
>    [<ffffffff803e6be4>] jbd2__journal_start+0xf4/0x190
>    [<ffffffff803cfc7e>] __ext4_journal_start_sb+0x4e/0x70
>    [<ffffffff803b31ec>] ext4_evict_inode+0x12c/0x3d0
>    [<ffffffff8035338b>] evict+0xbb/0x190
>    [<ffffffff80354190>] iput+0x130/0x190
>    [<ffffffff80360223>] inode_switch_wbs_work_fn+0x343/0x4c0
>    [<ffffffff80279819>] process_one_work+0x129/0x300
>    [<ffffffff80279b16>] worker_thread+0x126/0x480
>    [<ffffffff8027ed14>] kthread+0xc4/0xe0
>    [<ffffffff809771df>] ret_from_fork+0x3f/0x70
>
> Fix it by bumping s_active while cgroup association switching is in
> flight.

Added for 4.5.

-- 
Jens Axboe

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

#1336751 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On Tue 16-02-16 13:24:57, Tejun Heo wrote:
> From 586afaa034bec88934bad4eb6ab38ba07031ec5a Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Tue, 16 Feb 2016 13:14:35 -0500
> 
> If cgroup writeback is in use, an inode is associated with a cgroup
> for writeback.  If the inode's main dirtier changes to another cgroup,
> the association gets updated asynchronously.  Nothing was pinning the
> superblock while such switches are in progress and superblock could go
> away while async switching is pending or in progress leading to
> crashes like the following.
> 
>  kernel BUG at fs/jbd2/transaction.c:319!
>  invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
>  CPU: 1 PID: 29158 Comm: kworker/1:10 Not tainted 4.5.0-rc3 #51
>  Hardware name: Google Google, BIOS Google 01/01/2011
>  Workqueue: events inode_switch_wbs_work_fn
>  task: ffff880213dbbd40 ti: ffff880209264000 task.ti: ffff880209264000
>  RIP: 0010:[<ffffffff803e6922>]  [<ffffffff803e6922>] start_this_handle+0x382/0x3e0
>  RSP: 0018:ffff880209267c30  EFLAGS: 00010202
>  ...
>  Call Trace:
>   [<ffffffff803e6be4>] jbd2__journal_start+0xf4/0x190
>   [<ffffffff803cfc7e>] __ext4_journal_start_sb+0x4e/0x70
>   [<ffffffff803b31ec>] ext4_evict_inode+0x12c/0x3d0
>   [<ffffffff8035338b>] evict+0xbb/0x190
>   [<ffffffff80354190>] iput+0x130/0x190
>   [<ffffffff80360223>] inode_switch_wbs_work_fn+0x343/0x4c0
>   [<ffffffff80279819>] process_one_work+0x129/0x300
>   [<ffffffff80279b16>] worker_thread+0x126/0x480
>   [<ffffffff8027ed14>] kthread+0xc4/0xe0
>   [<ffffffff809771df>] ret_from_fork+0x3f/0x70
> 
> Fix it by bumping s_active while cgroup association switching is in
> flight.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-and-tested-by: Tahsin Erdogan <tahsin@google.com>
> Link: http://lkml.kernel.org/g/CAAeU0aNCq7LGODvVGRU-oU_o-6enii5ey0p1c26D1ZzYwkDc5A@mail.gmail.com
> Fixes: d10c80955265 ("writeback: implement foreign cgroup inode bdi_writeback switching")
> Cc: stable@vger.kernel.org #v4.5+

Well, but this has the side-effect that trying to umount a filesystem while
migrations are happening will result in EBUSY error. Without obvious reason
why that happens. As an admin I would be rather upset when umount sometimes
returns EBUSY without apparent reason and you have to basically implement a
loop around umount to make it reliable. So a nack from me for this patch.

Traditionally, we have used sb->s_count and sb->s_umount semaphore to pin
superblock while writeback code was working on it. That makes umount block
until we can safely unmount the filesystem and thus doesn't result in these
spurious EBUSY errors. But from a quick look this can be problematic for the
cgroup setting.

Alternatively, you could either cancel all the switching work when
unmounting filesystem or maybe just handle I_WB_SWITCH similarly to I_SYNC
- don't grab inode reference when switching is going on, just make
I_WB_SWITCH pin the inode and wait in evict() for it to be clear (similarly
as we call inode_wait_for_writeback() there).

								Honza
> ---
>  fs/fs-writeback.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 6915c95..1f76d89 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -317,6 +317,7 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
>  	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;
> @@ -423,6 +424,7 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
>  	wb_put(new_wb);
>  
>  	iput(inode);
> +	deactivate_super(sb);
>  	kfree(isw);
>  }
>  
> @@ -469,11 +471,14 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
>  
>  	/* 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) {
> -		spin_unlock(&inode->i_lock);
> -		goto out_free;
> -	}
> +	    inode_to_wb(inode) == isw->new_wb)
> +		goto out_unlock;
> +
> +	if (!atomic_inc_not_zero(&inode->i_sb->s_active))
> +		goto out_unlock;
> +
>  	inode->i_state |= I_WB_SWITCH;
>  	spin_unlock(&inode->i_lock);
>  
> @@ -489,6 +494,8 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
>  	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);
> -- 
> 2.5.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

#1336765 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

Hello, Jan.

On Wed, Feb 17, 2016 at 09:57:21PM +0100, Jan Kara wrote:
> Well, but this has the side-effect that trying to umount a filesystem while
> migrations are happening will result in EBUSY error. Without obvious reason
> why that happens. As an admin I would be rather upset when umount sometimes
> returns EBUSY without apparent reason and you have to basically implement a
> loop around umount to make it reliable. So a nack from me for this patch.

I see.  Can you please point me to the s_active check during umount?
I first tried s_umount but couldn't transfer its ownership to the
worker so ended up doing s_active.  I looked at how s_active is used
and couldn't find where it'd block umount.  may_umount() checks
mnt_count, not s_active, so it looked like holding s_active may delay
destruction of the superblock but not prevent umount.

> Traditionally, we have used sb->s_count and sb->s_umount semaphore to pin
> superblock while writeback code was working on it. That makes umount block
> until we can safely unmount the filesystem and thus doesn't result in these
> spurious EBUSY errors. But from a quick look this can be problematic for the
> cgroup setting.
> 
> Alternatively, you could either cancel all the switching work when
> unmounting filesystem or maybe just handle I_WB_SWITCH similarly to I_SYNC
> - don't grab inode reference when switching is going on, just make
> I_WB_SWITCH pin the inode and wait in evict() for it to be clear (similarly
> as we call inode_wait_for_writeback() there).

Yeah, this is an alternative but likely more involved.

Thanks.

-- 
tejun

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

#1336819 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On Wed 17-02-16 16:07:44, Tejun Heo wrote:
> Hello, Jan.
> 
> On Wed, Feb 17, 2016 at 09:57:21PM +0100, Jan Kara wrote:
> > Well, but this has the side-effect that trying to umount a filesystem while
> > migrations are happening will result in EBUSY error. Without obvious reason
> > why that happens. As an admin I would be rather upset when umount sometimes
> > returns EBUSY without apparent reason and you have to basically implement a
> > loop around umount to make it reliable. So a nack from me for this patch.
> 
> I see.  Can you please point me to the s_active check during umount?
> I first tried s_umount but couldn't transfer its ownership to the
> worker so ended up doing s_active.  I looked at how s_active is used
> and couldn't find where it'd block umount.  may_umount() checks
> mnt_count, not s_active, so it looked like holding s_active may delay
> destruction of the superblock but not prevent umount.

Bah, sorry. It's too late here. You are right that s_active will just delay
destruction of the superblock until the reference is dropped. So I don't
see obvious issues with what you do and I retract my nack. I still feel
somewhat uneasy about postponing fs shutdown to a workqueue like this but
hopefully there's no hidden catch.

								Honza


> > Traditionally, we have used sb->s_count and sb->s_umount semaphore to pin
> > superblock while writeback code was working on it. That makes umount block
> > until we can safely unmount the filesystem and thus doesn't result in these
> > spurious EBUSY errors. But from a quick look this can be problematic for the
> > cgroup setting.
> > 
> > Alternatively, you could either cancel all the switching work when
> > unmounting filesystem or maybe just handle I_WB_SWITCH similarly to I_SYNC
> > - don't grab inode reference when switching is going on, just make
> > I_WB_SWITCH pin the inode and wait in evict() for it to be clear (similarly
> > as we call inode_wait_for_writeback() there).
> 
> Yeah, this is an alternative but likely more involved.
> 
> Thanks.
> 
> -- 
> tejun
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

#1336842 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

With this patch, I am starting to have issues running fsck immediately
after umount.

*** fsck.ext4 output ***
fsck from util-linux-ng 2.17.2
e2fsck 1.42.12-gg3 (9-Sep-2014)
Warning!  /dev/sdb3 is in use.
Pass 1: Checking inodes, blocks, and sizes
Deleted inode 62346243 has zero dtime.  Fix? no


On Wed, Feb 17, 2016 at 2:30 PM, Jan Kara <jack@suse.cz> wrote:
> On Wed 17-02-16 16:07:44, Tejun Heo wrote:
>> Hello, Jan.
>>
>> On Wed, Feb 17, 2016 at 09:57:21PM +0100, Jan Kara wrote:
>> > Well, but this has the side-effect that trying to umount a filesystem while
>> > migrations are happening will result in EBUSY error. Without obvious reason
>> > why that happens. As an admin I would be rather upset when umount sometimes
>> > returns EBUSY without apparent reason and you have to basically implement a
>> > loop around umount to make it reliable. So a nack from me for this patch.
>>
>> I see.  Can you please point me to the s_active check during umount?
>> I first tried s_umount but couldn't transfer its ownership to the
>> worker so ended up doing s_active.  I looked at how s_active is used
>> and couldn't find where it'd block umount.  may_umount() checks
>> mnt_count, not s_active, so it looked like holding s_active may delay
>> destruction of the superblock but not prevent umount.
>
> Bah, sorry. It's too late here. You are right that s_active will just delay
> destruction of the superblock until the reference is dropped. So I don't
> see obvious issues with what you do and I retract my nack. I still feel
> somewhat uneasy about postponing fs shutdown to a workqueue like this but
> hopefully there's no hidden catch.
>
>                                                                 Honza
>
>
>> > Traditionally, we have used sb->s_count and sb->s_umount semaphore to pin
>> > superblock while writeback code was working on it. That makes umount block
>> > until we can safely unmount the filesystem and thus doesn't result in these
>> > spurious EBUSY errors. But from a quick look this can be problematic for the
>> > cgroup setting.
>> >
>> > Alternatively, you could either cancel all the switching work when
>> > unmounting filesystem or maybe just handle I_WB_SWITCH similarly to I_SYNC
>> > - don't grab inode reference when switching is going on, just make
>> > I_WB_SWITCH pin the inode and wait in evict() for it to be clear (similarly
>> > as we call inode_wait_for_writeback() there).
>>
>> Yeah, this is an alternative but likely more involved.
>>
>> Thanks.
>>
>> --
>> tejun
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

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

#1336859 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On Wed, Feb 17, 2016 at 02:41:25PM -0800, Tahsin Erdogan wrote:
> With this patch, I am starting to have issues running fsck immediately
> after umount.
> 
> *** fsck.ext4 output ***
> fsck from util-linux-ng 2.17.2
> e2fsck 1.42.12-gg3 (9-Sep-2014)
> Warning!  /dev/sdb3 is in use.
> Pass 1: Checking inodes, blocks, and sizes
> Deleted inode 62346243 has zero dtime.  Fix? no

Yeah, that'd be one of the side effects.  Will think more about it.
Jan, do you know why we have both s_active and the rwsem at this
point?  I can't tell why both are needed.

Thanks.

-- 
tejun

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

#1337217 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On Wed 17-02-16 18:02:31, Tejun Heo wrote:
> On Wed, Feb 17, 2016 at 02:41:25PM -0800, Tahsin Erdogan wrote:
> > With this patch, I am starting to have issues running fsck immediately
> > after umount.
> > 
> > *** fsck.ext4 output ***
> > fsck from util-linux-ng 2.17.2
> > e2fsck 1.42.12-gg3 (9-Sep-2014)
> > Warning!  /dev/sdb3 is in use.
> > Pass 1: Checking inodes, blocks, and sizes
> > Deleted inode 62346243 has zero dtime.  Fix? no
> 
> Yeah, that'd be one of the side effects.  Will think more about it.
> Jan, do you know why we have both s_active and the rwsem at this
> point?  I can't tell why both are needed.

I'm not sure I understand the question. Do you mean why both s_active and
s_umount rwsem exist? s_active is a reference count keeping superblock
alive - e.g. if the filesystem is mounted in more places, we need a
reference for each mountpoint. s_umount is used when we want to block any
umount operation until we are done. For example sync(2) is using it to make
sure superblock doesn't disappear and so that we don't keep superblock
alive after admin called umount(2).

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

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

#1337340 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

Hello, Jan.

On Thu, Feb 18, 2016 at 10:55:38AM +0100, Jan Kara wrote:
> I'm not sure I understand the question. Do you mean why both s_active and
> s_umount rwsem exist? s_active is a reference count keeping superblock

Yes.

> alive - e.g. if the filesystem is mounted in more places, we need a
> reference for each mountpoint. s_umount is used when we want to block any

I could be mistaken but I *think* we used to reject umounts based on
s_active and s_umount is the mechanism to delay umounts rather than
failing them and probably with bind mounts the behavior changed.

> umount operation until we are done. For example sync(2) is using it to make
> sure superblock doesn't disappear and so that we don't keep superblock
> alive after admin called umount(2).

So, the question is why aren't we just using s_active and draining it
on umount of the last mountpoint.  Because, right now, the behavior is
weird in that we allow umounts to proceed but then let the superblock
hang onto the block device till s_active is drained.  This really
should be synchronous.

Thanks.

-- 
tejun

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

#1337350 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

Hi Tejun,

On Thu 18-02-16 08:00:33, Tejun Heo wrote:
> On Thu, Feb 18, 2016 at 10:55:38AM +0100, Jan Kara wrote:
> > I'm not sure I understand the question. Do you mean why both s_active and
> > s_umount rwsem exist? s_active is a reference count keeping superblock
> 
> Yes.
> 
> > alive - e.g. if the filesystem is mounted in more places, we need a
> > reference for each mountpoint. s_umount is used when we want to block any
> 
> I could be mistaken but I *think* we used to reject umounts based on
> s_active and s_umount is the mechanism to delay umounts rather than
> failing them and probably with bind mounts the behavior changed.
> 
> > umount operation until we are done. For example sync(2) is using it to make
> > sure superblock doesn't disappear and so that we don't keep superblock
> > alive after admin called umount(2).
> 
> So, the question is why aren't we just using s_active and draining it
> on umount of the last mountpoint.  Because, right now, the behavior is
> weird in that we allow umounts to proceed but then let the superblock
> hang onto the block device till s_active is drained.  This really
> should be synchronous.

Hum, I'm not sure. I guess Al can give you more qualified answer than me.
Added to CC...

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

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

#1338436 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On Thu, Feb 18, 2016 at 08:00:33AM -0500, Tejun Heo wrote:

> So, the question is why aren't we just using s_active and draining it
> on umount of the last mountpoint.  Because, right now, the behavior is
> weird in that we allow umounts to proceed but then let the superblock
> hang onto the block device till s_active is drained.  This really
> should be synchronous.

This really should not.  First of all, umount -l (or exit of the last
namespace user, for that matter) can leave you with actual fs shutdown
postponed until some opened files get closed.  Nothing synchronous about
that.

If you need details on s_active/s_umount/etc., I can give you a braindump,
but I suspect your real question is a lot more specific.  Details, please...

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

#1338454 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

Hello, Al.

On Fri, Feb 19, 2016 at 08:18:06PM +0000, Al Viro wrote:
> On Thu, Feb 18, 2016 at 08:00:33AM -0500, Tejun Heo wrote:
> > So, the question is why aren't we just using s_active and draining it
> > on umount of the last mountpoint.  Because, right now, the behavior is
> > weird in that we allow umounts to proceed but then let the superblock
> > hang onto the block device till s_active is drained.  This really
> > should be synchronous.
> 
> This really should not.  First of all, umount -l (or exit of the last
> namespace user, for that matter) can leave you with actual fs shutdown
> postponed until some opened files get closed.  Nothing synchronous about
> that.

I see, I suppose that's what distinguishes s_active and s_umount
usages - whether pinning should block umounting?

> If you need details on s_active/s_umount/etc., I can give you a braindump,
> but I suspect your real question is a lot more specific.  Details, please...

So, the problem is that cgroup writeback path sometimes schedules a
work item to change the cgroup an inode is associated.  Currently,
only the inode was pinned and the underlying sb may go away while the
work item is still pending.  The work item performs iput() at the end
and that explodes if the underlying sb is already gone.

As writeback path relies on s_umount for synchronization anyway, I
think that'd be the most natural way to hold onto the sb but
unfortunately there's no way to pass on the down_read to the async
execution context, so I made it grap s_active, which worked fine but
it made the sb hang around until such work items are finished.  It's
an unlikely race to hit but still broken.

The last option would be canceling / flushing these work items from sb
shutdown path which is likely more involved.

What should it be doing?

Thanks!

-- 
tejun

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

#1338510 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On Fri, Feb 19, 2016 at 03:51:47PM -0500, Tejun Heo wrote:

> I see, I suppose that's what distinguishes s_active and s_umount
> usages - whether pinning should block umounting?

???

->s_active is plain and simple count of "I hold a long-term reference to
this superblock, don't you shut it down until I drop that".

->s_umount is held across some of the transitions in struct super_block
life cycle, including the actual process of shutdown.

> > If you need details on s_active/s_umount/etc., I can give you a braindump,
> > but I suspect your real question is a lot more specific.  Details, please...
> 
> So, the problem is that cgroup writeback path sometimes schedules a
> work item to change the cgroup an inode is associated.  Currently,
> only the inode was pinned and the underlying sb may go away while the
> work item is still pending.  The work item performs iput() at the end
> and that explodes if the underlying sb is already gone.
> 
> As writeback path relies on s_umount for synchronization anyway, I
> think that'd be the most natural way to hold onto the sb but
> unfortunately there's no way to pass on the down_read to the async
> execution context, so I made it grap s_active, which worked fine but
> it made the sb hang around until such work items are finished.  It's
> an unlikely race to hit but still broken.
> 
> The last option would be canceling / flushing these work items from sb
> shutdown path which is likely more involved.
> 
> What should it be doing?

Um...  What ordering requirements do you have?  You obviously shouldn't
let it continue past the shutdown - as the matter of fact, you *can't* let
it continue past generic_shutdown_super(), since any inode references
held at evict_inodes() time will make it very unhappy.  Attempts to do
any IO after that will make things a lot worse than unhappy - data structures
needed to do it might be gone (and if you hold a bit longer, filesystem
driver itself might very well be gone, along with the functions you were
going to call).

Grabbing ->s_active is a seriously bad idea for another reason - in
a situation when there's only one mount of given fs, plain umount() should
_not_ return 0 before fs shutdown is over.  Sure, it is possible that there's
a binding somewhere, or that it's a lazy umount, etc., but those are "you've
asked for it" situations; having plain umount of e.g. ext3 on a USB stick
return success before it is safe to pull that stick out is a Bloody Bad Idea,
for obvious usability reasons.

IOW, while fs shutdown may be async, making it *always* async would be a bad
bug.  And bumping ->s_active does just that.

I'd go for trylock inside that work + making generic_shutdown_super()
kill all such works.  I assume that it *can* be abandoned in situation
when we know that sync_filesystem() is about to be called and that
said sync_filesystem() won't, in turn, schedule any such works, of course...

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

#1338523 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

Hello,

On Fri, Feb 19, 2016 at 09:58:11PM +0000, Al Viro wrote:
> Um...  What ordering requirements do you have?  You obviously shouldn't
> let it continue past the shutdown - as the matter of fact, you *can't* let
> it continue past generic_shutdown_super(), since any inode references
> held at evict_inodes() time will make it very unhappy.  Attempts to do
> any IO after that will make things a lot worse than unhappy - data structures
> needed to do it might be gone (and if you hold a bit longer, filesystem
> driver itself might very well be gone, along with the functions you were
> going to call).

It can be thought of as an extension of fs writeback operation and
it'd be ideal if it can hold off sb shutdown as on-going writeback
does through holding s_umount.  Unfortunately, that doesn't seem
possible because there's no way to transfer rwsem ownership.

It doesn't generate any IO.  The reason it's done asynchronously is
because the operation requires an RCU grace period.  After the grace
period, it accesses only the generic inode and address_space and the
only time it ends up accessing sb is through the iput call.
Everything else AFAICS doesn't really care whether the underlying sb
is shut down or not.

> Grabbing ->s_active is a seriously bad idea for another reason - in
> a situation when there's only one mount of given fs, plain umount() should
> _not_ return 0 before fs shutdown is over.  Sure, it is possible that there's
> a binding somewhere, or that it's a lazy umount, etc., but those are "you've
> asked for it" situations; having plain umount of e.g. ext3 on a USB stick
> return success before it is safe to pull that stick out is a Bloody Bad Idea,
> for obvious usability reasons.

I see.

> IOW, while fs shutdown may be async, making it *always* async would be a bad
> bug.  And bumping ->s_active does just that.
> 
> I'd go for trylock inside that work + making generic_shutdown_super()
> kill all such works.  I assume that it *can* be abandoned in situation
> when we know that sync_filesystem() is about to be called and that
> said sync_filesystem() won't, in turn, schedule any such works, of course...

I'll make generic_shutdown_super() to kill all such work items.  I
don't think the work item itself would need further locking tho.  Can
you please elaborate why you thought adding trylock to the work would
be necessary?

Thanks.

-- 
tejun

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

#1338535 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On Fri, Feb 19, 2016 at 05:15:12PM -0500, Tejun Heo wrote:

> > IOW, while fs shutdown may be async, making it *always* async would be a bad
> > bug.  And bumping ->s_active does just that.
> > 
> > I'd go for trylock inside that work + making generic_shutdown_super()
> > kill all such works.  I assume that it *can* be abandoned in situation
> > when we know that sync_filesystem() is about to be called and that
> > said sync_filesystem() won't, in turn, schedule any such works, of course...
> 
> I'll make generic_shutdown_super() to kill all such work items.  I
> don't think the work item itself would need further locking tho.  Can
> you please elaborate why you thought adding trylock to the work would
> be necessary?

Umm...  Not much, except that it would make the life cycle rules a bit
more regular.

Is that code OK with e.g. running in parallel with remounting filesystem r/o?

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

#1337224 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On 02/16/2016 08:24 PM, Tejun Heo wrote:
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-and-tested-by: Tahsin Erdogan <tahsin@google.com>
> Link: http://lkml.kernel.org/g/CAAeU0aNCq7LGODvVGRU-oU_o-6enii5ey0p1c26D1ZzYwkDc5A@mail.gmail.com
> Fixes: d10c80955265 ("writeback: implement foreign cgroup inode bdi_writeback switching")
> Cc: stable@vger.kernel.org #v4.5+


Why is this tagged for 4.5+ since the commit in question was merged in 4.2?

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

#1337331 — Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

On Thu, Feb 18, 2016 at 12:12:40PM +0200, Nikolay Borisov wrote:
> 
> 
> On 02/16/2016 08:24 PM, Tejun Heo wrote:
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Reported-and-tested-by: Tahsin Erdogan <tahsin@google.com>
> > Link: http://lkml.kernel.org/g/CAAeU0aNCq7LGODvVGRU-oU_o-6enii5ey0p1c26D1ZzYwkDc5A@mail.gmail.com
> > Fixes: d10c80955265 ("writeback: implement foreign cgroup inode bdi_writeback switching")
> > Cc: stable@vger.kernel.org #v4.5+
> 
> Why is this tagged for 4.5+ since the commit in question was merged in 4.2?

Up until 4.5, the whole cgroup v2 interface is hidden behind a devel
flag and cgroup writeback can only be used through cgroup v2.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web