question about xfs_alloc_fix_freelist()

question about xfs_alloc_fix_freelist()

[Michael Nishimoto]

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)?

   Michael



[[HTML alternate version deleted]]

Re: question about xfs_alloc_fix_freelist()

[David Chinner]

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.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: question about xfs_alloc_fix_freelist()

[Michael Nishimoto]

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.
> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> Principal Engineer
> SGI Australian Software Group

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?

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. :-)

    thanks,

        Michael

Re: question about xfs_alloc_fix_freelist()

[David Chinner]

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

Re: question about xfs_alloc_fix_freelist()

[Michael Nishimoto]

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
> 

Re: question about xfs_alloc_fix_freelist()

[David Chinner]

On Wed, May 07, 2008 at 12:16:16AM -0700, Michael Nishimoto wrote:
> 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:
.....
> > > > > 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?

Yes, it could, but remember that if the calling code is obeying the
rules then the AG will be considered ENOSPC before we try to touch
the AGFL. i.e. this case will never happen if the code is correct.

As it turns out, the recent fix I made for a long standing shutdown
at ENOSPC during inode create was a result of the inode creation
code not following the rules correctly - making it follows the rules
meant it the early checks in xfs_alloc_fix_freelist() prevented the
piece of code you pointed out dirtying the AGF/AGFL and then ENOSPCing
in the AG. See:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=75de2a91c98a6f486f261c1367fe59f5583e15a3

> Couldn't we end up affecting freelists in multiple AGs with the same
> transaction?

Yes and that is a bug if we don't actually end up allocating out of
those AGs.  However, in almost cases it is not a fatal bug and the
code reflects that....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group