xfs
[Top] [All Lists]

Re: [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 6 Dec 2013 07:59:10 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131205155951.679310054@xxxxxxxxxxxxxxxxxxxxxx>
References: <20131205155830.620826868@xxxxxxxxxxxxxxxxxxxxxx> <20131205155951.679310054@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Dec 05, 2013 at 07:58:34AM -0800, Christoph Hellwig wrote:
> We might not have read in the extent list at this point, so make sure we
> take the ilock exclusively if we have to do so.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index b861270..5343034 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -164,6 +164,7 @@ xfs_attr_get(
>  {
>       int             error;
>       struct xfs_name xname;
> +     uint            lock_mode;
>  
>       XFS_STATS_INC(xs_attr_get);
>  
> @@ -174,9 +175,9 @@ xfs_attr_get(
>       if (error)
>               return error;
>  
> -     xfs_ilock(ip, XFS_ILOCK_SHARED);
> +     lock_mode = xfs_ilock_map_shared(ip);
>       error = xfs_attr_get_int(ip, &xname, value, valuelenp, flags);
> -     xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +     xfs_iunlock_map_shared(ip, lock_mode);
>       return(error);
>  }

I think the locking here should be moved inside xfs_attr_get_int()
so that it uses the same locking pattern as xfs_attr_set() and
xfs_attr_remove().

Also, xfs_attr_list() needs this treatment (the attr version of
readdir) as well (and it has the locking inside xfs_attr_list_int(),
too ;).

It looks like xfs_readlink needs fixing, too.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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