On Mon, Feb 06, 2012 at 09:51:54PM +0100, Jesper Juhl wrote: > On Mon, 6 Feb 2012, Raghavendra D Prabhu wrote: > > As far as second one is concerned, looks fine, though this one should also do > > the same. > > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c > > index ab30253..d331f5b 100644 > > --- a/fs/xfs/xfs_iops.c > > +++ b/fs/xfs/xfs_iops.c > > @@ -730,9 +730,9 @@ xfs_setattr_nonsize( > > return 0; > > > > out_trans_cancel: > > - xfs_trans_cancel(tp, 0); > > xfs_iunlock(ip, XFS_ILOCK_EXCL); > > out_dqrele: > > + xfs_trans_cancel(tp, 0); > > xfs_qm_dqrele(udqp); > > xfs_qm_dqrele(gdqp); > > return error; > > > > Thank you for the feedback. > > I worry about the fact that this suddenly calls xfs_trans_cancel() without > holding the lock. I don't know if that's actually significant though. You're right to worry about it, because it is significant. The transaction needs to be cancelled before we unlock the inode because the transaction cancel cleans up state on the inode if the inode has been joined to the transaction. Unlocking the inode before the transaction is cancelled means some other transaction can lock the inode and join it to a new transaction before the old one is cleaned up. Then Bad Stuff Happens. IOWs, the above change is not safe to make. > If it *is* significant, then I think the patch I just submitted in reply to > Dave Chinner is better since there we do the alloc and cancel before even > taking the lock at all in the leaky case and all other case have > identical behaviour as before. I'll go check it out. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
