xfs
[Top] [All Lists]

Re: [PATCH 1/4] xfs: kill XBF_FS_MANAGED buffers

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: kill XBF_FS_MANAGED buffers
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 8 Sep 2010 21:33:13 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1283958778-28610-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1283958778-28610-1-git-send-email-david@xxxxxxxxxxxxx> <1283958778-28610-2-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-08-17)
Looks good, but a few comments below:

> +     bp = xfs_buf_get_noaddr(sector_size, mp->m_ddev_targp);
> +
>       if (!bp || XFS_BUF_ISERROR(bp)) {

xfs_buf_get_noaddr will never return a buffer with an error set.

> -     ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
> +
> +     /* set up the buffer for a read IO */
> +     xfs_buf_lock(bp);
> +     XFS_BUF_SET_ADDR(bp, XFS_SB_DADDR);
> +        XFS_BUF_READ(bp);
> +        XFS_BUF_BUSY(bp);

Various indentation problems.

> +     /* grab a reference for caching the buffer */
> +        XFS_BUF_HOLD(bp);
>       mp->m_sb_bp = bp;
> +
>       xfs_buf_relse(bp);

Grabbing the reference just to drop it three lines later is rather
pointless, just remove both.

>       ASSERT(XFS_BUF_VALUSEMA(bp) > 0);

Given that we took the lock a few lines above this one also feels rather
poinless.

> +fail:
> +     if (bp)
>               xfs_buf_relse(bp);
> -     }
>       return error;

I'd rather see this split into a fail_buf_relese label that puts the
buffer, and a fail label that just returns the error.

>        * when we call xfs_buf_relse().
>        */
>       bp = xfs_getsb(mp, 0);
> -     XFS_BUF_UNMANAGE(bp);
> -     xfs_buf_relse(bp);
>       mp->m_sb_bp = NULL;
> +
> +     /*
> +      * need to release the buffer twice to free it because we hold an extra
> +      * reference count on it.
> +      */
> +     xfs_buf_relse(bp);
> +     xfs_buf_relse(bp);

I'd rather rewrite xfs_freesb to not use xfs_getsb and thus avoid taking
the superflous reference:

void
xfs_freesb(
        struct xfs_mount        *mp);

        struct xfs_buf          *bp = mp->m_sb_bp;

        mp->m_sb_bp = NULL;
        if (xfs_buf_cond_lock(bp)
                BUG();
        xfs_buf_relse(bp);
}

<Prev in Thread] Current Thread [Next in Thread>