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@xxxxxxxxx>
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)))
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
