On Tue, Jun 16, 2015 at 07:27:06AM -0400, Brian Foster wrote:
> On Tue, Jun 16, 2015 at 07:51:19AM +1000, Dave Chinner wrote:
> > On Mon, Jun 15, 2015 at 10:58:14AM -0400, Brian Foster wrote:
> > > On Wed, Jun 03, 2015 at 04:04:40PM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > > >
> > > > The error handling is currently an inconsistent mess as every error
> > > > condition handles return values and releasing buffers individually.
> > > > Clean this up by using gotos and a sane error label stack.
> > > >
> > > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > > > ---
> > > > fs/xfs/libxfs/xfs_alloc.c | 103
> > > > +++++++++++++++++++++-------------------------
> > > > 1 file changed, 47 insertions(+), 56 deletions(-)
> > > >
> > > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > > > index 2471cb5..352db46 100644
> > > > --- a/fs/xfs/libxfs/xfs_alloc.c
> > > > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > > > @@ -1909,80 +1909,65 @@ xfs_alloc_space_available(
> > > > */
> > > > STATIC int /* error */
> > > > xfs_alloc_fix_freelist(
> > > > - xfs_alloc_arg_t *args, /* allocation argument structure */
> > > > - int flags) /* XFS_ALLOC_FLAG_... */
> > > > + struct xfs_alloc_arg *args, /* allocation argument
> > > > structure */
> > > > + int flags) /* XFS_ALLOC_FLAG_... */
> > > > {
> > > > - xfs_buf_t *agbp; /* agf buffer pointer */
> > > > - xfs_buf_t *agflbp;/* agfl buffer pointer */
> > > > - xfs_agblock_t bno; /* freelist block */
> > > > - int error; /* error result code */
> > > > - xfs_mount_t *mp; /* file system mount point structure */
> > > > - xfs_extlen_t need; /* total blocks needed in freelist */
> > > > - xfs_perag_t *pag; /* per-ag information structure */
> > > > - xfs_alloc_arg_t targs; /* local allocation arguments */
> > > > - xfs_trans_t *tp; /* transaction pointer */
> > > > -
> > > > - mp = args->mp;
> > > > + struct xfs_mount *mp = args->mp;
> > > > + struct xfs_perag *pag = args->pag;
> > > > + struct xfs_trans *tp = args->tp;
> > > > + struct xfs_buf *agbp = NULL;
> > > > + struct xfs_buf *agflbp = NULL;
> > > > + struct xfs_alloc_arg targs; /* local allocation arguments */
> > > > + xfs_agblock_t bno; /* freelist block */
> > > > + xfs_extlen_t need; /* total blocks needed in
> > > > freelist */
> > > > + int error;
> > > >
> > > > - pag = args->pag;
> > > > - tp = args->tp;
> > > > if (!pag->pagf_init) {
> > > > - if ((error = xfs_alloc_read_agf(mp, tp, args->agno,
> > > > flags,
> > > > - &agbp)))
> > > > - return error;
> > > > + error = xfs_alloc_read_agf(mp, tp, args->agno, flags,
> > > > &agbp);
> > > > + if (error)
> > > > + goto out_no_agbp;
> > > > if (!pag->pagf_init) {
> > > > ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
> > > > ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> > > > - args->agbp = NULL;
> > > > - return 0;
> > > > + goto out_agbp_relse;
> > > > }
> > > > - } else
> > > > - agbp = NULL;
> > > > + }
> > > >
> > > > /*
> > > > - * If this is a metadata preferred pag and we are user data
> > > > - * then try somewhere else if we are not being asked to
> > > > - * try harder at this point
> > > > + * If this is a metadata preferred pag and we are user data
> > > > then try
> > > > + * somewhere else if we are not being asked to try harder at
> > > > this
> > > > + * point
> > > > */
> > > > if (pag->pagf_metadata && args->userdata &&
> > > > (flags & XFS_ALLOC_FLAG_TRYLOCK)) {
> > > > ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> > > > - args->agbp = NULL;
> > > > - return 0;
> > > > + goto out_agbp_relse;
> > > > }
> > > >
> > > > need = XFS_MIN_FREELIST_PAG(pag, mp);
> > > > - if (!xfs_alloc_space_available(args, need, flags)) {
> > > > - if (agbp)
> > > > - xfs_trans_brelse(tp, agbp);
> > > > - args->agbp = NULL;
> > > > - return 0;
> > > > - }
> > > > + if (!xfs_alloc_space_available(args, need, flags))
> > > > + goto out_agbp_relse;
> > > >
> > > > /*
> > > > * Get the a.g. freespace buffer.
> > > > * Can fail if we're not blocking on locks, and it's held.
> > > > */
> > > > - if (agbp == NULL) {
> > > > - if ((error = xfs_alloc_read_agf(mp, tp, args->agno,
> > > > flags,
> > > > - &agbp)))
> > > > - return error;
> > > > - if (agbp == NULL) {
> > > > + if (!agbp) {
> > > > + error = xfs_alloc_read_agf(mp, tp, args->agno, flags,
> > > > &agbp);
> > > > + if (error)
> > > > + goto out_no_agbp;
> > > > + if (!agbp) {
> > > > ASSERT(flags & XFS_ALLOC_FLAG_TRYLOCK);
> > > > ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
> > > > - args->agbp = NULL;
> > > > - return 0;
> > > > + goto out_no_agbp;
> > > > }
> > > > }
> > > >
> > > >
> > > > /* If there isn't enough total space or single-extent, reject
> > > > it. */
> > > > need = XFS_MIN_FREELIST_PAG(pag, mp);
> > > > - if (!xfs_alloc_space_available(args, need, flags)) {
> > > > - xfs_trans_brelse(tp, agbp);
> > > > - args->agbp = NULL;
> > > > - return 0;
> > > > - }
> > > > + if (!xfs_alloc_space_available(args, need, flags))
> > > > + goto out_agbp_relse;
> > > >
> > > > /*
> > > > * Make the freelist shorter if it's too long.
> > > > @@ -1997,10 +1982,10 @@ xfs_alloc_fix_freelist(
> > > >
> > > > error = xfs_alloc_get_freelist(tp, agbp, &bno, 0);
> > > > if (error)
> > > > - return error;
> > > > + goto out_agbp_relse;
> > >
> > > So at this point it looks like the buffer could be logged (i.e., dirty
> > > in the transaction). Perhaps this is the reason for the lack of agbp
> > > releases from this point forward in the current error handling. That
> > > said, we do the agflbp release unconditionally at the end of the
> > > function even when it might be logged. xfs_trans_brelse() appears to
> > > handle this case as it just skips removing the item from the tp.
> > >
> > > This is a bit confusing and at the very least seems like an unexpected
> > > use of xfs_trans_brelse(). Is this intentional?
> >
> > Yes, very much so. If the agf is clean, then releasing it
> > immediately on error in the function that grabbed the buffer is the
> > right thing to do. This can happen if the first call to
> > xfs_alloc_get_freelist() fails (which only occurs if we fail to read
> > the AGFL buffer).
> >
>
> Ok.
>
> > And, as you noticed, if it is dirty it will remain held by the
> > transaction until it is cancelled, as happens everywhere else in the
> > code. So this is effectively making the current code consistent with
> > the usual error handling patterns...
> >
>
> Effectively, yeah. My point was more that it doesn't seem to be the
> prevalent pattern. Case in point, this code prior to these changes, attr
> code doesn't seem to use brelse() after potential modification
> (xfs_attr_leaf_[add|remove]name()), inode allocation, etc. Anyways, it
> seems valid and is probably the more simple method for handling errors
> in this situation:
>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
>
> ... though I think a comment around it couldn't hurt.
No worries, I added this hunk:
/*
* Make the freelist shorter if it's too long.
*
+ * Note that from this point onwards, we will always release the agf and
+ * agfl buffers on error. This means that if we error out and the
+ * buffers are clean, they are correctly handled as they may not have
+ * been joined to the transaction and hence need to be released
+ * manually. If they have been joined to the transaction, then
+ * xfs_trans_brelse() will handle them according to the recursion count
+ * and dirty state of the buffer.
+ *
* XXX (dgc): When we have lots of free space, does this buy us
* anything other than extra overhead when we need to put more blocks
* back on the free list? Maybe we should only do this when space is
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
|