Re: [PATCH 1/1] fiemap: move i_op->fiemap() locking to the ioctl_fiemap()

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Fri, 2012-09-21 at 17:14 +0300, Dmitry Kasatkin wrote:
> On some filesystems calling i_op->fiemap() takes the i_mutex,
> on others it doesn't. For example, on ext2/ext3 fiemap calls
> generic_block_fiemap(), which takes the i_mutex, but on ext4
> it doesn't call generic_block_fiemap() for inodes, that have no
> extents and does not take the i_mutex in other cases. Other i_op
> functions, like setxattr/setattr, rely on the caller to lock the
> i_mutex.
> 
> This patch moves the i_mutex locking from generic_block_fiemap()
> to ioctl_fiemap(), which is currently the only caller of i_op->fiemap().
> 
> This change is needed in preparation for EVM to include a hash of
> the extent data to be used in the HMAC calculation. EVM is called
> when the i_mutex has already been taken.
> 
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@intel.com>

I'm just wondering whether or not we need the i_mutex lock to be taken
at all in GFS2.... the reason that it was moved up a level was that we
require i_mutex to be taken before the glock (rule being local locking
before cluster locking).

However, we do hold a shared glock over the same portion of code and
that should be enough to prevent any changes in the on-disk extents for
the inode in question.

Since we have the new truncate sequence, the inode cannot be truncated
outside of the glock any more, so I don't think that i_mutex really buys
us anything there, unless anybody else can spot a good reason for it?

That said, the patch as is looks ok to me, its just that we may not need
i_mutex protection at all now,

Steve.


> ---
>  fs/ext2/inode.c   |    4 ++--
>  fs/ext3/inode.c   |    4 ++--
>  fs/ext4/extents.c |    4 ++--
>  fs/gfs2/inode.c   |    3 ---
>  fs/ioctl.c        |    2 ++
>  5 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 6363ac6..0e7a0b3 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -760,8 +760,8 @@ int ext2_get_block(struct inode *inode, sector_t iblock, struct buffer_head *bh_
>  int ext2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  		u64 start, u64 len)
>  {
> -	return generic_block_fiemap(inode, fieinfo, start, len,
> -				    ext2_get_block);
> +	return __generic_block_fiemap(inode, fieinfo, start, len,
> +				      ext2_get_block);
>  }
>  
>  static int ext2_writepage(struct page *page, struct writeback_control *wbc)
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index a075973..0727254 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -1046,8 +1046,8 @@ out:
>  int ext3_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  		u64 start, u64 len)
>  {
> -	return generic_block_fiemap(inode, fieinfo, start, len,
> -				    ext3_get_block);
> +	return __generic_block_fiemap(inode, fieinfo, start, len,
> +				      ext3_get_block);
>  }
>  
>  /*
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index cd0c7ed..e960831 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4916,8 +4916,8 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  
>  	/* fallback to generic here if not in extents fmt */
>  	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
> -		return generic_block_fiemap(inode, fieinfo, start, len,
> -			ext4_get_block);
> +		return __generic_block_fiemap(inode, fieinfo, start, len,
> +					      ext4_get_block);
>  
>  	if (fiemap_check_flags(fieinfo, EXT4_FIEMAP_FLAGS))
>  		return -EBADR;
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 4ce22e5..bb8d541 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -1775,8 +1775,6 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  	if (ret)
>  		return ret;
>  
> -	mutex_lock(&inode->i_mutex);
> -
>  	ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
>  	if (ret)
>  		goto out;
> @@ -1802,7 +1800,6 @@ static int gfs2_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  
>  	gfs2_glock_dq_uninit(&gh);
>  out:
> -	mutex_unlock(&inode->i_mutex);
>  	return ret;
>  }
>  
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 29167be..320993f 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -206,7 +206,9 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
>  	if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
>  		filemap_write_and_wait(inode->i_mapping);
>  
> +	mutex_lock(&inode->i_mutex);
>  	error = inode->i_op->fiemap(inode, &fieinfo, fiemap.fm_start, len);
> +	mutex_unlock(&inode->i_mutex);
>  	fiemap.fm_flags = fieinfo.fi_flags;
>  	fiemap.fm_mapped_extents = fieinfo.fi_extents_mapped;
>  	if (copy_to_user(ufiemap, &fiemap, sizeof(fiemap)))