Re: [PATCH] xfs: re-organize XFS_ILOCK asserts in xfs_itruncate_extents()

[Carlos Maiolino <cmaiolino@redhat.com>]

On Mon, Jan 28, 2013 at 10:52:57AM -0600, Geoffrey Wehrman wrote:
> On Mon, Jan 28, 2013 at 11:48:30AM -0500, Carlos Maiolino wrote:
> | An logically OR'red assert for check an inode locked in XFS_ILOCK_EXCL and
> | XFS_IOLOCK_EXCL looks better than the old way, avoiding possible mistakes while
> | readin the code
> | 
> | Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> | ---
> |  fs/xfs/xfs_inode.c | 5 +++--
> |  1 file changed, 3 insertions(+), 2 deletions(-)
> | 
> | diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> | index 66282dc..f7efe77 100644
> | --- a/fs/xfs/xfs_inode.c
> | +++ b/fs/xfs/xfs_inode.c
> | @@ -1395,9 +1395,10 @@ xfs_itruncate_extents(
> |  	int			error = 0;
> |  	int			done = 0;
> |  
> | -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> | -	ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
> | +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL) ||
> |  	       xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> | +	ASSERT(!atomic_read(&VFS_I(ip)->i_count));
> | +	ASSERT(!atomic_read(&VFS_I(ip)->i_count));
> |  	ASSERT(new_size <= XFS_ISIZE(ip));
> |  	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> |  	ASSERT(ip->i_itemp != NULL);
> | -- 
> | 1.8.1
> 
> NACK.  You are changing the logic of the asserts.  The original first
> assert indicates that the ILOCK is always locked.  The modified asserts
> allow eith the ILOCK or the IOLOCK to be locked.  This is not correct.
> 
Yeah, makes sense, I thought about a possible logic change, but didn't realize
it was really true.

Thanks for feedback.

forget about this idea then, too much for little gain :)
> 
> Geoffrey

-- 
Carlos

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