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
|