Re: [PATCH 2/2] Btrfs: fix memory leak on extent map after fsync

[Liu Bo <bo.li.liu@oracle.com>]

On Thu, Jan 24, 2013 at 11:44:33AM -0500, Josef Bacik wrote:
> On Tue, Jan 08, 2013 at 07:49:21AM -0700, Liu Bo wrote:
> > During fsync, we put the changed parts(i.e. extent map) into the log tree,
> > and we ship these parts from a list of modified_extents to a local list
> > to process, of course, we must increment the refs of the extent maps to
> > avoid it from getting evicted from cache.
> > 
> > The problem is
> > we don't hold the tree writer lock all the time of iterating the local list,
> > and it is possible that other threads hack in and delete the extent map from
> > the local list silently.  So we'll end up with memory leak here.
> > 
> > I hit this when testing xfstest 274 with mount options 'autodefrag,compress=zlib'.
> > 
> > With this fix, the memory leak has gone away.
> 
> This isn't going to work, we use the LOGGING flag to make sure the em isn't
> merged as well.  Thanks,

A quick grep shows,

   1     16  fs/btrfs/extent_map.h <<GLOBAL>>
             #define EXTENT_FLAG_LOGGING 4
   2    406  fs/btrfs/extent_map.c <<remove_extent_mapping>>
             if (!test_bit(EXTENT_FLAG_LOGGING, &em->flags))
   3   3403  fs/btrfs/tree-log.c <<btrfs_log_changed_extents>>
             set_bit(EXTENT_FLAG_LOGGING, &em->flags);
   4   3413  fs/btrfs/tree-log.c <<btrfs_log_changed_extents>>
             clear_bit(EXTENT_FLAG_LOGGING, &em->flags);

how does the flag avoid merging em?

Seems we lost the check.

static int mergable_maps(struct extent_map *prev, struct extent_map *next)       
{                                                                                
        if (test_bit(EXTENT_FLAG_PINNED, &prev->flags))                          
                return 0;                                                        
                                                                                 
        /*                                                                       
         * don't merge compressed extents, we need to know their                 
         * actual size                                                           
         */                                                                      
        if (test_bit(EXTENT_FLAG_COMPRESSED, &prev->flags))                      
                return 0;                                                        

	...
}
                                                                                 
thanks,
liubo