Re: question about xfs_alloc_fix_freelist()

[Michael Nishimoto <miken@agami.com>]

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
>