xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/5] xfs: recovery of XLOG_UNMOUNT_TRANS leaks memory
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Sat, 27 Sep 2014 09:27:45 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140926120104.GA10574@xxxxxxxxxxxxx>
References: <1411697952-24741-1-git-send-email-david@xxxxxxxxxxxxx> <1411697952-24741-3-git-send-email-david@xxxxxxxxxxxxx> <20140926120104.GA10574@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Sep 26, 2014 at 05:01:04AM -0700, Christoph Hellwig wrote:
> 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.

Folded it into the first patch.

> Also shouldn't we handle any sort of on disk corruption more gracefully?

Yes, we should. However, the recovery code is so full of this sort
of ASSERT() checking that graceful handling of errors is fairly
significant piece of work. It's already on my "cleanup work for a
rainy day" list. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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