Re: question about xfs_alloc_fix_freelist()

[David Chinner <dgc@xxxxxxx>]

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.

> 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