xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfs: clear inode per-lifetime state when recycling it

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: clear inode per-lifetime state when recycling it
From: Alex Elder <aelder@xxxxxxx>
Date: Tue, 31 May 2011 17:19:19 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <1306815659-23346-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1306815659-23346-1-git-send-email-david@xxxxxxxxxxxxx> <1306815659-23346-2-git-send-email-david@xxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Tue, 2011-05-31 at 14:20 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> XFs inodes have several per-lifetime state flags that determine heaviour of 
> the
> inode. These state flags are not reset when an inode is reallocated and reused
> from the reclaimable state.
> 
> This can lead to specualtive preallocation not being truncated away in the
> expected manner for local files until the inode is subsequently truncated,
> freed or cycles out of the cache. It can also lead to an inode being 
> considered
> to be a filestream inode or having been truncated when that is not the case.
> 
> Clear the state flags when the inode is recycled to avoid these problems.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

Other than simple typo's, this looks good to me.

Comment below.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> ---
>  fs/xfs/xfs_iget.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
> index cb9b6d1..36467f1 100644
> --- a/fs/xfs/xfs_iget.c
> +++ b/fs/xfs/xfs_iget.c
> @@ -262,7 +262,17 @@ xfs_iget_cache_hit(
>  
>               spin_lock(&pag->pag_ici_lock);
>               spin_lock(&ip->i_flags_lock);
> -             ip->i_flags &= ~(XFS_IRECLAIMABLE | XFS_IRECLAIM);
> +
> +             /*
> +              * Clear the relcaim and per-lifetime state flags as we are now

                              reclaim

> +              * effectively a new inode and so we need to reset to the
> +              * initial state.
> +              *
> +              * XXX(dgc): should the XFS_ISTALE flag only be cleared here?
> +              */
> +             ip->i_flags &= ~(XFS_IRECLAIMABLE | XFS_IRECLAIM |
> +                              XFS_IDIRTY_RELEASE | XFS_ITRUNCATED |
> +                              XFS_IFILESTREAM);
>               ip->i_flags |= XFS_INEW;

If you clear XFS_ISTALE here you could simply re-phrase the
above comment to emphasize the "reset to initial state" and
change it to a simple assignment:

                ip->i_flags = XFS_INEW;

(Though that makes clearing the rest of the flags
implicit, which I think is undesirable from the point
of view of code searching.  XFS_IDIRTY_RELEASE is
already cleared only implicitly.)

Otherwise I was going to suggest you define a symbol
representing the subset of XFS inode flags that are
per-lifetime state flags, so if another one gets
invented along the way it could get added to the set.


>               __xfs_inode_clear_reclaim_tag(mp, pag, ip);
>               inode->i_state = I_NEW;



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