Re: tr_ifree transaction log reservation calculation

[Brian Foster <bfoster@redhat.com>]

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@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html