Hi Jaegeuk, Pengyang,
On 2017/4/26 9:27, Jaegeuk Kim wrote:
> On 04/25, Jaegeuk Kim wrote:
>> Hi Pengyang,
>>
>> This makes xfstests/generic/013 stuck on fsstress.
>
> Oh, it seems lock inversion problem between f2fs_lock_op and
> get_dnode_of_data.
> The basic rule is get_dnode_of_data is covered by f2fs_lock_op.
> I'll drop this patch at this moment.
Seems we need to change as below?
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 9ec9eace05df..f3564a948293 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1364,12 +1364,17 @@ int do_write_data_page(struct f2fs_io_info *fio)
if (valid_ipu_blkaddr(fio)) {
ipu_force = true;
+ fio->need_lock = false;
goto got_it;
}
}
+
+ if (fio->need_lock)
+ f2fs_lock_op(fio->sbi);
+
err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
if (err)
- return err;
+ goto out;
fio->old_blkaddr = dn.data_blkaddr;
@@ -1400,17 +1405,16 @@ int do_write_data_page(struct f2fs_io_info *fio)
}
/* LFS mode write path */
- if (!fio->cp_rwsem_locked)
- f2fs_lock_op(fio->sbi);
write_data_page(&dn, fio);
trace_f2fs_do_write_data_page(page, OPU);
set_inode_flag(inode, FI_APPEND_WRITE);
if (page->index == 0)
set_inode_flag(inode, FI_FIRST_BLOCK_WRITTEN);
- if (!fio->cp_rwsem_locked)
- f2fs_unlock_op(fio->sbi);
out_writepage:
f2fs_put_dnode(&dn);
+out:
+ if (fio->need_lock)
+ f2fs_unlock_op(fio->sbi);
return err;
}
@@ -1435,7 +1439,7 @@ static int __write_data_page(struct page *page, bool
*submitted,
.page = page,
.encrypted_page = NULL,
.submitted = false,
- .cp_rwsem_locked = true,
+ .need_lock = true,
};
trace_f2fs_writepage(page, DATA);
@@ -1471,6 +1475,7 @@ static int __write_data_page(struct page *page, bool
*submitted,
/* Dentry blocks are controlled by checkpoint */
if (S_ISDIR(inode->i_mode)) {
+ fio.need_lock = false,
err = do_write_data_page(&fio);
goto done;
}
@@ -1488,13 +1493,12 @@ static int __write_data_page(struct page *page, bool
*submitted,
if (!err)
goto out;
}
- f2fs_lock_op(sbi);
+
if (err == -EAGAIN)
err = do_write_data_page(&fio);
if (F2FS_I(inode)->last_disk_size < psize)
F2FS_I(inode)->last_disk_size = psize;
- if (fio.cp_rwsem_locked)
- f2fs_unlock_op(sbi);
+
done:
if (err && err != -ENOENT)
goto redirty_out;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index bf48213642ab..95fb347e7abe 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -802,7 +802,7 @@ struct f2fs_io_info {
struct page *page; /* page to be written */
struct page *encrypted_page; /* encrypted page */
bool submitted; /* indicate IO submission */
- bool cp_rwsem_locked; /* indicate cp_rwsem is held */
+ bool need_lock; /* indicate we need to lock cp_rwsem */
};
#define is_read_io(rw) ((rw) == READ)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index c2a9ae8397d3..ffabdcf52b85 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -717,6 +717,7 @@ static void move_data_page(struct inode *inode, block_t
bidx, int gc_type,
.old_blkaddr = NULL_ADDR,
.page = page,
.encrypted_page = NULL,
+ .need_lock = false,
};
bool is_dirty = PageDirty(page);
int err;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index ee9b56da0b7e..c155adbac8fc 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -293,6 +293,7 @@ static int __commit_inmem_pages(struct inode *inode,
.op_flags = REQ_SYNC | REQ_PRIO,
.old_blkaddr = NULL_ADDR,
.encrypted_page = NULL,
+ .need_lock = false,
};
pgoff_t last_idx = ULONG_MAX;
int err = 0;
>
> Thanks,
>
>>
>> Thanks,
>>
>> On 04/25, Hou Pengyang wrote:
>>> IPU checking is under f2fs_lock_op, as a result, some IPU page(such as
>>> fsync/fdatasync IPU)
>>> may be blocked by a long time cp.
>>>
>>> This patch fix this by doing IPU checking before f2fs_lock_op, so fsync IPU
>>> could go along with cp.
>>>
>>> Suggested-by: He Yunlei <heyun...@huawei.com>
>>> Signed-off-by: Hou Pengyang <houpengy...@huawei.com>
>>> ---
>>> fs/f2fs/data.c | 14 +++++++-------
>>> fs/f2fs/gc.c | 1 +
>>> fs/f2fs/segment.c | 1 +
>>> 3 files changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 183a426..9bf9c7d 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -1380,19 +1380,20 @@ int do_write_data_page(struct f2fs_io_info *fio)
>>> * it had better in-place writes for updated data.
>>> */
>>> if (need_inplace_update(fio)) {
>>> - f2fs_bug_on(fio->sbi, !fio->cp_rwsem_locked);
>>> - f2fs_unlock_op(fio->sbi);
>>> - fio->cp_rwsem_locked = false;
>>> -
>>> + f2fs_bug_on(fio->sbi, fio->cp_rwsem_locked);
>>> err = rewrite_data_page(fio);
>>> trace_f2fs_do_write_data_page(fio->page, IPU);
>>> set_inode_flag(inode, FI_UPDATE_WRITE);
>>> } else {
>>> + if (!fio->cp_rwsem_locked)
>>> + f2fs_lock_op(fio->sbi);
>>> write_data_page(&dn, fio);
>>> trace_f2fs_do_write_data_page(page, OPU);
>>> set_inode_flag(inode, FI_APPEND_WRITE);
>>> if (page->index == 0)
>>> set_inode_flag(inode, FI_FIRST_BLOCK_WRITTEN);
>>> + if (!fio->cp_rwsem_locked)
>>> + f2fs_unlock_op(fio->sbi);
>>> }
>>> out_writepage:
>>> f2fs_put_dnode(&dn);
>>> @@ -1473,13 +1474,12 @@ static int __write_data_page(struct page *page,
>>> bool *submitted,
>>> if (!err)
>>> goto out;
>>> }
>>> - f2fs_lock_op(sbi);
>>> +
>>> + fio.cp_rwsem_locked = false;
>>> if (err == -EAGAIN)
>>> err = do_write_data_page(&fio);
>>> if (F2FS_I(inode)->last_disk_size < psize)
>>> F2FS_I(inode)->last_disk_size = psize;
>>> - if (fio.cp_rwsem_locked)
>>> - f2fs_unlock_op(sbi);
>>> done:
>>> if (err && err != -ENOENT)
>>> goto redirty_out;
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index e034857..b32cc30 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -715,6 +715,7 @@ static void move_data_page(struct inode *inode, block_t
>>> bidx, int gc_type,
>>> .old_blkaddr = NULL_ADDR,
>>> .page = page,
>>> .encrypted_page = NULL,
>>> + .cp_rwsem_locked = true,
>>> };
>>> bool is_dirty = PageDirty(page);
>>> int err;
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 9f86b98..463a77b 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -293,6 +293,7 @@ static int __commit_inmem_pages(struct inode *inode,
>>> .op_flags = REQ_SYNC | REQ_PRIO,
>>> .old_blkaddr = NULL_ADDR,
>>> .encrypted_page = NULL,
>>> + .cp_rwsem_locked = true,
>>> };
>>> pgoff_t last_idx = ULONG_MAX;
>>> int err = 0;
>>> --
>>> 2.10.1
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> 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
>>
>> ------------------------------------------------------------------------------
>> 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
>
> ------------------------------------------------------------------------------
> 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
>
> .
>
------------------------------------------------------------------------------
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
