Updated patch attached,
Moved case where there is no transaction back into
xfs_bmap_add_attrfork() and rely on caller to call xfs_trans_roll(),

Christoph Hellwig <hch@infradead.org> writes:
>> +xfs_bmap_add_attrfork(
[...]
> Care to add this below xfs_trans_bmap_add_attrfork?  Also please
> us struct xfs_inode instead of the typedef.

Done,

>> +	if (tpp) {
>> +		ASSERT(*tpp);
>> +		tp = *tpp;
>> +	} else {
[...]
>
> I think it would be much cleaner if all the transaction setup, joining
> etc was done in xfs_bmap_add_attrfork, and xfs_trans_bmap_add_attrfork
> just gets an always non-NULL xfs_trans pointer.  That would mean
> the common code would become just a helper, and
> xfs_trans_bmap_add_attrfork would be a small wrapper including the 
> xfs_trans_roll.  Alternatively the caller could always do the roll.

I think I got it right, but error handeling is a bit weird this way,
looks logically identical.