Re: [dm-devel] [PATCH] dm-snap-persistent-fix-dtr-cleanup.patch

[Jonathan Brassow <jbrassow redhat com>]

On Mar 19, 2009, at 5:42 PM, Alasdair G Kergon wrote:

> On Wed, Mar 04, 2009 at 03:33:56PM -0600, Jon Brassow wrote:
> > @@ -481,10 +484,17 @@ static void persistent_dtr(struct dm_exc
> > {
> > 	struct pstore *ps = get_info(store);
> >
> > -	destroy_workqueue(ps->metadata_wq);
> > -	dm_io_client_destroy(ps->io_client);
> > -	vfree(ps->callbacks);
> > +	/* Created in read_header */
> > +	if (ps->io_client)
> > +		dm_io_client_destroy(ps->io_client);
> > 	free_area(ps);
> > +
> > +	/* Allocated in persistent_read_metadata */
> > +	if (ps->callbacks)
> > +		vfree(ps->callbacks);
> > +
> > +	/* Don't need to check these, because they are done in ctr */
> > +	destroy_workqueue(ps->metadata_wq);
> > 	kfree(ps);
> > }
>
> I presume it's safe to reorder those operations as the workqueue is
> guaranteed to be empty?  It needs an inline comment though.
> (Or can we just keep the original, logical, order?)

The patch puts them in the order they /should/ have been in in the  
first place; that is, the reverse order in which they are created.   
The inline comments I put in state where/when those items are created,  
and hence why they need checks before being freed.  Perhaps you are  
suggesting the comments could be better. (?)

 brassow