Skip to contentSkip to conversation

ocfs2: using i_size_read() to access i_size

PATCH New Junxiao Bi 3 participants 12 messages Jul 15, 2013 – Jul 22, 2013 OCFS2

lore
Share & tools
lore

Junxiao Bi

author
#0 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Signed-off-by Junxiao Bi <[email protected]>
---
 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(-)
fs/ocfs2/aops.c +1 -1
index 20dfec7..ab3ebf3 100644
@@ -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);
 	}
fs/ocfs2/extent_map.c +5 -5
index 2487116..4bf2b76 100644
@@ -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;
fs/ocfs2/file.c +1 -1
index 3ce6a8b..7158710 100644
@@ -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) {
fs/ocfs2/ioctl.c +1 -1
index 0c60ef2..fa32ce9 100644
@@ -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);
fs/ocfs2/journal.c +4 -4
index 8eccfab..b5c8c53 100644
@@ -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,
fs/ocfs2/move_extents.c +1 -1
index f1fc172..33472e4 100644
@@ -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)
fs/ocfs2/quota_global.c +3 -3
index 332a281..aaa5061 100644
@@ -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;
 	}
fs/ocfs2/quota_local.c +6 -6
index 27fe7ee..2e4344b 100644
@@ -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;
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

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

Quoted 199 lines · Junxiao Bi
Signed-off-by Junxiao Bi <[email protected]>
---
 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(-)
fs/ocfs2/aops.c +1 -1
index 20dfec7..ab3ebf3 100644
@@ -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);
 	}
fs/ocfs2/extent_map.c +5 -5
index 2487116..4bf2b76 100644
@@ -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;
fs/ocfs2/file.c +1 -1
index 3ce6a8b..7158710 100644
@@ -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) {
fs/ocfs2/ioctl.c +1 -1
index 0c60ef2..fa32ce9 100644
@@ -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);
fs/ocfs2/journal.c +4 -4
index 8eccfab..b5c8c53 100644
@@ -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,
fs/ocfs2/move_extents.c +1 -1
index f1fc172..33472e4 100644
@@ -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)
fs/ocfs2/quota_global.c +3 -3
index 332a281..aaa5061 100644
@@ -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;
 	}
fs/ocfs2/quota_local.c +6 -6
index 27fe7ee..2e4344b 100644
@@ -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;
#2 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Quoted 4 lines · Tao Ma

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.

Quoted 61 lines

Thanks,
Tao

Quoted 57 lines · Junxiao Bi
Signed-off-by Junxiao Bi <[email protected]>
---
 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(-)
fs/ocfs2/aops.c +1 -1
index 20dfec7..ab3ebf3 100644
@@ -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);
 	}
fs/ocfs2/extent_map.c +3 -3
index 2487116..4bf2b76 100644
@@ -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;
Quoted 12 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 12 lines
fs/ocfs2/extent_map.c +2 -2
-		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;
fs/ocfs2/file.c +0 -0
index 3ce6a8b..7158710 100644
@@ -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.

Quoted 5 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2653

Quoted 5 lines
fs/ocfs2/file.c +1 -1
 	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 <[email protected]>
Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

Would you like to take care of this?

Thanks,
-Jeff

Quoted 134 lines
Quoted 118 lines
fs/ocfs2/file.c +0 -0
 		break;
 	case SEEK_CUR:
 		if (offset == 0) {
fs/ocfs2/ioctl.c +1 -1
index 0c60ef2..fa32ce9 100644
@@ -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);
fs/ocfs2/journal.c +4 -4
index 8eccfab..b5c8c53 100644
@@ -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,
fs/ocfs2/move_extents.c +1 -1
index f1fc172..33472e4 100644
@@ -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)
fs/ocfs2/quota_global.c +3 -3
index 332a281..aaa5061 100644
@@ -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;
 	}
fs/ocfs2/quota_local.c +5 -5
index 27fe7ee..2e4344b 100644
@@ -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));

fs/ocfs2/quota_local.c static struct ocfs2_quota_chunk *ocfs2_extend_local_quota_file( · line 1138

Quoted 10 lines
fs/ocfs2/quota_local.c +1 -1
 	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 at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

author
#3 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Hi Jeff,

Quoted 69 lines · Jeff Liu
Quoted 4 lines · Tao Ma

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.

Quoted 60 lines

Thanks,
Tao

Quoted 57 lines · Junxiao Bi
Signed-off-by Junxiao Bi <[email protected]>
---
 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(-)
fs/ocfs2/aops.c +1 -1
index 20dfec7..ab3ebf3 100644
@@ -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);
 	}
