Re: [PATCH v2] f2fs: Fix deadlock in shutdown ioctl
On Thu, May 17, 2018 at 06:08:26PM -0700, Jaegeuk Kim wrote: > > if (in == F2FS_GOING_DOWN_FULLSYNC) { > sb = freeze_bdev(); > if (IS_ERR(sb)) > return PTR_ERR(sb); > if (unlikely(!sb)) > return -EINVAL; > } > > ret = mnt_want_write_file(); It will be stuck/blocked here as freeze_bdev() now holds the write lock for all the cases including SB_FREEZE_WRITE. As freeze_bdev() holds the write lock, I think f2fs_stop_checkpoint() should be safe even without mnt_want_write_file(). I have posted v3 as per Chao's comments to exclude mnt_want_write_file() for F2FS_GOING_DOWN_FULLSYNC case. Please check and let me know if there are any further comments. > ... > switch() { > case F2FS_GOING_DOWN_FULLSYNC: > f2fs_stop_checkpoint(); > dhaw_bdev(); > break; > ... > } > > drop: > ... > > -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH v2] f2fs: Fix deadlock in shutdown ioctl
On Thu, May 17, 2018 at 06:08:26PM -0700, Jaegeuk Kim wrote: > > if (in == F2FS_GOING_DOWN_FULLSYNC) { > sb = freeze_bdev(); > if (IS_ERR(sb)) > return PTR_ERR(sb); > if (unlikely(!sb)) > return -EINVAL; > } > > ret = mnt_want_write_file(); It will be stuck/blocked here as freeze_bdev() now holds the write lock for all the cases including SB_FREEZE_WRITE. As freeze_bdev() holds the write lock, I think f2fs_stop_checkpoint() should be safe even without mnt_want_write_file(). I have posted v3 as per Chao's comments to exclude mnt_want_write_file() for F2FS_GOING_DOWN_FULLSYNC case. Please check and let me know if there are any further comments. > ... > switch() { > case F2FS_GOING_DOWN_FULLSYNC: > f2fs_stop_checkpoint(); > dhaw_bdev(); > break; > ... > } > > drop: > ... > > -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH v2] f2fs: Fix deadlock in shutdown ioctl
On 05/17, Chao Yu wrote: > On 2018/5/17 16:03, Sahitya Tummala wrote: > > f2fs_ioc_shutdown() ioctl gets stuck in the below path > > when issued with F2FS_GOING_DOWN_FULLSYNC option. > > > > __switch_to+0x90/0xc4 > > percpu_down_write+0x8c/0xc0 > > freeze_super+0xec/0x1e4 > > freeze_bdev+0xc4/0xcc > > f2fs_ioctl+0xc0c/0x1ce0 > > f2fs_compat_ioctl+0x98/0x1f0 > > > > Signed-off-by: Sahitya Tummala> > --- > > v2: > > remove lock coverage for only F2FS_GOING_DOWN_FULLSYNC case as suggested by > > Chao. > > > > fs/f2fs/file.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > index 6b94f19..5a132c9 100644 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@ -1857,6 +1857,7 @@ static int f2fs_ioc_shutdown(struct file *filp, > > unsigned long arg) > > How about: > > if (in != F2FS_GOING_DOWN_FULLSYNC) > mnt_want_write_file(); > > switch() > { > handle command; > } > > if (in != F2FS_GOING_DOWN_FULLSYNC) > mnt_drop_write_file(); > > Thanks, if (in == F2FS_GOING_DOWN_FULLSYNC) { sb = freeze_bdev(); if (IS_ERR(sb)) return PTR_ERR(sb); if (unlikely(!sb)) return -EINVAL; } ret = mnt_want_write_file(); ... switch() { case F2FS_GOING_DOWN_FULLSYNC: f2fs_stop_checkpoint(); dhaw_bdev(); break; ... } drop: ... > > > > > switch (in) { > > case F2FS_GOING_DOWN_FULLSYNC: > > + mnt_drop_write_file(filp); > > sb = freeze_bdev(sb->s_bdev); > > if (IS_ERR(sb)) { > > ret = PTR_ERR(sb); > > @@ -1894,7 +1895,8 @@ static int f2fs_ioc_shutdown(struct file *filp, > > unsigned long arg) > > > > f2fs_update_time(sbi, REQ_TIME); > > out: > > - mnt_drop_write_file(filp); > > + if (in != F2FS_GOING_DOWN_FULLSYNC) > > + mnt_drop_write_file(filp); > > return ret; > > } > > > >
Re: [PATCH v2] f2fs: Fix deadlock in shutdown ioctl
On 05/17, Chao Yu wrote: > On 2018/5/17 16:03, Sahitya Tummala wrote: > > f2fs_ioc_shutdown() ioctl gets stuck in the below path > > when issued with F2FS_GOING_DOWN_FULLSYNC option. > > > > __switch_to+0x90/0xc4 > > percpu_down_write+0x8c/0xc0 > > freeze_super+0xec/0x1e4 > > freeze_bdev+0xc4/0xcc > > f2fs_ioctl+0xc0c/0x1ce0 > > f2fs_compat_ioctl+0x98/0x1f0 > > > > Signed-off-by: Sahitya Tummala > > --- > > v2: > > remove lock coverage for only F2FS_GOING_DOWN_FULLSYNC case as suggested by > > Chao. > > > > fs/f2fs/file.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > index 6b94f19..5a132c9 100644 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@ -1857,6 +1857,7 @@ static int f2fs_ioc_shutdown(struct file *filp, > > unsigned long arg) > > How about: > > if (in != F2FS_GOING_DOWN_FULLSYNC) > mnt_want_write_file(); > > switch() > { > handle command; > } > > if (in != F2FS_GOING_DOWN_FULLSYNC) > mnt_drop_write_file(); > > Thanks, if (in == F2FS_GOING_DOWN_FULLSYNC) { sb = freeze_bdev(); if (IS_ERR(sb)) return PTR_ERR(sb); if (unlikely(!sb)) return -EINVAL; } ret = mnt_want_write_file(); ... switch() { case F2FS_GOING_DOWN_FULLSYNC: f2fs_stop_checkpoint(); dhaw_bdev(); break; ... } drop: ... > > > > > switch (in) { > > case F2FS_GOING_DOWN_FULLSYNC: > > + mnt_drop_write_file(filp); > > sb = freeze_bdev(sb->s_bdev); > > if (IS_ERR(sb)) { > > ret = PTR_ERR(sb); > > @@ -1894,7 +1895,8 @@ static int f2fs_ioc_shutdown(struct file *filp, > > unsigned long arg) > > > > f2fs_update_time(sbi, REQ_TIME); > > out: > > - mnt_drop_write_file(filp); > > + if (in != F2FS_GOING_DOWN_FULLSYNC) > > + mnt_drop_write_file(filp); > > return ret; > > } > > > >
Re: [PATCH v2] f2fs: Fix deadlock in shutdown ioctl
On 2018/5/17 16:03, Sahitya Tummala wrote: > f2fs_ioc_shutdown() ioctl gets stuck in the below path > when issued with F2FS_GOING_DOWN_FULLSYNC option. > > __switch_to+0x90/0xc4 > percpu_down_write+0x8c/0xc0 > freeze_super+0xec/0x1e4 > freeze_bdev+0xc4/0xcc > f2fs_ioctl+0xc0c/0x1ce0 > f2fs_compat_ioctl+0x98/0x1f0 > > Signed-off-by: Sahitya Tummala> --- > v2: > remove lock coverage for only F2FS_GOING_DOWN_FULLSYNC case as suggested by > Chao. > > fs/f2fs/file.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 6b94f19..5a132c9 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -1857,6 +1857,7 @@ static int f2fs_ioc_shutdown(struct file *filp, > unsigned long arg) How about: if (in != F2FS_GOING_DOWN_FULLSYNC) mnt_want_write_file(); switch() { handle command; } if (in != F2FS_GOING_DOWN_FULLSYNC) mnt_drop_write_file(); Thanks, > > switch (in) { > case F2FS_GOING_DOWN_FULLSYNC: > + mnt_drop_write_file(filp); > sb = freeze_bdev(sb->s_bdev); > if (IS_ERR(sb)) { > ret = PTR_ERR(sb); > @@ -1894,7 +1895,8 @@ static int f2fs_ioc_shutdown(struct file *filp, > unsigned long arg) > > f2fs_update_time(sbi, REQ_TIME); > out: > - mnt_drop_write_file(filp); > + if (in != F2FS_GOING_DOWN_FULLSYNC) > + mnt_drop_write_file(filp); > return ret; > } > >
Re: [PATCH v2] f2fs: Fix deadlock in shutdown ioctl
On 2018/5/17 16:03, Sahitya Tummala wrote: > f2fs_ioc_shutdown() ioctl gets stuck in the below path > when issued with F2FS_GOING_DOWN_FULLSYNC option. > > __switch_to+0x90/0xc4 > percpu_down_write+0x8c/0xc0 > freeze_super+0xec/0x1e4 > freeze_bdev+0xc4/0xcc > f2fs_ioctl+0xc0c/0x1ce0 > f2fs_compat_ioctl+0x98/0x1f0 > > Signed-off-by: Sahitya Tummala > --- > v2: > remove lock coverage for only F2FS_GOING_DOWN_FULLSYNC case as suggested by > Chao. > > fs/f2fs/file.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 6b94f19..5a132c9 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -1857,6 +1857,7 @@ static int f2fs_ioc_shutdown(struct file *filp, > unsigned long arg) How about: if (in != F2FS_GOING_DOWN_FULLSYNC) mnt_want_write_file(); switch() { handle command; } if (in != F2FS_GOING_DOWN_FULLSYNC) mnt_drop_write_file(); Thanks, > > switch (in) { > case F2FS_GOING_DOWN_FULLSYNC: > + mnt_drop_write_file(filp); > sb = freeze_bdev(sb->s_bdev); > if (IS_ERR(sb)) { > ret = PTR_ERR(sb); > @@ -1894,7 +1895,8 @@ static int f2fs_ioc_shutdown(struct file *filp, > unsigned long arg) > > f2fs_update_time(sbi, REQ_TIME); > out: > - mnt_drop_write_file(filp); > + if (in != F2FS_GOING_DOWN_FULLSYNC) > + mnt_drop_write_file(filp); > return ret; > } > >