fuse_finish_open() will be called with FUSE_NOWRITE set in case of atomic
O_TRUNC open(), so commit 76224355db75 ("fuse: truncate pagecache on
atomic_o_trunc") replaced invalidate_inode_pages2() by truncate_pagecache()
in such a case to avoid the A-A deadlock. However, we found another A-B-B-A
deadlock related to the case above, which will cause the xfstests
generic/464 testcase hung in our virtio-fs test environment.
Consider two processes concurrently open one same file, one with O_TRUNC
and another without O_TRUNC. The deadlock case is described below, if
open(O_TRUNC) is already set_nowrite(acquired A), and is trying to lock
a page (acquiring B), open() could have held the page lock (acquired B),
and waiting on the page writeback (acquiring A). This would lead to
deadlocks.
This commit tries to fix it by locking inode in fuse_open_common() even
if O_TRUNC is not set. This introduces a lock C to protect the area with
A-B-B-A the deadlock rick.
open(O_TRUNC)
----------------------------------------------------------------
fuse_open_common
lock_inode [C acquire]
fuse_set_nowrite [A acquire]
fuse_finish_open
truncate_pagecache
lock_page [B acquire]
truncate_inode_page
unlock_page [B release]
fuse_release_nowrite [A release]
unlock_inode [C release]
----------------------------------------------------------------
open()
----------------------------------------------------------------
fuse_open_common
(lock_inode) [C acquire, introduced by this commit]
fuse_finish_open
invalidate_inode_pages2
lock_page [B acquire]
fuse_launder_page
fuse_wait_on_page_writeback [A acquire]
unlock_page [B release]
(unlock_inode) [C release, introduced by this commit]
----------------------------------------------------------------
Signed-off-by: Jiachen Zhang <[email protected]>
---
fs/fuse/file.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 829094451774..73fec30e0a37 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -229,8 +229,10 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
bool is_wb_truncate = (file->f_flags & O_TRUNC) &&
fc->atomic_o_trunc &&
fc->writeback_cache;
- bool dax_truncate = (file->f_flags & O_TRUNC) &&
+ bool is_dax_truncate = (file->f_flags & O_TRUNC) &&
fc->atomic_o_trunc && FUSE_IS_DAX(inode);
+ bool may_truncate = fc->atomic_o_trunc &&
+ (fc->writeback_cache || FUSE_IS_DAX(inode));
if (fuse_is_bad(inode))
return -EIO;
@@ -239,12 +241,12 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
if (err)
return err;
- if (is_wb_truncate || dax_truncate) {
+ if (may_truncate)
inode_lock(inode);
+ if (is_wb_truncate || is_dax_truncate)
fuse_set_nowrite(inode);
- }
- if (dax_truncate) {
+ if (is_dax_truncate) {
filemap_invalidate_lock(inode->i_mapping);
err = fuse_dax_break_layouts(inode, 0, 0);
if (err)
@@ -256,13 +258,13 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
fuse_finish_open(inode, file);
out:
- if (dax_truncate)
+ if (is_dax_truncate)
filemap_invalidate_unlock(inode->i_mapping);
- if (is_wb_truncate | dax_truncate) {
+ if (is_wb_truncate | is_dax_truncate)
fuse_release_nowrite(inode);
+ if (may_truncate)
inode_unlock(inode);
- }
return err;
}
--
2.20.1
On Thu, 16 Dec 2021 at 15:46, Jiachen Zhang
<[email protected]> wrote:
>
> fuse_finish_open() will be called with FUSE_NOWRITE set in case of atomic
> O_TRUNC open(), so commit 76224355db75 ("fuse: truncate pagecache on
> atomic_o_trunc") replaced invalidate_inode_pages2() by truncate_pagecache()
> in such a case to avoid the A-A deadlock. However, we found another A-B-B-A
> deadlock related to the case above, which will cause the xfstests
> generic/464 testcase hung in our virtio-fs test environment.
>
> Consider two processes concurrently open one same file, one with O_TRUNC
> and another without O_TRUNC. The deadlock case is described below, if
> open(O_TRUNC) is already set_nowrite(acquired A), and is trying to lock
> a page (acquiring B), open() could have held the page lock (acquired B),
> and waiting on the page writeback (acquiring A). This would lead to
> deadlocks.
>
> This commit tries to fix it by locking inode in fuse_open_common() even
> if O_TRUNC is not set. This introduces a lock C to protect the area with
> A-B-B-A the deadlock rick.
Okay.
One problem is that this seems to affect a number of other calls to
invalidate_inode_pages2(), specifically those without lock_inode()
protection:
- dmap_writeback_invalidate()
- fuse_file_mmap()
- fuse_change_attributes()
- fuse_reverse_inval_inode()
fuse_change_attributes() is especially problematic because it can be
called with or without the inode lock.
The other issue is that locking the inode may impact performance and
doing it unconditionally for all opens seems excessive.
If there are no better ideas, then the brute force fix is to introduce
another lock (since the inode lock cannot always be used) to protect
fuse_set_nowrite()/fuse_clear_nowrite() racing with
invalidate_inode_pages2().
Thoughts?
Thanks,
Miklos