[Cluster-devel] [PATCH 1/3] gfs2: Prevent double iput for journal on error

[Andreas Gruenbacher <agruenba@redhat.com>]

On Wed, Jul 27, 2022 at 6:32 PM Bob Peterson <rpeterso@redhat.com> wrote:
> When a gfs2 file system is withdrawn, it requests recovery from another
> cluster node. To do that, it evicts its journal from memory, but it
> keeps the journal entry queued to the journals queue, jindex_list. After
> recovery it tries to grab a new inode for its (recovered) journal. If it
> cannot, it skips further recovery but its evicted journal is still on
> the jindex list, which means unmount will try to iput it a second time
> after it's been evicted. This second iput causes vfs to complain and BUG
> out:
>
> kernel BUG at fs/inode.c:1680!
>
> To prevent this, this patch takes steps to dequeue the journal
> descriptor from the list when it cannot get a replacement inode. So
> unmount won't find it on the list and try to iput it again.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/util.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
> index 8241029a2a5d..78cb12d0fba1 100644
> --- a/fs/gfs2/util.c
> +++ b/fs/gfs2/util.c
> @@ -275,6 +275,17 @@ static void signal_our_withdraw(struct gfs2_sbd *sdp)
>         if (IS_ERR(inode)) {
>                 fs_warn(sdp, "Reprocessing of jid %d failed with %ld.\n",
>                         sdp->sd_lockstruct.ls_jid, PTR_ERR(inode));
> +               /*
> +                * We couldn't get a replacement inode for our journal but we
> +                * evicted the old one. So dequeue it from the journals queue,
> +                * jindex_list, so that unmount doesn't do iput on it twice.
> +                */
> +               spin_lock(&sdp->sd_jindex_spin);
> +               list_del(&sdp->sd_jdesc->jd_list);
> +               sdp->sd_journals--;
> +               spin_unlock(&sdp->sd_jindex_spin);
> +               kfree(sdp->sd_jdesc);
> +               sdp->sd_jdesc = NULL;

Wouldn't it make more sense to set sdp->sd_jdesc->jd_inode to NULL
where we call iput() on that inode? An iput(NULL) is a no-op, so we'd
not need to change gfs2_jindex_free() to make that work.

Thanks,
Andreas

>                 goto skip_recovery;
>         }
>         sdp->sd_jdesc->jd_inode = inode;
> --
> 2.36.1
>