On Tue, Nov 28, 2017 at 08:28:02AM -0500, Brian Foster wrote: > On Tue, Nov 28, 2017 at 08:29:46AM +1100, Dave Chinner wrote: > > On Mon, Nov 27, 2017 at 01:46:51PM -0500, Brian Foster wrote: > > > Also, it looks like this makes some other changes that are not > > > immediately clear to me, > > > > Which ones? > > > > The truncate and iunlink bits... The truncate reservation includes a reservation for an inobt modification. We *never* modify the inobt inside a truncate transaction, so even if we ignore the fact it was wrong (like all the others we are fixing), it really shouldn't be there. THe commit message from the archive tree doesn't help explain it, either. It just says (paraphrasing) "fix transaction reservation". So I removed it. As for the iunlink bit, you mean this the fact I added the on disk inode cluster to the reservation in xfs_calc_iunlink_add_reservation()? That's because we log the inode cluster in unlink when we modify the inode unlinked list. That's missing from all the unlinked inode manipulation reservations - some of them take into account modifying the inode cluster of another inode in the unlinked list (e.g. ifree) but they all fail to reserve space for logging the unlinked list pointer in the inode being unlinked/freed. > > > squashes in a couple of the fixes I've already > > > made and doesn't actually add a new allocfree res in some of the > > > create/alloc calculations (but rather refactors the existing allocfree > > > res to use the new helper). It's not clear to me if the latter is > > > intentional..? > > > > Not sure what you are asking? I factored existing open-coded inobt > > reservations to use helpers so that I didn't have to modify the > > reservation in lots of places. Get it right in one place, all the > > others are correct too. Indeed, most of the cases I factored already > > had the allocfree reservation, so I'm not sure why we'd be adding a > > new one to those reservations? > > > > Understood wrt to the helpers. What I'm asking is why > xfs_calc_create_resv_alloc(), for example, doesn't actually add another > allocfree res if the intent was to add one for all inode tree related > ops..? Because the patch wasn't complete and finished and there's no guarantee I factored it correctly. You're still reading that patch as though it was a completely polished, finished patch, not a set of "noticed these things while thinking about the problem" notes. I factored the code first, never went back the create code because we were focussed on the problems in the iunlink reservations.... > Using that example, my understanding is that tx should now have > an allocfree res for the inode chunk allocation and a new one for the > inobt insertion. The latter is the one that's now abstracted into the > helpers, yes..? That's the model I tried to follow for the rfc patch, > anyways. Sure, that's what needs to be done, as we've discussed. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
