<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<blockquote style="margin-left: 0.8ex; padding-left: 1ex; border-left: 3px solid rgb(200, 200, 200);">
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
- Flag name suggests it should always indicate that inode is locked,</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
but for !close_wait path it does not, this can be a problem if we reuse</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
this flag without checking that it is not always set.</div>
</blockquote>
<p class="elementToProof" style="margin-top: 0px; margin-bottom: 0px; font-family: Aptos; font-size: 11pt;">
<span style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">I don't think the flag will ever be used in !close_wait path. </span></p>
<div class="elementToProof" style="margin-top: 0px; margin-bottom: 0px; font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<blockquote style="margin-left: 0.8ex; padding-left: 1ex; border-left: 3px solid rgb(200, 200, 200);">
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
- Let's maybe indicate the flag scope, we currently use it only on fuse_open,</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
maybe we should rename it to FUSE_S_OPEN_INODE_LOCKED?</div>
</blockquote>
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
yeah, it's more appropriate </div>
<blockquote style="margin-left: 0.8ex; padding-left: 1ex; border-left: 3px solid rgb(200, 200, 200);">
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
- What if after rebase the lock condition will change e.g. not (is_wb_truncate</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
|| dax_truncate) but something else, maybe we should use a helper variable</div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
"locked" in fuse_open which is set only when lock is taken?</div>
</blockquote>
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
agreed, this could be a risk. </div>
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<hr style="display: inline-block; width: 98%;">
<div id="divRplyFwdMsg">
<div style="direction: ltr; font-family: Calibri, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);">
<b>From:</b> Pavel Tikhomirov <ptikhomirov@virtuozzo.com><br>
<b>Sent:</b> 29 June 2026 18:04<br>
<b>To:</b> Kui Liu <kui.liu@virtuozzo.com>; devel@openvz.org <devel@openvz.org><br>
<b>Cc:</b> Andrey Zaitsev <azaitsev@virtuozzo.com><br>
<b>Subject:</b> Re: [Devel] [PATCH VZ10 v3] fs/fuse: align fuse_create_open()'s open path with fuse_open()</div>
<div style="direction: ltr;"> </div>
</div>
<div style="font-size: 11pt;">Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com><br>
<br>
Generally looks good, though, please, see comments inline.<br>
<br>
On 6/26/26 16:32, Liu Kui wrote:<br>
> fuse_create_open()'s close_wait branch was not correct; in particular the<br>
> fput() in its error path was wrong. fuse_open()'s close_wait branch should<br>
> apply instead.<br>
><br>
> The two sites differ in one important way, though: fuse_open() runs the<br>
> block before FMODE_OPENED is set, where fuse_sync_release() is the correct<br>
> cleanup on error, whereas fuse_create_open() runs it after FMODE_OPENED is<br>
> set, where fuse_sync_release() must not be called. To unify them, refactor<br>
> the sequence into fuse_open_close_wait() and call it from<br>
> fuse_finish_open(), so the block runs before FMODE_OPENED on both paths.<br>
> The error then propagates to the caller and the block itself no longer<br>
> needs to call fuse_sync_release().<br>
><br>
> Add a per-open FUSE_S_INODE_LOCKED flag telling fuse_open_close_wait() that<br>
> the caller already holds the inode lock, so it is not re-acquired and<br>
> deadlocks on the atomic O_TRUNC path.<br>
><br>
> <a href="https://virtuozzo.atlassian.net/browse/VSTOR-136027" id="OWAd3c088a2-5827-6ff5-d503-ff7bf9ca59e6" class="OWAAutoLink" data-auth="NotApplicable">
https://virtuozzo.atlassian.net/browse/VSTOR-136027</a><br>
> Signed-off-by: Liu Kui <kui.liu@virtuozzo.com><br>
> ---<br>
> Changes in v3:<br>
> - Call fuse_open_close_wait() from fuse_finish_open() instead of from<br>
> the fuse_open() and fuse_create_open() call sites, so the sequence<br>
> runs before FMODE_OPENED is set on both paths.<br>
> - Drop the fuse_sync_release() call inside fuse_open_close_wait(); the<br>
> error now propagates to the caller, which releases the file before<br>
> FMODE_OPENED is set.<br>
> - Make fuse_open_close_wait() static and drop its fuse_i.h prototype.<br>
> - Acquire the inode lock in fuse_open_close_wait() only when the caller<br>
> does not already hold it, tracked by a per-open FUSE_S_INODE_LOCKED<br>
> flag, so the atomic O_TRUNC path (which already holds it) does not<br>
> self-deadlock.<br>
><br>
> Changes in v2:<br>
> - Factor the close_wait open branch into fuse_open_close_wait();<br>
> call it from both fuse_open() and fuse_create_open() instead of<br>
> duplicating the block in each.<br>
> - Use fuse_sync_release() on the close_wait error path for both<br>
> callers. It is functionally equivalent to fuse_release_common()<br>
> used in fuse_open().<br>
><br>
> fs/fuse/dir.c | 17 -----------<br>
> fs/fuse/file.c | 73 ++++++++++++++++++++++++++++--------------------<br>
> fs/fuse/fuse_i.h | 4 ++-<br>
> 3 files changed, 46 insertions(+), 48 deletions(-)<br>
><br>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c<br>
> index 45ee4f1e4b68..9799c2b6eb4d 100644<br>
> --- a/fs/fuse/dir.c<br>
> +++ b/fs/fuse/dir.c<br>
> @@ -722,23 +722,6 @@ static int fuse_create_open(struct mnt_idmap *idmap, struct inode *dir,<br>
> invalidate_inode_pages2(inode->i_mapping);<br>
> }<br>
> <br>
> - if (fm->fc->close_wait) {<br>
> - struct fuse_inode *fi = get_fuse_inode(inode);<br>
> - bool need_open;<br>
> -<br>
> - inode_lock(inode);<br>
> - spin_lock(&fi->lock);<br>
> - need_open = (++fi->num_openers == 1);<br>
> - spin_unlock(&fi->lock);<br>
> -<br>
> - if (need_open && fm->fc->kio.op && fm->fc->kio.op->file_open) {<br>
> - err = fm->fc->kio.op->file_open(file, inode);<br>
> - if (err)<br>
> - fput(file);<br>
> - }<br>
> - inode_unlock(inode);<br>
> - }<br>
> -<br>
> return err;<br>
> <br>
> out_free_ff:<br>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c<br>
> index 58202a96517e..8750cbdf0596 100644<br>
> --- a/fs/fuse/file.c<br>
> +++ b/fs/fuse/file.c<br>
> @@ -286,6 +286,43 @@ static void fuse_link_rw_file(struct file *file)<br>
> spin_unlock(&fi->lock);<br>
> }<br>
> <br>
> +static int fuse_open_close_wait(struct inode *inode, struct file *file)<br>
> +{<br>
> + struct fuse_conn *fc = get_fuse_conn(inode);<br>
> + struct fuse_inode *fi = get_fuse_inode(inode);<br>
> + struct fuse_file *ff = file->private_data;<br>
> + /* the caller already holds the inode lock (e.g. truncate open) */<br>
> + bool locked = test_and_clear_bit(FUSE_S_INODE_LOCKED, &ff->ff_state);<br>
> + int err = 0;<br>
> +<br>
> + if (!locked)<br>
> + inode_lock(inode);<br>
> + spin_lock(&fi->lock);<br>
> +<br>
> + if (++fi->num_openers == 1 || fi->i_size_unstable) {<br>
> + fi->i_size_unstable = 1;<br>
> + fi->inval_mask = ~0;<br>
> + spin_unlock(&fi->lock);<br>
> + err = fuse_update_attributes(inode, file, ~0);<br>
> +<br>
> + if (!err && fc->kio.op && fc->kio.op->file_open)<br>
> + err = fc->kio.op->file_open(file, inode);<br>
> +<br>
> + spin_lock(&fi->lock);<br>
> + fi->i_size_unstable = 0;<br>
> + if (err)<br>
> + fi->num_openers--;<br>
> + }<br>
> +<br>
> + file->f_mode |= FMODE_NOWAIT;<br>
> +<br>
> + spin_unlock(&fi->lock);<br>
> + if (!locked)<br>
> + inode_unlock(inode);<br>
> +<br>
> + return err;<br>
> +}<br>
> +<br>
> int fuse_finish_open(struct inode *inode, struct file *file)<br>
> {<br>
> struct fuse_file *ff = file->private_data;<br>
> @@ -319,6 +356,9 @@ int fuse_finish_open(struct inode *inode, struct file *file)<br>
> */<br>
> if (!test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state))<br>
> err = fuse_wait_on_inval_files(inode);<br>
> +<br>
> + if (!err)<br>
> + err = fuse_open_close_wait(inode, file);<br>
> }<br>
> <br>
> return err;<br>
> @@ -378,6 +418,9 @@ static int fuse_open(struct inode *inode, struct file *file)<br>
> err = fuse_do_open(fm, get_node_id(inode), file, false);<br>
> if (!err) {<br>
> ff = file->private_data;<br>
> + /* tell fuse_open_close_wait() we already hold the inode lock */<br>
> + if ((is_wb_truncate || dax_truncate) && fc->close_wait)<br>
> + set_bit(FUSE_S_INODE_LOCKED, &ff->ff_state);<br>
<br>
Technically correct. Some concerns:<br>
<br>
- Flag name suggests it should always indicate that inode is locked,<br>
but for !close_wait path it does not, this can be a problem if we reuse<br>
this flag without checking that it is not always set.<br>
<br>
- Let's maybe indicate the flag scope, we currently use it only on fuse_open,<br>
maybe we should rename it to FUSE_S_OPEN_INODE_LOCKED?<br>
<br>
- What if after rebase the lock condition will change e.g. not (is_wb_truncate<br>
|| dax_truncate) but something else, maybe we should use a helper variable<br>
"locked" in fuse_open which is set only when lock is taken?<br>
<br>
> err = fuse_finish_open(inode, file);<br>
> if (err)<br>
> fuse_sync_release(fi, ff, file->f_flags);<br>
> @@ -399,36 +442,6 @@ static int fuse_open(struct inode *inode, struct file *file)<br>
> if (is_wb_truncate || dax_truncate)<br>
> inode_unlock(inode);<br>
> <br>
> - if (!err && fc->close_wait) {<br>
> - inode_lock(inode);<br>
> - spin_lock(&fi->lock);<br>
> -<br>
> - if (++fi->num_openers == 1 || fi->i_size_unstable) {<br>
> - fi->i_size_unstable = 1;<br>
> - fi->inval_mask = ~0;<br>
> - spin_unlock(&fi->lock);<br>
> - err = fuse_update_attributes(inode, file, ~0);<br>
> -<br>
> - if (!err && fc->kio.op && fc->kio.op->file_open)<br>
> - err = fc->kio.op->file_open(file, inode);<br>
> -<br>
> - spin_lock(&fi->lock);<br>
> - fi->i_size_unstable = 0;<br>
> - if (err)<br>
> - fi->num_openers--;<br>
> - }<br>
> -<br>
> - file->f_mode |= FMODE_NOWAIT;<br>
> -<br>
> - spin_unlock(&fi->lock);<br>
> - inode_unlock(inode);<br>
> -<br>
> - if (err) {<br>
> - fuse_release_common(file, false);<br>
> - return err;<br>
> - }<br>
> - }<br>
> -<br>
> return err;<br>
> }<br>
> <br>
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h<br>
> index a834a5b4dfe2..53dfcb495ca0 100644<br>
> --- a/fs/fuse/fuse_i.h<br>
> +++ b/fs/fuse/fuse_i.h<br>
> @@ -327,7 +327,9 @@ struct fuse_file {<br>
> enum {<br>
> /** Any fops on given ff should fail immediately */<br>
> FUSE_S_FAIL_IMMEDIATELY = 0,<br>
> - FUSE_S_CLOSING = 1<br>
> + FUSE_S_CLOSING = 1,<br>
> + /** The open path already holds the inode lock (close_wait) */<br>
> + FUSE_S_INODE_LOCKED = 2,<br>
> };<br>
> <br>
> /** One input argument of a request */<br>
<br>
--<br>
Best regards, Pavel Tikhomirov<br>
Senior Software Developer, Virtuozzo.<br>
<br>
</div>
</body>
</html>