Re: xfs_growfs_data_private memory leak

[Dave Chinner <david@fromorbit.com>]

On Tue, Jul 01, 2014 at 06:06:38PM +0300, Alex Lyakas wrote:
> Greetings,
> 
> It appears that if xfs_growfs_data_private fails during the "new AG
> headers" loop, it does not free all the per-AG structures for the
> new AGs. When XFS is unmounted later, they are not freed as well,
> because xfs_growfs_data_private did not update the "sb_agcount"
> field, so xfs_free_perag will not free them. This happens on 3.8.13,
> but looking at the latest master branch, it seems to have the same
> issue.
> 
> Code like [1] in xfs_growfs_data, seems to fix the issue.

Why not just do this in the appropriate error stack, like is
done inside xfs_initialize_perag() on error?

        for (i = oagcount; i < nagcount; i++) {
                pag = radix_tree_delete(&mp->m_perag_tree, index);
                kmem_free(pag);
        }

(though it might need RCU freeing)

When you have a fix, can you send a proper patch with a sign-off on
it?

> A follow-up question: if xfs_grows_data_private fails during the
> loop that updates all the secondary superblocks, what is the
> consequence? (I am aware that in the latest master branch, the loop
> is not broken on first error, but attempts to initialize whatever
> possible). When these secondary superblocks will get updated? Is
> there a way to force-update them? Otherwise, what can be the
> consequence of leaving them not updated?

The consequence is documented in mainline tree - if we don't update
them all, then repair will do the wrong thing.  Repair requires a
majority iof identical secondaries to determine if the primary is
correct or out of date. The old behaviour of not updating after the
first error meant that the majority were old superblocks and so at
some time in the future repair could decide your filesystem is
smaller than it really is and hence truncate away the grown section
of the filesystem. i.e. trigger catastrophic, unrecoverable data
loss.

Hence it's far better to write every seconday we can than to leave
a majority in a bad state....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs