| Message ID | 1373876172-6467-1-git-send-email-junxiao.bi@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Junxiao, IIRC, in the case we use inode->i_size directly in ocfs2, inode->i_mutex is already locked, so no one can change i_size and it is safe. Thanks, Tao On 07/15/2013 04:16 PM, Junxiao Bi wrote: > Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> > --- > fs/ocfs2/aops.c | 2 +- > fs/ocfs2/extent_map.c | 10 +++++----- > fs/ocfs2/file.c | 2 +- > fs/ocfs2/ioctl.c | 2 +- > fs/ocfs2/journal.c | 8 ++++---- > fs/ocfs2/move_extents.c | 2 +- > fs/ocfs2/quota_global.c | 6 +++--- > fs/ocfs2/quota_local.c | 12 ++++++------ > 8 files changed, 22 insertions(+), 22 deletions(-) > > diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c > index 20dfec7..ab3ebf3 100644 > --- a/fs/ocfs2/aops.c > +++ b/fs/ocfs2/aops.c > @@ -2049,7 +2049,7 @@ int ocfs2_write_end_nolock(struct address_space *mapping, > > out_write_size: > pos += copied; > - if (pos > inode->i_size) { > + if (pos > i_size_read(inode)) { > i_size_write(inode, pos); > mark_inode_dirty(inode); > } > diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c > index 2487116..4bf2b76 100644 > --- a/fs/ocfs2/extent_map.c > +++ b/fs/ocfs2/extent_map.c > @@ -852,20 +852,20 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) > > down_read(&OCFS2_I(inode)->ip_alloc_sem); > > - if (*offset >= inode->i_size) { > + if (*offset >= i_size_read(inode)) { > ret = -ENXIO; > goto out_unlock; > } > > if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { > if (whence == SEEK_HOLE) > - *offset = inode->i_size; > + *offset = i_size_read(inode); > goto out_unlock; > } > > clen = 0; > cpos = *offset >> cs_bits; > - cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); > + cend = ocfs2_clusters_for_bytes(inode->i_sb, i_size_read(inode)); > > while (cpos < cend && !is_last) { > ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, > @@ -904,8 +904,8 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) > extlen = clen; > extlen <<= cs_bits; > > - if ((extoff + extlen) > inode->i_size) > - extlen = inode->i_size - extoff; > + if ((extoff + extlen) > i_size_read(inode)) > + extlen = i_size_read(inode) - extoff; > extoff += extlen; > if (extoff > *offset) > *offset = extoff; > diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c > index 3ce6a8b..7158710 100644 > --- a/fs/ocfs2/file.c > +++ b/fs/ocfs2/file.c > @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) > case SEEK_SET: > break; > case SEEK_END: > - offset += inode->i_size; > + offset += i_size_read(inode); > break; > case SEEK_CUR: > if (offset == 0) { > diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c > index 0c60ef2..fa32ce9 100644 > --- a/fs/ocfs2/ioctl.c > +++ b/fs/ocfs2/ioctl.c > @@ -303,7 +303,7 @@ int ocfs2_info_handle_journal_size(struct inode *inode, > if (o2info_from_user(oij, req)) > goto bail; > > - oij.ij_journal_size = osb->journal->j_inode->i_size; > + oij.ij_journal_size = i_size_read(osb->journal->j_inode); > > o2info_set_request_filled(&oij.ij_req); > > diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c > index 8eccfab..b5c8c53 100644 > --- a/fs/ocfs2/journal.c > +++ b/fs/ocfs2/journal.c > @@ -801,14 +801,14 @@ int ocfs2_journal_init(struct ocfs2_journal *journal, int *dirty) > inode_lock = 1; > di = (struct ocfs2_dinode *)bh->b_data; > > - if (inode->i_size < OCFS2_MIN_JOURNAL_SIZE) { > + if (i_size_read(inode) < OCFS2_MIN_JOURNAL_SIZE) { > mlog(ML_ERROR, "Journal file size (%lld) is too small!\n", > - inode->i_size); > + i_size_read(inode)); > status = -EINVAL; > goto done; > } > > - trace_ocfs2_journal_init(inode->i_size, > + trace_ocfs2_journal_init(i_size_read(inode), > (unsigned long long)inode->i_blocks, > OCFS2_I(inode)->ip_clusters); > > @@ -1096,7 +1096,7 @@ static int ocfs2_force_read_journal(struct inode *inode) > > memset(bhs, 0, sizeof(struct buffer_head *) * CONCURRENT_JOURNAL_FILL); > > - num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, inode->i_size); > + num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, i_size_read(inode)); > v_blkno = 0; > while (v_blkno < num_blocks) { > status = ocfs2_extent_map_get_blocks(inode, v_blkno, > diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c > index f1fc172..33472e4 100644 > --- a/fs/ocfs2/move_extents.c > +++ b/fs/ocfs2/move_extents.c > @@ -845,7 +845,7 @@ static int __ocfs2_move_extents_range(struct buffer_head *di_bh, > struct ocfs2_move_extents *range = context->range; > struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); > > - if ((inode->i_size == 0) || (range->me_len == 0)) > + if ((i_size_read(inode) == 0) || (range->me_len == 0)) > return 0; > > if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) > diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c > index 332a281..aaa5061 100644 > --- a/fs/ocfs2/quota_global.c > +++ b/fs/ocfs2/quota_global.c > @@ -234,7 +234,7 @@ ssize_t ocfs2_quota_write(struct super_block *sb, int type, > len = sb->s_blocksize - OCFS2_QBLK_RESERVED_SPACE - offset; > } > > - if (gqinode->i_size < off + len) { > + if (i_size_read(gqinode) < off + len) { > loff_t rounded_end = > ocfs2_align_bytes_to_blocks(sb, off + len); > > @@ -778,8 +778,8 @@ static int ocfs2_acquire_dquot(struct dquot *dquot) > */ > WARN_ON(journal_current_handle()); > status = ocfs2_extend_no_holes(gqinode, NULL, > - gqinode->i_size + (need_alloc << sb->s_blocksize_bits), > - gqinode->i_size); > + i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits), > + i_size_read(gqinode)); > if (status < 0) > goto out_dq; > } > diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c > index 27fe7ee..2e4344b 100644 > --- a/fs/ocfs2/quota_local.c > +++ b/fs/ocfs2/quota_local.c > @@ -982,14 +982,14 @@ static struct ocfs2_quota_chunk *ocfs2_local_quota_add_chunk( > > /* We are protected by dqio_sem so no locking needed */ > status = ocfs2_extend_no_holes(lqinode, NULL, > - lqinode->i_size + 2 * sb->s_blocksize, > - lqinode->i_size); > + i_size_read(lqinode) + 2 * sb->s_blocksize, > + i_size_read(lqinode)); > if (status < 0) { > mlog_errno(status); > goto out; > } > status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, > - lqinode->i_size + 2 * sb->s_blocksize); > + i_size_read(lqinode) + 2 * sb->s_blocksize); > if (status < 0) { > mlog_errno(status); > goto out; > @@ -1125,14 +1125,14 @@ static struct ocfs2_quota_chunk *ocfs2_extend_local_quota_file( > > /* We are protected by dqio_sem so no locking needed */ > status = ocfs2_extend_no_holes(lqinode, NULL, > - lqinode->i_size + sb->s_blocksize, > - lqinode->i_size); > + i_size_read(lqinode) + sb->s_blocksize, > + i_size_read(lqinode)); > if (status < 0) { > mlog_errno(status); > goto out; > } > status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, > - lqinode->i_size + sb->s_blocksize); > + i_size_read(lqinode) + sb->s_blocksize); > if (status < 0) { > mlog_errno(status); > goto out; >
On 07/16/2013 11:05 AM, Tao Ma wrote: > Hi Junxiao, > IIRC, in the case we use inode->i_size directly in ocfs2, > inode->i_mutex is already locked, so no one can change i_size and it is > safe. Yes, Just as Tao's comments, we're safe to fetch the i_size in current approach. But...Please see my comments inline. > > Thanks, > Tao > On 07/15/2013 04:16 PM, Junxiao Bi wrote: >> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> >> --- >> fs/ocfs2/aops.c | 2 +- >> fs/ocfs2/extent_map.c | 10 +++++----- >> fs/ocfs2/file.c | 2 +- >> fs/ocfs2/ioctl.c | 2 +- >> fs/ocfs2/journal.c | 8 ++++---- >> fs/ocfs2/move_extents.c | 2 +- >> fs/ocfs2/quota_global.c | 6 +++--- >> fs/ocfs2/quota_local.c | 12 ++++++------ >> 8 files changed, 22 insertions(+), 22 deletions(-) >> >> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >> index 20dfec7..ab3ebf3 100644 >> --- a/fs/ocfs2/aops.c >> +++ b/fs/ocfs2/aops.c >> @@ -2049,7 +2049,7 @@ int ocfs2_write_end_nolock(struct address_space *mapping, >> >> out_write_size: >> pos += copied; >> - if (pos > inode->i_size) { >> + if (pos > i_size_read(inode)) { >> i_size_write(inode, pos); >> mark_inode_dirty(inode); >> } >> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c >> index 2487116..4bf2b76 100644 >> --- a/fs/ocfs2/extent_map.c >> +++ b/fs/ocfs2/extent_map.c >> @@ -852,20 +852,20 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) >> >> down_read(&OCFS2_I(inode)->ip_alloc_sem); >> >> - if (*offset >= inode->i_size) { >> + if (*offset >= i_size_read(inode)) { >> ret = -ENXIO; >> goto out_unlock; >> } >> >> if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { >> if (whence == SEEK_HOLE) >> - *offset = inode->i_size; >> + *offset = i_size_read(inode); >> goto out_unlock; >> } >> >> clen = 0; >> cpos = *offset >> cs_bits; >> - cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); >> + cend = ocfs2_clusters_for_bytes(inode->i_sb, i_size_read(inode)); >> >> while (cpos < cend && !is_last) { >> ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, >> @@ -904,8 +904,8 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) >> extlen = clen; >> extlen <<= cs_bits; >> >> - if ((extoff + extlen) > inode->i_size) >> - extlen = inode->i_size - extoff; >> + if ((extoff + extlen) > i_size_read(inode)) >> + extlen = i_size_read(inode) - extoff; >> extoff += extlen; >> if (extoff > *offset) >> *offset = extoff; >> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >> index 3ce6a8b..7158710 100644 >> --- a/fs/ocfs2/file.c >> +++ b/fs/ocfs2/file.c >> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) We have the inode mutex around ocfs2_file_llseek(), and that is too extensive to block the concurrent access for particular seek operations. At least, we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, we can fall through generic_file_llseek() directly without the mutex lock. >> case SEEK_SET: >> break; >> case SEEK_END: >> - offset += inode->i_size; >> + offset += i_size_read(inode); Hmmm, so you made this patch against an old kernel source tree... We'd better to write OCFS2 upstream patch based on linux-next or -mm tree. :) Canquan already fixed this in anther patch: From: Jensen <shencanquan@huawei.com> Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END Would you like to take care of this? Thanks, -Jeff >> break; >> case SEEK_CUR: >> if (offset == 0) { >> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c >> index 0c60ef2..fa32ce9 100644 >> --- a/fs/ocfs2/ioctl.c >> +++ b/fs/ocfs2/ioctl.c >> @@ -303,7 +303,7 @@ int ocfs2_info_handle_journal_size(struct inode *inode, >> if (o2info_from_user(oij, req)) >> goto bail; >> >> - oij.ij_journal_size = osb->journal->j_inode->i_size; >> + oij.ij_journal_size = i_size_read(osb->journal->j_inode); >> >> o2info_set_request_filled(&oij.ij_req); >> >> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c >> index 8eccfab..b5c8c53 100644 >> --- a/fs/ocfs2/journal.c >> +++ b/fs/ocfs2/journal.c >> @@ -801,14 +801,14 @@ int ocfs2_journal_init(struct ocfs2_journal *journal, int *dirty) >> inode_lock = 1; >> di = (struct ocfs2_dinode *)bh->b_data; >> >> - if (inode->i_size < OCFS2_MIN_JOURNAL_SIZE) { >> + if (i_size_read(inode) < OCFS2_MIN_JOURNAL_SIZE) { >> mlog(ML_ERROR, "Journal file size (%lld) is too small!\n", >> - inode->i_size); >> + i_size_read(inode)); >> status = -EINVAL; >> goto done; >> } >> >> - trace_ocfs2_journal_init(inode->i_size, >> + trace_ocfs2_journal_init(i_size_read(inode), >> (unsigned long long)inode->i_blocks, >> OCFS2_I(inode)->ip_clusters); >> >> @@ -1096,7 +1096,7 @@ static int ocfs2_force_read_journal(struct inode *inode) >> >> memset(bhs, 0, sizeof(struct buffer_head *) * CONCURRENT_JOURNAL_FILL); >> >> - num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, inode->i_size); >> + num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, i_size_read(inode)); >> v_blkno = 0; >> while (v_blkno < num_blocks) { >> status = ocfs2_extent_map_get_blocks(inode, v_blkno, >> diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c >> index f1fc172..33472e4 100644 >> --- a/fs/ocfs2/move_extents.c >> +++ b/fs/ocfs2/move_extents.c >> @@ -845,7 +845,7 @@ static int __ocfs2_move_extents_range(struct buffer_head *di_bh, >> struct ocfs2_move_extents *range = context->range; >> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); >> >> - if ((inode->i_size == 0) || (range->me_len == 0)) >> + if ((i_size_read(inode) == 0) || (range->me_len == 0)) >> return 0; >> >> if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) >> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c >> index 332a281..aaa5061 100644 >> --- a/fs/ocfs2/quota_global.c >> +++ b/fs/ocfs2/quota_global.c >> @@ -234,7 +234,7 @@ ssize_t ocfs2_quota_write(struct super_block *sb, int type, >> len = sb->s_blocksize - OCFS2_QBLK_RESERVED_SPACE - offset; >> } >> >> - if (gqinode->i_size < off + len) { >> + if (i_size_read(gqinode) < off + len) { >> loff_t rounded_end = >> ocfs2_align_bytes_to_blocks(sb, off + len); >> >> @@ -778,8 +778,8 @@ static int ocfs2_acquire_dquot(struct dquot *dquot) >> */ >> WARN_ON(journal_current_handle()); >> status = ocfs2_extend_no_holes(gqinode, NULL, >> - gqinode->i_size + (need_alloc << sb->s_blocksize_bits), >> - gqinode->i_size); >> + i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits), >> + i_size_read(gqinode)); >> if (status < 0) >> goto out_dq; >> } >> diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c >> index 27fe7ee..2e4344b 100644 >> --- a/fs/ocfs2/quota_local.c >> +++ b/fs/ocfs2/quota_local.c >> @@ -982,14 +982,14 @@ static struct ocfs2_quota_chunk *ocfs2_local_quota_add_chunk( >> >> /* We are protected by dqio_sem so no locking needed */ >> status = ocfs2_extend_no_holes(lqinode, NULL, >> - lqinode->i_size + 2 * sb->s_blocksize, >> - lqinode->i_size); >> + i_size_read(lqinode) + 2 * sb->s_blocksize, >> + i_size_read(lqinode)); >> if (status < 0) { >> mlog_errno(status); >> goto out; >> } >> status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, >> - lqinode->i_size + 2 * sb->s_blocksize); >> + i_size_read(lqinode) + 2 * sb->s_blocksize); >> if (status < 0) { >> mlog_errno(status); >> goto out; >> @@ -1125,14 +1125,14 @@ static struct ocfs2_quota_chunk *ocfs2_extend_local_quota_file( >> >> /* We are protected by dqio_sem so no locking needed */ >> status = ocfs2_extend_no_holes(lqinode, NULL, >> - lqinode->i_size + sb->s_blocksize, >> - lqinode->i_size); >> + i_size_read(lqinode) + sb->s_blocksize, >> + i_size_read(lqinode)); >> if (status < 0) { >> mlog_errno(status); >> goto out; >> } >> status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, >> - lqinode->i_size + sb->s_blocksize); >> + i_size_read(lqinode) + sb->s_blocksize); >> if (status < 0) { >> mlog_errno(status); >> goto out; >> > > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel
Hi Tao, On 07/16/2013 11:05 AM, Tao Ma wrote: > Hi Junxiao, > IIRC, in the case we use inode->i_size directly in ocfs2, > inode->i_mutex is already locked, so no one can change i_size and it is > safe. Yes, but there are both inode->i_size and i_size_read/write() in the code, I think we'd better change it to ease confuse. Thanks, Junxiao. > > Thanks, > Tao > On 07/15/2013 04:16 PM, Junxiao Bi wrote: >> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> >> --- >> fs/ocfs2/aops.c | 2 +- >> fs/ocfs2/extent_map.c | 10 +++++----- >> fs/ocfs2/file.c | 2 +- >> fs/ocfs2/ioctl.c | 2 +- >> fs/ocfs2/journal.c | 8 ++++---- >> fs/ocfs2/move_extents.c | 2 +- >> fs/ocfs2/quota_global.c | 6 +++--- >> fs/ocfs2/quota_local.c | 12 ++++++------ >> 8 files changed, 22 insertions(+), 22 deletions(-) >> >> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >> index 20dfec7..ab3ebf3 100644 >> --- a/fs/ocfs2/aops.c >> +++ b/fs/ocfs2/aops.c >> @@ -2049,7 +2049,7 @@ int ocfs2_write_end_nolock(struct address_space *mapping, >> >> out_write_size: >> pos += copied; >> - if (pos > inode->i_size) { >> + if (pos > i_size_read(inode)) { >> i_size_write(inode, pos); >> mark_inode_dirty(inode); >> } >> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c >> index 2487116..4bf2b76 100644 >> --- a/fs/ocfs2/extent_map.c >> +++ b/fs/ocfs2/extent_map.c >> @@ -852,20 +852,20 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) >> >> down_read(&OCFS2_I(inode)->ip_alloc_sem); >> >> - if (*offset >= inode->i_size) { >> + if (*offset >= i_size_read(inode)) { >> ret = -ENXIO; >> goto out_unlock; >> } >> >> if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { >> if (whence == SEEK_HOLE) >> - *offset = inode->i_size; >> + *offset = i_size_read(inode); >> goto out_unlock; >> } >> >> clen = 0; >> cpos = *offset >> cs_bits; >> - cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); >> + cend = ocfs2_clusters_for_bytes(inode->i_sb, i_size_read(inode)); >> >> while (cpos < cend && !is_last) { >> ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, >> @@ -904,8 +904,8 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) >> extlen = clen; >> extlen <<= cs_bits; >> >> - if ((extoff + extlen) > inode->i_size) >> - extlen = inode->i_size - extoff; >> + if ((extoff + extlen) > i_size_read(inode)) >> + extlen = i_size_read(inode) - extoff; >> extoff += extlen; >> if (extoff > *offset) >> *offset = extoff; >> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >> index 3ce6a8b..7158710 100644 >> --- a/fs/ocfs2/file.c >> +++ b/fs/ocfs2/file.c >> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) >> case SEEK_SET: >> break; >> case SEEK_END: >> - offset += inode->i_size; >> + offset += i_size_read(inode); >> break; >> case SEEK_CUR: >> if (offset == 0) { >> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c >> index 0c60ef2..fa32ce9 100644 >> --- a/fs/ocfs2/ioctl.c >> +++ b/fs/ocfs2/ioctl.c >> @@ -303,7 +303,7 @@ int ocfs2_info_handle_journal_size(struct inode *inode, >> if (o2info_from_user(oij, req)) >> goto bail; >> >> - oij.ij_journal_size = osb->journal->j_inode->i_size; >> + oij.ij_journal_size = i_size_read(osb->journal->j_inode); >> >> o2info_set_request_filled(&oij.ij_req); >> >> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c >> index 8eccfab..b5c8c53 100644 >> --- a/fs/ocfs2/journal.c >> +++ b/fs/ocfs2/journal.c >> @@ -801,14 +801,14 @@ int ocfs2_journal_init(struct ocfs2_journal *journal, int *dirty) >> inode_lock = 1; >> di = (struct ocfs2_dinode *)bh->b_data; >> >> - if (inode->i_size < OCFS2_MIN_JOURNAL_SIZE) { >> + if (i_size_read(inode) < OCFS2_MIN_JOURNAL_SIZE) { >> mlog(ML_ERROR, "Journal file size (%lld) is too small!\n", >> - inode->i_size); >> + i_size_read(inode)); >> status = -EINVAL; >> goto done; >> } >> >> - trace_ocfs2_journal_init(inode->i_size, >> + trace_ocfs2_journal_init(i_size_read(inode), >> (unsigned long long)inode->i_blocks, >> OCFS2_I(inode)->ip_clusters); >> >> @@ -1096,7 +1096,7 @@ static int ocfs2_force_read_journal(struct inode *inode) >> >> memset(bhs, 0, sizeof(struct buffer_head *) * CONCURRENT_JOURNAL_FILL); >> >> - num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, inode->i_size); >> + num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, i_size_read(inode)); >> v_blkno = 0; >> while (v_blkno < num_blocks) { >> status = ocfs2_extent_map_get_blocks(inode, v_blkno, >> diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c >> index f1fc172..33472e4 100644 >> --- a/fs/ocfs2/move_extents.c >> +++ b/fs/ocfs2/move_extents.c >> @@ -845,7 +845,7 @@ static int __ocfs2_move_extents_range(struct buffer_head *di_bh, >> struct ocfs2_move_extents *range = context->range; >> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); >> >> - if ((inode->i_size == 0) || (range->me_len == 0)) >> + if ((i_size_read(inode) == 0) || (range->me_len == 0)) >> return 0; >> >> if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) >> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c >> index 332a281..aaa5061 100644 >> --- a/fs/ocfs2/quota_global.c >> +++ b/fs/ocfs2/quota_global.c >> @@ -234,7 +234,7 @@ ssize_t ocfs2_quota_write(struct super_block *sb, int type, >> len = sb->s_blocksize - OCFS2_QBLK_RESERVED_SPACE - offset; >> } >> >> - if (gqinode->i_size < off + len) { >> + if (i_size_read(gqinode) < off + len) { >> loff_t rounded_end = >> ocfs2_align_bytes_to_blocks(sb, off + len); >> >> @@ -778,8 +778,8 @@ static int ocfs2_acquire_dquot(struct dquot *dquot) >> */ >> WARN_ON(journal_current_handle()); >> status = ocfs2_extend_no_holes(gqinode, NULL, >> - gqinode->i_size + (need_alloc << sb->s_blocksize_bits), >> - gqinode->i_size); >> + i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits), >> + i_size_read(gqinode)); >> if (status < 0) >> goto out_dq; >> } >> diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c >> index 27fe7ee..2e4344b 100644 >> --- a/fs/ocfs2/quota_local.c >> +++ b/fs/ocfs2/quota_local.c >> @@ -982,14 +982,14 @@ static struct ocfs2_quota_chunk *ocfs2_local_quota_add_chunk( >> >> /* We are protected by dqio_sem so no locking needed */ >> status = ocfs2_extend_no_holes(lqinode, NULL, >> - lqinode->i_size + 2 * sb->s_blocksize, >> - lqinode->i_size); >> + i_size_read(lqinode) + 2 * sb->s_blocksize, >> + i_size_read(lqinode)); >> if (status < 0) { >> mlog_errno(status); >> goto out; >> } >> status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, >> - lqinode->i_size + 2 * sb->s_blocksize); >> + i_size_read(lqinode) + 2 * sb->s_blocksize); >> if (status < 0) { >> mlog_errno(status); >> goto out; >> @@ -1125,14 +1125,14 @@ static struct ocfs2_quota_chunk *ocfs2_extend_local_quota_file( >> >> /* We are protected by dqio_sem so no locking needed */ >> status = ocfs2_extend_no_holes(lqinode, NULL, >> - lqinode->i_size + sb->s_blocksize, >> - lqinode->i_size); >> + i_size_read(lqinode) + sb->s_blocksize, >> + i_size_read(lqinode)); >> if (status < 0) { >> mlog_errno(status); >> goto out; >> } >> status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, >> - lqinode->i_size + sb->s_blocksize); >> + i_size_read(lqinode) + sb->s_blocksize); >> if (status < 0) { >> mlog_errno(status); >> goto out; >>
Hi Jeff, On 07/17/2013 07:13 AM, Jeff Liu wrote: > On 07/16/2013 11:05 AM, Tao Ma wrote: > >> Hi Junxiao, >> IIRC, in the case we use inode->i_size directly in ocfs2, >> inode->i_mutex is already locked, so no one can change i_size and it is >> safe. > Yes, Just as Tao's comments, we're safe to fetch the i_size in current approach. > But...Please see my comments inline. > >> Thanks, >> Tao >> On 07/15/2013 04:16 PM, Junxiao Bi wrote: >>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> >>> --- >>> fs/ocfs2/aops.c | 2 +- >>> fs/ocfs2/extent_map.c | 10 +++++----- >>> fs/ocfs2/file.c | 2 +- >>> fs/ocfs2/ioctl.c | 2 +- >>> fs/ocfs2/journal.c | 8 ++++---- >>> fs/ocfs2/move_extents.c | 2 +- >>> fs/ocfs2/quota_global.c | 6 +++--- >>> fs/ocfs2/quota_local.c | 12 ++++++------ >>> 8 files changed, 22 insertions(+), 22 deletions(-) >>> >>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >>> index 20dfec7..ab3ebf3 100644 >>> --- a/fs/ocfs2/aops.c >>> +++ b/fs/ocfs2/aops.c >>> @@ -2049,7 +2049,7 @@ int ocfs2_write_end_nolock(struct address_space *mapping, >>> >>> out_write_size: >>> pos += copied; >>> - if (pos > inode->i_size) { >>> + if (pos > i_size_read(inode)) { >>> i_size_write(inode, pos); >>> mark_inode_dirty(inode); >>> } >>> diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c >>> index 2487116..4bf2b76 100644 >>> --- a/fs/ocfs2/extent_map.c >>> +++ b/fs/ocfs2/extent_map.c >>> @@ -852,20 +852,20 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) >>> >>> down_read(&OCFS2_I(inode)->ip_alloc_sem); >>> >>> - if (*offset >= inode->i_size) { >>> + if (*offset >= i_size_read(inode)) { >>> ret = -ENXIO; >>> goto out_unlock; >>> } >>> >>> if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { >>> if (whence == SEEK_HOLE) >>> - *offset = inode->i_size; >>> + *offset = i_size_read(inode); >>> goto out_unlock; >>> } >>> >>> clen = 0; >>> cpos = *offset >> cs_bits; >>> - cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); >>> + cend = ocfs2_clusters_for_bytes(inode->i_sb, i_size_read(inode)); >>> >>> while (cpos < cend && !is_last) { >>> ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, >>> @@ -904,8 +904,8 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) >>> extlen = clen; >>> extlen <<= cs_bits; >>> >>> - if ((extoff + extlen) > inode->i_size) >>> - extlen = inode->i_size - extoff; >>> + if ((extoff + extlen) > i_size_read(inode)) >>> + extlen = i_size_read(inode) - extoff; >>> extoff += extlen; >>> if (extoff > *offset) >>> *offset = extoff; >>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>> index 3ce6a8b..7158710 100644 >>> --- a/fs/ocfs2/file.c >>> +++ b/fs/ocfs2/file.c >>> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) > We have the inode mutex around ocfs2_file_llseek(), and that is too extensive > to block the concurrent access for particular seek operations. At least, > we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, > we can fall through generic_file_llseek() directly without the mutex lock. Agree, if we apply this patch, it seemes that we can directly remove this mutex from ocfs2_file_llseek(). > >>> case SEEK_SET: >>> break; >>> case SEEK_END: >>> - offset += inode->i_size; >>> + offset += i_size_read(inode); > Hmmm, so you made this patch against an old kernel source tree... > We'd better to write OCFS2 upstream patch based on linux-next or -mm tree. :) Oh, I just made the patch based on the kernel mainline. > > Canquan already fixed this in anther patch: > From: Jensen <shencanquan@huawei.com> > Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END > > Would you like to take care of this? > > Thanks, > -Jeff > >>> break; >>> case SEEK_CUR: >>> if (offset == 0) { > > >>> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c >>> index 0c60ef2..fa32ce9 100644 >>> --- a/fs/ocfs2/ioctl.c >>> +++ b/fs/ocfs2/ioctl.c >>> @@ -303,7 +303,7 @@ int ocfs2_info_handle_journal_size(struct inode *inode, >>> if (o2info_from_user(oij, req)) >>> goto bail; >>> >>> - oij.ij_journal_size = osb->journal->j_inode->i_size; >>> + oij.ij_journal_size = i_size_read(osb->journal->j_inode); >>> >>> o2info_set_request_filled(&oij.ij_req); >>> >>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c >>> index 8eccfab..b5c8c53 100644 >>> --- a/fs/ocfs2/journal.c >>> +++ b/fs/ocfs2/journal.c >>> @@ -801,14 +801,14 @@ int ocfs2_journal_init(struct ocfs2_journal *journal, int *dirty) >>> inode_lock = 1; >>> di = (struct ocfs2_dinode *)bh->b_data; >>> >>> - if (inode->i_size < OCFS2_MIN_JOURNAL_SIZE) { >>> + if (i_size_read(inode) < OCFS2_MIN_JOURNAL_SIZE) { >>> mlog(ML_ERROR, "Journal file size (%lld) is too small!\n", >>> - inode->i_size); >>> + i_size_read(inode)); >>> status = -EINVAL; >>> goto done; >>> } >>> >>> - trace_ocfs2_journal_init(inode->i_size, >>> + trace_ocfs2_journal_init(i_size_read(inode), >>> (unsigned long long)inode->i_blocks, >>> OCFS2_I(inode)->ip_clusters); >>> >>> @@ -1096,7 +1096,7 @@ static int ocfs2_force_read_journal(struct inode *inode) >>> >>> memset(bhs, 0, sizeof(struct buffer_head *) * CONCURRENT_JOURNAL_FILL); >>> >>> - num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, inode->i_size); >>> + num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, i_size_read(inode)); >>> v_blkno = 0; >>> while (v_blkno < num_blocks) { >>> status = ocfs2_extent_map_get_blocks(inode, v_blkno, >>> diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c >>> index f1fc172..33472e4 100644 >>> --- a/fs/ocfs2/move_extents.c >>> +++ b/fs/ocfs2/move_extents.c >>> @@ -845,7 +845,7 @@ static int __ocfs2_move_extents_range(struct buffer_head *di_bh, >>> struct ocfs2_move_extents *range = context->range; >>> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); >>> >>> - if ((inode->i_size == 0) || (range->me_len == 0)) >>> + if ((i_size_read(inode) == 0) || (range->me_len == 0)) >>> return 0; >>> >>> if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) >>> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c >>> index 332a281..aaa5061 100644 >>> --- a/fs/ocfs2/quota_global.c >>> +++ b/fs/ocfs2/quota_global.c >>> @@ -234,7 +234,7 @@ ssize_t ocfs2_quota_write(struct super_block *sb, int type, >>> len = sb->s_blocksize - OCFS2_QBLK_RESERVED_SPACE - offset; >>> } >>> >>> - if (gqinode->i_size < off + len) { >>> + if (i_size_read(gqinode) < off + len) { >>> loff_t rounded_end = >>> ocfs2_align_bytes_to_blocks(sb, off + len); >>> >>> @@ -778,8 +778,8 @@ static int ocfs2_acquire_dquot(struct dquot *dquot) >>> */ >>> WARN_ON(journal_current_handle()); >>> status = ocfs2_extend_no_holes(gqinode, NULL, >>> - gqinode->i_size + (need_alloc << sb->s_blocksize_bits), >>> - gqinode->i_size); >>> + i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits), >>> + i_size_read(gqinode)); >>> if (status < 0) >>> goto out_dq; >>> } >>> diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c >>> index 27fe7ee..2e4344b 100644 >>> --- a/fs/ocfs2/quota_local.c >>> +++ b/fs/ocfs2/quota_local.c >>> @@ -982,14 +982,14 @@ static struct ocfs2_quota_chunk *ocfs2_local_quota_add_chunk( >>> >>> /* We are protected by dqio_sem so no locking needed */ >>> status = ocfs2_extend_no_holes(lqinode, NULL, >>> - lqinode->i_size + 2 * sb->s_blocksize, >>> - lqinode->i_size); >>> + i_size_read(lqinode) + 2 * sb->s_blocksize, >>> + i_size_read(lqinode)); >>> if (status < 0) { >>> mlog_errno(status); >>> goto out; >>> } >>> status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, >>> - lqinode->i_size + 2 * sb->s_blocksize); >>> + i_size_read(lqinode) + 2 * sb->s_blocksize); >>> if (status < 0) { >>> mlog_errno(status); >>> goto out; >>> @@ -1125,14 +1125,14 @@ static struct ocfs2_quota_chunk *ocfs2_extend_local_quota_file( >>> >>> /* We are protected by dqio_sem so no locking needed */ >>> status = ocfs2_extend_no_holes(lqinode, NULL, >>> - lqinode->i_size + sb->s_blocksize, >>> - lqinode->i_size); >>> + i_size_read(lqinode) + sb->s_blocksize, >>> + i_size_read(lqinode)); >>> if (status < 0) { >>> mlog_errno(status); >>> goto out; >>> } >>> status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, >>> - lqinode->i_size + sb->s_blocksize); >>> + i_size_read(lqinode) + sb->s_blocksize); >>> if (status < 0) { >>> mlog_errno(status); >>> goto out; >>> >> >> _______________________________________________ >> Ocfs2-devel mailing list >> Ocfs2-devel@oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >
Hi Jeff, On 07/17/2013 07:13 AM, Jeff Liu wrote: >>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>> >> index 3ce6a8b..7158710 100644 >>> >> --- a/fs/ocfs2/file.c >>> >> +++ b/fs/ocfs2/file.c >>> >> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) > We have the inode mutex around ocfs2_file_llseek(), and that is too extensive > to block the concurrent access for particular seek operations. At least, > we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, > we can fall through generic_file_llseek() directly without the mutex lock. Think about this again, I think we can't remove the mutex, as it is used to protect other inode member beside i_size, like inode->i_sb->s_maxbytes which is accessed in all SEEK request and also more in ocfs2_inode_lock(). Thanks, Junxiao. > >>> >> case SEEK_SET: >>> >> break; >>> >> case SEEK_END: >>> >> - offset += inode->i_size; >>> >> + offset += i_size_read(inode);
Hi Junxiao, On 07/19/2013 03:38 PM, Junxiao Bi wrote: > Hi Jeff, > > On 07/17/2013 07:13 AM, Jeff Liu wrote: >>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>>>>> index 3ce6a8b..7158710 100644 >>>>>> --- a/fs/ocfs2/file.c >>>>>> +++ b/fs/ocfs2/file.c >>>>>> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) >> We have the inode mutex around ocfs2_file_llseek(), and that is too extensive >> to block the concurrent access for particular seek operations. At least, >> we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, >> we can fall through generic_file_llseek() directly without the mutex lock. > Think about this again, I think we can't remove the mutex, as it is used > to protect other inode member beside i_size, like > inode->i_sb->s_maxbytes which is accessed in all SEEK request and also > more in ocfs2_inode_lock(). Thanks for digging into this. s_maxbytes is a numeric constant which is calculated out at OCFS2 mount with filling up the super blocks, IOWs, it does not need any lock protection IMO. Besides that, could you please pointed me out anything am missed? Thanks, -Jeff > > Thanks, > Junxiao. >> >>>>>> case SEEK_SET: >>>>>> break; >>>>>> case SEEK_END: >>>>>> - offset += inode->i_size; >>>>>> + offset += i_size_read(inode); >
Hi Jeff, On 07/19/2013 04:03 PM, Jeff Liu wrote: > Hi Junxiao, > > On 07/19/2013 03:38 PM, Junxiao Bi wrote: > >> Hi Jeff, >> >> On 07/17/2013 07:13 AM, Jeff Liu wrote: >>>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>>>>>> index 3ce6a8b..7158710 100644 >>>>>>> --- a/fs/ocfs2/file.c >>>>>>> +++ b/fs/ocfs2/file.c >>>>>>> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) >>> We have the inode mutex around ocfs2_file_llseek(), and that is too extensive >>> to block the concurrent access for particular seek operations. At least, >>> we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, >>> we can fall through generic_file_llseek() directly without the mutex lock. >> Think about this again, I think we can't remove the mutex, as it is used >> to protect other inode member beside i_size, like >> inode->i_sb->s_maxbytes which is accessed in all SEEK request and also >> more in ocfs2_inode_lock(). > Thanks for digging into this. s_maxbytes is a numeric constant which is > calculated out at OCFS2 mount with filling up the super blocks, IOWs, it > does not need any lock protection IMO. > > Besides that, could you please pointed me out anything am missed? Can inode->i_sb be set to NULL during this? Thanks, Junxiao. > > Thanks, > -Jeff > >> Thanks, >> Junxiao. >>>>>>> case SEEK_SET: >>>>>>> break; >>>>>>> case SEEK_END: >>>>>>> - offset += inode->i_size; >>>>>>> + offset += i_size_read(inode); >
On 07/19/2013 04:12 PM, Junxiao Bi wrote: > Hi Jeff, > > On 07/19/2013 04:03 PM, Jeff Liu wrote: >> Hi Junxiao, >> >> On 07/19/2013 03:38 PM, Junxiao Bi wrote: >> >>> Hi Jeff, >>> >>> On 07/17/2013 07:13 AM, Jeff Liu wrote: >>>>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>>>>>>> index 3ce6a8b..7158710 100644 >>>>>>>> --- a/fs/ocfs2/file.c >>>>>>>> +++ b/fs/ocfs2/file.c >>>>>>>> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) >>>> We have the inode mutex around ocfs2_file_llseek(), and that is too extensive >>>> to block the concurrent access for particular seek operations. At least, >>>> we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, >>>> we can fall through generic_file_llseek() directly without the mutex lock. >>> Think about this again, I think we can't remove the mutex, as it is used >>> to protect other inode member beside i_size, like >>> inode->i_sb->s_maxbytes which is accessed in all SEEK request and also >>> more in ocfs2_inode_lock(). >> Thanks for digging into this. s_maxbytes is a numeric constant which is >> calculated out at OCFS2 mount with filling up the super blocks, IOWs, it >> does not need any lock protection IMO. >> >> Besides that, could you please pointed me out anything am missed? > Can inode->i_sb be set to NULL during this? No, actually when we get into ocfs2_file_llseek(), the corresponding inode should be in VFS inode cache and it should not be a bad inode as well, otherwise, it can not go through vfs_llseek(). :) Thanks, -Jeff > > Thanks, > Junxiao. >> >> Thanks, >> -Jeff >> >>> Thanks, >>> Junxiao. >>>>>>>> case SEEK_SET: >>>>>>>> break; >>>>>>>> case SEEK_END: >>>>>>>> - offset += inode->i_size; >>>>>>>> + offset += i_size_read(inode); >> >
Hi Jeff, On 07/19/2013 04:30 PM, Jeff Liu wrote: > On 07/19/2013 04:12 PM, Junxiao Bi wrote: > >> Hi Jeff, >> >> On 07/19/2013 04:03 PM, Jeff Liu wrote: >>> Hi Junxiao, >>> >>> On 07/19/2013 03:38 PM, Junxiao Bi wrote: >>> >>>> Hi Jeff, >>>> >>>> On 07/17/2013 07:13 AM, Jeff Liu wrote: >>>>>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>>>>>>>> index 3ce6a8b..7158710 100644 >>>>>>>>> --- a/fs/ocfs2/file.c >>>>>>>>> +++ b/fs/ocfs2/file.c >>>>>>>>> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) >>>>> We have the inode mutex around ocfs2_file_llseek(), and that is too extensive >>>>> to block the concurrent access for particular seek operations. At least, >>>>> we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, >>>>> we can fall through generic_file_llseek() directly without the mutex lock. >>>> Think about this again, I think we can't remove the mutex, as it is used >>>> to protect other inode member beside i_size, like >>>> inode->i_sb->s_maxbytes which is accessed in all SEEK request and also >>>> more in ocfs2_inode_lock(). >>> Thanks for digging into this. s_maxbytes is a numeric constant which is >>> calculated out at OCFS2 mount with filling up the super blocks, IOWs, it >>> does not need any lock protection IMO. >>> >>> Besides that, could you please pointed me out anything am missed? >> Can inode->i_sb be set to NULL during this? > No, actually when we get into ocfs2_file_llseek(), the corresponding > inode should be in VFS inode cache and it should not be a bad inode > as well, otherwise, it can not go through vfs_llseek(). :) Thank you for explaining this. So for SEEK_SET and SEEK_CUR, we may make some improvement, indeed in gereric_file_llseek(), it also use a spin lock to protect concurrent SEEK_CUR access. is there really a perf issue that deserve us to make a hack to hurt code readability? Thanks, Junxiao. > > Thanks, > -Jeff > >> Thanks, >> Junxiao. >>> Thanks, >>> -Jeff >>> >>>> Thanks, >>>> Junxiao. >>>>>>>>> case SEEK_SET: >>>>>>>>> break; >>>>>>>>> case SEEK_END: >>>>>>>>> - offset += inode->i_size; >>>>>>>>> + offset += i_size_read(inode); >
On 07/19/2013 05:16 PM, Junxiao Bi wrote: > Hi Jeff, > > On 07/19/2013 04:30 PM, Jeff Liu wrote: >> On 07/19/2013 04:12 PM, Junxiao Bi wrote: >> >>> Hi Jeff, >>> >>> On 07/19/2013 04:03 PM, Jeff Liu wrote: >>>> Hi Junxiao, >>>> >>>> On 07/19/2013 03:38 PM, Junxiao Bi wrote: >>>> >>>>> Hi Jeff, >>>>> >>>>> On 07/17/2013 07:13 AM, Jeff Liu wrote: >>>>>>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>>>>>>>>> index 3ce6a8b..7158710 100644 >>>>>>>>>> --- a/fs/ocfs2/file.c >>>>>>>>>> +++ b/fs/ocfs2/file.c >>>>>>>>>> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) >>>>>> We have the inode mutex around ocfs2_file_llseek(), and that is too extensive >>>>>> to block the concurrent access for particular seek operations. At least, >>>>>> we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, >>>>>> we can fall through generic_file_llseek() directly without the mutex lock. >>>>> Think about this again, I think we can't remove the mutex, as it is used >>>>> to protect other inode member beside i_size, like >>>>> inode->i_sb->s_maxbytes which is accessed in all SEEK request and also >>>>> more in ocfs2_inode_lock(). >>>> Thanks for digging into this. s_maxbytes is a numeric constant which is >>>> calculated out at OCFS2 mount with filling up the super blocks, IOWs, it >>>> does not need any lock protection IMO. >>>> >>>> Besides that, could you please pointed me out anything am missed? >>> Can inode->i_sb be set to NULL during this? >> No, actually when we get into ocfs2_file_llseek(), the corresponding >> inode should be in VFS inode cache and it should not be a bad inode >> as well, otherwise, it can not go through vfs_llseek(). :) > Thank you for explaining this. > So for SEEK_SET and SEEK_CUR, we may make some improvement, indeed in > gereric_file_llseek(), it also use a spin lock to protect concurrent > SEEK_CUR access. > is there really a perf issue that deserve us to make a hack to hurt code > readability? Sure, and also, I don't think this is *hack*. Instead, it's an old significant improvements in VFS generic_file_lseek() from Andi Kleen back to 2011. Maybe OCFS2 is the only file system without this turn-up as we lack of upstream activity for a long time. For detail, please refer to: commit ef3d0fd27e90f67e35da516dafc1482c82939a60 Author: Andi Kleen <ak@linux.intel.com> Date: Thu Sep 15 16:06:48 2011 -0700 vfs: do (nearly) lockless generic_file_llseek The i_mutex lock use of generic _file_llseek hurts. Independent processes accessing the same file synchronize over a single lock, even though they have no need for synchronization at all. Under high utilization this can cause llseek to scale very poorly on larger systems. This patch does some rethinking of the llseek locking model: Thanks, -Jeff
Hi Jeff, On 07/19/2013 05:50 PM, Jeff Liu wrote: > On 07/19/2013 05:16 PM, Junxiao Bi wrote: > >> Hi Jeff, >> >> On 07/19/2013 04:30 PM, Jeff Liu wrote: >>> On 07/19/2013 04:12 PM, Junxiao Bi wrote: >>> >>>> Hi Jeff, >>>> >>>> On 07/19/2013 04:03 PM, Jeff Liu wrote: >>>>> Hi Junxiao, >>>>> >>>>> On 07/19/2013 03:38 PM, Junxiao Bi wrote: >>>>> >>>>>> Hi Jeff, >>>>>> >>>>>> On 07/17/2013 07:13 AM, Jeff Liu wrote: >>>>>>>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c >>>>>>>>>>> index 3ce6a8b..7158710 100644 >>>>>>>>>>> --- a/fs/ocfs2/file.c >>>>>>>>>>> +++ b/fs/ocfs2/file.c >>>>>>>>>>> @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) >>>>>>> We have the inode mutex around ocfs2_file_llseek(), and that is too extensive >>>>>>> to block the concurrent access for particular seek operations. At least, >>>>>>> we can get rid of this lock for SEEK_SET/SEEK_CUR. i.e, In either case, >>>>>>> we can fall through generic_file_llseek() directly without the mutex lock. >>>>>> Think about this again, I think we can't remove the mutex, as it is used >>>>>> to protect other inode member beside i_size, like >>>>>> inode->i_sb->s_maxbytes which is accessed in all SEEK request and also >>>>>> more in ocfs2_inode_lock(). >>>>> Thanks for digging into this. s_maxbytes is a numeric constant which is >>>>> calculated out at OCFS2 mount with filling up the super blocks, IOWs, it >>>>> does not need any lock protection IMO. >>>>> >>>>> Besides that, could you please pointed me out anything am missed? >>>> Can inode->i_sb be set to NULL during this? >>> No, actually when we get into ocfs2_file_llseek(), the corresponding >>> inode should be in VFS inode cache and it should not be a bad inode >>> as well, otherwise, it can not go through vfs_llseek(). :) >> Thank you for explaining this. >> So for SEEK_SET and SEEK_CUR, we may make some improvement, indeed in >> gereric_file_llseek(), it also use a spin lock to protect concurrent >> SEEK_CUR access. >> is there really a perf issue that deserve us to make a hack to hurt code >> readability? > Sure, and also, I don't think this is *hack*. Instead, it's an old significant > improvements in VFS generic_file_lseek() from Andi Kleen back to 2011. Maybe OCFS2 > is the only file system without this turn-up as we lack of upstream activity for a > long time. For detail, please refer to: > > commit ef3d0fd27e90f67e35da516dafc1482c82939a60 > Author: Andi Kleen <ak@linux.intel.com> > Date: Thu Sep 15 16:06:48 2011 -0700 > > vfs: do (nearly) lockless generic_file_llseek > > The i_mutex lock use of generic _file_llseek hurts. Independent processes > accessing the same file synchronize over a single lock, even though > they have no need for synchronization at all. > > Under high utilization this can cause llseek to scale very poorly on larger > systems. > > This patch does some rethinking of the llseek locking model: Thanks for point this out for me, I will make a patch for this. Thanks, Junxiao. > > > Thanks, > -Jeff
diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c index 20dfec7..ab3ebf3 100644 --- a/fs/ocfs2/aops.c +++ b/fs/ocfs2/aops.c @@ -2049,7 +2049,7 @@ int ocfs2_write_end_nolock(struct address_space *mapping, out_write_size: pos += copied; - if (pos > inode->i_size) { + if (pos > i_size_read(inode)) { i_size_write(inode, pos); mark_inode_dirty(inode); } diff --git a/fs/ocfs2/extent_map.c b/fs/ocfs2/extent_map.c index 2487116..4bf2b76 100644 --- a/fs/ocfs2/extent_map.c +++ b/fs/ocfs2/extent_map.c @@ -852,20 +852,20 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) down_read(&OCFS2_I(inode)->ip_alloc_sem); - if (*offset >= inode->i_size) { + if (*offset >= i_size_read(inode)) { ret = -ENXIO; goto out_unlock; } if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) { if (whence == SEEK_HOLE) - *offset = inode->i_size; + *offset = i_size_read(inode); goto out_unlock; } clen = 0; cpos = *offset >> cs_bits; - cend = ocfs2_clusters_for_bytes(inode->i_sb, inode->i_size); + cend = ocfs2_clusters_for_bytes(inode->i_sb, i_size_read(inode)); while (cpos < cend && !is_last) { ret = ocfs2_get_clusters_nocache(inode, di_bh, cpos, &hole_size, @@ -904,8 +904,8 @@ int ocfs2_seek_data_hole_offset(struct file *file, loff_t *offset, int whence) extlen = clen; extlen <<= cs_bits; - if ((extoff + extlen) > inode->i_size) - extlen = inode->i_size - extoff; + if ((extoff + extlen) > i_size_read(inode)) + extlen = i_size_read(inode) - extoff; extoff += extlen; if (extoff > *offset) *offset = extoff; diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c index 3ce6a8b..7158710 100644 --- a/fs/ocfs2/file.c +++ b/fs/ocfs2/file.c @@ -2650,7 +2650,7 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) case SEEK_SET: break; case SEEK_END: - offset += inode->i_size; + offset += i_size_read(inode); break; case SEEK_CUR: if (offset == 0) { diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c index 0c60ef2..fa32ce9 100644 --- a/fs/ocfs2/ioctl.c +++ b/fs/ocfs2/ioctl.c @@ -303,7 +303,7 @@ int ocfs2_info_handle_journal_size(struct inode *inode, if (o2info_from_user(oij, req)) goto bail; - oij.ij_journal_size = osb->journal->j_inode->i_size; + oij.ij_journal_size = i_size_read(osb->journal->j_inode); o2info_set_request_filled(&oij.ij_req); diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c index 8eccfab..b5c8c53 100644 --- a/fs/ocfs2/journal.c +++ b/fs/ocfs2/journal.c @@ -801,14 +801,14 @@ int ocfs2_journal_init(struct ocfs2_journal *journal, int *dirty) inode_lock = 1; di = (struct ocfs2_dinode *)bh->b_data; - if (inode->i_size < OCFS2_MIN_JOURNAL_SIZE) { + if (i_size_read(inode) < OCFS2_MIN_JOURNAL_SIZE) { mlog(ML_ERROR, "Journal file size (%lld) is too small!\n", - inode->i_size); + i_size_read(inode)); status = -EINVAL; goto done; } - trace_ocfs2_journal_init(inode->i_size, + trace_ocfs2_journal_init(i_size_read(inode), (unsigned long long)inode->i_blocks, OCFS2_I(inode)->ip_clusters); @@ -1096,7 +1096,7 @@ static int ocfs2_force_read_journal(struct inode *inode) memset(bhs, 0, sizeof(struct buffer_head *) * CONCURRENT_JOURNAL_FILL); - num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, inode->i_size); + num_blocks = ocfs2_blocks_for_bytes(inode->i_sb, i_size_read(inode)); v_blkno = 0; while (v_blkno < num_blocks) { status = ocfs2_extent_map_get_blocks(inode, v_blkno, diff --git a/fs/ocfs2/move_extents.c b/fs/ocfs2/move_extents.c index f1fc172..33472e4 100644 --- a/fs/ocfs2/move_extents.c +++ b/fs/ocfs2/move_extents.c @@ -845,7 +845,7 @@ static int __ocfs2_move_extents_range(struct buffer_head *di_bh, struct ocfs2_move_extents *range = context->range; struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); - if ((inode->i_size == 0) || (range->me_len == 0)) + if ((i_size_read(inode) == 0) || (range->me_len == 0)) return 0; if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c index 332a281..aaa5061 100644 --- a/fs/ocfs2/quota_global.c +++ b/fs/ocfs2/quota_global.c @@ -234,7 +234,7 @@ ssize_t ocfs2_quota_write(struct super_block *sb, int type, len = sb->s_blocksize - OCFS2_QBLK_RESERVED_SPACE - offset; } - if (gqinode->i_size < off + len) { + if (i_size_read(gqinode) < off + len) { loff_t rounded_end = ocfs2_align_bytes_to_blocks(sb, off + len); @@ -778,8 +778,8 @@ static int ocfs2_acquire_dquot(struct dquot *dquot) */ WARN_ON(journal_current_handle()); status = ocfs2_extend_no_holes(gqinode, NULL, - gqinode->i_size + (need_alloc << sb->s_blocksize_bits), - gqinode->i_size); + i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits), + i_size_read(gqinode)); if (status < 0) goto out_dq; } diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c index 27fe7ee..2e4344b 100644 --- a/fs/ocfs2/quota_local.c +++ b/fs/ocfs2/quota_local.c @@ -982,14 +982,14 @@ static struct ocfs2_quota_chunk *ocfs2_local_quota_add_chunk( /* We are protected by dqio_sem so no locking needed */ status = ocfs2_extend_no_holes(lqinode, NULL, - lqinode->i_size + 2 * sb->s_blocksize, - lqinode->i_size); + i_size_read(lqinode) + 2 * sb->s_blocksize, + i_size_read(lqinode)); if (status < 0) { mlog_errno(status); goto out; } status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, - lqinode->i_size + 2 * sb->s_blocksize); + i_size_read(lqinode) + 2 * sb->s_blocksize); if (status < 0) { mlog_errno(status); goto out; @@ -1125,14 +1125,14 @@ static struct ocfs2_quota_chunk *ocfs2_extend_local_quota_file( /* We are protected by dqio_sem so no locking needed */ status = ocfs2_extend_no_holes(lqinode, NULL, - lqinode->i_size + sb->s_blocksize, - lqinode->i_size); + i_size_read(lqinode) + sb->s_blocksize, + i_size_read(lqinode)); if (status < 0) { mlog_errno(status); goto out; } status = ocfs2_simple_size_update(lqinode, oinfo->dqi_lqi_bh, - lqinode->i_size + sb->s_blocksize); + i_size_read(lqinode) + sb->s_blocksize); if (status < 0) { mlog_errno(status); goto out;
Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> --- fs/ocfs2/aops.c | 2 +- fs/ocfs2/extent_map.c | 10 +++++----- fs/ocfs2/file.c | 2 +- fs/ocfs2/ioctl.c | 2 +- fs/ocfs2/journal.c | 8 ++++---- fs/ocfs2/move_extents.c | 2 +- fs/ocfs2/quota_global.c | 6 +++--- fs/ocfs2/quota_local.c | 12 ++++++------ 8 files changed, 22 insertions(+), 22 deletions(-)