David Chinner wrote:
On Tue, May 06, 2008 at 02:21:57PM -0700, Michael Nishimoto wrote:
> David Chinner wrote:
> >On Fri, May 02, 2008 at 02:01:47PM -0700, Michael Nishimoto wrote:
> > >
> > > The following code can be found near the end of
xfs_alloc_fix_freelist:
> > >
> > > if (targs.agbno == NULLAGBLOCK) {
> > > if (flags & XFS_ALLOC_FLAG_FREEING)
> > > break;
> > > xfs_trans_brelse(tp, agflbp);
> > > args->agbp = NULL;
> > > return 0;
> > > }
> > >
> > > Don't we need to release agbp too by calling xfs_trans_brelse(tp,
agbp)?
> >
> >I don't think so. AFAICT, The agbp (agf block) is linked into the
> >transaction and by this point we may have modified the AGF (think
> >multiple iterations of the loop to fill the free list). Given that
> >it may be modified, we shouldn't release it here but instead allow
> >the transaction commit/abort to do that for us at the appropriate
> >time.
>
> I understood that the transaction abort code will free resources/locks
> when passing back an error. However, by returning args->agbp = NULL,
> aren't we telling the calling code that we have not satisfied the request
> in the current AG?
Yes.
If this happens and the calling function decides to choose another AG, is
there a possibility of using up the entire transaction log reservation?
Couldn't we end up affecting freelists in multiple AGs with the same
transaction?
> I do see the part of about the AGF possibly getting modified.
>
> It would help to have a clear comment description of possible inputs vs.
> expected outputs for this function. :-)
Like a lot of code, not just XFS. :/\
Cheers,
Dave.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
|