xfs
[Top] [All Lists]

RE: [PATCH 4/6] XFS: convert remaining direct references to m_perag

To: "Dave Chinner" <david@xxxxxxxxxxxxx>
Subject: RE: [PATCH 4/6] XFS: convert remaining direct references to m_perag
From: "Alex Elder" <aelder@xxxxxxx>
Date: Wed, 23 Dec 2009 14:49:54 -0600
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <1260857477-2368-5-git-send-email-david@xxxxxxxxxxxxx>
Thread-index: Acp9UAYwE6rT5iuWRAStPKEsaObwLAFyk6Qw
Thread-topic: [PATCH 4/6] XFS: convert remaining direct references to m_perag
Dave Chinner wrote:
> Convert the remaining direct lookups of the per ag structures
> to use get/put accesses. Ensure that the loops across AGs and
> prior users of the interface balance gets and puts correctly.

Generally looks good, but I have a question below.

Also, it looks to me like you missed adding a matching
xfs_perag_put() call in xfs_alloc_get_freelist().  I
have otherwise satisfied myself that all the get/put
calls are all matched up.  Furthermore, the only real
references to the m_perag field in the mount structure
is via xfs_perag_get().

                                        -Alex

> Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>
> ---
>  fs/xfs/xfs_bmap.c   |    8 +++++++-
>  fs/xfs/xfs_ialloc.c |   35 +++++++++++++++++++++++++----------
>  fs/xfs/xfs_inode.c  |    5 ++++-
>  fs/xfs/xfs_mount.c  |    9 ++++++---
>  4 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index 98251cd..a9b95d9 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
. . .
> @@ -2667,6 +2668,7 @@ xfs_bmap_btalloc(
>                                               break;
> 
>                                       error = xfs_filestream_new_ag(ap, &ag);
> +                                     xfs_perag_put(pag);

Is there a reason you do the put operation *after* the call to
xfs_filestream_new_ag()?  I only ask because I had a similar
question (below) where xfs_ialloc_read_agi() was called, without
passing pag, but before calling xfs_perag_put() and I wondered
whether there was an implied requirement there.  If there is,
I'd like to see that made more explicit.  If not, I prefer to
see the put call done as early as possible.

>                                       if (error) {
>                                               up_read(&mp->m_peraglock);
>                                               return error;
. . .
> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
> index cb907ba..884ee13 100644
> --- a/fs/xfs/xfs_ialloc.c
> +++ b/fs/xfs/xfs_ialloc.c
. . .
> @@ -772,11 +778,14 @@ nextag:
>                       return noroom ? ENOSPC : 0;
>               }
>               down_read(&mp->m_peraglock);
> -             if (mp->m_perag[tagno].pagi_inodeok == 0) {
> +             pag = xfs_perag_get(mp, tagno);
> +             if (pag->pagi_inodeok == 0) {
> +                     xfs_perag_put(pag);
>                       up_read(&mp->m_peraglock);
>                       goto nextag;
>               }
>               error = xfs_ialloc_read_agi(mp, tp, tagno, &agbp);
> +             xfs_perag_put(pag);

Here is the spot I wondered about--whether the pag needed to
be held across the call to xfs_ialloc_read_agi().  If so,
it isn't obvious from looking directly at this spot in the
code so it should maybe be spelled out more clearly in a
comment.

>               up_read(&mp->m_peraglock);
>               if (error)
>                       goto nextag;
. . .

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