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

[Brian Foster <bfoster@redhat.com>]

On Wed, Aug 12, 2015 at 12:21:09AM -0700, Christoph Hellwig wrote:
> 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?
> 

Perhaps... that's an interesting thought. I guess we'd also need to
consider other transaction operations (i.e., xfs_trans_roll()) to
support patterns such as that in xfs_attr_set(), for example. That one
also joins the inode between handling of the free list and transaction
commit, so that would need to be pushed down one way or another.

I wonder if there are enough vanilla bmap_finish()->trans_commit()
callers (e.g., without any intermediate code) to justify creation of a
new higher level xfs_trans_bmap_finish_commit() helper or some such that
handles both (retaining the existing interfaces for everything else).

> > +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?

No, I've just been conditioned at this point to try and fix up any of
the typedefs that I notice in functions I touch so we can eventually
kill them. This was RT code, so I suspect isn't touched often and ended
up larger than typical.

Brian

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