[Cluster-devel] [GFS2 PATCH v2] gfs2: if finish_open returns error, clean up iopen glock mess

[Cluster-devel] [GFS2 PATCH v2] gfs2: if finish_open returns error, clean up iopen glock mess

[Bob Peterson]

Hi,

Just noticed (and fixed) the redundant clear_bit. Sorry.
---
Before this patch, if anything went wrong in function gfs2_create_inode
it would goto fail_gunlock3 and clean up the iopen glock it had just
created and locked. However, if function finish_open returns an error
it did not. That meant subsequent attempts to create the file were
seen as glock recursion errors on the iopen glock.

This patch adds additional checking for an error from finish_open and
cleans up the iopen glock appropriately.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index dcb5d363f9b9..36eb223b185e 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -772,6 +772,11 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 	gfs2_glock_dq_uninit(ghs);
 	gfs2_glock_dq_uninit(ghs + 1);
 	clear_bit(GLF_INODE_CREATING, &io_gl->gl_flags);
+	if (error) {
+		glock_clear_object(io_gl, ip);
+		gfs2_glock_dq_uninit(&ip->i_iopen_gh);
+		gfs2_glock_put(io_gl);
+	}
 	return error;
 
 fail_gunlock3:

[Cluster-devel] [GFS2 PATCH v2] gfs2: if finish_open returns error, clean up iopen glock mess

[Andreas Gruenbacher]

Bob,

On Fri, Nov 15, 2019 at 5:03 PM Bob Peterson <rpeterso@redhat.com> wrote:
> Just noticed (and fixed) the redundant clear_bit. Sorry.
> ---
> Before this patch, if anything went wrong in function gfs2_create_inode
> it would goto fail_gunlock3 and clean up the iopen glock it had just
> created and locked. However, if function finish_open returns an error
> it did not. That meant subsequent attempts to create the file were
> seen as glock recursion errors on the iopen glock.
>
> This patch adds additional checking for an error from finish_open and
> cleans up the iopen glock appropriately.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/inode.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index dcb5d363f9b9..36eb223b185e 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -772,6 +772,11 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>         gfs2_glock_dq_uninit(ghs);
>         gfs2_glock_dq_uninit(ghs + 1);
>         clear_bit(GLF_INODE_CREATING, &io_gl->gl_flags);
> +       if (error) {
> +               glock_clear_object(io_gl, ip);
> +               gfs2_glock_dq_uninit(&ip->i_iopen_gh);
> +               gfs2_glock_put(io_gl);
> +       }
>         return error;
>
>  fail_gunlock3:
>

this doesn't look quite right. In gfs2_create_inode, the call to
d_instantiate is supposed to be the "point of no return": after that,
the vfs should be calling .evict_inode once the inode goes away. So it
looks more like gfs2_evict_inode isn't cleaning things up properly to
me. I need to investigate further.

Thanks,
Andreas