On Fri, Sep 21, 2012 at 5:27 PM, Steven Whitehouse <swhiteho@xxxxxxxxxx> wrote:
> 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.
>
So you say that we might not need to lock i_mutex at all when we call
__generic_block_fiemap?
- Dmitry
>
>> ---
>> 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