fs/ocfs2/extent_map.c +3 -3
index 2487116..4bf2b76 100644
@@ -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;
Quoted 16 lines
Quoted 4 lines
Quoted 4 lines
fs/ocfs2/extent_map.c +2 -2
-		if ((extoff + extlen) > inode->i_size)
-			extlen = inode->i_size - extoff;
+		if ((extoff + extlen) > i_size_read(inode))
+			extlen = i_size_read(inode) - extoff;
Quoted 8 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 8 lines
fs/ocfs2/extent_map.c +0 -0
 		extoff += extlen;
 		if (extoff > *offset)
 			*offset = extoff;
fs/ocfs2/file.c +0 -0
index 3ce6a8b..7158710 100644
@@ -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().

Quoted 8 lines
Quoted 6 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2653

Quoted 6 lines
fs/ocfs2/file.c +1 -1
 	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.

Quoted 143 lines

Canquan already fixed this in anther patch:
From: Jensen <[email protected]>
Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

Would you like to take care of this?

Thanks,
-Jeff

Quoted 132 lines
Quoted 117 lines
fs/ocfs2/file.c +0 -0
 		break;
 	case SEEK_CUR:
 		if (offset == 0) {
fs/ocfs2/ioctl.c +1 -1
index 0c60ef2..fa32ce9 100644
@@ -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);
fs/ocfs2/journal.c +4 -4
index 8eccfab..b5c8c53 100644
@@ -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,
fs/ocfs2/move_extents.c +1 -1
index f1fc172..33472e4 100644
@@ -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)
fs/ocfs2/quota_global.c +3 -3
index 332a281..aaa5061 100644
@@ -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;
 	}
fs/ocfs2/quota_local.c +5 -5
index 27fe7ee..2e4344b 100644
@@ -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));

fs/ocfs2/quota_local.c static struct ocfs2_quota_chunk *ocfs2_extend_local_quota_file( · line 1138

Quoted 10 lines
fs/ocfs2/quota_local.c +1 -1
 	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 at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

author
#4 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

In reply to Jeff Liu

Hi Jeff,

Quoted 9 lines · Jeff Liu
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +0 -0
Quoted 4 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 4 lines

index 3ce6a8b..7158710 100644

fs/ocfs2/file.c +0 -0
@@ -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.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
fs/ocfs2/file.c +1 -1
 	case SEEK_SET:
 		break;
 	case SEEK_END:
-		offset += inode->i_size;
+		offset += i_size_read(inode);
#5 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Hi Junxiao,

Quoted 12 lines · Junxiao Bi

Hi Jeff,

Quoted 9 lines · Jeff Liu
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +0 -0
Quoted 4 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 4 lines

index 3ce6a8b..7158710 100644

fs/ocfs2/file.c +0 -0
@@ -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.

Quoted 4 lines

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

Quoted 10 lines

Thanks,
Junxiao.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
fs/ocfs2/file.c +1 -1
 	case SEEK_SET:
 		break;
 	case SEEK_END:
-		offset += inode->i_size;
+		offset += i_size_read(inode);
author
#6 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Hi Jeff,

Quoted 20 lines · Jeff Liu

Hi Junxiao,

Quoted 12 lines · Junxiao Bi

Hi Jeff,

Quoted 9 lines · Jeff Liu
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +0 -0
Quoted 4 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 4 lines

index 3ce6a8b..7158710 100644

fs/ocfs2/file.c +0 -0
@@ -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.

Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 12 lines

Thanks,
-Jeff

Quoted 7 lines

Thanks,
Junxiao.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +1 -1
 	case SEEK_SET:
 		break;
 	case SEEK_END:
-		offset += inode->i_size;
+		offset += i_size_read(inode);
#7 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Quoted 23 lines · Junxiao Bi

Hi Jeff,

Quoted 20 lines · Jeff Liu

Hi Junxiao,

Quoted 16 lines · Junxiao Bi

Hi Jeff,

Quoted 9 lines · Jeff Liu
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +0 -0
Quoted 4 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 4 lines

index 3ce6a8b..7158710 100644

fs/ocfs2/file.c +0 -0
@@ -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().

Quoted 6 lines
Quoted 5 lines

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

Quoted 16 lines

Thanks,
Junxiao.

Quoted 12 lines

Thanks,
-Jeff

Quoted 7 lines

Thanks,
Junxiao.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +1 -1
 	case SEEK_SET:
 		break;
 	case SEEK_END:
-		offset += inode->i_size;
+		offset += i_size_read(inode);
author
#8 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Hi Jeff,

Quoted 29 lines · Jeff Liu
Quoted 27 lines · Junxiao Bi

Hi Jeff,

Quoted 24 lines · Jeff Liu

Hi Junxiao,

Quoted 12 lines · Junxiao Bi

Hi Jeff,

Quoted 9 lines · Jeff Liu
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +0 -0
Quoted 4 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 4 lines

index 3ce6a8b..7158710 100644

fs/ocfs2/file.c +0 -0
@@ -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.

Quoted 4 lines

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.

Quoted 5 lines
Quoted 2 lines
Quoted 1 line

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.

Quoted 17 lines

Thanks,
-Jeff

Quoted 12 lines

Thanks,
Junxiao.

Quoted 10 lines

Thanks,
-Jeff

Quoted 7 lines

Thanks,
Junxiao.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +1 -1
 	case SEEK_SET:
 		break;
 	case SEEK_END:
-		offset += inode->i_size;
+		offset += i_size_read(inode);
#9 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Quoted 37 lines · Junxiao Bi

Hi Jeff,

Quoted 29 lines · Jeff Liu
Quoted 27 lines · Junxiao Bi

Hi Jeff,

Quoted 24 lines · Jeff Liu

Hi Junxiao,

Quoted 12 lines · Junxiao Bi

Hi Jeff,

Quoted 9 lines · Jeff Liu
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +0 -0
Quoted 4 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 4 lines

index 3ce6a8b..7158710 100644

fs/ocfs2/file.c +0 -0
@@ -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.

Quoted 4 lines

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.

Quoted 5 lines
Quoted 2 lines
Quoted 1 line

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(). :)

