On Wed, Nov 29, 2017 at 08:34:08AM +1100, Dave Chinner wrote: > 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. > Ok.. > 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. > This is clear to me now via the other thread. I'll incorporate both of these fixes, thanks. > > > > 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. > Nope.. I skimmed it and was confused about a hunk that was reworked in a way that conflicted with my understanding of the intent. It's really that simple. I appreciate the psychological evaluation, but I think "no that's not intentional, it's just a bug/incomplete" would have sufficed. ;) Brian > 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 -- 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
