Re: [PATCH 04/11] xfs: ensure EFD trans aborts on log recovery extent free failure

[Brian Foster <bfoster@redhat.com>]

On Mon, Aug 10, 2015 at 08:38:04AM -0400, Brian Foster wrote:
> On Sun, Aug 09, 2015 at 12:53:11AM -0700, Christoph Hellwig wrote:
> > > +		/*
> > > +		 * Log the EFD before error handling to ensure the EFD aborts
> > > +		 * (and releases the EFI) on error.
> > > +		 */
> > >  		error = xfs_free_extent(tp, extp->ext_start, extp->ext_len);
> > >  		xfs_trans_log_efd_extent(tp, efdp, extp->ext_start,
> > >  					 extp->ext_len);
> > 
> > Given that we now always need to log the extents in the EFD even on
> > error maybe it's time to move the call to xfs_free_extent into
> > xfs_trans_log_efd_extent and rename it to xfs_trans_free_extent?
> > 
> > Or even better convert xfs_bmap_finish to also walk the extents in the
> > EFI instead of xfs_bmap_free list and have a xfs_trans_free_extents helper
> > ala:
> > 
> 
> Good idea, I'll incorporate something like this.
> 
> Brian
> 

After taking a closer look at this, I think I'll go with the first idea
to push down xfs_free_extent(). The code below to iterate based on the
EFI, while a nice cleanup, complicates handling the free list on error.
E.g., we currently remove each bmap_free_item as it is successfully
processed so the resulting list is consistent with the items that were
not processed before the error.

It might not matter functionally at the moment since the caller probably
cancels the free list, but I consider that a landmine should we decide
to do something more with the unprocessed entries down the road.

Brian

> > int
> > xfs_trans_free_extents(
> > 	struct xfs_trans	*tp,
> > 	struct xfs_efi_log_item	*efip)
> > {
> > 	struct xfs_efd_log_item	*efdp;
> > 	int			error = 0, i;
> > 
> > 	efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
> > 	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
> > 		struct xfs_extent *extp = &efip->efi_format.efi_extents[i];
> > 
> > 		error = xfs_free_extent(tp, extp->ext_start, extp->ext_len);
> > 		xfs_trans_log_efd_extent(tp, efdp, extp->ext_start,
> > 					extp->ext_len);
> > 
> > 		if (error)
> > 			break;
> > 	}
> > 
> > 	return error;
> > }
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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