xfs
[Top] [All Lists]

Re: [PATCH 2/5] xfs: recovery of XLOG_UNMOUNT_TRANS leaks memory

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/5] xfs: recovery of XLOG_UNMOUNT_TRANS leaks memory
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 26 Sep 2014 05:01:04 -0700
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1411697952-24741-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1411697952-24741-1-git-send-email-david@xxxxxxxxxxxxx> <1411697952-24741-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Sep 26, 2014 at 12:19:09PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The XLOG_UNMOUNT_TRANS case skips the transaction, despite the fact
> an unmount record is always in a standalone transaction. Hence
> whenever we come across one of these we need to free the transaction
> structure associated with it as there is no commit record that
> follows it.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

Looks good,

Reviewed-by: Christoph Hellwig <hch@xxxxxx>

> @@ -3600,8 +3605,10 @@ xlog_recover_ophdr_to_trans(
>        * on this opheader is allocate a new recovery container to hold
>        * the recovery ops that will follow.
>        */
> -     if (ohead->oh_flags & XLOG_START_TRANS)
> +     if (ohead->oh_flags & XLOG_START_TRANS) {
> +             ASSERT(be32_to_cpu(ohead->oh_len) == 0);
>               xlog_recover_new_tid(rhp, tid, be64_to_cpu(rhead->h_lsn));
> +     }
>       return NULL;

.. but I suspect this hunk fits better into the previous patch.
Also shouldn't we handle any sort of on disk corruption more gracefully?

<Prev in Thread] Current Thread [Next in Thread>