Re: [PATCH v3 13/13] xfs: add missing bmap cancel calls in error paths

[Christoph Hellwig <hch@infradead.org>]

On Mon, Aug 10, 2015 at 03:01:49PM -0400, Brian Foster wrote:
> If a failure occurs after the bmap free list is populated and before
> xfs_bmap_finish() completes successfully (which returns a partial list
> on failure), the bmap free list must be cancelled. Otherwise, the extent
> items on the list are never freed and a memory leak occurs.
> 
> Several random error paths throughout the code suffer this problem. Fix
> these up such that xfs_bmap_cancel() is always called on error.

Looks sensible.  I wonder if we should attach the freelist to the
transaction and make xfs_trans_commit call into bmap_finish and
xfs_trans_cancel into xfs_bmap_cancel in the longer run?

> +STATIC int
>  xfs_growfs_rt_alloc(
> -	xfs_mount_t	*mp,		/* file system mount point */
> -	xfs_extlen_t	oblocks,	/* old count of blocks */
> -	xfs_extlen_t	nblocks,	/* new count of blocks */
> -	xfs_inode_t	*ip)		/* inode (bitmap/summary) */
> +	struct xfs_mount	*mp,		/* file system mount point */
> +	xfs_extlen_t		oblocks,	/* old count of blocks */
> +	xfs_extlen_t		nblocks,	/* new count of blocks */
> +	struct xfs_inode	*ip)		/* inode (bitmap/summary) */
>  {
> -	xfs_fileoff_t	bno;		/* block number in file */
> -	xfs_buf_t	*bp;		/* temporary buffer for zeroing */
> -	int		committed;	/* transaction committed flag */
> -	xfs_daddr_t	d;		/* disk block address */
> -	int		error;		/* error return value */
> -	xfs_fsblock_t	firstblock;	/* first block allocated in xaction */
> -	xfs_bmap_free_t	flist;		/* list of freed blocks */
> -	xfs_fsblock_t	fsbno;		/* filesystem block for bno */
> -	xfs_bmbt_irec_t	map;		/* block map output */
> -	int		nmap;		/* number of block maps */
> -	int		resblks;	/* space reservation */
> +	xfs_fileoff_t		bno;		/* block number in file */
> +	struct xfs_buf		*bp;	/* temporary buffer for zeroing */
> +	int			committed;	/* transaction committed flag */
> +	xfs_daddr_t		d;		/* disk block address */
> +	int			error;		/* error return value */
> +	xfs_fsblock_t		firstblock;/* first block allocated in xaction */
> +	struct xfs_bmap_free	flist;		/* list of freed blocks */
> +	xfs_fsblock_t		fsbno;		/* filesystem block for bno */
> +	struct xfs_bmbt_irec	map;		/* block map output */
> +	int			nmap;		/* number of block maps */
> +	int			resblks;	/* space reservation */
> +	struct xfs_trans	*tp;

Do we really need all this unrelated reformatting?

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