TAKE 981498 - Use xfs_idestroy() to cleanup an inode.

TAKE 981498 - Use xfs_idestroy() to cleanup an inode.

[Lachlan McIlroy]

Use xfs_idestroy() to cleanup an inode.

Date:  Wed Aug 20 13:32:01 AEST 2008
Workarea:  redback.melbourne.sgi.com:/home/lachlan/isms/2.6.x-inode
Inspected by:  
lachlan
david@fromorbit.com
Author:  lachlan

The following file(s) were checked into:
  longdrop.melbourne.sgi.com:/isms/linux/2.6.x-xfs-melb


Modid:  xfs-linux-melb:xfs-kern:31927a
fs/xfs/xfs_inode.c - 1.520 - changed
http://oss.sgi.com/cgi-bin/cvsweb.cgi/xfs-linux/xfs_inode.c.diff?r1=text&tr1=1.520&r2=text&tr2=1.519&f=h
	- Use xfs_idestroy() to cleanup an inode.

Re: TAKE 981498 - Use xfs_idestroy() to cleanup an inode.

[Dave Chinner]

On Wed, Aug 20, 2008 at 01:33:02PM +1000, Lachlan McIlroy wrote:
> Use xfs_idestroy() to cleanup an inode.

I'm not sure what has been checked in this patch, even though it's
recorded that I reviewed it. Yes, I reviewed the original patch, but
I asked for changes to be made. I *haven't reviewed* what got
checked in.

With the changelog being entirely useless - not even mentioning it
fixes a memory leak or a deadlock - I have no idea what changes were
made in response to my initial review.  I now have to wait for CVS
or git to be updated before being able to find out what just got
checked in so I can review it.

Lachlan (and for everyone else @ sgi), in future if someone in the
community reviews a patch and asks for change, can you please repost
the modified patches to close the review cycle before anything is
checked in?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: TAKE 981498 - Use xfs_idestroy() to cleanup an inode.

[Lachlan McIlroy]

This change was part of another patch that you reviewed.  This small
change got left out when I merged my changes in with your inode
allocation cleanup which you asked me to do.  I also had to modify
your original patch because it did not apply cleanly due other changes
that you made (the semaphore completion stuff).  I didn't have to take
your cleanup patch - I could have just fixed the bug.  So in amongst
your complaining I'll assume there is a covert "thanks for taking my
patch".


Dave Chinner wrote:
> On Wed, Aug 20, 2008 at 01:33:02PM +1000, Lachlan McIlroy wrote:
>> Use xfs_idestroy() to cleanup an inode.
> 
> I'm not sure what has been checked in this patch, even though it's
> recorded that I reviewed it. Yes, I reviewed the original patch, but
> I asked for changes to be made. I *haven't reviewed* what got
> checked in.
> 
> With the changelog being entirely useless - not even mentioning it
> fixes a memory leak or a deadlock - I have no idea what changes were
> made in response to my initial review.  I now have to wait for CVS
> or git to be updated before being able to find out what just got
> checked in so I can review it.
> 
> Lachlan (and for everyone else @ sgi), in future if someone in the
> community reviews a patch and asks for change, can you please repost
> the modified patches to close the review cycle before anything is
> checked in?
> 
> Cheers,
> 
> Dave.

Re: TAKE 981498 - Use xfs_idestroy() to cleanup an inode.

[Dave Chinner]

On Wed, Aug 20, 2008 at 04:13:22PM +1000, Lachlan McIlroy wrote:
> This change was part of another patch that you reviewed.  This small
> change got left out when I merged my changes in with your inode
> allocation cleanup which you asked me to do. 

Yes, I did ask for you to do that and I kinda expected to see the
result for review again after that. I did not review the changes
that were committed.

A second review would have caught the bug you introduced by
integrating the bug fix into my patch as I would have suggested that
you keep the enhancment and the bug fix as two separare commits.
Then the commit logs that would have a 'use init_once' commit and a
'deadlock + memory leak fix' commit....

> I also had to modify
> your original patch because it did not apply cleanly due other changes
> that you made (the semaphore completion stuff).

You could have asked for an updated patch when you found it
didn't apply. I had one ready to go and ended up posting
it twice before your commit...

> I didn't have to take
> your cleanup patch - I could have just fixed the bug.

Your choice, but either way I kind of expect some kind of dialogue
when changes are neceessary.  It only takes a few seconds to send a
'doesn't apply - can you update/going with original bugfix'
message.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com