* [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
@ 2017-06-24 16:25 Jaegeuk Kim
2017-06-24 16:25 ` [PATCH 2/2] f2fs: report # of free inodes more precisely Jaegeuk Kim
2017-06-26 8:15 ` [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op Chao Yu
0 siblings, 2 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2017-06-24 16:25 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/data.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7d3af48d34a9..9141bd19a902 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
}
}
- if (fio->need_lock == LOCK_REQ)
- f2fs_lock_op(fio->sbi);
+ /* Deadlock due to between page->lock and f2fs_lock_op */
+ if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+ return -EAGAIN;
err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
if (err)
--
2.13.0.rc1.294.g07d810a77f-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] f2fs: report # of free inodes more precisely
2017-06-24 16:25 [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op Jaegeuk Kim
@ 2017-06-24 16:25 ` Jaegeuk Kim
2017-06-26 10:52 ` [f2fs-dev] " Chao Yu
2017-06-26 8:15 ` [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op Chao Yu
1 sibling, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2017-06-24 16:25 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
If the partition is small, we don't need to report total # of inodes including
hidden free nodes.
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/super.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8e39b850bfc0..3da6fb276f8b 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
struct f2fs_sb_info *sbi = F2FS_SB(sb);
u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
block_t total_count, user_block_count, start_count, ovp_count;
+ u64 avail_node_count;
total_count = le64_to_cpu(sbi->raw_super->block_count);
user_block_count = sbi->user_block_count;
@@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
buf->f_bavail = user_block_count - valid_user_blocks(sbi);
- buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
- buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
- buf->f_bavail);
+ avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
+
+ if (avail_node_count > user_block_count) {
+ buf->f_files = user_block_count;
+ buf->f_ffree = buf->f_bavail;
+ } else {
+ buf->f_files = avail_node_count;
+ buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
+ buf->f_bavail);
+ }
buf->f_namelen = F2FS_NAME_LEN;
buf->f_fsid.val[0] = (u32)id;
--
2.13.0.rc1.294.g07d810a77f-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-06-24 16:25 [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op Jaegeuk Kim
2017-06-24 16:25 ` [PATCH 2/2] f2fs: report # of free inodes more precisely Jaegeuk Kim
@ 2017-06-26 8:15 ` Chao Yu
2017-06-26 14:54 ` Jaegeuk Kim
1 sibling, 1 reply; 16+ messages in thread
From: Chao Yu @ 2017-06-26 8:15 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-fsdevel, linux-f2fs-devel
Hi Jaegeuk,
On 2017/6/25 0:25, Jaegeuk Kim wrote:
> - punch_hole
> - fill_zero
> - f2fs_lock_op
> - get_new_data_page
> - lock_page
>
> - f2fs_write_data_pages
> - lock_page
> - do_write_data_page
> - f2fs_lock_op
Good catch!
With this implementation, page writeback can fail due to concurrent checkpoint,
this will make fsync/atomic_commit which trigger synchronous write failed randomly.
How about unifying the lock order in punch_hole as one in writepages for regular
inode? We can add one more parameter in get_new_data_page to indicate whether
callee needs to lock cp_rwsem.
Thanks,
>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> fs/f2fs/data.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 7d3af48d34a9..9141bd19a902 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
> }
> }
>
> - if (fio->need_lock == LOCK_REQ)
> - f2fs_lock_op(fio->sbi);
> + /* Deadlock due to between page->lock and f2fs_lock_op */
> + if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
> + return -EAGAIN;
>
> err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
> if (err)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely
2017-06-24 16:25 ` [PATCH 2/2] f2fs: report # of free inodes more precisely Jaegeuk Kim
@ 2017-06-26 10:52 ` Chao Yu
2017-06-26 14:58 ` Jaegeuk Kim
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2017-06-26 10:52 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-fsdevel, linux-f2fs-devel
Hi Jaegeuk,
On 2017/6/25 0:25, Jaegeuk Kim wrote:
> If the partition is small, we don't need to report total # of inodes including
> hidden free nodes.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> fs/f2fs/super.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 8e39b850bfc0..3da6fb276f8b 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
> struct f2fs_sb_info *sbi = F2FS_SB(sb);
> u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
> block_t total_count, user_block_count, start_count, ovp_count;
> + u64 avail_node_count;
>
> total_count = le64_to_cpu(sbi->raw_super->block_count);
> user_block_count = sbi->user_block_count;
> @@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
> buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
> buf->f_bavail = user_block_count - valid_user_blocks(sbi);
>
> - buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
> - buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
> - buf->f_bavail);
> + avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
> +
> + if (avail_node_count > user_block_count) {
> + buf->f_files = user_block_count;
> + buf->f_ffree = buf->f_bavail;
f_ffree is limited both by remained free nid count and free block count, so it
needs to change like this?
if (avail_node_count > user_block_count)
avail_node_count = user_block_count;
buf->f_files = avail_node_count;
buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
buf->f_bavail);
Thanks,
> + } else {
> + buf->f_files = avail_node_count;
> + buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
> + buf->f_bavail);
> + }
>
> buf->f_namelen = F2FS_NAME_LEN;
> buf->f_fsid.val[0] = (u32)id;
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-06-26 8:15 ` [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op Chao Yu
@ 2017-06-26 14:54 ` Jaegeuk Kim
2017-06-26 15:42 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2017-06-26 14:54 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel
Hi Chao,
On 06/26, Chao Yu wrote:
> Hi Jaegeuk,
>
> On 2017/6/25 0:25, Jaegeuk Kim wrote:
> > - punch_hole
> > - fill_zero
> > - f2fs_lock_op
> > - get_new_data_page
> > - lock_page
> >
> > - f2fs_write_data_pages
> > - lock_page
> > - do_write_data_page
> > - f2fs_lock_op
>
> Good catch!
>
> With this implementation, page writeback can fail due to concurrent checkpoint,
> this will make fsync/atomic_commit which trigger synchronous write failed randomly.
>
> How about unifying the lock order in punch_hole as one in writepages for regular
> inode? We can add one more parameter in get_new_data_page to indicate whether
> callee needs to lock cp_rwsem.
Currently, there would be some places to keep cp_rwsem -> page.lock, which seems
not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry
flushing data in f2fs_sync_file, once it gets -EAGAIN.
Any thoughts?
>
> Thanks,
>
> >
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > fs/f2fs/data.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 7d3af48d34a9..9141bd19a902 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
> > }
> > }
> >
> > - if (fio->need_lock == LOCK_REQ)
> > - f2fs_lock_op(fio->sbi);
> > + /* Deadlock due to between page->lock and f2fs_lock_op */
> > + if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
> > + return -EAGAIN;
> >
> > err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
> > if (err)
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely
2017-06-26 10:52 ` [f2fs-dev] " Chao Yu
@ 2017-06-26 14:58 ` Jaegeuk Kim
2017-06-28 13:01 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2017-06-26 14:58 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel
On 06/26, Chao Yu wrote:
> Hi Jaegeuk,
>
> On 2017/6/25 0:25, Jaegeuk Kim wrote:
> > If the partition is small, we don't need to report total # of inodes including
> > hidden free nodes.
> >
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > fs/f2fs/super.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 8e39b850bfc0..3da6fb276f8b 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
> > struct f2fs_sb_info *sbi = F2FS_SB(sb);
> > u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
> > block_t total_count, user_block_count, start_count, ovp_count;
> > + u64 avail_node_count;
> >
> > total_count = le64_to_cpu(sbi->raw_super->block_count);
> > user_block_count = sbi->user_block_count;
> > @@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
> > buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
> > buf->f_bavail = user_block_count - valid_user_blocks(sbi);
> >
> > - buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
> > - buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
> > - buf->f_bavail);
> > + avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
> > +
> > + if (avail_node_count > user_block_count) {
> > + buf->f_files = user_block_count;
> > + buf->f_ffree = buf->f_bavail;
>
> f_ffree is limited both by remained free nid count and free block count, so it
> needs to change like this?
I thought both of them are same, since node block will consume user block. So,
we don't need to do min() again.
>
> if (avail_node_count > user_block_count)
> avail_node_count = user_block_count;
>
> buf->f_files = avail_node_count;
> buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
> buf->f_bavail);
>
> Thanks,
>
> > + } else {
> > + buf->f_files = avail_node_count;
> > + buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
> > + buf->f_bavail);
> > + }
> >
> > buf->f_namelen = F2FS_NAME_LEN;
> > buf->f_fsid.val[0] = (u32)id;
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-06-26 14:54 ` Jaegeuk Kim
@ 2017-06-26 15:42 ` Chao Yu
2017-07-01 7:28 ` Jaegeuk Kim
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2017-06-26 15:42 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel
Hi Jaegeuk,
On 2017/6/26 22:54, Jaegeuk Kim wrote:
> Hi Chao,
>
> On 06/26, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2017/6/25 0:25, Jaegeuk Kim wrote:
>>> - punch_hole
>>> - fill_zero
>>> - f2fs_lock_op
>>> - get_new_data_page
>>> - lock_page
>>>
>>> - f2fs_write_data_pages
>>> - lock_page
>>> - do_write_data_page
>>> - f2fs_lock_op
>>
>> Good catch!
>>
>> With this implementation, page writeback can fail due to concurrent checkpoint,
>> this will make fsync/atomic_commit which trigger synchronous write failed randomly.
>>
>> How about unifying the lock order in punch_hole as one in writepages for regular
>> inode? We can add one more parameter in get_new_data_page to indicate whether
>> callee needs to lock cp_rwsem.
>
> Currently, there would be some places to keep cp_rwsem -> page.lock, which seems
> not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry
> flushing data in f2fs_sync_file, once it gets -EAGAIN.
>
> Any thoughts?
What about adding inode_lock in f2fs_sync_file to exclude other
foreground operation which have reversed lock order? Atomic_commit is OK
since it has inode_lock in its path.
Thanks,
>
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>> fs/f2fs/data.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 7d3af48d34a9..9141bd19a902 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
>>> }
>>> }
>>>
>>> - if (fio->need_lock == LOCK_REQ)
>>> - f2fs_lock_op(fio->sbi);
>>> + /* Deadlock due to between page->lock and f2fs_lock_op */
>>> + if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
>>> + return -EAGAIN;
>>>
>>> err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
>>> if (err)
>>>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely
2017-06-26 14:58 ` Jaegeuk Kim
@ 2017-06-28 13:01 ` Chao Yu
2017-07-01 7:27 ` Jaegeuk Kim
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2017-06-28 13:01 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel
On 2017/6/26 22:58, Jaegeuk Kim wrote:
> On 06/26, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2017/6/25 0:25, Jaegeuk Kim wrote:
>>> If the partition is small, we don't need to report total # of inodes including
>>> hidden free nodes.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>> fs/f2fs/super.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index 8e39b850bfc0..3da6fb276f8b 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>> struct f2fs_sb_info *sbi = F2FS_SB(sb);
>>> u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
>>> block_t total_count, user_block_count, start_count, ovp_count;
>>> + u64 avail_node_count;
>>>
>>> total_count = le64_to_cpu(sbi->raw_super->block_count);
>>> user_block_count = sbi->user_block_count;
>>> @@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>> buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
>>> buf->f_bavail = user_block_count - valid_user_blocks(sbi);
>>>
>>> - buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
>>> - buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
>>> - buf->f_bavail);
>>> + avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
>>> +
>>> + if (avail_node_count > user_block_count) {
>>> + buf->f_files = user_block_count;
>>> + buf->f_ffree = buf->f_bavail;
>>
>> f_ffree is limited both by remained free nid count and free block count, so it
>> needs to change like this?
>
> I thought both of them are same, since node block will consume user block. So,
> we don't need to do min() again.
avail_node_count comes from total free nid counts which is limited with
nid_bitmap size, buf->f_bavail comes from total user block count which
can both cosumed by node and data. So the value of them may not be the same.
Thanks,
>
>>
>> if (avail_node_count > user_block_count)
>> avail_node_count = user_block_count;
>>
>> buf->f_files = avail_node_count;
>> buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
>> buf->f_bavail);
>>
>> Thanks,
>>
>>> + } else {
>>> + buf->f_files = avail_node_count;
>>> + buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
>>> + buf->f_bavail);
>>> + }
>>>
>>> buf->f_namelen = F2FS_NAME_LEN;
>>> buf->f_fsid.val[0] = (u32)id;
>>>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely
2017-06-28 13:01 ` Chao Yu
@ 2017-07-01 7:27 ` Jaegeuk Kim
2017-07-01 8:28 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2017-07-01 7:27 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-fsdevel, linux-kernel, linux-f2fs-devel
On 06/28, Chao Yu wrote:
> On 2017/6/26 22:58, Jaegeuk Kim wrote:
> > On 06/26, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2017/6/25 0:25, Jaegeuk Kim wrote:
> >>> If the partition is small, we don't need to report total # of inodes including
> >>> hidden free nodes.
> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>> ---
> >>> fs/f2fs/super.c | 14 +++++++++++---
> >>> 1 file changed, 11 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> >>> index 8e39b850bfc0..3da6fb276f8b 100644
> >>> --- a/fs/f2fs/super.c
> >>> +++ b/fs/f2fs/super.c
> >>> @@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
> >>> struct f2fs_sb_info *sbi = F2FS_SB(sb);
> >>> u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
> >>> block_t total_count, user_block_count, start_count, ovp_count;
> >>> + u64 avail_node_count;
> >>>
> >>> total_count = le64_to_cpu(sbi->raw_super->block_count);
> >>> user_block_count = sbi->user_block_count;
> >>> @@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
> >>> buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
> >>> buf->f_bavail = user_block_count - valid_user_blocks(sbi);
> >>>
> >>> - buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
> >>> - buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
> >>> - buf->f_bavail);
> >>> + avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
> >>> +
> >>> + if (avail_node_count > user_block_count) {
> >>> + buf->f_files = user_block_count;
> >>> + buf->f_ffree = buf->f_bavail;
> >>
> >> f_ffree is limited both by remained free nid count and free block count, so it
> >> needs to change like this?
> >
> > I thought both of them are same, since node block will consume user block. So,
> > we don't need to do min() again.
>
> avail_node_count comes from total free nid counts which is limited with
> nid_bitmap size, buf->f_bavail comes from total user block count which
> can both cosumed by node and data. So the value of them may not be the same.
What I mean was, if avail_node_count is larger than user_block_count, we can
see buf->f_bavail is always smaller than avali_node_count - valid_node_count,
since node blocks concumes blocks as well.
>
> Thanks,
>
> >
> >>
> >> if (avail_node_count > user_block_count)
> >> avail_node_count = user_block_count;
> >>
> >> buf->f_files = avail_node_count;
> >> buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
> >> buf->f_bavail);
> >>
> >> Thanks,
> >>
> >>> + } else {
> >>> + buf->f_files = avail_node_count;
> >>> + buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
> >>> + buf->f_bavail);
> >>> + }
> >>>
> >>> buf->f_namelen = F2FS_NAME_LEN;
> >>> buf->f_fsid.val[0] = (u32)id;
> >>>
> >
> > ------------------------------------------------------------------------------
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-06-26 15:42 ` Chao Yu
@ 2017-07-01 7:28 ` Jaegeuk Kim
2017-07-01 8:41 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2017-07-01 7:28 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-fsdevel, linux-kernel, linux-f2fs-devel
On 06/26, Chao Yu wrote:
> Hi Jaegeuk,
>
> On 2017/6/26 22:54, Jaegeuk Kim wrote:
> > Hi Chao,
> >
> > On 06/26, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2017/6/25 0:25, Jaegeuk Kim wrote:
> >>> - punch_hole
> >>> - fill_zero
> >>> - f2fs_lock_op
> >>> - get_new_data_page
> >>> - lock_page
> >>>
> >>> - f2fs_write_data_pages
> >>> - lock_page
> >>> - do_write_data_page
> >>> - f2fs_lock_op
> >>
> >> Good catch!
> >>
> >> With this implementation, page writeback can fail due to concurrent checkpoint,
> >> this will make fsync/atomic_commit which trigger synchronous write failed randomly.
> >>
> >> How about unifying the lock order in punch_hole as one in writepages for regular
> >> inode? We can add one more parameter in get_new_data_page to indicate whether
> >> callee needs to lock cp_rwsem.
> >
> > Currently, there would be some places to keep cp_rwsem -> page.lock, which seems
> > not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry
> > flushing data in f2fs_sync_file, once it gets -EAGAIN.
> >
> > Any thoughts?
>
> What about adding inode_lock in f2fs_sync_file to exclude other
> foreground operation which have reversed lock order? Atomic_commit is OK
> since it has inode_lock in its path.
I have concerned about performance regression, if we do that.
>
> Thanks,
>
> >
> >>
> >> Thanks,
> >>
> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>> ---
> >>> fs/f2fs/data.c | 5 +++--
> >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 7d3af48d34a9..9141bd19a902 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
> >>> }
> >>> }
> >>>
> >>> - if (fio->need_lock == LOCK_REQ)
> >>> - f2fs_lock_op(fio->sbi);
> >>> + /* Deadlock due to between page->lock and f2fs_lock_op */
> >>> + if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
> >>> + return -EAGAIN;
> >>>
> >>> err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
> >>> if (err)
> >>>
> >
> > ------------------------------------------------------------------------------
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely
2017-07-01 7:27 ` Jaegeuk Kim
@ 2017-07-01 8:28 ` Chao Yu
0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2017-07-01 8:28 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-fsdevel, linux-kernel, linux-f2fs-devel
On 2017/7/1 15:27, Jaegeuk Kim wrote:
> On 06/28, Chao Yu wrote:
>> On 2017/6/26 22:58, Jaegeuk Kim wrote:
>>> On 06/26, Chao Yu wrote:
>>>> Hi Jaegeuk,
>>>>
>>>> On 2017/6/25 0:25, Jaegeuk Kim wrote:
>>>>> If the partition is small, we don't need to report total # of inodes including
>>>>> hidden free nodes.
>>>>>
>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>> ---
>>>>> fs/f2fs/super.c | 14 +++++++++++---
>>>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>> index 8e39b850bfc0..3da6fb276f8b 100644
>>>>> --- a/fs/f2fs/super.c
>>>>> +++ b/fs/f2fs/super.c
>>>>> @@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>>>> struct f2fs_sb_info *sbi = F2FS_SB(sb);
>>>>> u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
>>>>> block_t total_count, user_block_count, start_count, ovp_count;
>>>>> + u64 avail_node_count;
>>>>>
>>>>> total_count = le64_to_cpu(sbi->raw_super->block_count);
>>>>> user_block_count = sbi->user_block_count;
>>>>> @@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>>>> buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
>>>>> buf->f_bavail = user_block_count - valid_user_blocks(sbi);
>>>>>
>>>>> - buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
>>>>> - buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
>>>>> - buf->f_bavail);
>>>>> + avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
>>>>> +
>>>>> + if (avail_node_count > user_block_count) {
>>>>> + buf->f_files = user_block_count;
>>>>> + buf->f_ffree = buf->f_bavail;
>>>>
>>>> f_ffree is limited both by remained free nid count and free block count, so it
>>>> needs to change like this?
>>>
>>> I thought both of them are same, since node block will consume user block. So,
>>> we don't need to do min() again.
>>
>> avail_node_count comes from total free nid counts which is limited with
>> nid_bitmap size, buf->f_bavail comes from total user block count which
>> can both cosumed by node and data. So the value of them may not be the same.
>
> What I mean was, if avail_node_count is larger than user_block_count, we can
> see buf->f_bavail is always smaller than avali_node_count - valid_node_count,
> since node blocks concumes blocks as well.
Got you. :)
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Thanks,
>
>>
>> Thanks,
>>
>>>
>>>>
>>>> if (avail_node_count > user_block_count)
>>>> avail_node_count = user_block_count;
>>>>
>>>> buf->f_files = avail_node_count;
>>>> buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
>>>> buf->f_bavail);
>>>>
>>>> Thanks,
>>>>
>>>>> + } else {
>>>>> + buf->f_files = avail_node_count;
>>>>> + buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
>>>>> + buf->f_bavail);
>>>>> + }
>>>>>
>>>>> buf->f_namelen = F2FS_NAME_LEN;
>>>>> buf->f_fsid.val[0] = (u32)id;
>>>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Check out the vibrant tech community on one of the world's most
>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-07-01 7:28 ` Jaegeuk Kim
@ 2017-07-01 8:41 ` Chao Yu
2017-07-01 14:27 ` Jaegeuk Kim
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2017-07-01 8:41 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-fsdevel, linux-kernel, linux-f2fs-devel
On 2017/7/1 15:28, Jaegeuk Kim wrote:
> On 06/26, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2017/6/26 22:54, Jaegeuk Kim wrote:
>>> Hi Chao,
>>>
>>> On 06/26, Chao Yu wrote:
>>>> Hi Jaegeuk,
>>>>
>>>> On 2017/6/25 0:25, Jaegeuk Kim wrote:
>>>>> - punch_hole
>>>>> - fill_zero
>>>>> - f2fs_lock_op
>>>>> - get_new_data_page
>>>>> - lock_page
>>>>>
>>>>> - f2fs_write_data_pages
>>>>> - lock_page
>>>>> - do_write_data_page
>>>>> - f2fs_lock_op
>>>>
>>>> Good catch!
>>>>
>>>> With this implementation, page writeback can fail due to concurrent checkpoint,
>>>> this will make fsync/atomic_commit which trigger synchronous write failed randomly.
>>>>
>>>> How about unifying the lock order in punch_hole as one in writepages for regular
>>>> inode? We can add one more parameter in get_new_data_page to indicate whether
>>>> callee needs to lock cp_rwsem.
>>>
>>> Currently, there would be some places to keep cp_rwsem -> page.lock, which seems
>>> not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry
>>> flushing data in f2fs_sync_file, once it gets -EAGAIN.
>>>
>>> Any thoughts?
>>
>> What about adding inode_lock in f2fs_sync_file to exclude other
>> foreground operation which have reversed lock order? Atomic_commit is OK
>> since it has inode_lock in its path.
>
> I have concerned about performance regression, if we do that.
I think fsync vs write or fsync vs fsync scenarios are unusual, so is
there any usecase?
Thanks,
>
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>> ---
>>>>> fs/f2fs/data.c | 5 +++--
>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>> index 7d3af48d34a9..9141bd19a902 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
>>>>> }
>>>>> }
>>>>>
>>>>> - if (fio->need_lock == LOCK_REQ)
>>>>> - f2fs_lock_op(fio->sbi);
>>>>> + /* Deadlock due to between page->lock and f2fs_lock_op */
>>>>> + if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
>>>>> + return -EAGAIN;
>>>>>
>>>>> err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
>>>>> if (err)
>>>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Check out the vibrant tech community on one of the world's most
>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-07-01 8:41 ` Chao Yu
@ 2017-07-01 14:27 ` Jaegeuk Kim
2017-07-05 2:58 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2017-07-01 14:27 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-fsdevel, linux-kernel, linux-f2fs-devel
On 07/01, Chao Yu wrote:
> On 2017/7/1 15:28, Jaegeuk Kim wrote:
> > On 06/26, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2017/6/26 22:54, Jaegeuk Kim wrote:
> >>> Hi Chao,
> >>>
> >>> On 06/26, Chao Yu wrote:
> >>>> Hi Jaegeuk,
> >>>>
> >>>> On 2017/6/25 0:25, Jaegeuk Kim wrote:
> >>>>> - punch_hole
> >>>>> - fill_zero
> >>>>> - f2fs_lock_op
> >>>>> - get_new_data_page
> >>>>> - lock_page
> >>>>>
> >>>>> - f2fs_write_data_pages
> >>>>> - lock_page
> >>>>> - do_write_data_page
> >>>>> - f2fs_lock_op
> >>>>
> >>>> Good catch!
> >>>>
> >>>> With this implementation, page writeback can fail due to concurrent checkpoint,
> >>>> this will make fsync/atomic_commit which trigger synchronous write failed randomly.
> >>>>
> >>>> How about unifying the lock order in punch_hole as one in writepages for regular
> >>>> inode? We can add one more parameter in get_new_data_page to indicate whether
> >>>> callee needs to lock cp_rwsem.
> >>>
> >>> Currently, there would be some places to keep cp_rwsem -> page.lock, which seems
> >>> not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry
> >>> flushing data in f2fs_sync_file, once it gets -EAGAIN.
> >>>
> >>> Any thoughts?
> >>
> >> What about adding inode_lock in f2fs_sync_file to exclude other
> >> foreground operation which have reversed lock order? Atomic_commit is OK
> >> since it has inode_lock in its path.
> >
> > I have concerned about performance regression, if we do that.
>
> I think fsync vs write or fsync vs fsync scenarios are unusual, so is
> there any usecase?
Well, that'd be common to call multiple fsync calls at the same time.
Like dbench or tiotest?
>
> Thanks,
>
> >
> >>
> >> Thanks,
> >>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>
> >>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>>> ---
> >>>>> fs/f2fs/data.c | 5 +++--
> >>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>>>> index 7d3af48d34a9..9141bd19a902 100644
> >>>>> --- a/fs/f2fs/data.c
> >>>>> +++ b/fs/f2fs/data.c
> >>>>> @@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> - if (fio->need_lock == LOCK_REQ)
> >>>>> - f2fs_lock_op(fio->sbi);
> >>>>> + /* Deadlock due to between page->lock and f2fs_lock_op */
> >>>>> + if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
> >>>>> + return -EAGAIN;
> >>>>>
> >>>>> err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
> >>>>> if (err)
> >>>>>
> >>>
> >>> ------------------------------------------------------------------------------
> >>> Check out the vibrant tech community on one of the world's most
> >>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> >>> _______________________________________________
> >>> Linux-f2fs-devel mailing list
> >>> Linux-f2fs-devel@lists.sourceforge.net
> >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-07-01 14:27 ` Jaegeuk Kim
@ 2017-07-05 2:58 ` Chao Yu
2017-07-05 3:28 ` Jaegeuk Kim
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2017-07-05 2:58 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-fsdevel, linux-kernel, linux-f2fs-devel
On 2017/7/1 22:27, Jaegeuk Kim wrote:
> On 07/01, Chao Yu wrote:
>> On 2017/7/1 15:28, Jaegeuk Kim wrote:
>>> On 06/26, Chao Yu wrote:
>>>> Hi Jaegeuk,
>>>>
>>>> On 2017/6/26 22:54, Jaegeuk Kim wrote:
>>>>> Hi Chao,
>>>>>
>>>>> On 06/26, Chao Yu wrote:
>>>>>> Hi Jaegeuk,
>>>>>>
>>>>>> On 2017/6/25 0:25, Jaegeuk Kim wrote:
>>>>>>> - punch_hole
>>>>>>> - fill_zero
>>>>>>> - f2fs_lock_op
>>>>>>> - get_new_data_page
>>>>>>> - lock_page
>>>>>>>
>>>>>>> - f2fs_write_data_pages
>>>>>>> - lock_page
>>>>>>> - do_write_data_page
>>>>>>> - f2fs_lock_op
>>>>>>
>>>>>> Good catch!
>>>>>>
>>>>>> With this implementation, page writeback can fail due to concurrent checkpoint,
>>>>>> this will make fsync/atomic_commit which trigger synchronous write failed randomly.
>>>>>>
>>>>>> How about unifying the lock order in punch_hole as one in writepages for regular
>>>>>> inode? We can add one more parameter in get_new_data_page to indicate whether
>>>>>> callee needs to lock cp_rwsem.
>>>>>
>>>>> Currently, there would be some places to keep cp_rwsem -> page.lock, which seems
>>>>> not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry
>>>>> flushing data in f2fs_sync_file, once it gets -EAGAIN.
>>>>>
>>>>> Any thoughts?
>>>>
>>>> What about adding inode_lock in f2fs_sync_file to exclude other
>>>> foreground operation which have reversed lock order? Atomic_commit is OK
>>>> since it has inode_lock in its path.
>>>
>>> I have concerned about performance regression, if we do that.
>>
>> I think fsync vs write or fsync vs fsync scenarios are unusual, so is
>> there any usecase?
>
> Well, that'd be common to call multiple fsync calls at the same time.
> Like dbench or tiotest?
Do you have test numbers of dbench/tiotest with inode:lock in fsync?
Thanks,
>
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>>>> ---
>>>>>>> fs/f2fs/data.c | 5 +++--
>>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>>> index 7d3af48d34a9..9141bd19a902 100644
>>>>>>> --- a/fs/f2fs/data.c
>>>>>>> +++ b/fs/f2fs/data.c
>>>>>>> @@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> - if (fio->need_lock == LOCK_REQ)
>>>>>>> - f2fs_lock_op(fio->sbi);
>>>>>>> + /* Deadlock due to between page->lock and f2fs_lock_op */
>>>>>>> + if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
>>>>>>> + return -EAGAIN;
>>>>>>>
>>>>>>> err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
>>>>>>> if (err)
>>>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>> Check out the vibrant tech community on one of the world's most
>>>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>>>> _______________________________________________
>>>>> Linux-f2fs-devel mailing list
>>>>> Linux-f2fs-devel@lists.sourceforge.net
>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-07-05 2:58 ` Chao Yu
@ 2017-07-05 3:28 ` Jaegeuk Kim
2017-07-07 19:13 ` [f2fs-dev] [PATCH 1/2 v2] " Jaegeuk Kim
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2017-07-05 3:28 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-fsdevel, linux-kernel, linux-f2fs-devel
On 07/05, Chao Yu wrote:
> On 2017/7/1 22:27, Jaegeuk Kim wrote:
> > On 07/01, Chao Yu wrote:
> >> On 2017/7/1 15:28, Jaegeuk Kim wrote:
> >>> On 06/26, Chao Yu wrote:
> >>>> Hi Jaegeuk,
> >>>>
> >>>> On 2017/6/26 22:54, Jaegeuk Kim wrote:
> >>>>> Hi Chao,
> >>>>>
> >>>>> On 06/26, Chao Yu wrote:
> >>>>>> Hi Jaegeuk,
> >>>>>>
> >>>>>> On 2017/6/25 0:25, Jaegeuk Kim wrote:
> >>>>>>> - punch_hole
> >>>>>>> - fill_zero
> >>>>>>> - f2fs_lock_op
> >>>>>>> - get_new_data_page
> >>>>>>> - lock_page
> >>>>>>>
> >>>>>>> - f2fs_write_data_pages
> >>>>>>> - lock_page
> >>>>>>> - do_write_data_page
> >>>>>>> - f2fs_lock_op
> >>>>>>
> >>>>>> Good catch!
> >>>>>>
> >>>>>> With this implementation, page writeback can fail due to concurrent checkpoint,
> >>>>>> this will make fsync/atomic_commit which trigger synchronous write failed randomly.
> >>>>>>
> >>>>>> How about unifying the lock order in punch_hole as one in writepages for regular
> >>>>>> inode? We can add one more parameter in get_new_data_page to indicate whether
> >>>>>> callee needs to lock cp_rwsem.
> >>>>>
> >>>>> Currently, there would be some places to keep cp_rwsem -> page.lock, which seems
> >>>>> not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry
> >>>>> flushing data in f2fs_sync_file, once it gets -EAGAIN.
> >>>>>
> >>>>> Any thoughts?
> >>>>
> >>>> What about adding inode_lock in f2fs_sync_file to exclude other
> >>>> foreground operation which have reversed lock order? Atomic_commit is OK
> >>>> since it has inode_lock in its path.
> >>>
> >>> I have concerned about performance regression, if we do that.
> >>
> >> I think fsync vs write or fsync vs fsync scenarios are unusual, so is
> >> there any usecase?
> >
> > Well, that'd be common to call multiple fsync calls at the same time.
> > Like dbench or tiotest?
>
> Do you have test numbers of dbench/tiotest with inode:lock in fsync?
No, do we need?
>
> Thanks,
>
> >
> >>
> >> Thanks,
> >>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>>>>> ---
> >>>>>>> fs/f2fs/data.c | 5 +++--
> >>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>>>>>> index 7d3af48d34a9..9141bd19a902 100644
> >>>>>>> --- a/fs/f2fs/data.c
> >>>>>>> +++ b/fs/f2fs/data.c
> >>>>>>> @@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
> >>>>>>> }
> >>>>>>> }
> >>>>>>>
> >>>>>>> - if (fio->need_lock == LOCK_REQ)
> >>>>>>> - f2fs_lock_op(fio->sbi);
> >>>>>>> + /* Deadlock due to between page->lock and f2fs_lock_op */
> >>>>>>> + if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
> >>>>>>> + return -EAGAIN;
> >>>>>>>
> >>>>>>> err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
> >>>>>>> if (err)
> >>>>>>>
> >>>>>
> >>>>> ------------------------------------------------------------------------------
> >>>>> Check out the vibrant tech community on one of the world's most
> >>>>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> >>>>> _______________________________________________
> >>>>> Linux-f2fs-devel mailing list
> >>>>> Linux-f2fs-devel@lists.sourceforge.net
> >>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 1/2 v2] f2fs: avoid deadlock caused by lock order of page and lock_op
2017-07-05 3:28 ` Jaegeuk Kim
@ 2017-07-07 19:13 ` Jaegeuk Kim
0 siblings, 0 replies; 16+ messages in thread
From: Jaegeuk Kim @ 2017-07-07 19:13 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
Change log from v1:
- hide EAGAIN to users
fs/f2fs/data.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 72fc866cad19..7dd5fb647d43 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
}
}
- if (fio->need_lock == LOCK_REQ)
- f2fs_lock_op(fio->sbi);
+ /* Deadlock due to between page->lock and f2fs_lock_op */
+ if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+ return -EAGAIN;
err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
if (err)
@@ -1667,7 +1668,7 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
}
done_index = page->index;
-
+retry_write:
lock_page(page);
if (unlikely(page->mapping != mapping)) {
@@ -1703,6 +1704,15 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
unlock_page(page);
ret = 0;
continue;
+ } else if (ret == -EAGAIN) {
+ ret = 0;
+ if (wbc->sync_mode == WB_SYNC_ALL) {
+ cond_resched();
+ congestion_wait(BLK_RW_ASYNC,
+ HZ/50);
+ goto retry_write;
+ }
+ continue;
}
done_index = page->index + 1;
done = 1;
--
2.13.0.rc1.294.g07d810a77f-goog
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-07-07 19:14 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-24 16:25 [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op Jaegeuk Kim
2017-06-24 16:25 ` [PATCH 2/2] f2fs: report # of free inodes more precisely Jaegeuk Kim
2017-06-26 10:52 ` [f2fs-dev] " Chao Yu
2017-06-26 14:58 ` Jaegeuk Kim
2017-06-28 13:01 ` Chao Yu
2017-07-01 7:27 ` Jaegeuk Kim
2017-07-01 8:28 ` Chao Yu
2017-06-26 8:15 ` [f2fs-dev] [PATCH 1/2] f2fs: avoid deadlock caused by lock order of page and lock_op Chao Yu
2017-06-26 14:54 ` Jaegeuk Kim
2017-06-26 15:42 ` Chao Yu
2017-07-01 7:28 ` Jaegeuk Kim
2017-07-01 8:41 ` Chao Yu
2017-07-01 14:27 ` Jaegeuk Kim
2017-07-05 2:58 ` Chao Yu
2017-07-05 3:28 ` Jaegeuk Kim
2017-07-07 19:13 ` [f2fs-dev] [PATCH 1/2 v2] " Jaegeuk Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).