xfs_growfs_data_private memory leak

[Alex Lyakas]

Hi Eric,
What I was trying to achieve is to allow aborting XFS resize in the middle. 
I am not sure whether a typical user needs this feature, but for my use case 
it is needed.

So XFS resize is performed in two steps:
1) Initialize the newly-added AGs (AGFL, BNO btree, CNT btree etc)
2) Commit this by updating the primary superblock
3) Update all the existing secondary superblocks, and initialize all the 
newly-added secondary superblocks

So I added a custom IOCTL, which can tell this sequence to abort gracefully 
(just checking a flag on each loop iteration).

Aborting during step 1 works perfectly (apart from the memory leak that I 
discovered and fixed). Since the primary superblock has not been updated, 
XFS doesn't know about the newly-added AGs, so it doesn't care what's 
written there. Resize can be restarted, and XFS will redo this step from the 
beginning.
Aborting during step 3 can leave some of the secondary superblocks totally 
uninitialized. However, from XFS standpoint, the resize has been committed 
already. That's where my question came from. I realize that kernel can crash 
during step 3, leading to same results as with my code that aborts this 
step.

Anyways, since I was not sure about these secondary superblocks, I did not 
add an exit point at this step. (Again, realize that we can crash during 
this step anyways).

Thanks,
Alex.



-----Original Message----- 
From: Eric Sandeen
Sent: 04 August, 2014 9:15 PM
To: Alex Lyakas ; Dave Chinner
Cc: xfs at oss.sgi.com
Subject: Re: xfs_growfs_data_private memory leak

On 7/2/14, 7:27 AM, Alex Lyakas wrote:
> Hi Dave,
> Thank you for your comments.
>
> I realize that secondary superblocks are needed mostly for repairing

s/mostly/only/

> a broken filesystem. However, I don't see that they get updated
> regularly, i.e., during normal operation they don't seem to get
> updated at all. I put a print in xfs_sb_write_verify, and it gets
> called only with: bp->b_bn==XFS_SB_DADDR.

See the comments above verify_sb(): not everything is validated, so
not everything needs to be constantly updated.  It's just the basic
fs geometry, not counters, etc.

/*
* verify a superblock -- does not verify root inode #
*      can only check that geometry info is internally
*      consistent.  because of growfs, that's no guarantee
*      of correctness (e.g. geometry may have changed)
*
* fields verified or consistency checked:
*
*                      sb_magicnum
*
*                      sb_versionnum
*
*                      sb_inprogress
*
*                      sb_blocksize    (as a group)
*                      sb_blocklog
*
* geometry info -      sb_dblocks      (as a group)
*                      sb_agcount
*                      sb_agblocks
*                      sb_agblklog
*
* inode info -         sb_inodesize    (x-checked with geo info)
*                      sb_inopblock
*
* sector size info -
*                      sb_sectsize
*                      sb_sectlog
*                      sb_logsectsize
*                      sb_logsectlog
*
* not checked here -
*                      sb_rootino
*                      sb_fname
*                      sb_fpack
*                      sb_logstart
*                      sb_uuid
*
*                      ALL real-time fields
*                      final 4 summary counters
*/


> So do I understand correctly (also from comments in
> xfs_growfs_data_private), that it is safe to operate a filesystem
> while having broken secondary superblocks? For me, it appears to
> mount properly, and all the data seems to be there, but xfs_check
> complains like:

> bad sb magic # 0xc2a4baf2 in ag 6144
> bad sb version # 0x4b5d in ag 6144
> blocks 6144/65536..2192631388 out of range
> blocks 6144/65536..2192631388 claimed by block 6144/0
> bad sb magic # 0xb20f3079 in ag 6145
> bad sb version # 0x6505 in ag 6145
> blocks 6145/65536..3530010017 out of range
> blocks 6145/65536..3530010017 claimed by block 6145/0
> ...

some of that looks more serious than "just" bad backup sb's.
But the bad secondaries shouldn't cause runtime problems AFAIK.

> Also, if secondary superblocks do not get updated regularly, and
> there is no way to ask an operational XFS to update them, then during
> repair we may not find a good secondary superblock.

You seem to have 6144 (!) allocation groups; one would hope that a
majority of those supers would be "good" and the others will be
properly corrected by an xfs_repair.

> As for the patch, I cannot post a patch against the upstream kernel,
> because I am running an older kernel. Unfortunately, I cannot qualify
> an upstream patch properly in a reasonable time. Is there a value in
> posting a patch against 3.8.13? Otherwise, it's fine by me if
> somebody else posts it and takes the credit.

If the patch applies cleanly to both kernels, probably fine to
go ahead and post it, with that caveat.

-Eric

> Thanks,
> Alex.
>
>
>
> -----Original Message----- From: Dave Chinner
> Sent: 02 July, 2014 12:56 AM
> To: Alex Lyakas
> Cc: xfs at oss.sgi.com
> Subject: Re: xfs_growfs_data_private memory leak
>
> 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.