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:33 +0300, Kasatkin, Dmitry wrote:
> On Fri, Sep 21, 2012 at 5:27 PM, Steven Whitehouse <swhiteho@redhat.com> 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@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.
> >
> 
> So you say that we might not need to lock i_mutex at all when we call
> __generic_block_fiemap?
> 
> - Dmitry
> 
Yes for GFS2. Although, also it makes little odds if it is held. I'm not
sure what the requirements are for the other filesystems, they may or
may not have their own locking,

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)))
> >
> >