Quoted 6 lines

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 <[email protected]>
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

author
#10 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

Hi Jeff,

Quoted 56 lines · Jeff Liu
Quoted 37 lines · Junxiao Bi

Hi Jeff,

Quoted 29 lines · Jeff Liu
Quoted 27 lines · Junxiao Bi

Hi Jeff,

Quoted 24 lines · Jeff Liu

Hi Junxiao,

Quoted 12 lines · Junxiao Bi

Hi Jeff,

Quoted 9 lines · Jeff Liu
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/file.c +0 -0
Quoted 4 lines

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)

Quoted 4 lines

index 3ce6a8b..7158710 100644

fs/ocfs2/file.c +0 -0
@@ -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.

Quoted 4 lines

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.

Quoted 5 lines
Quoted 2 lines
Quoted 1 line

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(). :)

Quoted 6 lines

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 <[email protected]>
Date:   Thu Sep 15 16:06:48 2011 -0700

vfs: do (nearly) lockless generic_file_llseek

Quoted 8 lines

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.

Quoted 4 lines

Thanks,
-Jeff

author
#11 lore raw

PATCH 1/1 ocfs2: using i_size_read() to access i_size

In reply to Tao Ma

Hi Tao,

Quoted 4 lines · Tao Ma

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.

Quoted 203 lines

Thanks,
Tao

Quoted 199 lines · Junxiao Bi
Signed-off-by Junxiao Bi <[email protected]>
---
 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(-)
fs/ocfs2/aops.c +1 -1
index 20dfec7..ab3ebf3 100644
@@ -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);
 	}
fs/ocfs2/extent_map.c +5 -5
index 2487116..4bf2b76 100644
@@ -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;
fs/ocfs2/file.c +1 -1
index 3ce6a8b..7158710 100644
@@ -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) {
fs/ocfs2/ioctl.c +1 -1
index 0c60ef2..fa32ce9 100644
@@ -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);
fs/ocfs2/journal.c +4 -4
index 8eccfab..b5c8c53 100644
@@ -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,
fs/ocfs2/move_extents.c +1 -1
index f1fc172..33472e4 100644
@@ -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)
fs/ocfs2/quota_global.c +3 -3
index 332a281..aaa5061 100644
@@ -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;
 	}
fs/ocfs2/quota_local.c +6 -6
index 27fe7ee..2e4344b 100644
@@ -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;