Re: [PATCH 4/5] [PATCH] xfs: simplify buffer to transaction matching

[Dave Chinner <david@fromorbit.com>]

On Sat, May 01, 2010 at 08:58:39AM -0400, Christoph Hellwig wrote:
> On Tue, Apr 20, 2010 at 04:41:55PM +1000, Dave Chinner wrote:
> > Good start, but I think that it should use xfs_trans_first_item()
> > and xfs_trans_next_item() rather than walking the descriptor
> > table directly.
> 
> I tried implementing it, but it doesn't work.  We can call the buffer
> matching routines on transactions that don't have any item linked to
> it, which will cause xfs_trans_first_item to panic.  Compare this code
> in xfs_trans_buf_item_match:
> 
> 	for (licp = &tp->t_items; licp != NULL; licp = licp->lic_next) {
> 		if (xfs_lic_are_all_free(licp)) {
> 			ASSERT(licp == &tp->t_items);
> 			ASSERT(licp->lic_next == NULL);
> 			return NULL;
> 		}
> 
> 		...
> 	}
> 
> to this in xfs_trans_first_item:
> 
> 	licp = &tp->t_items;
>         /*
> 	 * If it's not in the first chunk, skip to the second.
> 	 */
> 	if (xfs_lic_are_all_free(licp)) {
> 		licp = licp->lic_next;
> 	}
> 
> 	/*
> 	 * Return the first non-free descriptor in the chunk.
> 	 */
> 	ASSERT(!xfs_lic_are_all_free(licp));

Is there any reason why xfs_trans_first_item() should panic if it
doesn't find a log item? I can't think of one that can't be handled
by returning NULL and the caller doing ASSERT(lidp)? The callers:

	xfs_trans_count_vecs - has assert, triggers shutdown
	xfs_trans_fill_vecs - has assert, handles null return
	xfs_trans_committed - handles null return
	xfs_trans_uncommit - handles null return

So it seems like we could make xfs_trans_first_item() not assert
fail on debug kernels.

The other thing that concerns me is that we have two different
definitions of "empty transactions" in these two functions. It seems
to me that xfs_trans_first_item() is the correct one - if we remove
items from the transaction there is no guarantee that the embedded
chunk in the transaction contains any active descriptors. Hence I
think the code in xfs_trans_buf_item_match[_all]() is actually
buggy...

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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